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 07:32:58AM -0500, Limonciello, Mario wrote:
> On 4/1/2022 07:29, Bjorn Helgaas wrote:
> > 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://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%7Cc8ff8ac0568049a166bd08da13db41f3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637844129686586061%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=bjd5NnFaGslXcSVvShZMRHzuedEx8VgGnEnCGlWZ%2F2Q%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%7Cc8ff8ac0568049a166bd08da13db41f3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637844129686586061%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=r11UKOwL6SvGv4VCxaDyH0k15dsJib%2BJvRCS2MRobUQ%3D&amp;reserved=0
> > > > 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!
> 
> Bjorn,
> 
> Please let me know if you want me to send up a v7 with the minor remark
> updates, or if you can just fix those up while committing.

Fixing those up right now.  I'll post the updates shortly so you can
make sure I didn't make it worse.

Bjorn