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@xxxxxxx> 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@xxxxxxx>
> ---
>  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
>