[PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume

[PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume

[Hans de Goede]

When not configured for wakeup lis3lv02d_i2c_suspend() will call
lis3lv02d_poweroff() even if the device has already been turned off
by the runtime-suspend handler and if configured for wakeup and
the device is runtime-suspended at this point then it is not turned
back on to serve as a wakeup source.

Before commit b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting
of the reg_ctrl callback"), lis3lv02d_poweroff() failed to disable
the regulators which as a side effect made calling poweroff() twice ok.

Now that poweroff() correctly disables the regulators, doing this twice
triggers a WARN() in the regulator core:

unbalanced disables for regulator-dummy
WARNING: CPU: 1 PID: 92 at drivers/regulator/core.c:2999 _regulator_disable
...

Fix lis3lv02d_i2c_suspend() to not call poweroff() a second time if
already runtime-suspended and add a poweron() call when necessary to
make wakeup work.

lis3lv02d_i2c_resume() has similar issues, with an added weirness that
it always powers on the device if it is runtime suspended, after which
the first runtime-resume will call poweron() again, causing the enabled
count for the regulator to increase by 1 every suspend/resume. These
unbalanced regulator_enable() calls cause the regulator to never
be turned off and trigger the following WARN() on driver unbind:

WARNING: CPU: 1 PID: 1724 at drivers/regulator/core.c:2396 _regulator_put

Fix this by making lis3lv02d_i2c_resume() mirror the new suspend().

Fixes: b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting of the reg_ctrl callback")
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/regressions/5fc6da74-af0a-4aac-b4d5-a000b39a63a5@molgen.mpg.de/
Cc: stable@vger.kernel.org
Cc: regressions@lists.linux.dev
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/misc/lis3lv02d/lis3lv02d_i2c.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
index c6eb27d46cb0..15119584473c 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
@@ -198,8 +198,14 @@ static int lis3lv02d_i2c_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
 
-	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
+	/* Turn on for wakeup if turned off by runtime suspend */
+	if (lis3->pdata && lis3->pdata->wakeup_flags) {
+		if (pm_runtime_suspended(dev))
+			lis3lv02d_poweron(lis3);
+	/* For non wakeup turn off if not already turned off by runtime suspend */
+	} else if (!pm_runtime_suspended(dev))
 		lis3lv02d_poweroff(lis3);
+
 	return 0;
 }
 
@@ -208,13 +214,12 @@ static int lis3lv02d_i2c_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
 
-	/*
-	 * pm_runtime documentation says that devices should always
-	 * be powered on at resume. Pm_runtime turns them off after system
-	 * wide resume is complete.
-	 */
-	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
-		pm_runtime_suspended(dev))
+	/* Turn back off if turned on for wakeup and runtime suspended*/
+	if (lis3->pdata && lis3->pdata->wakeup_flags) {
+		if (pm_runtime_suspended(dev))
+			lis3lv02d_poweroff(lis3);
+	/* For non wakeup turn back on if not runtime suspended */
+	} else if (!pm_runtime_suspended(dev))
 		lis3lv02d_poweron(lis3);
 
 	return 0;
-- 
2.43.0

Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume

[Linux regression tracking (Thorsten Leemhuis)]

On 20.02.24 20:00, Hans de Goede wrote:
> When not configured for wakeup lis3lv02d_i2c_suspend() will call
> lis3lv02d_poweroff() even if the device has already been turned off
> by the runtime-suspend handler and if configured for wakeup and
> the device is runtime-suspended at this point then it is not turned
> back on to serve as a wakeup source.
>
> [...]
>
> Fixes: b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting of the reg_ctrl callback")
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/regressions/5fc6da74-af0a-4aac-b4d5-a000b39a63a5@molgen.mpg.de/
> Cc: stable@vger.kernel.org
> Cc: regressions@lists.linux.dev

Paul, did you maybe test this? I suppose Greg had no time to review this
yet due to all the CVE stuff and stable tree maintenance; but with a bit
of luck a "Tested-by" from your side might motivate him or somebody else
to look into this.

