Re: [PATCH v5 1/2] PCI / ACPI: Assume `HotPlugSupportInD3` only if device can wake from D3

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 3/31/2022 14:04, Bjorn Helgaas wrote:
> [+cc Rafael, Mika, linux-pm]
> 
> On Mon, Mar 28, 2022 at 03:55:18PM -0500, Mario Limonciello 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`.
> 
> acpi_pci_bridge_d3() currently depends only on HotPlugSupportInD3 for
> the Root Port.  This patch adds dependencies on _S0W (if it exists)
> and _PRW.  The _PRW connection is indirect; this patch tests
> device->wakeup.flags.valid, which is only set in this path:
> 
>    acpi_add_single_object
>      acpi_bus_get_wakeup_device_flags
>        if (!acpi_has_method(device->handle, "_PRW"))
> 	return;
>        acpi_bus_extract_wakeup_device_power_package
> 	acpi_evaluate_object("_PRW")
> 	if (package)
> 	  wakeup->gpe_device = ...
> 	  wakeup->gpe_number = ...
>        device->wakeup.flags.valid = acpi_wakeup_gpe_init(device);
> 
> So IIUC the proposed logic here is:
> 
>    - If Root Port has no _PRW, we can't put this device in D3 (this is
>      new).
> 
>    - If Root Port has _S0W that says wake is not supported in D3hot
>      (Linux sets OSC_SB_PR3_SUPPORT), we can't put this device in D3
>      (this is also new).
> 
>    - If Root Port has HotPlugSupportInD3, we can put this device in D3
>      (this is the existing behavior).

Correct.

> 
> Proposed text:
> 
>    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 _S0W tells us the device cannot
>    wake from D3hot.  With the previous code, these devices could be put
>    in D3hot and hotplugged devices would not be recognized.
> 
>    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.

Very well, I'll incorporate into the commit message and scrap some of 
the old stuff.

> 
> The _S0W part makes sense to me.  The _PRW part hasn't been explained
> yet.  We didn't depend on it before, but we think it's safe to depend
> on it now?

An earlier version of this patch actually was only checking this rather 
than _S0W alone.  It was suggested that both should be checked together 
by Rafael.

https://lore.kernel.org/linux-pci/CAJZ5v0grj=vE1wGJpMxh-Hy7=ommfFUh5hw++nmQdLVxVtCSWw@mail.gmail.com/

FWIW at least some earlier versions Rafael and Mika both agreed towards 
that direction (and presumably weren't worried about existing systems 
that this code was used for).

> 
> In the commit log and comments, can we be more explicit about whether
> "D3" means "D3hot" or "D3cold"?

The check for _S0W return is looking for "3", so it's really D3hot "or" 
D3cold.  In the problematic case on Yellow Carp, it was D3hot.  I'll add 
this detail.


> 
>> 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%7Cdc09192c789f4da990f108da1349553d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637843502933651021%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=RxmXRsx7df1c1x%2FDqHNxG6iRpy798Aok%2Fl0vhs32D18%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%7Cdc09192c789f4da990f108da1349553d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637843502933651021%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=DN%2FfHQTayYZHCDY1NC3cG%2ByrgImwMVMlMhGGcb2ozWk%3D&reserved=0
>> Fixes: 26ad34d510a87 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v4-v5:
>>   * Don't fail if _S0W is missing
>>   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 1f15ab7eabf8..91c165ea4346 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_SUCCESS(acpi_evaluate_integer(adev->handle, "_S0W", NULL, &ret)))
>> +		return ret >= ACPI_STATE_D3_HOT;
> 
> I think it would make more sense to move the generic easy tests
> earlier, before acpi_dev_get_property(), since there's no need to look
> up the property if we might fail later.  E.g., something like the
> patch below, so it's:
> 
>    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;
> 
>    if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
>                               ACPI_TYPE_INTEGER, &obj) &&
>        obj->integer.value == 1)
>      return true;
> 
>    return false;
> >> +
>> +	return true;
>>   }
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 1f15ab7eabf8..9959bfdc0746 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -974,9 +974,11 @@ bool acpi_pci_power_manageable(struct pci_dev *dev)
>   
>   bool acpi_pci_bridge_d3(struct pci_dev *dev)
>   {
> -	const union acpi_object *obj;
> -	struct acpi_device *adev;
>   	struct pci_dev *rpdev;
> +	struct acpi_device *adev;
> +	acpi_status status;
> +	unsigned long long state;
> +	const union acpi_object *obj;
>   
>   	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
>   		return false;
> @@ -985,25 +987,37 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>   	if (acpi_pci_power_manageable(dev))
>   		return true;
>   
> +	rpdev = pcie_find_root_port(dev);
> +	if (!rpdev)
> +		return false;
> +
> +	adev = ACPI_COMPANION(&rpdev->dev);
> +	if (!adev)
> +		return false;
> +
> +	/*
> +	 * If the bridge can't wake from D3hot, it can't signal hotplug
> +	 * events in D3hot.
> +	 */
> +	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 know the hierarchy behind it
>   	 * supports D3 just fine.
>   	 */
> -	rpdev = pcie_find_root_port(dev);
> -	if (!rpdev)
> -		return false;
> +	if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
> +				   ACPI_TYPE_INTEGER, &obj) &&
> +	    obj->integer.value == 1)
> +		return true;
>   
> -	adev = ACPI_COMPANION(&rpdev->dev);
> -	if (!adev)
> -		return false;
> -
> -	if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
> -				   ACPI_TYPE_INTEGER, &obj) < 0)
> -		return false;
> -
> -	return obj->integer.value == 1;
> +	return false;
>   }
>   
>   int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)

Sure, I'll move them around.