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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Sep 13, 2023 at 07:42:05PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 13, 2023 at 6:35 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > On 9/13/2023 10:40, Bjorn Helgaas wrote:
> > > On Wed, Sep 13, 2023 at 12:20:14PM +0200, Rafael J. Wysocki wrote:
> > >> On Wed, Sep 13, 2023 at 6:11 AM Mario Limonciello
> > >> <mario.limonciello@amd.com> wrote:
> 
> [cut]
> 
> > >
> > > Also, do we have some indication that this is specific to Ryzen?  If
> > > not, I assume this is an ongoing issue, and matching on Device IDs
> > > just means we'll have to debug the same problem again and add more
> > > IDs.
> >
> > This is why my earlier attempts (v16 and v17) tried to tie it to
> > constraints.  These are what the uPEP driver in Windows uses to make the
> > decision of what power state to put integrated devices like the root
> > port into.
> >
> > In Windows if no uPEP driver is installed "Windows internal policy"
> > dictates what happens.  If the uPEP driver is installed then it
> > influences the policy based upon the constraints.
> >
> > Rafael had feedback against constraints in v17, which is why I'm back to
> > a quirk for v18.
> >
> > This issue as I've described it is specific to AMD Ryzen.
> 
> OK, so a quirk is the way to go IMO, because starting to rely on LPI
> constraints in general retroactively is almost guaranteed to regress
> things this way or another.
> 
> Whatever is done, it needs to be Ryzen-specific, unless there is
> evidence that other (and in particular non-AMD) platforms are
> affected.
> 
> > I expect it to be an ongoing issue.  I also expect unless we use
> > constraints or convince the firmware team to add a _S0W object with a
> > value of "0" for the sake of Linux that we will be adding IDs every year
> > to wherever this lands as we reproduce it on newer SoCs.
> 
> So maybe the way to go is to make the AMD PMC driver set a flag for
> Root Ports on suspend or similar.

I like the quirk approach.  When PMC is involved, the device behavior
doesn't conform to what it advertised via PME_Support.

The v18 quirk isn't connected to PMC at all, so IIUC it avoids
D3hot/D3cold unnecessarily when amd/pmc is not loaded.

I don't object to avoiding D3hot/D3cold unconditionally.  Presumably
we *could* save a little power by using them when amd/pci isn't
loaded, but amd/pci would have to iterate through all PCI devices when
it loads, save previous state, do the quirk, and then restore the
previous state on module unload.  And it would have to use notifiers
or assume no Root Port hotplug.  All sounds kind of complicated.

Maybe it would even be enough to just clear dev->pme_support so we
know wakeups don't work.  It would be a pretty big benefit if we
didn't have to add another bit and complicate pci_prepare_to_sleep()
or pci_target_state().

Bjorn