Ciao, Thorsten

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/misc/lis3lv02d/lis3lv02d_i2c.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
> index c6eb27d46cb0..15119584473c 100644
> --- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
> +++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
> @@ -198,8 +198,14 @@ static int lis3lv02d_i2c_suspend(struct device *dev)
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>  
> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
> +	/* Turn on for wakeup if turned off by runtime suspend */
> +	if (lis3->pdata && lis3->pdata->wakeup_flags) {
> +		if (pm_runtime_suspended(dev))
> +			lis3lv02d_poweron(lis3);
> +	/* For non wakeup turn off if not already turned off by runtime suspend */
> +	} else if (!pm_runtime_suspended(dev))
>  		lis3lv02d_poweroff(lis3);
> +
>  	return 0;
>  }
>  
> @@ -208,13 +214,12 @@ static int lis3lv02d_i2c_resume(struct device *dev)
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>  
> -	/*
> -	 * pm_runtime documentation says that devices should always
> -	 * be powered on at resume. Pm_runtime turns them off after system
> -	 * wide resume is complete.
> -	 */
> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
> -		pm_runtime_suspended(dev))
> +	/* Turn back off if turned on for wakeup and runtime suspended*/
> +	if (lis3->pdata && lis3->pdata->wakeup_flags) {
> +		if (pm_runtime_suspended(dev))
> +			lis3lv02d_poweroff(lis3);
> +	/* For non wakeup turn back on if not runtime suspended */
> +	} else if (!pm_runtime_suspended(dev))
>  		lis3lv02d_poweron(lis3);
>  
>  	return 0;

Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume

[Linux regression tracking (Thorsten Leemhuis)]

On 27.02.24 17:25, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 20.02.24 20:00, Hans de Goede wrote:
>> When not configured for wakeup lis3lv02d_i2c_suspend() will call
>> lis3lv02d_poweroff() even if the device has already been turned off
>> by the runtime-suspend handler and if configured for wakeup and
>> the device is runtime-suspended at this point then it is not turned
>> back on to serve as a wakeup source.
>>
>> [...]
>>
>> Fixes: b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting of the reg_ctrl callback")
>> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Closes: https://lore.kernel.org/regressions/5fc6da74-af0a-4aac-b4d5-a000b39a63a5@molgen.mpg.de/
>> Cc: stable@vger.kernel.org
>> Cc: regressions@lists.linux.dev
> 
> Paul, did you maybe test this? I suppose Greg had no time to review this
> yet due to all the CVE stuff and stable tree maintenance; but with a bit
> of luck a "Tested-by" from your side might motivate him or somebody else
> to look into this.

Hmmm, Greg seems to be pretty busy with other stuff. Hans, is there
maybe someone we can motivate into reviewing this to make it easier for
Greg to pick this up and send it to Linus before -rc8/the final?

Sure, it's "just" a warning fix, still would have been nice to get this
into -rc7. But I guess time has already run out on that. :-/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/misc/lis3lv02d/lis3lv02d_i2c.c | 21 +++++++++++++--------
>>  1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
>> index c6eb27d46cb0..15119584473c 100644
>> --- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
>> +++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
>> @@ -198,8 +198,14 @@ static int lis3lv02d_i2c_suspend(struct device *dev)
>>  	struct i2c_client *client = to_i2c_client(dev);
>>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>  
>> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
>> +	/* Turn on for wakeup if turned off by runtime suspend */
>> +	if (lis3->pdata && lis3->pdata->wakeup_flags) {
>> +		if (pm_runtime_suspended(dev))
>> +			lis3lv02d_poweron(lis3);
>> +	/* For non wakeup turn off if not already turned off by runtime suspend */
>> +	} else if (!pm_runtime_suspended(dev))
>>  		lis3lv02d_poweroff(lis3);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -208,13 +214,12 @@ static int lis3lv02d_i2c_resume(struct device *dev)
>>  	struct i2c_client *client = to_i2c_client(dev);
>>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>  
>> -	/*
>> -	 * pm_runtime documentation says that devices should always
>> -	 * be powered on at resume. Pm_runtime turns them off after system
>> -	 * wide resume is complete.
>> -	 */
>> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
>> -		pm_runtime_suspended(dev))
>> +	/* Turn back off if turned on for wakeup and runtime suspended*/
>> +	if (lis3->pdata && lis3->pdata->wakeup_flags) {
>> +		if (pm_runtime_suspended(dev))
>> +			lis3lv02d_poweroff(lis3);
>> +	/* For non wakeup turn back on if not runtime suspended */
>> +	} else if (!pm_runtime_suspended(dev))
>>  		lis3lv02d_poweron(lis3);
>>  
>>  	return 0;
> 
> 

Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume

[Greg Kroah-Hartman]

On Fri, Mar 01, 2024 at 06:20:52AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 27.02.24 17:25, Linux regression tracking (Thorsten Leemhuis) wrote:
> > On 20.02.24 20:00, Hans de Goede wrote:
> >> When not configured for wakeup lis3lv02d_i2c_suspend() will call
> >> lis3lv02d_poweroff() even if the device has already been turned off
> >> by the runtime-suspend handler and if configured for wakeup and
> >> the device is runtime-suspended at this point then it is not turned
> >> back on to serve as a wakeup source.
> >>
> >> [...]
> >>
> >> Fixes: b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting of the reg_ctrl callback")
> >> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> >> Closes: https://lore.kernel.org/regressions/5fc6da74-af0a-4aac-b4d5-a000b39a63a5@molgen.mpg.de/
> >> Cc: stable@vger.kernel.org
> >> Cc: regressions@lists.linux.dev
> > 
> > Paul, did you maybe test this? I suppose Greg had no time to review this
> > yet due to all the CVE stuff and stable tree maintenance; but with a bit
> > of luck a "Tested-by" from your side might motivate him or somebody else
> > to look into this.
> 
> Hmmm, Greg seems to be pretty busy with other stuff. Hans, is there
> maybe someone we can motivate into reviewing this to make it easier for
> Greg to pick this up and send it to Linus before -rc8/the final?
> 
> Sure, it's "just" a warning fix, still would have been nice to get this
> into -rc7. But I guess time has already run out on that. :-/

