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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Apr 01, 2022 at 01:44:42PM +0200, Rafael J. Wysocki wrote:
> On Fri, Apr 1, 2022 at 5:40 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > 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 the platform does not supply power
> > resources through _PRW and _S0W tells us the device cannot wake from
> 
> More importantly, it doesn't supply a wakeup GPE number through _PRW,
> so from the ACPI perspective the bridge has no way of generating
> wakeup signals.

Perfect, thanks.  That's the missing piece about _PRW.

> > D3hot.  With the previous code, these devices could be put in D3hot
> > and hotplugged devices would not be recognized without manually
> > rescanning.
> >
> > 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.
> >
> > Windows 10 and Windows 11 both will prevent the bridge from going in D3
> > when the firmware is configured this way and this change 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>
> > --
> > v5->v6:
> >  * Re-order checks to only check for _DSD if necessary as suggested by Bjorn.
> >  * Adjust commit message wording
> >  * Drop rewording comment patch, just apply it while moving text to avoid
> >    ping-ponging the same lines in the commits.
> > v4-v5:
> >  * Don't fail if _S0W is missing
> > ---
> >  drivers/pci/pci-acpi.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 1f15ab7eabf8..5ab797e2709d 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -976,7 +976,9 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >  {
> >         const union acpi_object *obj;
> >         struct acpi_device *adev;
> > +       unsigned long long state;
> >         struct pci_dev *rpdev;
> > +       acpi_status status;
> >
> >         if (acpi_pci_disabled || !dev->is_hotplug_bridge)
> >                 return false;
> > @@ -985,12 +987,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >         if (acpi_pci_power_manageable(dev))
> >                 return true;
> >
> > -       /*
> > -        * 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;
> > @@ -999,6 +995,23 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >         if (!adev)
> >                 return false;
> >
> > +       /*
> > +        * If the bridge can't wake from D3hot, it can't signal hotplug
> > +        * events in D3hot.
> 
> I would say "If the bridge cannot signal wakeup signals at all, it
> cannot signal hotplug events from low-power states including D3hot and
> D3cold."
> 
> With the two above remarks addressed, please feel free to add
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Great, thanks!

> > +        */
> > +       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 make an assumption that the
> > +        * hierarchy behind it supports D3 as well.
> > +        */
> >         if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
> >                                    ACPI_TYPE_INTEGER, &obj) < 0)
> >                 return false;
> > --
> > 2.34.1
> >