iommu/arm-smmu: Minor probe_device related improvements
Subsystems (2)
Robin Murphy
reviewercover iommu/arm-smmu: Minor probe_device related improvements
Hi all,
Just a few tweaks to the SMMU drivers and core code which have fallen out of some hacking on the __iommu_probe_device() path.
Cheers,
Robin.
Robin Murphy (3):
iommu/arm-smmu: Make instance lookup robust iommu/arm-smmu-v3: Clean up iopf queue on probe failure
iommu: Manage driver probe deferral better
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 19 ++++++++----- drivers/iommu/arm/arm-smmu/arm-smmu.c | 31 ++++++++++----------- drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 2 -- 4 files changed, 27 insertions(+), 27 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
reviewerPATCH 1/3 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 that code is also a perfect excuse to give it a little dev_err_probe() tidyup as well.
---
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(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -2
@@ -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 ; }
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 ; }
@@ -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()
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Will Deacon
maintainerRe: [PATCH 1/3] iommu/arm-smmu: Make instance lookup robust
Quoted 14 lines · Robin Murphy
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 theiommu_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 littledev_err_probe()tidyup as well.
Quoted 6 lines
Signed-off-by Robin Murphy <[email protected]>---
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?
Quoted 3 lines
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(-)
[...]
Quoted 4 lines
drivers/iommu/arm/arm-smmu/arm-smmu.c +0
-0@@ -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);
drivers/iommu/arm/arm-smmu/arm-smmu.c static int arm_smmu_device_probe(struct platform_device *pdev) · line 2252
Quoted 10 lines
drivers/iommu/arm/arm-smmu/arm-smmu.c +10
-0+ 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
Robin Murphy
reviewerRe: [PATCH 1/3] iommu/arm-smmu: Make instance lookup robust
Quoted 15 lines · Will Deacon
Quoted 14 lines · Robin Murphy
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 theiommu_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 littledev_err_probe()tidyup as well.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Robin Murphy <[email protected]>---
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...)
Quoted 27 lines
Quoted 3 lines
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(-)[...]
Quoted 4 lines
drivers/iommu/arm/arm-smmu/arm-smmu.c +0
-0@@ -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);drivers/iommu/arm/arm-smmu/arm-smmu.c static int arm_smmu_device_probe(struct platform_device *pdev) · line 2252
Quoted 10 lines
drivers/iommu/arm/arm-smmu/arm-smmu.c +10
-0+ 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 theEPROBE_DEFERcase, but after your other changes to usebus_find_device_by_fwnode()I'm wondering whether we actually hit that (particularly iniommu_device_sysfs_add())?
Quoted 2 lines
I'm not saying there's a problem, just that I'm not sure why
dev_err_probe()is tidier thandev_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.
Robin Murphy
reviewerPATCH 2/3 iommu/arm-smmu-v3: Clean up iopf queue 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(). Hardly a big deal when probe failure represents something much more seriously wrong in the first place, but on principle, adopt a dedicated cleanup path for those.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -5
@@ -4609,7 +4609,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 free_iopf;/* Record our private device structure */ platform_set_drvdata (pdev, smmu);
@@ -4620,22 +4620,27 @@ static int arm_smmu_device_probe(struct platform_device *pdev)/* Reset the device */ ret =arm_smmu_device_reset (smmu);if (ret) -return ret; + goto free_iopf;/* And we're up. Go go go! */ ret =iommu_device_sysfs_add (&smmu->iommu, dev,NULL ,"smmu3.%pa" , &ioaddr);if (ret) -return ret; + goto free_iopf; 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 free_sysfs; }return 0 ; + +free_sysfs: +iommu_device_sysfs_remove (&smmu->iommu); +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
Will Deacon
maintainerRe: [PATCH 2/3] iommu/arm-smmu-v3: Clean up iopf queue on probe failure
Quoted 44 lines · Robin Murphy
kmemleak noticed that the iopf queue allocated deep down within
arm_smmu_init_structures()can be leaked by a subsequent error return fromarm_smmu_device_probe(). Hardly a big deal when probe failure represents something much more seriously wrong in the first place, but on principle, adopt a dedicated cleanup path for those.Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-5@@ -4609,7 +4609,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 free_iopf;/* Record our private device structure */ platform_set_drvdata (pdev, smmu);@@ -4620,22 +4620,27 @@ static int arm_smmu_device_probe(struct platform_device *pdev)/* Reset the device */ ret =arm_smmu_device_reset (smmu);if (ret) -return ret; + goto free_iopf;/* And we're up. Go go go! */ ret =iommu_device_sysfs_add (&smmu->iommu, dev,NULL ,"smmu3.%pa" , &ioaddr);if (ret) -return ret; + goto free_iopf; 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 free_sysfs;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_device_probe(struct platform_device *pdev) · line 4644
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6
-0}return 0 ; + +free_sysfs: +iommu_device_sysfs_remove (&smmu->iommu); +free_iopf: +iopf_queue_free (smmu->evtq.iopf); +return ret; }
Thanks. I think this is correct (and iopf_queue_free() checks for NULL, so we're good there) but I just wonder whether it would be more consistent to use devm_add_action_or_reset(), like we do for the MSIs.
Will
Robin Murphy
reviewerRe: [PATCH 2/3] iommu/arm-smmu-v3: Clean up iopf queue on probe failure
Quoted 47 lines · Will Deacon
Quoted 46 lines · Robin Murphy
kmemleak noticed that the iopf queue allocated deep down within
arm_smmu_init_structures()can be leaked by a subsequent error return fromarm_smmu_device_probe(). Hardly a big deal when probe failure represents something much more seriously wrong in the first place, but on principle, adopt a dedicated cleanup path for those.Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-5@@ -4609,7 +4609,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 free_iopf;/* Record our private device structure */ platform_set_drvdata (pdev, smmu);@@ -4620,22 +4620,27 @@ static int arm_smmu_device_probe(struct platform_device *pdev)/* Reset the device */ ret =arm_smmu_device_reset (smmu);if (ret) -return ret; + goto free_iopf;/* And we're up. Go go go! */ ret =iommu_device_sysfs_add (&smmu->iommu, dev,NULL ,"smmu3.%pa" , &ioaddr);if (ret) -return ret; + goto free_iopf; 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 free_sysfs; }
Quoted 12 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_device_probe(struct platform_device *pdev) · line 4644
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6
-0return 0 ; + +free_sysfs: +iommu_device_sysfs_remove (&smmu->iommu); +free_iopf: +iopf_queue_free (smmu->evtq.iopf); +return ret; }Thanks. I think this is correct (and
iopf_queue_free()checks for NULL, so we're good there) but I just wonder whether it would be more consistent to usedevm_add_action_or_reset(), like we do for the MSIs.
Yeah, I did consider it, but in the end I had a hunch that we were liable to end up with more cleanup steps on this path anyway. Which then conveniently came true this morning when I realised why the box still wasn't seeing its disk even with the SATA probe un-deferred by patch #3.
(For reference I'm provoking an error from arm_smmu_probe_device() intentionally, by virtue of having a PCIe-to-PCI bridge aliasing two devices to the same StreamID - adding group support to make that actually work is still on the "maybe one day" list...)
Cheers,
Robin.
Robin Murphy
reviewerPATCH 3/3 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.
--- drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)
drivers/iommu/iommu.c +1 -1
@@ -2842,7 +2842,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
Re: [PATCH 3/3] iommu: Manage driver probe deferral better
Quoted 11 lines · Robin Murphy
Since
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.Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)
Will
Robin Murphy
reviewerPATCH 1/1 iommu/arm-smmu-v3: Undo SMMU enable on probe failure
In reply to Robin MurphyIf SMMU probe fails afer arm_smmu_device_reset(), we 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.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + 1 file changed, 1 insertion(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -0
@@ -4640,6 +4640,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)iommu_device_sysfs_remove (&smmu->iommu); free_iopf:iopf_queue_free (smmu->evtq.iopf); +arm_smmu_device_disable (smmu);return ret; }
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Will Deacon
maintainerRe: [PATCH v2.5/3] iommu/arm-smmu-v3: Undo SMMU enable on probe failure
Quoted 16 lines · Robin Murphy
If SMMU probe fails afer
arm_smmu_device_reset(), we 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.Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + 1 file changed, 1 insertion(+)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0
-0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_device_probe(struct platform_device *pdev) · line 4645
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-0@@ -4640,6 +4640,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)iommu_device_sysfs_remove (&smmu->iommu); free_iopf:iopf_queue_free (smmu->evtq.iopf); +arm_smmu_device_disable (smmu);return ret; }
Heh, this is starting to look an awful lot like arm_smmu_device_remove()...
Will
Robin Murphy
reviewerRe: [PATCH v2.5/3] iommu/arm-smmu-v3: Undo SMMU enable on probe failure
Quoted 12 lines · Will Deacon
Quoted 11 lines · Robin Murphy
If SMMU probe fails afer
arm_smmu_device_reset(), we 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.Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + 1 file changed, 1 insertion(+)
Quoted 13 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_device_probe(struct platform_device *pdev) · line 4645
Quoted 11 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-0@@ -4640,6 +4640,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)iommu_device_sysfs_remove (&smmu->iommu); free_iopf:iopf_queue_free (smmu->evtq.iopf); +arm_smmu_device_disable (smmu);return ret; }Heh, this is starting to look an awful lot like
arm_smmu_device_remove()...
Indeed, functionally that's rather the point; implementation-wise, you know I'd branch straight into the middle of a different function if the language let me :)
Robin.