| Message ID | 20180116132540.18939-2-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeffy, Thanks for the patch. Please see my comments inline. On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: Please add patch description. > Suggested-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- [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
On Wed, Jan 17, 2018 at 4:08 PM, JeffyChen <jeffy.chen@rock-chips.com> wrote: > 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 <jeffy.chen@rock-chips.com> >> wrote: >> >> Please add patch description. > > > ok, will do. >> >> >>> Suggested-by: Robin Murphy <robin.murphy@arm.com> >>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>> --- >> >> [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.. Was there anything wrong with platform_irq_count() used by existing code? > >> >>> + 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? Best regards, Tomasz
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 > > >
On 16/01/18 13:25, Jeffy Chen wrote: > Suggested-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > 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 <linux/mm.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > @@ -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, >
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.
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 <linux/mm.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/slab.h> @@ -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,
Suggested-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v2: None drivers/iommu/rockchip-iommu.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-)