Re: [PATCH v3] rtc: bd70528: enable the device's wakeup in the last step of .probe()

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com