| Message ID | 20241212100403.3799667-1-joe@pf.is.s.u-tokyo.ac.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] rtc: bd70528: enable the device's wakeup in the last step of .probe() | expand |
On 12/12/2024 19:04:03+0900, Joe Hattori wrote: > Current code leaves the device's wakeup enabled in the error path of > .probe(), which results in a memory leak. Call device_init_wakeup() as > the last step in the .probe() to avoid this leak. Do you have more info on where the memory is allocated? Coudln't we have a devm_ version of device_init_wakeup instead? > > 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> > --- > Changes in V2: > - Move the device_init_wakeup() to the last step of the .probe() to make > the cleanup simpler. > --- > drivers/rtc/rtc-bd70528.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c > index 954ac4ef53e8..d5cc4993f918 100644 > --- a/drivers/rtc/rtc-bd70528.c > +++ b/drivers/rtc/rtc-bd70528.c > @@ -312,9 +312,6 @@ static int bd70528_probe(struct platform_device *pdev) > } > } > > - device_set_wakeup_capable(&pdev->dev, true); > - device_wakeup_enable(&pdev->dev); > - > rtc = devm_rtc_allocate_device(&pdev->dev); > if (IS_ERR(rtc)) { > dev_err(&pdev->dev, "RTC device creation failed\n"); > @@ -331,7 +328,12 @@ static int bd70528_probe(struct platform_device *pdev) > if (ret) > return ret; > > - return devm_rtc_register_device(rtc); > + ret = devm_rtc_register_device(rtc); > + if (ret) > + return ret; > + > + device_init_wakeup(&pdev->dev, true); > + return 0; > } > > static const struct platform_device_id bd718x7_rtc_id[] = { > -- > 2.34.1 >
Hi Alexandre, Thank you for your review. On 12/12/24 19:34, Alexandre Belloni wrote: > On 12/12/2024 19:04:03+0900, Joe Hattori wrote: >> Current code leaves the device's wakeup enabled in the error path of >> .probe(), which results in a memory leak. Call device_init_wakeup() as >> the last step in the .probe() to avoid this leak. > > Do you have more info on where the memory is allocated? device_wakeup_enable() calls wakeup_source_register(), which allocates struct wakeup_source. Also, when the device is already registered, wakeup_source_sysfs_add() is called to allocate and register a child device through wakeup_source_device_create(). If this information needs to be in the commit message, please let me know and I will include it in the V4 patch. > > Coudln't we have a devm_ version of device_init_wakeup instead? Yes, I think it is possible. However, for this patch, calling device_init_wakeup() at the end of the .probe() would suffice, and I think implementing the devm_ function should be in a different patch. Please let me know if you think otherwise. > >> >> 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> >> --- >> Changes in V2: >> - Move the device_init_wakeup() to the last step of the .probe() to make >> the cleanup simpler. >> --- >> drivers/rtc/rtc-bd70528.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c >> index 954ac4ef53e8..d5cc4993f918 100644 >> --- a/drivers/rtc/rtc-bd70528.c >> +++ b/drivers/rtc/rtc-bd70528.c >> @@ -312,9 +312,6 @@ static int bd70528_probe(struct platform_device *pdev) >> } >> } >> >> - device_set_wakeup_capable(&pdev->dev, true); >> - device_wakeup_enable(&pdev->dev); >> - >> rtc = devm_rtc_allocate_device(&pdev->dev); >> if (IS_ERR(rtc)) { >> dev_err(&pdev->dev, "RTC device creation failed\n"); >> @@ -331,7 +328,12 @@ static int bd70528_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - return devm_rtc_register_device(rtc); >> + ret = devm_rtc_register_device(rtc); >> + if (ret) >> + return ret; >> + >> + device_init_wakeup(&pdev->dev, true); >> + return 0; >> } >> >> static const struct platform_device_id bd718x7_rtc_id[] = { >> -- >> 2.34.1 >> > Best, Joe
On 12/12/2024 21:04:34+0900, Joe Hattori wrote: > Hi Alexandre, > > Thank you for your review. > > On 12/12/24 19:34, Alexandre Belloni wrote: > > On 12/12/2024 19:04:03+0900, Joe Hattori wrote: > > > Current code leaves the device's wakeup enabled in the error path of > > > .probe(), which results in a memory leak. Call device_init_wakeup() as > > > the last step in the .probe() to avoid this leak. > > > > Do you have more info on where the memory is allocated? > > device_wakeup_enable() calls wakeup_source_register(), which allocates > struct wakeup_source. Also, when the device is already registered, > wakeup_source_sysfs_add() is called to allocate and register a child device > through wakeup_source_device_create(). If this information needs to be in > the commit message, please let me know and I will include it in the V4 > patch. > > > > > Coudln't we have a devm_ version of device_init_wakeup instead? > > Yes, I think it is possible. However, for this patch, calling > device_init_wakeup() at the end of the .probe() would suffice, and I think > implementing the devm_ function should be in a different patch. Please let > me know if you think otherwise. Well, if I'm going to receive 30 or so patches to fix this, I would prefer moving to a better API at once. > > > > > > > > > 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> > > > --- > > > Changes in V2: > > > - Move the device_init_wakeup() to the last step of the .probe() to make > > > the cleanup simpler. > > > --- > > > drivers/rtc/rtc-bd70528.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c > > > index 954ac4ef53e8..d5cc4993f918 100644 > > > --- a/drivers/rtc/rtc-bd70528.c > > > +++ b/drivers/rtc/rtc-bd70528.c > > > @@ -312,9 +312,6 @@ static int bd70528_probe(struct platform_device *pdev) > > > } > > > } > > > - device_set_wakeup_capable(&pdev->dev, true); > > > - device_wakeup_enable(&pdev->dev); > > > - > > > rtc = devm_rtc_allocate_device(&pdev->dev); > > > if (IS_ERR(rtc)) { > > > dev_err(&pdev->dev, "RTC device creation failed\n"); > > > @@ -331,7 +328,12 @@ static int bd70528_probe(struct platform_device *pdev) > > > if (ret) > > > return ret; > > > - return devm_rtc_register_device(rtc); > > > + ret = devm_rtc_register_device(rtc); > > > + if (ret) > > > + return ret; > > > + > > > + device_init_wakeup(&pdev->dev, true); > > > + return 0; > > > } > > > static const struct platform_device_id bd718x7_rtc_id[] = { > > > -- > > > 2.34.1 > > > > > > > Best, > Joe
Hi Alexandre, On 12/13/24 02:52, Alexandre Belloni wrote: > On 12/12/2024 21:04:34+0900, Joe Hattori wrote: >> Hi Alexandre, >> >> Thank you for your review. >> >> On 12/12/24 19:34, Alexandre Belloni wrote: >>> On 12/12/2024 19:04:03+0900, Joe Hattori wrote: >>>> Current code leaves the device's wakeup enabled in the error path of >>>> .probe(), which results in a memory leak. Call device_init_wakeup() as >>>> the last step in the .probe() to avoid this leak. >>> >>> Do you have more info on where the memory is allocated? >> >> device_wakeup_enable() calls wakeup_source_register(), which allocates >> struct wakeup_source. Also, when the device is already registered, >> wakeup_source_sysfs_add() is called to allocate and register a child device >> through wakeup_source_device_create(). If this information needs to be in >> the commit message, please let me know and I will include it in the V4 >> patch. >> >>> >>> Coudln't we have a devm_ version of device_init_wakeup instead? >> >> Yes, I think it is possible. However, for this patch, calling >> device_init_wakeup() at the end of the .probe() would suffice, and I think >> implementing the devm_ function should be in a different patch. Please let >> me know if you think otherwise. > > Well, if I'm going to receive 30 or so patches to fix this, I would > prefer moving to a better API at once. Makes sense. Just sent a patch to add the devm_ version of device_init_wakeup(dev, true). I CC'd you so please take a look. I also tagged you in the "Suggested-by" tag, so please let me know if you have any concerns. https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/T/#u > >> >>> >>>> >>>> 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> >>>> --- >>>> Changes in V2: >>>> - Move the device_init_wakeup() to the last step of the .probe() to make >>>> the cleanup simpler. >>>> --- >>>> drivers/rtc/rtc-bd70528.c | 10 ++++++---- >>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c >>>> index 954ac4ef53e8..d5cc4993f918 100644 >>>> --- a/drivers/rtc/rtc-bd70528.c >>>> +++ b/drivers/rtc/rtc-bd70528.c >>>> @@ -312,9 +312,6 @@ static int bd70528_probe(struct platform_device *pdev) >>>> } >>>> } >>>> - device_set_wakeup_capable(&pdev->dev, true); >>>> - device_wakeup_enable(&pdev->dev); >>>> - >>>> rtc = devm_rtc_allocate_device(&pdev->dev); >>>> if (IS_ERR(rtc)) { >>>> dev_err(&pdev->dev, "RTC device creation failed\n"); >>>> @@ -331,7 +328,12 @@ static int bd70528_probe(struct platform_device *pdev) >>>> if (ret) >>>> return ret; >>>> - return devm_rtc_register_device(rtc); >>>> + ret = devm_rtc_register_device(rtc); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + device_init_wakeup(&pdev->dev, true); >>>> + return 0; >>>> } >>>> static const struct platform_device_id bd718x7_rtc_id[] = { >>>> -- >>>> 2.34.1 >>>> >>> >> >> Best, >> Joe > Best, Joe
diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c index 954ac4ef53e8..d5cc4993f918 100644 --- a/drivers/rtc/rtc-bd70528.c +++ b/drivers/rtc/rtc-bd70528.c @@ -312,9 +312,6 @@ static int bd70528_probe(struct platform_device *pdev) } } - device_set_wakeup_capable(&pdev->dev, true); - device_wakeup_enable(&pdev->dev); - rtc = devm_rtc_allocate_device(&pdev->dev); if (IS_ERR(rtc)) { dev_err(&pdev->dev, "RTC device creation failed\n"); @@ -331,7 +328,12 @@ static int bd70528_probe(struct platform_device *pdev) if (ret) return ret; - return devm_rtc_register_device(rtc); + ret = devm_rtc_register_device(rtc); + if (ret) + return ret; + + device_init_wakeup(&pdev->dev, true); + return 0; } 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. Call device_init_wakeup() as the last step in the .probe() to avoid this leak. 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> --- Changes in V2: - Move the device_init_wakeup() to the last step of the .probe() to make the cleanup simpler. --- drivers/rtc/rtc-bd70528.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)