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@xxxxxxxxxxxxxxx");
>> +
>>   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
>>
>