All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd
@ 2023-09-18 14:50 Mikhail Khvainitski
  2023-09-23 22:58 ` Mikhail Khvainitski
  0 siblings, 1 reply; 16+ messages in thread
From: Mikhail Khvainitski @ 2023-09-18 14:50 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, linux-kernel, Mikhail Khvainitski

Built-in firmware of cptkbd handles scrolling by itself (when middle
button is pressed) but with issues: it does not support horizontal and
hi-res scrolling and upon middle button release it sends middle button
click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
lenovo: Hide middle-button press until release") workarounds last
issue but it's impossible to workaround scrolling-related issues
without firmware modification.

Likely, Dennis Schneider has reverse engineered the firmware and
provided an instruction on how to patch it [1]. However,
aforementioned workaround prevents userspace (libinput) from knowing
exact moment when middle button has been pressed down and performing
"On-Button scrolling". This commit makes it possible to disable this
workaround by sysfs knob.

Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]

Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>
---
 drivers/hid/hid-lenovo.c | 84 +++++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..8993fa1ce66a 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -53,6 +53,7 @@ struct lenovo_drvdata {
 	int press_speed;
 	u8 middlebutton_state; /* 0:Up, 1:Down (undecided), 2:Scrolling */
 	bool fn_lock;
+	bool middleclick_workaround_cptkbd;
 };
 
 #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
@@ -603,6 +604,36 @@ static ssize_t attr_sensitivity_store_cptkbd(struct device *dev,
 	return count;
 }
 
+static ssize_t attr_middleclick_workaround_show_cptkbd(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		cptkbd_data->middleclick_workaround_cptkbd);
+}
+
+static ssize_t attr_middleclick_workaround_store_cptkbd(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf,
+		size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+	int value;
+
+	if (kstrtoint(buf, 10, &value))
+		return -EINVAL;
+	if (value < 0 || value > 1)
+		return -EINVAL;
+
+	cptkbd_data->middleclick_workaround_cptkbd = !!value;
+
+	return count;
+}
+
 
 static struct device_attribute dev_attr_fn_lock =
 	__ATTR(fn_lock, S_IWUSR | S_IRUGO,
@@ -614,10 +645,16 @@ static struct device_attribute dev_attr_sensitivity_cptkbd =
 			attr_sensitivity_show_cptkbd,
 			attr_sensitivity_store_cptkbd);
 
+static struct device_attribute dev_attr_middleclick_workaround_cptkbd =
+	__ATTR(middleclick_workaround, S_IWUSR | S_IRUGO,
+			attr_middleclick_workaround_show_cptkbd,
+			attr_middleclick_workaround_store_cptkbd);
+
 
 static struct attribute *lenovo_attributes_cptkbd[] = {
 	&dev_attr_fn_lock.attr,
 	&dev_attr_sensitivity_cptkbd.attr,
+	&dev_attr_middleclick_workaround_cptkbd.attr,
 	NULL
 };
 
@@ -668,31 +705,33 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
 {
 	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
 
-	/* "wheel" scroll events */
-	if (usage->type == EV_REL && (usage->code == REL_WHEEL ||
-			usage->code == REL_HWHEEL)) {
-		/* Scroll events disable middle-click event */
-		cptkbd_data->middlebutton_state = 2;
-		return 0;
-	}
+	if (cptkbd_data->middleclick_workaround_cptkbd) {
+		/* "wheel" scroll events */
+		if (usage->type == EV_REL && (usage->code == REL_WHEEL ||
+				usage->code == REL_HWHEEL)) {
+			/* Scroll events disable middle-click event */
+			cptkbd_data->middlebutton_state = 2;
+			return 0;
+		}
 
-	/* Middle click events */
-	if (usage->type == EV_KEY && usage->code == BTN_MIDDLE) {
-		if (value == 1) {
-			cptkbd_data->middlebutton_state = 1;
-		} else if (value == 0) {
-			if (cptkbd_data->middlebutton_state == 1) {
-				/* No scrolling inbetween, send middle-click */
-				input_event(field->hidinput->input,
-					EV_KEY, BTN_MIDDLE, 1);
-				input_sync(field->hidinput->input);
-				input_event(field->hidinput->input,
-					EV_KEY, BTN_MIDDLE, 0);
-				input_sync(field->hidinput->input);
+		/* Middle click events */
+		if (usage->type == EV_KEY && usage->code == BTN_MIDDLE) {
+			if (value == 1) {
+				cptkbd_data->middlebutton_state = 1;
+			} else if (value == 0) {
+				if (cptkbd_data->middlebutton_state == 1) {
+					/* No scrolling inbetween, send middle-click */
+					input_event(field->hidinput->input,
+						EV_KEY, BTN_MIDDLE, 1);
+					input_sync(field->hidinput->input);
+					input_event(field->hidinput->input,
+						EV_KEY, BTN_MIDDLE, 0);
+					input_sync(field->hidinput->input);
+				}
+				cptkbd_data->middlebutton_state = 0;
 			}
-			cptkbd_data->middlebutton_state = 0;
+			return 1;
 		}
-		return 1;
 	}
 
 	if (usage->type == EV_KEY && usage->code == KEY_FN_ESC && value == 1) {
@@ -1146,6 +1185,7 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
 	cptkbd_data->middlebutton_state = 0;
 	cptkbd_data->fn_lock = true;
 	cptkbd_data->sensitivity = 0x05;
+	cptkbd_data->middleclick_workaround_cptkbd = true;
 	lenovo_features_set_cptkbd(hdev);
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_cptkbd);
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd
  2023-09-18 14:50 [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd Mikhail Khvainitski
@ 2023-09-23 22:58 ` Mikhail Khvainitski
  2023-09-23 22:58   ` [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround Mikhail Khvainitski
  0 siblings, 1 reply; 16+ messages in thread
From: Mikhail Khvainitski @ 2023-09-23 22:58 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, linux-kernel, Mikhail Khvainitski


So, while I was waiting for the review of the patch, I've come up with
much better idea: we can just autodetect if stock and buggy or
patched and bug-free firmware is used and have no need to introduce
additional sysfs knob. So the patch which implements this follows in
the next message.

While the patch is ready to be merged as is (of course, if there are
no comments from reviewers), I thought about the following: may be we
should add a message to dmesg upon detecting stock and buggy firmware
that user should consider patching the firmware? If so, what is the
best place to put an instruction to? Post a link to dmesg? Can the
link point directly to Dennis' blog or its better to put it somewhere
else (with credits, of course)?

Thanks.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround
  2023-09-23 22:58 ` Mikhail Khvainitski
