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.
