[PATCH] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3`

[PATCH] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3`

[Mario Limonciello]

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(+)

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

Re: [PATCH] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3`

[Mika Westerberg]

+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

RE: [PATCH] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3`

[Limonciello, Mario]

[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&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7Cde80f5d9ce
> 484b608b0c08da027c84d3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> C0%7C637825031070157114%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> p;sdata=mD6EJTcL5V0syIkscYe5kmmbyg3zDqEQBwP8zCAO5h4%3D&amp;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&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7Cde80f5d9ce48
> 4b608b0c08da027c84d3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0
> %7C637825031070157114%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;s
> data=S2JtOwrUHOb5VXNV3BepBS2VxlF0s9FW4DTMHC54ULU%3D&amp;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

Re: [PATCH] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3`

[Rafael J. Wysocki]

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