Re: [PATCH v21] PCI: Avoid D3 at suspend for AMD PCIe root ports w/ USB4 controllers

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

On 10/3/2023 14:59, Bjorn Helgaas wrote:
> On Tue, Oct 03, 2023 at 02:24:26PM -0500, Mario Limonciello wrote:
>> On 10/3/2023 14:16, Bjorn Helgaas wrote:
>>> On Tue, Oct 03, 2023 at 01:37:34PM -0500, Mario Limonciello wrote:
>>>> On 10/3/2023 13:31, Bjorn Helgaas wrote:
>> ...
> 
>>>>> That's one thing I liked about the v20 iteration -- instead of
>>>>> pci_d3cold_disable(), we changed dev->pme_support, which should mean
>>>>> that we only avoid D3hot/D3cold if we need PMEs while in those states,
>>>>> so I assumed that we *could* use D3 when we don't need the wakeups.
>>>>
>>>> If you think it's worth spinning again for this optimization I think a
>>>> device_may_wakeup() check on the root port can achieve the same result as
>>>> the v20 PME solution did, but without the walking of a tree in the quirk.
>>>
>>> Why would we use device_may_wakeup() here?  That seems like too much
>>> assumption about the suspend path,
>>
>> Because that's what pci_target_state() passes as well to determine if a
>> wakeup is needed.
> 
> That's exactly what I mean about having too many assumptions here
> about other parts of the kernel.  I like pme_support because it's the
> most specific piece of information about the issue and we don't have
> to know anything about how pci_target_state() works to understand it.
> 

Got it.

>>> and we already have the Root Port
>>> pci_dev, so rp->pme_support is available.  What about something like
>>> this:
>>
>> It includes the round trip to config space which Lukas called out as
>> negative previously but it should work.
> 
> True.  But I can't get too excited about one config read in the resume
> path.
> 
>>> +	rp = pcie_find_root_port(dev);
>>> +	if (!rp->pm_cap)
>>> +		return;
>>> +
>>> +	rp->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >>
>>> +				    PCI_PM_CAP_PME_SHIFT);
> 
> Is it actually necessary to look up the Root Port here?  Would it be
> enough if we removed D3 from the xHCI devices (0x162e, 0x162f, 0x1668,
> 0x1669), e.g., just do this:
> 
>    dev->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >>
>                                PCI_PM_CAP_PME_SHIFT);
> 
> I assume that if we knew the xHCI couldn't generate wakeups from D3,
> we would leave the xHCI in D0, and that would mean we'd also leave the
> Root Port in D0?
> 
> Or is the desired behavior that we put the xHCI in D3hot/cold and only
> leave the the Root Port in D0?
> 
> Bjorn

The intended behavior is that the PCI end points go into D3, but the 
root port stays in D0.  That matches how Windows behaves.