| 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
| ||
|
Message-ID: <7ed97f5f-edb2-7f15-1d53-42b7b16a5ae6@linux.intel.com>
Date: Fri, 27 Jun 2025 10:28:44 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Vishnu Sankar <vishnuocv@...il.com>
cc: pali@...nel.org, dmitry.torokhov@...il.com, hmh@....eng.br,
hansg@...nel.org, tglx@...utronix.de, mingo@...nel.org, jon_xie@...art.com,
jay_lee@...art.com, zhoubinbin@...ngson.cn, linux-input@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, ibm-acpi-devel@...ts.sourceforge.net,
platform-driver-x86@...r.kernel.org, vsankar@...ovo.com,
Mark Pearson <mpearson-lenovo@...ebb.ca>
Subject: Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap
handling
On Fri, 27 Jun 2025, Vishnu Sankar wrote:
> Hi Ilpo,
>
> Thanks a lot for the review.
>
> On Fri, Jun 27, 2025 at 12:09 AM Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> wrote:
> On Thu, 26 Jun 2025, Vishnu Sankar wrote:
>
> > Hi Ilpo,
> >
> > Thanks a lot for the comments and guidance.
> > Please find my comments inline below.
> >
> > On Wed, Jun 25, 2025 at 9:07 PM Ilpo Järvinen <
> ilpo.jarvinen@...ux.intel.com >
> > wrote:
> > On Fri, 20 Jun 2025, Vishnu Sankar wrote:
> >
> > I don't like the shortlog prefixes (in the subject), please don't
> make
> > confusing prefixes up like that.
> >
> > Got it.
> > I will correct this in V2 (as a patch series).
> >
> > > Newer ThinkPads have a doubletap feature that needs to be
> turned
> > > ON/OFF via the trackpoint registers.
> >
> > Don't leave lines short mid-paragraph. Either reflow the
> paragraph or add
> > an empty line in between paragraphs.
> >
> > Acked.
> > Will correct this in V2.
> >
> > > Systems released from 2023 have doubletap disabled by default
> and
> > > need the feature enabling to be useful.
> > >
> > > This patch introduces support for exposing and controlling the
> > > trackpoint doubletap feature via a sysfs attribute.
> > > /sys/devices/platform/thinkpad_acpi/tp_doubletap
> > > This can be toggled by an "enable" or a "disable".
> >
> > This too has too short lines.
> >
> > Sorry!
> > Will do the needful in v2.
> >
> > >
> > > With this implemented we can remove the masking of events, and
> rely on
> >
> > "With this implemented" is way too vague wording.
> >
> > Sorry for this!
> > I will make this better.
> >
> > > HW control instead, when the feature is disabled.
> > >
> > > Note - Early Thinkpads (pre 2015) used the same register for
> hysteris
> >
> > hysteresis ?
> >
> > Sorry for not being clear.
> > It's the trackpoint drag hysteris functionality. Pre-2015 ThinkPads
> used to have a
> > user-configurable drag hysterisis register (0x58).
> > Drag hysterisis is not user configurable now.
> >
> > > control, Check the FW IDs to make sure these are not affected.
> >
> > This Note feels very much disconnected from the rest. Should be
> properly
> > described and perhaps in own patch (I don't know)?
> >
> > my bad, it's not FW ID, but PnP ID.
> > The older ThinkPad's trackpoint controllers use the same register
> (0x58) to control
> > the drag hysteresis, which is obsolete now.
> > Now (on newer systems from 2023) the same register (0x58) is remapped
> as
> > doubletap register.
> > Just to exclude those older systems (with drag hysterisis control), we
> check the PnP
> > ID's.
> >
> > I will give a better commit message in V2.
> >
> > > trackpoint.h is moved to linux/input/.
> >
> > This patch doesn't look minimal and seems to be combining many
> changes
> > into one. Please make a patch series out of changes that can be
> separated
> > instead of putting all together.
> >
> > Understood.
> > Will make a patch series on V2
> > 0001: Move trackpoint.h to include/linux/input
> > 0002: Add new doubletap set/status/capable logics to trackpoint.c
> > 0003: Add new trackpoint api's and trackpoint sysfs in thinkpad_acpi.c
>
> Okay, sounds better.
>
> > > +/* 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
> > > + * The PNP ID format eg: is "PNP: LEN030d PNP0f13".
There's case difference between this comment and the list.
> > > + */
> > > +bool is_trackpoint_dt_capable(const char *pnp_id)
> > > +{
> > > + char id[16];
> > > +
> > > + /* Make sure string starts with "PNP: " */
> > > + if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)
> >
> > We seem to have strstarts().
> >
> > Thanks a lot for the suggestion.
> > Will make use of this.
> >
> > > + return false;
> > > +
> > > + /* Extract the first word after "PNP: " */
> > > + if (sscanf(pnp_id + 5, "%15s", id) != 1)
> > > + return false;
> > > +
> > > + /* Check if it's blacklisted */
> > > + for (size_t i = 0; i <
> ARRAY_SIZE(dt_incompatible_devices); ++i)
> > {
> > > + if (strcmp(pnp_id, dt_incompatible_devices[i]) ==
> 0)
> >
> > Isn't this buggy wrt. the PNP: prefix??
> >
> > Perhaps it would have been better to just do pnp_id += 5 before
> sscanf()
> > as you don't care about the prefix after that.
> >
> >
> > Understood.
> > Shall we have something like the following:
> > if (!strstarts(pnp_id, "PNP: LEN03"))
> > return false;
> >
> > id = pnp_id + 5;
> >
> > for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices);
> ++i) {
> > if (strncmp(id, dt_incompatible_devices[i],
> > strlen(dt_incompatible_devices[i])) == 0)
>
> Why did you change to strncmp()?
>
> I switched to strncmp() to allow matching just the known prefix part in
> dt_incompatible_devices, rather than requiring an exact full string match.
> Will keep the original "if (strcmp(id, dt_incompatible_devices[i]) == 0) " logic as
> it serves the purpose.
I didn't mean to say the change is necessarily incorrect, I was just
asking for reasonale as it was different from the original.
If you think you it needs to be broader to the match to a prefix only,
I've no problem with that.
--
i.
Powered by blists - more mailing lists