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

[Mario Limonciello]

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

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