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@xxxxxxxxx>
>> Tested-by: Daniel Gibson <daniel@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx");
>> +
>>  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.