On Thu, Mar 10, 2022 at 6:58 PM Mario Limonciello <mario.limonciello@xxxxxxx> wrote: > > The `_DSD` `HotPlugSupportInD3` is supposed to indicate the ability for a > bridge to be able to wakeup from D3. > > This however is static information in the ACPI table at BIOS compilation > time and on some platforms it's possible to configure the firmware at boot > up such that `_S0W` will not return "0" indicating the inability to wake > up the device from D3. To be precise, _S0W returning 0 means that the device cannot generate wakeup signals from any D-states other than D0 while the system as a whole is in S0. > 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. > This changes aligns the handling of the situation the same as Windows 10 > and Windows 11 both do as well. > > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html?highlight=s0w#s0w-s0-device-wake-state > Link: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3 > Fixes: 26ad34d510a87 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports") > Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > --- > v1->v2: > * Add Mika's tag > * Update commit message for Rafael's suggestions > drivers/pci/pci-acpi.c | 3 +++ > 1 file changed, 3 insertions(+) > > 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. 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. > + > if (acpi_dev_get_property(adev, "HotPlugSupportInD3", > ACPI_TYPE_INTEGER, &obj) < 0) > return false; > --
