Re: [PATCH v2] iommu: fix smmu initialization memory leak problem

[Will Deacon <will@kernel.org>]

On Tue, Dec 20, 2022 at 10:37:51PM +0100, Marion & Christophe JAILLET wrote:
> 
> Le 20/12/2022 à 04:17, liulongfang a écrit :
> > On 2022/12/1 21:31, Will Deacon Wrote:
> > > On Thu, Dec 01, 2022 at 08:42:02PM +0800, liulongfang wrote:
> > > > On 2022/11/29 23:24, Will Deacon wrote:
> > > > > On Tue, Nov 22, 2022 at 08:00:39PM +0800, liulongfang wrote:
> > > > > > On 2022/11/22 2:05, Will Deacon wrote:
> > > > > > > On Mon, Nov 21, 2022 at 11:04:21AM +0800, Longfang Liu wrote:
> > Hi Christophe:
> > "[PATCH] iommu/arm-smmu-v3: Fix an error handling path in arm_smmu_device_probe()"
> > 
> > the patch you sent is the same as mine. The maintainer hopes to optimize the queue
> > application part of iopf with devres_alloc().
> 
> You also added a arm_smmu_device_disable() call in the error handling path.
> Looks good to me, but should be confirmed by s.o who knows the hardware.
> 
> That said, I think that what has been suggested by Will Deacon would be
> something like:
> 
> 
> 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 ab160198edd6..1994990decb8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2930,6 +2930,11 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device
> *smmu)
>      return 0;
>  }
> 
> +static void arm_smmu_free_queues(void *ptr)
> +{
> +    iopf_queue_free(ptr);
> +}
> +
>  static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  {
>      int ret;
> @@ -2957,6 +2962,11 @@ static int arm_smmu_init_queues(struct
> arm_smmu_device *smmu)
>          smmu->evtq.iopf = iopf_queue_alloc(dev_name(smmu->dev));
>          if (!smmu->evtq.iopf)
>              return -ENOMEM;
> +
> +        ret = devm_add_action_or_reset(smmu->dev, arm_smmu_free_queues,
> +                           smmu->evtq.iopf);
> +        if (ret)
> +            return ret;
>      }
> 
>      /* priq */
> @@ -3832,16 +3842,21 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
>      ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
>                       "smmu3.%pa", &ioaddr);
>      if (ret)
> -        return ret;
> +        goto err_reset;
> 
>      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 err_sysfs_add;
>      }
> 
>      return 0;
> +
> +err_sysfs_add:
> +    iommu_device_sysfs_remove(&smmu->iommu);
> +err_reset:
> +    arm_smmu_device_disable(smmu);
> +    return ret;
>  }

I don't see the need for this hunk -- we presently call
iommu_device_sysfs_remove() if iommu_device_register() fails, so that
can stay as-is. If it's necessary to call arm_smmu_device_disable() then
I think that should be a separate patch because it doesn't seem related
to the freeing of the iopf queue at all.

Otherwise, I'm happy to queue something like this using
devm_add_action_or_reset(), thanks.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel