On 3/11/2022 10:05, Rafael J. Wysocki wrote:
On Thu, Mar 10, 2022 at 9:13 PM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
[Public]
To fix these situations explicitly check that the ACPI device has a GPE
allowing the device to generate wakeup signals handled by the platform
in `acpi_pci_bridge_d3`.
Which may be orthogonal to the _S0W return value mentioned above.
Also, I'm not quite sure why acpi_pci_bridge_d3() should require the
root port to have a wake GPE associated with it as an indication that
the hierarchy below it can be put into D3cold.
The reason that brought me down the path in this patch was actually
acpi_dev_pm_get_state. _S0W isn't actually evaluated unless
adev->wakeup.flags.valid is set.
That's true, but it is unclear how this is related to whether or not a
given PCIe port can handle D3cold. But see below.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a42dbf448860..9f8f55ed09d9 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -999,6 +999,9 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
if (!adev)
return false;
+ if (!adev->wakeup.flags.valid)
+ return false;
Minor nit: the two checks above could be combined.
OK if we stick to this approach I'll do that.
Also I would add a comment explaining why exactly wakeup.flags.valid
is checked here, because I can imagine a case in which the wakeup
signaling capability is irrelevant for whether or not the given port
can handle D3cold.
Specifically a case that it's a hotplug bridge that has HotPlugSupportInD3
though? In practice I've only seen that in use on USB4 and Thunderbolt
bridges "so far".
I haven't tried yet but I would think directly evaluating _S0W at this time
seems it should also work and would match closer to my original intent
of the patch. Would you prefer that?
I guess, but I'm not sure, that you are trying to kind of validate
HotPlugSupportInD3 by checking if the root port in question actually
can signal wakeup via ACPI and if it cannot, assume that the flag was
set by mistake and so the bridge should not be assumed to be able to
handle D3cold.
That is not unreasonable, but in that case you need to check
wakeup.flags.valid first and then _S0W too, because it can return 0
even if the "valid" flag is set. And explain in a comment why this is
done.
OK I'll make these changes and check the situation again.
+
if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
ACPI_TYPE_INTEGER, &obj) < 0)
return false;
--
