Re: [PATCH v21] PCI: Avoid D3 at suspend for AMD PCIe root ports w/ USB4 controllers

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Oct 03, 2023 at 02:24:26PM -0500, Mario Limonciello wrote:
> On 10/3/2023 14:16, Bjorn Helgaas wrote:
> > On Tue, Oct 03, 2023 at 01:37:34PM -0500, Mario Limonciello wrote:
> > > On 10/3/2023 13:31, Bjorn Helgaas wrote:
> ...

> > > > That's one thing I liked about the v20 iteration -- instead of
> > > > pci_d3cold_disable(), we changed dev->pme_support, which should mean
> > > > that we only avoid D3hot/D3cold if we need PMEs while in those states,
> > > > so I assumed that we *could* use D3 when we don't need the wakeups.
> > > 
> > > If you think it's worth spinning again for this optimization I think a
> > > device_may_wakeup() check on the root port can achieve the same result as
> > > the v20 PME solution did, but without the walking of a tree in the quirk.
> > 
> > Why would we use device_may_wakeup() here?  That seems like too much
> > assumption about the suspend path,
> 
> Because that's what pci_target_state() passes as well to determine if a
> wakeup is needed.

That's exactly what I mean about having too many assumptions here
about other parts of the kernel.  I like pme_support because it's the
most specific piece of information about the issue and we don't have
to know anything about how pci_target_state() works to understand it.

> > and we already have the Root Port
> > pci_dev, so rp->pme_support is available.  What about something like
> > this:
> 
> It includes the round trip to config space which Lukas called out as
> negative previously but it should work.

True.  But I can't get too excited about one config read in the resume
path.

> > +	rp = pcie_find_root_port(dev);
> > +	if (!rp->pm_cap)
> > +		return;
> > +
> > +	rp->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >>
> > +				    PCI_PM_CAP_PME_SHIFT);

Is it actually necessary to look up the Root Port here?  Would it be
enough if we removed D3 from the xHCI devices (0x162e, 0x162f, 0x1668,
0x1669), e.g., just do this:

  dev->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >>
                              PCI_PM_CAP_PME_SHIFT);

I assume that if we knew the xHCI couldn't generate wakeups from D3,
we would leave the xHCI in D0, and that would mean we'd also leave the
Root Port in D0?

Or is the desired behavior that we put the xHCI in D3hot/cold and only
leave the the Root Port in D0?

Bjorn