Skip to contentSkip to conversation

i8042: Work around HP TGL bug

PATCH New Anton Zhilyaev 1 participant 1 message Feb 1, 2021

Subsystems (2)
HID Input drivers
lore
Share & tools
lore

Anton Zhilyaev

author
#0 lore raw

PATCH 1/1 i8042: Work around HP TGL bug

Some Tiger Lake laptops like HP Spectre x360 13-aw2xxx have a bug in i8042 implementation that causes keyboard initialization to take from few seconds to sometimes a minute or more each time the system starts or resumes.

In a nutshell problem is caused by interrupt being deasserted after byte read, even when there are more bytes readily available and I8042_STR_OBF is still set. Once in a while it keeps interrupt asserted, I haven't found any explanation for this.

This causes atkbd_probe to fail on both ATKBD_CMD_GETID (no id received) and ATKBD_CMD_SETLEDS (id received instead of ack) approaches which causes keyboard initialization to be re-tried multiple times until it randomly scores to get a valid reply either by interrupt surviving long enough for ID to be read or by ATKBD_CMD_SETLEDS luckily receiving some old ack.

This patch turns single-shot interrupt handler into a read loop that runs until I8042_STR_OBF goes unset or 4 valid non-filtered bytes are read or 32 iterations are performed. Another loop passes the read bytes to the serio layer later after unlocking i8042_lock.

Here's HP Spectre x360 issue discussion with this problem mentioned a few times:
https://bbs.archlinux.org/viewtopic.php?id=261108

Here's some logs illustrating the problem and the solution:
http://cpp.in/dev/hp_x360_tgl_kbd_before.log
http://cpp.in/dev/hp_x360_tgl_kbd_after.log

Signed-off-by Anton Zhilyaev <[email protected]>

---

