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

["Mario Limonciello (kernel.org)" <superm1@kernel.org>]

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