linux-kernel - Re: [PATCH v4 1/3] input: trackpoint - Enable doubletap by default on capable devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABxCQKt8U-QkT-LWiFR72X_XkRrkeUFsbC_rWOb=90LQxJ7MjQ@mail.gmail.com>
Date: Thu, 11 Dec 2025 23:46:13 +0900
From: Vishnu Sankar <vishnuocv@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: corbet@....net, hmh@....eng.br, derekjohn.clark@...il.com, 
	hansg@...nel.org, ilpo.jarvinen@...ux.intel.com, mpearson-lenovo@...ebb.ca, 
	linux-doc@...r.kernel.org, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org, ibm-acpi-devel@...ts.sourceforge.net, 
	platform-driver-x86@...r.kernel.org, vsankar@...ovo.com
Subject: Re: [PATCH v4 1/3] input: trackpoint - Enable doubletap by default on
 capable devices

Dimitry,

Thank you so much for the review.



On Thu, Dec 11, 2025 at 3:49 PM Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
>
> Hi Vishnu,
>
> On Sat, Nov 29, 2025 at 09:25:31AM +0900, Vishnu Sankar wrote:
> > Enable doubletap functionality by default on TrackPoint devices that
> > support it. The feature is detected using firmware ID pattern matching
> > (PNP: LEN03xxx) with a deny list of incompatible devices.
> >
> > This provides immediate doubletap functionality without requiring
> > userspace configuration. The hardware is enabled during device
> > detection, while event filtering continues to be handled by the
> > thinkpad_acpi driver as before.
> >
> > Signed-off-by: Vishnu Sankar <vishnuocv@...il.com>
> > Suggested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
> > ---
> > Changes in v4:
> > - Simplified approach: removed all sysfs attributes and user interface
> > - Enable doubletap by default during device detection
> > - Removed global variables and complex attribute infrastructure
> > - Uses minimal firmware ID detection with deny list
> > - Follows KISS principle as suggested by reviewers
> >
> > Changes in v3:
> > - No changes
> >
> > Changes in v2:
> > - Improve commit messages
> > - Sysfs attributes moved to trackpoint.c
> > - Removed unnecessary comments
> > - Removed unnecessary debug messages
> > - Using strstarts() instead of strcmp()
> > - is_trackpoint_dt_capable() modified
> > - Removed _BIT suffix and used BIT() define
> > - Reverse the trackpoint_doubletap_status() logic to return error first
> > - Removed export functions as a result of the design change
> > - Changed trackpoint_dev->psmouse to parent_psmouse
> > - The path of trackpoint.h is not changed
> > ---
> >  drivers/input/mouse/trackpoint.c | 51 ++++++++++++++++++++++++++++++++
> >  drivers/input/mouse/trackpoint.h |  5 ++++
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> > index 5f6643b69a2c..67144c27bccd 100644
> > --- a/drivers/input/mouse/trackpoint.c
> > +++ b/drivers/input/mouse/trackpoint.c
> > @@ -393,6 +393,48 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
> >       return 0;
> >  }
> >
> > +/* List of known incapable device PNP IDs */
> > +static const char * const dt_incompatible_devices[] = {
> > +     "LEN0304",
> > +     "LEN0306",
> > +     "LEN0317",
> > +     "LEN031A",
> > +     "LEN031B",
> > +     "LEN031C",
> > +     "LEN031D",
> > +};
> > +
> > +/*
> > + * Checks if it's a doubletap capable device
>
> Please finish the sentence with a period.
Got it.
I will complete the sentence in the comment:
>
> > + * The PNP ID format is "PNP: LEN030d PNP0f13".
> > + */
> > +static bool is_trackpoint_dt_capable(const char *pnp_id)
>
> Let's call it trackpoint_is_dt_capable() to keep with common
> "trackpoint_" prefix in the file.
Agreed.
Will use trackpoint_is_dt_capable() instead of is_trackpoint_dt_capable.
>
> > +{
> > +     const char *id_start;
> > +     char id[8];
> > +     size_t i;
> > +
> > +     if (!strstarts(pnp_id, "PNP: LEN03"))
> > +             return false;
> > +
> > +     /* Points to "LEN03xxxx" */
> > +     id_start = pnp_id + 5;
> > +     if (sscanf(id_start, "%7s", id) != 1)
> > +             return false;
> > +
> > +     /* Check if it's in the deny list */
> > +     for (i = 0; i < ARRAY_SIZE(dt_incompatible_devices); i++) {
> > +             if (strcmp(id, dt_incompatible_devices[i]) == 0)
>
> Why can't we use strncmp(pnp_id + 5, dt_incompatible_devices[i], 7) here
> (after ensuring that pnp_id is of sufficient length to begin with) and
> avoid sscanf()?
>
Agreed.
I can avoid the temporary buffer completely and compare directly using
strncmp().
Thank you.
> > +                     return false;
> > +     }
> > +     return true;
> > +}
> > +
> > +static int trackpoint_set_doubletap(struct ps2dev *ps2dev, bool enable)
> > +{
> > +     return trackpoint_write(ps2dev, TP_DOUBLETAP, enable ? TP_DOUBLETAP_ENABLE : TP_DOUBLETAP_DISABLE);
> > +}
>
> This wrapper seems an overkill given that it is called only once and
> always to enable the doubletap.
Understood.
Will call trackpoint_write() directly instead of using the
trackpoint_set_doubletap() wrapper.
>
> Thanks.
>
> --
> Dmitry



-- 

Regards,

      Vishnu Sankar
     +817015150407 (Japan)