Message-ID: <j3isljjyd6rlddlhpp7knxgss2mpr4ft3pcx5lc7r5r4bnnzpw@wjr6brfv2hsf>
Date: Thu, 3 Jul 2025 15:48:00 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: "Derek J. Clark" <derekjohn.clark@...il.com>
Cc: Jiri Kosina <jikos@...nel.org>, Mario Limonciello <superm1@...nel.org>, 
	Xino Ni <nijs1@...ovo.com>, Zhixin Zhang <zhangzx36@...ovo.com>, 
	Mia Shao <shaohz1@...ovo.com>, Mark Pearson <mpearson-lenovo@...ebb.ca>, 
	"Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>, linux-input@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/6] HID: Add Legion Go S Driver

Hi Derek,

[I'll answer to this email with a very high level overview of it, as I'm
not sure I'll have time to dig much deeper in 6/6 today.]

On Jul 02 2025, Derek J. Clark wrote:
> This series adds initial support for the Legion Go S's built-in
> controller HID configuration interface. In the first patch a new HID
> uevent property is added, HID_FIRMWARE_VERSION, so as to permit fwupd
> to read the firmware version of the HID interface without detaching the
> kernel driver.

That immediately raise red flags on my side. HID_FIRMWARE_VERSION will
likely be used only for this new driver, and that means a special case
in each and every client.

We had to deal with firmware versions in the past in the HID drivers,
and we ended up relying on the uniq field of the hid_device (because the
serial+firmware version uniquely identify the device).

> The second patch adds the ability for an hid_driver to
> assign new/arbitrary uevent properties for static data that doesn't
> benefit from having a sysfs entry.

That, in my mind, is even worse (for the reasons above).

> The third patch adds the VID and PID
> for the Lenovo Legion Go S MCU. 

Which shouldn't be in its own patch, but part of the driver initial
patch.

> The fourth patch adds ABI documentation
> for the config interface introduced in the final patch. The fifth patch
> introduces the core lenovo-legos-hid driver which acts as a routing
> interface for the different endpoints. 

That "core" patch is IMO useless. All it does is:
- check for the USB endpoint (but in the wrong way, because if you
	insert a device through uhid with the same PID/VID it will crash)
- replace the HID-core core functions with the same code

Really, this should be squashed into the next patch (with 3/6 then).

Also, why adding a new subdirectory? All the hid drivers are flat in the
drivers/hid/ directory, and the subdirs are for transport layers. There
is one exception for the surface driver but I don't see why you need
such an exception (yeah, the code is big, but what's the difference in
having a 1500 lines of code source in its own subdir vs at the root?)

> The sixth path introduces the 
> config lenovo-legos-hid driver wich uses both the HID_FIRMWARE_VERSION
> as well as arbitrary uevent properties. Additional interfaces and config
> properties are planned to be added in a future series.

That one is too big for my liking. Generally speaking, a commit
descrition which says "this does this and that" can be split into 2
patches at least :)

What kind of future interfaces and config properties are you planning?

> 
> Patch 6 introduces a checkpatch WARNING that I'm unable to resolve:
> WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> 1292: FILE: drivers/hid/lenovo-legos-hid/lenovo-legos-hid-config.c:1085:
> +       case -ENOSYS: /* during rmmod -ENOSYS is expected */

We can losely waive those while merging. We do it quite often actually.

But trying to minimize checkpatch warnings is a good thing, so thanks
for that.

> 
> This error handling case was added as it is experienced in the real world
> when the driver is rmmod. The LED subsystem produces this error code in
> its legacy code and this is not a new novel use of -ENOSYS, we are simply
> catching the case to avoid spurious errors in dmesg when the driver is
> removed. If there is a way to prevent this error from being triggered by
> checkpatch in the first place, that would be an ideal remedy, but I'm not
> aware how that can be done at this time.

Again, nothing to worry about.

Cheers,
Benjamin

> 
> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
> 
> 
> Derek J. Clark (4):
>   HID: Add Legion Go S ID's
>   HID: Add documentation for lenovo-legos-hid driver
>   HID: Add lenovo-legos-hid core
>   HID: Add lenovo-legos-hid configuration endpoint interface
> 
> Mario Limonciello (2):
>   HID: Include firmware version in the uevent
>   HID: Allow HID drivers to add more uevent variables
> 
>  .../ABI/testing/sysfs-driver-lenovo-legos-hid |  269 +++
>  MAINTAINERS                                   |    7 +
>  drivers/hid/Kconfig                           |    2 +
>  drivers/hid/Makefile                          |    2 +
>  drivers/hid/hid-core.c                        |   11 +
>  drivers/hid/hid-ids.h                         |    4 +
>  drivers/hid/lenovo-legos-hid/Kconfig          |   11 +
>  drivers/hid/lenovo-legos-hid/Makefile         |    6 +
>  drivers/hid/lenovo-legos-hid/config.c         | 1518 +++++++++++++++++
>  drivers/hid/lenovo-legos-hid/config.h         |   19 +
>  drivers/hid/lenovo-legos-hid/core.c           |  122 ++
>  drivers/hid/lenovo-legos-hid/core.h           |   25 +
>  include/linux/hid.h                           |    2 +
>  13 files changed, 1998 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-lenovo-legos-hid
>  create mode 100644 drivers/hid/lenovo-legos-hid/Kconfig
>  create mode 100644 drivers/hid/lenovo-legos-hid/Makefile
>  create mode 100644 drivers/hid/lenovo-legos-hid/config.c
>  create mode 100644 drivers/hid/lenovo-legos-hid/config.h
>  create mode 100644 drivers/hid/lenovo-legos-hid/core.c
>  create mode 100644 drivers/hid/lenovo-legos-hid/core.h
> 
> -- 
> 2.50.0
> 

Powered by blists - more mailing lists