Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

[Mario Limonciello]

On 4/30/26 22:26, Daniel Gibson wrote:
> [You don't often get email from daniel@gibson.sh. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Enabling it delays suspend for 2.5 seconds which is known to help for
> some AMD-based Lenovo Laptops that otherwise failed to send/receive
> events for key presses or the lid switch after s2idle.
> Apparently the EC needs to do some things in the background before
> suspend or it gets into a bad state.
> 
> There are many reports of AMD-based laptops (mostly but not exclusively
> IdeaPads) about similar issues on the web; this parameter gives
> affected users an easy way to try out if their issues have the same
> root cause and to work around them until their specific device is added
> to the quirks list.
> I added a note to the parameter description encouraging users to report
> their device so it can be added to the quirks list, inspired by a
> similar request in parameter descriptions of the ideapad-laptop module.
> 
> The module parameter can be set to "1" to explicitly enable it,
> "0" to disable it even on devices that are assumed to be affected,
> or -1 (the default) to enable it if the device is assumed to be affected
> (according to fwbug_list[])
> 
> This is related to https://bugzilla.kernel.org/show_bug.cgi?id=221383
> 
> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
> Tested-by: Daniel Gibson <daniel@gibson.sh>
> ---
>   drivers/platform/x86/amd/pmc/pmc.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index c604dc7207ed..f76936036d1f 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -89,6 +89,11 @@ static bool disable_workarounds;
>   module_param(disable_workarounds, bool, 0644);
>   MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs");
> 
> +static int delay_suspend = -1;
> +module_param(delay_suspend, int, 0644);
> +MODULE_PARM_DESC(delay_suspend,
> +                "Delays s2idle by 2.5 seconds to work around buggy ECs, often causing keyboard issues after suspend. 0: don't delay, 1: do delay, -1 (default): let amd_pmc decide. If you need this please report this to: platform-driver-x86@vger.kernel.org");
> +
>   static struct amd_pmc_dev pmc;

Generally speaking; I don't like new module parameters.

Since this is purely for debugging purposes and is going to hopefully 
lead to a new patch; how about if it's done in debugfs?

I have a couple reasons I say this.

1. If you make it "too easy" to make the kernel command line permanent 
people won't report bugs and they never get fixed.

2. It needs to become ABI essentially forever, even if we found out some 
day this was actually something we can fix with other kernel changes 
instead.

But then the problem arguing in the direction of a module parameter is 
discoverability of this debugging knob.  And for that; I would suggest 
to add a bullet here: https://docs.kernel.org/arch/x86/amd-debugging.html

Then when people report this problem we can point them to that document 
indicating they can use it.

> 
>   static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
> @@ -634,11 +639,19 @@ static void amd_pmc_s2idle_check(void)
>          struct amd_pmc_dev *pdev = &pmc;
>          struct smu_metrics table;
>          int rc;
> -       bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
> +       bool ec_needs_sleep;
> +
> +       if (delay_suspend < 0)
> +               ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
> +       else
> +               ec_needs_sleep = delay_suspend != 0;
> 
>          /* Avoid triggering OVP */
>          if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {
> -               dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
> +               if (delay_suspend > 0)
> +                       dev_info(pdev->dev, "Delaying suspend by 2.5s because delay_suspend=1\n");

Rather than telling them to report to platform-driver-x86@ in the module 
parameter description I would suggest you should be putting it in this 
message.  I think that's what some other drivers do when they want 
people to report bugs.

> +               else
> +                       dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>                  msleep(2500);
>          }
> 
> --
> 2.48.1
> 

Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

[Daniel Gibson]

On 04.05.26 16:37, Mario Limonciello wrote:
> On 4/30/26 22:26, Daniel Gibson wrote:
(...)
>> +static int delay_suspend = -1;
>> +module_param(delay_suspend, int, 0644);
>> +MODULE_PARM_DESC(delay_suspend,
>> +                "Delays s2idle by 2.5 seconds to work around buggy
>> ECs, often causing keyboard issues after suspend. 0: don't delay, 1:
>> do delay, -1 (default): let amd_pmc decide. If you need this please
>> report this to: platform-driver-x86@vger.kernel.org");
>> +
>>   static struct amd_pmc_dev pmc;
> 
> Generally speaking; I don't like new module parameters.
> 
> Since this is purely for debugging purposes and is going to hopefully
> lead to a new patch; how about if it's done in debugfs?
> 
> I have a couple reasons I say this.
> 
> 1. If you make it "too easy" to make the kernel command line permanent
> people won't report bugs and they never get fixed.

Sorry, I don't think this makes a difference.
Even people using a bleeding-edge distro like Arch Linux will have to
wait at least a month or so after reporting their device until a stable
kernel with that quirk is released and that kernel lands in their
distro. Much longer for users of more traditional distros.

No one wants to have a broken laptop for months if it can be avoided, so
they'd make that workaround permanent either way, it's just more
annoying to do when it's in debugfs.

If someone reports the bug it's because they think it's the right thing
to do (and maybe because the module parameter or dmesg message asked
them to), not because the workaround requires a systemd unit instead of
a kernel parameter.

> 
> 2. It needs to become ABI essentially forever, even if we found out some
> day this was actually something we can fix with other kernel changes
> instead.

Does it? prefer_microsoft_dsm_guid was removed, did that break anything?

> 
> But then the problem arguing in the direction of a module parameter is
> discoverability of this debugging knob.  And for that; I would suggest
> to add a bullet here: https://docs.kernel.org/arch/x86/amd-debugging.html
> 
> Then when people report this problem we can point them to that document
> indicating they can use it.

So far I've missed that document - it certainly looks like a good place
to document the issue and workaround.

Side-Note: "18.10. Random reboot issues" sounds interesting, I guess it
would be helpful to document how that value can be read or identified in
dmesg ;)

> 
>>
>>   static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int
>> reg_offset)
>> @@ -634,11 +639,19 @@ static void amd_pmc_s2idle_check(void)
>>          struct amd_pmc_dev *pdev = &pmc;
>>          struct smu_metrics table;
>>          int rc;
>> -       bool ec_needs_sleep = !disable_workarounds &&
>> amd_pmc_quirk_need_suspend_delay(pdev);
>> +       bool ec_needs_sleep;
>> +
>> +       if (delay_suspend < 0)
>> +               ec_needs_sleep = !disable_workarounds &&
>> amd_pmc_quirk_need_suspend_delay(pdev);
>> +       else
>> +               ec_needs_sleep = delay_suspend != 0;
>>
>>          /* Avoid triggering OVP */
>>          if (ec_needs_sleep || (!get_metrics_table(pdev, &table) &&
>> table.s0i3_last_entry_status)) {
>> -               dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid
>> platform bug\n");
>> +               if (delay_suspend > 0)
>> +                       dev_info(pdev->dev, "Delaying suspend by 2.5s
>> because delay_suspend=1\n");
> 
> Rather than telling them to report to platform-driver-x86@ in the module
> parameter description I would suggest you should be putting it in this
> message.  I think that's what some other drivers do when they want
> people to report bugs.

Great idea!

> 
>> +               else
>> +                       dev_info(pdev->dev, "Delaying suspend by 2.5s
>> to avoid platform bug\n");
>>                  msleep(2500);
>>          }
>>
>> -- 
>> 2.48.1
>>
> 

Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

[Mario Limonciello]

On 5/4/26 10:38, Daniel Gibson wrote:
> On 04.05.26 16:37, Mario Limonciello wrote:
>> On 4/30/26 22:26, Daniel Gibson wrote:
> (...)
>>> +static int delay_suspend = -1;
>>> +module_param(delay_suspend, int, 0644);
>>> +MODULE_PARM_DESC(delay_suspend,
>>> +                "Delays s2idle by 2.5 seconds to work around buggy
>>> ECs, often causing keyboard issues after suspend. 0: don't delay, 1:
>>> do delay, -1 (default): let amd_pmc decide. If you need this please
>>> report this to: platform-driver-x86@vger.kernel.org");
>>> +
>>>    static struct amd_pmc_dev pmc;
>>
>> Generally speaking; I don't like new module parameters.
>>
>> Since this is purely for debugging purposes and is going to hopefully
>> lead to a new patch; how about if it's done in debugfs?
>>
>> I have a couple reasons I say this.
>>
>> 1. If you make it "too easy" to make the kernel command line permanent
>> people won't report bugs and they never get fixed.
> 
> Sorry, I don't think this makes a difference.

OK.  I'd say up to the driver maintainers to decide which way to go.

> Even people using a bleeding-edge distro like Arch Linux will have to
> wait at least a month or so after reporting their device until a stable
> kernel with that quirk is released and that kernel lands in their
> distro. Much longer for users of more traditional distros.
> 
> No one wants to have a broken laptop for months if it can be avoided, so
> they'd make that workaround permanent either way, it's just more
> annoying to do when it's in debugfs.
> 
> If someone reports the bug it's because they think it's the right thing
> to do (and maybe because the module parameter or dmesg message asked
> them to), not because the workaround requires a systemd unit instead of
> a kernel parameter.
> 
>>
>> 2. It needs to become ABI essentially forever, even if we found out some
>> day this was actually something we can fix with other kernel changes
>> instead.
> 
> Does it? prefer_microsoft_dsm_guid was removed, did that break anything?

Very good point.

> 
>>
>> But then the problem arguing in the direction of a module parameter is
>> discoverability of this debugging knob.  And for that; I would suggest
>> to add a bullet here: https://docs.kernel.org/arch/x86/amd-debugging.html
>>
>> Then when people report this problem we can point them to that document
>> indicating they can use it.
> 
> So far I've missed that document - it certainly looks like a good place
> to document the issue and workaround.

Yeah; whatever direction is agreed upon (module parameter or debugfs), 
please update the document for a section about it.

> 
> Side-Note: "18.10. Random reboot issues" sounds interesting, I guess it
> would be helpful to document how that value can be read or identified in
> dmesg ;)

"This information is read by the kernel at bootup and printed into the 
syslog".

Do you mean actually outputting the prefix of the message in this document?

This is a sample message from my local machine.

x86/amd: Previous system reset reason [0x00080800]: software wrote 0x6 
to reset control register 0xCF9

I have no qualms with this - feel free to send a patch if you're 
touching it already anyways.

> 
>>
>>>
>>>    static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int
>>> reg_offset)
>>> @@ -634,11 +639,19 @@ static void amd_pmc_s2idle_check(void)
>>>           struct amd_pmc_dev *pdev = &pmc;
>>>           struct smu_metrics table;
>>>           int rc;
>>> -       bool ec_needs_sleep = !disable_workarounds &&
>>> amd_pmc_quirk_need_suspend_delay(pdev);
>>> +       bool ec_needs_sleep;
>>> +
>>> +       if (delay_suspend < 0)
>>> +               ec_needs_sleep = !disable_workarounds &&
>>> amd_pmc_quirk_need_suspend_delay(pdev);
>>> +       else
>>> +               ec_needs_sleep = delay_suspend != 0;
>>>
>>>           /* Avoid triggering OVP */
>>>           if (ec_needs_sleep || (!get_metrics_table(pdev, &table) &&
>>> table.s0i3_last_entry_status)) {
>>> -               dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid
>>> platform bug\n");
>>> +               if (delay_suspend > 0)
>>> +                       dev_info(pdev->dev, "Delaying suspend by 2.5s
>>> because delay_suspend=1\n");
>>
>> Rather than telling them to report to platform-driver-x86@ in the module
>> parameter description I would suggest you should be putting it in this
>> message.  I think that's what some other drivers do when they want
>> people to report bugs.
> 
> Great idea!
> 
>>
>>> +               else
>>> +                       dev_info(pdev->dev, "Delaying suspend by 2.5s
>>> to avoid platform bug\n");
>>>                   msleep(2500);
>>>           }
>>>
>>> -- 
>>> 2.48.1
>>>
>>
> 

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Ilpo Järvinen]

On Fri, 1 May 2026, Daniel Gibson wrote:

> Some IdeaPad Slim 3 devices and similar with AMD CPUs have a
> nonfunctional keyboard and lid switch after s2idle.
> It helps to delay suspend by 2.5 seconds so the EC has some time
> to do whatever it needs to get done before suspend.
> 
> This issue has been reported for many different devices, this patch
> has been tested with the Zen3-based IdeaPad Slim 3 16ABR8 (82XR)
> and the Zen3+-based IdeaPad Slim 3 14ARP10 (83K6), see
> https://bugzilla.kernel.org/show_bug.cgi?id=221383
> 
> Reported-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221383
> Tested-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
> Tested-by: Daniel Gibson <daniel@gibson.sh>
> Suggested-by: Mario Limonciello (AMD) <superm1@kernel.org>
> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
> ---
>  drivers/platform/x86/amd/pmc/pmc-quirks.c | 36 +++++++++++++++++++++++
>  drivers/platform/x86/amd/pmc/pmc.c        |  5 +++-
>  drivers/platform/x86/amd/pmc/pmc.h        |  1 +
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc-quirks.c b/drivers/platform/x86/amd/pmc/pmc-quirks.c
> index 24506e342943..cea30f68f8dc 100644
> --- a/drivers/platform/x86/amd/pmc/pmc-quirks.c
> +++ b/drivers/platform/x86/amd/pmc/pmc-quirks.c
> @@ -18,6 +18,7 @@
>  struct quirk_entry {
>  	u32 s2idle_bug_mmio;
>  	bool spurious_8042;
> +	bool need_suspend_delay;
>  };
>  
>  static struct quirk_entry quirk_s2idle_bug = {
> @@ -33,6 +34,10 @@ static struct quirk_entry quirk_s2idle_spurious_8042 = {
>  	.spurious_8042 = true,
>  };
>  
> +static struct quirk_entry quirk_s2idle_need_suspend_delay = {
> +	.need_suspend_delay = true

Please add comma to any non-terminating entry.

> +};
> +
>  static const struct dmi_system_id fwbug_list[] = {
>  	{
>  		.ident = "L14 Gen2 AMD",
> @@ -203,6 +208,32 @@ static const struct dmi_system_id fwbug_list[] = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "82XQ"),
>  		}
>  	},
> +	/*
> +	 * Some Lenovo Laptops (like different IdeaPad 3 Slims) need some
> +	 * me-time before sleeping or they get uncooperative after waking
> +	 * up and don't send events for keyboard and lid switch anymore.
> +	 * See https://bugzilla.kernel.org/show_bug.cgi?id=221383
> +	 */
> +	{
> +		.ident = "Zen3-based IdeaPad Slim and similar",
> +		.driver_data = &quirk_s2idle_need_suspend_delay,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
> +			/*
> +			 * Note: there are also some Zen2-based 82X* devices that
> +			 * need different quirks, they're already handled above
> +			 */
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82X")

Ditto.

> +		}
> +	},
> +	{
> +		.ident = "Zen3+-based IdeaPad Slim and similar",
> +		.driver_data = &quirk_s2idle_need_suspend_delay,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "83K")

Ditto.

> +		}
> +	},
>  	/* https://bugzilla.kernel.org/show_bug.cgi?id=221273 */
>  	{
>  		.ident = "Thinkpad L14 Gen3",
> @@ -356,6 +387,11 @@ void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
>  		amd_pmc_skip_nvme_smi_handler(dev->quirks->s2idle_bug_mmio);
>  }
>  
> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev)
> +{
> +	return dev->quirks->need_suspend_delay;
> +}
> +
>  void amd_pmc_quirks_init(struct amd_pmc_dev *dev)
>  {
>  	const struct dmi_system_id *dmi_id;
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index cae3fcafd4d7..c604dc7207ed 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -634,10 +634,13 @@ static void amd_pmc_s2idle_check(void)
>  	struct amd_pmc_dev *pdev = &pmc;
>  	struct smu_metrics table;
>  	int rc;
> +	bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);

