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 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.