Hi Rafael,
On 8/8/23 15:37, Rafael J. Wysocki wrote:
> Hi,
>
> On Tue, Aug 8, 2023 at 3:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> 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 ?
>
> I need to take a deeper look into this, which I guess is going to take
> some time.
>
> If you figure this out in the meantime, please let me know.
I don't see myself having time to dive deeper into this anytime soon,
so if you can take a look (as time permits) that would be great.
Regards,
Hans
