RE: [PATCH v2] ACPI: resource: Remove "Zen" specific match and quirks
From: Limonciello, Mario
Date: Mon May 22 2023 - 12:44:19 EST
[AMD Official Use Only - General]
> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Sent: Monday, May 22, 2023 11:42 AM
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: rafael@xxxxxxxxxx; gch981213@xxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; regressions@xxxxxxxxxxxxx;
> ofenfisch@xxxxxxxxxxxxxx; wse@xxxxxxxxxxxxxxxxxxx;
> adam.niederer@xxxxxxxxx; adrian@xxxxxxxxx; jirislaby@xxxxxxxxxx;
> Pananchikkal, Renjith <Renjith.Pananchikkal@xxxxxxx>; Tsao, Anson
> <anson.tsao@xxxxxxx>; Gong, Richard <Richard.Gong@xxxxxxx>;
> evilsnoo@xxxxxxxxx; ruinairas1992@xxxxxxxxx
> Subject: Re: [PATCH v2] ACPI: resource: Remove "Zen" specific match and
> quirks
>
> On Thu, May 18, 2023 at 8:39 PM Mario Limonciello
> <mario.limonciello@xxxxxxx> wrote:
> >
> > commit 9946e39fe8d0 ("ACPI: resource: skip IRQ override on
> > AMD Zen platforms") attempted to overhaul the override logic so it
> > didn't apply on X86 AMD Zen systems. This was intentional so that
> > systems would prefer DSDT values instead of default MADT value for
> > IRQ 1 on Ryzen 6000 systems which use ActiveLow for IRQ1.
> >
> > This turned out to be a bad assumption because several vendors seem
> > to add Interrupt Source Override but don't fix the DSDT. A pile of
> > quirks was collecting that proved this wasn't sustaintable.
> >
> > Adjust the logic so that only IRQ1 is overridden in Ryzen 6000 case.
> >
> > This effectively reverts the following commits:
> > commit 17bb7046e7ce ("ACPI: resource: Do IRQ override on all TongFang
> > GMxRGxx")
> > commit f3cb9b740869 ("ACPI: resource: do IRQ override on Lenovo
> 14ALC7")
> > commit bfcdf58380b1 ("ACPI: resource: do IRQ override on LENOVO
> IdeaPad")
> > commit 7592b79ba4a9 ("ACPI: resource: do IRQ override on XMG Core 15")
>
> Should it also remove the quirk added by commit 71a485624c4c ("ACPI:
> resource: Add IRQ override quirk for LG UltraPC 17U70P")?
I wondered the same thing, but a quick search showed that
"LG UltraPC 17U70P" is
" LG Ultra PC 17” Lightweight & High Performance Laptop Intel® 11th Gen Core® i7, NVIDIA® GeForce® GTX™ 1650Ti Graphics, Windows 11 Home, 16GB RAM, 512GB SSD, Silver"
So I don't think so.
>
> > Cc: ofenfisch@xxxxxxxxxxxxxx
> > Cc: wse@xxxxxxxxxxxxxxxxxxx
> > Cc: adam.niederer@xxxxxxxxx
> > Cc: adrian@xxxxxxxxx
> > Cc: jirislaby@xxxxxxxxxx
> > Tested-by: Renjith.Pananchikkal@xxxxxxx
> > Tested-by: anson.tsao@xxxxxxx
> > Tested-by: Richard.Gong@xxxxxxx
> > Tested-by: Chuanhong Guo <gch981213@xxxxxxxxx>
> > Reported-by: evilsnoo@xxxxxxxxx
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217394
> > Reported-by: ruinairas1992@xxxxxxxxx
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217406
> > Fixes: 9946e39fe8d0 ("ACPI: resource: skip IRQ override on AMD Zen
> platforms")
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > v1->v2:
> > * Rebase on 71a485624c4c ("ACPI: resource: Add IRQ override quirk for LG
> UltraPC 17U70P")
> > * Pick up tag
> > ---
> > drivers/acpi/resource.c | 154 +++++++++++++++++-----------------------
> > 1 file changed, 65 insertions(+), 89 deletions(-)
> >
> > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> > index 0800a9d77558..c6ac87e01e1c 100644
> > --- a/drivers/acpi/resource.c
> > +++ b/drivers/acpi/resource.c
> > @@ -470,52 +470,6 @@ static const struct dmi_system_id asus_laptop[] = {
> > { }
> > };
> >
> > -static const struct dmi_system_id lenovo_laptop[] = {
> > - {
> > - .ident = "LENOVO IdeaPad Flex 5 14ALC7",
> > - .matches = {
> > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > - DMI_MATCH(DMI_PRODUCT_NAME, "82R9"),
> > - },
> > - },
> > - {
> > - .ident = "LENOVO IdeaPad Flex 5 16ALC7",
> > - .matches = {
> > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > - DMI_MATCH(DMI_PRODUCT_NAME, "82RA"),
> > - },
> > - },
> > - { }
> > -};
> > -
> > -static const struct dmi_system_id tongfang_gm_rg[] = {
> > - {
> > - .ident = "TongFang GMxRGxx/XMG CORE 15 (M22)/TUXEDO Stellaris
> 15 Gen4 AMD",
> > - .matches = {
> > - DMI_MATCH(DMI_BOARD_NAME, "GMxRGxx"),
> > - },
> > - },
> > - { }
> > -};
> > -
> > -static const struct dmi_system_id maingear_laptop[] = {
> > - {
> > - .ident = "MAINGEAR Vector Pro 2 15",
> > - .matches = {
> > - DMI_MATCH(DMI_SYS_VENDOR, "Micro Electronics Inc"),
> > - DMI_MATCH(DMI_PRODUCT_NAME, "MG-VCP2-15A3070T"),
> > - }
> > - },
> > - {
> > - .ident = "MAINGEAR Vector Pro 2 17",
> > - .matches = {
> > - DMI_MATCH(DMI_SYS_VENDOR, "Micro Electronics Inc"),
> > - DMI_MATCH(DMI_PRODUCT_NAME, "MG-VCP2-17A3070T"),
> > - },
> > - },
> > - { }
> > -};
> > -
> > static const struct dmi_system_id lg_laptop[] = {
> > {
> > .ident = "LG Electronics 17U70P",
> > @@ -527,7 +481,7 @@ static const struct dmi_system_id lg_laptop[] = {
> > { }
> > };
> >
> > -struct irq_override_cmp {
> > +struct irq_override_dmi_cmp {
> > const struct dmi_system_id *system;
> > unsigned char irq;
> > unsigned char triggering;
> > @@ -536,50 +490,86 @@ struct irq_override_cmp {
> > bool override;
> > };
> >
> > -static const struct irq_override_cmp override_table[] = {
> > +struct irq_override_acpi_cmp {
> > + const char *id;
> > + unsigned char irq;
> > + unsigned char triggering;
> > + unsigned char polarity;
> > +};
> > +
> > +static const struct irq_override_dmi_cmp dmi_override_table[] = {
> > { medion_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, false
> },
> > { asus_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, false },
> > - { lenovo_laptop, 6, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, true
> },
> > - { lenovo_laptop, 10, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, true
> },
> > - { tongfang_gm_rg, 1, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW, 1,
> true },
> > - { maingear_laptop, 1, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW, 1,
> true },
> > { lg_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, false },
> > };
> >
> > -static bool acpi_dev_irq_override(u32 gsi, u8 triggering, u8 polarity,
> > - u8 shareable)
> > +/*
> > + * Ryzen 6000 requires ActiveLow for keyboard, but a number of machines
> > + * seem to get it wrong in DSDT or don't have an Interrupt Source
> > + * Override.
> > + */
> > +static const struct irq_override_acpi_cmp acpi_override_table[] = {
> > + { "AMDI0007", 1, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW },
> > +};
> > +
> > +static void acpi_dev_irq_override(u32 gsi, u8 *triggering, u8 *polarity,
> > + u8 *shareable)
> > {
> > - int i;
> > + int i, p, t;
> > + int check_override = true;
> >
> > - for (i = 0; i < ARRAY_SIZE(override_table); i++) {
> > - const struct irq_override_cmp *entry = &override_table[i];
> > + for (i = 0; i < ARRAY_SIZE(dmi_override_table); i++) {
> > + const struct irq_override_dmi_cmp *entry =
> &dmi_override_table[i];
> >
> > if (dmi_check_system(entry->system) &&
> > entry->irq == gsi &&
> > - entry->triggering == triggering &&
> > - entry->polarity == polarity &&
> > - entry->shareable == shareable)
> > - return entry->override;
> > + entry->triggering == *triggering &&
> > + entry->polarity == *polarity &&
> > + entry->shareable == *shareable)
> > + check_override = entry->override;
> > }
> >
> > -#ifdef CONFIG_X86
> > - /*
> > - * IRQ override isn't needed on modern AMD Zen systems and
> > - * this override breaks active low IRQs on AMD Ryzen 6000 and
> > - * newer systems. Skip it.
> > - */
> > - if (boot_cpu_has(X86_FEATURE_ZEN))
> > - return false;
> > -#endif
> > + if (!check_override)
> > + return;
> >
> > - return true;
> > + if (!acpi_get_override_irq(gsi, &t, &p)) {
> > + u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> > + u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> > +
> > + if (*triggering != trig || *polarity != pol) {
> > + pr_warn("ACPI: IRQ %d override to %s%s, %s%s\n", gsi,
> > + t ? "level" : "edge",
> > + trig == *triggering ? "" : "(!)",
> > + p ? "low" : "high",
> > + pol == *polarity ? "" : "(!)");
> > + *triggering = trig;
> > + *polarity = pol;
> > + }
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(acpi_override_table); i++) {
> > + const struct irq_override_acpi_cmp *entry =
> &acpi_override_table[i];
> > +
> > + if (acpi_dev_found(entry->id) && gsi == entry->irq &&
> > + (*polarity != entry->polarity || *triggering != entry->triggering)) {
> > + pr_warn("ACPI: IRQ %d override to %s%s, %s%s due to %s\n",
> > + gsi,
> > + entry->triggering ? "level" : "edge",
> > + entry->triggering == *triggering ? "" : "(!)",
> > + entry->polarity ? "low" : "high",
> > + entry->polarity == *polarity ? "" : "(!)",
> > + entry->id);
> > + *polarity = entry->polarity;
> > + *triggering = entry->triggering;
> > + }
> > + }
> > }
> >
> > static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> > u8 triggering, u8 polarity, u8 shareable,
> > u8 wake_capable, bool check_override)
> > {
> > - int irq, p, t;
> > + int irq;
> >
> > if (!valid_IRQ(gsi)) {
> > irqresource_disabled(res, gsi);
> > @@ -592,26 +582,12 @@ static void acpi_dev_get_irqresource(struct
> resource *res, u32 gsi,
> > * 2. BIOS uses IO-APIC mode Interrupt Source Override
> > *
> > * We do this only if we are dealing with IRQ() or IRQNoFlags()
> > - * resource (the legacy ISA resources). With modern ACPI 5 devices
> > + * resource (the legacy ISA resources). With ACPI devices
> > * using extended IRQ descriptors we take the IRQ configuration
> > * from _CRS directly.
> > */
> > - if (check_override &&
> > - acpi_dev_irq_override(gsi, triggering, polarity, shareable) &&
> > - !acpi_get_override_irq(gsi, &t, &p)) {
> > - u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> > - u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> > -
> > - if (triggering != trig || polarity != pol) {
> > - pr_warn("ACPI: IRQ %d override to %s%s, %s%s\n", gsi,
> > - t ? "level" : "edge",
> > - trig == triggering ? "" : "(!)",
> > - p ? "low" : "high",
> > - pol == polarity ? "" : "(!)");
> > - triggering = trig;
> > - polarity = pol;
> > - }
> > - }
> > + if (check_override)
> > + acpi_dev_irq_override(gsi, &triggering, &polarity, &shareable);
> >
> > res->flags = acpi_dev_irq_flags(triggering, polarity, shareable,
> wake_capable);
> > irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> >
> > base-commit: c554eee18c9a440bd2dd5a363b0f79325717f0bf
> > --
> > 2.34.1
> >