@ 2023-09-23 22:58   ` Mikhail Khvainitski
  2023-10-04 19:02     ` Jiri Kosina
  2023-12-09 12:50     ` Yauhen Kharuzhy
  0 siblings, 2 replies; 16+ messages in thread
From: Mikhail Khvainitski @ 2023-09-23 22:58 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, linux-kernel, Mikhail Khvainitski

Built-in firmware of cptkbd handles scrolling by itself (when middle
button is pressed) but with issues: it does not support horizontal and
hi-res scrolling and upon middle button release it sends middle button
click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
lenovo: Hide middle-button press until release") workarounds last
issue but it's impossible to workaround scrolling-related issues
without firmware modification.

Likely, Dennis Schneider has reverse engineered the firmware and
provided an instruction on how to patch it [1]. However,
aforementioned workaround prevents userspace (libinput) from knowing
exact moment when middle button has been pressed down and performing
"On-Button scrolling". This commit detects correctly-behaving patched
firmware if cursor movement events has been received during middle
button being pressed and stops applying workaround for this device.

Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]

Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>
---
 drivers/hid/hid-lenovo.c | 68 ++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..9c1181313e44 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -51,7 +51,12 @@ struct lenovo_drvdata {
 	int select_right;
 	int sensitivity;
 	int press_speed;
-	u8 middlebutton_state; /* 0:Up, 1:Down (undecided), 2:Scrolling */
+	/* 0: Up
+	 * 1: Down (undecided)
+	 * 2: Scrolling
+	 * 3: Patched firmware, disable workaround
+	 */
+	u8 middlebutton_state;
 	bool fn_lock;
 };
 
@@ -668,31 +673,48 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
 {
 	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
 
-	/* "wheel" scroll events */
-	if (usage->type == EV_REL && (usage->code == REL_WHEEL ||
-			usage->code == REL_HWHEEL)) {
-		/* Scroll events disable middle-click event */
-		cptkbd_data->middlebutton_state = 2;
-		return 0;
-	}
+	if (cptkbd_data->middlebutton_state != 3) {
+		/* REL_X and REL_Y events during middle button pressed
+		 * are only possible on patched, bug-free firmware
+		 * so set middlebutton_state to 3
+		 * to never apply workaround anymore
+		 */
+		if (cptkbd_data->middlebutton_state == 1 &&
+				usage->type == EV_REL &&
+				(usage->code == REL_X || usage->code == REL_Y)) {
+			cptkbd_data->middlebutton_state = 3;
+			/* send middle button press which was hold before */
+			input_event(field->hidinput->input,
+				EV_KEY, BTN_MIDDLE, 1);
+			input_sync(field->hidinput->input);
+		}
 
-	/* Middle click events */
-	if (usage->type == EV_KEY && usage->code == BTN_MIDDLE) {
-		if (value == 1) {
-			cptkbd_data->middlebutton_state = 1;
-		} else if (value == 0) {
-			if (cptkbd_data->middlebutton_state == 1) {
-				/* No scrolling inbetween, send middle-click */
-				input_event(field->hidinput->input,
-					EV_KEY, BTN_MIDDLE, 1);
-				input_sync(field->hidinput->input);
-				input_event(field->hidinput->input,
-					EV_KEY, BTN_MIDDLE, 0);
-				input_sync(field->hidinput->input);
+		/* "wheel" scroll events */
+		if (usage->type == EV_REL && (usage->code == REL_WHEEL ||
+				usage->code == REL_HWHEEL)) {
+			/* Scroll events disable middle-click event */
+			cptkbd_data->middlebutton_state = 2;
+			return 0;
+		}
+
+		/* Middle click events */
+		if (usage->type == EV_KEY && usage->code == BTN_MIDDLE) {
+			if (value == 1) {
+				cptkbd_data->middlebutton_state = 1;
+			} else if (value == 0) {
+				if (cptkbd_data->middlebutton_state == 1) {
+					/* No scrolling inbetween, send middle-click */
+					input_event(field->hidinput->input,
+						EV_KEY, BTN_MIDDLE, 1);
+					input_sync(field->hidinput->input);
+					input_event(field->hidinput->input,
+						EV_KEY, BTN_MIDDLE, 0);
+					input_sync(field->hidinput->input);
+				}
+				cptkbd_data->middlebutton_state = 0;
 			}
-			cptkbd_data->middlebutton_state = 0;
+			return 1;
 		}
-		return 1;
 	}
 
 	if (usage->type == EV_KEY && usage->code == KEY_FN_ESC && value == 1) {
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround
  2023-09-23 22:58   ` [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround Mikhail Khvainitski
@ 2023-10-04 19:02     ` Jiri Kosina
  2023-12-09 12:50     ` Yauhen Kharuzhy
  1 sibling, 0 replies; 16+ messages in thread
From: Jiri Kosina @ 2023-10-04 19:02 UTC (permalink / raw)
  To: Mikhail Khvainitski; +Cc: Benjamin Tissoires, linux-input, linux-kernel

On Sun, 24 Sep 2023, Mikhail Khvainitski wrote:

> Built-in firmware of cptkbd handles scrolling by itself (when middle
> button is pressed) but with issues: it does not support horizontal and
> hi-res scrolling and upon middle button release it sends middle button
> click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
> lenovo: Hide middle-button press until release") workarounds last
> issue but it's impossible to workaround scrolling-related issues
> without firmware modification.
> 
> Likely, Dennis Schneider has reverse engineered the firmware and
> provided an instruction on how to patch it [1]. However,
> aforementioned workaround prevents userspace (libinput) from knowing
> exact moment when middle button has been pressed down and performing
> "On-Button scrolling". This commit detects correctly-behaving patched
> firmware if cursor movement events has been received during middle
> button being pressed and stops applying workaround for this device.
> 
> Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]
> 
> Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>

Applied to for-6.7/lenovo, thanks.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround
  2023-09-23 22:58   ` [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround Mikhail Khvainitski
  2023-10-04 19:02     ` Jiri Kosina
@ 2023-12-09 12:50     ` Yauhen Kharuzhy
  2023-12-09 16:56       ` Yauhen Kharuzhy
  1 sibling, 1 reply; 16+ messages in thread
