Received-SPF: pass (google.com: domain of linux-kernel+bounces-87912-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d2465271-1e4b-4bae-9399-4d49d3938048@leemhuis.info>
Date: Fri, 1 Mar 2024 06:20:52 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators
 getting en-/dis-abled twice on suspend/resume
Content-Language: en-US, de-DE
From: "Linux regression tracking (Thorsten Leemhuis)"
 <regressions@leemhuis.info>
To: Hans de Goede <hdegoede@redhat.com>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>, Eric Piel <eric.piel@tremplin-utc.net>,
 linux-kernel@vger.kernel.org, Paul Menzel <pmenzel@molgen.mpg.de>,
 stable@vger.kernel.org, regressions@lists.linux.dev
Reply-To: Linux regressions mailing list <regressions@lists.linux.dev>,
 Linux regressions mailing list <regressions@lists.linux.dev>
References: <20240220190035.53402-1-hdegoede@redhat.com>
 <1d8226cd-df43-4ef6-8425-2db01d513b32@leemhuis.info>
In-Reply-To: <1d8226cd-df43-4ef6-8425-2db01d513b32@leemhuis.info>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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