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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Mar 23, 2022 at 11:01:05AM -0500, Mario Limonciello wrote:
> On 3/15/22 11:36, Rafael J. Wysocki wrote:
> > 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://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%7Cae5d5d8f4ac1452dac0c08da06a200b2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829590116901616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=3cTSyBgSEOBo5mPeUcNDrc9qhdMJeZ4cVtLqroDRZqY%3D&amp;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&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7Cae5d5d8f4ac1452dac0c08da06a200b2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829590116901616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=rZHk%2FgII0qOvr9cmmB8N1auNc1nLSFViVTG9f%2FwEREY%3D&amp;reserved=0
> > > > 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.
> 
> Bjorn,
> 
> Friendly reminder on this one.

Thanks; we're in the middle of the merge window now, so I'll wait till
that's over unless this is an emergency.

IIUC the bug this fixes is that "when a bridge is in D3cold, we don't
notice when a device is hot-added below it," right?  So we need to
avoid putting the bridge in D3cold?

Is there a typical scenario where this bites users?  I don't think we
ever saw an actual problem report?

> > > > ---
> > > > 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
> > > > 
>