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

[Mario Limonciello <mario.limonciello@amd.com>]

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.