linux-kernel - Re: [PATCH] pinctrl: amd: Take suspend type into consideration which pins are non-wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <36e47f2c-a8d4-4ad2-8f7e-60fa0b2787e8@maciej.szmigiero.name>
Date: Mon, 6 Jan 2025 10:51:54 +0100
From: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To: Mario Limonciello <mario.limonciello@....com>,
 Basavaraj Natikar <Basavaraj.Natikar@....com>,
 Shyam Sundar S K <Shyam-sundar.S-k@....com>
Cc: Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pinctrl: amd: Take suspend type into consideration which
 pins are non-wake

On 6.01.2025 04:18, Mario Limonciello wrote:
> On 12/29/2024 10:42, Maciej S. Szmigiero wrote:
>> Some laptops have pins which are a wake source for S0i3/S3 but which
>> aren't a wake source for S4/S5 and which cause issues when left unmasked
>> during hibernation (S4).
>>
>> For example HP EliteBook 855 G7 has pin #24 that causes instant wakeup
>> (hibernation failure) if left unmasked (it is a wake source only for
>> S0i3/S3).
> 
> On your machine do you know what pin 24 is connected to?

Yes, it's connected to WWAN modem, since this GPIO pin triggers wake notify
to this modem's parent PCIe port:
 From \_SB.GPIO._EVT:
Case (0x18)
{
	MSTP (0x3918)
	Notify (\_SB.PCI0.GP12, 0x02) // Device Wake
}

WWAN modem is _SB.PCI0.GP12.PWAN, there's no other device under _SB.PCI0.GP12.

Will add this note to v2 commit message.

> If not, can you run https://gitlab.freedesktop.org/drm/amd/-/blob/master/scripts/amd_s2idle.py and share the text report it generates to me?
> 
> I'll see if I can make sense in that report what it's most likely connected to.
> 
> Just want to make sure we're not papering over an issue in another component by making this change.

I think if firmware wants some pins as a wake source just for S4/S5
then it still should be respected.

>>
>> Fix this by considering a pin a wake source only if it is marked as one
>> for the current suspend type (S0i3/S3 vs S4/S5).
>>
>> Since I'm not sure if Z-wake pins should be included in either suspend
>> category I excluded them from both, so pins with only the Z-wake flag set
>> are treated as non-wake pins.
> 
> Z only makes sense at runtime.  As long as it's restored to previous value after exiting suspend or hibernate that should be totally fine.

Ack.

>>
>> Fixes: 2fff0b5e1a6b ("pinctrl: amd: Mask non-wake source pins with interrupt enabled at suspend")
>> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
>> ---
>>   drivers/pinctrl/pinctrl-amd.c | 27 +++++++++++++++++++++------
>>   drivers/pinctrl/pinctrl-amd.h |  7 +++----
>>   2 files changed, 24 insertions(+), 10 deletions(-)
>>
(..)
>> diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
>> index 667be49c3f48..8bf9f410d7fb 100644
>> --- a/drivers/pinctrl/pinctrl-amd.h
>> +++ b/drivers/pinctrl/pinctrl-amd.h
>> @@ -80,10 +80,9 @@
>>   #define FUNCTION_MASK        GENMASK(1, 0)
>>   #define FUNCTION_INVALID    GENMASK(7, 0)
>> -#define WAKE_SOURCE    (BIT(WAKE_CNTRL_OFF_S0I3) | \
>> -             BIT(WAKE_CNTRL_OFF_S3)   | \
>> -             BIT(WAKE_CNTRL_OFF_S4)   | \
>> -             BIT(WAKECNTRL_Z_OFF))
>> +#define WAKE_SOURCE_S03 (BIT(WAKE_CNTRL_OFF_S0I3) | \
>> +             BIT(WAKE_CNTRL_OFF_S3))
>> +#define WAKE_SOURCE_S4  BIT(WAKE_CNTRL_OFF_S4)
> 
> Since s03 doesn't make sense and s0i3 is wrong for s3 and s3 is wrong for s0i3 as a personal preference I would just call it
> 
> WAKE_SOURCE_SUSPEND
> WAKE_SOURCE_HIBERNATE
> 

Will change them accordingly.

Thanks,
Maciej