Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()
Hi Robin, On 01/17/2018 08:18 PM, Robin Murphy wrote: @@ -91,7 +92,6 @@ struct rk_iommu { void __iomem **bases; int num_mmu; int *irq; Nit: irq seems to be redundant now as well. oops, will fix it. -int num_irq; bool reset_disabled; struct iommu_device iommu; struct list_head node; /* entry in rk_iommu_domain.iommus */ @@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, iommu->domain = domain; -for (i = 0; i < iommu->num_irq; i++) { -ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq, - IRQF_SHARED, dev_name(dev), iommu); -if (ret) -return ret; -} - for (i = 0; i < iommu->num_mmu; i++) { rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, rk_domain->dt_dma); @@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, } rk_iommu_disable_stall(iommu); -for (i = 0; i < iommu->num_irq; i++) -devm_free_irq(iommu->dev, iommu->irq[i], iommu); - iommu->domain = NULL; dev_dbg(dev, "Detached from iommu domain\n"); @@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev) struct rk_iommu *iommu; struct resource *res; int num_res = pdev->num_resources; -int err, i; +int err, i, irq, num_irq; iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL); if (!iommu) @@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev) if (iommu->num_mmu == 0) return PTR_ERR(iommu->bases[0]); -iommu->num_irq = platform_irq_count(pdev); -if (iommu->num_irq < 0) -return iommu->num_irq; -if (iommu->num_irq == 0) -return -ENXIO; - -iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq), - GFP_KERNEL); -if (!iommu->irq) -return -ENOMEM; - -for (i = 0; i < iommu->num_irq; i++) { -iommu->irq[i] = platform_get_irq(pdev, i); -if (iommu->irq[i] < 0) { -dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]); +num_irq = of_irq_count(dev->of_node); To follow up on the other reply, I'm not sure you really need to count the IRQs beforehand at all - you're going to be looping through platform_get_irq() and handling errors anyway, so you may as well just start at 0 and keep going until -ENOENT (or use platform_get_resource() to double-check whether an index should be valid, as we do in arm_smmu). ok, will do that. Otherwise, it looks like everything that the IRQ handler needs in the iommu struct (dev, num_mmu and bases) is already initialised by this point, so we should be OK with respect to races. ok. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()
On 16/01/18 13:25, Jeffy Chen wrote: Suggested-by: Robin Murphy Signed-off-by: Jeffy Chen --- Changes in v2: None drivers/iommu/rockchip-iommu.c | 38 +++--- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 9d991c2d8767..4a12d4746095 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -91,7 +92,6 @@ struct rk_iommu { void __iomem **bases; int num_mmu; int *irq; Nit: irq seems to be redundant now as well. - int num_irq; bool reset_disabled; struct iommu_device iommu; struct list_head node; /* entry in rk_iommu_domain.iommus */ @@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, iommu->domain = domain; - for (i = 0; i < iommu->num_irq; i++) { - ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq, - IRQF_SHARED, dev_name(dev), iommu); - if (ret) - return ret; - } - for (i = 0; i < iommu->num_mmu; i++) { rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, rk_domain->dt_dma); @@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, } rk_iommu_disable_stall(iommu); - for (i = 0; i < iommu->num_irq; i++) - devm_free_irq(iommu->dev, iommu->irq[i], iommu); - iommu->domain = NULL; dev_dbg(dev, "Detached from iommu domain\n"); @@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev) struct rk_iommu *iommu; struct resource *res; int num_res = pdev->num_resources; - int err, i; + int err, i, irq, num_irq; iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL); if (!iommu) @@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev) if (iommu->num_mmu == 0) return PTR_ERR(iommu->bases[0]); - iommu->num_irq = platform_irq_count(pdev); - if (iommu->num_irq < 0) - return iommu->num_irq; - if (iommu->num_irq == 0) - return -ENXIO; - - iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq), - GFP_KERNEL); - if (!iommu->irq) - return -ENOMEM; - - for (i = 0; i < iommu->num_irq; i++) { - iommu->irq[i] = platform_get_irq(pdev, i); - if (iommu->irq[i] < 0) { - dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]); + num_irq = of_irq_count(dev->of_node); To follow up on the other reply, I'm not sure you really need to count the IRQs beforehand at all - you're going to be looping through platform_get_irq() and handling errors anyway, so you may as well just start at 0 and keep going until -ENOENT (or use platform_get_resource() to double-check whether an index should be valid, as we do in arm_smmu). Otherwise, it looks like everything that the IRQ handler needs in the iommu struct (dev, num_mmu and bases) is already initialised by this point, so we should be OK with respect to races. Robin. + for (i = 0; i < num_irq; i++) { + irq = platform_get_irq(pdev, i); + if (irq < 0) { + dev_err(dev, "Failed to get IRQ, %d\n", irq); return -ENXIO; } + err = devm_request_irq(iommu->dev, irq, rk_iommu_irq, + IRQF_SHARED, dev_name(dev), iommu); + if (err) + return err; } iommu->reset_disabled = device_property_read_bool(dev, ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()
Hi Tomasz, On 01/17/2018 03:16 PM, Tomasz Figa wrote: >> >>This lacks consistency. of_irq_count() is used for counting, but >>platform_get_irq() is used for getting. Either platform_ or of_ API >>should be used for both and I'd lean for platform_, since it's more >>general. > >hmmm, right, i was thinking of removing num_irq, and do something like: >while (nr++) { > err = platform_get_irq(dev, nr); > if (err == -EPROBE_DEFER) > break; > if (err < 0) > return err; > >} > >but forgot to do that.. Was there anything wrong with platform_irq_count() used by existing code? just thought the platform_irq_count might ignore some errors(except for EPROBE_DEFER): int platform_irq_count(struct platform_device *dev) { int ret, nr = 0; while ((ret = platform_get_irq(dev, nr)) >= 0) nr++; if (ret == -EPROBE_DEFER) return ret; return nr; } but guess that would not matter.. > >> >>>+ if (irq < 0) { >>>+ dev_err(dev, "Failed to get IRQ, %d\n", irq); >>> return -ENXIO; >>> } >>>+ err = devm_request_irq(iommu->dev, irq, rk_iommu_irq, >>>+ IRQF_SHARED, dev_name(dev), >>>iommu); >>>+ if (err) >>>+ return err; >>> } >> >> >>Looks like there is some more initialization below. Is the driver okay >>with the IRQ being potentially fired right here? (Shared IRQ handlers >>might be run at request_irq() time for testing.) >> >right, forget about that. maybe we can check iommu->domain not NULL in >rk_iommu_irq() Maybe we could just move IRQ requesting to the end of probe? ok, that should work too. and maybe we should also check power status in the irq handler? if it get fired after powered off... Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()
Hi Tomasz, Thanks for your reply. On 01/17/2018 12:21 PM, Tomasz Figa wrote: Hi Jeffy, Thanks for the patch. Please see my comments inline. On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen wrote: Please add patch description. ok, will do. Suggested-by: Robin Murphy Signed-off-by: Jeffy Chen --- [snip] - for (i = 0; i < iommu->num_irq; i++) { - iommu->irq[i] = platform_get_irq(pdev, i); - if (iommu->irq[i] < 0) { - dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]); + num_irq = of_irq_count(dev->of_node); + for (i = 0; i < num_irq; i++) { + irq = platform_get_irq(pdev, i); This lacks consistency. of_irq_count() is used for counting, but platform_get_irq() is used for getting. Either platform_ or of_ API should be used for both and I'd lean for platform_, since it's more general. hmmm, right, i was thinking of removing num_irq, and do something like: while (nr++) { err = platform_get_irq(dev, nr); if (err == -EPROBE_DEFER) break; if (err < 0) return err; } but forgot to do that.. + if (irq < 0) { + dev_err(dev, "Failed to get IRQ, %d\n", irq); return -ENXIO; } + err = devm_request_irq(iommu->dev, irq, rk_iommu_irq, + IRQF_SHARED, dev_name(dev), iommu); + if (err) + return err; } Looks like there is some more initialization below. Is the driver okay with the IRQ being potentially fired right here? (Shared IRQ handlers might be run at request_irq() time for testing.) right, forget about that. maybe we can check iommu->domain not NULL in rk_iommu_irq() iommu->reset_disabled = device_property_read_bool(dev, ^^ Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()
Hi Jeffy, Thanks for the patch. Please see my comments inline. On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen wrote: Please add patch description. > Suggested-by: Robin Murphy > Signed-off-by: Jeffy Chen > --- [snip] > - for (i = 0; i < iommu->num_irq; i++) { > - iommu->irq[i] = platform_get_irq(pdev, i); > - if (iommu->irq[i] < 0) { > - dev_err(dev, "Failed to get IRQ, %d\n", > iommu->irq[i]); > + num_irq = of_irq_count(dev->of_node); > + for (i = 0; i < num_irq; i++) { > + irq = platform_get_irq(pdev, i); This lacks consistency. of_irq_count() is used for counting, but platform_get_irq() is used for getting. Either platform_ or of_ API should be used for both and I'd lean for platform_, since it's more general. > + if (irq < 0) { > + dev_err(dev, "Failed to get IRQ, %d\n", irq); > return -ENXIO; > } > + err = devm_request_irq(iommu->dev, irq, rk_iommu_irq, > + IRQF_SHARED, dev_name(dev), iommu); > + if (err) > + return err; > } Looks like there is some more initialization below. Is the driver okay with the IRQ being potentially fired right here? (Shared IRQ handlers might be run at request_irq() time for testing.) > > iommu->reset_disabled = device_property_read_bool(dev, ^^ Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()
Suggested-by: Robin Murphy Signed-off-by: Jeffy Chen --- Changes in v2: None drivers/iommu/rockchip-iommu.c | 38 +++--- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 9d991c2d8767..4a12d4746095 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -91,7 +92,6 @@ struct rk_iommu { void __iomem **bases; int num_mmu; int *irq; - int num_irq; bool reset_disabled; struct iommu_device iommu; struct list_head node; /* entry in rk_iommu_domain.iommus */ @@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, iommu->domain = domain; - for (i = 0; i < iommu->num_irq; i++) { - ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq, - IRQF_SHARED, dev_name(dev), iommu); - if (ret) - return ret; - } - for (i = 0; i < iommu->num_mmu; i++) { rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, rk_domain->dt_dma); @@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, } rk_iommu_disable_stall(iommu); - for (i = 0; i < iommu->num_irq; i++) - devm_free_irq(iommu->dev, iommu->irq[i], iommu); - iommu->domain = NULL; dev_dbg(dev, "Detached from iommu domain\n"); @@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev) struct rk_iommu *iommu; struct resource *res; int num_res = pdev->num_resources; - int err, i; + int err, i, irq, num_irq; iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL); if (!iommu) @@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev) if (iommu->num_mmu == 0) return PTR_ERR(iommu->bases[0]); - iommu->num_irq = platform_irq_count(pdev); - if (iommu->num_irq < 0) - return iommu->num_irq; - if (iommu->num_irq == 0) - return -ENXIO; - - iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq), - GFP_KERNEL); - if (!iommu->irq) - return -ENOMEM; - - for (i = 0; i < iommu->num_irq; i++) { - iommu->irq[i] = platform_get_irq(pdev, i); - if (iommu->irq[i] < 0) { - dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]); + num_irq = of_irq_count(dev->of_node); + for (i = 0; i < num_irq; i++) { + irq = platform_get_irq(pdev, i); + if (irq < 0) { + dev_err(dev, "Failed to get IRQ, %d\n", irq); return -ENXIO; } + err = devm_request_irq(iommu->dev, irq, rk_iommu_irq, + IRQF_SHARED, dev_name(dev), iommu); + if (err) + return err; } iommu->reset_disabled = device_property_read_bool(dev, -- 2.11.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu