| Message ID | 20220705112815.282835-1-windhl@126.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: brcmstb: pm: Add of_node_put() when the iteration breaks | expand |
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
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
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
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
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; }
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(+)