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/14/2023 07:32, Bjorn Helgaas wrote:
> 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

Right; when done in a module it would be done with code that is part of 
probe.

So it has the implication that it would prevent D3hot/D3cold for this 
root port at runtime as well.

If you think it should be tied to pci_fixup_device() calls then it needs 
to be built-in.