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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 31, 2022 at 02:33:12PM -0500, Limonciello, Mario wrote:
> On 3/31/2022 14:04, Bjorn Helgaas wrote:
> > [+cc Rafael, Mika, linux-pm]
> > On Mon, Mar 28, 2022 at 03:55:18PM -0500, Mario Limonciello wrote:
> > ...

> >    On some platforms, e.g., AMD Yellow Carp, firmware may supply
> >    "HotPlugSupportInD3" even though _S0W tells us the device cannot
> >    wake from D3hot.  With the previous code, these devices could be put
> >    in D3hot and hotplugged devices would not be recognized.
> > 
> >    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.
> 
> Very well, I'll incorporate into the commit message and scrap some of the
> old stuff.
> 
> > The _S0W part makes sense to me.  The _PRW part hasn't been explained
> > yet.  We didn't depend on it before, but we think it's safe to depend
> > on it now?
> 
> An earlier version of this patch actually was only checking this rather than
> _S0W alone.  It was suggested that both should be checked together by
> Rafael.
> 
> https://lore.kernel.org/linux-pci/CAJZ5v0grj=vE1wGJpMxh-Hy7=ommfFUh5hw++nmQdLVxVtCSWw@mail.gmail.com/
> 
> FWIW at least some earlier versions Rafael and Mika both agreed towards that
> direction (and presumably weren't worried about existing systems that this
> code was used for).

OK.  I think it's worth mentioning _PRW even if we don't have a good
explanation for it.  For someone bringing up a platform like this,
that will be a better hint than "adev->wakeup.flags.valid" alone.

> > In the commit log and comments, can we be more explicit about whether
> > "D3" means "D3hot" or "D3cold"?
> 
> The check for _S0W return is looking for "3", so it's really D3hot "or"
> D3cold.  In the problematic case on Yellow Carp, it was D3hot.  I'll add
> this detail.

Linux advertises OSC_SB_PR3_SUPPORT, so I think we are checking for
D3hot.  Of course, if a device can't wake from D3hot, it can't wake
from D3cold either.

You didn't *add* any D3 comments and only quoted the Microsoft doc, so
maybe nothing to change in this patch.  It's confusing to me that the
existing code, like acpi_pci_bridge_d3(), pci_bridge_d3_possible(),
pci_bridge_d3_force, etc., are all ambiguous.

Bjorn