| 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: <b3603d57b6d2be22f2a5615e66775d0b@lentin.co.uk>
Date: Sat, 30 Sep 2023 10:58:52 +0100
From: Jamie Lentin <jm@...tin.co.uk>
To: Martin Kepplinger <martink@...teo.de>
Cc: jikos@...nel.org, benjamin.tissoires@...hat.com,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] hid: lenovo: move type checks to
lenovo_features_set_cptkbd()
On 2023-09-30 10:26, Martin Kepplinger wrote:
> Am Donnerstag, dem 28.09.2023 um 22:06 +0100 schrieb Jamie Lentin:
>> On 2023-09-27 12:20, Martin Kepplinger wrote:
>> > Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
>> > > On 2023-09-25 11:23, Martin Kepplinger wrote:
>> > > > These custom commands will be sent to both the USB keyboard &
>> > > > mouse
>> > > > devices but only the mouse will respond. Avoid sending known-
>> > > > useless
>> > > > messages by always prepending the filter before sending them.
>> > > >
>> > > > Suggested-by: Jamie Lentin <jm@...tin.co.uk>
>> > > > Signed-off-by: Martin Kepplinger <martink@...teo.de>
>> > > > ---
>> > > > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
>> > > > 1 file changed, 9 insertions(+), 18 deletions(-)
>> > > >
>> > > > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-
>> > > > lenovo.c
>> > > > index 29aa6d372bad..922f3e5462f4 100644
>> > > > --- a/drivers/hid/hid-lenovo.c
>> > > > +++ b/drivers/hid/hid-lenovo.c
>> > > > @@ -521,6 +521,14 @@ static void
>> > > > lenovo_features_set_cptkbd(struct
>> > > > hid_device *hdev)
>> > > > int ret;
>> > > > struct lenovo_drvdata *cptkbd_data =
>> > > > hid_get_drvdata(hdev);
>> > > >
>> > > > + /* All the custom action happens on the USBMOUSE device
>> > > > for
>> > > > USB */
>> > > > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > > > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
>> > > > &&
>> > > > + hdev->type != HID_TYPE_USBMOUSE) {
>> > > > + hid_dbg(hdev, "Ignoring keyboard half of
>> > > > device\n");
>> > > > + return;
>> > > > + }
>> > > > +
>> > > > /*
>> > > > * Tell the keyboard a driver understands it, and turn
>> > > > F7,
>> > > > F9, F11
>> > > > into
>> > > > * regular keys
>> > > > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
>> > > > hid_device
>> > > > *hdev)
>> > > > int ret;
>> > > > struct lenovo_drvdata *cptkbd_data;
>> > > >
>> > > > - /* All the custom action happens on the USBMOUSE device
>> > > > for
>> > > > USB */
>> > > > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > > > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
>> > > > &&
>> > > > - hdev->type != HID_TYPE_USBMOUSE) {
>> > > > - hid_dbg(hdev, "Ignoring keyboard half of
>> > > > device\n");
>> > > > - return 0;
>> > > > - }
>> > > > -
>> > >
>> > > I like the idea of doing it once then forgetting about it, but
>> > > removing
>> > > this will mean that the "keyboard half" will have it's own set of
>> > > non-functional sysfs parameters I think? Currently:-
>> > >
>> > > # evtest
>> > > . . .
>> > > /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
>> > > TrackPoint
>> > > /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
>> > > TrackPoint
>> > > /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
>> > > TrackPoint
>> > >
>> > > # ls -1 /sys/class/input/event*/device/device/fn_lock
>> > > /sys/class/input/event10/device/device/fn_lock
>> > > /sys/class/input/event12/device/device/fn_lock
>> > >
>> > > (note 11 is missing.)
>> > >
>> > > I think the easiest (but ugly) thing to do is to copy-paste this
>> > > lump
>> > > of
>> > > code to the top of lenovo_reset_resume.
>> > > Cheers,
>> > >
>> > > > cptkbd_data = devm_kzalloc(&hdev->dev,
>> > > > sizeof(*cptkbd_data),
>> > > > GFP_KERNEL);
>> > > > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct
>> > > > hid_device
>> > > > *hdev,
>> > > > #ifdef CONFIG_PM
>> > > > static int lenovo_reset_resume(struct hid_device *hdev)
>> > > > {
>> > > > - switch (hdev->product) {
>> > > > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
>> > > > - if (hdev->type == HID_TYPE_USBMOUSE) {
>> > > > - lenovo_features_set_cptkbd(hdev);
>> > > > - }
>> > > > -
>> > > > - break;
>> > > > - default:
>> > > > - break;
>> > > > - }
>> > > > + lenovo_features_set_cptkbd(hdev);
>> >
>> > ok. ignore my change (this whole patch) and look at your addition
>> > here,
>> > don't you already make sure only the mouse-part gets the messages?
>> > you
>> > just write switch()case instead of if(); what do you think is
>> > missing
>> > here?
>>
>> Correct, this switch statement() that you're removing in this patch
>> already does exactly this, so replacing it with the
>> if()-and-return-early block would result in equivalent code (ignoring
>> the Trackpoint keyboard II). That suggestion wasn't the most helpful
>> of
>> mine, sorry!
>>
>> The reason I originally used a switch here is for symmetry with
>> lenovo_probe(), lenovo_remove(), etc. It might some day be useful to
>> add
>> something like:
>>
>> case USB_DEVICE_ID_LENOVO_X1_TAB:
>> lenovo_reset_resume_tp10ubkbd(hdev);
>> break;
>>
>> ...to the switch. For completeness, lenovo_reset_resume() should
>> probably call a separate lenovo_reset_resume_cptkbd() that does the
>> work. For just 3 lines of code it didn't seem worth it at the time
>> though.
>>
>> Cheers,
>
> ok your original patch seems to basically be a valid first fix. Should
> I send it on your behalf (with you as author) or do you want to send it
> yourself? Let's get this fixed :)
If it's working for you and you don't mind, feel free send it on my
behalf. I don't have the hardware to test the patch currently.
Thanks!
>
> thanks,
> martin
Powered by blists - more mailing lists