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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Oct 03, 2023 at 01:37:34PM -0500, Mario Limonciello wrote:
> On 10/3/2023 13:31, Bjorn Helgaas wrote:
> > On Tue, Oct 03, 2023 at 01:06:48PM -0500, Mario Limonciello wrote:
> > > 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.
> > > > ...
> > 
> > > > 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.
> > 
> > I agree, that name is super irritating.  I don't even know how to
> > figure out or verify that pci_d3cold_disable() also disables D3hot.
> > 
> > > >     - 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.
> > 
> > Even if users don't intentionally turn off wakeup, there are devices
> > like mass storage and NICs without wake-on-LAN that don't require
> > wakeup.
> > 
> > I assume that if there's no downstream device that needs wakeup, this
> > quirk means we will keep the Root Port in D0 even though we could
> > safely put it in D3hot or D3cold.
> 
> Yes that matches my expectation as well.
> 
> > That's one thing I liked about the v20 iteration -- instead of
> > pci_d3cold_disable(), we changed dev->pme_support, which should mean
> > that we only avoid D3hot/D3cold if we need PMEs while in those states,
> > so I assumed that we *could* use D3 when we don't need the wakeups.
> 
> If you think it's worth spinning again for this optimization I think a
> device_may_wakeup() check on the root port can achieve the same result as
> the v20 PME solution did, but without the walking of a tree in the quirk.

Why would we use device_may_wakeup() here?  That seems like too much
assumption about the suspend path, and we already have the Root Port
pci_dev, so rp->pme_support is available.  What about something like
this:

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..4b601b1c0830 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6188,3 +6188,60 @@ 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, remove D3hot and D3cold from the PME_Support advertised
+ * by the Root Port so we don't use those states if we're expecting wakeup
+ * interrupts.  Restore the advertised PME_Support when resuming.
+ */
+static void amd_rp_pme_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);
+	if (!rp->pm_cap)
+		return;
+
+	rp->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >>
+				    PCI_PM_CAP_PME_SHIFT);
+	dev_info_once(&rp->dev, "quirk: disabling D3cold for suspend\n");
+}
+
+static void amd_rp_pme_resume(struct pci_dev *dev)
+{
+	struct pci_dev *rp;
+	u16 pmc;
+
+	rp = pcie_find_root_port(dev);
+	if (!rp->pm_cap)
+		return;
+
+	pci_read_config_word(rp, rp->pm_cap + PCI_PM_PMC, &pmc);
+	rp->pme_support = FIELD_GET(PCI_PM_CAP_PME_MASK, pmc);
+}
+/* Rembrandt (yellow_carp) */
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x162e, amd_rp_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x162e, amd_rp_pme_resume);
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x162f, amd_rp_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x162f, amd_rp_pme_resume);
+/* Phoenix (pink_sardine) */
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x1668, amd_rp_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1668, amd_rp_pme_resume);
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x1669, amd_rp_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x1669, amd_rp_pme_resume);
+#endif /* CONFIG_SUSPEND */