Re: [PATCH v6] ACPI: skip IRQ override on AMD Zen platforms

[Jiri Slaby <jirislaby@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4685 bytes --]

On 28. 09. 22, 8:20, Jiri Slaby wrote:
> Hi,
> 
> On 12. 07. 22, 4:00, Chuanhong Guo wrote:
>> IRQ override isn't needed on modern AMD Zen systems.
>> There's an active low keyboard IRQ on AMD Ryzen 6000 and it will stay
>> this way on newer platforms. This IRQ override breaks keyboards for
>> almost all Ryzen 6000 laptops currently on the market.
>>
>> Skip this IRQ override for all AMD Zen platforms because this IRQ
>> override is supposed to be a workaround for buggy ACPI DSDT and we can't
>> have a long list of all future AMD CPUs/Laptops in the kernel code.
>> If a device with buggy ACPI DSDT shows up, a separated list containing
>> just them should be created.
> 
> This breaks pads on IdeaPad 5 Flex:
> https://bugzilla.suse.com/show_bug.cgi?id=1203794
> 
>  > [    1.058135] hid-generic 0020:1022:0001.0001: hidraw0: SENSOR HUB 
> HID v0.00 Device [hid-amdsfh 1022:0001] on pcie_mp2_amd
>  > [    2.038937] i2c_designware AMDI0010:00: controller timed out
>  > [    2.146627] i2c_designware AMDI0010:03: controller timed out
>  > [    6.166859] i2c_hid_acpi i2c-MSFT0001:00: failed to reset device: -61
>  > [    8.279604] i2c_designware AMDI0010:03: controller timed out
>  > [   12.310897] i2c_hid_acpi i2c-MSFT0001:00: failed to reset device: -61
>  > [   14.429372] i2c_designware AMDI0010:03: controller timed out
>  > [   18.462629] i2c_hid_acpi i2c-MSFT0001:00: failed to reset device: -61
>  > [   20.579183] i2c_designware AMDI0010:03: controller timed out
>  > [   24.598703] i2c_hid_acpi i2c-MSFT0001:00: failed to reset device: -61
>  > [   25.629071] i2c_hid_acpi i2c-MSFT0001:00: can't add hid device: -61
>  > [   25.629430] i2c_hid_acpi: probe of i2c-MSFT0001:00 failed with 
> error -61
> 
> The diff of good and bad dmesgs:
> -ACPI: IRQ 10 override to edge, high
> -ACPI: IRQ 6 override to edge, high
> 
> The diff of /proc/interrupts:
>       6: ...  IR-IO-APIC    [-6-fasteoi-]    {+6-edge+}      AMDI0010:03
>      10: ...  IR-IO-APIC   [-10-fasteoi-]   {+10-edge+}      AMDI0010:00
> 
> And:
>    i2c_designware: /devices/platform/AMDI0010:00
>    i2c_designware: /devices/platform/AMDI0010:03
> 
> 
> So the if needs to be fine-tuned, apparently. Maybe introduce some list 
> as suggested in the commit log. Based on the below?

Something like the attached. It's:
1) untested yet
2) contains more debug messaging
3) contains both cases for ACPI_ACTIVE_* as I don't know the original 
polarity

I don't know how widely this is spread -- maybe it would be worth a 
commandline parameter so that people can work around this until this is 
fixed by a DMI entry permanently?

