[PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks

[PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks

[Liang He]

In brcmstb_pm_probe(), we should call of_node_put() when the
for_each_matching_node() iteration breaks as the for_each_xx OF
APIs will automatically increase and decrease  the refcount in
each iteration.

Fixes: 0b741b8234c8 ("soc: bcm: brcmstb: Add support for S2/S3/S5 suspend states (ARM)")
Signed-off-by: Liang He <windhl@126.com>
---
 Patched file has been compiled test in 5.19rc5.

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index 70ad0f3dce28..f5573d6efae3 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -732,11 +732,13 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
 		i = ctrl.num_memc;
 		if (i >= MAX_NUM_MEMC) {
 			pr_warn("too many MEMCs (max %d)\n", MAX_NUM_MEMC);
+			of_node_put(dn);
 			break;
 		}
 
 		base = of_io_request_and_map(dn, 0, dn->full_name);
 		if (IS_ERR(base)) {
+			of_node_put(dn);
 			if (!ctrl.support_warm_boot)
 				break;
 
@@ -752,12 +754,14 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
 	for_each_matching_node(dn, brcmstb_memc_of_match) {
 		base = of_iomap(dn, 0);
 		if (!base) {
+			of_node_put(dn);
 			pr_err("error mapping DDR Sequencer %d\n", i);
 			return -ENOMEM;
 		}
 
 		of_id = of_match_node(brcmstb_memc_of_match, dn);
 		if (!of_id) {
+			of_node_put(dn);
 			iounmap(base);
 			return -EINVAL;
 		}
-- 
2.25.1


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

Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks

[Arnd Bergmann]

On Tue, Jul 5, 2022 at 1:28 PM Liang He <windhl@126.com> wrote:
> --- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> +++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> @@ -732,11 +732,13 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>                 i = ctrl.num_memc;
>                 if (i >= MAX_NUM_MEMC) {
>                         pr_warn("too many MEMCs (max %d)\n", MAX_NUM_MEMC);
> +                       of_node_put(dn);
>                         break;
>                 }
>
>                 base = of_io_request_and_map(dn, 0, dn->full_name);
>                 if (IS_ERR(base)) {
> +                       of_node_put(dn);
>                         if (!ctrl.support_warm_boot)
>                                 break;
>
> @@ -752,12 +754,14 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>         for_each_matching_node(dn, brcmstb_memc_of_match) {
>                 base = of_iomap(dn, 0);
>                 if (!base) {
> +                       of_node_put(dn);
>                         pr_err("error mapping DDR Sequencer %d\n", i);
>                         return -ENOMEM;
>                 }
>
>                 of_id = of_match_node(brcmstb_memc_of_match, dn);
>                 if (!of_id) {
> +                       of_node_put(dn);
>                         iounmap(base);
>                         return -EINVAL;
>                 }

While all of these changes look correct to me, there seems to be a
larger issue in the error handling for these loops, as they also leak
the __iomem token returned by of_iomap()/of_io_request_and_map()
and by the earlier calls from this function.

Can you try to rework the unwinding to address those as well?

       Arnd

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

Re:Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks

[Liang He]

At 2022-07-05 19:54:05, "Arnd Bergmann" <arnd@arndb.de> wrote:
>On Tue, Jul 5, 2022 at 1:28 PM Liang He <windhl@126.com> wrote:
>> --- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
>> +++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
>> @@ -732,11 +732,13 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>>                 i = ctrl.num_memc;
>>                 if (i >= MAX_NUM_MEMC) {
>>                         pr_warn("too many MEMCs (max %d)\n", MAX_NUM_MEMC);
>> +                       of_node_put(dn);
>>                         break;
>>                 }
>>
>>                 base = of_io_request_and_map(dn, 0, dn->full_name);
>>                 if (IS_ERR(base)) {
>> +                       of_node_put(dn);
>>                         if (!ctrl.support_warm_boot)
>>                                 break;
>>
>> @@ -752,12 +754,14 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>>         for_each_matching_node(dn, brcmstb_memc_of_match) {
>>                 base = of_iomap(dn, 0);
>>                 if (!base) {
>> +                       of_node_put(dn);
>>                         pr_err("error mapping DDR Sequencer %d\n", i);
>>                         return -ENOMEM;
>>                 }
>>
>>                 of_id = of_match_node(brcmstb_memc_of_match, dn);
>>                 if (!of_id) {
>> +                       of_node_put(dn);
>>                         iounmap(base);
>>                         return -EINVAL;
>>                 }
>
>While all of these changes look correct to me, there seems to be a
>larger issue in the error handling for these loops, as they also leak
>the __iomem token returned by of_iomap()/of_io_request_and_map()
>and by the earlier calls from this function.
>
>Can you try to rework the unwinding to address those as well?
>
>       Arnd

Hi, Arnd

Thanks very much for your reply and your advice.

I would like to give more useful patching work, but now I have only learned
the 'device_node' related OF APIs.

It will be easies if you could provide a special case to teach me how to patch
of_io_xx related bugs or just a related  link in lore.kernel.org.

Thanks again,

Liang 


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

Re: Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks

[Arnd Bergmann]

On Tue, Jul 5, 2022 at 2:18 PM Liang He <windhl@126.com> wrote:
> At 2022-07-05 19:54:05, "Arnd Bergmann" <arnd@arndb.de> wrote:
>
> Thanks very much for your reply and your advice.
>
> I would like to give more useful patching work, but now I have only learned
> the 'device_node' related OF APIs.
>
> It will be easies if you could provide a special case to teach me how to patch
> of_io_xx related bugs or just a related  link in lore.kernel.org.

The general rule is that for anything that happens in the probe() function,
you have to negate what happened in case the probe function fails.
If you look at drivers/soc/bcm/brcmstb/pm/pm-mips.c, you find that
its probe function has an iounmap() for each ioremap().

Alternatively, you can also use 'devm_*()' functions that do an
automatic cleanup when a probe function fails, or when the
remove function returns (which this driver does not have).

       Arnd

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

Re:Re: Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks

[Liang He]

At 2022-07-05 21:09:19, "Arnd Bergmann" <arnd@arndb.de> wrote:
>On Tue, Jul 5, 2022 at 2:18 PM Liang He <windhl@126.com> wrote:
>> At 2022-07-05 19:54:05, "Arnd Bergmann" <arnd@arndb.de> wrote:
>>
>> Thanks very much for your reply and your advice.
>>
>> I would like to give more useful patching work, but now I have only learned
>> the 'device_node' related OF APIs.
>>
>> It will be easies if you could provide a special case to teach me how to patch
>> of_io_xx related bugs or just a related  link in lore.kernel.org.
>
>The general rule is that for anything that happens in the probe() function,
>you have to negate what happened in case the probe function fails.
>If you look at drivers/soc/bcm/brcmstb/pm/pm-mips.c, you find that
>its probe function has an iounmap() for each ioremap().
>
>Alternatively, you can also use 'devm_*()' functions that do an
>automatic cleanup when a probe function fails, or when the
>remove function returns (which this driver does not have).
>
>       Arnd

Thanks, I will try it.

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