| Message ID | 20231002180906.82089-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v21] PCI: Avoid D3 at suspend for AMD PCIe root ports w/ USB4 controllers | expand |
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@vger.kernel.org # 6.1.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space > Cc: stable@vger.kernel.org # 6.5.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space > Cc: stable@vger.kernel.org # 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@orangesquash.org.uk> > 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@amd.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
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@vger.kernel.org # 6.1.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space > Cc: stable@vger.kernel.org # 6.5.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space > Cc: stable@vger.kernel.org # 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@orangesquash.org.uk> > 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@amd.com> > --- > 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? - 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? I like the fact that we don't have to walk the hierarchy with pci_walk_bus(). > + 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 >
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@vger.kernel.org # 6.1.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space >> Cc: stable@vger.kernel.org # 6.5.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space >> Cc: stable@vger.kernel.org # 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@orangesquash.org.uk> >> 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@amd.com> >> --- >> 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 >>
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. 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. Bjorn
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. > > Bjorn 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.
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 */
On 10/3/2023 14:16, Bjorn Helgaas wrote: > 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, Because that's what pci_target_state() passes as well to determine if a wakeup is needed. > and we already have the Root Port > pci_dev, so rp->pme_support is available. What about something like > this: > It includes the round trip to config space which Lukas called out as negative previously but it should work. > 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 */
On Tue, Oct 03, 2023 at 02:24:26PM -0500, Mario Limonciello wrote: > On 10/3/2023 14:16, Bjorn Helgaas wrote: > > On Tue, Oct 03, 2023 at 01:37:34PM -0500, Mario Limonciello wrote: > > > On 10/3/2023 13:31, Bjorn Helgaas wrote: > ... > > > > 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, > > Because that's what pci_target_state() passes as well to determine if a > wakeup is needed. That's exactly what I mean about having too many assumptions here about other parts of the kernel. I like pme_support because it's the most specific piece of information about the issue and we don't have to know anything about how pci_target_state() works to understand it. > > and we already have the Root Port > > pci_dev, so rp->pme_support is available. What about something like > > this: > > It includes the round trip to config space which Lukas called out as > negative previously but it should work. True. But I can't get too excited about one config read in the resume path. > > + 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); Is it actually necessary to look up the Root Port here? Would it be enough if we removed D3 from the xHCI devices (0x162e, 0x162f, 0x1668, 0x1669), e.g., just do this: dev->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> PCI_PM_CAP_PME_SHIFT); I assume that if we knew the xHCI couldn't generate wakeups from D3, we would leave the xHCI in D0, and that would mean we'd also leave the Root Port in D0? Or is the desired behavior that we put the xHCI in D3hot/cold and only leave the the Root Port in D0? Bjorn
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
On 10/3/2023 14:59, Bjorn Helgaas wrote: > On Tue, Oct 03, 2023 at 02:24:26PM -0500, Mario Limonciello wrote: >> On 10/3/2023 14:16, Bjorn Helgaas wrote: >>> On Tue, Oct 03, 2023 at 01:37:34PM -0500, Mario Limonciello wrote: >>>> On 10/3/2023 13:31, Bjorn Helgaas wrote: >> ... > >>>>> 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, >> >> Because that's what pci_target_state() passes as well to determine if a >> wakeup is needed. > > That's exactly what I mean about having too many assumptions here > about other parts of the kernel. I like pme_support because it's the > most specific piece of information about the issue and we don't have > to know anything about how pci_target_state() works to understand it. > Got it. >>> and we already have the Root Port >>> pci_dev, so rp->pme_support is available. What about something like >>> this: >> >> It includes the round trip to config space which Lukas called out as >> negative previously but it should work. > > True. But I can't get too excited about one config read in the resume > path. > >>> + 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); > > Is it actually necessary to look up the Root Port here? Would it be > enough if we removed D3 from the xHCI devices (0x162e, 0x162f, 0x1668, > 0x1669), e.g., just do this: > > dev->pme_support &= ~((PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> > PCI_PM_CAP_PME_SHIFT); > > I assume that if we knew the xHCI couldn't generate wakeups from D3, > we would leave the xHCI in D0, and that would mean we'd also leave the > Root Port in D0? > > Or is the desired behavior that we put the xHCI in D3hot/cold and only > leave the the Root Port in D0? > > Bjorn The intended behavior is that the PCI end points go into D3, but the root port stays in D0. That matches how Windows behaves.
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.
On Tue, Oct 03, 2023 at 03:16:47PM -0500, Mario Limonciello wrote: > On 10/3/2023 15:00, Lukas Wunner wrote: > > 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()? Not really, no. The information I was missing is that these Device IDs are unique to the SoC and will never appear in a Thunderbolt-attached device. > 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. Fair enough, I'm sorry I led you down the wrong path with that suggestion. I guess no_d3cold is generally only useful for the "D3cold is known to be broken *permanently*" use case. Incidentally, there's only a single driver in the tree calling pci_d3cold_enable() and that's i915. And it likewise disables and re-enables the flag at the Root Port, just like you did. Thanks, Lukas
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); + 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 */
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@vger.kernel.org # 6.1.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space Cc: stable@vger.kernel.org # 6.5.y: 70b70a4: PCI/sysfs: Protect driver's D3cold preference from user space Cc: stable@vger.kernel.org # 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@orangesquash.org.uk> 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@amd.com> --- 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(+)