| Message ID | 9d2ec624a640c421e97334deff537a2e899356c7.1731088789.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/arm-smmu: Minor probe_device related improvements | expand |
On Fri, Nov 08, 2024 at 06:30:15PM +0000, Robin Murphy wrote: > Relying on the driver list was a cute idea for minimising the scope of > our SMMU device lookups, however it turns out to have a subtle flaw. The > SMMU device only gets added to that list after arm_smmu_device_probe() > returns success, so there's actually no way the iommu_device_register() > call from there could ever work as intended, even if it wasn't already > hampered by the fwspec setup not happening early enough. > > Switch both arm_smmu_get_by_fwnode() implementations to use a platform > bus lookup instead, which *will* reliably work. Also make sure that we > don't register SMMUv2 instances until we've fully initialised them, to > avoid similar consequences of the lookup now finding a device with no > drvdata. Moving that code is also a perfect excuse to give it a little > dev_err_probe() tidyup as well. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > This should in theory replace 229e6ee43d2a ("iommu/arm-smmu: Defer > probe of clients after smmu device bound"), or at least allow it to be > reverted going forward. Let's revert that as part of this series, then? > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 +-- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 31 ++++++++++----------- > 2 files changed, 16 insertions(+), 19 deletions(-) [...] > @@ -2255,6 +2240,18 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > arm_smmu_device_reset(smmu); > arm_smmu_test_smr_masks(smmu); > > + err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL, > + "smmu.%pa", &smmu->ioaddr); > + if (err) > + return dev_err_probe(dev, err, "Failed to register iommu in sysfs\n"); > + > + err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, > + using_legacy_binding ? NULL : dev); > + if (err) { > + iommu_device_sysfs_remove(&smmu->iommu); > + return dev_err_probe(dev, err, "Failed to register iommu\n"); Why is dev_err_probe() better here? I've not seen it before, so I was trying to figure out what it does. It mainly looks to be handling the EPROBE_DEFER case, but after your other changes to use bus_find_device_by_fwnode() I'm wondering whether we actually hit that (particularly in iommu_device_sysfs_add())? I'm not saying there's a problem, just that I'm not sure why dev_err_probe() is tidier than dev_err(). Will
On 12/11/2024 3:43 pm, Will Deacon wrote: > On Fri, Nov 08, 2024 at 06:30:15PM +0000, Robin Murphy wrote: >> Relying on the driver list was a cute idea for minimising the scope of >> our SMMU device lookups, however it turns out to have a subtle flaw. The >> SMMU device only gets added to that list after arm_smmu_device_probe() >> returns success, so there's actually no way the iommu_device_register() >> call from there could ever work as intended, even if it wasn't already >> hampered by the fwspec setup not happening early enough. >> >> Switch both arm_smmu_get_by_fwnode() implementations to use a platform >> bus lookup instead, which *will* reliably work. Also make sure that we >> don't register SMMUv2 instances until we've fully initialised them, to >> avoid similar consequences of the lookup now finding a device with no >> drvdata. Moving that code is also a perfect excuse to give it a little >> dev_err_probe() tidyup as well. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> >> This should in theory replace 229e6ee43d2a ("iommu/arm-smmu: Defer >> probe of clients after smmu device bound"), or at least allow it to be >> reverted going forward. > > Let's revert that as part of this series, then? Sure, at time of posting I wasn't entirely sure where we'd stand on replace vs. revert (TBH I'd lost track and didn't even realise we're at rc7 already...) >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 +-- >> drivers/iommu/arm/arm-smmu/arm-smmu.c | 31 ++++++++++----------- >> 2 files changed, 16 insertions(+), 19 deletions(-) > > [...] > >> @@ -2255,6 +2240,18 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >> arm_smmu_device_reset(smmu); >> arm_smmu_test_smr_masks(smmu); >> >> + err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL, >> + "smmu.%pa", &smmu->ioaddr); >> + if (err) >> + return dev_err_probe(dev, err, "Failed to register iommu in sysfs\n"); >> + >> + err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, >> + using_legacy_binding ? NULL : dev); >> + if (err) { >> + iommu_device_sysfs_remove(&smmu->iommu); >> + return dev_err_probe(dev, err, "Failed to register iommu\n"); > > Why is dev_err_probe() better here? I've not seen it before, so I was > trying to figure out what it does. It mainly looks to be handling > the EPROBE_DEFER case, but after your other changes to use > bus_find_device_by_fwnode() I'm wondering whether we actually hit that > (particularly in iommu_device_sysfs_add())? > > I'm not saying there's a problem, just that I'm not sure why > dev_err_probe() is tidier than dev_err(). Just that it's more concise and convenient, and as such becoming an established pattern for returning errors from driver probe routines, even in cases which don't defer. Cheers, Robin.
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 737c5b882355..b7dcb1494aa4 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3171,8 +3171,8 @@ static struct platform_driver arm_smmu_driver; static struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode) { - struct device *dev = driver_find_device_by_fwnode(&arm_smmu_driver.driver, - fwnode); + struct device *dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode); + put_device(dev); return dev ? dev_get_drvdata(dev) : NULL; } diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index 8321962b3714..aba315aa6848 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1411,8 +1411,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) static struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode) { - struct device *dev = driver_find_device_by_fwnode(&arm_smmu_driver.driver, - fwnode); + struct device *dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode); + put_device(dev); return dev ? dev_get_drvdata(dev) : NULL; } @@ -2232,21 +2232,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev) i, irq); } - err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL, - "smmu.%pa", &smmu->ioaddr); - if (err) { - dev_err(dev, "Failed to register iommu in sysfs\n"); - return err; - } - - err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, - using_legacy_binding ? NULL : dev); - if (err) { - dev_err(dev, "Failed to register iommu\n"); - iommu_device_sysfs_remove(&smmu->iommu); - return err; - } - platform_set_drvdata(pdev, smmu); /* Check for RMRs and install bypass SMRs if any */ @@ -2255,6 +2240,18 @@ static int arm_smmu_device_probe(struct platform_device *pdev) arm_smmu_device_reset(smmu); arm_smmu_test_smr_masks(smmu); + err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL, + "smmu.%pa", &smmu->ioaddr); + if (err) + return dev_err_probe(dev, err, "Failed to register iommu in sysfs\n"); + + err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, + using_legacy_binding ? NULL : dev); + if (err) { + iommu_device_sysfs_remove(&smmu->iommu); + return dev_err_probe(dev, err, "Failed to register iommu\n"); + } + /* * We want to avoid touching dev->power.lock in fastpaths unless * it's really going to do something useful - pm_runtime_enabled()
Relying on the driver list was a cute idea for minimising the scope of our SMMU device lookups, however it turns out to have a subtle flaw. The SMMU device only gets added to that list after arm_smmu_device_probe() returns success, so there's actually no way the iommu_device_register() call from there could ever work as intended, even if it wasn't already hampered by the fwspec setup not happening early enough. Switch both arm_smmu_get_by_fwnode() implementations to use a platform bus lookup instead, which *will* reliably work. Also make sure that we don't register SMMUv2 instances until we've fully initialised them, to avoid similar consequences of the lookup now finding a device with no drvdata. Moving that code is also a perfect excuse to give it a little dev_err_probe() tidyup as well. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- This should in theory replace 229e6ee43d2a ("iommu/arm-smmu: Defer probe of clients after smmu device bound"), or at least allow it to be reverted going forward. drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 +-- drivers/iommu/arm/arm-smmu/arm-smmu.c | 31 ++++++++++----------- 2 files changed, 16 insertions(+), 19 deletions(-)