Hi,
On 8/8/23 13:18, Rafael J. Wysocki wrote:
> On Tue, Aug 8, 2023 at 10:36 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 8/7/23 17:19, Hans de Goede wrote:
>>> Hi All,
>>>
>>> On 8/6/23 17: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.
>>>>
>>>> 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>
>>>
>>> I've spend most of today analysing the situation / this problem :
>>>
>>> 213031 - MEDION notebook internal keyboard not recognized / not working correctly
>>> https://bugzilla.kernel.org/show_bug.cgi?id=213031
>>>
>>> This is the bug that started it all, the issue here was overriding
>>> a level low DSDT entry:
>>>
>>> IRQ (Level, ActiveLow, Exclusive, )
>>> {1}
>>>
>>> With an edge high entry from the MADT, note that edge high is the default
>>> mp_irqs[idx].irqflags value for legacy/ISA IRQs. The dmesg for the Ice Lake
>>> Medion M15T this bug is about shows no INT_SRC_OVR entry for IRQ 1
>>> in the MADT, it does show INT_SRC_OVR entries for IRQ 0 and 9.
>>>
>>> At first a fix was attempted to not use the MADT override unless
>>> the DSDT entry was edge high. But that caused regressions, so a switch
>>> to a DMI based approach was used instead. Noteworthy is that some of
>>> the regressions benefitted from a MADT override to high edge for
>>> IRQ 3 and 4 (UART IRQs) even though there are no INT_SRC_OVR messages
>>> in the dmesg of the machine with the regression.
>>>
>>> *** fast forward to today ***
>>>
>>> The DMI quirk based approach seems to have worked well for the Ice Lake
>>> era problems from approx. 3 years ago. But on AMD Zen based systems
>>> the situation seems to be more complex. Not using the MADT override
>>> is a problem for quite a few models. But using the MADT override
>>> is a problem on quite a few other models ...
>>>
>>> Looking at the status quo for v6.4 where MADT overriding by default
>>> is not used, 3 bugs have been filed where the override is actually
>>> necessary (note dmesg snippets with patched kernel to enable
>>> MADT override):
>>>
>>> 217394 - IRQ override skipping breaks the Aya Neo Air Plus 6800U keyboard buttons
>>> https://bugzilla.kernel.org/show_bug.cgi?id=217394
>>>
>>> Aya Neo Air Plus - AMD Ryzen 7 6800U
>>>
>>> [ 0.003333] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
>>> [ 0.003333] ACPI: INT_SRC_OVR (bus 0 bus_irq 1 global_irq 1 low edge)
>>> [ 0.003333] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 low level)
>>> [ 0.410670] ACPI: IRQ 1 override to edge, low(!)
>>>
>>> 217406 - very slow keyboard typing without IRQ override with new AMD Ryzen CPUs
>>> https://bugzilla.kernel.org/show_bug.cgi?id=217406
>>>
>>> HP Pavilion Aero 13 - AMD Ryzen 7735U
>>>
>>> [ 0.026135] ACPI: INT_SRC_OVR (bus 0 bus_irq 1 global_irq 1 low edge)
>>> [ 0.026136] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
>>> [ 0.026137] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 low level)
>>>
>>> [ 0.361640] ACPI: IRQ 1 override to edge, low(!)
>>>
>>> 217336 - keyboard not working Asus TUF FA617NS
>>> https://bugzilla.kernel.org/show_bug.cgi?id=217336
>>>
>>> Asus TUF FA617NS - AMD Ryzen 7 7735HS
>>>
>>> Noteworthy DSTD keyboard resource:
>>>
>>> IRQNoFlags ()
>>> {1}
>>>
>>> ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
>>> ACPI: INT_SRC_OVR (bus 0 bus_irq 1 global_irq 1 low edge)
>>> ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 low level)
>>> ACPI: IRQ 1 override to edge, low(!)
>>>
>>> So for all 3 do use MADT override on Zen bugs we have an INT_SRC_OVR dmesg entry
>>> for IRQ 1.
>>>
>>> Unfortunately the "MAINGEAR Vector Pro 2 17" / "MG-VCP2-17A3070T" for
>>> which a quirk was added in commit 9946e39fe8d0 to force the override
>>> even though it it Zen based breaks this pattern:
>>>
>>> [ 0.073733] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
>>> [ 0.073734] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 low level)
>>> [ 0.341347] ACPI: IRQ 1 override to edge, high(!)
>>>
>>> Still the presence of an INT_SRC_OVR for a specific legacy IRQ seems
>>> to be a strong indicator that MADT overriding should be used in that
>>> case and can be used to at least reduce the amount of DMI quirks.
>>>
>>> Another interesting data point is that all the devices for which
>>> DMI quirks are present for which MADT overriding should not be used
>>> for IRQ 1 all have a DSDT entry with the IRQ configured as level low
>>> and exclusive.
>>>
>>> I think that the best thing to do might be to go back to
>>> the original approach from:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?id=0ec4e55e9f571f08970ed115ec0addc691eda613
>>>
>>> and then limit this to IRQ1. Also maybe inverting the check to:
>>>
>>> static bool irq_is_legacy(struct acpi_resource_irq *irq)
>>> {
>>> return !(irq->triggering == ACPI_LEVEL_SENSITIVE &&
>>> irq->polarity == ACPI_ACTIVE_LOW &&
>>> irq->shareable == ACPI_EXCLUSIVE);
>>> }
>>>
>>> But I need to check if this will work for all the new Zen models
>>> for which we got bug reports after the recent dropping of
>>> 9946e39fe8d0 ("ACPI: resource: skip IRQ override on AMD Zen platforms")
>>
>> So today I have started with continueing the investigation looking
>> at laptop models where we used to not override because of:
>>
>> if (boot_cpu_has(X86_FEATURE_ZEN))
>> return false;
>>
>> And where removing this and thus using the override has led to a
>> regression.
>>
>> Looking at the acpidump-s from the following bugs:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=2229165
>> https://bugzilla.redhat.com/show_bug.cgi?id=2229317
>> https://bugzilla.kernel.org/show_bug.cgi?id=217726
>>
>> All of these use the following settings for the kbd in the DSDT:
>>
>> IRQ (Edge, ActiveLow, Shared, )
>> {1}
>>
>> So we know these are at least 3 models with "Edge, ActiveLow, Shared" IRQ 1 settings which must not use the override. But the existing quirks before a9c4a912b7dc ("ACPI: resource: Remove "Zen" specific match and quirks") contain:
>>
>> { tongfang_gm_rg, 1, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW, 1, true },
>> { maingear_laptop, 1, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW, 1, true },
>>
>> IOW models with "Edge, ActiveLow, Shared" IRQ 1 settings which OTOH must use the override, so as I was already afraid there is no easy for these DSDT IRQ 1 settings skip the override solution :|
>>
>> So I have 2 plans to move forward with this:
>>
>> Plan 1. Short them for 6.5 and backporting to 6.4.y (and other stable series):
>>
>> 1. Revert a9c4a912b7dc
>> 2. Limit the acpi_dev_irq_override() check to only ever skip the IRQ override
>> (return false) for GSI 1.
>> 3. Add a check if there is a INT_SRC_OVR MADT entry for GSI 1 and in that case
>> use the override even on ZEN, fixing:
>>
>> 217394 - IRQ override skipping breaks the Aya Neo Air Plus 6800U keyboard buttons
>> https://bugzilla.kernel.org/show_bug.cgi?id=217394
>> 217406 - very slow keyboard typing without IRQ override with new AMD Ryzen CPUs
>> https://bugzilla.kernel.org/show_bug.cgi?id=217406
>> 217336 - keyboard not working Asus TUF FA617NS
>> https://bugzilla.kernel.org/show_bug.cgi?id=217336
>>
>> Which are known AMD ZEN based laptops which do need the override for IRQ 1.
>>
>> This short term plan is not ideal, but it does fix all currently known issues / models and does so in a way which will hopefully not cause regressions on any other models.
>>
>>
>> Plan 2. Long term, see if I can come up with a way to read back the actual trigger type set in the IOAPIC for IRQ 1 at boot (in drivers/acpi/resource.c) and use that.
>
> Sounds reasonable to me.
>
> It looks like using the IRQ 1 configuration left by the BIOS as is
> would be the best choice unless that is not viable for some reason.
Agreed, do you have a suggestion how to do that ? I've been looking at this but I've gotten a bit lost in all the layers of ioapic code.
It looks like we need to add a flag to acpi_register_gsi() for it to register a gsi while keeping the IOAPIC settings as is (or a new function) but it is not clear to me how to implement this.
An alternative method would be to call irq_get_trigger_type() for IRQ 1 and use that for the IRQ trigger info when calling acpi_register_gsi(), but I think we need to have the IRQ registered / added to the IRQ domain first ?
Regards,
Hans
