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 07:55:46PM -0500, Mario Limonciello wrote:
> On 9/13/2023 23:59, 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.
> 
> I've also got a variation with pci/quirks.c working too.
> 
> Here's the trade offs:
> 
> pci/quirks.c
> ------------
> * Two lines for every platform affected by this.  IE:
> DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14b9,
> quirk_disable_pme_suspend);
> DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14b9, quirk_reenable_pme);
> * D3hot/D3cold works at runtime (since PME works at runtime)
> * Only runs if s2idle is used
> * Runs whether amd-pmc is bound or not.
> 
> drivers/platform/x86/amd/pmc/pmc-quirks.c
> -----------------------------------------
> * 1 line for adding new affected platforms
> * Runs at probe; PME is disabled for D3hot/D3cold at runtime.
> * Only runs if s2idle is used
> * Only runs if amd-pmc is bound.
> 
> Having implemented both ways and given users will effectively always use
> amd-pmc, I have a preference towards pci/quirks.c which only patches
> dev->pme_support to drop D3hot/cold at suspend time and restores it at
> resume.
> 
> Please let me know which way you prefer.

I think the pci/quirks.c one sounds nicer.  I was envisioning a
one-time quirk where it'd be easy to log a note about this issue, but
I see why the suspend/resume quirk has advantages.  I don't really
like opaque magic behavior (like D3hot/D3cold not being used when
dmesg and lspci claim that PME in those states *should* work), but
maybe we can figure out some way to make that visible.

Bjorn