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

[Mario Limonciello <superm1@kernel.org>]

On 9/10/25 12:11 PM, Bjorn Helgaas wrote:
> 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?

The two functions are kernel_halt() and kernel_power_off().

And looking again, Ahhhh!  kernel_power_off() is the only thing that 
actually leads to machine_power_off().  Halt just stops the CPUs.

I think we should only be using the hibernate flows for SYSTEM_POWER_OFF.

This has implications for a lot of the patches.  Thanks a lot for 
pointing this out. I'll walk the series again and change accordingly.

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