Re: [PATCH v4] PCI: Avoid putting some root ports into D3 on TUXEDO Sirius Gen1

[Werner Sembach]

Am 10.01.25 um 18:15 schrieb Werner Sembach:
> Am 08.01.25 um 22:26 schrieb Mario Limonciello:
> > On 12/20/2024 05:35, Werner Sembach wrote:
> > > From: Mario Limonciello <mario.limonciello@xxxxxxx>
> > >
> > > 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 the TUXEDO Sirius 16 Gen 1 with a
> > > specific old BIOS. This manifests as a system hang.
> > >
> > > On the affected Device + BIOS combination, add a quirk for the root port of
> > > the problematic controller to ensure that these root ports are not put into
> > > D3hot at suspend.
> > >
> > > This patch is based on
> > > https://lore.kernel.org/linux-pci/20230708214457.1229-2-mario.limonciello@xxxxxxx/ 
> > >
> > > but with the added condition both in the documentation and in the code to
> > > apply only to the TUXEDO Sirius 16 Gen 1 with a specific old BIOS and only
> > > the affected root ports.
> > >
> > > Co-developed-by: Georg Gottleuber <ggo@xxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Georg Gottleuber <ggo@xxxxxxxxxxxxxxxxxxx>
> > > Co-developed-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx # 6.1+
> > > Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >
> > So I don't think this should have my S-o-b.  At most it should Suggested-by: 
> > or Co-developed-by: since it was based on my original patch.
> kk
> > > ---
> > >   drivers/pci/quirks.c | 30 ++++++++++++++++++++++++++++++
> >
> > I think a better location for this is arch/x86/pci/fixup.c, similar to how we 
> > have https://git.kernel.org/torvalds/c/7d08f21f8c630
> >
> > thoughts?
>
> Fine with me
>
> I will make a v5
In fixup.c i don't have access to acpi_pci_power_manageable, but since 
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x1502, quirk_ryzen_rp_d3); matches 
to only one device anyways can i just skip it?
> > >   1 file changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index 76f4df75b08a1..d2f45c3e24c0a 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> >
> > > @@ -3908,6 +3908,36 @@ 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 the TUXEDO Sirius 16 Gen 1 with a specific old BIOS this manifests as
> > > + * a system hang.
> > > + */
> > > +static const struct dmi_system_id quirk_ryzen_rp_d3_dmi_table[] = {
> > > +    {
> > > +        .matches = {
> > > +            DMI_EXACT_MATCH(DMI_SYS_VENDOR, "TUXEDO"),
> > > +            DMI_EXACT_MATCH(DMI_BOARD_NAME, "APX958"),
> > > +            DMI_EXACT_MATCH(DMI_BIOS_VERSION, "V1.00A00_20240108"),
> > > +        },
> > > +    },
> > > +    {}
> > > +};
> > > +
> > > +static void quirk_ryzen_rp_d3(struct pci_dev *pdev)
> > > +{
> > > +    struct pci_dev *root_pdev;
> > > +
> > > +    if (dmi_check_system(quirk_ryzen_rp_d3_dmi_table)) {
> > > +        root_pdev = pcie_find_root_port(pdev);
> > > +        if (root_pdev && !acpi_pci_power_manageable(root_pdev))
> > > +            root_pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
> > > +    }
> > > +}
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x1502, quirk_ryzen_rp_d3);
> > >   #endif
> > >     /*