Re: [PATCH v7 05/12] PCI/PM: Disable device wakeups when halting or powering off system

[Mario Limonciello <mario.limonciello@amd.com>]

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.

> 
>> 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 <dnaim@cachyos.org>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v7:
>>   * Reword title
>>   * Reword commit
>> 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);
>> +
>>   	if (pci_has_legacy_pm_support(pci_dev))
>>   		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>>   
>> -- 
>> 2.43.0
>>