| Message ID | 20210609021442.12446-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v3] ACPI : don't always override the acpi irq | expand |
On Wed, Jun 9, 2021 at 4:14 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 the Section 6.4.2.1 of ACPI 6.4 spec [1], > 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. > > Here check 3 conditions to decide if the legacy is true or not, if it > is true, it means the IRQ() is empty or defines IRQNoFlags and need to > call irq_override(). > > Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#irq-descriptor # [1] > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031 > BugLink: http://bugs.launchpad.net/bugs/1909814 > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reported-and-tested-by: Manuel Krause <manuelkrause@netscape.net> > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/acpi/resource.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index ee78a210c606..dc01fb550b28 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -423,6 +423,13 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, > } > } > > +static bool irq_is_legacy(struct acpi_resource_irq *irq) > +{ > + return irq->triggering == ACPI_EDGE_SENSITIVE && > + irq->polarity == ACPI_ACTIVE_HIGH && > + irq->shareable == ACPI_EXCLUSIVE; > +} > + > /** > * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information. > * @ares: Input ACPI resource object. > @@ -461,7 +468,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > } > acpi_dev_get_irqresource(res, irq->interrupts[index], > irq->triggering, irq->polarity, > - irq->shareable, true); > + irq->shareable, irq_is_legacy(irq)); > break; > case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > ext_irq = &ares->data.extended_irq; > -- Applied as 5.14 material under a new subject ("ACPI: resources: Add checks for ACPI IRQ override") and with some edits in the patch changelog. Note, however, that this is a change in behavior that may result in regressions on old machines, in which case some further refinements of the code will be necessary. Thanks!
On 09/06/2021 14:44, Rafael J. Wysocki wrote: > On Wed, Jun 9, 2021 at 4:14 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 the Section 6.4.2.1 of ACPI 6.4 spec [1], >> 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. >> >> Here check 3 conditions to decide if the legacy is true or not, if it >> is true, it means the IRQ() is empty or defines IRQNoFlags and need to >> call irq_override(). >> >> Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#irq-descriptor # [1] >> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031 >> BugLink: http://bugs.launchpad.net/bugs/1909814 >> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Reported-and-tested-by: Manuel Krause <manuelkrause@netscape.net> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> drivers/acpi/resource.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c >> index ee78a210c606..dc01fb550b28 100644 >> --- a/drivers/acpi/resource.c >> +++ b/drivers/acpi/resource.c >> @@ -423,6 +423,13 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, >> } >> } >> >> +static bool irq_is_legacy(struct acpi_resource_irq *irq) >> +{ >> + return irq->triggering == ACPI_EDGE_SENSITIVE && >> + irq->polarity == ACPI_ACTIVE_HIGH && >> + irq->shareable == ACPI_EXCLUSIVE; >> +} >> + >> /** >> * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information. >> * @ares: Input ACPI resource object. >> @@ -461,7 +468,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> } >> acpi_dev_get_irqresource(res, irq->interrupts[index], >> irq->triggering, irq->polarity, >> - irq->shareable, true); >> + irq->shareable, irq_is_legacy(irq)); >> break; >> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: >> ext_irq = &ares->data.extended_irq; >> -- > > Applied as 5.14 material under a new subject ("ACPI: resources: Add > checks for ACPI IRQ override") and with some edits in the patch > changelog. > > Note, however, that this is a change in behavior that may result in > regressions on old machines, in which case some further refinements of > the code will be necessary. > > Thanks! > I just want to assure you, that also [PATCH v3] works fine on here. Maybe I can find some time to re-activate my other, over 12 years old, HP notebook and check for possible issues with this patch. I have to thank you all for your great work! Best regards, Manuel
On 6/9/21 8:44 PM, Rafael J. Wysocki wrote: > On Wed, Jun 9, 2021 at 4:14 AM Hui Wang <hui.wang@canonical.com> wrote: >> The laptop keyboard doesn't work on many MEDION notebooks, but the >> >> + irq->shareable, irq_is_legacy(irq)); >> break; >> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: >> ext_irq = &ares->data.extended_irq; >> -- > Applied as 5.14 material under a new subject ("ACPI: resources: Add > checks for ACPI IRQ override") and with some edits in the patch > changelog. > > Note, however, that this is a change in behavior that may result in > regressions on old machines, in which case some further refinements of > the code will be necessary. > > Thanks! Understand. Thanks.
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index ee78a210c606..dc01fb550b28 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -423,6 +423,13 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, } } +static bool irq_is_legacy(struct acpi_resource_irq *irq) +{ + return irq->triggering == ACPI_EDGE_SENSITIVE && + irq->polarity == ACPI_ACTIVE_HIGH && + irq->shareable == ACPI_EXCLUSIVE; +} + /** * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information. * @ares: Input ACPI resource object. @@ -461,7 +468,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, } acpi_dev_get_irqresource(res, irq->interrupts[index], irq->triggering, irq->polarity, - irq->shareable, true); + irq->shareable, irq_is_legacy(irq)); break; case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: ext_irq = &ares->data.extended_irq;
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 the Section 6.4.2.1 of ACPI 6.4 spec [1], 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. Here check 3 conditions to decide if the legacy is true or not, if it is true, it means the IRQ() is empty or defines IRQNoFlags and need to call irq_override(). Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#irq-descriptor # [1] BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031 BugLink: http://bugs.launchpad.net/bugs/1909814 Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reported-and-tested-by: Manuel Krause <manuelkrause@netscape.net> Signed-off-by: Hui Wang <hui.wang@canonical.com> --- drivers/acpi/resource.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)