Re: [PATCH 2/2] platform/x86/amd/pmc: Add delay_suspend module argument

[Mario Limonciello <superm1@kernel.org>]

On 5/4/26 10:38, Daniel Gibson wrote:
> 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@vger.kernel.org");
>>> +
>>>    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.

OK.  I'd say up to the driver maintainers to decide which way to go.

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

Very good point.

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

Yeah; whatever direction is agreed upon (module parameter or debugfs), 
please update the document for a section about it.

> 
> 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 ;)

"This information is read by the kernel at bootup and printed into the 
syslog".

Do you mean actually outputting the prefix of the message in this document?

This is a sample message from my local machine.

x86/amd: Previous system reset reason [0x00080800]: software wrote 0x6 
to reset control register 0xCF9

I have no qualms with this - feel free to send a patch if you're 
touching it already anyways.

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