| Message ID | 20221121030421.19295-1-liulongfang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iommu: fix smmu initialization memory leak problem | expand |
On Mon, Nov 21, 2022 at 11:04:21AM +0800, Longfang Liu wrote: > When iommu_device_register() in arm_smmu_device_probe() fails, > in addition to sysfs needs to be deleted, device should also > be disabled, and the memory of iopf needs to be released to > prevent memory leak of iopf. > > Changes v1 -> v2: > -Improve arm_smmu_device_probe() abnormal exit function. > > Signed-off-by: Longfang Liu <liulongfang@huawei.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- > 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -3815,7 +3815,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 err_iopf; > > /* Record our private device structure */ > platform_set_drvdata(pdev, smmu); > @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > /* Reset the device */ > ret = arm_smmu_device_reset(smmu, bypass); > if (ret) > - return ret; > + goto err_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 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); > +err_iopf: > + iopf_queue_free(smmu->evtq.iopf); > + return ret; I previously suggested using devres_alloc() for this instead. Did that not work? Will
On 2022/11/22 2:05, Will Deacon wrote: > On Mon, Nov 21, 2022 at 11:04:21AM +0800, Longfang Liu wrote: >> When iommu_device_register() in arm_smmu_device_probe() fails, >> in addition to sysfs needs to be deleted, device should also >> be disabled, and the memory of iopf needs to be released to >> prevent memory leak of iopf. >> >> Changes v1 -> v2: >> -Improve arm_smmu_device_probe() abnormal exit function. >> >> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >> --- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- >> 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> @@ -3815,7 +3815,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 err_iopf; >> >> /* Record our private device structure */ >> platform_set_drvdata(pdev, smmu); >> @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >> /* Reset the device */ >> ret = arm_smmu_device_reset(smmu, bypass); >> if (ret) >> - return ret; >> + goto err_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 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); >> +err_iopf: >> + iopf_queue_free(smmu->evtq.iopf); >> + return ret; > > I previously suggested using devres_alloc() for this instead. Did that > not work? > This patch is only for fixing iopf's memory leak. The use of devres_alloc() is an optimization solution for iopf queue management, which is another set of patch matters. Thanks, Longfang. > Will > . >
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: > >> When iommu_device_register() in arm_smmu_device_probe() fails, > >> in addition to sysfs needs to be deleted, device should also > >> be disabled, and the memory of iopf needs to be released to > >> prevent memory leak of iopf. > >> > >> Changes v1 -> v2: > >> -Improve arm_smmu_device_probe() abnormal exit function. > >> > >> Signed-off-by: Longfang Liu <liulongfang@huawei.com> > >> --- > >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- > >> 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 > >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >> @@ -3815,7 +3815,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 err_iopf; > >> > >> /* Record our private device structure */ > >> platform_set_drvdata(pdev, smmu); > >> @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > >> /* Reset the device */ > >> ret = arm_smmu_device_reset(smmu, bypass); > >> if (ret) > >> - return ret; > >> + goto err_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 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); > >> +err_iopf: > >> + iopf_queue_free(smmu->evtq.iopf); > >> + return ret; > > > > I previously suggested using devres_alloc() for this instead. Did that > > not work? > > > > This patch is only for fixing iopf's memory leak. > The use of devres_alloc() is an optimization solution for iopf queue management, > which is another set of patch matters. Great, I look forward to that set of patches! Will
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: >>>> When iommu_device_register() in arm_smmu_device_probe() fails, >>>> in addition to sysfs needs to be deleted, device should also >>>> be disabled, and the memory of iopf needs to be released to >>>> prevent memory leak of iopf. >>>> >>>> Changes v1 -> v2: >>>> -Improve arm_smmu_device_probe() abnormal exit function. >>>> >>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- >>>> 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 >>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>> @@ -3815,7 +3815,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 err_iopf; >>>> >>>> /* Record our private device structure */ >>>> platform_set_drvdata(pdev, smmu); >>>> @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >>>> /* Reset the device */ >>>> ret = arm_smmu_device_reset(smmu, bypass); >>>> if (ret) >>>> - return ret; >>>> + goto err_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 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); >>>> +err_iopf: >>>> + iopf_queue_free(smmu->evtq.iopf); >>>> + return ret; >>> >>> I previously suggested using devres_alloc() for this instead. Did that >>> not work? >>> >> >> This patch is only for fixing iopf's memory leak. >> The use of devres_alloc() is an optimization solution for iopf queue management, >> which is another set of patch matters. > > Great, I look forward to that set of patches! > > Will > . > Will this patch be merged into the next branch? Thanks, Longfang.
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: > >>>> When iommu_device_register() in arm_smmu_device_probe() fails, > >>>> in addition to sysfs needs to be deleted, device should also > >>>> be disabled, and the memory of iopf needs to be released to > >>>> prevent memory leak of iopf. > >>>> > >>>> Changes v1 -> v2: > >>>> -Improve arm_smmu_device_probe() abnormal exit function. > >>>> > >>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> > >>>> --- > >>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- > >>>> 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 > >>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>>> @@ -3815,7 +3815,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 err_iopf; > >>>> > >>>> /* Record our private device structure */ > >>>> platform_set_drvdata(pdev, smmu); > >>>> @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > >>>> /* Reset the device */ > >>>> ret = arm_smmu_device_reset(smmu, bypass); > >>>> if (ret) > >>>> - return ret; > >>>> + goto err_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 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); > >>>> +err_iopf: > >>>> + iopf_queue_free(smmu->evtq.iopf); > >>>> + return ret; > >>> > >>> I previously suggested using devres_alloc() for this instead. Did that > >>> not work? > >>> > >> > >> This patch is only for fixing iopf's memory leak. > >> The use of devres_alloc() is an optimization solution for iopf queue management, > >> which is another set of patch matters. > > > > Great, I look forward to that set of patches! > > > > Will this patch be merged into the next branch? I don't plan to merge this one, no. I'll wait for the other patches which do this using devres_alloc() instead. Thanks, Will
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: >>>>>> When iommu_device_register() in arm_smmu_device_probe() fails, >>>>>> in addition to sysfs needs to be deleted, device should also >>>>>> be disabled, and the memory of iopf needs to be released to >>>>>> prevent memory leak of iopf. >>>>>> >>>>>> Changes v1 -> v2: >>>>>> -Improve arm_smmu_device_probe() abnormal exit function. >>>>>> >>>>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >>>>>> --- >>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- >>>>>> 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 >>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>>> @@ -3815,7 +3815,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 err_iopf; >>>>>> >>>>>> /* Record our private device structure */ >>>>>> platform_set_drvdata(pdev, smmu); >>>>>> @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >>>>>> /* Reset the device */ >>>>>> ret = arm_smmu_device_reset(smmu, bypass); >>>>>> if (ret) >>>>>> - return ret; >>>>>> + goto err_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 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); >>>>>> +err_iopf: >>>>>> + iopf_queue_free(smmu->evtq.iopf); >>>>>> + return ret; >>>>> >>>>> I previously suggested using devres_alloc() for this instead. Did that >>>>> not work? >>>>> >>>> >>>> This patch is only for fixing iopf's memory leak. >>>> The use of devres_alloc() is an optimization solution for iopf queue management, >>>> which is another set of patch matters. >>> >>> Great, I look forward to that set of patches! >>> >> >> Will this patch be merged into the next branch? > > I don't plan to merge this one, no. I'll wait for the other patches which do > this using devres_alloc() instead. > > Thanks, > > Will > . > 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(). I hope you can modify it, and I will quit this repair work. Thanks, Longfang.
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: >>>>>>> When iommu_device_register() in arm_smmu_device_probe() fails, >>>>>>> in addition to sysfs needs to be deleted, device should also >>>>>>> be disabled, and the memory of iopf needs to be released to >>>>>>> prevent memory leak of iopf. >>>>>>> >>>>>>> Changes v1 -> v2: >>>>>>> -Improve arm_smmu_device_probe() abnormal exit function. >>>>>>> >>>>>>> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >>>>>>> --- >>>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- >>>>>>> 1 file changed, 11 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 ab160198edd6..b892f5233f88 100644 >>>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>>>> @@ -3815,7 +3815,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 err_iopf; >>>>>>> >>>>>>> /* Record our private device structure */ >>>>>>> platform_set_drvdata(pdev, smmu); >>>>>>> @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >>>>>>> /* Reset the device */ >>>>>>> ret = arm_smmu_device_reset(smmu, bypass); >>>>>>> if (ret) >>>>>>> - return ret; >>>>>>> + goto err_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 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); >>>>>>> +err_iopf: >>>>>>> + iopf_queue_free(smmu->evtq.iopf); >>>>>>> + return ret; >>>>>> I previously suggested using devres_alloc() for this instead. Did that >>>>>> not work? >>>>>> >>>>> This patch is only for fixing iopf's memory leak. >>>>> The use of devres_alloc() is an optimization solution for iopf queue management, >>>>> which is another set of patch matters. >>>> Great, I look forward to that set of patches! >>>> >>> Will this patch be merged into the next branch? >> I don't plan to merge this one, no. I'll wait for the other patches which do >> this using devres_alloc() instead. >> >> Thanks, >> >> Will >> . >> > 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(). Hi, more or less. 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; } static int arm_smmu_device_remove(struct platform_device *pdev) @@ -3851,7 +3866,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev) iommu_device_unregister(&smmu->iommu); iommu_device_sysfs_remove(&smmu->iommu); arm_smmu_device_disable(smmu); - iopf_queue_free(smmu->evtq.iopf); return 0; } I'm not a really big fan because it adds too much code for me. But I'm not a maintainer, so let them have the last word on it. At least, this avoids an odd iopf_queue_free() call that comes from nowhere without looking deeper in the code. It has been compile tested only on arm64. > I hope you can modify it, and I will quit this repair work. If it please you and Will, feel free to propose it as a v3 of your patch. CJ > Thanks, > Longfang.
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
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..b892f5233f88 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3815,7 +3815,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 err_iopf; /* Record our private device structure */ platform_set_drvdata(pdev, smmu); @@ -3826,22 +3826,28 @@ static int arm_smmu_device_probe(struct platform_device *pdev) /* Reset the device */ ret = arm_smmu_device_reset(smmu, bypass); if (ret) - return ret; + goto err_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 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); +err_iopf: + iopf_queue_free(smmu->evtq.iopf); + return ret; } static int arm_smmu_device_remove(struct platform_device *pdev)
When iommu_device_register() in arm_smmu_device_probe() fails, in addition to sysfs needs to be deleted, device should also be disabled, and the memory of iopf needs to be released to prevent memory leak of iopf. Changes v1 -> v2: -Improve arm_smmu_device_probe() abnormal exit function. Signed-off-by: Longfang Liu <liulongfang@huawei.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)