On Fri, Apr 1, 2022 at 5:40 AM Mario Limonciello <mario.limonciello@xxxxxxx> wrote: > > acpi_pci_bridge_d3(dev) returns "true" if "dev" is a hotplug bridge > that can handle hotplug events while in D3. Previously this meant: > > 1) "dev" has a _PS0 or _PR0 method, or > > 2) The Root Port above "dev" has a _DSD with a "HotPlugSupportInD3" > property with value 1. > > This did not consider_S0W, which tells us the deepest D-state from > which a device can wake itself (ACPI v6.4, sec 7.3.20). > > On some platforms, e.g., AMD Yellow Carp, firmware may supply > "HotPlugSupportInD3" even though the platform does not supply power > resources through _PRW and _S0W tells us the device cannot wake from More importantly, it doesn't supply a wakeup GPE number through _PRW, so from the ACPI perspective the bridge has no way of generating wakeup signals. > D3hot. With the previous code, these devices could be put in D3hot > and hotplugged devices would not be recognized without manually > rescanning. > > If _S0W exists and says the Root Port cannot wake itself from D3hot, > return "false" to indicate that "dev" cannot handle hotplug events > while in D3. > > 1) "dev" has a _PS0 or _PR0 method, or > > 2a) The Root Port above "dev" has _PRW and > > 2b) If the Root Port above "dev" has _S0W, it can wake from D3hot or > D3cold and > > 2c) The Root Port above "dev" has a _DSD with a > "HotPlugSupportInD3" property with value 1. > > Windows 10 and Windows 11 both will prevent the bridge from going in D3 > when the firmware is configured this way and this change aligns the > handling of the situation to be the same. > > 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") > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > -- > v5->v6: > * Re-order checks to only check for _DSD if necessary as suggested by Bjorn. > * Adjust commit message wording > * Drop rewording comment patch, just apply it while moving text to avoid > ping-ponging the same lines in the commits. > v4-v5: > * Don't fail if _S0W is missing > --- > drivers/pci/pci-acpi.c | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 1f15ab7eabf8..5ab797e2709d 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -976,7 +976,9 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev) > { > const union acpi_object *obj; > struct acpi_device *adev; > + unsigned long long state; > struct pci_dev *rpdev; > + acpi_status status; > > if (acpi_pci_disabled || !dev->is_hotplug_bridge) > return false; > @@ -985,12 +987,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev) > if (acpi_pci_power_manageable(dev)) > return true; > > - /* > - * The ACPI firmware will provide the device-specific properties through > - * _DSD configuration object. Look for the 'HotPlugSupportInD3' property > - * for the root port and if it is set we know the hierarchy behind it > - * supports D3 just fine. > - */ > rpdev = pcie_find_root_port(dev); > if (!rpdev) > return false; > @@ -999,6 +995,23 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev) > if (!adev) > return false; > > + /* > + * If the bridge can't wake from D3hot, it can't signal hotplug > + * events in D3hot. I would say "If the bridge cannot signal wakeup signals at all, it cannot signal hotplug events from low-power states including D3hot and D3cold." With the two above remarks addressed, please feel free to add Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> to this patch. > + */ > + if (!adev->wakeup.flags.valid) > + return false; > + > + status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state); > + if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT) > + return false; > + > + /* > + * The ACPI firmware will provide the device-specific properties through > + * _DSD configuration object. Look for the 'HotPlugSupportInD3' property > + * for the root port and if it is set we make an assumption that the > + * hierarchy behind it supports D3 as well. > + */ > if (acpi_dev_get_property(adev, "HotPlugSupportInD3", > ACPI_TYPE_INTEGER, &obj) < 0) > return false; > -- > 2.34.1 >
