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

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

[Joe Hattori]

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(-)

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

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

[Alexandre Belloni]

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
> 

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

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

[Joe Hattori]

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

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

[Matti Vaittinen]

to 12.12.2024 klo 14.04 Joe Hattori (joe@pf.is.s.u-tokyo.ac.jp) kirjoitti:

> >
> > 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,

I agree. But...

> and I
> think implementing the devm_ function should be in a different patch.
> Please let me know if you think otherwise.

... I just took a very quick glance at the first four RTC drivers calling the
device_init_wakeup(). Two of them seemed to potentially leave the
device_init_wakeup() undone on some occasion. (Either in remove(), or
at exit via error path). I know 4 is not a good set of samples, but assuming
it suffices and extrapolating from there it seems there might be a few cases
to check. Having the devm_ variant should be helpful - and this would be
plenty of beans for those who do count them ;)

After that being said, I am not really sure how this is supposed to
work. Is there
ever a valid reason to keep the wakeup capability information around after a
driver is detached? I'd guess the enable/disable of wakeup is usually handled by
the driver, and if driver is gone, do we still need the wakeup stuff?
If we don't, then
it sounds like a thing that [sh/c]ould be automatically handled by the
core, without
the driver having to explicitly remove it. (But as I said, I am not
sure how this is
supposed to work).

Yours,
    -- Matti

-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

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

[Alexandre Belloni]

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

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

[Joe Hattori]

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