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

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

[Mario Limonciello]

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.

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.

Comparing registers between Linux and Windows 11 this behavior to put
these specific root ports into D3 at suspend is unique to Linux. On an
affected system Windows does not put those specific root ports into D3
over Modern Standby.

Windows doesn't put the root ports into D3 because root ports are not
power manageable.

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 these ports.

Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
Reported-by: Iain Lane <iain@orangesquash.org.uk>
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@amd.com>
---
v2->v3:
 * Only apply to root ports
 * Update commit message
---
 drivers/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5ede93222bc1..51126891a2db 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2976,6 +2976,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 
 	switch (pci_pcie_type(bridge)) {
 	case PCI_EXP_TYPE_ROOT_PORT:
+		if (!platform_pci_power_manageable(bridge))
+			return false;
+		fallthrough;
 	case PCI_EXP_TYPE_UPSTREAM:
 	case PCI_EXP_TYPE_DOWNSTREAM:
 		if (pci_bridge_d3_disable)
-- 
2.34.1

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

[Lukas Wunner]

On Wed, May 24, 2023 at 10:21:36AM -0500, Mario Limonciello wrote:
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2976,6 +2976,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  
>  	switch (pci_pcie_type(bridge)) {
>  	case PCI_EXP_TYPE_ROOT_PORT:
> +		if (!platform_pci_power_manageable(bridge))
> +			return false;
> +		fallthrough;
>  	case PCI_EXP_TYPE_UPSTREAM:
>  	case PCI_EXP_TYPE_DOWNSTREAM:
>  		if (pci_bridge_d3_disable)

This will exempt the Root Ports from pcie_port_pm=force.
Not sure if that's desirable.

Thanks,

Lukas

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

[Limonciello, Mario]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Wednesday, May 24, 2023 10:45 AM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Rafael J . Wysocki <rafael@kernel.org>;
> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; S-k, Shyam-sundar
> <Shyam-sundar.S-k@amd.com>; Natikar, Basavaraj
> <Basavaraj.Natikar@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; linux-pm@vger.kernel.org; Iain Lane
> <iain@orangesquash.org.uk>
> Subject: Re: [PATCH v3] PCI: Don't assume root ports from > 2015 are power
> manageable
>
> On Wed, May 24, 2023 at 10:21:36AM -0500, Mario Limonciello wrote:
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2976,6 +2976,9 @@ bool pci_bridge_d3_possible(struct pci_dev
> *bridge)
> >
> >     switch (pci_pcie_type(bridge)) {
> >     case PCI_EXP_TYPE_ROOT_PORT:
> > +           if (!platform_pci_power_manageable(bridge))
> > +                   return false;
> > +           fallthrough;
> >     case PCI_EXP_TYPE_UPSTREAM:
> >     case PCI_EXP_TYPE_DOWNSTREAM:
> >             if (pci_bridge_d3_disable)
>
> This will exempt the Root Ports from pcie_port_pm=force.
> Not sure if that's desirable.

Right; It will only exempt root ports from pcie_port_pm=force
if they aren't power manageable.

If it's still desirable to let pcie_port_pm=force work on these
I think it's worth refactoring the function otherwise it's going
to be a nested if that matches the same variable as the
switch.

Something like this:

bool pci_bridge_d3_possible(struct pci_dev *bridge)
{
        if (!pci_is_pcie(bridge))
                return false;

        switch (pci_pcie_type(bridge)) {
        case PCI_EXP_TYPE_ROOT_PORT:
        case PCI_EXP_TYPE_UPSTREAM:
        case PCI_EXP_TYPE_DOWNSTREAM:
                break;
        default:
                return false;
        }

        if (pci_bridge_d3_disable)
                return false;

        /*
         * Hotplug ports handled by firmware in System Management Mode
         * may not be put into D3 by the OS (Thunderbolt on non-Macs).
         */
        if (bridge->is_hotplug_bridge && !pciehp_is_native(bridge))
                return false;

        if (pci_bridge_d3_force)
                return true;

        /* Even the oldest 2010 Thunderbolt controller supports D3. */
        if (bridge->is_thunderbolt)
                return true;

        /* Platform might know better if the bridge supports D3 */
        if (platform_pci_bridge_d3(bridge))
                return true;

        /*
         * Hotplug ports handled natively by the OS were not validated
         * by vendors for runtime D3 at least until 2018 because there
         * was no OS support.
         */
        if (bridge->is_hotplug_bridge)
                return false;

        if (dmi_check_system(bridge_d3_blacklist))
                return false;

        /*
         * It should be safe to put PCIe ports from 2015 or newer
         * to D3.
         */
        if (dmi_get_bios_year() >= 2015)
                return true;

        return false;
}

Then the check I'm proposing can injected anywhere after the force like this:

if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
    !platform_pci_power_manageable(bridge)))
        return false;

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

[Rafael J. Wysocki]

On Wed, May 24, 2023 at 6:16 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> > -----Original Message-----
> > From: Lukas Wunner <lukas@wunner.de>
> > Sent: Wednesday, May 24, 2023 10:45 AM
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>; Mika Westerberg
> > <mika.westerberg@linux.intel.com>; Rafael J . Wysocki <rafael@kernel.org>;
> > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; S-k, Shyam-sundar
> > <Shyam-sundar.S-k@amd.com>; Natikar, Basavaraj
> > <Basavaraj.Natikar@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; linux-pm@vger.kernel.org; Iain Lane
> > <iain@orangesquash.org.uk>
> > Subject: Re: [PATCH v3] PCI: Don't assume root ports from > 2015 are power
> > manageable
> >
> > On Wed, May 24, 2023 at 10:21:36AM -0500, Mario Limonciello wrote:
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2976,6 +2976,9 @@ bool pci_bridge_d3_possible(struct pci_dev
> > *bridge)
> > >
> > >     switch (pci_pcie_type(bridge)) {
> > >     case PCI_EXP_TYPE_ROOT_PORT:
> > > +           if (!platform_pci_power_manageable(bridge))
> > > +                   return false;
> > > +           fallthrough;
> > >     case PCI_EXP_TYPE_UPSTREAM:
> > >     case PCI_EXP_TYPE_DOWNSTREAM:
> > >             if (pci_bridge_d3_disable)
> >
> > This will exempt the Root Ports from pcie_port_pm=force.
> > Not sure if that's desirable.
>
> Right; It will only exempt root ports from pcie_port_pm=force
> if they aren't power manageable.
>
> If it's still desirable to let pcie_port_pm=force work on these
> I think it's worth refactoring the function otherwise it's going
> to be a nested if that matches the same variable as the
> switch.
>
> Something like this:
>
> bool pci_bridge_d3_possible(struct pci_dev *bridge)
> {
>         if (!pci_is_pcie(bridge))
>                 return false;
>
>         switch (pci_pcie_type(bridge)) {
>         case PCI_EXP_TYPE_ROOT_PORT:
>         case PCI_EXP_TYPE_UPSTREAM:
>         case PCI_EXP_TYPE_DOWNSTREAM:
>                 break;
>         default:
>                 return false;
>         }
>
>         if (pci_bridge_d3_disable)
>                 return false;
>
>         /*
>          * Hotplug ports handled by firmware in System Management Mode
>          * may not be put into D3 by the OS (Thunderbolt on non-Macs).
>          */
>         if (bridge->is_hotplug_bridge && !pciehp_is_native(bridge))
>                 return false;
>
>         if (pci_bridge_d3_force)
>                 return true;
>
>         /* Even the oldest 2010 Thunderbolt controller supports D3. */
>         if (bridge->is_thunderbolt)
>                 return true;
>
>         /* Platform might know better if the bridge supports D3 */
>         if (platform_pci_bridge_d3(bridge))
>                 return true;
>
>         /*
>          * Hotplug ports handled natively by the OS were not validated
>          * by vendors for runtime D3 at least until 2018 because there
>          * was no OS support.
>          */
>         if (bridge->is_hotplug_bridge)
>                 return false;
>
>         if (dmi_check_system(bridge_d3_blacklist))
>                 return false;
>
>         /*
>          * It should be safe to put PCIe ports from 2015 or newer
>          * to D3.
>          */
>         if (dmi_get_bios_year() >= 2015)
>                 return true;
>
>         return false;
> }
>
> Then the check I'm proposing can injected anywhere after the force like this:
>
> if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
>     !platform_pci_power_manageable(bridge)))
>         return false;

Sounds reasonable.  I would even put it after the Thunderbolt check.