Hi,
On 8/7/23 06:38, Mario Limonciello wrote:
> On 8/6/23 13:20, Hans de Goede wrote:
>> Hi Mario,
>>
>> On 8/6/23 19:13, Mario Limonciello wrote:
>>> On 8/6/23 10:14, Hans de Goede wrote:
>>>> Commit a9c4a912b7dc ("ACPI: resource: Remove "Zen" specific match and
>>>> quirks") is causing keyboard problems for quite a log of AMD based
>>>> laptop users, leading to many bug reports.
>>>>
>>>> Revert this change for now, until we can come up with
>>>> a better fix for the PS/2 IRQ trigger-type/polarity problems
>>>> on some x86 laptops.
>>>
>>> Reverting it is going to help a bunch of machines but cause regressions for others. How do you prioritize which to fix when it comes to a regression?
>>
>> I understand where you are coming from. But we are dealing with a lot of bug reports from users of recent kernel versions (so not on LTS distros) where things used to work fine. Which is pretty much the definition of a regression.
>>
>> OTOH the do not override IRQ on Zen behavior has been with us for a while now, so things which are broken by it have been broken for a while and have only started working since 6.4.7.
>>
>> So to me it is clear that we first need to revert to the old state, so that users for which everything was working fine get back to a working system.
>>
>> I realize this will unfix some very recently fixed systems, but notice the very recently bit here most users are no in 6.4.7 or .8 yet, so most users will not even have gotten the fixing effect OTOH many users are seeing breakage now.
>>
>> So IMHO we should first get back to the known bad state, instead of introducing a new unknown bad state like a9c4a912b7dc did.
>>
>> If there are known systems which will need an override with a9c4a912b7dc reverted, then we should probably add those to the override table right away.
>>
>> And I think what we also need is a way to specify an IRQ trigger-type override for IRQ 1 on the kernel commandline, so that we can easily ask users to try a different trigger-type without them needing to build a kernel with an updated quirk table.
>>
>> As Linus mentioned already, we really need to find a proper fix for this, but for now my main priority is to stop the influx of new Fedora bugzilla and bugzilla.kernel.org bugs caused by a9c4a912b7dc, counting the extra bug links added in this thread it seems we are up to 6 known bug reports for this and that likely is just the tip of the iceberg.
>>
>> Regards,
>>
>> Hans
>>
>>
>
> We haven't even given a try to fixing it; I think the revert is still hasty.
>
> I don't have a machine that can reproduce this failure, but I did confirm that my keyboard still works with this:
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 1dd8d5aebf678..b74d7d8cc8630 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -19,7 +19,7 @@
> #include <linux/dmi.h>
>
> #ifdef CONFIG_X86
> -#define valid_IRQ(i) (((i) != 0) && ((i) != 2))
> +#define valid_IRQ(i) ((i) > 2)
> static inline bool acpi_iospace_resource_valid(struct resource *res)
> {
> /* On X86 IO space is limited to the [0 - 64K] IO port range */
>
> Can we perhaps see if that works instead for some affected people?
That does not just skip the override stuff, it will make the kernel return irqresource_disabled(res, 1) for the kbd IRQ:
static inline void irqresource_disabled(struct resource *res, u32 irq)
{
res->start = irq;
res->end = irq;
res->flags |= IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
}
Now not much code seems to actually check the IORESOURCE_DISABLED | IORESOURCE_UNSET flags, so this might still work but it does not seem like the right thing to do.
Regards,
Hans
>
> If it does we should be able to throw away the entire quirks table.
>
>>
>>>
>>>>
>>>> Fixes: a9c4a912b7dc ("ACPI: resource: Remove "Zen" specific match and quirks")
>>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2229165
>>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2229317
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217726
>>>> Cc: Mario Limonciello <mario.limonciello@xxxxxxx>
>>>> Cc: Linux regressions mailing list <regressions@xxxxxxxxxxxxxxx>
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>>> ---
>>>> drivers/acpi/resource.c | 60 +++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>>>> index 1dd8d5aebf67..0800a9d77558 100644
>>>> --- a/drivers/acpi/resource.c
>>>> +++ b/drivers/acpi/resource.c
>>>> @@ -470,6 +470,52 @@ 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",
>>>> @@ -493,6 +539,10 @@ struct irq_override_cmp {
>>>> static const struct irq_override_cmp 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 },
>>>> };
>>>> @@ -512,6 +562,16 @@ static bool acpi_dev_irq_override(u32 gsi, u8 triggering, u8 polarity,
>>>> return 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
>>>> +
>>>> return true;
>>>> }
>>>>
>>>
>>
>