Why isn't disable_workarounds inside amd_pmc_quirk_need_suspend_delay() ?

>  
>  	/* Avoid triggering OVP */
> -	if (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)
> +	if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {

It would be clearer what's going on here if these two checks are clearly 
separated.

As is, the comment confuses if it's only meant for the 2nd check.

One of the checks will be in amd_pmc_quirk_need_suspend_delay() and the 
other (the existing one + the comment) should be moved in to own function 
(perhaps in a patch preceeding this one) so it's clear the reasoning is 
different(?). You can check the commit that introduced msleep(2500) here
for details about the existing check.

> +		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>  		msleep(2500);
> +	}
>  
>  	/* Dump the IdleMask before we add to the STB */
>  	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index fe3f53eb5955..f5257e47b8c4 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -147,6 +147,7 @@ enum amd_pmc_def {
>  };
>  
>  void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev);
>  void amd_pmc_quirks_init(struct amd_pmc_dev *dev);
>  void amd_mp2_stb_init(struct amd_pmc_dev *dev);
>  void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
> 

-- 
 i.

Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

[Ilpo Järvinen]

On Fri, 1 May 2026, Daniel Gibson wrote:

> Enabling it delays suspend for 2.5 seconds which is known to help for

Please don't link shortlog to longer description like this. Write them 
such that either can be read entirely on its own.

