From: "Limonciello, Mario" <mario.limonciello@amd.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
Len Brown <lenb@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
Mika Westerberg <mika.westerberg@linux.intel.com>,
Mehta Sanju <Sanju.Mehta@amd.com>, Lukas Wunner <lukas@wunner.de>,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3
Date: Fri, 11 Nov 2022 12:58:28 -0600 [thread overview]
Message-ID: <5754039e-e3c6-cdb8-8d64-6332b0093940@amd.com> (raw)
In-Reply-To: <20221111174101.GA729137@bhelgaas>
On 11/11/2022 11:41, Bjorn Helgaas wrote:
> On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
>> Firmware typically advertises that ACPI devices that represent PCIe
>> devices can support D3 by a combination of the value returned by
>> _S0W as well as the HotPlugSupportInD3 _DSD [1].
>>
>> `acpi_pci_bridge_d3` looks for this combination but also contains
>> an assumption that if an ACPI device contains power resources the PCIe
>> device it's associated with can support D3. This was introduced
>> from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
>> D3 if power managed by ACPI").
>>
>> Some firmware configurations for "AMD Pink Sardine" do not support
>> wake from D3 in _S0W for the ACPI device representing the PCIe root
>> port used for tunneling. The PCIe device will still be opted into
>> runtime PM in the kernel [2] because of the logic within
>> `acpi_pci_bridge_d3`. This currently happens because the ACPI
>> device contains power resources.
>>
>> When the thunderbolt driver is loaded two device links are created:
>> * USB4 router <- PCIe root port for tunneling
>> * USB4 router <- XHCI PCIe device
>>
>> These device links are created because the ACPI devices declare the
>> `usb4-host-interface` _DSD [3]. For both links the USB4 router is the
>> supplier and these other devices are the consumers.
>> Here is a demonstration of this topology that occurs:
>>
>> |
>> ├─ 00:03.1
>> | | "PCIe root port used for tunneling"
>> | | ACPI Path: \_SB_.PCI0.GP11
>> | | ACPI Power Resources: Yes
>
> I guess this means it has _PR0 and/or _PS0? (same for devices below)
Yeah.
>
>> | | ACPI _S0W return value: 0
>> | | Device Links: supplier:pci:0000:c4:00.5
>> | └─ PCIe Power state: D0
>
> What bus does 00:03.1 lead to? I guess it doesn't lead to any of the
> devices below?
By default - nothing. The topology was drawn correctly.
When you plug in a USB4 device with PCIe the newly enumerated devices
hang off this bus.
>
>> └─ 00:08.3
>> | ACPI Path: \_SB_.PCI0.GP19
>> ├─ PCIe Power state: D0
>
> I guess 00:08.3 is a Root Port leading to bus c4?
>
Right.
>> ├─ c4:00.3
>> | | "XHCI PCIe device used for tunneling"
>> | | ACPI Path: \_SB_.PCI0.GP19.XHC3
>> | | ACPI Power Resources: Yes
>> | | ACPI _S0W return value: 4
>> | | Device Links: supplier:pci:0000:c4:00.5
>> | └─ PCIe Power state: D3cold
>> └─ c4:00.5
>> | "USB4 Router"
>> | ACPI Path: \_SB_.PCI0.GP19.NHI0
>> | ACPI Power Resources: Yes
>> | ACPI _S0W return value: 4
>> | Device Links: consumer:pci:0000:00:03.1 consumer:pci:0000:c4:00.3
>> └─ PCIe Power state: D3cold
>
> I'm reading the excellent Documentation/driver-api/device_link.rst and
> trying to match up with this topology. This is a case where 00:03.1
> is a consumer of c4:00.5. The driver core automatically enforces that
> children are suspended before parents, but since 00:03.1 is an aunt
> (not a child) of c4:00.5, the device link enforces the requirement
> that 00:03.1 be suspended before c4:00.5. Right?
Yup!
>
> Is c4:00.5 an NHI?
Yes.
>
> The PCI power states shown above are the failure case? c4:00.5
> *should* remain in D0 as long as 00:03.1 is in D0, but was instead put
> in D3cold?
Yes.
>
>> Currently runtime PM is allowed for all of these devices.
>
> This is because they all have _PR0 and/or _PS0? (Diagram doesn't show
> that for 00:08.3, but I assume it must?)
For the root port used for tunneling it's because
pci_bridge_d3_possible() returns TRUE.
This returns true because platform_pci_bridge(d3) return TRUE.
For the NHI it's because thunderbolt.ko sets this for all NHIs (see
drivers/thunderbolt/nhi.c).
>
> And I guess they all must have dev->is_hotplug_bridge set?
The PCIe root port for tunneling does; yes.
>
>> This means that
>> when all consumers are idle long enough, they will runtime suspend to
>> enter their deepest allowed sleep state. Once all consumers are in their
>> deepest allowed sleep state the suppliers will enter the deepest sleep
>> state as well.
>>
>> * The PCIe root port for tunneling doesn't support waking from D3hot or
>> D3cold so although runtime suspended it stays in D0.
>> * The XHCI PCIe device supports wakeup from D3cold so it runtime suspends
>> to D3cold.
>> * Both consumers are in their deepest state and the USB4 router supports
>> wakeup from D3cold, so it runtime suspends into this state.
>>
>> At least for AMD's case the hardware designer's expectation is the USB4
>> router should have also remained in D0 since the PCIe root port for
>> tunneling remained in D0 and a device link exists between the two devices.
>> The current Linux behavior is undefined.
>
> Is the requirement that the supplier can never be in a lower-power
> state than the consumer?
>
> I guess that's *not* actually a requirement even though that's the
> effect of this patch in this situation. If it *were* that simple, we
> would just check the supplier and consumer power states instead of
> looking at all these ACPI properties.
Yeah I think that's too broad of a generalization.
I don't know how realistic this is but for example what if the supplier
supported D3 but the consumer supported D2?
>
>> Instead of making the assertion that the device can support D3 (and thus
>> runtime PM) solely from the presence of ACPI power resources, move the check
>> to later on in the function, which will have validated that the ACPI device
>> supports wake from D3hot or D3cold.
>>
>> This fix prevents the USB4 router being put into D3 when the firmware
>> says that ACPI device representing the PCIe root port for tunneling can't
>> handle it while still allowing ACPI devices that don't have the
>> HotplugSupportInD3 _DSD to also enter D3 if they have power resources that
>> can wake from D3.
>
> So I guess this changes what acpi_pci_bridge_d3() returns for 00:03.1?
> Previously it returned "true" because it has _PR0/_PS0 (so
> acpi_pci_power_manageable() is true), but now it will return "false"
> because _S0W says it can only wake from D0?
>
Exactly.
next prev parent reply other threads:[~2022-11-11 18:58 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-31 22:33 [PATCH v5] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3 Mario Limonciello
2022-11-11 17:41 ` Bjorn Helgaas
2022-11-11 18:58 ` Limonciello, Mario [this message]
2022-11-11 21:42 ` Bjorn Helgaas
2022-11-14 15:33 ` Rafael J. Wysocki
2022-11-14 15:37 ` Limonciello, Mario
2022-11-14 16:54 ` Bjorn Helgaas
2022-11-16 0:37 ` Bjorn Helgaas
2022-11-16 2:31 ` Limonciello, Mario
2022-11-16 12:00 ` Rafael J. Wysocki
2022-11-16 23:28 ` Bjorn Helgaas
2022-11-17 17:01 ` Rafael J. Wysocki
2022-11-17 22:16 ` Bjorn Helgaas
2022-11-18 13:16 ` Rafael J. Wysocki
2022-11-18 20:23 ` Bjorn Helgaas
2022-11-18 21:13 ` Rafael J. Wysocki
2022-11-21 14:33 ` Rafael J. Wysocki
2022-11-21 22:17 ` Bjorn Helgaas
2023-01-02 16:34 ` Rafael J. Wysocki
2023-01-02 16:59 ` Rafael J. Wysocki
2023-01-03 22:44 ` Limonciello, Mario
2023-01-10 18:02 ` Rafael J. Wysocki
2023-01-10 20:55 ` Bjorn Helgaas
2023-01-11 10:56 ` Rafael J. Wysocki
2023-01-12 20:13 ` Bjorn Helgaas
2023-01-11 10:38 ` [PATCH v3] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(() Rafael J. Wysocki
2023-01-12 20:21 ` Bjorn Helgaas
2023-01-12 20:31 ` Rafael J. Wysocki
2023-01-12 20:51 ` [PATCH v4] " Rafael J. Wysocki
2023-01-12 22:01 ` Bjorn Helgaas
2023-01-12 22:09 ` Limonciello, Mario
2023-01-12 22:40 ` Bjorn Helgaas
2023-01-12 22:45 ` Limonciello, Mario
2023-01-13 17:51 ` Bjorn Helgaas
2023-01-13 17:53 ` Limonciello, Mario
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5754039e-e3c6-cdb8-8d64-6332b0093940@amd.com \
--to=mario.limonciello@amd.com \
--cc=Sanju.Mehta@amd.com \
--cc=bhelgaas@google.com \
--cc=helgaas@kernel.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=mika.westerberg@linux.intel.com \
--cc=rafael.j.wysocki@intel.com \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).