Sorry for the delay, this ended up at the bottom of my pile.  I'll pick
it up now...

greg k-h

Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume

[Paul Menzel]

Dear Hans,


Thank you for the patch.

Am 20.02.24 um 20:00 schrieb Hans de Goede:
> When not configured for wakeup lis3lv02d_i2c_suspend() will call
> lis3lv02d_poweroff() even if the device has already been turned off
> by the runtime-suspend handler and if configured for wakeup and
> the device is runtime-suspended at this point then it is not turned
> back on to serve as a wakeup source.
> 
> Before commit b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting
> of the reg_ctrl callback"), lis3lv02d_poweroff() failed to disable
> the regulators which as a side effect made calling poweroff() twice ok.
> 
> Now that poweroff() correctly disables the regulators, doing this twice
> triggers a WARN() in the regulator core:
> 
> unbalanced disables for regulator-dummy
> WARNING: CPU: 1 PID: 92 at drivers/regulator/core.c:2999 _regulator_disable
> ...
> 
> Fix lis3lv02d_i2c_suspend() to not call poweroff() a second time if
> already runtime-suspended and add a poweron() call when necessary to
> make wakeup work.
> 
> lis3lv02d_i2c_resume() has similar issues, with an added weirness that
> it always powers on the device if it is runtime suspended, after which
> the first runtime-resume will call poweron() again, causing the enabled
> count for the regulator to increase by 1 every suspend/resume. These
> unbalanced regulator_enable() calls cause the regulator to never
> be turned off and trigger the following WARN() on driver unbind:
> 
> WARNING: CPU: 1 PID: 1724 at drivers/regulator/core.c:2396 _regulator_put
> 
> Fix this by making lis3lv02d_i2c_resume() mirror the new suspend().
> 
> Fixes: b1b9f7a49440 ("misc: lis3lv02d_i2c: Add missing setting of the reg_ctrl callback")
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/regressions/5fc6da74-af0a-4aac-b4d5-a000b39a63a5@molgen.mpg.de/
> Cc: stable@vger.kernel.org
> Cc: regressions@lists.linux.dev
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/misc/lis3lv02d/lis3lv02d_i2c.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
> index c6eb27d46cb0..15119584473c 100644
> --- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
> +++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
> @@ -198,8 +198,14 @@ static int lis3lv02d_i2c_suspend(struct device *dev)
>   	struct i2c_client *client = to_i2c_client(dev);
>   	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>   
> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
> +	/* Turn on for wakeup if turned off by runtime suspend */
> +	if (lis3->pdata && lis3->pdata->wakeup_flags) {
> +		if (pm_runtime_suspended(dev))
> +			lis3lv02d_poweron(lis3);
> +	/* For non wakeup turn off if not already turned off by runtime suspend */
> +	} else if (!pm_runtime_suspended(dev))
>   		lis3lv02d_poweroff(lis3);
> +
>   	return 0;
>   }
>   
> @@ -208,13 +214,12 @@ static int lis3lv02d_i2c_resume(struct device *dev)
>   	struct i2c_client *client = to_i2c_client(dev);
>   	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>   
> -	/*
> -	 * pm_runtime documentation says that devices should always
> -	 * be powered on at resume. Pm_runtime turns them off after system
> -	 * wide resume is complete.
> -	 */
> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
> -		pm_runtime_suspended(dev))
> +	/* Turn back off if turned on for wakeup and runtime suspended*/
> +	if (lis3->pdata && lis3->pdata->wakeup_flags) {
> +		if (pm_runtime_suspended(dev))
> +			lis3lv02d_poweroff(lis3);
> +	/* For non wakeup turn back on if not runtime suspended */
> +	} else if (!pm_runtime_suspended(dev))
>   		lis3lv02d_poweron(lis3);
>   
>   	return 0;

I applied this commit on top of Linus’ master branch, and successfully 
tested with S0ix and ACPI S3, that the warning is gone.

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de> # Dell XPS 15 7590

Looking at the diff, this also looks good. Thank you for writing the 
helpful commit message.

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Thank you for addressing this so quickly, and sorry for the late reply.


Kind regards,

Paul