From: Yauhen Kharuzhy @ 2023-12-09 12:50 UTC (permalink / raw)
  To: Mikhail Khvainitski
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel, ValdikSS

On Sun, Sep 24, 2023 at 01:58:30AM +0300, Mikhail Khvainitski wrote:
> Built-in firmware of cptkbd handles scrolling by itself (when middle
> button is pressed) but with issues: it does not support horizontal and
> hi-res scrolling and upon middle button release it sends middle button
> click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
> lenovo: Hide middle-button press until release") workarounds last
> issue but it's impossible to workaround scrolling-related issues
> without firmware modification.
> 
> Likely, Dennis Schneider has reverse engineered the firmware and
> provided an instruction on how to patch it [1]. However,
> aforementioned workaround prevents userspace (libinput) from knowing
> exact moment when middle button has been pressed down and performing
> "On-Button scrolling". This commit detects correctly-behaving patched
> firmware if cursor movement events has been received during middle
> button being pressed and stops applying workaround for this device.
> 
> Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]

This patch breaks a scrolling at my ThinkPad TrackPoint Keyboard II: it
starts to report middle-button push/release events with scrolling events
between. A support for this keyboard was added in
24401f291dcc4f2c18b9e2f65763cbaadc7a1528 "HID: lenovo: Add support for
ThinkPad TrackPoint Keyboard II" commit.

There is an evtest output below:

Without of commit:

Middle-button click:
Event: time 1702122290.593300, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
Event: time 1702122290.593300, -------------- SYN_REPORT ------------
Event: time 1702122290.593312, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
Event: time 1702122290.593312, -------------- SYN_REPORT ------------

Vertical scrolling:
Event: time 1702122300.441627, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
Event: time 1702122300.441627, -------------- SYN_REPORT ------------
Event: time 1702122300.565663, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
Event: time 1702122300.565663, -------------- SYN_REPORT ------------

Horizontal scrolling:
Event: time 1702122307.845969, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
Event: time 1702122307.845969, -------------- SYN_REPORT ------------
Event: time 1702122307.981954, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
Event: time 1702122307.981954, -------------- SYN_REPORT ------------



After commit:

Middle-button click:
Event: time 1702125091.290045, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702125091.290045, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
Event: time 1702125091.290045, -------------- SYN_REPORT ------------
Event: time 1702125092.626118, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702125092.626118, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
Event: time 1702125092.626118, -------------- SYN_REPORT ------------


Vscroll:
Event: time 1702125286.653639, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702125286.653639, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
Event: time 1702125286.653639, -------------- SYN_REPORT ------------
Event: time 1702125287.929689, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
Event: time 1702125287.929689, -------------- SYN_REPORT ------------
Event: time 1702125288.037688, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
Event: time 1702125288.037688, -------------- SYN_REPORT ------------
Event: time 1702125290.481787, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702125290.481787, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
Event: time 1702125290.481787, -------------- SYN_REPORT ------------

Hscroll:
Event: time 1702125293.841920, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702125293.841920, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
Event: time 1702125293.841920, -------------- SYN_REPORT ------------
Event: time 1702125294.761952, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
Event: time 1702125294.761952, -------------- SYN_REPORT ------------
Event: time 1702125294.893967, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
Event: time 1702125294.893967, -------------- SYN_REPORT ------------
Event: time 1702125296.134006, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702125296.134006, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
Event: time 1702125296.134006, -------------- SYN_REPORT ------------

-- 
Yauhen Kharuzhy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround
  2023-12-09 12:50     ` Yauhen Kharuzhy
@ 2023-12-09 16:56       ` Yauhen Kharuzhy
  2023-12-09 18:21         ` Yauhen Kharuzhy
  0 siblings, 1 reply; 16+ messages in thread
From: Yauhen Kharuzhy @ 2023-12-09 16:56 UTC (permalink / raw)
  To: Mikhail Khvainitski
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel, ValdikSS

On Sat, Dec 09, 2023 at 02:50:16PM +0200, Yauhen Kharuzhy wrote:
> On Sun, Sep 24, 2023 at 01:58:30AM +0300, Mikhail Khvainitski wrote:
> > Built-in firmware of cptkbd handles scrolling by itself (when middle
> > button is pressed) but with issues: it does not support horizontal and
> > hi-res scrolling and upon middle button release it sends middle button
> > click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
> > lenovo: Hide middle-button press until release") workarounds last
> > issue but it's impossible to workaround scrolling-related issues
> > without firmware modification.
> > 
> > Likely, Dennis Schneider has reverse engineered the firmware and
> > provided an instruction on how to patch it [1]. However,
> > aforementioned workaround prevents userspace (libinput) from knowing
> > exact moment when middle button has been pressed down and performing
> > "On-Button scrolling". This commit detects correctly-behaving patched
> > firmware if cursor movement events has been received during middle
> > button being pressed and stops applying workaround for this device.
> > 
> > Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]
> 
> This patch breaks a scrolling at my ThinkPad TrackPoint Keyboard II: it
> starts to report middle-button push/release events with scrolling events
> between. A support for this keyboard was added in
> 24401f291dcc4f2c18b9e2f65763cbaadc7a1528 "HID: lenovo: Add support for
> ThinkPad TrackPoint Keyboard II" commit.

I figured this out.

This keyboard can emit REL_Y/REL_X events between of middle-button
events (if user was moving a cursor and press middle button without of
stopping this), so this algorithm does a false-positive detection and switches
the workaround off like for patched firmware:

Event: time 1702140625.854777, type 2 (EV_REL), code 1 (REL_Y), value 2
Event: time 1702140625.854777, -------------- SYN_REPORT ------------
Event: time 1702140625.870769, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
Event: time 1702140625.870769, -------------- SYN_REPORT ------------
Event: time 1702140625.870771, type 2 (EV_REL), code 1 (REL_Y), value 2
Event: time 1702140625.870771, -------------- SYN_REPORT ------------
Event: time 1702140625.970780, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
Event: time 1702140625.970780, -------------- SYN_REPORT ------------
Event: time 1702140626.058800, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
Event: time 1702140626.058800, -------------- SYN_REPORT ------------
Event: time 1702140630.462974, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
Event: time 1702140630.462974, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
Event: time 1702140630.462974, -------------- SYN_REPORT ------------


> 
> There is an evtest output below:
> 
> Without of commit:
> 
> Middle-button click:
> Event: time 1702122290.593300, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> Event: time 1702122290.593300, -------------- SYN_REPORT ------------
> Event: time 1702122290.593312, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> Event: time 1702122290.593312, -------------- SYN_REPORT ------------
> 
> Vertical scrolling:
> Event: time 1702122300.441627, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> Event: time 1702122300.441627, -------------- SYN_REPORT ------------
> Event: time 1702122300.565663, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> Event: time 1702122300.565663, -------------- SYN_REPORT ------------
> 
> Horizontal scrolling:
> Event: time 1702122307.845969, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> Event: time 1702122307.845969, -------------- SYN_REPORT ------------
> Event: time 1702122307.981954, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> Event: time 1702122307.981954, -------------- SYN_REPORT ------------
> 
> 
> 
> After commit:
> 
> Middle-button click:
> Event: time 1702125091.290045, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702125091.290045, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> Event: time 1702125091.290045, -------------- SYN_REPORT ------------
> Event: time 1702125092.626118, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702125092.626118, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> Event: time 1702125092.626118, -------------- SYN_REPORT ------------
> 
> 
> Vscroll:
> Event: time 1702125286.653639, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702125286.653639, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> Event: time 1702125286.653639, -------------- SYN_REPORT ------------
> Event: time 1702125287.929689, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> Event: time 1702125287.929689, -------------- SYN_REPORT ------------
> Event: time 1702125288.037688, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> Event: time 1702125288.037688, -------------- SYN_REPORT ------------
> Event: time 1702125290.481787, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702125290.481787, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> Event: time 1702125290.481787, -------------- SYN_REPORT ------------
> 
> Hscroll:
> Event: time 1702125293.841920, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702125293.841920, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> Event: time 1702125293.841920, -------------- SYN_REPORT ------------
> Event: time 1702125294.761952, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> Event: time 1702125294.761952, -------------- SYN_REPORT ------------
> Event: time 1702125294.893967, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> Event: time 1702125294.893967, -------------- SYN_REPORT ------------
> Event: time 1702125296.134006, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702125296.134006, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> Event: time 1702125296.134006, -------------- SYN_REPORT ------------

-- 
Yauhen Kharuzhy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround
  2023-12-09 16:56       ` Yauhen Kharuzhy
