The subject convention in drivers/pci would be "PCI/ACPI:"
But more importantly, please turn the subject from a non-specific
negative ("Don't blindly trust") into a more specific *positive*,
e.g.,
PCI/ACPI: Assume HotPlugSupportInD3 only if device can wake from D3
On Thu, Mar 10, 2022 at 11:58:32AM -0600, Mario Limonciello wrote:
> The `_DSD` `HotPlugSupportInD3` is supposed to indicate the ability for a
> bridge to be able to wakeup from D3.
Thanks for the Microsoft URL. To make this commit log self-contained
and guard against the link becoming stale, can you quote the relevant
text here, since it's not long:
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` will not return "0" indicating the inability to wake
> up the device from D3.
Please include the spec reference (ACPI v6.4, sec 7.3.20) in case the
URL below becomes stale, and again, the relevant text is barely longer
than the URL and could be included:
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.
I guess the argument is that we can put a Root Port into D3 only if
_S0W indicates that it can wake from D3 *and* it has the
HotPlugSupportInD3 property?
I'm naive about ACPI sleep/wake, but that does sound plausible.
But the patch doesn't say anything about _S0W, so we need to somehow
connect the dots there.
> To fix these situations explicitly check that the ACPI device has a GPE
> allowing the device to generate wakeup signals handled by the platform
> in `acpi_pci_bridge_d3`.
acpi_pci_bridge_d3()
Would be good to say what "these situations" are. I guess this fixes
a bug, so let's outline what that bug is, what the symptoms are, who
reported and tested it, etc.
> This changes aligns the handling of the situation the same as Windows 10
> and Windows 11 both do as well.
s/changes/change/
Sentence also needs a little polishing: "aligns ... both do as well"
doesn't quite flow.
Does this make things work like Windows 10/11 from a user's point of
view, or have you somehow confirmed that Windows actually checks _S0W
and HotPlugSupportInD3 in the same way?
> 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")
> Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v1->v2:
> * Add Mika's tag
> * Update commit message for Rafael's suggestions
> 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
>
