* [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks
@ 2022-07-05 11:28 Liang He
2022-07-05 11:54 ` Arnd Bergmann
0 siblings, 1 reply; 5+ messages in thread
From: Liang He @ 2022-07-05 11:28 UTC (permalink / raw)
To: f.fainelli, bcm-kernel-feedback-list, linux-arm-kernel, windhl
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks
2022-07-05 11:28 [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks Liang He
@ 2022-07-05 11:54 ` Arnd Bergmann
2022-07-05 12:18 ` Liang He
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2022-07-05 11:54 UTC (permalink / raw)
To: Liang He; +Cc: Florian Fainelli, bcm-kernel-feedback-list, Linux ARM
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re:Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks
2022-07-05 11:54 ` Arnd Bergmann
@ 2022-07-05 12:18 ` Liang He
2022-07-05 13:09 ` Arnd Bergmann
0 siblings, 1 reply; 5+ messages in thread
From: Liang He @ 2022-07-05 12:18 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Florian Fainelli, bcm-kernel-feedback-list, Linux ARM
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks
2022-07-05 12:18 ` Liang He
@ 2022-07-05 13:09 ` Arnd Bergmann
2022-07-06 0:59 ` Liang He
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2022-07-05 13:09 UTC (permalink / raw)
To: Liang He
Cc: Arnd Bergmann, Florian Fainelli, bcm-kernel-feedback-list, Linux ARM
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re:Re: Re: [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks
2022-07-05 13:09 ` Arnd Bergmann
@ 2022-07-06 0:59 ` Liang He
0 siblings, 0 replies; 5+ messages in thread
From: Liang He @ 2022-07-06 0:59 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Florian Fainelli, bcm-kernel-feedback-list, Linux ARM
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-06 1:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-05 11:28 [PATCH] soc: brcmstb: pm: Add of_node_put() when the iteration breaks Liang He
2022-07-05 11:54 ` Arnd Bergmann
2022-07-05 12:18 ` Liang He
2022-07-05 13:09 ` Arnd Bergmann
2022-07-06 0:59 ` Liang He
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.