Re: [PATCH v4 2/3] platform/x86/amd/pmc: Add delay_suspend module parameter

[Daniel Gibson]

Hi,

On 08.06.26 13:22, Hans de Goede wrote:
> Hi,
> 
> On 6-Jun-26 6:47 AM, Daniel Gibson wrote:
>> Enabling the new delay_suspend module parameter 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.
>>
>> The parameter description has a note 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[])
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=221383
>> Signed-off-by: Daniel Gibson <daniel@xxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>>  drivers/platform/x86/amd/pmc/pmc.c | 25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index 6bafd8661d68..2d3d180c15d2 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/bits.h>
>>  #include <linux/debugfs.h>
>>  #include <linux/delay.h>
>> +#include <linux/dmi.h>
>>  #include <linux/io.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/limits.h>
> 
> This addition of including dmi.h seems unnecessary.
> 

It's for that dev_info() call in the case that someone has enforced the option, see below

>> @@ -89,6 +90,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)
>> @@ -625,8 +631,23 @@ static bool amd_pmc_want_suspend_delay(struct amd_pmc_dev *pdev)
>>  	 *
>>  	 * See https://bugzilla.kernel.org/show_bug.cgi?id=221383
>>  	 */
>> -	if (!disable_workarounds && amd_pmc_quirk_need_suspend_delay(pdev)) {
>> -		dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>> +	if (amd_pmc_quirk_need_suspend_delay(pdev)) {
>> +		/*
>> +		 * delay_suspend=1 force-enables this, otherwise it can be
>> +		 * disabled with disable_workarounds or delay_suspend=0
>> +		 */
>> +		if (delay_suspend == 1 || (delay_suspend == -1 && !disable_workarounds)) {
>> +			dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>> +			return true;
>> +		}
>> +		dev_info(pdev->dev, "Not delaying suspend because of module parameter, even though your device is assumed to need it!\n");
>> +	} else if (delay_suspend == 1) {
>> +		dev_info(pdev->dev, "Delaying suspend by 2.5s because delay_suspend=1. If this solves problems on your machine, please report this whole line to: platform-driver-x86@xxxxxxxxxxxxxxx so it can be automatically detected as affected in the future. System Vendor: \"%s\" Product Name: \"%s\" Product Family: \"%s\" Board Vendor: \"%s\" Board Name: \"%s\"\n",
>> +			 dmi_get_system_info(DMI_SYS_VENDOR),
>> +			 dmi_get_system_info(DMI_PRODUCT_NAME),
>> +			 dmi_get_system_info(DMI_PRODUCT_FAMILY),
>> +			 dmi_get_system_info(DMI_BOARD_VENDOR),
>> +			 dmi_get_system_info(DMI_BOARD_NAME));

this one

>>  		return true;
>>  	}
>>  	return false;
> 
> Otherwise this looks good to me:
> 
> Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Thank you very much for the reviews!

> 
> Regards,
> 
> Hans
> 

Cheers,
Daniel