Re: [PATCH 3/3] Input: Fix incorrectly halved touchpad range on ELAN v3 touchpads

[Eirin Nya <nyanpasu256@gmail.com>]

On Fri, 07 Oct 2022 16:06:08 +0200
Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> On Thu, Sep 29, 2022 at 01:21, Eirin Nya <nyanpasu256@gmail.com>
> 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.