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

[Lukas Wunner <lukas@wunner.de>]

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.

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

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?

Thanks,

Lukas