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 15:00, Lukas Wunner wrote:
> On Mon, Oct 02, 2023 at 01:09:06PM -0500, Mario Limonciello wrote:
>> Iain reports that USB devices can't be used to wake a Lenovo Z13 from
>> suspend.  This occurs because on some AMD platforms, even though the Root
>> Ports advertise PME_Support for D3hot and D3cold, they don't handle PME
>> messages and generate wakeup interrupts from those states when amd-pmc has
>> put the platform in a hardware sleep state.
>>
>> Iain reported this on an AMD Rembrandt platform, but it also affects
>> Phoenix SoCs.  On Iain's system, a USB4 router below the affected Root Port
>> generates the PME. To avoid this issue, disable D3 for the root port
>> associated with USB4 controllers at suspend time.
> [...]
>> +static void quirk_disable_rp_d3cold_suspend(struct pci_dev *dev)
>> +{
>> +	struct pci_dev *rp;
>> +
>> +	/*
>> +	 * PM_SUSPEND_ON means we're doing runtime suspend, which means
>> +	 * amd-pmc will not be involved so PMEs during D3 work as advertised.
>> +	 *
>> +	 * The PMEs *do* work if amd-pmc doesn't put the SoC in the hardware
>> +	 * sleep state, but we assume amd-pmc is always present.
>> +	 */
>> +	if (pm_suspend_target_state == PM_SUSPEND_ON)
>> +		return;
>> +
>> +	rp = pcie_find_root_port(dev);
>> +	pci_d3cold_disable(rp);
>> +	dev_info_once(&rp->dev, "quirk: disabling D3cold for suspend\n");
>> +}
> 
> I think you mentioned in an earlier version of the patch that the
> USB controller could in theory be built into a Thunderbolt-attached
> device and that you wouldn't want to apply the quirk in that case.

It's not necessary with this approach of detecting the PCI IDs used for 
USB4 controllers in Rembrandt and Phoenix.  Those would not be used in 
any hypothetical discrete device.

> 
> Yet this patch doesn't seem to check for that possibility.
> 
> I guess in the affected systems, the USB controller is directly
> below the Root Port.  

Yes

> The pcie_find_root_port() function you're
> using here will walk up the hierarchy until it finds the Root Port,
> i.e. it's specifically for the case where there are switches between
> the USB controller and Root Port (which I think you want to exclude).
> I would have expected that you just call pci_upstream_bridge(dev) once
> and check whether the returned device is a PCI_EXP_TYPE_ROOT_PORT.
> 

Is there an advantage to using pci_upstream_bridge() given it's just one 
step up with pcie_find_root_port()?

> I'm also wondering why you're not invoking pci_d3cold_disable() with
> the USB controller's device (instead of the Root Port).  Setting
> no_d3cold on the USB controller should force all upstream bridges
> into D0.
> 
> Perhaps the reason you're not doing this is because the xhci_hcd driver
> might have called pci_d3cold_disable() as part of a quirk and the
> unconditional pci_d3cold_enable() on resume might clobber that?

That's exactly what I was worried about - what if other callers end up 
using pci_d3cold_disable/pci_d3cold_enable for some reason. We're all 
fighting for the same policy bits.

This being said, I am tending to agree with Bjorn, it's better to just 
clear the PME bits.