On 9/17/2023 16:56, Bjorn Helgaas wrote:
On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
Iain reports that USB devices can't be used to wake a Lenovo Z13
from suspend. This problem occurs because the PCIe root port has been put
into D3hot and AMD's platform can't handle USB devices waking the platform
from a hardware sleep state in this case. The platform is put into
a hardware sleep state by the actions of the amd-pmc driver.
Although the issue is initially reported on a single model it actually
affects all Yellow Carp (Rembrandt) and Pink Sardine (Phoenix) SoCs.
This problem only occurs on Linux specifically when attempting to
wake the platform from a hardware sleep state.
Comparing the behavior on Windows and Linux, Windows doesn't put
the root ports into D3 at this time.
Linux decides the target state to put the device into at suspend by
this policy:
1. If platform_pci_power_manageable():
Use platform_pci_choose_state()
2. If the device is armed for wakeup:
Select the deepest D-state that supports a PME.
3. Else:
Use D3hot.
Devices are considered power manageable by the platform when they have
one or more objects described in the table in section 7.3 of the ACPI 6.5
specification [1]. In this case the root ports are not power manageable.
If devices are not considered power manageable; specs are ambiguous as
to what should happen. In this situation Windows 11 puts PCIe ports
in D0 ostensibly due the policy from the "uPEP driver" which is a
complimentary driver the Linux "amd-pmc" driver.
Linux chooses to allow D3 for these root ports due to the policy
introduced by commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
suspend").
The Windows uPEP driver expresses the desired state that should be
selected for suspend but Linux doesn't, so introduce a quirk for the
problematic root ports.
The quirk removes PME support for D3hot and D3cold at suspend time if the
system will be using s2idle. When the port is configured for wakeup this
will prevent these states from being selected in pci_target_state().
After the system is resumes the PME support is re-read from the PM
capabilities register to allow opportunistic power savings at runtime by
letting the root port go into D3hot or D3cold.
There's a lot of text here, but I think the essential thing is:
These Root Ports advertise D3hot and D3cold in the PME_Support
register, but PMEs do not work in those states when the amd-pmc
driver has put the platform in a sleep state.
It's specific to the PMEs for root ports with USB4 controller connected,
but yes otherwise correct.
I've confirmed that XHCI controllers connected to other root ports work
fine.
Cc: stable@xxxxxxxxxxxxxxx
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>
---
drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..ebc0afbc814e 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6188,3 +6188,64 @@ 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);
+
+/*
+ * When AMD PCIe root ports with AMD USB4 controllers attached to them are put
+ * into D3hot or D3cold downstream USB devices may fail to wakeup the system.
+ * This manifests as a missing wakeup interrupt.
+ *
+ * Prevent the associated root port from using PME to wake from D3hot or
+ * D3cold power states during s2idle.
+ * This will effectively put the root port into D0 power state over s2idle.
+ */
+static bool child_has_amd_usb4(struct pci_dev *pdev)
+{
+ struct pci_dev *child = NULL;
+
+ while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
+ if (child->vendor != PCI_VENDOR_ID_AMD)
+ continue;
+ if (pcie_find_root_port(child) != pdev)
+ continue;
+ return true;
+ }
+
+ return false;
+}
+
+static void quirk_reenable_pme(struct pci_dev *dev)
+{
+ u16 pmc;
+
+ if (!dev->pm_cap)
+ return;
+
+ if (!child_has_amd_usb4(dev))
+ return;
+
+ pci_read_config_word(dev, dev->pm_cap + PCI_PM_PMC, &pmc);
+ pmc &= PCI_PM_CAP_PME_MASK;
+ dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
+}
+
+static void quirk_disable_pme_suspend(struct pci_dev *dev)
+{
+ int mask;
+
+ if (pm_suspend_via_firmware())
+ return;
There's always something more to confuse me. Why does
pm_suspend_via_firmware() matter? I can sort of see that Linux
platform power management, which uses the PMC, is not the same
as platform firmware being invoked at the end of a system-wide power
management transition to a sleep state.
I guess this must have something to do with acpi_suspend_begin() and
acpi_hibernation_begin() (the callers of
pm_set_suspend_via_firmware())?
The "why" has to do with the implementation details of how the platform
enters and exits hardware sleep and what happens.
It's much different than how ACPI S3 works.
Most OEM platforms don't support S3, so if it distracts from the issue
and quirk I'm fine to drop this check.
+ if (!child_has_amd_usb4(dev))
+ return;
+
+ mask = (PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> PCI_PM_CAP_PME_SHIFT;
+ if (!(dev->pme_support & mask))
+ return;
+ dev->pme_support &= ~mask;
+ dev_info_once(&dev->dev, "quirk: disabling PME from D3hot and D3cold at suspend\n");
+}
+
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14b9, quirk_disable_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14b9, quirk_reenable_pme);
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14eb, quirk_disable_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14eb, quirk_reenable_pme);
--
2.34.1
