Re: [PATCH v5 2/2] PCI: Don't assume root ports are power manageable

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

On 6/2/2023 6:19 PM, Bjorn Helgaas wrote:
> On Fri, Jun 02, 2023 at 05:38:37PM -0500, Limonciello, Mario wrote:
>> On 6/2/2023 5:20 PM, Bjorn Helgaas wrote:
>>> On Tue, May 30, 2023 at 11:39:47AM -0500, Mario Limonciello wrote:
>>>> Using a USB keyboard or mouse to wakeup the system from s2idle fails when
>>>> that xHCI device is connected to a USB-C port for an AMD USB4 router.
>>> It sounds like the real issue is that "Root Ports in D3hot/D3cold may
>>> not support wakeup", and the USB, xHCI, USB-C, AMD USB4 router bits
>>> are probably not really relevant.  And hopefully even the "AMD
>>> platforms" mentioned below is not relevant.
>> Yeah.  It comes down to how much you want in the commit
>> about how we got to this conclusion versus a generic
>> fix.  I generally like to be verbose about a specific case
>> something fixes so that when distros decide what to pull
>> in to their older maintenance kernels they can understand
>> what's important.
> That's actually my point.  I think this problem probably affects
> non-USB devices, non-xHCI devices, non-USB4 routers, etc.
>
> If we can say "Any device below a Root Port in D3hot/D3cold may not
> support wakeup if X, Y, Z.  Root Ports may be put in D3hot/D3cold for
> sleep/hibernate/s2idle/...",  that's much more actionable.
Completely agree.
>
>>>> Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> all PCIe ports go into D3 during s2idle.
>>>>
>>>> When specific root ports are put into D3 over s2idle on some AMD platforms
>>>> it is not possible for the platform to properly identify wakeup sources.
>>>> This happens whether the root port goes into D3hot or D3cold.
>>> Can we connect this to a spec so it's not just the empirical "some AMD
>>> platforms work like X" observation?
>>>
>>> "s2idle" is meaningful on the power management side of the house, but
>>> it doesn't appear in PCI or ACPI specs, so I don't know what it means
>>> here.  I assume the D3hot/D3cold state of the Root Port is the
>>> critical factor, regardless of how it got there.
>> Unfortunately (?) for this particular issue it's only a
>> critical factor when the system is in s2idle.
>>
>> PME works fine to wake up the device if the root port is
>> in either D3hot or D3cold when the system isn't in s2idle.
> So that must mean something other than the Root Port has to be in some
> specific state.  "System in s2idle" is not actionable in terms of PCI
> maintenance.  It sounds like we just haven't really gotten to the root
> cause yet.
The root cause of this behavior is deep in the platform
firmware.  This is why the platform firmware doesn't
advertise power management support for the root port.
>
>>>> Linux shouldn't assume root ports support D3 just because they're on a
>>>> machine newer than 2015, the ports should also be deemed power manageable.
>>>> Add an extra check explicitly for root ports to ensure D3 isn't selected
>>>> for them if they are not power-manageable through platform firmware.
>>> But I *would* like to know specifically what "power manageable" means
>>> here.  I might naively assume that a device with the PCI Power
>>> Management Capability is "power manageable", and that if PME_Support
>>> includes D3hot and D3cold, we're good to go.  But obviously it's more
>>> complicated than that, and I'd like to cite the spec that mentions the
>>> actual things we need here.
>> Power manageable through platform firmware means the device
>> has ACPI methods like like _PR0, _PS0.
> What's the connection to wakeup?
There is also no _PRW (power resources for wake) for this
root port.
>
>>>> +	 * It's not safe to put root ports that don't support power
>>>> +	 * management into D3.
>>> I assume "it's not safe" really means "Root Ports in D3hot/D3cold may
>>> not be able to signal PME interrupts unless ... <mumble> platform
>>> firmware <mumble> e.g., ACPI method <mumble> ..."
>>>
>>> Can we include some of those hints here?
>> I'm cautious about hardcoding logic used by
>> acpi_bus_get_power_flags() in a comment in case it changes.
>>
>> How about:
>>
>> "Root ports in D3 may not be able to reliably signal wakeup
>> events unless platform firmware signals power management
>> capabilities".
> I'm really looking hard for that spec citation :)  Without that, this
> just devolves into "this seems to work on these systems."
I mean that's exactly what the broken logic below this
fix is....

But I think I can get you what you're looking for.

 From this failing system the problematic root port is GP19.

The DSDT has:

Device (GP19) {
     Method (_DSM,..)
     Method (_PRT,..)
     Device (NHI0)
     Device (NHI1)
}

The SSDT has:

Scope (\_SB.PCI0.GP19) {
Method (YS0W,..)
Method (YPRW,..)
Method (RPRM,..)
Method (WPRM,..)
Method (SPDP,..)
Method (SPCH,..)
Method (_STA,..)
Method (_INI,..)
Method (_REG,..)
}

Section 7.3 from the ACPI spec [1] says

"For a device that is power-managed using ACPI, a Definition Block 
contains one or more of the objects found in the table below. Power 
management of a device is done using Power Resource control"

The GP19 device has NONE of the objects mentioned in the table.

Outside of this change, I do think this means acpi_bus_get_power_flags() 
may want to also look for some of the other objects besides _PS0 and 
_PR0 to resolve that a device is power manageable though.

[1] 
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html?highlight=pr0#device-power-management-objects

>>>> +	 */
>>>> +	if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
>>>> +	    !platform_pci_power_manageable(bridge))
>>>> +		return false;
>>>> +
>>>>    	/*
>>>>    	 * It should be safe to put PCIe ports from 2015 or newer
>>>>    	 * to D3.
>>>> -- 
>>>> 2.34.1
>>>>