| Message ID | 20241212015121.3410941-1-joe@pf.is.s.u-tokyo.ac.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: bd70528: disable wakeup in the error path of .probe() | expand |
Hi Matti, Thank you for your review. On 12/12/24 18:20, Matti Vaittinen wrote: > Hi Joe, > > Thanks for the patch :) > > On 12/12/2024 03:51, Joe Hattori wrote: >> Current code leaves the device's wakeup enabled in the error path of >> .probe(), which results in a memory leak. Therefore, add the >> device_init_wakeup(&device->dev, false) calls before returning an error. >> >> This bug was found by an experimental static analysis tool that I am >> developing. >> >> Fixes: 32a4a4ebf768 ("rtc: bd70528: Initial support for ROHM bd70528 >> RTC") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> drivers/rtc/rtc-bd70528.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c >> index 954ac4ef53e8..50059f7ba6d0 100644 >> --- a/drivers/rtc/rtc-bd70528.c >> +++ b/drivers/rtc/rtc-bd70528.c >> @@ -312,12 +312,12 @@ static int bd70528_probe(struct platform_device >> *pdev) >> } >> } >> - device_set_wakeup_capable(&pdev->dev, true); >> - device_wakeup_enable(&pdev->dev); >> + device_init_wakeup(&pdev->dev, true); > > Is there a reason to do this prior rtc-device allocation/registration? > If not, then the flow would be cleaner if this was done as the last step > in probe() because we could omit the clean-up. Enabling the device's wakeup in the last step makes much more sense. Applied to the V2 patch. > >> rtc = devm_rtc_allocate_device(&pdev->dev); >> if (IS_ERR(rtc)) { >> dev_err(&pdev->dev, "RTC device creation failed\n"); >> + device_init_wakeup(&pdev->dev, false); >> return PTR_ERR(rtc); >> } >> @@ -328,10 +328,15 @@ static int bd70528_probe(struct platform_device >> *pdev) >> /* Request alarm IRQ prior to registerig the RTC */ >> ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, &alm_hndlr, >> IRQF_ONESHOT, "bd70528-rtc", rtc); >> - if (ret) >> + if (ret) { >> + device_init_wakeup(&pdev->dev, false); >> return ret; >> + } >> - return devm_rtc_register_device(rtc); >> + ret = devm_rtc_register_device(rtc); >> + if (ret) >> + device_init_wakeup(&pdev->dev, false); >> + return ret; >> } >> static const struct platform_device_id bd718x7_rtc_id[] = { > > Yours, > -- Matti > Best, Joe
diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c index 954ac4ef53e8..50059f7ba6d0 100644 --- a/drivers/rtc/rtc-bd70528.c +++ b/drivers/rtc/rtc-bd70528.c @@ -312,12 +312,12 @@ static int bd70528_probe(struct platform_device *pdev) } } - device_set_wakeup_capable(&pdev->dev, true); - device_wakeup_enable(&pdev->dev); + device_init_wakeup(&pdev->dev, true); rtc = devm_rtc_allocate_device(&pdev->dev); if (IS_ERR(rtc)) { dev_err(&pdev->dev, "RTC device creation failed\n"); + device_init_wakeup(&pdev->dev, false); return PTR_ERR(rtc); } @@ -328,10 +328,15 @@ static int bd70528_probe(struct platform_device *pdev) /* Request alarm IRQ prior to registerig the RTC */ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, &alm_hndlr, IRQF_ONESHOT, "bd70528-rtc", rtc); - if (ret) + if (ret) { + device_init_wakeup(&pdev->dev, false); return ret; + } - return devm_rtc_register_device(rtc); + ret = devm_rtc_register_device(rtc); + if (ret) + device_init_wakeup(&pdev->dev, false); + return ret; } static const struct platform_device_id bd718x7_rtc_id[] = {
Current code leaves the device's wakeup enabled in the error path of .probe(), which results in a memory leak. Therefore, add the device_init_wakeup(&device->dev, false) calls before returning an error. This bug was found by an experimental static analysis tool that I am developing. Fixes: 32a4a4ebf768 ("rtc: bd70528: Initial support for ROHM bd70528 RTC") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/rtc/rtc-bd70528.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)