linux-kernel - Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ef0f15c3b17ebbd58f7481910b3f40ff@lentin.co.uk>
Date:   Wed, 27 Sep 2023 09:19:04 +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-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);
> 
>  	return 0;
>  }