| Message ID | 20210608045632.8832-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ACPI : don't always override the acpi irq | expand |
On Tue, Jun 8, 2021 at 6:56 AM Hui Wang <hui.wang@canonical.com> wrote: > > The laptop keyboard doesn't work on many MEDION notebooks, but the > keyboard works well under Windows and Unix. > > Through debugging, we found this log in the dmesg: > ACPI: IRQ 1 override to edge, high > pnp 00:03: Plug and Play ACPI device, IDs PNP0303 (active) > > And we checked the IRQ definition in the DSDT, it is: > IRQ (Level, ActiveLow, Exclusive, ) > {1} > > So the BIOS defines the keyboard irq to Level_Low, but the linux > kernel override it to Edge_High. If let the linux kernel skip the irq > override, the keyboard will work normally. > > From the existing comment in the acpi_dev_get_irqresource(), the > override function only needs to be called when BIOS defines IRQ() or > IRQNoFlags, and according to page 419 and 420 of the > ACPI_6_3_final_Jan30.pdf, if IRQ() is empty or defines IRQNoFlags, Say "Section ... of ACPI 6.3" instead of referring directly to a PDF file. And if you refer to ACPI 6.4 instead, you may use a Link tag to point to the relevant section in the HTML format of the spec. > the IRQ is High true, edge sensitive and non-shareable. The linux > ACPI driver (acpi_rs_set_irq[] in rsirq.c) also assumes so. > > So here add a function to check 4 conditions, if all of them are true, > call override function. otherwise, it means IRQ descriptior in the > BIOS is not legacy or is not empty. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031 > BugLink: http://bugs.launchpad.net/bugs/1909814 > Reported-and-tested-by: Manuel Krause <manuelkrause@netscape.net> > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/acpi/resource.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index ee78a210c606..d346aa24ffd6 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -380,6 +380,16 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity) > } > EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type); > > +static bool acpi_dev_irq_empty_or_noflags(bool legacy, u8 triggering, u8 polarity, > + u8 shareable) > +{ > + if (legacy && (triggering == ACPI_EDGE_SENSITIVE) && > + (polarity == ACPI_ACTIVE_HIGH) && (shareable == ACPI_EXCLUSIVE)) > + return true; > + else > + return false; Because the function returns bool, you can do return legacy && triggering == ACPI_EDGE_SENSITIVE && polarity == ACPI_ACTIVE_HIGH && shareable == ACPI_EXCLUSIVE; Also I'm not sure why a new function is needed for this at all, as the check can be done in-line below just fine. Moreover, as it stands, the only purpose of the "legacy" argument of acpi_dev_get_irqresource() is whether or not to do the override, so the triggering/polarity/shareable check can be used to determine the value of "legacy" when calling that function from acpi_dev_resource_interrupt() in the ACPI_RESOURCE_TYPE_IRQ case. > +} > + > static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, > u8 triggering, u8 polarity, u8 shareable, > bool legacy) > @@ -401,7 +411,8 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, > * using extended IRQ descriptors we take the IRQ configuration > * from _CRS directly. > */ > - if (legacy && !acpi_get_override_irq(gsi, &t, &p)) { > + if (acpi_dev_irq_empty_or_noflags(legacy, 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; > > --
On 6/8/21 7:50 PM, Rafael J. Wysocki wrote: > On Tue, Jun 8, 2021 at 6:56 AM Hui Wang <hui.wang@canonical.com> wrote: >> The laptop keyboard doesn't work on many MEDION notebooks, but the >> keyboard works well under Windows and Unix. >> >> Through debugging, we found this log in the dmesg: >> ACPI: IRQ 1 override to edge, high >> pnp 00:03: Plug and Play ACPI device, IDs PNP0303 (active) >> >> And we checked the IRQ definition in the DSDT, it is: >> IRQ (Level, ActiveLow, Exclusive, ) >> {1} >> >> So the BIOS defines the keyboard irq to Level_Low, but the linux >> kernel override it to Edge_High. If let the linux kernel skip the irq >> override, the keyboard will work normally. >> >> From the existing comment in the acpi_dev_get_irqresource(), the >> override function only needs to be called when BIOS defines IRQ() or >> IRQNoFlags, and according to page 419 and 420 of the >> ACPI_6_3_final_Jan30.pdf, if IRQ() is empty or defines IRQNoFlags, > Say "Section ... of ACPI 6.3" instead of referring directly to a PDF file. > > And if you refer to ACPI 6.4 instead, you may use a Link tag to point > to the relevant section in the HTML format of the spec. OK, go it. >> the IRQ is High true, edge sensitive and non-shareable. The linux >> ACPI driver (acpi_rs_set_irq[] in rsirq.c) also assumes so. >> >> So here add a function to check 4 conditions, if all of them are true, >> call override function. otherwise, it means IRQ descriptior in the >> BIOS is not legacy or is not empty. >> >> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031 >> BugLink: http://bugs.launchpad.net/bugs/1909814 >> Reported-and-tested-by: Manuel Krause <manuelkrause@netscape.net> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> drivers/acpi/resource.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c >> index ee78a210c606..d346aa24ffd6 100644 >> --- a/drivers/acpi/resource.c >> +++ b/drivers/acpi/resource.c >> @@ -380,6 +380,16 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity) >> } >> EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type); >> >> +static bool acpi_dev_irq_empty_or_noflags(bool legacy, u8 triggering, u8 polarity, >> + u8 shareable) >> +{ >> + if (legacy && (triggering == ACPI_EDGE_SENSITIVE) && >> + (polarity == ACPI_ACTIVE_HIGH) && (shareable == ACPI_EXCLUSIVE)) >> + return true; >> + else >> + return false; > Because the function returns bool, you can do > > return legacy && triggering == ACPI_EDGE_SENSITIVE && polarity == > ACPI_ACTIVE_HIGH && shareable == ACPI_EXCLUSIVE; > > Also I'm not sure why a new function is needed for this at all, as the > check can be done in-line below just fine. > > Moreover, as it stands, the only purpose of the "legacy" argument of > acpi_dev_get_irqresource() is whether or not to do the override, so > the triggering/polarity/shareable check can be used to determine the > value of "legacy" when calling that function from > acpi_dev_resource_interrupt() in the ACPI_RESOURCE_TYPE_IRQ case. Got it, will do a change like that. Thanks. > >> +} >> + >> static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, >> u8 triggering, u8 polarity, u8 shareable, >> bool legacy) >> @@ -401,7 +411,8 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, >> * using extended IRQ descriptors we take the IRQ configuration >> * from _CRS directly. >> */ >> - if (legacy && !acpi_get_override_irq(gsi, &t, &p)) { >> + if (acpi_dev_irq_empty_or_noflags(legacy, 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; >> >> --
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index ee78a210c606..d346aa24ffd6 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -380,6 +380,16 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity) } EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type); +static bool acpi_dev_irq_empty_or_noflags(bool legacy, u8 triggering, u8 polarity, + u8 shareable) +{ + if (legacy && (triggering == ACPI_EDGE_SENSITIVE) && + (polarity == ACPI_ACTIVE_HIGH) && (shareable == ACPI_EXCLUSIVE)) + return true; + else + return false; +} + static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, u8 triggering, u8 polarity, u8 shareable, bool legacy) @@ -401,7 +411,8 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, * using extended IRQ descriptors we take the IRQ configuration * from _CRS directly. */ - if (legacy && !acpi_get_override_irq(gsi, &t, &p)) { + if (acpi_dev_irq_empty_or_noflags(legacy, 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;
The laptop keyboard doesn't work on many MEDION notebooks, but the keyboard works well under Windows and Unix. Through debugging, we found this log in the dmesg: ACPI: IRQ 1 override to edge, high pnp 00:03: Plug and Play ACPI device, IDs PNP0303 (active) And we checked the IRQ definition in the DSDT, it is: IRQ (Level, ActiveLow, Exclusive, ) {1} So the BIOS defines the keyboard irq to Level_Low, but the linux kernel override it to Edge_High. If let the linux kernel skip the irq override, the keyboard will work normally. From the existing comment in the acpi_dev_get_irqresource(), the override function only needs to be called when BIOS defines IRQ() or IRQNoFlags, and according to page 419 and 420 of the ACPI_6_3_final_Jan30.pdf, if IRQ() is empty or defines IRQNoFlags, the IRQ is High true, edge sensitive and non-shareable. The linux ACPI driver (acpi_rs_set_irq[] in rsirq.c) also assumes so. So here add a function to check 4 conditions, if all of them are true, call override function. otherwise, it means IRQ descriptior in the BIOS is not legacy or is not empty. BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031 BugLink: http://bugs.launchpad.net/bugs/1909814 Reported-and-tested-by: Manuel Krause <manuelkrause@netscape.net> Signed-off-by: Hui Wang <hui.wang@canonical.com> --- drivers/acpi/resource.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)