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.
