On Linux 5.19.10, on my laptop (Dell Inspiron 15R SE 7520) with an Elan
v3 touchpad, the reported size of my touchpad (1470 by 700) is half that
of the actual touch range (2940 by 1400), and the upper half of my
touchpad reports negative values. As a result, with the Synaptics or
libinput X11 driver set to edge scrolling mode, the entire right half of
my touchpad has x-values past evdev's reported maximum size, and acts as
a giant scrollbar!
The problem is that elantech_setup_ps2() -> elantech_set_absolute_mode()
sets up absolute mode and doubles the hardware resolution (doubling the
hardware's maximum reported x/y coordinates and its response to
ETP_FW_ID_QUERY), *after* elantech_query_info() fetches the touchpad
coordinate system size using ETP_FW_ID_QUERY, which gets cached and
reported to userspace through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the
touchpad size reported to userspace (and used to subtract vertical
coordinates from) is half the maximum position of actual touches.
This patch series splits out a function elantech_query_range_v3() which
fetches *only* ETP_FW_ID_QUERY (touchpad size), and calls it a second
time if elantech_set_absolute_mode() enables double-size mode. This
means the first call is redundant and wasted if a second call occurs,
but this minimizes the need to restructure the driver.
A possible alternative approach is to restructure the driver and set
resolution before querying touchpad size. I did not attempt this myself,
and don't know how the Windows Dell Touchpad ELAN driver handles
double-resolution.
Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/
Eirin Nya (3):
Input: Remove redundant field elantech_data::y_max
Input: Remove redundant field elantech_data::width
Input: Fix incorrectly halved touchpad range on ELAN v3 touchpads
drivers/input/mouse/elantech.c | 51 +++++++++++++++++++++++-----------
drivers/input/mouse/elantech.h | 2 --
2 files changed, 35 insertions(+), 18 deletions(-)
base-commit: fff1011a26d6cbf26b18c8ee4c61d99943174f8c
--
2.37.3
elantech_data::width is copied from elantech_device_info::width, and
neither is mutated after initialization. So remove the redundant
variable to prevent future bugs.
Signed-off-by: Eirin Nya <[email protected]>
---
drivers/input/mouse/elantech.c | 4 +---
drivers/input/mouse/elantech.h | 1 -
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 79e31611fc..263779c031 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -691,7 +691,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
input_report_abs(dev, ABS_MT_POSITION_X, etd->mt[id].x);
input_report_abs(dev, ABS_MT_POSITION_Y, etd->mt[id].y);
input_report_abs(dev, ABS_MT_PRESSURE, pres);
- input_report_abs(dev, ABS_MT_TOUCH_MAJOR, traces * etd->width);
+ input_report_abs(dev, ABS_MT_TOUCH_MAJOR, traces * etd->info.width);
/* report this for backwards compatibility */
input_report_abs(dev, ABS_TOOL_WIDTH, traces);
@@ -1253,8 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
}
- etd->width = width;
-
return 0;
}
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 1ec004f72d..fb093134ea 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -180,7 +180,6 @@ struct elantech_data {
unsigned char reg_25;
unsigned char reg_26;
unsigned int single_finger_reports;
- unsigned int width;
struct finger_pos mt[ETP_MAX_FINGERS];
unsigned char parity[256];
struct elantech_device_info info;
--
2.37.3
On Linux 5.19.10, on my laptop (Dell Inspiron 15R SE 7520) with an Elan
v3 touchpad (dmesg says "with firmware version 0x450f02"), the reported
size of my touchpad (in userspace by calling mtdev_configure() and
libevdev_get_abs_maximum(), in kernel space
elantech_device_info::x_max/y_max, either way 1470 by 700) is half that
of the actual touch range (2940 by 1400), and the upper half of my
touchpad reports negative values. As a result, with the Synaptics or
libinput X11 driver set to edge scrolling mode, the entire right half of
my touchpad has x-values past evdev's reported maximum size, and acts as
a giant scrollbar!
The problem is that elantech_setup_ps2() -> elantech_set_absolute_mode()
sets up absolute mode and doubles the hardware resolution (doubling the
hardware's maximum reported x/y coordinates and its response to
ETP_FW_ID_QUERY), *after* elantech_query_info() fetches the touchpad
coordinate system size using ETP_FW_ID_QUERY, which gets cached and
reported to userspace through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the
touchpad size reported to userspace (and used to subtract vertical
coordinates from) is half the maximum position of actual touches.
This patch splits out a function elantech_query_range_v3() which fetches
*only* ETP_FW_ID_QUERY (touchpad size), and calls it a second time if
elantech_set_absolute_mode() enables double-size mode. This means the
first call is redundant and wasted if a second call occurs, but this
minimizes the need to restructure the driver.
Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/
Signed-off-by: Eirin Nya <[email protected]>
---
drivers/input/mouse/elantech.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 263779c031..a2176f0fd3 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -1006,6 +1006,9 @@ static void elantech_set_rate_restore_reg_07(struct psmouse *psmouse,
psmouse_err(psmouse, "restoring reg_07 failed\n");
}
+static int elantech_query_range_v3(struct psmouse *psmouse,
+ struct elantech_device_info *info);
+
/*
* Put the touchpad into absolute mode
*/
@@ -1047,6 +1050,14 @@ static int elantech_set_absolute_mode(struct psmouse *psmouse)
if (elantech_write_reg(psmouse, 0x10, etd->reg_10))
rc = -1;
+ /*
+ * If we boost hardware resolution, we have to re-query
+ * info->x_max and y_max.
+ */
+ if (etd->info.set_hw_resolution)
+ if (elantech_query_range_v3(psmouse, &etd->info))
+ rc = -1;
+
break;
case 4:
@@ -1671,6 +1682,20 @@ static int elantech_set_properties(struct elantech_device_info *info)
return 0;
}
+static int elantech_query_range_v3(struct psmouse *psmouse,
+ struct elantech_device_info *info)
+{
+ unsigned char param[3];
+
+ if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
+ return -EINVAL;
+
+ info->x_max = (0x0f & param[0]) << 8 | param[1];
+ info->y_max = (0xf0 & param[0]) << 4 | param[2];
+
+ return 0;
+}
+
static int elantech_query_info(struct psmouse *psmouse,
struct elantech_device_info *info)
{
@@ -1826,11 +1851,8 @@ static int elantech_query_info(struct psmouse *psmouse,
break;
case 3:
- if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
+ if (elantech_query_range_v3(psmouse, info))
return -EINVAL;
-
- info->x_max = (0x0f & param[0]) << 8 | param[1];
- info->y_max = (0xf0 & param[0]) << 4 | param[2];
break;
case 4:
--
2.37.3
elantech_data::y_max is copied from elantech_device_info::y_max, and
neither is mutated after initialization. So remove the redundant
variable to prevent future bugs when updating y_max.
Signed-off-by: Eirin Nya <[email protected]>
---
drivers/input/mouse/elantech.c | 17 ++++++++---------
drivers/input/mouse/elantech.h | 1 -
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index ece97f8c6a..79e31611fc 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -360,7 +360,7 @@ static void elantech_report_absolute_v1(struct psmouse *psmouse)
input_report_abs(dev, ABS_X,
((packet[1] & 0x0c) << 6) | packet[2]);
input_report_abs(dev, ABS_Y,
- etd->y_max - (((packet[1] & 0x03) << 8) | packet[3]));
+ etd->info.y_max - (((packet[1] & 0x03) << 8) | packet[3]));
}
input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
@@ -435,7 +435,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
* byte 4: . . . . y11 y10 y9 y8
* byte 5: y7 y6 y5 y4 y3 y2 y1 y0
*/
- y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
width = ((packet[0] & 0x30) >> 2) | ((packet[3] & 0x30) >> 4);
@@ -450,7 +450,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
*/
x1 = (((packet[0] & 0x10) << 4) | packet[1]) << 2;
/* byte 2: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0 */
- y1 = etd->y_max -
+ y1 = etd->info.y_max -
((((packet[0] & 0x20) << 3) | packet[2]) << 2);
/*
* byte 3: . . by8 bx8 . . . .
@@ -458,7 +458,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
*/
x2 = (((packet[3] & 0x10) << 4) | packet[4]) << 2;
/* byte 5: by7 by8 by5 by4 by3 by2 by1 by0 */
- y2 = etd->y_max -
+ y2 = etd->info.y_max -
((((packet[3] & 0x20) << 3) | packet[5]) << 2);
/* Unknown so just report sensible values */
@@ -579,7 +579,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
* byte 4: . . . . y11 y10 y9 y8
* byte 5: y7 y6 y5 y4 y3 y2 y1 y0
*/
- y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
break;
case 2:
@@ -593,7 +593,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
* byte 4: . . . . ay11 ay10 ay9 ay8
* byte 5: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0
*/
- etd->mt[0].y = etd->y_max -
+ etd->mt[0].y = etd->info.y_max -
(((packet[4] & 0x0f) << 8) | packet[5]);
/*
* wait for next packet
@@ -605,7 +605,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
x1 = etd->mt[0].x;
y1 = etd->mt[0].y;
x2 = ((packet[1] & 0x0f) << 8) | packet[2];
- y2 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ y2 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
break;
}
@@ -681,7 +681,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
return;
etd->mt[id].x = ((packet[1] & 0x0f) << 8) | packet[2];
- etd->mt[id].y = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ etd->mt[id].y = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
traces = (packet[0] & 0xf0) >> 4;
@@ -1253,7 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
}
- etd->y_max = y_max;
etd->width = width;
return 0;
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 571e6ca11d..1ec004f72d 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -180,7 +180,6 @@ struct elantech_data {
unsigned char reg_25;
unsigned char reg_26;
unsigned int single_finger_reports;
- unsigned int y_max;
unsigned int width;
struct finger_pos mt[ETP_MAX_FINGERS];
unsigned char parity[256];
--
2.37.3
On Thu, Sep 29, 2022 at 01:21, Eirin Nya <[email protected]> wrote:
> elantech_data::width is copied from elantech_device_info::width, and
> neither is mutated after initialization. So remove the redundant
> variable to prevent future bugs.
>
> Signed-off-by: Eirin Nya <[email protected]>
Reviewed-by: Mattijs Korpershoek <[email protected]>
ps: please consider using the proper subject line for input subsystem if
you need to send again.
It should be "Input: elantech - Remove redundant field elantech_data::width"
$ git log --oneline drivers/input/mouse/elantech.c shows:
1d72d9f960cc Input: elantech - fix stack out of bound access in elantech_change_report_id()
be896bd3b72b Input: elantench - fix misreporting trackpoint coordinates
9d383e96448d Input: elantech - Prepare a complete software node for the device
(this is a minor remark, so no need to send a v2 to just change the
subject line)
Cheers
Mattijs
> ---
> drivers/input/mouse/elantech.c | 4 +---
> drivers/input/mouse/elantech.h | 1 -
> 2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index 79e31611fc..263779c031 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -691,7 +691,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
> input_report_abs(dev, ABS_MT_POSITION_X, etd->mt[id].x);
> input_report_abs(dev, ABS_MT_POSITION_Y, etd->mt[id].y);
> input_report_abs(dev, ABS_MT_PRESSURE, pres);
> - input_report_abs(dev, ABS_MT_TOUCH_MAJOR, traces * etd->width);
> + input_report_abs(dev, ABS_MT_TOUCH_MAJOR, traces * etd->info.width);
> /* report this for backwards compatibility */
> input_report_abs(dev, ABS_TOOL_WIDTH, traces);
>
> @@ -1253,8 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
> input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
> }
>
> - etd->width = width;
> -
> return 0;
> }
>
> diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
> index 1ec004f72d..fb093134ea 100644
> --- a/drivers/input/mouse/elantech.h
> +++ b/drivers/input/mouse/elantech.h
> @@ -180,7 +180,6 @@ struct elantech_data {
> unsigned char reg_25;
> unsigned char reg_26;
> unsigned int single_finger_reports;
> - unsigned int width;
> struct finger_pos mt[ETP_MAX_FINGERS];
> unsigned char parity[256];
> struct elantech_device_info info;
> --
> 2.37.3
Hi Eirin,
Thank you for your patch.
On Thu, Sep 29, 2022 at 01:21, Eirin Nya <[email protected]> wrote:
> elantech_data::y_max is copied from elantech_device_info::y_max, and
> neither is mutated after initialization. So remove the redundant
> variable to prevent future bugs when updating y_max.
>
> Signed-off-by: Eirin Nya <[email protected]>
> ---
> drivers/input/mouse/elantech.c | 17 ++++++++---------
> drivers/input/mouse/elantech.h | 1 -
> 2 files changed, 8 insertions(+), 10 deletions(-)
Reviewed-by: Mattijs Korpershoek <[email protected]>
>
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index ece97f8c6a..79e31611fc 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -360,7 +360,7 @@ static void elantech_report_absolute_v1(struct psmouse *psmouse)
> input_report_abs(dev, ABS_X,
> ((packet[1] & 0x0c) << 6) | packet[2]);
> input_report_abs(dev, ABS_Y,
> - etd->y_max - (((packet[1] & 0x03) << 8) | packet[3]));
> + etd->info.y_max - (((packet[1] & 0x03) << 8) | packet[3]));
> }
>
> input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
> @@ -435,7 +435,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
> * byte 4: . . . . y11 y10 y9 y8
> * byte 5: y7 y6 y5 y4 y3 y2 y1 y0
> */
> - y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> + y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
>
> pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
> width = ((packet[0] & 0x30) >> 2) | ((packet[3] & 0x30) >> 4);
> @@ -450,7 +450,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
> */
> x1 = (((packet[0] & 0x10) << 4) | packet[1]) << 2;
> /* byte 2: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0 */
> - y1 = etd->y_max -
> + y1 = etd->info.y_max -
> ((((packet[0] & 0x20) << 3) | packet[2]) << 2);
> /*
> * byte 3: . . by8 bx8 . . . .
> @@ -458,7 +458,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
> */
> x2 = (((packet[3] & 0x10) << 4) | packet[4]) << 2;
> /* byte 5: by7 by8 by5 by4 by3 by2 by1 by0 */
> - y2 = etd->y_max -
> + y2 = etd->info.y_max -
> ((((packet[3] & 0x20) << 3) | packet[5]) << 2);
>
> /* Unknown so just report sensible values */
> @@ -579,7 +579,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
> * byte 4: . . . . y11 y10 y9 y8
> * byte 5: y7 y6 y5 y4 y3 y2 y1 y0
> */
> - y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> + y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> break;
>
> case 2:
> @@ -593,7 +593,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
> * byte 4: . . . . ay11 ay10 ay9 ay8
> * byte 5: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0
> */
> - etd->mt[0].y = etd->y_max -
> + etd->mt[0].y = etd->info.y_max -
> (((packet[4] & 0x0f) << 8) | packet[5]);
> /*
> * wait for next packet
> @@ -605,7 +605,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
> x1 = etd->mt[0].x;
> y1 = etd->mt[0].y;
> x2 = ((packet[1] & 0x0f) << 8) | packet[2];
> - y2 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> + y2 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> break;
> }
>
> @@ -681,7 +681,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
> return;
>
> etd->mt[id].x = ((packet[1] & 0x0f) << 8) | packet[2];
> - etd->mt[id].y = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> + etd->mt[id].y = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
> traces = (packet[0] & 0xf0) >> 4;
>
> @@ -1253,7 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
> input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
> }
>
> - etd->y_max = y_max;
> etd->width = width;
>
> return 0;
> diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
> index 571e6ca11d..1ec004f72d 100644
> --- a/drivers/input/mouse/elantech.h
> +++ b/drivers/input/mouse/elantech.h
> @@ -180,7 +180,6 @@ struct elantech_data {
> unsigned char reg_25;
> unsigned char reg_26;
> unsigned int single_finger_reports;
> - unsigned int y_max;
> unsigned int width;
> struct finger_pos mt[ETP_MAX_FINGERS];
> unsigned char parity[256];
> --
> 2.37.3
On Thu, Sep 29, 2022 at 01:21, Eirin Nya <[email protected]> wrote:
> On Linux 5.19.10, on my laptop (Dell Inspiron 15R SE 7520) with an Elan
> v3 touchpad (dmesg says "with firmware version 0x450f02"), the reported
> size of my touchpad (in userspace by calling mtdev_configure() and
> libevdev_get_abs_maximum(), in kernel space
> elantech_device_info::x_max/y_max, either way 1470 by 700) is half that
> of the actual touch range (2940 by 1400), and the upper half of my
> touchpad reports negative values. As a result, with the Synaptics or
> libinput X11 driver set to edge scrolling mode, the entire right half of
> my touchpad has x-values past evdev's reported maximum size, and acts as
> a giant scrollbar!
>
> The problem is that elantech_setup_ps2() -> elantech_set_absolute_mode()
> sets up absolute mode and doubles the hardware resolution (doubling the
> hardware's maximum reported x/y coordinates and its response to
> ETP_FW_ID_QUERY), *after* elantech_query_info() fetches the touchpad
> coordinate system size using ETP_FW_ID_QUERY, which gets cached and
> reported to userspace through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the
> touchpad size reported to userspace (and used to subtract vertical
> coordinates from) is half the maximum position of actual touches.
>
> This patch splits out a function elantech_query_range_v3() which fetches
> *only* ETP_FW_ID_QUERY (touchpad size), and calls it a second time if
> elantech_set_absolute_mode() enables double-size mode. This means the
> first call is redundant and wasted if a second call occurs, but this
> minimizes the need to restructure the driver.
>
> Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/
>
> Signed-off-by: Eirin Nya <[email protected]>
This seems like a candidate patch for stable kernels as well.
Maybe consider adding the following in the commit message footer:
Fixes: 28f49616113f ("Input: elantech - add v3 hardware support")
Reviewed-by: Mattijs Korpershoek <[email protected]>
> ---
> drivers/input/mouse/elantech.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index 263779c031..a2176f0fd3 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -1006,6 +1006,9 @@ static void elantech_set_rate_restore_reg_07(struct psmouse *psmouse,
> psmouse_err(psmouse, "restoring reg_07 failed\n");
> }
>
> +static int elantech_query_range_v3(struct psmouse *psmouse,
> + struct elantech_device_info *info);
> +
> /*
> * Put the touchpad into absolute mode
> */
> @@ -1047,6 +1050,14 @@ static int elantech_set_absolute_mode(struct psmouse *psmouse)
> if (elantech_write_reg(psmouse, 0x10, etd->reg_10))
> rc = -1;
>
> + /*
> + * If we boost hardware resolution, we have to re-query
> + * info->x_max and y_max.
> + */
> + if (etd->info.set_hw_resolution)
> + if (elantech_query_range_v3(psmouse, &etd->info))
> + rc = -1;
> +
> break;
>
> case 4:
> @@ -1671,6 +1682,20 @@ static int elantech_set_properties(struct elantech_device_info *info)
> return 0;
> }
>
> +static int elantech_query_range_v3(struct psmouse *psmouse,
> + struct elantech_device_info *info)
> +{
> + unsigned char param[3];
> +
> + if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
> + return -EINVAL;
> +
> + info->x_max = (0x0f & param[0]) << 8 | param[1];
> + info->y_max = (0xf0 & param[0]) << 4 | param[2];
> +
> + return 0;
> +}
> +
> static int elantech_query_info(struct psmouse *psmouse,
> struct elantech_device_info *info)
> {
> @@ -1826,11 +1851,8 @@ static int elantech_query_info(struct psmouse *psmouse,
> break;
>
> case 3:
> - if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
> + if (elantech_query_range_v3(psmouse, info))
> return -EINVAL;
> -
> - info->x_max = (0x0f & param[0]) << 8 | param[1];
> - info->y_max = (0xf0 & param[0]) << 4 | param[2];
> break;
>
> case 4:
> --
> 2.37.3
On 10/7/22 6:35 AM, Mattijs Korpershoek wrote:
> Reviewed-by: Mattijs Korpershoek <[email protected]>
>
> ps: please consider using the proper subject line for input subsystem if
> you need to send again.
>
> It should be "Input: elantech - Remove redundant field elantech_data::width
On 10/7/22 7:06 AM, Mattijs Korpershoek wrote:
> This seems like a candidate patch for stable kernels as well.
>
> Maybe consider adding the following in the commit message footer:
> Fixes: 28f49616113f ("Input: elantech - add v3 hardware support")
>
> Reviewed-by: Mattijs Korpershoek <[email protected]>
Should I submit a v2 where I change the subject line for all commits
and add a Fixes: tag to patch 3/3 (or 0/3, not sure), but not change
the code? Sorry, I'm new to kernel development.
On Fri, 07 Oct 2022 16:06:08 +0200
Mattijs Korpershoek <[email protected]> wrote:
> On Thu, Sep 29, 2022 at 01:21, Eirin Nya <[email protected]>
> wrote:
>
> > On Linux 5.19.10, on my laptop (Dell Inspiron 15R SE 7520) with an
> > Elan v3 touchpad (dmesg says "with firmware version 0x450f02"), the
> > reported size of my touchpad (in userspace by calling
> > mtdev_configure() and libevdev_get_abs_maximum(), in kernel space
> > elantech_device_info::x_max/y_max, either way 1470 by 700) is half
> > that of the actual touch range (2940 by 1400), and the upper half
> > of my touchpad reports negative values. As a result, with the
> > Synaptics or libinput X11 driver set to edge scrolling mode, the
> > entire right half of my touchpad has x-values past evdev's reported
> > maximum size, and acts as a giant scrollbar!
> >
> > The problem is that elantech_setup_ps2() ->
> > elantech_set_absolute_mode() sets up absolute mode and doubles the
> > hardware resolution (doubling the hardware's maximum reported x/y
> > coordinates and its response to ETP_FW_ID_QUERY), *after*
> > elantech_query_info() fetches the touchpad coordinate system size
> > using ETP_FW_ID_QUERY, which gets cached and reported to userspace
> > through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the touchpad size
> > reported to userspace (and used to subtract vertical coordinates
> > from) is half the maximum position of actual touches.
> > ...
>
> This seems like a candidate patch for stable kernels as well.
>
> Maybe consider adding the following in the commit message footer:
> Fixes: 28f49616113f ("Input: elantech - add v3 hardware support")
Interestingly, if I understand correctly, the bug was actually *not*
introduced by 28f49616113f ("Input: elantech - add v3 hardware
support"). In that commit, elantech_init called
(elantech_set_absolute_mode -> elantech_write_reg(...0x0b)) to set
absolute double-size mode, though bit 3 controlling double-resolution
was not documented at this point, then (elantech_set_input_params ->
elantech_set_range -> ETP_FW_ID_QUERY) *after* enabling double-size
mode. This code structure was maintained through 36189cc3cd57 ("Input:
elantech - fix touchpad initialization on Gigabyte U2442"), which
introduced etd->set_hw_resolution.
By the time of f07875920116 ("Input: elantech - split device info into
a separate structure"), elantech_setup_ps2 called
(elantech_set_absolute_mode -> elantech_write_reg(...0x0b or 0x01)
(later changed to 0x03)) before (elantech_set_input_params ->
elantech_set_range -> ETP_FW_ID_QUERY).
The bug was actually introduced by 37548659bb22 ("Input: elantech -
query the min/max information beforehand too"), which removed
elantech_set_range() and moved ETP_FW_ID_QUERY into
elantech_query_info(). At this point, elantech_init_ps2() for non-SMBus
touchpads calls (elantech_query_info() -> ETP_FW_ID_QUERY) before
(elantech_setup_ps2 -> elantech_set_absolute_mode ->
elantech_write_reg), so now the size is queried *before* setting
doubled size for the touchpad.
Why was this change made? The commit message says:
> For the latest generation of Elantech touchpads, we need to forward
> the min/max information from PS/2 to SMBus.
I don't know which generation of touchpad that is, and whether it
correlates with protocol version 3 vs. 4 or not. I'm guessing that in
the process of adding/fixing support for newer touchpads, Benjamin
Tissoires subtly broke size queries for older v3 PS/2 touchpads. At
this point, is it worth asking him about this bug?
In any case, given the complexity of this code designed to handle
multiple hardware types, including SMBus Elan touchpads I cannot test
myself, I now feel that my surgical patch is the safest way to fix v3
touchpads. It's probably error-prone to *conditionally* query size
later on in elantech_set_input_params for v3 (and below?) touchpads,
without breaking hardware fixed by 37548659bb22.
Should we move (elantech_set_absolute_mode ->
elantech_write_reg(...0x0b or 0x01)) *earlier* into
elantech_query_info() before "query range information"? I don't know if
that will break anything either, since I can't test on anything but v3
touchpads. But it's an option to explore if you want to avoid querying
range twice.
Hi Eirin,
On Sat, Oct 08, 2022 at 03:38, Eirin Nya <[email protected]> wrote:
> On 10/7/22 6:35 AM, Mattijs Korpershoek wrote:
>> Reviewed-by: Mattijs Korpershoek <[email protected]>
>>
>> ps: please consider using the proper subject line for input subsystem if
>> you need to send again.
>>
>> It should be "Input: elantech - Remove redundant field elantech_data::width
> On 10/7/22 7:06 AM, Mattijs Korpershoek wrote:
>> This seems like a candidate patch for stable kernels as well.
>>
>> Maybe consider adding the following in the commit message footer:
>> Fixes: 28f49616113f ("Input: elantech - add v3 hardware support")
>>
>> Reviewed-by: Mattijs Korpershoek <[email protected]>
>
> Should I submit a v2 where I change the subject line for all commits
> and add a Fixes: tag to patch 3/3 (or 0/3, not sure), but not change
> the code? Sorry, I'm new to kernel development.
No worries. It's not a very newcomer friendly process. I'm not that
experienced myself but i'll try to help you.
No need to just re-submit a v2 for changing the subject line.
If you do have to send a v2, however, please keep in mind to do the change.
For the Fixes: tag itself, I think it should only be applied on patch
[3/3] since that's the one "actually doing the fix"
The stable maintainers will detect a Fixes: tag and will eventually pick
this up once it lands in linus' tree.