> some AMD-based Lenovo Laptops that otherwise failed to send/receive
> events for key presses or the lid switch after s2idle.
> Apparently the EC needs to do some things in the background before
> suspend or it gets into a bad state.
> 
> There are many reports of AMD-based laptops (mostly but not exclusively
> IdeaPads) about similar issues on the web; this parameter gives
> affected users an easy way to try out if their issues have the same
> root cause and to work around them until their specific device is added
> to the quirks list.
> I added a note to the parameter description encouraging users to report
> their device so it can be added to the quirks list, inspired by a
> similar request in parameter descriptions of the ideapad-laptop module.
> 
> The module parameter can be set to "1" to explicitly enable it,
> "0" to disable it even on devices that are assumed to be affected,
> or -1 (the default) to enable it if the device is assumed to be affected
> (according to fwbug_list[])
> 
> This is related to https://bugzilla.kernel.org/show_bug.cgi?id=221383

Link: ...

> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
> Tested-by: Daniel Gibson <daniel@gibson.sh>

Normally, we don't tag our own testing. :-)

> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index c604dc7207ed..f76936036d1f 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -89,6 +89,11 @@ static bool disable_workarounds;
>  module_param(disable_workarounds, bool, 0644);
>  MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs");
>  
> +static int delay_suspend = -1;
> +module_param(delay_suspend, int, 0644);
> +MODULE_PARM_DESC(delay_suspend,
> +		 "Delays s2idle by 2.5 seconds to work around buggy ECs, often causing keyboard issues after suspend. 0: don't delay, 1: do delay, -1 (default): let amd_pmc decide. If you need this please report this to: platform-driver-x86@vger.kernel.org");
> +
>  static struct amd_pmc_dev pmc;
>  
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
> @@ -634,11 +639,19 @@ static void amd_pmc_s2idle_check(void)
>  	struct amd_pmc_dev *pdev = &pmc;
>  	struct smu_metrics table;
>  	int rc;
> -	bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
> +	bool ec_needs_sleep;
> +
> +	if (delay_suspend < 0)
> +		ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
> +	else
> +		ec_needs_sleep = delay_suspend != 0;

By doing this, you added overlap between disable_workarounds and 
delay_suspend. It gets confusing.

>  	/* Avoid triggering OVP */
>  	if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {
> -		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
> +		if (delay_suspend > 0)
> +			dev_info(pdev->dev, "Delaying suspend by 2.5s because delay_suspend=1\n");
> +		else
> +			dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");

Overall, the end result looks pretty messy.


-- 
 i.

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Daniel Gibson]

Thanks for the review!

