Re: [PATCH v6 05/11] PCI: PM: Disable device wakeups when halting system through S4 flow

Mario Limonciello (kernel.org)
2025-09-08T21:51:19Z

On 9/8/2025 4:34 PM, Bjorn Helgaas wrote:
> In subject, s|PCI: PM:|PCI/PM:| to follow previous practice.

👍

>
> On Sun, Aug 17, 2025 at 09:00:55PM -0500, Mario Limonciello (AMD) wrote:
>> PCI devices can be programmed as a wakeup source from low power states
>> by sysfs. However when using the S4 flow to go into S5 these wakeup
>> sources should be disabled to avoid what users would perceive as
>> spurious wakeup events.
>
> Is the "can be programmed vis sysfs" part relevant here?

No, I can drop that part of the sentence.

>
> I think S4 and S5 are ACPI sleep states not applicable to all
> platforms. Is it relevant that we got here via ACPI?

But a non-ACPI system would still run the exact same callbacks for
hibernation wouldn't it?

In general you can 's,S4,hibernation,; s,s5,shutdown,' and it would be
accurate.
>
> I assume non-ACPI systems can also exercise this path. Is there a way
> to describe this scenario in a way that would apply to all systems?
>
> I'm not sure what "using the S4 flow to go in to S5" means.

It means to run the hibernate related device callbacks instead of the
shutdown related device callbacks.

I suppose anywhere in the series that is not ACPI specific (besides this
patch) I should clarify this as well.

>
> It would be nice to have a spec reference or some sort of rationale
> for the requirement to disable all wakeup sources in SYSTEM_HALT and
> SYSTEM_POWER_OFF.

I didn't observe it in the PCI path, but I did see that in USB that if
you leave them enabled they stay enabled when the system is shutdown.
For example if a USB mouse was connected and a wakeup source it could
wakeup from S5 too.

So this patch was aiming for congruence between the two subsystems.

I would hypothesize that means that a dock could wake a system from S5
on a hotplug event, which I don't believe is intended behavior.

>
>> Tested-by: Eric Naim <dnaim@cachyos.org>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v5:
>> * Re-order
>> * Add tags
>> v4:
>> * https://lore.kernel.org/linux-pci/20250616175019.3471583-1-superm1@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);
>
> I guess the suggestion is that we can't wake up at all from
> SYSTEM_HALT or SYSTEM_POWER_OFF? Would both be considered S5?

Correct.

>
> Does this mean we need a physical power button push to start up again?
> I guess ACPI r6.5, sec 16.1.5 kind of suggests that: "hardware does
> allow a transition to S0 due to power button press or a Remote Start."

Correct.

>
>> if (pci_has_legacy_pm_support(pci_dev))
>> return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>>
>> --
>> 2.43.0
>>