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

[Limonciello, Mario]

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