On 07.05.26 15:22, Ilpo Järvinen wrote:
> On Fri, 1 May 2026, Daniel Gibson wrote:
> 
>> Some IdeaPad Slim 3 devices and similar with AMD CPUs have a
>> nonfunctional keyboard and lid switch after s2idle.
>> It helps to delay suspend by 2.5 seconds so the EC has some time
>> to do whatever it needs to get done before suspend.
>>
>> This issue has been reported for many different devices, this patch
>> has been tested with the Zen3-based IdeaPad Slim 3 16ABR8 (82XR)
>> and the Zen3+-based IdeaPad Slim 3 14ARP10 (83K6), see
>> https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>
>> Reported-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221383
>> Tested-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>> Tested-by: Daniel Gibson <daniel@gibson.sh>
>> Suggested-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
>> ---
>>  drivers/platform/x86/amd/pmc/pmc-quirks.c | 36 +++++++++++++++++++++++
>>  drivers/platform/x86/amd/pmc/pmc.c        |  5 +++-
>>  drivers/platform/x86/amd/pmc/pmc.h        |  1 +
>>  3 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc-quirks.c b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>> index 24506e342943..cea30f68f8dc 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc-quirks.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>> @@ -18,6 +18,7 @@
>>  struct quirk_entry {
>>  	u32 s2idle_bug_mmio;
>>  	bool spurious_8042;
>> +	bool need_suspend_delay;
>>  };
>>  
>>  static struct quirk_entry quirk_s2idle_bug = {
>> @@ -33,6 +34,10 @@ static struct quirk_entry quirk_s2idle_spurious_8042 = {
>>  	.spurious_8042 = true,
>>  };
>>  
>> +static struct quirk_entry quirk_s2idle_need_suspend_delay = {
>> +	.need_suspend_delay = true
> 
> Please add comma to any non-terminating entry.

will do

> 
>> +};
>> +
>>  static const struct dmi_system_id fwbug_list[] = {
>>  	{
>>  		.ident = "L14 Gen2 AMD",
>> @@ -203,6 +208,32 @@ static const struct dmi_system_id fwbug_list[] = {
>>  			DMI_MATCH(DMI_PRODUCT_NAME, "82XQ"),
>>  		}
>>  	},
>> +	/*
>> +	 * Some Lenovo Laptops (like different IdeaPad 3 Slims) need some
>> +	 * me-time before sleeping or they get uncooperative after waking
>> +	 * up and don't send events for keyboard and lid switch anymore.
>> +	 * See https://bugzilla.kernel.org/show_bug.cgi?id=221383
>> +	 */
>> +	{
>> +		.ident = "Zen3-based IdeaPad Slim and similar",
>> +		.driver_data = &quirk_s2idle_need_suspend_delay,
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>> +			/*
>> +			 * Note: there are also some Zen2-based 82X* devices that
>> +			 * need different quirks, they're already handled above
>> +			 */
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "82X")
> 
> Ditto.
> 
>> +		}
>> +	},
>> +	{
>> +		.ident = "Zen3+-based IdeaPad Slim and similar",
>> +		.driver_data = &quirk_s2idle_need_suspend_delay,
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "83K")
> 
> Ditto.
> 
>> +		}
>> +	},
>>  	/* https://bugzilla.kernel.org/show_bug.cgi?id=221273 */
>>  	{
>>  		.ident = "Thinkpad L14 Gen3",
>> @@ -356,6 +387,11 @@ void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
>>  		amd_pmc_skip_nvme_smi_handler(dev->quirks->s2idle_bug_mmio);
>>  }
>>  
>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev)
>> +{
>> +	return dev->quirks->need_suspend_delay;
>> +}
>> +
>>  void amd_pmc_quirks_init(struct amd_pmc_dev *dev)
>>  {
>>  	const struct dmi_system_id *dmi_id;
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index cae3fcafd4d7..c604dc7207ed 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -634,10 +634,13 @@ static void amd_pmc_s2idle_check(void)
>>  	struct amd_pmc_dev *pdev = &pmc;
>>  	struct smu_metrics table;
>>  	int rc;
>> +	bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
> 
> Why isn't disable_workarounds inside amd_pmc_quirk_need_suspend_delay() ?

Because disable_workarounds is a static variable in pmc.c and
amd_pmc_quirk_need_suspend_delay() is implemented in pmc-quirks.c

> 
>>  
>>  	/* Avoid triggering OVP */
>> -	if (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)
>> +	if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {
> 
> It would be clearer what's going on here if these two checks are clearly 
> separated.
> 
> As is, the comment confuses if it's only meant for the 2nd check.
> 
> One of the checks will be in amd_pmc_quirk_need_suspend_delay() and the 
> other (the existing one + the comment) should be moved in to own function 
> (perhaps in a patch preceeding this one) so it's clear the reasoning is 
> different(?). You can check the commit that introduced msleep(2500) here
> for details about the existing check.
> 

Ok, I'll move that check for the existing workaround into its own 
function to make it more clear what is happening

>> +		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>>  		msleep(2500);
>> +	}
>>  
>>  	/* Dump the IdleMask before we add to the STB */
>>  	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
>> index fe3f53eb5955..f5257e47b8c4 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.h
>> +++ b/drivers/platform/x86/amd/pmc/pmc.h
>> @@ -147,6 +147,7 @@ enum amd_pmc_def {
>>  };
>>  
>>  void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev);
>>  void amd_pmc_quirks_init(struct amd_pmc_dev *dev);
>>  void amd_mp2_stb_init(struct amd_pmc_dev *dev);
>>  void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>>
> 

Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

[Daniel Gibson]

On 07.05.26 15:33, Ilpo Järvinen wrote:
> On Fri, 1 May 2026, Daniel Gibson wrote:
> 
>> Enabling it delays suspend for 2.5 seconds which is known to help for
> 
> Please don't link shortlog to longer description like this. Write them 
> such that either can be read entirely on its own.
> 

ok

>> some AMD-based Lenovo Laptops that otherwise failed to send/receive
>> events for key presses or the lid switch after s2idle.
>> Apparently the EC needs to do some things in the background before
>> suspend or it gets into a bad state.
>>
>> There are many reports of AMD-based laptops (mostly but not exclusively
>> IdeaPads) about similar issues on the web; this parameter gives
>> affected users an easy way to try out if their issues have the same
>> root cause and to work around them until their specific device is added
>> to the quirks list.
>> I added a note to the parameter description encouraging users to report
>> their device so it can be added to the quirks list, inspired by a
>> similar request in parameter descriptions of the ideapad-laptop module.
>>
>> The module parameter can be set to "1" to explicitly enable it,
>> "0" to disable it even on devices that are assumed to be affected,
>> or -1 (the default) to enable it if the device is assumed to be affected
>> (according to fwbug_list[])
>>
>> This is related to https://bugzilla.kernel.org/show_bug.cgi?id=221383
> 
> Link: ...
> 
>> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
>> Tested-by: Daniel Gibson <daniel@gibson.sh>
> 
> Normally, we don't tag our own testing. :-)

Ok, will change, I didn't know this - I thought at least in case of
hardware-specific changes it's not clear if the author of a patch even
has the hardware to test it, so it's not necessarily implied.

> 
>> ---
>>  drivers/platform/x86/amd/pmc/pmc.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index c604dc7207ed..f76936036d1f 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -89,6 +89,11 @@ static bool disable_workarounds;
>>  module_param(disable_workarounds, bool, 0644);
>>  MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs");
>>  
>> +static int delay_suspend = -1;
>> +module_param(delay_suspend, int, 0644);
>> +MODULE_PARM_DESC(delay_suspend,
>> +		 "Delays s2idle by 2.5 seconds to work around buggy ECs, often causing keyboard issues after suspend. 0: don't delay, 1: do delay, -1 (default): let amd_pmc decide. If you need this please report this to: platform-driver-x86@vger.kernel.org");
>> +
>>  static struct amd_pmc_dev pmc;
>>  
>>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
>> @@ -634,11 +639,19 @@ static void amd_pmc_s2idle_check(void)
>>  	struct amd_pmc_dev *pdev = &pmc;
>>  	struct smu_metrics table;
>>  	int rc;
>> -	bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
>> +	bool ec_needs_sleep;
>> +
>> +	if (delay_suspend < 0)
>> +		ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
>> +	else
>> +		ec_needs_sleep = delay_suspend != 0;
> 
> By doing this, you added overlap between disable_workarounds and 
> delay_suspend. It gets confusing.

I think this confusion primarily stems from having one variable
(disable_workarounds) that disables all quirks, instead of a way
to individually disable (or enable/enforce) them.

But disable_workarounds is already there so I try to behave consistently.

> 
>>  	/* Avoid triggering OVP */
>>  	if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {
>> -		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>> +		if (delay_suspend > 0)
>> +			dev_info(pdev->dev, "Delaying suspend by 2.5s because delay_suspend=1\n");
>> +		else
>> +			dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
> 
> Overall, the end result looks pretty messy.
> 

I agree. I will try to clean this up together with the separating of the
checks you requested for the other patch.

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Daniel Gibson]

