| Message ID | 20220929082119.22112-4-nyanpasu256@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: Fix incorrectly halved touchpad range on ELAN v3 touchpads | expand |
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 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 <nyanpasu256@gmail.com> 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 <mkorpershoek@baylibre.com> > --- > 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 <mkorpershoek@baylibre.com> > > 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 <mkorpershoek@baylibre.com> 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 <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.
Hi Eirin, On Sat, Oct 08, 2022 at 03:38, Eirin Nya <nyanpasu256@gmail.com> wrote: > On 10/7/22 6:35 AM, Mattijs Korpershoek wrote: >> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> >> >> 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 <mkorpershoek@baylibre.com> > > 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.
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:
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 <nyanpasu256@gmail.com> --- drivers/input/mouse/elantech.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)