* [PATCH v2] iommu: fix smmu initialization memory leak problem
@ 2022-11-21 3:04 Longfang Liu
2022-11-21 18:05 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: Longfang Liu @ 2022-11-21 3:04 UTC (permalink / raw)
To: robin.murphy, will; +Cc: linux-arm-kernel, iommu, liulongfang
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;
}
static int arm_smmu_device_remove(struct platform_device *pdev)
--
2.24.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-11-21 3:04 [PATCH v2] iommu: fix smmu initialization memory leak problem Longfang Liu
@ 2022-11-21 18:05 ` Will Deacon
2022-11-22 12:00 ` liulongfang
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2022-11-21 18:05 UTC (permalink / raw)
To: Longfang Liu; +Cc: robin.murphy, linux-arm-kernel, iommu
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-11-21 18:05 ` Will Deacon
@ 2022-11-22 12:00 ` liulongfang
2022-11-29 15:24 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: liulongfang @ 2022-11-22 12:00 UTC (permalink / raw)
To: Will Deacon; +Cc: robin.murphy, linux-arm-kernel, iommu
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
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-11-22 12:00 ` liulongfang
@ 2022-11-29 15:24 ` Will Deacon
2022-12-01 12:42 ` liulongfang
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2022-11-29 15:24 UTC (permalink / raw)
To: liulongfang; +Cc: robin.murphy, linux-arm-kernel, iommu
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-11-29 15:24 ` Will Deacon
@ 2022-12-01 12:42 ` liulongfang
2022-12-01 13:31 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: liulongfang @ 2022-12-01 12:42 UTC (permalink / raw)
To: Will Deacon; +Cc: robin.murphy, linux-arm-kernel, iommu
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-12-01 12:42 ` liulongfang
@ 2022-12-01 13:31 ` Will Deacon
2022-12-20 3:17 ` liulongfang
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2022-12-01 13:31 UTC (permalink / raw)
To: liulongfang; +Cc: robin.murphy, linux-arm-kernel, iommu
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-12-01 13:31 ` Will Deacon
@ 2022-12-20 3:17 ` liulongfang
2022-12-20 21:37 ` Marion & Christophe JAILLET
0 siblings, 1 reply; 9+ messages in thread
From: liulongfang @ 2022-12-20 3:17 UTC (permalink / raw)
To: Robin Murphy, Will Deacon, Christophe JAILLET, iommu
Cc: linux-arm-kernel@lists.infradead.org
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-12-20 3:17 ` liulongfang
@ 2022-12-20 21:37 ` Marion & Christophe JAILLET
2023-01-10 12:00 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: Marion & Christophe JAILLET @ 2022-12-20 21:37 UTC (permalink / raw)
To: liulongfang, Robin Murphy, Will Deacon, iommu
Cc: linux-arm-kernel@lists.infradead.org
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iommu: fix smmu initialization memory leak problem
2022-12-20 21:37 ` Marion & Christophe JAILLET
@ 2023-01-10 12:00 ` Will Deacon
0 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2023-01-10 12:00 UTC (permalink / raw)
To: Marion & Christophe JAILLET
Cc: liulongfang, Robin Murphy, iommu,
linux-arm-kernel@lists.infradead.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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-10 12:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-21 3:04 [PATCH v2] iommu: fix smmu initialization memory leak problem Longfang Liu
2022-11-21 18:05 ` Will Deacon
2022-11-22 12:00 ` liulongfang
2022-11-29 15:24 ` Will Deacon
2022-12-01 12:42 ` liulongfang
2022-12-01 13:31 ` Will Deacon
2022-12-20 3:17 ` liulongfang
2022-12-20 21:37 ` Marion & Christophe JAILLET
2023-01-10 12:00 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox