On 10/3/2023 12:24, Bjorn Helgaas 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.
Restore D3 support at resume so that it can be used by runtime suspend.
The amd-pmc driver doesn't put the platform in a hardware sleep state for
runtime suspend, so PMEs work as advertised.
Cc: stable@xxxxxxxxxxxxxxx # 6.1.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space
Cc: stable@xxxxxxxxxxxxxxx # 6.5.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space
Cc: stable@xxxxxxxxxxxxxxx # 6.6.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
Reported-by: Iain Lane <iain@xxxxxxxxxxxxxxxxxxx>
Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
---
v20-v21:
* Rewrite commit message, lifting most of what Bjorn clipped down to on v20.
* Use pci_d3cold_disable()/pci_d3cold_enable() instead
* Do the quirk on the USB4 controller instead of RP->USB->RP
---
drivers/pci/quirks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..5674065011e7 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6188,3 +6188,47 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
+
+#ifdef CONFIG_SUSPEND
+/*
+ * Root Ports on some AMD SoCs advertise PME_Support for D3hot and D3cold, but
+ * if the SoC is put into a hardware sleep state by the amd-pmc driver, the
+ * Root Ports don't generate wakeup interrupts for USB devices.
+ *
+ * When suspending, disable D3 support for the Root Port so we don't use it.
+ * Restore D3 support when resuming.
+ */
+static void quirk_enable_rp_d3cold(struct pci_dev *dev)
+{
+ pci_d3cold_enable(pcie_find_root_port(dev));
+}
+
+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);
I think this prevents D3cold from being used at all, right?
Two questions:
- PME also doesn't work in D3hot, right?
Right.
IMO pci_d3cold_*() is poorly named.
It's going to prevent D3 on the bridge.
Maybe we can discuss renaming it to be clearer what it is doing after
we're done with this quirk and my other proposed change to drop
d3cold_allowed.
I'd prefer to keep renaming the symbol separate from this quirk because
it's going to require changes to other drivers too and make the quirk
harder to land and backport to stable trees.
- Is it OK to use D3hot and D3cold if we don't have a wakeup device
below the Root Port? I assume that scenario is possible?
Yes; it's "fine to do that" if there is no wakeup device below the root
port.
If a user intentionally turns off power/wakeup for the child devices
(which as said before was USB4 and XHCI PCIe devices) then wakeup won't
be set.
So in this case as the quirk is implemented I expect the root port will
be left in D0 even if a user intentionally turns off power/wakeup for
the USB4 and XHCI devices.
I like the fact that we don't have to walk the hierarchy with
pci_walk_bus().
Yeah, it really is a lot cleaner this way even if it is "8 quirks"
instead of "4".
+ dev_info_once(&rp->dev, "quirk: disabling D3cold for suspend\n");
+}
+/* Rembrandt (yellow_carp) */
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x162e, quirk_disable_rp_d3cold_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x162e, quirk_enable_rp_d3cold);
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x162f, quirk_disable_rp_d3cold_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x162f, quirk_enable_rp_d3cold);
+/* Phoenix (pink_sardine) */
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x1668, quirk_disable_rp_d3cold_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1668, quirk_enable_rp_d3cold);
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x1669, quirk_disable_rp_d3cold_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1669, quirk_enable_rp_d3cold);
+#endif /* CONFIG_SUSPEND */
--
2.34.1