drivers/input/serio/i8042.c +58 -41
@@ -159,6 +159,8 @@ struct i8042_port {
 #define I8042_MUX_PORT_NO	2
 #define I8042_NUM_PORTS		(I8042_NUM_MUX_PORTS + 2)
 
+#define I8042_QUEUE_SIZE	4
+
 static struct i8042_port i8042_ports[I8042_NUM_PORTS];
 
 static unsigned char i8042_initial_ctr;

@@ -520,33 +522,36 @@ static irqreturn_t i8042_interrupt(int i
 	struct i8042_port *port;
 	struct serio *serio;
 	unsigned long flags;
-	unsigned char str, data;
-	unsigned int dfl;
-	unsigned int port_no;
+	unsigned char str;
 	bool filtered;
+	unsigned char q_data[I8042_QUEUE_SIZE];
+	unsigned int q_port[I8042_QUEUE_SIZE];
+	unsigned int q_dfl[I8042_QUEUE_SIZE];
+	unsigned int q_used = 0, q_cycles = 0, q_i;
 	int ret = 1;
 
 	spin_lock_irqsave(&i8042_lock, flags);
 
 	str = i8042_read_status();
-	if (unlikely(~str & I8042_STR_OBF)) {
-		spin_unlock_irqrestore(&i8042_lock, flags);
-		if (irq)
-			dbg("Interrupt %d, without any data\n", irq);
-		ret = 0;
-		goto out;
-	}
+	do {
+		if (unlikely(~str & I8042_STR_OBF)) {
+			spin_unlock_irqrestore(&i8042_lock, flags);
+			if (irq)
+				dbg("Interrupt %d, without any data\n", irq);
+			ret = 0;
+			goto out;
+		}
 
-	data = i8042_read_data();
+		q_data[q_used] = i8042_read_data();
 
-	if (i8042_mux_present && (str & I8042_STR_AUXDATA)) {
-		static unsigned long last_transmit;
-		static unsigned char last_str;
-
-		dfl = 0;
-		if (str & I8042_STR_MUXERR) {
-			dbg("MUX error, status is %02x, data is %02x\n",
-			    str, data);
+		if (i8042_mux_present && (str & I8042_STR_AUXDATA)) {
+			static unsigned long last_transmit;
+			static unsigned char last_str;
+
+			q_dfl[q_used] = 0;
+			if (str & I8042_STR_MUXERR) {
+				dbg("MUX error, status is %02x, data is %02x\n",
+				    str, q_data[q_used]);
 /*
  * When MUXERR condition is signalled the data register can only contain
  * 0xfd, 0xfe or 0xff if implementation follows the spec. Unfortunately

@@ -560,7 +565,7 @@ static irqreturn_t i8042_interrupt(int i
  * was transmitted (if transmission happened not too long ago).
  */
 
-			switch (data) {
+				switch (q_data[q_used]) {
 				default:
 					if (time_before(jiffies, last_transmit + HZ/10)) {
 						str = last_str;

@@ -569,37 +574,49 @@ static irqreturn_t i8042_interrupt(int i
 					fallthrough;	/* report timeout */
 				case 0xfc:
 				case 0xfd:
-				case 0xfe: dfl = SERIO_TIMEOUT; data = 0xfe; break;
-				case 0xff: dfl = SERIO_PARITY;  data = 0xfe; break;
+				case 0xfe:
+					q_dfl[q_used] = SERIO_TIMEOUT;
+					q_data[q_used] = 0xfe;
+					break;
+				case 0xff:
+					q_dfl[q_used] = SERIO_PARITY;
+					q_data[q_used] = 0xfe;
+					break;
+				}
 			}
-		}
 
-		port_no = I8042_MUX_PORT_NO + ((str >> 6) & 3);
-		last_str = str;
-		last_transmit = jiffies;
-	} else {
+			q_port[q_used] = I8042_MUX_PORT_NO + ((str >> 6) & 3);
+			last_str = str;
+			last_transmit = jiffies;
+		} else {
 
-		dfl = ((str & I8042_STR_PARITY) ? SERIO_PARITY : 0) |
-		      ((str & I8042_STR_TIMEOUT && !i8042_notimeout) ? SERIO_TIMEOUT : 0);
+			q_dfl[q_used] = ((str & I8042_STR_PARITY) ? SERIO_PARITY : 0) |
+					((str & I8042_STR_TIMEOUT && !i8042_notimeout) ? SERIO_TIMEOUT : 0);
 
-		port_no = (str & I8042_STR_AUXDATA) ?
-				I8042_AUX_PORT_NO : I8042_KBD_PORT_NO;
-	}
+			q_port[q_used] = (str & I8042_STR_AUXDATA) ?
+					I8042_AUX_PORT_NO : I8042_KBD_PORT_NO;
+		}
+
+		port = &i8042_ports[q_port[q_used]];
+		serio = port->exists ? port->serio : NULL;
+
+		filter_dbg(port->driver_bound, q_data[q_used], "<- i8042 (interrupt, %d, %d%s%s)\n",
+			   q_port[q_used], irq,
+			   q_dfl[q_used] & SERIO_PARITY ? ", bad parity" : "",
+			   q_dfl[q_used] & SERIO_TIMEOUT ? ", timeout" : "");
 
-	port = &i8042_ports[port_no];
-	serio = port->exists ? port->serio : NULL;
+		filtered = i8042_filter(q_data[q_used], str, serio);
 
-	filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
-		   port_no, irq,
-		   dfl & SERIO_PARITY ? ", bad parity" : "",
-		   dfl & SERIO_TIMEOUT ? ", timeout" : "");
+		if (likely(serio && !filtered))
+			++q_used;
 
-	filtered = i8042_filter(data, str, serio);
+		str = i8042_read_status();
+	} while ((q_used < I8042_QUEUE_SIZE) && (++q_cycles < 32) && (str & I8042_STR_OBF));
 
 	spin_unlock_irqrestore(&i8042_lock, flags);
 
-	if (likely(serio && !filtered))
-		serio_interrupt(serio, data, dfl);
+	for (q_i = 0; q_i < q_used; ++q_i)
+		serio_interrupt(i8042_ports[q_port[q_i]].serio, q_data[q_i], q_dfl[q_i]);
 
  out:
 	return IRQ_RETVAL(ret);