Re: [PATCH v2] PCI: Don't assume root ports from > 2015 are power manageable

[Limonciello, Mario]

On 5/23/2023 3:35 PM, Bjorn Helgaas wrote:
> [+cc Rafael, Lukas, linux-pm]
>
> On Wed, May 17, 2023 at 10:08:27AM -0500, Mario Limonciello wrote:
> > Using an XHCI device to wakeup the system from s2idle fails when
> > that XHCI device is connected to a USB-C port for an AMD USB4
> > router.
> Are XHCI, USB-C, and the AMD USB4 router just examples?

They're very specific to this case.  If XHCI
keyboard/mouse is connected to a type-C port that is
not connected to AMD USB4 router this issue doesn't occur.

> I assume the
> same issue could happen with non-XHCI and non-AMD devices, too?
Based on what's wrong with Linux and fixed by this patch,
yes this *can* happen to any vendor that the root port doesn't
support waking from for D3 but Linux uses it anyway.
> I assume the problem has something to do with PME_Support and some
> device being put in a power state where it cannot generate or forward
> PME messages?  I think the PCIe protocol details would be helpful
> here.

No; it's specific to an internal sequence in the SoC.

If the problematic root port is in D3 during s0i3 this
problematic sequence happens.  If the root port is in D0
then it doesn't.

From discussion with others in AMD that's at least one
reason why the firmware doesn't advertise any power management
on this root port and why Linux shouldn't be using it.

> > Comparing registers between Linux and Windows 11 this behavior to put root
> > ports into D3 at suspend is unique to Linux.  On an affected system
> > Windows does not put the root ports into D3 over Modern Standby.
> >
> > Windows doesn't put the root ports into D3 because root ports are not
> > power manageable; they're missing _PRW and _S0W.
> platform_pci_power_manageable() tests adev->flags.power_manageable,
> which is set by acpi_bus_get_power_flags() when a device has _PS0 or
> _PR0.
>
> So I don't know what's relevant out of _PRW, _S0W, _PS0, _PR0, but
> this sentence doesn't seem to match the code.

The firmware doesn't have _PS0 or _PR0 either for this root
port.

> > Linux shouldn't be assuming they support D3 just because they're newer
> > than 2015, the ports should also be deemed power manageable.
> > Add an extra check for this to ensure D3 isn't selected for such machines.
> Is this talking about D3hot or D3cold or both?  If we can make this
> explicit, it will help me out.  It's probably obvious to power
> experts, but I'm not one.
Both.
> > Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > Reported-by: Iain Lane <iain@xxxxxxxxxxxxxxxxxxx>
> > Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> >   drivers/pci/pci.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 5ede93222bc1..3fe27aef09e6 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3010,6 +3010,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >   		if (dmi_check_system(bridge_d3_blacklist))
> >   			return false;
> >   
> > +		if (!platform_pci_power_manageable(bridge))
> > +			return false;
> > +
> >   		/*
> >   		 * It should be safe to put PCIe ports from 2015 or newer
> >   		 * to D3.
> > --
> > 2.34.1
Something that this patch makes me wonder is if the original
heuristic was actually correct.

Did the PCIe ports from "older" machine have everything needed
to let them go to D3?

Or would this change also let the heuristic be dropped?