| Message ID | 20210729010940.5752-1-phoenix@emc.com.tw (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | be896bd3b72b44126c55768f14c22a8729b0992e |
| Headers | show |
| Series | Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally. | expand |
Hi Dmitry, Would you review this patch, thanks Best regards, Phoenix Huang -----Original Message----- From: Phoenix Huang [mailto:phoenix@emc.com.tw] Sent: Thursday, July 29, 2021 9:10 AM To: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; dmitry.torokhov@gmail.com Cc: jingle.wu@emc.com.tw; josh.chen@emc.com.tw; dave.wang@emc.com.tw; Phoenix Huang <phoenix@emc.com.tw> Subject: [PATCH] Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally. Signed-off-by: Phoenix Huang <phoenix@emc.com.tw> --- drivers/input/mouse/elantech.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c index 2d0bc029619f..07e1098f2d31 100644 --- a/drivers/input/mouse/elantech.c +++ b/drivers/input/mouse/elantech.c @@ -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
Hi Phoenix, On Mon, Aug 30, 2021 at 08:19:48PM +0800, phoenix wrote: > Hi Dmitry, > > Would you review this patch, thanks > > Best regards, > Phoenix Huang > > -----Original Message----- > From: Phoenix Huang [mailto:phoenix@emc.com.tw] > Sent: Thursday, July 29, 2021 9:10 AM > To: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; > dmitry.torokhov@gmail.com > Cc: jingle.wu@emc.com.tw; josh.chen@emc.com.tw; dave.wang@emc.com.tw; > Phoenix Huang <phoenix@emc.com.tw> > Subject: [PATCH] Input: elantench - Fix the firmware misreport coordinates > for trackpoint occasionally. > > Signed-off-by: Phoenix Huang <phoenix@emc.com.tw> > --- > drivers/input/mouse/elantech.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c > index 2d0bc029619f..07e1098f2d31 100644 > --- a/drivers/input/mouse/elantech.c > +++ b/drivers/input/mouse/elantech.c > @@ -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 || 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.
Hi Dmity, Sorry for late reply. > + 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:dmitry.torokhov@gmail.com] Sent: Tuesday, August 31, 2021 4:24 AM To: phoenix <phoenix@emc.com.tw> Cc: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; jingle.wu@emc.com.tw; josh.chen@emc.com.tw; dave.wang@emc.com.tw Subject: Re: [PATCH] Input: elantench - Fix the firmware misreport coordinates for trackpoint occasionally. Hi Phoenix, On Mon, Aug 30, 2021 at 08:19:48PM +0800, phoenix wrote: > Hi Dmitry, > > Would you review this patch, thanks > > Best regards, > Phoenix Huang > > -----Original Message----- > From: Phoenix Huang [mailto:phoenix@emc.com.tw] > Sent: Thursday, July 29, 2021 9:10 AM > To: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; > dmitry.torokhov@gmail.com > Cc: jingle.wu@emc.com.tw; josh.chen@emc.com.tw; dave.wang@emc.com.tw; > Phoenix Huang <phoenix@emc.com.tw> > Subject: [PATCH] Input: elantench - Fix the firmware misreport > coordinates for trackpoint occasionally. > > Signed-off-by: Phoenix Huang <phoenix@emc.com.tw> > --- > drivers/input/mouse/elantech.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/input/mouse/elantech.c > b/drivers/input/mouse/elantech.c index 2d0bc029619f..07e1098f2d31 > 100644 > --- a/drivers/input/mouse/elantech.c > +++ b/drivers/input/mouse/elantech.c > @@ -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 || 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
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. > 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. > 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
Hi Yufei, On Sun, Nov 07, 2021 at 03:02:27PM -0500, Yufei Du wrote: > 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. > > > 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. > > > 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. Sorry I lost track of this patch. Can I add your Tested-by to it before applying? Thanks.
Hi Dmitry, > Sorry I lost track of this patch. Can I add your Tested-by to it before > applying? Sure. Thanks! Best, Yufei
On Mon, Nov 08, 2021 at 01:14:44AM -0500, Yufei Du wrote: > Hi Dmitry, > > > 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.
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c index 2d0bc029619f..07e1098f2d31 100644 --- a/drivers/input/mouse/elantech.c +++ b/drivers/input/mouse/elantech.c @@ -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];
Signed-off-by: Phoenix Huang <phoenix@emc.com.tw> --- drivers/input/mouse/elantech.c | 11 +++++++++++ 1 file changed, 11 insertions(+)