[PATCH] ACPI: resources: add legacy irq override exception by DMI info

[PATCH] ACPI: resources: add legacy irq override exception by DMI info

[Hui Wang]

After the commit 0ec4e55e9f57 ("ACPI: resources: Add checks for ACPI
IRQ override") is reverted, the keyboard of those Medion laptops can't
work again.

To fix the keyboard issue, here adding an override check by DMI info,
this will not affect other machines and this design refers to
the prt_quirks[] in the drivers/acpi/pci_irq.c.

If we meet similar issues on other platforms, we could expand the
table of skip_override_table[] or medion_laptop[].

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031
BugLink: http://bugs.launchpad.net/bugs/1909814
Reported-by: Manuel Krause <manuelkrause@netscape.net>
Tested-by: Manuel Krause <manuelkrause@netscape.net>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 drivers/acpi/resource.c | 52 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index ee78a210c606..434c8964f182 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -16,6 +16,7 @@
 #include <linux/ioport.h>
 #include <linux/slab.h>
 #include <linux/irq.h>
+#include <linux/dmi.h>
 
 #ifdef CONFIG_X86
 #define valid_IRQ(i) (((i) != 0) && ((i) != 2))
@@ -423,6 +424,49 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 	}
 }
 
+static const struct dmi_system_id medion_laptop[] = {
+	{
+		.ident = "MEDION P15651",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "MEDION"),
+			DMI_MATCH(DMI_BOARD_NAME, "M15T"),
+		},
+	},
+	{ }
+};
+
+struct irq_override_cmp {
+	const struct dmi_system_id *system;
+	unsigned char irq;
+	unsigned char triggering;
+	unsigned char polarity;
+	unsigned char shareable;
+};
+
+static const struct irq_override_cmp skip_override_table[] = {
+	{ medion_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0 },
+};
+
+static bool acpi_dev_legacy_irq_override(u32 gsi, u8 triggering, u8 polarity,
+					 u8 shareable)
+{
+	int i;
+	const struct irq_override_cmp *en;
+
+	for (i = 0; i < ARRAY_SIZE(skip_override_table); i++) {
+		en = &skip_override_table[i];
+
+		if (dmi_check_system(en->system) &&
+		    en->irq == gsi &&
+		    en->triggering == triggering &&
+		    en->polarity == polarity &&
+		    en->shareable == shareable)
+			return false;
+	}
+
+	return true;
+}
+
 /**
  * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
  * @ares: Input ACPI resource object.
@@ -447,6 +491,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 {
 	struct acpi_resource_irq *irq;
 	struct acpi_resource_extended_irq *ext_irq;
+	bool is_legacy;
 
 	switch (ares->type) {
 	case ACPI_RESOURCE_TYPE_IRQ:
@@ -459,9 +504,14 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 			irqresource_disabled(res, 0);
 			return false;
 		}
+
+		is_legacy = acpi_dev_legacy_irq_override(irq->interrupts[index],
+							 irq->triggering, irq->polarity,
+							 irq->shareable);
+
 		acpi_dev_get_irqresource(res, irq->interrupts[index],
 					 irq->triggering, irq->polarity,
-					 irq->shareable, true);
+					 irq->shareable, is_legacy);
 		break;
 	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
 		ext_irq = &ares->data.extended_irq;
-- 
2.25.1

Re: [PATCH] ACPI: resources: add legacy irq override exception by DMI info

[Manuel Krause]

Hi Hui Wang and all others,

thank you for notifying me and for giving it a new run for the 
kernel! ;-)

On 04/09/2021 03:43, Hui Wang wrote:
> After the commit 0ec4e55e9f57 ("ACPI: resources: Add checks for ACPI
> IRQ override") is reverted, the keyboard of those Medion laptops can't
> work again.
> 
> To fix the keyboard issue, here adding an override check by DMI info,
> this will not affect other machines and this design refers to
> the prt_quirks[] in the drivers/acpi/pci_irq.c.
> 
> If we meet similar issues on other platforms, we could expand the
> table of skip_override_table[] or medion_laptop[].

^
| IMO this is the major positive aspect of this patch, that it 
enables additions, allowing the kernel to work-around other buggy 
BIOSs / hardware properly in future.

Maybe https://bugzilla.kernel.org/show_bug.cgi?id=213353 "IRQs 
for onboard UARTs are not level-triggered with IRQNoFlags even if 
the parent section has the defaults in PRS" is another candidate 
to add to the table (but I haven't digged deeply enough into it 
to be sure).

Many thanks to all helpful people involved,
best regards,

Manuel

> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031
> BugLink: http://bugs.launchpad.net/bugs/1909814
> Reported-by: Manuel Krause <manuelkrause@netscape.net>
> Tested-by: Manuel Krause <manuelkrause@netscape.net>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>   drivers/acpi/resource.c | 52 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index ee78a210c606..434c8964f182 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
[...]

Re: [PATCH] ACPI: resources: add legacy irq override exception by DMI info

[Hui Wang]

Hi Rafael,

Could you please take a look at this patch and give some comment when 
you have time, the patch will not introduce the regression like the 
previous one.

Hi Manuel,

Thanks for your comment. Let's work together to make the keyboard work 
under Linux kernel.

Regards,

Hui.

On 9/5/21 12:04 AM, Manuel Krause wrote:
> Hi Hui Wang and all others,
>
> thank you for notifying me and for giving it a new run for the kernel! 
> ;-)
>
> On 04/09/2021 03:43, Hui Wang wrote:
>> After the commit 0ec4e55e9f57 ("ACPI: resources: Add checks for ACPI
>> IRQ override") is reverted, the keyboard of those Medion laptops can't
>> work again.
>>
>> To fix the keyboard issue, here adding an override check by DMI info,
>> this will not affect other machines and this design refers to
>> the prt_quirks[] in the drivers/acpi/pci_irq.c.
>>
>> If we meet similar issues on other platforms, we could expand the
>> table of skip_override_table[] or medion_laptop[].
>
> ^
> | IMO this is the major positive aspect of this patch, that it enables 
> additions, allowing the kernel to work-around other buggy BIOSs / 
> hardware properly in future.
>
> Maybe https://bugzilla.kernel.org/show_bug.cgi?id=213353 "IRQs for 
> onboard UARTs are not level-triggered with IRQNoFlags even if the 
> parent section has the defaults in PRS" is another candidate to add to 
> the table (but I haven't digged deeply enough into it to be sure).
>
> Many thanks to all helpful people involved,
> best regards,
>
> Manuel
>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031
>> BugLink: http://bugs.launchpad.net/bugs/1909814
>> Reported-by: Manuel Krause <manuelkrause@netscape.net>
>> Tested-by: Manuel Krause <manuelkrause@netscape.net>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   drivers/acpi/resource.c | 52 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index ee78a210c606..434c8964f182 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
> [...]

Re: [PATCH] ACPI: resources: add legacy irq override exception by DMI info

[Rafael J. Wysocki]

On Sat, Sep 4, 2021 at 3:44 AM Hui Wang <hui.wang@canonical.com> wrote:
>
> After the commit 0ec4e55e9f57 ("ACPI: resources: Add checks for ACPI
> IRQ override") is reverted, the keyboard of those Medion laptops can't
> work again.
>
> To fix the keyboard issue, here adding an override check by DMI info,
> this will not affect other machines and this design refers to
> the prt_quirks[] in the drivers/acpi/pci_irq.c.
>
> If we meet similar issues on other platforms, we could expand the
> table of skip_override_table[] or medion_laptop[].
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031
> BugLink: http://bugs.launchpad.net/bugs/1909814
> Reported-by: Manuel Krause <manuelkrause@netscape.net>
> Tested-by: Manuel Krause <manuelkrause@netscape.net>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  drivers/acpi/resource.c | 52 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index ee78a210c606..434c8964f182 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -16,6 +16,7 @@
>  #include <linux/ioport.h>
>  #include <linux/slab.h>
>  #include <linux/irq.h>
> +#include <linux/dmi.h>
>
>  #ifdef CONFIG_X86
>  #define valid_IRQ(i) (((i) != 0) && ((i) != 2))
> @@ -423,6 +424,49 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
>         }
>  }
>
> +static const struct dmi_system_id medion_laptop[] = {
> +       {
> +               .ident = "MEDION P15651",
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "MEDION"),
> +                       DMI_MATCH(DMI_BOARD_NAME, "M15T"),
> +               },
> +       },
> +       { }
> +};
> +
> +struct irq_override_cmp {
> +       const struct dmi_system_id *system;
> +       unsigned char irq;
> +       unsigned char triggering;
> +       unsigned char polarity;
> +       unsigned char shareable;
> +};
> +
> +static const struct irq_override_cmp skip_override_table[] = {
> +       { medion_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0 },
> +};
> +
> +static bool acpi_dev_legacy_irq_override(u32 gsi, u8 triggering, u8 polarity,
> +                                        u8 shareable)
> +{
> +       int i;
> +       const struct irq_override_cmp *en;

This can be declared inside the for () loop.  Also, if "en" means
"entry", please call the variable "entry".

> +
> +       for (i = 0; i < ARRAY_SIZE(skip_override_table); i++) {
> +               en = &skip_override_table[i];
> +
> +               if (dmi_check_system(en->system) &&
> +                   en->irq == gsi &&
> +                   en->triggering == triggering &&
> +                   en->polarity == polarity &&
> +                   en->shareable == shareable)
> +                       return false;
> +       }
> +
> +       return true;
> +}
> +
>  /**
>   * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
>   * @ares: Input ACPI resource object.
> @@ -447,6 +491,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  {
>         struct acpi_resource_irq *irq;
>         struct acpi_resource_extended_irq *ext_irq;
> +       bool is_legacy;
>
>         switch (ares->type) {
>         case ACPI_RESOURCE_TYPE_IRQ:
> @@ -459,9 +504,14 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>                         irqresource_disabled(res, 0);
>                         return false;
>                 }
> +
> +               is_legacy = acpi_dev_legacy_irq_override(irq->interrupts[index],
> +                                                        irq->triggering, irq->polarity,
> +                                                        irq->shareable);
> +
>                 acpi_dev_get_irqresource(res, irq->interrupts[index],
>                                          irq->triggering, irq->polarity,
> -                                        irq->shareable, true);
> +                                        irq->shareable, is_legacy);

Maybe we can rename the last argument of acpi_dev_get_irqresource() to
check_override (or similar) and do the check in there when that is
set?

>                 break;
>         case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>                 ext_irq = &ares->data.extended_irq;
> --

Re: [PATCH] ACPI: resources: add legacy irq override exception by DMI info

[Hui Wang]

On 9/14/21 1:10 AM, Rafael J. Wysocki wrote:
> On Sat, Sep 4, 2021 at 3:44 AM Hui Wang <hui.wang@canonical.com> wrote:
>> After the commit 0ec4e55e9f57 ("ACPI: resources: Add checks for ACPI
>> IRQ override") is reverted, the keyboard of those Medion laptops can't
>> work again.
>>
>> To fix the keyboard issue, here adding an override check by DMI info,
>> this will not affect other machines and this design refers to
>> the prt_quirks[] in the drivers/acpi/pci_irq.c.
>>
>> If we meet similar issues on other platforms, we could expand the
>> table of skip_override_table[] or medion_laptop[].
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213031
>> BugLink: http://bugs.launchpad.net/bugs/1909814
>> Reported-by: Manuel Krause <manuelkrause@netscape.net>
>> Tested-by: Manuel Krause <manuelkrause@netscape.net>
[...]
>> +                                        u8 shareable)
>> +{
>> +       int i;
>> +       const struct irq_override_cmp *en;
> This can be declared inside the for () loop.  Also, if "en" means
> "entry", please call the variable "entry".
OK, got it.
>
>> +
>> +       for (i = 0; i < ARRAY_SIZE(skip_override_table); i++) {
>> +               en = &skip_override_table[i];
>> +
>> +               if (dmi_check_system(en->system) &&
>> +                   en->irq == gsi &&
>> +                   en->triggering == triggering &&
>> +                   en->polarity == polarity &&
>> +                   en->shareable == shareable)
>> +                       return false;
>> +       }
>> +
>> +       return true;
>> +}
>> +
>>   /**
>>    * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
>>    * @ares: Input ACPI resource object.
>> @@ -447,6 +491,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>>   {
>>          struct acpi_resource_irq *irq;
>>          struct acpi_resource_extended_irq *ext_irq;
>> +       bool is_legacy;
>>
>>          switch (ares->type) {
>>          case ACPI_RESOURCE_TYPE_IRQ:
>> @@ -459,9 +504,14 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>>                          irqresource_disabled(res, 0);
>>                          return false;
>>                  }
>> +
>> +               is_legacy = acpi_dev_legacy_irq_override(irq->interrupts[index],
>> +                                                        irq->triggering, irq->polarity,
>> +                                                        irq->shareable);
>> +
>>                  acpi_dev_get_irqresource(res, irq->interrupts[index],
>>                                           irq->triggering, irq->polarity,
>> -                                        irq->shareable, true);
>> +                                        irq->shareable, is_legacy);
> Maybe we can rename the last argument of acpi_dev_get_irqresource() to
> check_override (or similar) and do the check in there when that is
> set?

OK will change it as the suggestion.

Thanks.

>>                  break;
>>          case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>                  ext_irq = &ares->data.extended_irq;
>> --