Re: [PATCH v3] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3

[Rafael J. Wysocki]

On Wed, Oct 26, 2022 at 12:10 AM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> Firmware typically advertises that PCIe devices can support D3
> by a combination of the value returned by _S0W as well as the
> HotPlugSupportInD3 _DSD.
>
> `acpi_pci_bridge_d3` looks for this combination but also contains
> an assumption that if a device contains power resources it can support
> D3.  This was introduced from commit c6e331312ebf ("PCI/ACPI: Whitelist
> hotplug ports for D3 if power managed by ACPI").
>
> On some firmware configurations for "AMD Pink Sardine" D3 is not
> supported for wake in _S0W for the PCIe root port for tunneling.
> However the device will still be opted into runtime PM since
> `acpi_pci_bridge_d3` returns since the ACPI device contains power
> resources.
>
> When the thunderbolt driver is loaded a device link between the USB4
> router and the PCIe root port for tunneling is created where the PCIe
> root port for tunneling is the consumer and the USB4 router is the
> supplier.  Here is a demonstration of this topology that occurs:
>
> ├─ 0000:00:03.1
> |       | ACPI Path: \_SB_.PCI0.GP11 (Supports "0" in _S0W)
> |       | Device Links: supplier:pci:0000:c4:00.5
> |       └─ D0 (Runtime PM enabled)
> ├─ 0000:00:04.1
> |       | ACPI Path: \_SB_.PCI0.GP12 (Supports "0" in _S0W)
> |       | Device Links: supplier:pci:0000:c4:00.6
> |       └─ D0 (Runtime PM enabled)
> ├─ 0000:00:08.3
> |       | ACPI Path: \_SB_.PCI0.GP19
> |       ├─ D0 (Runtime PM disabled)
> |       ├─ 0000:c4:00.3
> |       |       | ACPI Path: \_SB_.PCI0.GP19.XHC3
> |       |       | Device Links: supplier:pci:0000:c4:00.5
> |       |       └─ D3cold (Runtime PM enabled)
> |       ├─ 0000:c4:00.4
> |       |       | ACPI Path: \_SB_.PCI0.GP19.XHC4
> |       |       | Device Links: supplier:pci:0000:c4:00.6
> |       |       └─ D3cold (Runtime PM enabled)
> |       ├─ 0000:c4:00.5
> |       |       | ACPI Path: \_SB_.PCI0.GP19.NHI0 (Supports "4" in _S0W)
> |       |       | Device Links: consumer:pci:0000:00:03.1 consumer:pci:0000:c4:00.3
> |       |       └─ D3cold (Runtime PM enabled)
> |       └─ 0000:c4:00.6
> |               | ACPI Path: \_SB_.PCI0.GP19.NHI1 (Supports "4" in _S0W)
> |               | Device Links: consumer:pci:0000:c4:00.4 consumer:pci:0000:00:04.1
> |               └─ D3cold (Runtime PM enabled)
>
> Allowing the PCIe root port for tunneling to go into runtime PM (even if
> it doesn't support D3) allows the USB4 router to also go into runtime PM.
> The PCIe root port for tunneling stays in D0 but is in runtime PM. Due to
> the device link the USB4 router transitions to D3cold when this happens.
>
> The expectation is the USB4 router should have also remained in D0 since
> the PCIe root port for tunneling remained in D0.
>
> Instead of making this assertion from the power resources check
> immediately, move the check to later on, which will have validated
> that the device supports wake from D3hot or D3cold.
>
> This fix prevents the USB4 router going into D3 when the firmware says that
> the PCIe root port for tunneling can't handle it while still allowing
> system that don't have the HotplugSupportInD3 _DSD to also enter D3 if they
> have power resources that can wake from D3.
>
> Fixes: dff6139015dc6 ("PCI/ACPI: Allow D3 only if Root Port can signal and wake from D3")
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

> ---
> v2->v3:
>  * Reword commit message
> v1->v2:
>  * Just return value of acpi_pci_power_manageable (Rafael)
>  * Remove extra word in commit message
> ---
>  drivers/pci/pci-acpi.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a46fec776ad77..8c6aec50dd471 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,7 +1019,8 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>             obj->integer.value == 1)
>                 return true;
>
> -       return false;
> +       /* Assume D3 support if the bridge is power-manageable by ACPI. */
> +       return acpi_pci_power_manageable(dev);
>  }
>
>  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> --
> 2.34.1
>