On 07.05.26 22:19, Daniel Gibson wrote:
> Thanks for the review!
> 
> On 07.05.26 15:22, Ilpo Järvinen wrote:
>> On Fri, 1 May 2026, Daniel Gibson wrote:
>>
>>> Some IdeaPad Slim 3 devices and similar with AMD CPUs have a
>>> nonfunctional keyboard and lid switch after s2idle.
>>> It helps to delay suspend by 2.5 seconds so the EC has some time
>>> to do whatever it needs to get done before suspend.
>>>
>>> This issue has been reported for many different devices, this patch
>>> has been tested with the Zen3-based IdeaPad Slim 3 16ABR8 (82XR)
>>> and the Zen3+-based IdeaPad Slim 3 14ARP10 (83K6), see
>>> https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>
>>> Reported-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>> Tested-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>>> Tested-by: Daniel Gibson <daniel@gibson.sh>
>>> Suggested-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
>>> ---
>>>  drivers/platform/x86/amd/pmc/pmc-quirks.c | 36 +++++++++++++++++++++++
>>>  drivers/platform/x86/amd/pmc/pmc.c        |  5 +++-
>>>  drivers/platform/x86/amd/pmc/pmc.h        |  1 +
>>>  3 files changed, 41 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmc/pmc-quirks.c b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>> index 24506e342943..cea30f68f8dc 100644
>>> --- a/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>> +++ b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>> @@ -18,6 +18,7 @@
>>>  struct quirk_entry {
>>>  	u32 s2idle_bug_mmio;
>>>  	bool spurious_8042;
>>> +	bool need_suspend_delay;
>>>  };
>>>  
>>>  static struct quirk_entry quirk_s2idle_bug = {
>>> @@ -33,6 +34,10 @@ static struct quirk_entry quirk_s2idle_spurious_8042 = {
>>>  	.spurious_8042 = true,
>>>  };
>>>  
>>> +static struct quirk_entry quirk_s2idle_need_suspend_delay = {
>>> +	.need_suspend_delay = true
>>
>> Please add comma to any non-terminating entry.
> 
> will do
> 
>>
>>> +};
>>> +
>>>  static const struct dmi_system_id fwbug_list[] = {
>>>  	{
>>>  		.ident = "L14 Gen2 AMD",
>>> @@ -203,6 +208,32 @@ static const struct dmi_system_id fwbug_list[] = {
>>>  			DMI_MATCH(DMI_PRODUCT_NAME, "82XQ"),
>>>  		}
>>>  	},
>>> +	/*
>>> +	 * Some Lenovo Laptops (like different IdeaPad 3 Slims) need some
>>> +	 * me-time before sleeping or they get uncooperative after waking
>>> +	 * up and don't send events for keyboard and lid switch anymore.
>>> +	 * See https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>> +	 */
>>> +	{
>>> +		.ident = "Zen3-based IdeaPad Slim and similar",
>>> +		.driver_data = &quirk_s2idle_need_suspend_delay,
>>> +		.matches = {
>>> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>>> +			/*
>>> +			 * Note: there are also some Zen2-based 82X* devices that
>>> +			 * need different quirks, they're already handled above
>>> +			 */
>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "82X")
>>
>> Ditto.
>>
>>> +		}
>>> +	},
>>> +	{
>>> +		.ident = "Zen3+-based IdeaPad Slim and similar",
>>> +		.driver_data = &quirk_s2idle_need_suspend_delay,
>>> +		.matches = {
>>> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "83K")
>>
>> Ditto.
>>
>>> +		}
>>> +	},
>>>  	/* https://bugzilla.kernel.org/show_bug.cgi?id=221273 */
>>>  	{
>>>  		.ident = "Thinkpad L14 Gen3",
>>> @@ -356,6 +387,11 @@ void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
>>>  		amd_pmc_skip_nvme_smi_handler(dev->quirks->s2idle_bug_mmio);
>>>  }
>>>  
>>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev)
>>> +{
>>> +	return dev->quirks->need_suspend_delay;
>>> +}
>>> +
>>>  void amd_pmc_quirks_init(struct amd_pmc_dev *dev)
>>>  {
>>>  	const struct dmi_system_id *dmi_id;
>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>>> index cae3fcafd4d7..c604dc7207ed 100644
>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>> @@ -634,10 +634,13 @@ static void amd_pmc_s2idle_check(void)
>>>  	struct amd_pmc_dev *pdev = &pmc;
>>>  	struct smu_metrics table;
>>>  	int rc;
>>> +	bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
>>
>> Why isn't disable_workarounds inside amd_pmc_quirk_need_suspend_delay() ?
> 
> Because disable_workarounds is a static variable in pmc.c and
> amd_pmc_quirk_need_suspend_delay() is implemented in pmc-quirks.c
> 
>>
>>>  
>>>  	/* Avoid triggering OVP */
>>> -	if (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)
>>> +	if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {
>>
>> It would be clearer what's going on here if these two checks are clearly 
>> separated.
>>
>> As is, the comment confuses if it's only meant for the 2nd check.
>>
>> One of the checks will be in amd_pmc_quirk_need_suspend_delay() and the 
>> other (the existing one + the comment) should be moved in to own function 
>> (perhaps in a patch preceeding this one) so it's clear the reasoning is 
>> different(?). You can check the commit that introduced msleep(2500) here
>> for details about the existing check.
>>
> 
> Ok, I'll move that check for the existing workaround into its own 
> function to make it more clear what is happening

While doing this and thinking learning more about this old patch I had a
realization:
The existing code does this 2.5s sleep before every suspend *except* for
the very first one after a (re)boot!

(Except on CPUs that don't have this metrics table (AMD_CPU_ID_PCO) or
some edge case when pdev->smu_virt_addr is NULL and can't be set with
amd_pmc_setup_smu_logging())

This by the way explains why in the rare cases suspend randomly worked
as intended on my machine (even without my patch) it kept on working in
subsequent suspends, until rebooting.

Anyway, radical idea: What if we just ALWAYS SLEEP for 2.5 seconds here?

I don't think the special case of the very first suspend after boot
justifies all the complexity of the existing checks *and* the conditions
I'm adding.

> 
>>> +		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>>>  		msleep(2500);
>>> +	}
>>>  
>>>  	/* Dump the IdleMask before we add to the STB */
>>>  	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
>>> index fe3f53eb5955..f5257e47b8c4 100644
>>> --- a/drivers/platform/x86/amd/pmc/pmc.h
>>> +++ b/drivers/platform/x86/amd/pmc/pmc.h
>>> @@ -147,6 +147,7 @@ enum amd_pmc_def {
>>>  };
>>>  
>>>  void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
>>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev);
>>>  void amd_pmc_quirks_init(struct amd_pmc_dev *dev);
>>>  void amd_mp2_stb_init(struct amd_pmc_dev *dev);
>>>  void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>>>
>>
> 

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Mario Limonciello]

