| Message ID | 20220309224302.2625343-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: ACPI: Don't blindly trust `HotPlugSupportInD3` | expand |
+Rafael On Wed, Mar 09, 2022 at 04:43:02PM -0600, Mario Limonciello 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 system from D3. Ideally the BIOS should not allow this to happen in the first place but yeah we've seen all kinds of weird behaviour in the past so just need to deal with it :/ I wonder if it makes sense to log this situation? > To fix these situations explicitly check that the ACPI device claims the > system can be awoken in `acpi_pci_bridge_d3`. > > 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@amd.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > 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; > + > if (acpi_dev_get_property(adev, "HotPlugSupportInD3", > ACPI_TYPE_INTEGER, &obj) < 0) > return false; > -- > 2.34.1
[Public] > -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Thursday, March 10, 2022 03:56 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com>; open list:PCI SUBSYSTEM <linux- > pci@vger.kernel.org>; Mehta, Sanju <Sanju.Mehta@amd.com>; Rafael J. > Wysocki <rafael.j.wysocki@intel.com> > Subject: Re: [PATCH] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3` > > +Rafael > > On Wed, Mar 09, 2022 at 04:43:02PM -0600, Mario Limonciello 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 system from D3. > > Ideally the BIOS should not allow this to happen in the first place but > yeah we've seen all kinds of weird behaviour in the past so just need > to deal with it :/ > > I wonder if it makes sense to log this situation? > We double checked how Windows handles these circumstances and this change makes it match how Windows handles it too. > > To fix these situations explicitly check that the ACPI device claims the > > system can be awoken in `acpi_pci_bridge_d3`. > > > > 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%7Cde80f5d9ce > 484b608b0c08da027c84d3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > C0%7C637825031070157114%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w > LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am > p;sdata=mD6EJTcL5V0syIkscYe5kmmbyg3zDqEQBwP8zCAO5h4%3D&res > erved=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%7Cde80f5d9ce48 > 4b608b0c08da027c84d3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0 > %7C637825031070157114%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&s > data=S2JtOwrUHOb5VXNV3BepBS2VxlF0s9FW4DTMHC54ULU%3D&res > erved=0 > > Fixes: 26ad34d510a87 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug > ports") > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Thanks! > > --- > > 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; > > + > > if (acpi_dev_get_property(adev, "HotPlugSupportInD3", > > ACPI_TYPE_INTEGER, &obj) < 0) > > return false; > > -- > > 2.34.1
On 3/10/2022 10:56 AM, Mika Westerberg wrote: > +Rafael Thanks! +linux-pm (all PM-related stuff should go there) > > On Wed, Mar 09, 2022 at 04:43:02PM -0600, Mario Limonciello 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 system from D3. It is "wake up the device from D3". The system is already in S0. > Ideally the BIOS should not allow this to happen in the first place but > yeah we've seen all kinds of weird behaviour in the past so just need > to deal with it :/ > > I wonder if it makes sense to log this situation? > >> To fix these situations explicitly check that the ACPI device claims the >> system can be awoken in `acpi_pci_bridge_d3`. This is not too precise. wakeup.flags.valid is 1 if there is a GPE allowing the device to generate wakeup signals that will be handled by the platform. Again, this is not only about waking up the system from sleep. >> 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@amd.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >> --- >> 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; >> + >> if (acpi_dev_get_property(adev, "HotPlugSupportInD3", >> ACPI_TYPE_INTEGER, &obj) < 0) >> return false; >> -- >> 2.34.1
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; + if (acpi_dev_get_property(adev, "HotPlugSupportInD3", ACPI_TYPE_INTEGER, &obj) < 0) return false;
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 system from D3. To fix these situations explicitly check that the ACPI device claims the system can be awoken in `acpi_pci_bridge_d3`. 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@amd.com> --- drivers/pci/pci-acpi.c | 3 +++ 1 file changed, 3 insertions(+)