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. 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. The third patch adds the VID and PID
for the Lenovo Legion Go S MCU. 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. 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.

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 */

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.

Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.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

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@gmail.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
>

On July 3, 2025 6:48:00 AM PDT, Benjamin Tissoires <bentiss@kernel.org> wrote:
>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).
>
Hi Benjamin,

Sorry abtthe late reply. Travel & holidays have me a bit behind.

I'll let Mario and Richard hash out the specifics on these points. I'll just add a bit of context to why they're in this series. Prior to this, the fwupd plugin would disconnect this driver to query this information to see if there was an available update. As this can be triggered by a system daemon during gameplay that's not a reasonable expectation. Originally we had these as sysfs entries, and returning to them, would be simple enough, but we felt like this is a fairly standard piece of information that should be available. I wasn't aware of the uniq property being used for this historically, but from an outside looking in perspective this seems a bit convoluted. Apart from just being unintuitive if you're not familiar, userspace is going to need bespoke ways to interpret this anyway as serial numbers and firmware formatting are not consistent between manufacturers.

I'll wait to adjust this until a more thorough discussion has taken place.

>> 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.

I can do that. My reasoning was simply that if another patch becomes reliant on the VID and you needed to revert the other patches for any reason there would be a conflict.

>> 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

Can you point me to a better way?

This series only implements the config endpoint. ATM the gamepad, touchpad, and IMU are combined into a single Steam Deck like interface in root level userspace as a uhid device. I do have some plans for how to do this in the kernel instead, but the proposal isn't ready yet so I need to keep the hidraw devices available to userspace that aren't implemented yet for backwards compatibility.

>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?)

Sure, I can change it to a single file if that's preferable in this subsystem. This is my first foray into kernel HID drivers so I'm not super familiar with the style preferences yet. Breaking everything up by logical subset made sense to me but I'm not married to it. There

>> 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 :)

I figured, but I wasn't sure how you'd prefer I break it up. I was thinking that one patch per attribute group would make sense but wanted some feedback before I did that to avoid going down the wrong path with them.

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

The MCU has a gamepad interface that is more complete than what the xpad driver uses, which includes some back paddles as well as native gyro data which is passed through from the IMU. There's also a separate IMU endpoint so there are a couple options how this can be used. My thoughts are that a sysfs attribute could toggle if gyro is added to one of the joysticks using the IMU data included in the gamepad report, and the external one could be exposed as a motion sensor with the same uniq. Then userspace can determine how to use it.

The touchpad works in both abs and rel modes with the default kernel implementations but additional functionality can be gained through Steam Input if this is integrated into the gamepad.

As far as additional attributes for the config interface, there is hardware level button remapping and calibration for all axes that need to be added but weren't considered critical for initial support from Lenovo.

>> 
>> 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.

Cool, I'll keep a brief note for posterity. 

Thanks,
Derek

>> 
>> 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@gmail.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
>> 

On 7/3/25 09:48, Benjamin Tissoires wrote:
> 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.]

I'll touch on my two patches at the front of the series and let Derek 
get to the questions/comments on the later ones.

> 
> 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.

Actually Richard and I had envisioned that all updatable HID devices 
would start exporting their firmware version through this HID property.
lenovo-legos-hid was just the first.

The idea would then be that userspace software like fwupd would know to 
parse this property to show the current version and never need to 
interrogate the device directly unless it was actually being updated.

> 
> 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).

I think this is a different case.  We don't care so much about the 
unique identification of the device as much as we care about the stream 
of firmware applied to the device.

If HID_UNIQ is the right way to get the firmware version but some 
drivers encode a serial+firmware and others encode firmware that's going 
to make for a very messy "generic" property to read the firmware version 
of a 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).

Do clients actually need to know about all the properties?  My thought 
was that if a client encounters a property it doesn't care about it can 
just ignore it.

If that's misplaced; what would you prefer for all this static 
information?  A pile of sysfs files?

> 
>> 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@gmail.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
>>

On Friday, July 4th, 2025 at 2:58 AM, Mario Limonciello <superm1@kernel.org> wrote:
> If HID_UNIQ is the right way to get the firmware version but some
> drivers encode a serial+firmware and others encode firmware that's going
> to make for a very messy "generic" property to read the firmware version
> of a device.

I think I've also changed my mind on HID_UNIQ -- IIUC the whole point of HID_UNIQ is to be unique comparing devices with the same VID&PID. If I plug in two identical devices with the same firmware version then HID_UNIQ would be the same -- so I think HID_UNIQ should probably always be the serial number. I think HID_UNIQ should also stay the same during the firmware update too, so we don't want to jam two things into one property like "HID_UNIQ=serial:1234,fwver=12.34"

Certainly exposing the firmware version as a HID property makes enumerating devices much easier in fwupd; the kernel driver often just knows the firmware version already and for userspace to re-query the firmware version using a per-device custom protocol seems pointless at best and dangerous at worse -- given we're typically doing a SetReport and GetReport with no sequence number available.

I don't have much to comment on wrt the implementation, but providing a way for the hidraw kernel driver to export the current firmware version up to userspace makes a lot of sense to me.

Richard