On 5/7/26 17:43, Daniel Gibson wrote:
> On 07.05.26 22:19, Daniel Gibson wrote:
>> Thanks for the review!
>>
>> On 07.05.26 15:22, Ilpo Järvinen wrote:
>>> On Fri, 1 May 2026, Daniel Gibson wrote:
>>>
>>>> Some IdeaPad Slim 3 devices and similar with AMD CPUs have a
>>>> nonfunctional keyboard and lid switch after s2idle.
>>>> It helps to delay suspend by 2.5 seconds so the EC has some time
>>>> to do whatever it needs to get done before suspend.
>>>>
>>>> This issue has been reported for many different devices, this patch
>>>> has been tested with the Zen3-based IdeaPad Slim 3 16ABR8 (82XR)
>>>> and the Zen3+-based IdeaPad Slim 3 14ARP10 (83K6), see
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>>
>>>> Reported-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>> Tested-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>>>> Tested-by: Daniel Gibson <daniel@gibson.sh>
>>>> Suggested-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
>>>> ---
>>>>   drivers/platform/x86/amd/pmc/pmc-quirks.c | 36 +++++++++++++++++++++++
>>>>   drivers/platform/x86/amd/pmc/pmc.c        |  5 +++-
>>>>   drivers/platform/x86/amd/pmc/pmc.h        |  1 +
>>>>   3 files changed, 41 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc-quirks.c b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>>> index 24506e342943..cea30f68f8dc 100644
>>>> --- a/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>>> +++ b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>>> @@ -18,6 +18,7 @@
>>>>   struct quirk_entry {
>>>>   	u32 s2idle_bug_mmio;
>>>>   	bool spurious_8042;
>>>> +	bool need_suspend_delay;
>>>>   };
>>>>   
>>>>   static struct quirk_entry quirk_s2idle_bug = {
>>>> @@ -33,6 +34,10 @@ static struct quirk_entry quirk_s2idle_spurious_8042 = {
>>>>   	.spurious_8042 = true,
>>>>   };
>>>>   
>>>> +static struct quirk_entry quirk_s2idle_need_suspend_delay = {
>>>> +	.need_suspend_delay = true
>>>
>>> Please add comma to any non-terminating entry.
>>
>> will do
>>
>>>
>>>> +};
>>>> +
>>>>   static const struct dmi_system_id fwbug_list[] = {
>>>>   	{
>>>>   		.ident = "L14 Gen2 AMD",
>>>> @@ -203,6 +208,32 @@ static const struct dmi_system_id fwbug_list[] = {
>>>>   			DMI_MATCH(DMI_PRODUCT_NAME, "82XQ"),
>>>>   		}
>>>>   	},
>>>> +	/*
>>>> +	 * Some Lenovo Laptops (like different IdeaPad 3 Slims) need some
>>>> +	 * me-time before sleeping or they get uncooperative after waking
>>>> +	 * up and don't send events for keyboard and lid switch anymore.
>>>> +	 * See https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>> +	 */
>>>> +	{
>>>> +		.ident = "Zen3-based IdeaPad Slim and similar",
>>>> +		.driver_data = &quirk_s2idle_need_suspend_delay,
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>>>> +			/*
>>>> +			 * Note: there are also some Zen2-based 82X* devices that
>>>> +			 * need different quirks, they're already handled above
>>>> +			 */
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "82X")
>>>
>>> Ditto.
>>>
>>>> +		}
>>>> +	},
>>>> +	{
>>>> +		.ident = "Zen3+-based IdeaPad Slim and similar",
>>>> +		.driver_data = &quirk_s2idle_need_suspend_delay,
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "83K")
>>>
>>> Ditto.
>>>
>>>> +		}
>>>> +	},
>>>>   	/* https://bugzilla.kernel.org/show_bug.cgi?id=221273 */
>>>>   	{
>>>>   		.ident = "Thinkpad L14 Gen3",
>>>> @@ -356,6 +387,11 @@ void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev)
>>>>   		amd_pmc_skip_nvme_smi_handler(dev->quirks->s2idle_bug_mmio);
>>>>   }
>>>>   
>>>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev)
>>>> +{
>>>> +	return dev->quirks->need_suspend_delay;
>>>> +}
>>>> +
>>>>   void amd_pmc_quirks_init(struct amd_pmc_dev *dev)
>>>>   {
>>>>   	const struct dmi_system_id *dmi_id;
>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>>>> index cae3fcafd4d7..c604dc7207ed 100644
>>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>>> @@ -634,10 +634,13 @@ static void amd_pmc_s2idle_check(void)
>>>>   	struct amd_pmc_dev *pdev = &pmc;
>>>>   	struct smu_metrics table;
>>>>   	int rc;
>>>> +	bool ec_needs_sleep = !disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev);
>>>
>>> Why isn't disable_workarounds inside amd_pmc_quirk_need_suspend_delay() ?
>>
>> Because disable_workarounds is a static variable in pmc.c and
>> amd_pmc_quirk_need_suspend_delay() is implemented in pmc-quirks.c
>>
>>>
>>>>   
>>>>   	/* Avoid triggering OVP */
>>>> -	if (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)
>>>> +	if (ec_needs_sleep || (!get_metrics_table(pdev, &table) && table.s0i3_last_entry_status)) {
>>>
>>> It would be clearer what's going on here if these two checks are clearly
>>> separated.
>>>
>>> As is, the comment confuses if it's only meant for the 2nd check.
>>>
>>> One of the checks will be in amd_pmc_quirk_need_suspend_delay() and the
>>> other (the existing one + the comment) should be moved in to own function
>>> (perhaps in a patch preceeding this one) so it's clear the reasoning is
>>> different(?). You can check the commit that introduced msleep(2500) here
>>> for details about the existing check.
>>>
>>
>> Ok, I'll move that check for the existing workaround into its own
>> function to make it more clear what is happening
> 
> While doing this and thinking learning more about this old patch I had a
> realization:
> The existing code does this 2.5s sleep before every suspend *except* for
> the very first one after a (re)boot!
> 
> (Except on CPUs that don't have this metrics table (AMD_CPU_ID_PCO) or
> some edge case when pdev->smu_virt_addr is NULL and can't be set with
> amd_pmc_setup_smu_logging())
> 
> This by the way explains why in the rare cases suspend randomly worked
> as intended on my machine (even without my patch) it kept on working in
> subsequent suspends, until rebooting.
> 
> Anyway, radical idea: What if we just ALWAYS SLEEP for 2.5 seconds here?
> 
> I don't think the special case of the very first suspend after boot
> justifies all the complexity of the existing checks *and* the conditions
> I'm adding.
> 

That's not how it works - it only applies the 2.5s delay when it has 
reached HW sleep and needs to start a second HW sleep cycle.

That is something like this:
  * close the lid
  * suspend sequence starts
  * (no extra sleep)
  * enter HW sleep
  * plug in ac adapter
  * EC SCI fires
  * exits HW sleep
  * kernel decides to go back to HW sleep (no other interrupts active)
  * (extra sleep here)
  * enter HW sleep
  * open lid
  * exit HW sleep
  * resume sequence
  * back in OS

And that is how every cycle after a reboot works.  So this would be the 
second sequence if you tried to suspend again.

  * close the lid
  * suspend sequence starts
  * (no extra sleep)
  * enter HW sleep
  * open lid
  * exit HW sleep
  * resume sequence
  * back in OS

The reason for that 2.5s delay on second HW sleep is described in that 
commit message.  It's specifically to mitigate a case that a voltage 
regulator gets too much voltage from the rapid swings in and out of HW 
sleep rapidly.

It should not be applied every single cycle unless it's needed (such as 
this bug you found with the EC race).

Now you might have been confused by not looking at commit 
4dbd11796f3a8eb95647507befc41995458a4023.  This fixes the behavior as 
it's supposed to be.

>>
>>>> +		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>>>>   		msleep(2500);
>>>> +	}
>>>>   
>>>>   	/* Dump the IdleMask before we add to the STB */
>>>>   	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
>>>> index fe3f53eb5955..f5257e47b8c4 100644
>>>> --- a/drivers/platform/x86/amd/pmc/pmc.h
>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.h
>>>> @@ -147,6 +147,7 @@ enum amd_pmc_def {
>>>>   };
>>>>   
>>>>   void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
>>>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev);
>>>>   void amd_pmc_quirks_init(struct amd_pmc_dev *dev);
>>>>   void amd_mp2_stb_init(struct amd_pmc_dev *dev);
>>>>   void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>>>>
>>>
>>
> 

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Daniel Gibson]

