| Message ID | 20241212025007.3472182-1-joe@pf.is.s.u-tokyo.ac.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: brcmstb-waketimer: disable wakeup in .remove() and in the error path of .probe() | expand |
On 12/11/24 18:50, Joe Hattori wrote: > Current code leaves the device's wakeup enabled in .remove() and in the > error path of .probe(), which results in a memory leak. Therefore, add > the device_init_wakeup(dev, false) call. > > This bug was found by an experimental static analysis tool that I am > developing. > > Fixes: 2cd98b22c144 ("rtc: brcmstb-waketimer: non-functional code changes") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> Looks like you forgot to copy the maintainer (Doug Berger), added him. > --- > drivers/rtc/rtc-brcmstb-waketimer.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c > index fb47c32ab5ff..1bdfec94c693 100644 > --- a/drivers/rtc/rtc-brcmstb-waketimer.c > +++ b/drivers/rtc/rtc-brcmstb-waketimer.c > @@ -298,15 +298,17 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev) > device_init_wakeup(dev, true); > > ret = platform_get_irq(pdev, 0); > - if (ret < 0) > - return -ENODEV; > + if (ret < 0) { > + ret = -ENODEV; > + goto err_disable_wakeup; > + } > timer->wake_irq = (unsigned int)ret; > > timer->clk = devm_clk_get(dev, NULL); > if (!IS_ERR(timer->clk)) { > ret = clk_prepare_enable(timer->clk); > if (ret) > - return ret; > + goto err_disable_wakeup; > timer->rate = clk_get_rate(timer->clk); > if (!timer->rate) > timer->rate = BRCMSTB_WKTMR_DEFAULT_FREQ; > @@ -351,6 +353,8 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev) > err_clk: > clk_disable_unprepare(timer->clk); > > +err_disable_wakeup: > + device_init_wakeup(dev, false); > return ret; The error path change looks good to me, > } > > @@ -360,6 +364,7 @@ static void brcmstb_waketmr_remove(struct platform_device *pdev) > > unregister_reboot_notifier(&timer->reboot_notifier); > clk_disable_unprepare(timer->clk); > + device_init_wakeup(&pdev->dev, false); That part, I don't think is necessary at all, since the interrupts handlers have been freed and disabled, they will not be causing any system wake-up, besides that, the device is completely gone from /sys.
On 12/18/24 04:16, Florian Fainelli wrote: > On 12/11/24 18:50, Joe Hattori wrote: >> Current code leaves the device's wakeup enabled in .remove() and in the >> error path of .probe(), which results in a memory leak. Therefore, add >> the device_init_wakeup(dev, false) call. >> >> This bug was found by an experimental static analysis tool that I am >> developing. >> >> Fixes: 2cd98b22c144 ("rtc: brcmstb-waketimer: non-functional code >> changes") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > > Looks like you forgot to copy the maintainer (Doug Berger), added him. Thanks! > >> --- >> drivers/rtc/rtc-brcmstb-waketimer.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c >> b/drivers/rtc/rtc-brcmstb-waketimer.c >> index fb47c32ab5ff..1bdfec94c693 100644 >> --- a/drivers/rtc/rtc-brcmstb-waketimer.c >> +++ b/drivers/rtc/rtc-brcmstb-waketimer.c >> @@ -298,15 +298,17 @@ static int brcmstb_waketmr_probe(struct >> platform_device *pdev) >> device_init_wakeup(dev, true); >> ret = platform_get_irq(pdev, 0); >> - if (ret < 0) >> - return -ENODEV; >> + if (ret < 0) { >> + ret = -ENODEV; >> + goto err_disable_wakeup; >> + } >> timer->wake_irq = (unsigned int)ret; >> timer->clk = devm_clk_get(dev, NULL); >> if (!IS_ERR(timer->clk)) { >> ret = clk_prepare_enable(timer->clk); >> if (ret) >> - return ret; >> + goto err_disable_wakeup; >> timer->rate = clk_get_rate(timer->clk); >> if (!timer->rate) >> timer->rate = BRCMSTB_WKTMR_DEFAULT_FREQ; >> @@ -351,6 +353,8 @@ static int brcmstb_waketmr_probe(struct >> platform_device *pdev) >> err_clk: >> clk_disable_unprepare(timer->clk); >> +err_disable_wakeup: >> + device_init_wakeup(dev, false); >> return ret; > > The error path change looks good to me, > >> } >> @@ -360,6 +364,7 @@ static void brcmstb_waketmr_remove(struct >> platform_device *pdev) >> unregister_reboot_notifier(&timer->reboot_notifier); >> clk_disable_unprepare(timer->clk); >> + device_init_wakeup(&pdev->dev, false); > > That part, I don't think is necessary at all, since the interrupts > handlers have been freed and disabled, they will not be causing any > system wake-up, besides that, the device is completely gone from /sys. Thank you for the review. I am currently working on adding the devm_ version of the device_init_wakeup() [1], so let me come back to this patch after the function is added. [1]: https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/ Best, Joe
diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c index fb47c32ab5ff..1bdfec94c693 100644 --- a/drivers/rtc/rtc-brcmstb-waketimer.c +++ b/drivers/rtc/rtc-brcmstb-waketimer.c @@ -298,15 +298,17 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev) device_init_wakeup(dev, true); ret = platform_get_irq(pdev, 0); - if (ret < 0) - return -ENODEV; + if (ret < 0) { + ret = -ENODEV; + goto err_disable_wakeup; + } timer->wake_irq = (unsigned int)ret; timer->clk = devm_clk_get(dev, NULL); if (!IS_ERR(timer->clk)) { ret = clk_prepare_enable(timer->clk); if (ret) - return ret; + goto err_disable_wakeup; timer->rate = clk_get_rate(timer->clk); if (!timer->rate) timer->rate = BRCMSTB_WKTMR_DEFAULT_FREQ; @@ -351,6 +353,8 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev) err_clk: clk_disable_unprepare(timer->clk); +err_disable_wakeup: + device_init_wakeup(dev, false); return ret; } @@ -360,6 +364,7 @@ static void brcmstb_waketmr_remove(struct platform_device *pdev) unregister_reboot_notifier(&timer->reboot_notifier); clk_disable_unprepare(timer->clk); + device_init_wakeup(&pdev->dev, false); } #ifdef CONFIG_PM_SLEEP
Current code leaves the device's wakeup enabled in .remove() and in the error path of .probe(), which results in a memory leak. Therefore, add the device_init_wakeup(dev, false) call. This bug was found by an experimental static analysis tool that I am developing. Fixes: 2cd98b22c144 ("rtc: brcmstb-waketimer: non-functional code changes") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/rtc/rtc-brcmstb-waketimer.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)