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@xxxxxxxxx>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221383
>> Tested-by: Sindre Henriksen <sindrehenriksen93@xxxxxxxxx>
>> Tested-by: Daniel Gibson <daniel@xxxxxxxxx>
>> Suggested-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
>> Reviewed-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
>> Signed-off-by: Daniel Gibson <daniel@xxxxxxxxx>
>> ---
>>  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);
>>
>