| Message ID | 20220310175832.1259-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3` | expand |
On Thu, Mar 10, 2022 at 9:13 PM Limonciello, Mario <Mario.Limonciello@amd.com> wrote: > > [Public] > > > > 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`. > > > > Which may be orthogonal to the _S0W return value mentioned above. > > > > Also, I'm not quite sure why acpi_pci_bridge_d3() should require the > > root port to have a wake GPE associated with it as an indication that > > the hierarchy below it can be put into D3cold. > > The reason that brought me down the path in this patch was actually > acpi_dev_pm_get_state. _S0W isn't actually evaluated unless > adev->wakeup.flags.valid is set. That's true, but it is unclear how this is related to whether or not a given PCIe port can handle D3cold. But see below. > > > > > > > > > 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; > > > > Minor nit: the two checks above could be combined. > > OK if we stick to this approach I'll do that. > > > > > Also I would add a comment explaining why exactly wakeup.flags.valid > > is checked here, because I can imagine a case in which the wakeup > > signaling capability is irrelevant for whether or not the given port > > can handle D3cold. > > Specifically a case that it's a hotplug bridge that has HotPlugSupportInD3 > though? In practice I've only seen that in use on USB4 and Thunderbolt > bridges "so far". > > I haven't tried yet but I would think directly evaluating _S0W at this time > seems it should also work and would match closer to my original intent > of the patch. Would you prefer that? I guess, but I'm not sure, that you are trying to kind of validate HotPlugSupportInD3 by checking if the root port in question actually can signal wakeup via ACPI and if it cannot, assume that the flag was set by mistake and so the bridge should not be assumed to be able to handle D3cold. That is not unreasonable, but in that case you need to check wakeup.flags.valid first and then _S0W too, because it can return 0 even if the "valid" flag is set. And explain in a comment why this is done. > > > > > + > > > if (acpi_dev_get_property(adev, "HotPlugSupportInD3", > > > ACPI_TYPE_INTEGER, &obj) < 0) > > > return false; > > > --
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@linux.intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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
>
Hi, Bjorn, On 3/11/2022 12:23, Bjorn Helgaas wrote: > 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. Yes, thanks I will clarify the commit message to your suggestions in v3. > > But the patch doesn't say anything about _S0W, so we need to somehow > connect the dots there. Yes, per Rafael's suggestions I will be adding a comment about this situation inline with the code for v3. > >> 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. OK > > 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? We checked with BIOS debug logs and Windows does evaluate _S0W during startup and specifically modifying _S0W return value alone will cause Windows to not let the port into 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%7C94e638619b754591688b08da038c323e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637826197941955260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=sKq3X70W1mmKprMKD6oueDQVET2OMNosWmSddjS1Cho%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%7C94e638619b754591688b08da038c323e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637826197941955260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=oKvNC3MOVNehzpR0O7Jg2bTfJeYHu5GX2v%2FQ%2B0dvKlg%3D&reserved=0 >> Fixes: 26ad34d510a87 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports") >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> 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 >>
On 3/11/2022 10:05, Rafael J. Wysocki wrote: > On Thu, Mar 10, 2022 at 9:13 PM Limonciello, Mario > <Mario.Limonciello@amd.com> wrote: >> >> [Public] >> >>>> 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`. >>> >>> Which may be orthogonal to the _S0W return value mentioned above. >>> >>> Also, I'm not quite sure why acpi_pci_bridge_d3() should require the >>> root port to have a wake GPE associated with it as an indication that >>> the hierarchy below it can be put into D3cold. >> >> The reason that brought me down the path in this patch was actually >> acpi_dev_pm_get_state. _S0W isn't actually evaluated unless >> adev->wakeup.flags.valid is set. > > That's true, but it is unclear how this is related to whether or not a > given PCIe port can handle D3cold. But see below. > >> >>> >>>> >>>> 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; >>> >>> Minor nit: the two checks above could be combined. >> >> OK if we stick to this approach I'll do that. >> >>> >>> Also I would add a comment explaining why exactly wakeup.flags.valid >>> is checked here, because I can imagine a case in which the wakeup >>> signaling capability is irrelevant for whether or not the given port >>> can handle D3cold. >> >> Specifically a case that it's a hotplug bridge that has HotPlugSupportInD3 >> though? In practice I've only seen that in use on USB4 and Thunderbolt >> bridges "so far". >> >> I haven't tried yet but I would think directly evaluating _S0W at this time >> seems it should also work and would match closer to my original intent >> of the patch. Would you prefer that? > > I guess, but I'm not sure, that you are trying to kind of validate > HotPlugSupportInD3 by checking if the root port in question actually > can signal wakeup via ACPI and if it cannot, assume that the flag was > set by mistake and so the bridge should not be assumed to be able to > handle D3cold. > > That is not unreasonable, but in that case you need to check > wakeup.flags.valid first and then _S0W too, because it can return 0 > even if the "valid" flag is set. And explain in a comment why this is > done. OK I'll make these changes and check the situation again. > >>> >>>> + >>>> if (acpi_dev_get_property(adev, "HotPlugSupportInD3", >>>> ACPI_TYPE_INTEGER, &obj) < 0) >>>> return false; >>>> --
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 device from D3. 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`. This changes aligns the handling of the situation the same as Windows 10 and Windows 11 both do as well. 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@linux.intel.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v1->v2: * Add Mika's tag * Update commit message for Rafael's suggestions drivers/pci/pci-acpi.c | 3 +++ 1 file changed, 3 insertions(+)