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

[Mario Limonciello]

On 5/22/23 00:55, Mika Westerberg wrote:
> Hi Mario,
>
> 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.
> >
> > Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
> > suspend") all root port go into D3 during s2idle.
> > When the root ports are in D3 over s2idle it's not possible for the
> > platform firmware to properly identify the wakeup source.
> >
> > 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.
> >
> > 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.
> >
> > 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;
> > +
>
> We already call platform_pci_bridge_d3() few lines up. That function
> should know whether "platform" supports D3 for the bridges, and I think
> it actually calls acpi_device_power_manageable() that platform_pci_power_manageable()
> ends up checking too.

It does, but it doesn't end up returning false if it doesn't support it. 
 It only returns true if it does.

I've tested this patch on two different failing platforms and it works 
now on both.