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
>
