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 11:59:00PM -0500, Mario Limonciello wrote:
> On 9/13/2023 16:16, Mario Limonciello wrote:
> > On 9/13/2023 16:05, Bjorn Helgaas wrote:
> > [cut]
> > > > > 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.
> > 
> > Technically someone could; but realistically no one will be using these
> > machines without amd-pmc.
> > 
> > The battery life over suspend would be abhorrent.
> > 
> > > 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.
> > 
> > Yeah this does sound needlessly 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().
> > 
> > I don't think clearing PME support entirely is going to help.  The
> > reason is that pci_target_state() will fall back to PCI_D3hot when
> > dev->pme_support is fully cleared.
> > 
> > I think that clearing *just the bits* for D3hot and D3cold in PME
> > support should work though.  I'll test this.
> 
> I did confirm this works properly.
> 
> > Assuming it works how about if we put the quirk to clear the
> > D3hot/D3cold PME support bit in
> > drivers/platform/x86/amd/pmc/pmc-quirks.c?
> > 
> > It's still a quirk file and it makes it very clear that this behavior is
> > caused by what amd-pmc does.
> 
> I've got it coded up like this and working, so please let me know if this
> approach is amenable and I'll drop an updated version.
> 
> If you would prefer it to be in pci/quirks.c I believe I can do either.

If the quirk is in a loadable module, as opposed to being built-in,
does it get applied to the relevant Root Ports when the module is
loaded?  I didn't look exhaustively, but I don't see a reference to
pci_fixup_device() in the module load path.

Bjorn