[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 at 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