[PATCH] ACPI : don't always override the acpi irq

[PATCH] ACPI : don't always override the acpi irq

[Hui Wang]

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(-)

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;
 
-- 
2.25.1

Re: [PATCH] ACPI : don't always override the acpi irq

[Rafael J. Wysocki]

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;
>
> --

Re: [PATCH] ACPI : don't always override the acpi irq

[Hui Wang]

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;
>>
>> --