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

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.