Re: [PATCH v6 1/1] PCI: Avoid putting some root ports into D3 on some Ryzen chips

[Limonciello, Mario]

On 7/10/2023 14:32, Bjorn Helgaas wrote:
> On Sat, Jul 08, 2023 at 04:44:57PM -0500, Mario Limonciello wrote:
> > commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > sets the policy that all PCIe ports are allowed to use D3.  When
> > the system is suspended if the port is not power manageable by the
> > platform and won't be used for wakeup via a PME this sets up the
> > policy for these ports to go into D3hot.
> >
> > This policy generally makes sense from an OSPM perspective but it leads
> > to problems with wakeup from suspend on laptops with AMD chips:
> >
> > - On family 19h model 44h (PCI 0x14b9) this manifests as a missing wakeup
> >    interrupt.
> > - On family 19h model 74h (PCI 0x14eb) this manifests as a system hang.
> >
> > Add a quirk for the PCI device ID used by the problematic root port on
> > both chips to ensure that these root ports are not put into D3hot at
> > suspend.
>
> What is problematic about these root ports?  Is this a hardware
> erratum?  

I mentioned some of this in earlier versions; but the problem is deeper
in the platform firmware and only happens when the root ports are in 
D3hot across an s2idle cycle.

When looked at independently they can be moved in and out of D3hot no 
problem.

From the perspective of hardware designers they say this is a software 
bug that Linux puts PCI RP into D3hot over Modern Standby when they 
don't specify for this to be done.  I don't expect any erratum for it.

> Some corner of the ACPI spec that allows undefined behavior?

These root ports are not reported as power manageable by ACPI.

As I mentioned in the cover letter how the OSPM handles this case is 
outside of any spec AFAICT.

> Does AMD have any guidance about generic ways to use D3, or does AMD
> expect to add quirks piecemeal as problems are discovered?  How does
> Windows handle all this?

Windows doesn't put root ports into D3hot over suspend unless they are 
power managed by ACPI (as described in section 7.3 of the ACPI spec).

https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html

So on Windows these ports are all in D0 and none of the issues happen.

Linux if PCI devices aren't power managed by ACPI will either follow 
deepest state it can wake from PME or fall back to D3hot.

> Adding quirks as we discover random devices that don't behave
> correctly for reasons unknown is not very sustainable.

I don't disagree but in v5 I tried to align this to the Windows behavior 
and it wasn't accepted.

If you still think v5 is the better approach I'm fine to try to do that 
again and rewrite the commit message.

> Bjorn
>
> > Cc: stable@xxxxxxxxxxxxxxx # 6.1+
> > 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
> > Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> >   drivers/pci/quirks.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 321156ca273d5..e0346073e5855 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3867,6 +3867,22 @@ static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev)
> >   DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
> >   			       PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
> >   			       quirk_apple_poweroff_thunderbolt);
> > +
> > +/*
> > + * Putting PCIe root ports on Ryzen SoCs with USB4 controllers into D3hot
> > + * may cause problems when the system attempts wake up from s2idle.
> > + *
> > + * On family 19h model 44h (PCI 0x14b9) this manifests as a missing wakeup
> > + * interrupt.
> > + * On family 19h model 74h (PCI 0x14eb) this manifests as a system hang.
> > + */
> > +static void quirk_ryzen_rp_d3(struct pci_dev *pdev)
> > +{
> > +	if (!acpi_pci_power_manageable(pdev))
> > +		pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x14b9, quirk_ryzen_rp_d3);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x14eb, quirk_ryzen_rp_d3);
> >   #endif
> >   
> >   /*
> > --
> > 2.34.1