@ 2023-12-09 18:21         ` Yauhen Kharuzhy
  2023-12-11 16:17           ` Mikhail Khvoinitsky
  0 siblings, 1 reply; 16+ messages in thread
From: Yauhen Kharuzhy @ 2023-12-09 18:21 UTC (permalink / raw)
  To: Mikhail Khvainitski
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel, ValdikSS

On Sat, Dec 09, 2023 at 06:56:48PM +0200, Yauhen Kharuzhy wrote:
> On Sat, Dec 09, 2023 at 02:50:16PM +0200, Yauhen Kharuzhy wrote:
> > On Sun, Sep 24, 2023 at 01:58:30AM +0300, Mikhail Khvainitski wrote:
> > > Built-in firmware of cptkbd handles scrolling by itself (when middle
> > > button is pressed) but with issues: it does not support horizontal and
> > > hi-res scrolling and upon middle button release it sends middle button
> > > click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
> > > lenovo: Hide middle-button press until release") workarounds last
> > > issue but it's impossible to workaround scrolling-related issues
> > > without firmware modification.
> > > 
> > > Likely, Dennis Schneider has reverse engineered the firmware and
> > > provided an instruction on how to patch it [1]. However,
> > > aforementioned workaround prevents userspace (libinput) from knowing
> > > exact moment when middle button has been pressed down and performing
> > > "On-Button scrolling". This commit detects correctly-behaving patched
> > > firmware if cursor movement events has been received during middle
> > > button being pressed and stops applying workaround for this device.
> > > 
> > > Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]
> > 
> > This patch breaks a scrolling at my ThinkPad TrackPoint Keyboard II: it
> > starts to report middle-button push/release events with scrolling events
> > between. A support for this keyboard was added in
> > 24401f291dcc4f2c18b9e2f65763cbaadc7a1528 "HID: lenovo: Add support for
> > ThinkPad TrackPoint Keyboard II" commit.
> 
> I figured this out.
> 
> This keyboard can emit REL_Y/REL_X events between of middle-button
> events (if user was moving a cursor and press middle button without of
> stopping this), so this algorithm does a false-positive detection and switches
> the workaround off like for patched firmware:
> 
> Event: time 1702140625.854777, type 2 (EV_REL), code 1 (REL_Y), value 2
> Event: time 1702140625.854777, -------------- SYN_REPORT ------------
> Event: time 1702140625.870769, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> Event: time 1702140625.870769, -------------- SYN_REPORT ------------
> Event: time 1702140625.870771, type 2 (EV_REL), code 1 (REL_Y), value 2
> Event: time 1702140625.870771, -------------- SYN_REPORT ------------
> Event: time 1702140625.970780, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> Event: time 1702140625.970780, -------------- SYN_REPORT ------------
> Event: time 1702140626.058800, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> Event: time 1702140626.058800, -------------- SYN_REPORT ------------
> Event: time 1702140630.462974, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> Event: time 1702140630.462974, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> Event: time 1702140630.462974, -------------- SYN_REPORT ------------

Maybe we should map the wheel HID reports to REL_Y/REL_X in
lenovo_input_mapping_tpIIkbd() to allow libinput to do its wheel emulation job?
I tried this but I am not familiar with HID drivers and had no success.


