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

[Hans de Goede]

Hi,

On 8-Jun-26 1:41 PM, Daniel Gibson wrote:
> 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

Ah right, that makes sense. Never mind then :)

Regards,

Hans




> 
>>> @@ -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
>