> DMI says:
>    System Info: #14
>      Manufacturer: "LENOVO"
>      Product: "82RA"
>      Version: "IdeaPad Flex 5 16ALC7"
>      Serial: "PW02359K"
>      UUID: 6b2d54d9-cd80-11ec-83eb-e00af665fbac
>      Wake-up: 0x06 (Power Switch)
>    Board Info: #15
>      Manufacturer: "LENOVO"
>      Product: "LNVNB161216"
>      Version: "SDK0T76463 WIN"
>      Serial: "PW02359K"
>      Asset Tag: "No Asset Tag"
>      Type: 0x0a (Motherboard)
>      Features: 0x09
>        Hosting Board
>        Replaceable
>      Location: "Chassis Location Unknown"
>    Chassis Info: #16
>      Manufacturer: "LENOVO"
>      Version: "IdeaPad Flex 5 16ALC7"
>      Serial: "PW02359K"
>      Asset Tag: "No Asset Tag"
>      Type: 0x1f (Other)
>      Bootup State: 0x03 (Safe)
>      Power Supply State: 0x03 (Safe)
>      Thermal State: 0x01 (Other)
>      Security Status: 0x01 (Other)
> 
>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
>> ---
>> Change sice v5: reworked
>>
>>   drivers/acpi/resource.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index c2d494784425..510cdec375c4 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -416,6 +416,16 @@ static bool acpi_dev_irq_override(u32 gsi, u8 
>> triggering, u8 polarity,
>>   {
>>       int i;
>> +#ifdef CONFIG_X86
>> +    /*
>> +     * IRQ override isn't needed on modern AMD Zen systems and
>> +     * this override breaks active low IRQs on AMD Ryzen 6000 and
>> +     * newer systems. Skip it.
>> +     */
>> +    if (boot_cpu_has(X86_FEATURE_ZEN))
>> +        return false;
>> +#endif
>> +
>>       for (i = 0; i < ARRAY_SIZE(skip_override_table); i++) {
>>           const struct irq_override_cmp *entry = &skip_override_table[i];
> 
> thanks,

-- 
js

[-- Attachment #2: 0001-ACPI-resource-do-IRQ-override-on-LENOVO-IdeaPad.patch --]
[-- Type: text/x-patch, Size: 4020 bytes --]

From 485dd1229b9d2f322879d85731af4a7952b9cc4b Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
Date: Wed, 28 Sep 2022 09:43:05 +0200
Subject: [PATCH] ACPI: resource: do IRQ override on LENOVO IdeaPad

LENOVO IdeaPad Flex 5 is ryzen-5 based and the commit below removed irq
overriding for those. This broke touchscreen and trackpad:
 i2c_designware AMDI0010:00: controller timed out
 i2c_designware AMDI0010:03: controller timed out
 i2c_hid_acpi i2c-MSFT0001:00: failed to reset device: -61
 i2c_designware AMDI0010:03: controller timed out
 ...
 i2c_hid_acpi i2c-MSFT0001:00: can't add hid device: -61
 i2c_hid_acpi: probe of i2c-MSFT0001:00 failed with error -61

White list this specific model in the override_table.

For this to work, the ZEN test needs to be put below the table walk.

Fixes: 37c81d9f1d1b (ACPI: resource: skip IRQ override on AMD Zen platforms)
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/acpi/resource.c | 51 ++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index e40b2153911d..ab25e93cf22a 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -417,17 +417,33 @@ static const struct dmi_system_id asus_laptop[] = {
 	{ }
 };
 
+static const struct dmi_system_id lenovo_laptop[] = {
+	{
+		.ident = "LENOVO IdeaPad Flex 5 16ALC7",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "82RA"),
+		},
+	},
+	{ }
+};
+
 struct irq_override_cmp {
 	const struct dmi_system_id *system;
 	unsigned char irq;
 	unsigned char triggering;
 	unsigned char polarity;
 	unsigned char shareable;
+	bool override;
 };
 
-static const struct irq_override_cmp skip_override_table[] = {
-	{ medion_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0 },
-	{ asus_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0 },
+static const struct irq_override_cmp override_table[] = {
+	{ medion_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, false },
+	{ asus_laptop, 1, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW, 0, false },
+	{ lenovo_laptop, 6, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW, 0, true },
+	{ lenovo_laptop, 10, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_LOW, 0, true },
+	{ lenovo_laptop, 6, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_HIGH, 0, true },
+	{ lenovo_laptop, 10, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_HIGH, 0, true },
 };
 
 static bool acpi_dev_irq_override(u32 gsi, u8 triggering, u8 polarity,
@@ -435,6 +451,17 @@ static bool acpi_dev_irq_override(u32 gsi, u8 triggering, u8 polarity,
 {
 	int i;
 
+	for (i = 0; i < ARRAY_SIZE(override_table); i++) {
+		const struct irq_override_cmp *entry = &override_table[i];
+
+		if (dmi_check_system(entry->system) &&
+		    entry->irq == gsi &&
+		    entry->triggering == triggering &&
+		    entry->polarity == polarity &&
+		    entry->shareable == shareable)
+			return entry->override;
+	}
+
 #ifdef CONFIG_X86
 	/*
 	 * IRQ override isn't needed on modern AMD Zen systems and
@@ -445,17 +472,6 @@ static bool acpi_dev_irq_override(u32 gsi, u8 triggering, u8 polarity,
 		return false;
 #endif
 
-	for (i = 0; i < ARRAY_SIZE(skip_override_table); i++) {
-		const struct irq_override_cmp *entry = &skip_override_table[i];
-
-		if (dmi_check_system(entry->system) &&
-		    entry->irq == gsi &&
-		    entry->triggering == triggering &&
-		    entry->polarity == polarity &&
-		    entry->shareable == shareable)
-			return false;
-	}
-
 	return true;
 }
 
@@ -487,8 +503,11 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 		u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
 
 		if (triggering != trig || polarity != pol) {
-			pr_warn("ACPI: IRQ %d override to %s, %s\n", gsi,
-				t ? "level" : "edge", p ? "low" : "high");
+			pr_warn("ACPI: IRQ %d override to %s%s, %s%s\n", gsi,
+				t ? "level" : "edge",
+				trig == triggering ? "" : "(!)",
+				p ? "low" : "high",
+				pol == polarity ? "" : "(!)");
 			triggering = trig;
 			polarity = pol;
 		}
-- 
2.37.3