> 
> 
> > 
> > There is an evtest output below:
> > 
> > Without of commit:
> > 
> > Middle-button click:
> > Event: time 1702122290.593300, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > Event: time 1702122290.593300, -------------- SYN_REPORT ------------
> > Event: time 1702122290.593312, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > Event: time 1702122290.593312, -------------- SYN_REPORT ------------
> > 
> > Vertical scrolling:
> > Event: time 1702122300.441627, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > Event: time 1702122300.441627, -------------- SYN_REPORT ------------
> > Event: time 1702122300.565663, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > Event: time 1702122300.565663, -------------- SYN_REPORT ------------
> > 
> > Horizontal scrolling:
> > Event: time 1702122307.845969, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > Event: time 1702122307.845969, -------------- SYN_REPORT ------------
> > Event: time 1702122307.981954, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > Event: time 1702122307.981954, -------------- SYN_REPORT ------------
> > 
> > 
> > 
> > After commit:
> > 
> > Middle-button click:
> > Event: time 1702125091.290045, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702125091.290045, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > Event: time 1702125091.290045, -------------- SYN_REPORT ------------
> > Event: time 1702125092.626118, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702125092.626118, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > Event: time 1702125092.626118, -------------- SYN_REPORT ------------
> > 
> > 
> > Vscroll:
> > Event: time 1702125286.653639, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702125286.653639, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > Event: time 1702125286.653639, -------------- SYN_REPORT ------------
> > Event: time 1702125287.929689, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > Event: time 1702125287.929689, -------------- SYN_REPORT ------------
> > Event: time 1702125288.037688, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > Event: time 1702125288.037688, -------------- SYN_REPORT ------------
> > Event: time 1702125290.481787, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702125290.481787, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > Event: time 1702125290.481787, -------------- SYN_REPORT ------------
> > 
> > Hscroll:
> > Event: time 1702125293.841920, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702125293.841920, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > Event: time 1702125293.841920, -------------- SYN_REPORT ------------
> > Event: time 1702125294.761952, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > Event: time 1702125294.761952, -------------- SYN_REPORT ------------
> > Event: time 1702125294.893967, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > Event: time 1702125294.893967, -------------- SYN_REPORT ------------
> > Event: time 1702125296.134006, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702125296.134006, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > Event: time 1702125296.134006, -------------- SYN_REPORT ------------
> 
> -- 
> Yauhen Kharuzhy

-- 
Yauhen Kharuzhy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround
  2023-12-09 18:21         ` Yauhen Kharuzhy
@ 2023-12-11 16:17           ` Mikhail Khvoinitsky
  2023-12-12 13:31             ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Mikhail Khvainitski
  0 siblings, 1 reply; 16+ messages in thread
From: Mikhail Khvoinitsky @ 2023-12-11 16:17 UTC (permalink / raw)
  To: Yauhen Kharuzhy
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel, ValdikSS

Hello.

Well, that's unfortunate. As far as I can see, yes, TrackPoint
Keyboard II behaves similarly to 1st gen Trackpoint Keyboard (a.k.a.
Compact) but not quite. The best way would be to make the keyboard not
emulate scrolling at all but not sure if its possible. For cptkbd it
required patching firmware, it might be also the case for 2nd gen.

> Maybe we should map the wheel HID reports to REL_Y/REL_X in
> lenovo_input_mapping_tpIIkbd() to allow libinput to do its wheel emulation job?

It might work but since emulated wheel events aren't continuous (at
least this is the case with cptkbd), it doesn't make much sense in
comparison with the original implementation — just ignore the middle
button if scrolling.

I'll try to get the keyboard and check, but at the moment it seems
that the best thing to do is before stopping applying a workaround,
check that it's a Trackpoint Compact keyboard, not anything else. If I
won't be able to find a better solution shortly, I'll make a patch
which makes sure that it's only cptkbd before disabling the
workaround.

On Sat, 9 Dec 2023 at 20:21, Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>
> On Sat, Dec 09, 2023 at 06:56:48PM +0200, Yauhen Kharuzhy wrote:
> > On Sat, Dec 09, 2023 at 02:50:16PM +0200, Yauhen Kharuzhy wrote:
> > > On Sun, Sep 24, 2023 at 01:58:30AM +0300, Mikhail Khvainitski wrote:
> > > > Built-in firmware of cptkbd handles scrolling by itself (when middle
> > > > button is pressed) but with issues: it does not support horizontal and
> > > > hi-res scrolling and upon middle button release it sends middle button
> > > > click even if there was a scrolling event. Commit 3cb5ff0220e3 ("HID:
> > > > lenovo: Hide middle-button press until release") workarounds last
> > > > issue but it's impossible to workaround scrolling-related issues
> > > > without firmware modification.
> > > >
> > > > Likely, Dennis Schneider has reverse engineered the firmware and
> > > > provided an instruction on how to patch it [1]. However,
> > > > aforementioned workaround prevents userspace (libinput) from knowing
> > > > exact moment when middle button has been pressed down and performing
> > > > "On-Button scrolling". This commit detects correctly-behaving patched
> > > > firmware if cursor movement events has been received during middle
> > > > button being pressed and stops applying workaround for this device.
> > > >
> > > > Link: https://hohlerde.org/rauch/en/elektronik/projekte/tpkbd-fix/ [1]
> > >
> > > This patch breaks a scrolling at my ThinkPad TrackPoint Keyboard II: it
> > > starts to report middle-button push/release events with scrolling events
> > > between. A support for this keyboard was added in
> > > 24401f291dcc4f2c18b9e2f65763cbaadc7a1528 "HID: lenovo: Add support for
> > > ThinkPad TrackPoint Keyboard II" commit.
> >
> > I figured this out.
> >
> > This keyboard can emit REL_Y/REL_X events between of middle-button
> > events (if user was moving a cursor and press middle button without of
> > stopping this), so this algorithm does a false-positive detection and switches
> > the workaround off like for patched firmware:
> >
> > Event: time 1702140625.854777, type 2 (EV_REL), code 1 (REL_Y), value 2
> > Event: time 1702140625.854777, -------------- SYN_REPORT ------------
> > Event: time 1702140625.870769, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > Event: time 1702140625.870769, -------------- SYN_REPORT ------------
> > Event: time 1702140625.870771, type 2 (EV_REL), code 1 (REL_Y), value 2
> > Event: time 1702140625.870771, -------------- SYN_REPORT ------------
> > Event: time 1702140625.970780, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > Event: time 1702140625.970780, -------------- SYN_REPORT ------------
> > Event: time 1702140626.058800, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > Event: time 1702140626.058800, -------------- SYN_REPORT ------------
> > Event: time 1702140630.462974, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > Event: time 1702140630.462974, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > Event: time 1702140630.462974, -------------- SYN_REPORT ------------
>
> Maybe we should map the wheel HID reports to REL_Y/REL_X in
> lenovo_input_mapping_tpIIkbd() to allow libinput to do its wheel emulation job?
> I tried this but I am not familiar with HID drivers and had no success.
>
>
> >
> >
> > >
> > > There is an evtest output below:
> > >
> > > Without of commit:
> > >
> > > Middle-button click:
> > > Event: time 1702122290.593300, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > > Event: time 1702122290.593300, -------------- SYN_REPORT ------------
> > > Event: time 1702122290.593312, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > > Event: time 1702122290.593312, -------------- SYN_REPORT ------------
> > >
> > > Vertical scrolling:
> > > Event: time 1702122300.441627, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > > Event: time 1702122300.441627, -------------- SYN_REPORT ------------
> > > Event: time 1702122300.565663, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > > Event: time 1702122300.565663, -------------- SYN_REPORT ------------
> > >
> > > Horizontal scrolling:
> > > Event: time 1702122307.845969, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > > Event: time 1702122307.845969, -------------- SYN_REPORT ------------
> > > Event: time 1702122307.981954, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > > Event: time 1702122307.981954, -------------- SYN_REPORT ------------
> > >
> > >
> > >
> > > After commit:
> > >
> > > Middle-button click:
> > > Event: time 1702125091.290045, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > > Event: time 1702125091.290045, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > > Event: time 1702125091.290045, -------------- SYN_REPORT ------------
> > > Event: time 1702125092.626118, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > > Event: time 1702125092.626118, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > > Event: time 1702125092.626118, -------------- SYN_REPORT ------------
> > >
> > >
> > > Vscroll:
> > > Event: time 1702125286.653639, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > > Event: time 1702125286.653639, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > > Event: time 1702125286.653639, -------------- SYN_REPORT ------------
> > > Event: time 1702125287.929689, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > > Event: time 1702125287.929689, -------------- SYN_REPORT ------------
> > > Event: time 1702125288.037688, type 2 (EV_REL), code 8 (REL_WHEEL), value -1
> > > Event: time 1702125288.037688, -------------- SYN_REPORT ------------
> > > Event: time 1702125290.481787, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > > Event: time 1702125290.481787, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > > Event: time 1702125290.481787, -------------- SYN_REPORT ------------
> > >
> > > Hscroll:
> > > Event: time 1702125293.841920, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > > Event: time 1702125293.841920, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 1
> > > Event: time 1702125293.841920, -------------- SYN_REPORT ------------
> > > Event: time 1702125294.761952, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > > Event: time 1702125294.761952, -------------- SYN_REPORT ------------
> > > Event: time 1702125294.893967, type 2 (EV_REL), code 6 (REL_HWHEEL), value -1
> > > Event: time 1702125294.893967, -------------- SYN_REPORT ------------
> > > Event: time 1702125296.134006, type 4 (EV_MSC), code 4 (MSC_SCAN), value ffa000fb
> > > Event: time 1702125296.134006, type 1 (EV_KEY), code 274 (BTN_MIDDLE), value 0
> > > Event: time 1702125296.134006, -------------- SYN_REPORT ------------
> >
> > --
> > Yauhen Kharuzhy
>
> --
> Yauhen Kharuzhy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd
  2023-12-11 16:17           ` Mikhail Khvoinitsky
@ 2023-12-12 13:31             ` Mikhail Khvainitski
  2023-12-12 13:45               ` Jiri Kosina
  0 siblings, 1 reply; 16+ messages in thread
From: Mikhail Khvainitski @ 2023-12-12 13:31 UTC (permalink / raw)
  To: jikos, benjamin.tissoires; +Cc: me, iam, jekhor, linux-input, linux-kernel

Commit 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw on cptkbd and
stop applying workaround") introduced a regression for ThinkPad
TrackPoint Keyboard II which has similar quirks to cptkbd (so it uses
the same workarounds) but slightly different so that there are
false-positives during detecting well-behaving firmware. This commit
restricts detecting well-behaving firmware to the only model which
known to have one and have stable enough quirks to not cause
false-positives.

Fixes: 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround")
Link: https://lore.kernel.org/linux-input/ZXRiiPsBKNasioqH@jekhomev/
Link: https://bbs.archlinux.org/viewtopic.php?pid=2135468#p2135468
Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>
Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/hid/hid-lenovo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 7c1b33be9d13..149a3c74346b 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -692,7 +692,8 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
 		 * so set middlebutton_state to 3
 		 * to never apply workaround anymore
 		 */
-		if (cptkbd_data->middlebutton_state == 1 &&
+		if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD &&
+				cptkbd_data->middlebutton_state == 1 &&
 				usage->type == EV_REL &&
 				(usage->code == REL_X || usage->code == REL_Y)) {
 			cptkbd_data->middlebutton_state = 3;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd
  2023-12-12 13:31             ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Mikhail Khvainitski
@ 2023-12-12 13:45               ` Jiri Kosina
  2023-12-22 15:32                 ` Uli v. d. Ohe
  0 siblings, 1 reply; 16+ messages in thread
From: Jiri Kosina @ 2023-12-12 13:45 UTC (permalink / raw)
  To: Mikhail Khvainitski
  Cc: benjamin.tissoires, iam, jekhor, linux-input, linux-kernel

On Tue, 12 Dec 2023, Mikhail Khvainitski wrote:

> Commit 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw on cptkbd and
> stop applying workaround") introduced a regression for ThinkPad
> TrackPoint Keyboard II which has similar quirks to cptkbd (so it uses
> the same workarounds) but slightly different so that there are
> false-positives during detecting well-behaving firmware. This commit
> restricts detecting well-behaving firmware to the only model which
> known to have one and have stable enough quirks to not cause
> false-positives.
> 
> Fixes: 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround")
> Link: https://lore.kernel.org/linux-input/ZXRiiPsBKNasioqH@jekhomev/
> Link: https://bbs.archlinux.org/viewtopic.php?pid=2135468#p2135468
> Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>
> Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>  drivers/hid/hid-lenovo.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 7c1b33be9d13..149a3c74346b 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -692,7 +692,8 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
>  		 * so set middlebutton_state to 3
>  		 * to never apply workaround anymore
>  		 */
> -		if (cptkbd_data->middlebutton_state == 1 &&
> +		if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD &&
> +				cptkbd_data->middlebutton_state == 1 &&
>  				usage->type == EV_REL &&
>  				(usage->code == REL_X || usage->code == REL_Y)) {
>  			cptkbd_data->middlebutton_state = 3;

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd
  2023-12-12 13:45               ` Jiri Kosina
@ 2023-12-22 15:32                 ` Uli v. d. Ohe
  2023-12-23 18:04                   ` Mikhail Khvoinitsky
  0 siblings, 1 reply; 16+ messages in thread
From: Uli v. d. Ohe @ 2023-12-22 15:32 UTC (permalink / raw)
  To: jkosina; +Cc: benjamin.tissoires, iam, jekhor, linux-input, linux-kernel, me

I get buggy middle button scrolling behavior on my USB Compact Keyboard 
(i.e., "1st gen") with original, unmodified firmware and the patch (of 
Sep. 23).

Sometimes the keyboard sends REL_X events while the middle button is 
pressed. Thus the old "workaround" is disabled and middle button 
scrolling henceforth exhibits the known buggy behavior.

Explicitly, I can confirm that the following values occur, leading to 
erroneous disabling of the workaround:

cptkbd_data->middlebutton_state == 1
usage->type == 2 [i.e., EV_REL]
usage->code == 0 [i.e., REL_X]

The keyboard is identified by lsusb as:
ID 17ef:6047 Lenovo ThinkPad Compact Keyboard with TrackPoint

This was tested with kernel 6.1.67 which contains the backported patch 
(commit 6e2076cad8873cc2a9f96e4becab35425c3656dc).

I didn't test the latest patch of Dec. 12. However, I don't expect it to 
fix my issue as the only added condition
hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
should be satisfied, which I understand is also the intention. 
(USB_DEVICE_ID_LENOVO_CUSBKBD == 0x6047, which is the device ID of my 
keyboard as reported by lsusb.)

Best regards,
Uli

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd
  2023-12-22 15:32                 ` Uli v. d. Ohe
@ 2023-12-23 18:04                   ` Mikhail Khvoinitsky
  2023-12-23 19:12                     ` [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd Mikhail Khvainitski
  2023-12-24 15:51                     ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Uli v. d. Ohe
  0 siblings, 2 replies; 16+ messages in thread
From: Mikhail Khvoinitsky @ 2023-12-23 18:04 UTC (permalink / raw)
  To: Uli v. d. Ohe
  Cc: jkosina, benjamin.tissoires, iam, jekhor, linux-input, linux-kernel

Hello.

Thank you for the report, sorry about that. I've received one more
report of the same issue by email.
So this means that the only reliable way is to add a sysfs parameter.
I'll send a patch.


On Fri, 22 Dec 2023 at 17:32, Uli v. d. Ohe <u-v@mailbox.org> wrote:
>
> I get buggy middle button scrolling behavior on my USB Compact Keyboard
> (i.e., "1st gen") with original, unmodified firmware and the patch (of
> Sep. 23).
>
> Sometimes the keyboard sends REL_X events while the middle button is
> pressed. Thus the old "workaround" is disabled and middle button
> scrolling henceforth exhibits the known buggy behavior.
>
> Explicitly, I can confirm that the following values occur, leading to
> erroneous disabling of the workaround:
>
> cptkbd_data->middlebutton_state == 1
> usage->type == 2 [i.e., EV_REL]
> usage->code == 0 [i.e., REL_X]
>
> The keyboard is identified by lsusb as:
> ID 17ef:6047 Lenovo ThinkPad Compact Keyboard with TrackPoint
>
> This was tested with kernel 6.1.67 which contains the backported patch
> (commit 6e2076cad8873cc2a9f96e4becab35425c3656dc).
>
> I didn't test the latest patch of Dec. 12. However, I don't expect it to
> fix my issue as the only added condition
> hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
> should be satisfied, which I understand is also the intention.
> (USB_DEVICE_ID_LENOVO_CUSBKBD == 0x6047, which is the device ID of my
> keyboard as reported by lsusb.)
>
> Best regards,
> Uli

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd
  2023-12-23 18:04                   ` Mikhail Khvoinitsky
@ 2023-12-23 19:12                     ` Mikhail Khvainitski
  2024-01-23 10:55                       ` Jiri Kosina
  2023-12-24 15:51                     ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Uli v. d. Ohe
  1 sibling, 1 reply; 16+ messages in thread
From: Mikhail Khvainitski @ 2023-12-23 19:12 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, linux-kernel, Mikhail Khvainitski, iam, jekhor, u-v

Previous attempt to autodetect well-behaving patched firmware
introduced in commit 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw
on cptkbd and stop applying workaround") has shown that there are
false-positives on original firmware (on both 1st gen and 2nd gen
keyboards) which causes the middle button click workaround to be
mistakenly disabled.

This commit adds explicit parameter to sysfs to control this
workaround.

Fixes: 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround")
Fixes: 43527a0094c1 ("HID: lenovo: Restrict detection of patched firmware only to USB cptkbd")
Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>
---
 drivers/hid/hid-lenovo.c | 57 +++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 149a3c74346b..f86c1ea83a03 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -54,10 +54,10 @@ struct lenovo_drvdata {
 	/* 0: Up
 	 * 1: Down (undecided)
 	 * 2: Scrolling
-	 * 3: Patched firmware, disable workaround
 	 */
 	u8 middlebutton_state;
 	bool fn_lock;
+	bool middleclick_workaround_cptkbd;
 };
 
 #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
@@ -621,6 +621,36 @@ static ssize_t attr_sensitivity_store_cptkbd(struct device *dev,
 	return count;
 }
 
+static ssize_t attr_middleclick_workaround_show_cptkbd(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		cptkbd_data->middleclick_workaround_cptkbd);
+}
+
+static ssize_t attr_middleclick_workaround_store_cptkbd(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf,
+		size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+	int value;
+
+	if (kstrtoint(buf, 10, &value))
+		return -EINVAL;
+	if (value < 0 || value > 1)
+		return -EINVAL;
+
+	cptkbd_data->middleclick_workaround_cptkbd = !!value;
+
+	return count;
+}
+
 
 static struct device_attribute dev_attr_fn_lock =
 	__ATTR(fn_lock, S_IWUSR | S_IRUGO,
@@ -632,10 +662,16 @@ static struct device_attribute dev_attr_sensitivity_cptkbd =
 			attr_sensitivity_show_cptkbd,
 			attr_sensitivity_store_cptkbd);
 
+static struct device_attribute dev_attr_middleclick_workaround_cptkbd =
+	__ATTR(middleclick_workaround, S_IWUSR | S_IRUGO,
+			attr_middleclick_workaround_show_cptkbd,
+			attr_middleclick_workaround_store_cptkbd);
+
 
 static struct attribute *lenovo_attributes_cptkbd[] = {
 	&dev_attr_fn_lock.attr,
 	&dev_attr_sensitivity_cptkbd.attr,
+	&dev_attr_middleclick_workaround_cptkbd.attr,
 	NULL
 };
 
@@ -686,23 +722,7 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
 {
 	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
 
-	if (cptkbd_data->middlebutton_state != 3) {
-		/* REL_X and REL_Y events during middle button pressed
-		 * are only possible on patched, bug-free firmware
-		 * so set middlebutton_state to 3
-		 * to never apply workaround anymore
-		 */
-		if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD &&
-				cptkbd_data->middlebutton_state == 1 &&
-				usage->type == EV_REL &&
-				(usage->code == REL_X || usage->code == REL_Y)) {
-			cptkbd_data->middlebutton_state = 3;
-			/* send middle button press which was hold before */
-			input_event(field->hidinput->input,
-				EV_KEY, BTN_MIDDLE, 1);
-			input_sync(field->hidinput->input);
-		}
-
+	if (cptkbd_data->middleclick_workaround_cptkbd) {
 		/* "wheel" scroll events */
 		if (usage->type == EV_REL && (usage->code == REL_WHEEL ||
 				usage->code == REL_HWHEEL)) {
@@ -1166,6 +1186,7 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
 	cptkbd_data->middlebutton_state = 0;
 	cptkbd_data->fn_lock = true;
 	cptkbd_data->sensitivity = 0x05;
+	cptkbd_data->middleclick_workaround_cptkbd = true;
 	lenovo_features_set_cptkbd(hdev);
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_cptkbd);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd
  2023-12-23 18:04                   ` Mikhail Khvoinitsky
  2023-12-23 19:12                     ` [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd Mikhail Khvainitski
@ 2023-12-24 15:51                     ` Uli v. d. Ohe
  2023-12-26  9:36                       ` Yauhen Kharuzhy
  1 sibling, 1 reply; 16+ messages in thread
From: Uli v. d. Ohe @ 2023-12-24 15:51 UTC (permalink / raw)
  To: Mikhail Khvoinitsky
  Cc: jkosina, benjamin.tissoires, iam, jekhor, linux-input, linux-kernel

> So this means that the only reliable way is to add a sysfs parameter.
> I'll send a patch.

Thank you for the quick action!

Perhaps it would be possible to modify the firmware further in order to 
facilitate reliable detection of this modified firmware? But for now the 
solution with a sysfs parameter (and defaulting to the workaround) seems 
good.

Best regards,
Uli

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd
  2023-12-24 15:51                     ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Uli v. d. Ohe
@ 2023-12-26  9:36                       ` Yauhen Kharuzhy
  0 siblings, 0 replies; 16+ messages in thread
From: Yauhen Kharuzhy @ 2023-12-26  9:36 UTC (permalink / raw)
  To: Uli v. d. Ohe
  Cc: Mikhail Khvoinitsky, jkosina, benjamin.tissoires, iam,
	linux-input, linux-kernel

вс, 24 дек. 2023 г. в 18:51, Uli v. d. Ohe <u-v@mailbox.org>:
>
> > So this means that the only reliable way is to add a sysfs parameter.
> > I'll send a patch.
>
> Thank you for the quick action!
>
> Perhaps it would be possible to modify the firmware further in order to
> facilitate reliable detection of this modified firmware? But for now the
> solution with a sysfs parameter (and defaulting to the workaround) seems
> good.
Unfortunately no, the firmware is closed-source and was patched in
binary form AFAIR (by replacing 1-2 instructions). Moreover, it cannot
be updated in the wireless version of the keyboard (ThinkPad Compact
Keyboard II).
>
> Best regards,
> Uli



-- 
Yauhen Kharuzhy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd
  2023-12-23 19:12                     ` [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd Mikhail Khvainitski
@ 2024-01-23 10:55                       ` Jiri Kosina
  0 siblings, 0 replies; 16+ messages in thread
From: Jiri Kosina @ 2024-01-23 10:55 UTC (permalink / raw)
  To: Mikhail Khvainitski
  Cc: Benjamin Tissoires, linux-input, linux-kernel, iam, jekhor, u-v

On Sat, 23 Dec 2023, Mikhail Khvainitski wrote:

> Previous attempt to autodetect well-behaving patched firmware
> introduced in commit 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw
> on cptkbd and stop applying workaround") has shown that there are
> false-positives on original firmware (on both 1st gen and 2nd gen
> keyboards) which causes the middle button click workaround to be
> mistakenly disabled.
> 
> This commit adds explicit parameter to sysfs to control this
> workaround.
> 
> Fixes: 46a0a2c96f0f ("HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround")
> Fixes: 43527a0094c1 ("HID: lenovo: Restrict detection of patched firmware only to USB cptkbd")
> Signed-off-by: Mikhail Khvainitski <me@khvoinitsky.org>

I am not a huge fan of sysctl device-specific knobs like this, but I guess 
this is the best we can get here, unfortunately.

Now applied, thanks a lot.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2024-01-23 10:55 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-18 14:50 [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd Mikhail Khvainitski
2023-09-23 22:58 ` Mikhail Khvainitski
2023-09-23 22:58   ` [PATCH] HID: lenovo: Detect quirk-free fw on cptkbd and stop applying workaround Mikhail Khvainitski
2023-10-04 19:02     ` Jiri Kosina
2023-12-09 12:50     ` Yauhen Kharuzhy
2023-12-09 16:56       ` Yauhen Kharuzhy
2023-12-09 18:21         ` Yauhen Kharuzhy
2023-12-11 16:17           ` Mikhail Khvoinitsky
2023-12-12 13:31             ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Mikhail Khvainitski
2023-12-12 13:45               ` Jiri Kosina
2023-12-22 15:32                 ` Uli v. d. Ohe
2023-12-23 18:04                   ` Mikhail Khvoinitsky
2023-12-23 19:12                     ` [PATCH] HID: lenovo: Add middleclick_workaround sysfs knob for cptkbd Mikhail Khvainitski
2024-01-23 10:55                       ` Jiri Kosina
2023-12-24 15:51                     ` [PATCH] HID: lenovo: Restrict detection of patched firmware only to USB cptkbd Uli v. d. Ohe
2023-12-26  9:36                       ` Yauhen Kharuzhy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.