Re: [PATCH v2] PCI: ACPI: Don't blindly trust `HotPlugSupportInD3`

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

On Thu, Mar 10, 2022 at 9:13 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> > > To fix these situations explicitly check that the ACPI device has a GPE
> > > allowing the device to generate wakeup signals handled by the platform
> > > in `acpi_pci_bridge_d3`.
> >
> > Which may be orthogonal to the _S0W return value mentioned above.
> >
> > Also, I'm not quite sure why acpi_pci_bridge_d3() should require the
> > root port to have a wake GPE associated with it as an indication that
> > the hierarchy below it can be put into D3cold.
>
> The reason that brought me down the path in this patch was actually
> acpi_dev_pm_get_state.  _S0W isn't actually evaluated unless
> adev->wakeup.flags.valid is set.

That's true, but it is unclear how this is related to whether or not a
given PCIe port can handle D3cold.  But see below.

>
> >
> > >
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index a42dbf448860..9f8f55ed09d9 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -999,6 +999,9 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> > >         if (!adev)
> > >                 return false;
> > >
> > > +       if (!adev->wakeup.flags.valid)
> > > +               return false;
> >
> > Minor nit: the two checks above could be combined.
>
> OK if we stick to this approach I'll do that.
>
> >
> > Also I would add a comment explaining why exactly wakeup.flags.valid
> > is checked here, because I can imagine a case in which the wakeup
> > signaling capability is irrelevant for whether or not the given port
> > can handle D3cold.
>
> Specifically a case that it's a hotplug bridge that has HotPlugSupportInD3
> though?  In practice I've only seen that in use on USB4 and Thunderbolt
> bridges "so far".
>
> I haven't tried yet but I would think directly evaluating _S0W at this time
> seems it should also work and would match closer to my original intent
> of the patch.  Would you prefer that?

I guess, but I'm not sure, that you are trying to kind of validate
HotPlugSupportInD3 by checking if the root port in question actually
can signal wakeup via ACPI and if it cannot, assume that the flag was
set by mistake and so the bridge should not be assumed to be able to
handle D3cold.

That is not unreasonable, but in that case you need to check
wakeup.flags.valid first and then _S0W too, because it can return 0
even if the "valid" flag is set.  And explain in a comment why this is
done.

> >
> > > +
> > >         if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
> > >                                    ACPI_TYPE_INTEGER, &obj) < 0)
> > >                 return false;
> > > --