Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally.

PATCH In review Phoenix Huang 3 participants 8 messages Jul 29, 2021 – Nov 10, 2021

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Phoenix Huang

author

2021-07-29 01:10 UTC #0 lore raw

PATCH 1/1 Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally.

Signed-off-by Phoenix Huang <[email protected]>

---
 drivers/input/mouse/elantech.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/input/mouse/elantech.c +11 -0

index 2d0bc029619f..07e1098f2d31 100644
@@ -517,6 +517,17 @@ static void elantech_report_trackpoint(struct psmouse *psmouse,
 	case 0x16008020U:
 	case 0x26800010U:
 	case 0x36808000U:
+	
+		/* This firmware misreport coordinates for trackpoint occasionally.
+		* So we discard these packets by pattern to prevent cursor jumps.
+		*/
+		if (packet[4] == 0x80 || packet[5] == 0x80 ||
+		    packet[1] >> 7 == packet[4] >> 7 ||
+		    packet[2] >> 7 == packet[5] >> 7) {
+		    	elantech_debug("discarding packet [%6ph]\n", packet);
+			break;
+
+		}
 		x = packet[4] - (int)((packet[1]^0x80) << 1);
 		y = (int)((packet[2]^0x80) << 1) - packet[5];

Signature · 1 line

2.25.1

phoenix

author

2021-08-30 12:21 UTC #1 lore raw

Hi Dmitry,

Would you review this patch, thanks

Best regards,
Phoenix Huang

-----Original Message-----
From: Phoenix Huang [mailto:[email protected]]
Sent: Thursday, July 29, 2021 9:10 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; Phoenix Huang <[email protected]>
Subject: [PATCH] Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally.

Signed-off-by Phoenix Huang <[email protected]>

---
 drivers/input/mouse/elantech.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/input/mouse/elantech.c +0 -0

index 2d0bc029619f..07e1098f2d31 100644
@@ -517,6 +517,17 @@ static void elantech_report_trackpoint(struct psmouse

*psmouse,
case 0x16008020U:
case 0x26800010U:
case 0x36808000U:
+
+ /* This firmware misreport coordinates for trackpoint
occasionally.
+ * So we discard these packets by pattern to prevent cursor
jumps.
+ */
+ if (packet[4] == 0x80 || packet[5] == 0x80 ||

+		    packet[1] >> 7 == packet[4] >> 7 ||
+		    packet[2] >> 7 == packet[5] >> 7) {
+		    	elantech_debug("discarding packet [%6ph]\n",

packet);
+ break;
+
+ }
x = packet[4] - (int)((packet[1]^0x80) << 1); y = (int)((packet[2]^0x80) << 1) - packet[5];

--
2.25.1

Dmitry Torokhov

maintainer

2021-08-30 20:23 UTC #2 lore raw

Hi Phoenix,

Quoted 36 lines · phoenix

Hi Dmitry,
Would you review this patch, thanks
Best regards,
Phoenix Huang
-----Original Message-----
From: Phoenix Huang [mailto:[email protected]]
Sent: Thursday, July 29, 2021 9:10 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; Phoenix Huang <[email protected]>
Subject: [PATCH] Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally.
Signed-off-by Phoenix Huang <[email protected]>
---
 drivers/input/mouse/elantech.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/input/mouse/elantech.c +2 -0
index 2d0bc029619f..07e1098f2d31 100644
@@ -517,6 +517,17 @@ static void elantech_report_trackpoint(struct psmouse
 *psmouse,
 	case 0x16008020U:
 	case 0x26800010U:
 	case 0x36808000U:
+	
+		/* This firmware misreport coordinates for trackpoint
occasionally.
drivers/input/mouse/elantech.c +1 -0
+		* So we discard these packets by pattern to prevent cursor
jumps.

drivers/input/mouse/elantech.c

Quoted 3 lines

drivers/input/mouse/elantech.c +3 -0

+		*/
+		if (packet[4] == 0x80 || packet[5] == 0x80 ||
+		    packet[1] >> 7 == packet[4] >> 7 ||

I think this will reject X coordinates in range [0, 127]. Is this really what is needed? What kind of patterns are you observing when firmware misreports coordinates?

Thanks.

Signature · 1 line

Dmitry

phoenix

author

2021-09-10 02:18 UTC #3 lore raw

Hi Dmity,

Sorry for late reply.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		if (packet[4] == 0x80 || packet[5] == 0x80 ||
+		    packet[1] >> 7 == packet[4] >> 7 ||

"I think this will reject X coordinates in range [0, 127]. Is this really what is needed? What kind of patterns are you observing when firmware misreports coordinates?"
this will reject X in range [128, 256] and [-128, -256]. This patch prevent cursor suddenly jump to screen edge or corner.
The patterns we observing as below:
1. x (packet[4]) or y (packet[5]) is 0x80, ex: [xx xx xx xx 80 80]
2. ~x sign bit is same as MSB(most significant bit) of x, ex: [30 00 00 06 00 00]
3. ~y sign bit is same as MSB of y, ex: [30 00 00 06 fa 00]

Best regards,
Phoenix Huang
-----Original Message-----
From: Dmitry Torokhov [mailto:[email protected]]
Sent: Tuesday, August 31, 2021 4:24 AM
To: phoenix <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally.

Hi Phoenix,

Quoted 37 lines · phoenix

Hi Dmitry,
Would you review this patch, thanks
Best regards,
Phoenix Huang
-----Original Message-----
From: Phoenix Huang [mailto:[email protected]]
Sent: Thursday, July 29, 2021 9:10 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; Phoenix Huang <[email protected]>
Subject: [PATCH] Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally.
Signed-off-by Phoenix Huang <[email protected]>
---
 drivers/input/mouse/elantech.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
(quoted diff) +0 -0
b/drivers/input/mouse/elantech.c index 2d0bc029619f..07e1098f2d31 100644
drivers/input/mouse/elantech.c +2 -0
@@ -517,6 +517,17 @@ static void elantech_report_trackpoint(struct 
 psmouse *psmouse,
 	case 0x16008020U:
 	case 0x26800010U:
 	case 0x36808000U:
+	
+		/* This firmware misreport coordinates for trackpoint
occasionally.
drivers/input/mouse/elantech.c +1 -0
+		* So we discard these packets by pattern to prevent cursor
jumps.

drivers/input/mouse/elantech.c

Quoted 3 lines

drivers/input/mouse/elantech.c +3 -0

+		*/
+		if (packet[4] == 0x80 || packet[5] == 0x80 ||
+		    packet[1] >> 7 == packet[4] >> 7 ||

I think this will reject X coordinates in range [0, 127]. Is this really what is needed? What kind of patterns are you observing when firmware misreports coordinates?

Thanks.

--
Dmitry

Yufei Du

2021-11-07 20:02 UTC #4 lore raw

In reply to Dmitry Torokhov

Hi Dmitry,

I am a user currently suffering from the firmware bug that causes my trackpoint to be unusable on my Lenovo X13 Gen 1 AMD. I tested this patch in the latest 5.15 mainline kernel tree and can possibly answer your questions regarding the patch.

Quoted 2 lines

I think this will reject X coordinates in range [0, 127]. Is this really what is needed?

As Phoenix explained in his reply, the patch will reject X and Y coordinates larger than 127 or smaller than -127. This is because the most significant bit of packet[1] (for X) and packet[2] (for Y) are inverse of the sign bit. Although I do not have the formal design specification of the device, I tried pushing the trackpoint as hard as I could, and the X and Y coordinates never go beyond +/-127 normally, so I believe that this patch would not cause any functionality issue.

Quoted 2 lines

What kind of patterns are you observing when firmware misreports coordinates?

Here is a sample log I observed (with elantech.debug set to 2 for more debugging messages):
psmouse serio1: elantech: PS/2 packet [10 00 80 26 f9 0a]
psmouse serio1: elantech: PS/2 packet [10 00 80 26 00 80]
psmouse serio1: elantech: PS/2 packet [10 00 80 00 00 00]
psmouse serio1: Touchpad at isa0060/serio1/input0 lost sync at byte 6 psmouse serio1: elantech: PS/2 packet [10 00 80 26 f8 0b]
psmouse serio1: Touchpad at isa0060/serio1/input0 - driver resynced.

The first packet is a valid packet. The second packet is the incorrect packet that this patch prevents. The third packet is a corrupted packet that psmouse detects (as shown in the "lost sync" message), and the fourth packet is a valid packet again. Without the patch, the second packet would cause the cursor to jump to the edge of the screen.

So far, all my logs of this issue show the same pattern of a jumpy incorrect packet followed by a corrupted packet that causes psmouse to lose sync. There are more logs and discussions in the BugZilla entry here: https://bugzilla.kernel.org/show_bug.cgi?id=209167

Hope my explanation helped a bit.

Thanks!

Best,
Yufei Du

Dmitry Torokhov

maintainer

2021-11-08 06:06 UTC #5 lore raw

Hi Yufei,

Quoted 36 lines · Yufei Du

Hi Dmitry,
I am a user currently suffering from the firmware bug that causes my trackpoint to be unusable on my Lenovo X13 Gen 1 AMD. I tested this patch in the latest 5.15 mainline kernel tree and can possibly answer your questions regarding the patch.
Quoted 2 lines
I think this will reject X coordinates in range [0, 127]. Is this really what is needed?
As Phoenix explained in his reply, the patch will reject X and Y coordinates larger than 127 or smaller than -127. This is because the most significant bit of packet[1] (for X) and packet[2] (for Y) are inverse of the sign bit. Although I do not have the formal design specification of the device, I tried pushing the trackpoint as hard as I could, and the X and Y coordinates never go beyond +/-127 normally, so I believe that this patch would not cause any functionality issue.
Quoted 2 lines
What kind of patterns are you observing when firmware misreports coordinates?
Here is a sample log I observed (with elantech.debug set to 2 for more debugging messages):
psmouse serio1: elantech: PS/2 packet [10 00 80 26 f9 0a] psmouse serio1: elantech: PS/2 packet [10 00 80 26 00 80] psmouse serio1: elantech: PS/2 packet [10 00 80 00 00 00] psmouse serio1: Touchpad at isa0060/serio1/input0 lost sync at byte 6 psmouse serio1: elantech: PS/2 packet [10 00 80 26 f8 0b] psmouse serio1: Touchpad at isa0060/serio1/input0 - driver resynced.
The first packet is a valid packet. The second packet is the incorrect packet that this patch prevents. The third packet is a corrupted packet that psmouse detects (as shown in the "lost sync" message), and the fourth packet is a valid packet again. Without the patch, the second packet would cause the cursor to jump to the edge of the screen.

Quoted 6 lines

So far, all my logs of this issue show the same pattern of a jumpy incorrect packet followed by a corrupted packet that causes psmouse to lose sync. There are more logs and discussions in the BugZilla entry here:
https://bugzilla.kernel.org/show_bug.cgi?id=209167
Hope my explanation helped a bit.

Sorry I lost track of this patch. Can I add your Tested-by to it before applying?

Thanks.

Signature · 1 line

Dmitry

Yufei Du

2021-11-08 06:14 UTC #6 lore raw

Hi Dmitry,

Quoted 2 lines

Sorry I lost track of this patch. Can I add your Tested-by to it before applying?

Sure. Thanks!

Best,
Yufei

Dmitry Torokhov

maintainer

2021-11-10 06:24 UTC #7 lore raw

Quoted 6 lines · Yufei Du

Hi Dmitry,
Quoted 2 lines
Sorry I lost track of this patch. Can I add your Tested-by to it before applying?
Sure. Thanks!

It is applied now, thank you everyone.

Signature · 1 line

Dmitry