iommu/arm-smmu: Minor probe_device related improvements
Subsystems (2)
Robin Murphy
reviewerv2 cover iommu/arm-smmu: Minor probe_device related improvements
v1: https://lore.kernel.org/linux-iommu/[email protected]
Hi all,
Here's what I probably should have sent instead of rushing v1 out earlier, now rebased and polished :)
Cheers,
Robin.
Robin Murphy (4):
iommu/arm-smmu: Make instance lookup robust iommu/arm-smmu: Retire probe deferral workaround iommu/arm-smmu-v3: Clean up more on probe failure
iommu: Manage driver probe deferral better
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 +++++++---- drivers/iommu/arm/arm-smmu/arm-smmu.c | 42 +++++++-------------- drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 2 - 4 files changed, 29 insertions(+), 38 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
reviewerPATCH v2 1/4 iommu/arm-smmu: Make instance lookup robust
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 the error returns is also a perfect excuse to streamline them with dev_err_probe() in the process.
---
v2: Tweak commit message
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(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -2
@@ -3237,8 +3237,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 ; }
drivers/iommu/arm/arm-smmu/arm-smmu.c +14 -17
@@ -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 ; }
@@ -2227,21 +2227,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 */
@@ -2250,6 +2235,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()
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
reviewerPATCH v2 2/4 iommu/arm-smmu: Retire probe deferral workaround
In reply to Robin MurphyThis reverts commit 229e6ee43d2a160a1592b83aad620d6027084aad.
Now that the fundamental ordering issue between arm_smmu_get_by_fwnode() and iommu_device_register() is resolved, the race condition for client probe no longer exists either, so retire the specific workaround.
---
v2: New
drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ----------- 1 file changed, 11 deletions(-)
drivers/iommu/arm/arm-smmu/arm-smmu.c +0 -11
@@ -1437,17 +1437,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) goto out_free; }else { smmu =arm_smmu_get_by_fwnode (fwspec->iommu_fwnode); - -/* -* Defer probe if the relevant SMMU instance hasn't finished -* probing yet. This is a fragile hack and we'd ideally -* avoid this race in the core code. Until that's ironed -* out, however, this is the most pragmatic option on the -* table. -*/ -if (!smmu) -return ERR_PTR (dev_err_probe (dev, -EPROBE_DEFER , -"smmu dev has not bound yet\n" )); } ret = -EINVAL ;
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
reviewerPATCH v2 3/4 iommu/arm-smmu-v3: Clean up more on probe failure
In reply to Robin Murphykmemleak noticed that the iopf queue allocated deep down within arm_smmu_init_structures() can be leaked by a subsequent error return from arm_smmu_device_probe(). Furthermore, after arm_smmu_device_reset() we will also leave the SMMU enabled with an empty Stream Table, silently blocking all DMA. This proves rather annoying for debugging said probe failure, so let's handle it a bit better by putting the SMMU back into (more or less) the same state as if it hadn't probed at all.
---
v2: Squash disable fix, get labels right
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +12 -5
@@ -4663,7 +4663,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)/* Initialise in-memory data structures */ ret =arm_smmu_init_structures (smmu);if (ret) -return ret; + goto err_free_iopf;/* Record our private device structure */ platform_set_drvdata (pdev, smmu);
@@ -4674,22 +4674,29 @@ static int arm_smmu_device_probe(struct platform_device *pdev)/* Reset the device */ ret =arm_smmu_device_reset (smmu);if (ret) -return ret; + goto err_disable;/* And we're up. Go go go! */ ret =iommu_device_sysfs_add (&smmu->iommu, dev,NULL ,"smmu3.%pa" , &ioaddr);if (ret) -return ret; + goto err_disable; ret =iommu_device_register (&smmu->iommu, &arm_smmu_ops, dev);if (ret) {dev_err (dev,"Failed to register iommu\n" ); -iommu_device_sysfs_remove (&smmu->iommu); -return ret; + goto err_free_sysfs; }return 0 ; + +err_free_sysfs: +iommu_device_sysfs_remove (&smmu->iommu); +err_disable: +arm_smmu_device_disable (smmu); +err_free_iopf: +iopf_queue_free (smmu->evtq.iopf); +return ret; }static void arm_smmu_device_remove (struct platform_device *pdev)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
reviewerPATCH v2 4/4 iommu: Manage driver probe deferral better
In reply to Robin MurphySince iommu_fwspec_init() absorbed the basic driver probe deferral check to wait for an IOMMU to register, we may as well handle the probe deferral timeout there as well. The current inconsistency of callers results in client devices deferring forever on an arm64 ACPI system where an SMMU has failed its own driver probe.
---
v2: No change
drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)
drivers/iommu/iommu.c +1 -1
@@ -2819,7 +2819,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)struct iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);if (!ops) -return -EPROBE_DEFER ; +return driver_deferred_probe_check_state (dev);if (fwspec)return ops ==iommu_fwspec_ops (fwspec) ?0 : -EINVAL ;
drivers/iommu/of_iommu.c +0 -2
@@ -29,8 +29,6 @@ static int of_iommu_xlate(struct device *dev,return -ENODEV ; ret =iommu_fwspec_init (dev,of_fwnode_handle (iommu_spec->np)); -if (ret == -EPROBE_DEFER ) -return driver_deferred_probe_check_state (dev);if (ret)return ret;
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Quoted 8 lines · Robin Murphy
v1: https://lore.kernel.org/linux-iommu/[email protected]
Hi all,
Here's what I probably should have sent instead of rushing v1 out earlier, now rebased and polished :)
[...]
Applied to will (for-joerg/arm-smmu/updates), thanks!
[1/4] iommu/arm-smmu: Make instance lookup robust
https://git.kernel.org/will/c/7d835134d4e1 [2/4] iommu/arm-smmu: Retire probe deferral workaround https://git.kernel.org/will/c/97cb1fa02726 [3/4] iommu/arm-smmu-v3: Clean up more on probe failure https://git.kernel.org/will/c/fcbd62156742 [4/4] iommu: Manage driver probe deferral better https://git.kernel.org/will/c/46b3df8eb9bd
Cheers,
Signature · 5 lines
Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev