Re: [PATCH v18 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Sep 14, 2023 at 04:53:32PM +0200, Lukas Wunner wrote:
> On Thu, Sep 14, 2023 at 09:31:38AM -0500, Mario Limonciello wrote:
> > On 9/14/2023 09:17, Lukas Wunner wrote:
> > > On Wed, Sep 13, 2023 at 11:36:49AM -0500, Mario Limonciello wrote:
> > > > On 9/13/2023 09:31, Lukas Wunner wrote:
> > > > > If this only affects system sleep, not runtime PM, what you can do is
> > > > > define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable()
> > > > > and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls
> > > > > pci_d3cold_enable().
> > > > > 
> > > > > And I think you can make those calls conditional on pm_suspend_no_platform()
> > > > > to constrain to s2idle.
> > > > > 
> > > > > User space should still be able to influence runtime PM via the
> > > > > d3cold_allowed flag (unless I'm missing something).
> > > > 
> > > > The part you're missing is that D3hot is affected by this issue too,
> > > > otherwise it would be a good proposal.
> > > 
> > > I recall that in an earlier version of the patch, you solved the issue
> > > by amending pci_bridge_d3_possible().
> > > 
> > > Changing the dev->no_d3cold flag indirectly influences the bridge_d3
> > > flag (through pci_dev_check_d3cold() and pci_bridge_d3_update()).
> > > 
> > > If dev->no_d3cold is set on a device below a port, that port is
> > > prevented from entring D3hot because it would result in the
> > > device effectively being in D3cold.
> > > 
> > > So you might want to take a closer look at this approach despite
> > > the flag suggesting that it only influences D3cold.
> > 
> > Ah; I hadn't considered setting it on a device below the port. In this
> > particular situation the only devices below the root port are USB
> > controllers.
> > 
> > If those devices don't go into D3 the system can't enter hardware sleep.
> 
> If you set dev->no_d3cold on the USB controllers, they should still
> be able to go to D3hot, but not D3cold, which perhaps might be sufficient.
> It should prevent D3cold *and* D3hot on the Root Port above.  And if you
> set that on system sleep in a quirk and clear it on resume, runtime PM
> shouldn't be affected.

dev->no_d3cold appears to be mainly an administrative policy knob
twidded via sysfs.

There *are* a few cases where drivers (i915, nouveau, xhci) update it
via pci_d3cold_enable() or pci_d3cold_disable(), but they all look
vulnerable to issues if people use the sysfs knob, and I'm a little
dubious that they're legit in the first place.

This AMD Root Port issue is not an administrative choice; it's purely
a functional problem of the device advertising that it supports PME#
but not actually being able to do it.  So if we can do this by fixing
dev->pme_support (i.e., the copy of what it advertised), I'd rather do
that.

Bjorn