On Wed, Mar 23, 2022 at 11:01:05AM -0500, Mario Limonciello wrote: > On 3/15/22 11:36, Rafael J. Wysocki wrote: > > On Tue, Mar 15, 2022 at 5:35 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > > > > > > On Tue, Mar 15, 2022 at 4:33 PM Mario Limonciello > > > <mario.limonciello@xxxxxxx> wrote: > > > > > > > > According to the Microsoft spec the _DSD `HotPlugSupportInD3` is > > > > indicates the ability for a bridge to be able to wakeup from D3: > > > > > > > > This ACPI object [HotPlugSupportInD3] enables the operating system > > > > to identify and power manage PCIe Root Ports that are capable of > > > > handling hot plug events while in D3 state. > > > > > > > > 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 returns "0" indicating the inability to wake up the > > > > device from D3 as explained in the ACPI specification: > > > > > > > > 7.3.20 _S0W (S0 Device Wake State) > > > > > > > > This object evaluates to an integer that conveys to OSPM the deepest > > > > D-state supported by this device in the S0 system sleeping state > > > > where the device can wake itself. > > > > > > > > This mismatch may lead to being unable to enumerate devices behind the > > > > hotplug bridge when a device is plugged in. To remedy these situations > > > > that `HotPlugSupportInD3` is specified by _S0W returns 0, explicitly > > > > check that the ACPI companion has returned _S0W greater than or equal > > > > to 3 and the device has a GPE allowing the device to generate wakeup > > > > signals handled by the platform in `acpi_pci_bridge_d3`. > > > > > > > > Windows 10 and Windows 11 both will prevent the bridge from going in D3 > > > > when the firmware is configured this way and this changes aligns the > > > > handling of the situation to be the same. > > > > > > > > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fhtmlspecs%2FACPI_Spec_6_4_html%2F07_Power_and_Performance_Mgmt%2Fdevice-power-management-objects.html%3Fhighlight%3Ds0w%23s0w-s0-device-wake-state&data=04%7C01%7Cmario.limonciello%40amd.com%7Cae5d5d8f4ac1452dac0c08da06a200b2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829590116901616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3cTSyBgSEOBo5mPeUcNDrc9qhdMJeZ4cVtLqroDRZqY%3D&reserved=0 > > > > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-for-pcie-root-ports%23identifying-pcie-root-ports-supporting-hot-plug-in-d3&data=04%7C01%7Cmario.limonciello%40amd.com%7Cae5d5d8f4ac1452dac0c08da06a200b2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829590116901616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=rZHk%2FgII0qOvr9cmmB8N1auNc1nLSFViVTG9f%2FwEREY%3D&reserved=0 > > > > Fixes: 26ad34d510a87 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports") > > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > > > > > > No more comments from me: > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > Or please let me know if I should pick it up. > > Bjorn, > > Friendly reminder on this one. Thanks; we're in the middle of the merge window now, so I'll wait till that's over unless this is an emergency. IIUC the bug this fixes is that "when a bridge is in D3cold, we don't notice when a device is hot-added below it," right? So we need to avoid putting the bridge in D3cold? Is there a typical scenario where this bites users? I don't think we ever saw an actual problem report? > > > > --- > > > > changes from v3->v4: > > > > * rework comment > > > > * only evaluate _S0W if necessary > > > > * drop static function with only one caller > > > > > > > > drivers/pci/pci-acpi.c | 17 ++++++++++++++++- > > > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > > > index a42dbf448860..e535dab2c888 100644 > > > > --- a/drivers/pci/pci-acpi.c > > > > +++ b/drivers/pci/pci-acpi.c > > > > @@ -977,6 +977,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev) > > > > const union acpi_object *obj; > > > > struct acpi_device *adev; > > > > struct pci_dev *rpdev; > > > > + unsigned long long ret; > > > > > > > > if (acpi_pci_disabled || !dev->is_hotplug_bridge) > > > > return false; > > > > @@ -1003,7 +1004,21 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev) > > > > ACPI_TYPE_INTEGER, &obj) < 0) > > > > return false; > > > > > > > > - return obj->integer.value == 1; > > > > + if (!obj->integer.value) > > > > + return false; > > > > + > > > > + /* > > > > + * If 'HotPlugSupportInD3' is set, but wakeup is not actually supported, > > > > + * the former cannot be trusted anyway, so validate it by verifying the > > > > + * latter. > > > > + */ > > > > + if (!adev->wakeup.flags.valid) > > > > + return false; > > > > + > > > > + if (ACPI_FAILURE(acpi_evaluate_integer(adev->handle, "_S0W", NULL, &ret))) > > > > + return false; > > > > + > > > > + return ret >= ACPI_STATE_D3_HOT; > > > > } > > > > > > > > int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) > > > > -- > > > > 2.34.1 > > > > >
