On Wed, Sep 10, 2025 at 11:52:00AM -0500, Mario Limonciello wrote:
> On 9/10/25 10:06 AM, Bjorn Helgaas wrote:
> > On Tue, Sep 09, 2025 at 02:16:12PM -0500, Mario Limonciello (AMD) wrote:
> > > PCI devices can be configured as wakeup sources from low power states.
> > > However, when the system is halting or powering off such wakeups are
> > > not expected and may lead to spurious behavior.
> >
> > I'm a little unclear on the nomenclature for these low power states,
> > so I think it would be helpful to connect to the user action, e.g.,
> > suspend/hibernate/etc, and the ACPI state, e.g.,
> >
> > ... when the system is hibernating (e.g., transitioning to ACPI S4
> > and halting) or powering off (e.g., transitioning to ACPI S5 soft
> > off), such wakeups are not expected ...
>
> I will try to firm it up in the commit message. But yes you're getting the
> intent, having a wakeup occur at S5 would be unexpected, and would likely
> change semantics of what people "think" powering off a machine means.
>
> > When I suspend or power off my laptop from the GUI user interface, I
> > want to know if keyboard or mouse activity will resume or if I need to
> > press the power button.
>
> The way the kernel is set up today you get a single wakeup sysfs file for a
> device and that wakeup file means 3 things:
> * abort the process of entering a suspend state or hibernate
> * wake up the machine from a suspend state
> * wake up the machine from hibernate
>
> > > ACPI r6.5, section 16.1.5 notes:
> > >
> > > "Hardware does allow a transition to S0 due to power button press
> > > or a Remote Start."
> >
> > Important to note here that sec 16.1.5 is specifically for "S5
> > Soft Off State".
> >
> > S4 is a sleeping state and presumably sec 16.1.6 ("Transitioning
> > from the Working to the Sleeping State") applies. That section
> > mentions wakeup devices, so it's not obvious to me that PCI device
> > wakeup should be disabled for S4.
>
> It actually /shouldn't/ be disabled for S4 - it should only be
> disabled for S5.
>
> Are you implying a bug in the flow? I didn't think there was one:
>
> During entering hibernate the poweroff() call will have system_state
> = SYSTEM_SUSPEND so wakeups would be enabled.
>
> For powering off the system using hibernate flows poweroff() call
> would have system_state = SYSTEM_HALT or SYSTEM_POWER_OFF.
OK. I assumed that since you check for two states (SYSTEM_HALT or
SYSTEM_POWER_OFF), one must be hibernate (ending up in S4?) and the
other a soft power off (ending up in S5?).
But it sounds like there are two ways to power off. I'm just confused
about the correspondence between hibernate, soft poweroff, S4, S5,
SYSTEM_HALT, and SYSTEM_POWER_OFF.
*Do* both SYSTEM_HALT and SYSTEM_POWER_OFF lead to S5 on an ACPI
system? If so, what's the difference between them?
> > > This implies that wakeups from PCI devices should not be relied upon
> > > in these states. To align with this expectation and avoid unintended
> > > wakeups, disable device wakeup capability during these transitions.
> > >
> > > Tested-by: Eric Naim <dn...@cachyos.org>
> > > Signed-off-by: Mario Limonciello (AMD) <supe...@kernel.org>
> > > ---
> > > v7:
> > > * Reword title
> > > * Reword commit
> > > v5:
> > > * Re-order
> > > * Add tags
> > > v4:
> > > *
> > > https://lore.kernel.org/linux-pci/20250616175019.3471583-1-supe...@kernel.org/
> > > ---
> > > drivers/pci/pci-driver.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index 63665240ae87f..f201d298d7173 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -1139,6 +1139,10 @@ static int pci_pm_poweroff(struct device *dev)
> > > struct pci_dev *pci_dev = to_pci_dev(dev);
> > > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :
> > > NULL;
> > > + if (device_may_wakeup(dev) &&
> > > + (system_state == SYSTEM_HALT || system_state == SYSTEM_POWER_OFF))
> > > + device_set_wakeup_enable(dev, false);
> > > +
> > > if (pci_has_legacy_pm_support(pci_dev))
> > > return pci_legacy_suspend(dev, PMSG_HIBERNATE);
> > > --
> > > 2.43.0
> > >
>
