[PATCH 0/3] iommu/arm-smmu: Minor probe_device related improvements

[PATCH 0/3] iommu/arm-smmu: Minor probe_device related improvements

[Robin Murphy]

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(-)

-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 1/3] iommu/arm-smmu: Make instance lookup robust

[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 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(-)

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()
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 2/3] iommu/arm-smmu-v3: Clean up iopf queue on probe failure

[Robin Murphy]

kmemleak 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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

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 b7dcb1494aa4..7908fca962fe 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -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)
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 3/3] iommu: Manage driver probe deferral better

[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 <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c    | 2 +-
 drivers/iommu/of_iommu.c | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 83c8e617a2c5..dc8d11b585cd 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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;
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e7a6a1611d19..97987cd78da9 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -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;
 
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v2.5/3] iommu/arm-smmu-v3: Undo SMMU enable on probe failure

[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 <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
 1 file changed, 1 insertion(+)

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 7908fca962fe..566d66e9c91e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -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;
 }
 
-- 
2.39.2.101.g768bb238c484.dirty

Re: [PATCH 1/3] iommu/arm-smmu: Make instance lookup robust

[Will Deacon]

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

Re: [PATCH 2/3] iommu/arm-smmu-v3: Clean up iopf queue on probe failure

[Will Deacon]

On Fri, Nov 08, 2024 at 06:30:16PM +0000, Robin Murphy wrote:
> kmemleak 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> 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 b7dcb1494aa4..7908fca962fe 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -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;
>  }

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

Re: [PATCH v2.5/3] iommu/arm-smmu-v3: Undo SMMU enable on probe failure

[Will Deacon]

On Tue, Nov 12, 2024 at 02:49:59PM +0000, Robin Murphy wrote:
> 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 <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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 7908fca962fe..566d66e9c91e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -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

Re: [PATCH 3/3] iommu: Manage driver probe deferral better

[Will Deacon]

On Fri, Nov 08, 2024 at 06:30:17PM +0000, Robin Murphy wrote:
> 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 <robin.murphy@arm.com>
> ---
>  drivers/iommu/iommu.c    | 2 +-
>  drivers/iommu/of_iommu.c | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH 1/3] iommu/arm-smmu: Make instance lookup robust

[Robin Murphy]

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.

Re: [PATCH v2.5/3] iommu/arm-smmu-v3: Undo SMMU enable on probe failure

[Robin Murphy]

On 12/11/2024 3:57 pm, Will Deacon wrote:
> On Tue, Nov 12, 2024 at 02:49:59PM +0000, Robin Murphy wrote:
>> 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 <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> 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 7908fca962fe..566d66e9c91e 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -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.

Re: [PATCH 2/3] iommu/arm-smmu-v3: Clean up iopf queue on probe failure

[Robin Murphy]

On 12/11/2024 3:55 pm, Will Deacon wrote:
> On Fri, Nov 08, 2024 at 06:30:16PM +0000, Robin Murphy wrote:
>> kmemleak 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.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> 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 b7dcb1494aa4..7908fca962fe 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -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;
>>   }
> 
> 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.

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.