| Message ID | 161cf3bfde2993cc29e82589df947ff23fb82845.1731088789.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/arm-smmu: Minor probe_device related improvements | expand |
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
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.
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)
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(-)