[PATCH] PCI/ACPI: Don't assume D3 support if a device is power manageable

[PATCH] PCI/ACPI: Don't assume D3 support if a device is power manageable

[Mario Limonciello]

On some firmware configurations where D3 is not supported for
"AMD Pink Sardine" the PCIe root port for tunneling will still be
opted into runtime PM since `acpi_pci_bridge_d3` returns true.

This later causes the device link between the USB4 router and the
PCIe root port for tunneling to misbehave.  The USB4 router may
enters D3 via runtime PM, but the PCIe root port for tunneling
remains in D0.  The expectation is the USB4 router should also
remain in D0 since the PCIe root port for tunneling remained in D0.

`acpi_pci_bridge_d3` has a number of checks, but starts out with an
assumption that if a device is power manageable introduced from
commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for D3 if power
managed by ACPI") that it will support D3.  This is not a valid
assumption, as the PCIe root port for tunneling has power resources
but does not support D3hot or D3cold.

Instead of making this assertion from the power resources check
immediately, move the check to later on, which will have validated
that D3hot or D3cold can actually be reached.

This fixes the USB4 router going into D3 when the firmware says that
the PCIe root port for tunneling can't handle it.

Fixes: dff6139015dc6 ("PCI/ACPI: Allow D3 only if Root Port can signal and wake from D3")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci-acpi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a46fec776ad77..1e774a4645663 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
 		return false;
 
-	/* Assume D3 support if the bridge is power-manageable by ACPI. */
-	if (acpi_pci_power_manageable(dev))
-		return true;
-
 	rpdev = pcie_find_root_port(dev);
 	if (!rpdev)
 		return false;
@@ -1023,6 +1019,10 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	    obj->integer.value == 1)
 		return true;
 
+	/* Assume D3 support if the bridge is power-manageable by ACPI. */
+	if (acpi_pci_power_manageable(dev))
+		return true;
+
 	return false;
 }
 
-- 
2.34.1

Re: [PATCH] PCI/ACPI: Don't assume D3 support if a device is power manageable

[Rafael J. Wysocki]

On Wed, Oct 19, 2022 at 7:59 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On some firmware configurations where D3 is not supported for
> "AMD Pink Sardine" the PCIe root port for tunneling will still be
> opted into runtime PM since `acpi_pci_bridge_d3` returns true.
>
> This later causes the device link between the USB4 router and the
> PCIe root port for tunneling to misbehave.  The USB4 router may
> enters D3 via runtime PM, but the PCIe root port for tunneling
> remains in D0.  The expectation is the USB4 router should also
> remain in D0 since the PCIe root port for tunneling remained in D0.
>
> `acpi_pci_bridge_d3` has a number of checks, but starts out with an
> assumption that if a device is power manageable introduced from
> commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for D3 if power
> managed by ACPI") that it will support D3.  This is not a valid
> assumption, as the PCIe root port for tunneling has power resources
> but does not support D3hot or D3cold.
>
> Instead of making this assertion from the power resources check
> immediately, move the check to later on, which will have validated
> that D3hot or D3cold can actually be reached.
>
> This fixes the USB4 router going into D3 when the firmware says that
> the PCIe root port for tunneling can't handle it.
>
> Fixes: dff6139015dc6 ("PCI/ACPI: Allow D3 only if Root Port can signal and wake from D3")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci-acpi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a46fec776ad77..1e774a4645663 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>         if (acpi_pci_disabled || !dev->is_hotplug_bridge)
>                 return false;
>
> -       /* Assume D3 support if the bridge is power-manageable by ACPI. */
> -       if (acpi_pci_power_manageable(dev))
> -               return true;
> -
>         rpdev = pcie_find_root_port(dev);
>         if (!rpdev)
>                 return false;
> @@ -1023,6 +1019,10 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>             obj->integer.value == 1)
>                 return true;
>
> +       /* Assume D3 support if the bridge is power-manageable by ACPI. */
> +       if (acpi_pci_power_manageable(dev))
> +               return true;
> +
>         return false;

return acpi_pci_power_manageable(dev);

would be simpler.

Otherwise it looks OK to me.

>  }
>
> --
> 2.34.1
>