| 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: <CABxCQKvsv3K_gviYMNS2gUQMdd+Q1w_hzJ5irTk5m9pphdSm9g@mail.gmail.com>
Date: Mon, 30 Jun 2025 20:36:01 +0900
From: Vishnu Sankar <vishnuocv@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.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
Hi Ilpo,
Sorry for the late reply.
On Fri, Jun 27, 2025 at 4:28 PM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> 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.
Thank you for pointing this out.
Will correct this.
>
> > > > + */
> > > > +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.
Understood.
>
> If you think you it needs to be broader to the match to a prefix only,
> I've no problem with that.
Understood. Will keep the original as it is, without changes.
>
> --
> i.
--
Regards,
Vishnu Sankar
+817015150407 (Japan)
Powered by blists - more mailing lists