On 08.05.26 00:54, Mario Limonciello wrote:
> On 5/7/26 17:43, Daniel Gibson wrote:
>> On 07.05.26 22:19, Daniel Gibson wrote:
>>> Thanks for the review!
>>>
>>> On 07.05.26 15:22, Ilpo Järvinen wrote:
>>>> On Fri, 1 May 2026, Daniel Gibson wrote:
>>>>
>>>>> Some IdeaPad Slim 3 devices and similar with AMD CPUs have a
>>>>> nonfunctional keyboard and lid switch after s2idle.
>>>>> It helps to delay suspend by 2.5 seconds so the EC has some time
>>>>> to do whatever it needs to get done before suspend.
>>>>>
>>>>> This issue has been reported for many different devices, this patch
>>>>> has been tested with the Zen3-based IdeaPad Slim 3 16ABR8 (82XR)
>>>>> and the Zen3+-based IdeaPad Slim 3 14ARP10 (83K6), see
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>>>
>>>>> Reported-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>>> Tested-by: Sindre Henriksen <sindrehenriksen93@gmail.com>
>>>>> Tested-by: Daniel Gibson <daniel@gibson.sh>
>>>>> Suggested-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>>> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>>> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
>>>>> ---
>>>>>   drivers/platform/x86/amd/pmc/pmc-quirks.c | 36 ++++++++++++++++++
>>>>> +++++
>>>>>   drivers/platform/x86/amd/pmc/pmc.c        |  5 +++-
>>>>>   drivers/platform/x86/amd/pmc/pmc.h        |  1 +
>>>>>   3 files changed, 41 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc-quirks.c b/drivers/
>>>>> platform/x86/amd/pmc/pmc-quirks.c
>>>>> index 24506e342943..cea30f68f8dc 100644
>>>>> --- a/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>>>> +++ b/drivers/platform/x86/amd/pmc/pmc-quirks.c
>>>>> @@ -18,6 +18,7 @@
>>>>>   struct quirk_entry {
>>>>>       u32 s2idle_bug_mmio;
>>>>>       bool spurious_8042;
>>>>> +    bool need_suspend_delay;
>>>>>   };
>>>>>     static struct quirk_entry quirk_s2idle_bug = {
>>>>> @@ -33,6 +34,10 @@ static struct quirk_entry
>>>>> quirk_s2idle_spurious_8042 = {
>>>>>       .spurious_8042 = true,
>>>>>   };
>>>>>   +static struct quirk_entry quirk_s2idle_need_suspend_delay = {
>>>>> +    .need_suspend_delay = true
>>>>
>>>> Please add comma to any non-terminating entry.
>>>
>>> will do
>>>
>>>>
>>>>> +};
>>>>> +
>>>>>   static const struct dmi_system_id fwbug_list[] = {
>>>>>       {
>>>>>           .ident = "L14 Gen2 AMD",
>>>>> @@ -203,6 +208,32 @@ static const struct dmi_system_id fwbug_list[]
>>>>> = {
>>>>>               DMI_MATCH(DMI_PRODUCT_NAME, "82XQ"),
>>>>>           }
>>>>>       },
>>>>> +    /*
>>>>> +     * Some Lenovo Laptops (like different IdeaPad 3 Slims) need some
>>>>> +     * me-time before sleeping or they get uncooperative after waking
>>>>> +     * up and don't send events for keyboard and lid switch anymore.
>>>>> +     * See https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>>>> +     */
>>>>> +    {
>>>>> +        .ident = "Zen3-based IdeaPad Slim and similar",
>>>>> +        .driver_data = &quirk_s2idle_need_suspend_delay,
>>>>> +        .matches = {
>>>>> +            DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>>>>> +            /*
>>>>> +             * Note: there are also some Zen2-based 82X* devices that
>>>>> +             * need different quirks, they're already handled above
>>>>> +             */
>>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "82X")
>>>>
>>>> Ditto.
>>>>
>>>>> +        }
>>>>> +    },
>>>>> +    {
>>>>> +        .ident = "Zen3+-based IdeaPad Slim and similar",
>>>>> +        .driver_data = &quirk_s2idle_need_suspend_delay,
>>>>> +        .matches = {
>>>>> +            DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "83K")
>>>>
>>>> Ditto.
>>>>
>>>>> +        }
>>>>> +    },
>>>>>       /* https://bugzilla.kernel.org/show_bug.cgi?id=221273 */
>>>>>       {
>>>>>           .ident = "Thinkpad L14 Gen3",
>>>>> @@ -356,6 +387,11 @@ void amd_pmc_process_restore_quirks(struct
>>>>> amd_pmc_dev *dev)
>>>>>           amd_pmc_skip_nvme_smi_handler(dev->quirks->s2idle_bug_mmio);
>>>>>   }
>>>>>   +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev)
>>>>> +{
>>>>> +    return dev->quirks->need_suspend_delay;
>>>>> +}
>>>>> +
>>>>>   void amd_pmc_quirks_init(struct amd_pmc_dev *dev)
>>>>>   {
>>>>>       const struct dmi_system_id *dmi_id;
>>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/
>>>>> x86/amd/pmc/pmc.c
>>>>> index cae3fcafd4d7..c604dc7207ed 100644
>>>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>>>> @@ -634,10 +634,13 @@ static void amd_pmc_s2idle_check(void)
>>>>>       struct amd_pmc_dev *pdev = &pmc;
>>>>>       struct smu_metrics table;
>>>>>       int rc;
>>>>> +    bool ec_needs_sleep = !disable_workarounds &&
>>>>> amd_pmc_quirk_need_suspend_delay(pdev);
>>>>
>>>> Why isn't disable_workarounds inside
>>>> amd_pmc_quirk_need_suspend_delay() ?
>>>
>>> Because disable_workarounds is a static variable in pmc.c and
>>> amd_pmc_quirk_need_suspend_delay() is implemented in pmc-quirks.c
>>>
>>>>
>>>>>         /* Avoid triggering OVP */
>>>>> -    if (!get_metrics_table(pdev, &table) &&
>>>>> table.s0i3_last_entry_status)
>>>>> +    if (ec_needs_sleep || (!get_metrics_table(pdev, &table) &&
>>>>> table.s0i3_last_entry_status)) {
>>>>
>>>> It would be clearer what's going on here if these two checks are
>>>> clearly
>>>> separated.
>>>>
>>>> As is, the comment confuses if it's only meant for the 2nd check.
>>>>
>>>> One of the checks will be in amd_pmc_quirk_need_suspend_delay() and the
>>>> other (the existing one + the comment) should be moved in to own
>>>> function
>>>> (perhaps in a patch preceeding this one) so it's clear the reasoning is
>>>> different(?). You can check the commit that introduced msleep(2500)
>>>> here
>>>> for details about the existing check.
>>>>
>>>
>>> Ok, I'll move that check for the existing workaround into its own
>>> function to make it more clear what is happening
>>
>> While doing this and thinking learning more about this old patch I had a
>> realization:
>> The existing code does this 2.5s sleep before every suspend *except* for
>> the very first one after a (re)boot!
>>
>> (Except on CPUs that don't have this metrics table (AMD_CPU_ID_PCO) or
>> some edge case when pdev->smu_virt_addr is NULL and can't be set with
>> amd_pmc_setup_smu_logging())
>>
>> This by the way explains why in the rare cases suspend randomly worked
>> as intended on my machine (even without my patch) it kept on working in
>> subsequent suspends, until rebooting.
>>
>> Anyway, radical idea: What if we just ALWAYS SLEEP for 2.5 seconds here?
>>
>> I don't think the special case of the very first suspend after boot
>> justifies all the complexity of the existing checks *and* the conditions
>> I'm adding.
>>
> 
> That's not how it works - it only applies the 2.5s delay when it has
> reached HW sleep and needs to start a second HW sleep cycle.
> 
> That is something like this:
>  * close the lid
>  * suspend sequence starts
>  * (no extra sleep)
>  * enter HW sleep
>  * plug in ac adapter
>  * EC SCI fires
>  * exits HW sleep
>  * kernel decides to go back to HW sleep (no other interrupts active)
>  * (extra sleep here)
>  * enter HW sleep
>  * open lid
>  * exit HW sleep
>  * resume sequence
>  * back in OS
> 
> And that is how every cycle after a reboot works.  So this would be the
> second sequence if you tried to suspend again.
> 
>  * close the lid
>  * suspend sequence starts
>  * (no extra sleep)
>  * enter HW sleep
>  * open lid
>  * exit HW sleep
>  * resume sequence
>  * back in OS
> 
> The reason for that 2.5s delay on second HW sleep is described in that
> commit message.  It's specifically to mitigate a case that a voltage
> regulator gets too much voltage from the rapid swings in and out of HW
> sleep rapidly.
> 
> It should not be applied every single cycle unless it's needed (such as
> this bug you found with the EC race).

Are you sure it behaves like this?
The check is basically "is the SMU table's s0i3_last_entry_status != 0".
And as far as I can tell this is the case after every successful suspend,
it only seems to be reset by rebooting or if a suspend fails.

See smu_fw_info_show() (for /sys/kern/debug/amd_pmc/smu_fw_info): 
seq_printf(s, "Last S0i3 Status: %s\n", table.s0i3_last_entry_status ? "Success" :
		   "Unknown/Fail");

When I read /sys/kern/debug/amd_pmc/smu_fw_info right after booting, it prints 
"Last S0i3 Status: Unknown/Fail"
After resuming, even minutes (and presumably hours) later, it prints
"Last S0i3 Status: Success"
indicating that s0i3_last_entry_status still is != 0.

> 
> Now you might have been confused by not looking at commit
> 4dbd11796f3a8eb95647507befc41995458a4023.  This fixes the behavior as
> it's supposed to be.
> 
>>>
>>>>> +        dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid
>>>>> platform bug\n");
>>>>>           msleep(2500);
>>>>> +    }
>>>>>         /* Dump the IdleMask before we add to the STB */
>>>>>       amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
>>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/
>>>>> x86/amd/pmc/pmc.h
>>>>> index fe3f53eb5955..f5257e47b8c4 100644
>>>>> --- a/drivers/platform/x86/amd/pmc/pmc.h
>>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.h
>>>>> @@ -147,6 +147,7 @@ enum amd_pmc_def {
>>>>>   };
>>>>>     void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
>>>>> +bool amd_pmc_quirk_need_suspend_delay(struct amd_pmc_dev *dev);
>>>>>   void amd_pmc_quirks_init(struct amd_pmc_dev *dev);
>>>>>   void amd_mp2_stb_init(struct amd_pmc_dev *dev);
>>>>>   void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Mario Limonciello]

