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

["Rafael J. Wysocki" <rafael@kernel.org>]

On Tue, Mar 15, 2022 at 5:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Mar 15, 2022 at 4:33 PM Mario Limonciello
> <mario.limonciello@amd.com> 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`.
> >
> > 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://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>
>
> No more comments from me:
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Or please let me know if I should pick it up.

> > ---
> > changes from v3->v4:
> >  * rework comment
> >  * only evaluate _S0W if necessary
> >  * drop static function with only one caller
> >
> >  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 a42dbf448860..e535dab2c888 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_FAILURE(acpi_evaluate_integer(adev->handle, "_S0W", NULL, &ret)))
> > +               return false;
> > +
> > +       return ret >= ACPI_STATE_D3_HOT;
> >  }
> >
> >  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> > --
> > 2.34.1
> >