> 
> Are you sure it behaves like this?
> The check is basically "is the SMU table's s0i3_last_entry_status != 0".
> And as far as I can tell this is the case after every successful suspend,
> it only seems to be reset by rebooting or if a suspend fails.
> 
> See smu_fw_info_show() (for /sys/kern/debug/amd_pmc/smu_fw_info):
> seq_printf(s, "Last S0i3 Status: %s\n", table.s0i3_last_entry_status ? "Success" :
> 		   "Unknown/Fail");
> 
> When I read /sys/kern/debug/amd_pmc/smu_fw_info right after booting, it prints
> "Last S0i3 Status: Unknown/Fail"
> After resuming, even minutes (and presumably hours) later, it prints
> "Last S0i3 Status: Success"
> indicating that s0i3_last_entry_status still is != 0.
> 
>>
>> Now you might have been confused by not looking at commit
>> 4dbd11796f3a8eb95647507befc41995458a4023.  This fixes the behavior as
>> it's supposed to be.
>>
First suspend entry:

amd_pmc_s2idle_prepare()
->amd_pmc_setup_smu_logging()
->->memset_io(dev->smu_virt_addr, 0, sizeof(struct smu_metrics))

That is the table is cleared.

amd_pmc_s2idle_check()
->get_metrics_table() && table.s0i3_last_entry_status

The table is fetched and check (last entry will be 0).
No delay.

At this point system is in hardware sleep.

If system resumes from hardware sleep table.s0i3_last_entry_status will 
be non-zero.

If you re-enter:

amd_pmc_s2idle_check() will do the msleep.

If you exit:

amd_pmc_s2idle_restore()
-> amd_pmc_validate_deepest()

This will read metrics table and tell you in logs if you didn't get to 
HW sleep.

Re: [PATCH 1/2] platform/x86/amd/pmc: Delay suspend for some Lenovo Laptops

[Daniel Gibson]

On 08.05.26 01:48, Mario Limonciello wrote:
> 
>>
>> Are you sure it behaves like this?
>> The check is basically "is the SMU table's s0i3_last_entry_status != 0".
>> And as far as I can tell this is the case after every successful suspend,
>> it only seems to be reset by rebooting or if a suspend fails.
>>
>> See smu_fw_info_show() (for /sys/kern/debug/amd_pmc/smu_fw_info):
>> seq_printf(s, "Last S0i3 Status: %s\n", table.s0i3_last_entry_status ?
>> "Success" :
>>            "Unknown/Fail");
>>
>> When I read /sys/kern/debug/amd_pmc/smu_fw_info right after booting,
>> it prints
>> "Last S0i3 Status: Unknown/Fail"
>> After resuming, even minutes (and presumably hours) later, it prints
>> "Last S0i3 Status: Success"
>> indicating that s0i3_last_entry_status still is != 0.
>>
>>>
>>> Now you might have been confused by not looking at commit
>>> 4dbd11796f3a8eb95647507befc41995458a4023.  This fixes the behavior as
>>> it's supposed to be.
>>>
> First suspend entry:
> 
> amd_pmc_s2idle_prepare()
> ->amd_pmc_setup_smu_logging()
> ->->memset_io(dev->smu_virt_addr, 0, sizeof(struct smu_metrics))
> 
> That is the table is cleared.
> 
> amd_pmc_s2idle_check()
> ->get_metrics_table() && table.s0i3_last_entry_status
> 
> The table is fetched and check (last entry will be 0).
> No delay.
> 
> At this point system is in hardware sleep.
> 
> If system resumes from hardware sleep table.s0i3_last_entry_status will
> be non-zero.
> 
> If you re-enter:
> 
> amd_pmc_s2idle_check() will do the msleep.
> 
> If you exit:
> 
> amd_pmc_s2idle_restore()
> -> amd_pmc_validate_deepest()
> 
> This will read metrics table and tell you in logs if you didn't get to
> HW sleep.
> 

Ah, now I get it - sorry for the confusion!
I somehow missed that amd_pmc_setup_smu_logging() is called at each
amd_pmc_s2idle_prepare() - my mistake.

There goes the simple solution :-/