[PATCH v2 0/6] HID: hid-lenovo-go: fix controller configuration

[PATCH v2 0/6] HID: hid-lenovo-go: fix controller configuration

[Aditya Dash]

This series fixes several issues in the controller configuration and
calibration paths in hid-lenovo-go.

1/6 fixes the calibration attributes, where the command and sub-command
are swapped. 2/6 fixes two right-controller attributes: imu_enabled
selects the bypass feature, and reset targets the left controller.

Before 3/6, any decoded configuration reply completes the shared
completion. The driver then ignores the result of the wait, so an
unrelated reply, a firmware error, an interrupted wait, or a timeout can
still be reported as success.

3/6 tracks one pending request in the driver data and matches replies by
command ID, command, sub-command, and device. It also returns request
errors to the caller.

The firmware has no sequence number, so a late reply with the same tuple
can still match a newer request.

The remaining patches fix calibration status handling.

The calibration status table defines 0x00 as unknown, 0x01 as success,
and 0x02 as failure. A Legion Go 1 returned 0x08 after a Stop was sent
while no calibration was active. The driver stores the raw value as a
status-text index, so a later read returns -EINVAL.

4/6 preserves the defined values and maps larger status values to
failure.

5/6 rejects 'unknown' as an action. The action table contains it at index
zero, but the options attribute advertises only 'start' and 'stop'.

A previous result can also hide a new calibration attempt:

failure -> start -> failure

The status appears unchanged for the whole operation. 6/6 clears the
selected status to unknown before submitting a Start. Stop leaves the
status unchanged.

---
Changes in v2:
- Drop the FPS DPI patch; the existing request is correct.
- Drop the v1 Boolean calibration-result mapping. Preserve values 0x00
  through 0x02 and map only larger failure values.
- Move the existing pending command state into the driver data and
  initialize it in cfg_probe().
- Use scoped IRQ-save guards and validate the report size and ID together.
- Drop the v1 no-wait calibration path. Correct requests receive their
  matching SET replies within the existing 50 ms timeout.
- Reject 'unknown' and clear only the selected status on Start.

v1: https://lore.kernel.org/all/20260821214810.87826-1-mradityadash@gmail.com/


Aditya Dash (6):
  HID: hid-lenovo-go: use the correct calibration commands
  HID: hid-lenovo-go: use the right controller selectors
  HID: hid-lenovo-go: return configuration request errors
  HID: hid-lenovo-go: normalize calibration failure status
  HID: hid-lenovo-go: reject unknown calibration action
  HID: hid-lenovo-go: clear calibration status on start

 drivers/hid/hid-lenovo-go.c | 179 +++++++++++++++++++++++++++---------
 1 file changed, 136 insertions(+), 43 deletions(-)


base-commit: 1292bca0f8d835d2ad96d309595b2e97f3106d3d
-- 
2.55.0

[PATCH v2 1/6] HID: hid-lenovo-go: use the correct calibration commands

[Aditya Dash]

The calibration attributes pass the calibration operation as the command
and the configuration command as the sub-command. This sends each request
with the two fields reversed.

Swap the command and sub-command values for all six calibration attributes.
For example, the malformed left joystick Start request is:

  05 00 04 0c 03 01 ...

The corrected request is:

  05 00 0c 04 03 01 ...

The bytes identify the output report, configuration group, command,
sub-command, left controller, and Start action, in that order.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 318b1152ff8b..b8248bf8965c 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -1911,18 +1911,18 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification",
 static DEVICE_ATTR_RO_NAMED(rumble_notification_left_index,
 			    "rumble_notification_index");
 
-static struct go_cfg_attr cal_trigg_left = { TRIGGER_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", SET_TRIGGER_CFG,
+static struct go_cfg_attr cal_trigg_left = { SET_TRIGGER_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", TRIGGER_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_trigg_left_index, "calibrate_trigger_index");
 
-static struct go_cfg_attr cal_joy_left = { JOYSTICK_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_joy_left, "calibrate_joystick", SET_JOYSTICK_CFG,
+static struct go_cfg_attr cal_joy_left = { SET_JOYSTICK_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_joy_left, "calibrate_joystick", JOYSTICK_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_joy_left_index, "calibrate_joystick_index");
 
-static struct go_cfg_attr cal_gyro_left = { GYRO_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_gyro_left, "calibrate_gyro", SET_GYRO_CFG,
+static struct go_cfg_attr cal_gyro_left = { SET_GYRO_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_gyro_left, "calibrate_gyro", GYRO_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_gyro_left_index, "calibrate_gyro_index");
 
@@ -2018,18 +2018,18 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_right, "rumble_notification",
 static DEVICE_ATTR_RO_NAMED(rumble_notification_right_index,
 			    "rumble_notification_index");
 
-static struct go_cfg_attr cal_trigg_right = { TRIGGER_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_trigg_right, "calibrate_trigger", SET_TRIGGER_CFG,
+static struct go_cfg_attr cal_trigg_right = { SET_TRIGGER_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_trigg_right, "calibrate_trigger", TRIGGER_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_trigg_right_index, "calibrate_trigger_index");
 
-static struct go_cfg_attr cal_joy_right = { JOYSTICK_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_joy_right, "calibrate_joystick", SET_JOYSTICK_CFG,
+static struct go_cfg_attr cal_joy_right = { SET_JOYSTICK_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_joy_right, "calibrate_joystick", JOYSTICK_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_joy_right_index, "calibrate_joystick_index");
 
-static struct go_cfg_attr cal_gyro_right = { GYRO_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_gyro_right, "calibrate_gyro", SET_GYRO_CFG,
+static struct go_cfg_attr cal_gyro_right = { SET_GYRO_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_gyro_right, "calibrate_gyro", GYRO_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_gyro_right_index, "calibrate_gyro_index");
 
-- 
2.55.0

Re: [PATCH v2 1/6] HID: hid-lenovo-go: use the correct calibration commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Early return in remove() skips hid_hw_stop(), leading to Use-After-Free.
- [Critical] Singleton global state prevents multi-device support and causes data corruption.
- [High] Missing hid_hw_stop() and hid_hw_close() on probe error path.
- [High] Sysfs attribute groups are leaked on probe error paths.
- [High] ABBA deadlock between sysfs active references and cfg_mutex during removal.
--

commit 087e7a307d94a2ccdf00bf5d59ce9cddc609d002
Author: Aditya Dash <mradityadash@gmail.com>

    HID: hid-lenovo-go: use the correct calibration commands
    
    The calibration attributes pass the calibration operation as the command
    and the configuration command as the sub-command. This sends each request
    with the two fields reversed.
    
    Swap the command and sub-command values for all six calibration attributes.
    For example, the malformed left joystick Start request is:
    
      05 00 04 0c 03 01 ...
    
    The corrected request is:
    
      05 00 0c 04 03 01 ...

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 318b1152ff8ba..b8248bf8965c6 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
[ ... ]
> 	ret = hid_go_cfg_probe(hdev, id);
> 	if (ret)
> 		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");
> 
> 	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);
> 
> 	return ret;
> }

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_probe() leak
hardware resources?

If hid_go_cfg_probe() fails, the function logs the error and returns the
negative error code directly.

Does this bypass the mandatory hid_hw_close() and hid_hw_stop() teardown calls,
leaving the device in an active state despite a failed probe?

> static void hid_go_remove(struct hid_device *hdev)
> {
> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;

[Severity: Critical]
This isn't a bug introduced by this patch, but does this early return in
hid_go_remove() skip driver teardown steps?

If a malformed USB device without endpoints is probed, get_endpoint_address()
returns an error. If hid_go_probe() successfully binds the device as a generic
HID device, then during driver unbind, get_endpoint_address() again returns an
error.

Does this cause hid_go_remove() to hit the early return and exit immediately,
skipping hid_hw_stop() and leaving dangling input interfaces active while
devres releases the driver's underlying memory?

> 	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
> 	if (ret) {
> 		dev_err_probe(&hdev->dev, ret,
> 			      "Failed to create gamepad configuration attributes\n");
> 		return ret;
> 	}
> 
> 	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
> 	if (ret) {
> 		dev_err_probe(&hdev->dev, ret, "Failed to create RGB device\n");
> 		return ret;
> 	}

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_cfg_probe()
leak sysfs attributes?

If a subsequent initialization step like devm_led_classdev_multicolor_register()
fails, the function returns immediately without calling sysfs_remove_groups().

Could accessing these dangling sysfs nodes after module unload result in a
Use-After-Free panic?

> 	u32 tx_dongle_version_hardware;
> 	u32 tx_dongle_version_product;
> 	u32 tx_dongle_version_protocol;
> } drvdata;
[ ... ]
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;
> 	mutex_init(&drvdata.cfg_mutex);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this static global
structure prevent multi-device support and corrupt state on concurrent probes?

The driver stores all per-device state, including the hdev pointer and
cfg_mutex, in a single static global struct named drvdata.

If a second Lenovo Legion Go device is plugged in, its probe sequence in
hid_go_cfg_probe() overwrites the global hdev pointer and calls
mutex_init(&drvdata.cfg_mutex) on an active lock.

Does this lead to memory corruption and unpredictable driver behavior for all
connected devices?

> static void hid_go_cfg_remove(struct hid_device *hdev)
> {
[ ... ]
> 	cancel_delayed_work_sync(&drvdata.go_cfg_setup);
> 
> 	guard(mutex)(&drvdata.cfg_mutex);
> 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
> 	hid_hw_close(hdev);
> 	hid_hw_stop(hdev);

[Severity: High]
This is a pre-existing issue, but does this create an ABBA deadlock between
sysfs active references and cfg_mutex?

hid_go_cfg_remove() first acquires cfg_mutex and then calls
sysfs_remove_groups(), which internally blocks until all active sysfs read
and write operations complete.

Concurrently, a sysfs read first takes the kernfs active reference and then
calls mcu_property_out(), which attempts to lock cfg_mutex.

Does this establish a circular locking dependency that freezes the driver
unbind process and indefinitely hangs any tasks attempting to interact with
the device's sysfs files?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788096666.git.mradityadash@gmail.com?part=1

[PATCH v2 2/6] HID: hid-lenovo-go: use the right controller selectors

[Aditya Dash]

The right imu_enabled attribute selects FEATURE_IMU_BYPASS, and the right
reset attribute targets the left controller.

Select FEATURE_IMU_ENABLE for the right imu_enabled attribute. Target the
right controller for a right reset.

Fixes: 82cd9bc866e1 ("HID: hid-lenovo-go: Add Feature Status Attributes")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index b8248bf8965c..b937d11dd6f6 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -1999,13 +1999,13 @@ LEGO_DEVICE_ATTR_RW(imu_bypass_right, "imu_bypass_enabled", RIGHT_CONTROLLER,
 		    index, feature_status);
 static DEVICE_ATTR_RO_NAMED(imu_bypass_right_index, "imu_bypass_enabled_index");
 
-static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_BYPASS };
+static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_ENABLE };
 LEGO_DEVICE_ATTR_RW(imu_enabled_right, "imu_enabled", RIGHT_CONTROLLER, index,
 		    feature_status);
 static DEVICE_ATTR_RO_NAMED(imu_enabled_right_index, "imu_enabled_index");
 
 static struct go_cfg_attr reset_right = { FEATURE_RESET_GAMEPAD };
-LEGO_DEVICE_ATTR_WO(reset_right, "reset", LEFT_CONTROLLER, feature_status);
+LEGO_DEVICE_ATTR_WO(reset_right, "reset", RIGHT_CONTROLLER, feature_status);
 
 static struct go_cfg_attr rumble_mode_right = { RUMBLE_MODE };
 LEGO_DEVICE_ATTR_RW(rumble_mode_right, "rumble_mode", RIGHT_CONTROLLER, index,
-- 
2.55.0

Re: [PATCH v2 2/6] HID: hid-lenovo-go: use the right controller selectors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in probe error paths.
- [High] Missing hardware cleanup in remove() due to early return.
- [High] Global singleton used for per-device state, causing corruption and Use-After-Free with multiple devices.
- [High] Leaked sysfs attribute groups on probe failure.
- [High] Deadlock during device unbind due to lock ordering with sysfs_remove_groups.
--

commit c2d6b2210c5ca6d1fa356349f3cbde71b00ce595
Author: Aditya Dash <mradityadash@gmail.com>
Subject: HID: hid-lenovo-go: use the right controller selectors

This commit fixes the attribute selectors for the right controller on Lenovo
Go gamepads. It updates the imu_enabled_right attribute to select
FEATURE_IMU_ENABLE and ensures the reset_right attribute targets the right
controller.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b8248bf8965c6..b937d11dd6f6a 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1999,13 +1999,13 @@ LEGO_DEVICE_ATTR_RW(imu_bypass_right, "imu_bypass_enabled", RIGHT_CONTROLLER,

[ ... ]

> static int hid_go_cfg_probe(struct hid_device *hdev,
> 			    const struct hid_device_id *_id)
> {
> 	unsigned char *buf;
> 	int ret;
> 
> 	buf = devm_kzalloc(&hdev->dev, GO_PACKET_SIZE, GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;
> 	mutex_init(&drvdata.cfg_mutex);

[Severity: High]
This is a pre-existing issue, but does using a global singleton for instance-
specific state break the driver model if multiple devices are plugged in?

Since drvdata is globally allocated in drivers/hid/hid-lenovo-go.c, setting
drvdata.hdev here in hid_go_cfg_probe() will clobber the pointer from any
previously probed device.

> 
> 	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
> 	if (ret) {
> 		dev_err_probe(&hdev->dev, ret,
> 			      "Failed to create gamepad configuration attributes\n");
> 		return ret;
> 	}
> 
> 	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);

[Severity: High]
This isn't a bug introduced by this patch, but does this corrupt the LED
subsystem lists?

Since go_cdev_rgb is a statically allocated global struct, registering the
same object in hid_go_cfg_probe() for every device instance could corrupt
the lists if multiple devices are present.

> 	if (ret) {
> 		dev_err_probe(&hdev->dev, ret, "Failed to create RGB device\n");
> 		return ret;

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_cfg_probe()
leave sysfs attributes behind?

If devm_led_classdev_multicolor_register() fails, this returns immediately
without calling sysfs_remove_groups(). While devres will free the driver's
memory, the sysfs attributes remain active and reference freed memory,
which could cause a use-after-free if userspace interacts with them.

> 	}

[ ... ]

> static void hid_go_cfg_remove(struct hid_device *hdev)
> {
> 	/*
> 	 * cfg_setup is scheduled from hid_go_cfg_probe() with a 2 ms delay
> 	 * and dereferences drvdata.hdev.  Drain it here before tearing
> 	 * down so the workqueue cannot run after hid_destroy_device()'s
> 	 * put_device() has released the underlying hdev and dereference
> 	 * a stale drvdata.hdev pointer.
> 	 */
> 	cancel_delayed_work_sync(&drvdata.go_cfg_setup);
> 
> 	guard(mutex)(&drvdata.cfg_mutex);
> 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);

[Severity: High]
This isn't a bug introduced by this patch, but can this lock ordering in
hid_go_cfg_remove() cause a deadlock during device unbind?

This holds drvdata.cfg_mutex while synchronously waiting for
sysfs_remove_groups() to finish. If userspace is concurrently writing to a
sysfs attribute, that write path (e.g., mcu_property_out()) will attempt
to acquire drvdata.cfg_mutex, leading to an ABBA deadlock that freezes the
kernel thread.

> 	hid_hw_close(hdev);
> 	hid_hw_stop(hdev);
> }

[ ... ]

> 	ep = get_endpoint_address(hdev);
> 	if (ep != GO_GP_INTF_IN) {
> 		dev_dbg(&hdev->dev, "Started interface %x as generic HID device\n", ep);
> 		return 0;
> 	}
> 
> 	ret = hid_go_cfg_probe(hdev, id);
> 	if (ret)
> 		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");
> 
> 	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);
> 
> 	return ret;

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_probe()
fail to tear down the hardware?

If hid_go_cfg_probe() fails, the error code is immediately returned without
calling hid_hw_stop() or hid_hw_close(). The hardware is left running while
driver private data is freed by devres, which will trigger a use-after-free
when subsequent input events arrive.

> }
> 
> static void hid_go_remove(struct hid_device *hdev)
> {
> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;

[Severity: High]
This isn't a bug introduced by this patch, but does this early return in
hid_go_remove() skip mandatory hardware cleanup?

If get_endpoint_address() fails during device unplug, it hits this early
return and bypasses hid_hw_close() and hid_hw_stop(). Input interrupts
will keep firing into freed driver structures.

> 
> 	switch (ep) {
> 	case GO_GP_INTF_IN:
> 		hid_go_cfg_remove(hdev);
> 		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788096666.git.mradityadash@gmail.com?part=2

[PATCH v2 3/6] HID: hid-lenovo-go: return configuration request errors

[Aditya Dash]

Any decoded configuration reply completes the shared completion. The driver
also ignores the result of its interruptible wait. An unrelated reply, a
firmware error, an interrupted wait, or a timeout can therefore be reported
as success.

Add pending command state to the driver data. Match replies by command ID,
command, sub-command, and device, and return request errors to the caller.
Clear the pending request after an interruption or timeout so a late reply
cannot replace that result.

Keep the existing 50 ms wait for every configuration request. Corrected
calibration requests on an Original Legion Go received their matching SET
replies within 8 ms. The final calibration result remains asynchronous.

The firmware has no sequence number. A late reply with the same tuple can
still match a newer request.

Fixes: d69ccfcbc955 ("HID: hid-lenovo-go: Add Lenovo Legion Go Series HID Driver")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 107 +++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index b937d11dd6f6..dbfacbb70394 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -21,6 +21,7 @@
 #include <linux/led-class-multicolor.h>
 #include <linux/mutex.h>
 #include <linux/printk.h>
+#include <linux/spinlock.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
 #include <linux/unaligned.h>
@@ -31,13 +32,27 @@
 #include "hid-ids.h"
 
 #define GO_GP_INTF_IN		0x83
+#define GO_INPUT_REPORT_ID	0x04
 #define GO_OUTPUT_REPORT_ID	0x05
 #define GO_GP_RESET_SUCCESS	0x01
 #define GO_PACKET_SIZE		64
+#define GO_COMMAND_TIMEOUT_MS	50
+
+/* Lenovo replies identify a command and a sub-command, but have no sequence. */
+struct hid_go_cmd {
+	struct completion done;
+	spinlock_t lock; /* protects fields below */
+	bool pending;
+	u8 id;
+	u8 command;
+	u8 sub_command;
+	u8 device;
+	int result;
+};
 
 static struct hid_go_cfg {
 	struct delayed_work go_cfg_setup;
-	struct completion send_cmd_complete;
+	struct hid_go_cmd cmd;
 	struct led_classdev *led_cdev;
 	struct hid_device *hdev;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
@@ -331,6 +346,62 @@ static const char *const os_mode_text[] = {
 	[LINUX] = "linux",
 };
 
+static void hid_go_cmd_arm(u8 id, u8 command, u8 sub_command, u8 device)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	reinit_completion(&drvdata.cmd.done);
+	drvdata.cmd.pending = true;
+	drvdata.cmd.id = id;
+	drvdata.cmd.command = command;
+	drvdata.cmd.sub_command = sub_command;
+	drvdata.cmd.device = device;
+}
+
+static void hid_go_cmd_consume(const struct command_report *cmd_rep, int result)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	if (drvdata.cmd.pending && cmd_rep->id == drvdata.cmd.id &&
+	    cmd_rep->cmd == drvdata.cmd.command &&
+	    cmd_rep->sub_cmd == drvdata.cmd.sub_command &&
+	    cmd_rep->device_type == drvdata.cmd.device) {
+		drvdata.cmd.pending = false;
+		drvdata.cmd.result = result;
+		complete(&drvdata.cmd.done);
+	}
+}
+
+static int hid_go_cmd_finish(long wait_result)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	if (wait_result <= 0) {
+		drvdata.cmd.pending = false;
+		return wait_result < 0 ? wait_result : -ETIMEDOUT;
+	}
+
+	return drvdata.cmd.result;
+}
+
+static int hid_go_cmd_cancel(int result)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	drvdata.cmd.pending = false;
+	return result;
+}
+
+static int hid_go_send_output_report(struct hid_device *hdev, u8 *packet)
+{
+	int ret;
+
+	ret = hid_hw_output_report(hdev, packet, GO_PACKET_SIZE);
+	if (ret < 0)
+		return ret;
+	return ret == GO_PACKET_SIZE ? 0 : -EINVAL;
+}
+
 static int hid_go_version_event(struct command_report *cmd_rep)
 {
 	switch (cmd_rep->sub_cmd) {
@@ -654,7 +725,7 @@ static int hid_go_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct command_report *cmd_rep;
 	int ep, ret;
 
-	if (size != GO_PACKET_SIZE)
+	if (size != GO_PACKET_SIZE || data[0] != GO_INPUT_REPORT_ID)
 		goto passthrough;
 
 	ep = get_endpoint_address(hdev);
@@ -707,7 +778,7 @@ static int hid_go_raw_event(struct hid_device *hdev, struct hid_report *report,
 	dev_dbg(&hdev->dev, "Rx data as raw input report: [%*ph]\n",
 		GO_PACKET_SIZE, data);
 
-	complete(&drvdata.send_cmd_complete);
+	hid_go_cmd_consume(cmd_rep, ret);
 	return ret;
 
 passthrough:
@@ -722,7 +793,8 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
 	unsigned char *dmabuf __free(kfree) = NULL;
 	u8 header[] = { GO_OUTPUT_REPORT_ID, id, command, index, device };
 	size_t header_size = ARRAY_SIZE(header);
-	int timeout = 50;
+	unsigned long timeout = msecs_to_jiffies(GO_COMMAND_TIMEOUT_MS);
+	long wait_result;
 	int ret;
 
 	if (header_size + len > GO_PACKET_SIZE)
@@ -740,22 +812,14 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
 	dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
 		GO_PACKET_SIZE, dmabuf);
 
-	ret = hid_hw_output_report(hdev, dmabuf, GO_PACKET_SIZE);
-	if (ret < 0)
-		return ret;
-
-	ret = ret == GO_PACKET_SIZE ? 0 : -EINVAL;
+	hid_go_cmd_arm(id, command, index, device);
+	ret = hid_go_send_output_report(hdev, dmabuf);
 	if (ret)
-		return ret;
-
-	ret = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
-							msecs_to_jiffies(timeout));
-
-	if (ret == 0) /* timeout occurred */
-		ret = -EBUSY;
+		return hid_go_cmd_cancel(ret);
 
-	reinit_completion(&drvdata.send_cmd_complete);
-	return 0;
+	wait_result = wait_for_completion_interruptible_timeout(&drvdata.cmd.done,
+								timeout);
+	return hid_go_cmd_finish(wait_result);
 }
 
 static ssize_t version_show(struct device *dev, struct device_attribute *attr,
@@ -2362,9 +2426,12 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
 	if (!buf)
 		return -ENOMEM;
 
+	mutex_init(&drvdata.cfg_mutex);
+	init_completion(&drvdata.cmd.done);
+	spin_lock_init(&drvdata.cmd.lock);
+	drvdata.cmd.pending = false;
 	hid_set_drvdata(hdev, &drvdata);
 	drvdata.hdev = hdev;
-	mutex_init(&drvdata.cfg_mutex);
 
 	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
 	if (ret) {
@@ -2388,8 +2455,6 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
 
 	drvdata.led_cdev = &go_cdev_rgb.led_cdev;
 
-	init_completion(&drvdata.send_cmd_complete);
-
 	/* Executing calls prior to returning from probe will lock the MCU. Schedule
 	 * initial data call after probe has completed and MCU can accept calls.
 	 */
-- 
2.55.0

Re: [PATCH v2 3/6] HID: hid-lenovo-go: return configuration request errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The newly added spinlock and completion are accessed by the raw event handler before they are initialized, causing a potential kernel crash.

Pre-existing issues:
- [High] Missing cleanup on probe failure leaves the hardware started, the device open, and sysfs groups leaked.
- [High] Missing hardware stop in remove for devices with no endpoints.
- [High] Global static `drvdata` structure causes state corruption and kernel panics when multiple devices are connected.
- [High] ABBA deadlock between device removal and sysfs attribute stores.
- [High] Possible out-of-bounds array access in get_endpoint_address if USB interface has no endpoints.
--

commit 0d711ce676d4cb403114e39243d7e14be625eec6
Author: Aditya Dash <mradityadash@gmail.com>
Subject: HID: hid-lenovo-go: return configuration request errors

This commit adds a mechanism to track pending command state and correctly
match configuration replies by command ID, sub-command, and device. It
ensures that unrelated replies, errors, timeouts, or interrupted waits are
properly reported as errors to the caller rather than silently returning
success.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b937d11dd6f6a..dbfacbb70394b 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -31,13 +32,27 @@
>  #include "hid-ids.h"
>  
>  #define GO_GP_INTF_IN		0x83
> +#define GO_INPUT_REPORT_ID	0x04
> +#define GO_OUTPUT_REPORT_ID	0x05
>  #define GO_GP_RESET_SUCCESS	0x01
>  #define GO_PACKET_SIZE		64
> +#define GO_COMMAND_TIMEOUT_MS	50
> +
> +/* Lenovo replies identify a command and a sub-command, but have no sequence. */
> +struct hid_go_cmd {
> +	struct completion done;
> +	spinlock_t lock; /* protects fields below */
> +	bool pending;
> +	u8 id;
> +	u8 command;
> +	u8 sub_command;
> +	u8 device;
> +	int result;
> +};
>  
>  static struct hid_go_cfg {
>  	struct delayed_work go_cfg_setup;
> -	struct completion send_cmd_complete;
> +	struct hid_go_cmd cmd;
>  	struct led_classdev *led_cdev;
>  	struct hid_device *hdev;

[Severity: High]
This is a pre-existing issue, but the entire driver state is defined as a
single global variable instead of per-device driver data. Does this cause
state corruption or kernel panics when multiple supported devices are
connected simultaneously?

[ ... ]

> @@ -331,6 +346,62 @@ static const char *const os_mode_text[] = {
>  	[LINUX] = "linux",
>  };
>  
> +static void hid_go_cmd_arm(u8 id, u8 command, u8 sub_command, u8 device)
> +{

[ ... ]

> +static void hid_go_cmd_consume(const struct command_report *cmd_rep, int result)
> +{
> +	guard(spinlock_irqsave)(&drvdata.cmd.lock);
> +
> +	if (drvdata.cmd.pending && cmd_rep->id == drvdata.cmd.id &&

[Severity: High]
Can hid_go_raw_event() be triggered by an early interrupt before
hid_go_cfg_probe() runs?

Since hid_hw_start() and hid_hw_open() are called during device probe,
the hardware can generate interrupts before hid_go_cfg_probe() initializes
this spinlock.

If an early interrupt arrives, it will call hid_go_raw_event()
and then hid_go_cmd_consume(), which locks drvdata.cmd.lock.

Locking an uninitialized spinlock will lead to lock corruption or kernel crashes.

[ ... ]

> @@ -654,7 +725,7 @@ static int hid_go_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	struct command_report *cmd_rep;
>  	int ep, ret;
>  
> -	if (size != GO_PACKET_SIZE)
> +	if (size != GO_PACKET_SIZE || data[0] != GO_INPUT_REPORT_ID)
>  		goto passthrough;
>  
>  	ep = get_endpoint_address(hdev);

[Severity: High]
This is a pre-existing issue, but does get_endpoint_address() safely handle
USB interfaces with zero endpoints?

In get_endpoint_address():

    ep = intf->cur_altsetting->endpoint;
    if (!ep)
        return -ENODEV;
    return ep->desc.bEndpointAddress;

If a malformed USB interface reports 0 endpoints (bNumEndpoints == 0), the
endpoint array might be a 0-length array. The if (!ep) check only verifies
the pointer isn't NULL, but accessing ep->desc.bEndpointAddress on an empty
array could read memory out of bounds.

[ ... ]

> @@ -2362,9 +2426,12 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	mutex_init(&drvdata.cfg_mutex);
> +	init_completion(&drvdata.cmd.done);
> +	spin_lock_init(&drvdata.cmd.lock);
> +	drvdata.cmd.pending = false;
>  	hid_set_drvdata(hdev, &drvdata);
>  	drvdata.hdev = hdev;
> -	mutex_init(&drvdata.cfg_mutex);

[Severity: High]
This is a pre-existing issue, but this unconditionally re-initializes global
synchronization primitives (and the newly added drvdata.cmd.lock and
drvdata.cmd.done).

If a second device is plugged in while the first is active, will this corrupt
the wait queues and lock states of the first device?

[ ... ]

> @@ -2388,8 +2455,6 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>  
>  	drvdata.led_cdev = &go_cdev_rgb.led_cdev;
>  
> -	init_completion(&drvdata.send_cmd_complete);
> -
>  	/* Executing calls prior to returning from probe will lock the MCU. Schedule
>  	 * initial data call after probe has completed and MCU can accept calls.
>  	 */

[Severity: High]
This is a pre-existing issue, but does failing here or later in hid_go_probe()
leave the hardware running and resources leaked?

If schedule_delayed_work() fails (or if hid_go_cfg_probe() fails earlier),
the error is returned but the error paths do not call hid_hw_stop(),
hid_hw_close(), or clean up the manually created sysfs groups.

This leaves the hardware in an active state and leaks resources upon device
removal.


I also noticed a few other pre-existing issues in the driver outside the scope
of this patch:

[Severity: High]
This is a pre-existing issue, but does hid_go_remove() safely tear down
the hardware for all devices?

In hid_go_remove():

    int ep = get_endpoint_address(hdev);

    if (ep <= 0)
        return;

If get_endpoint_address() fails and returns <= 0, the function immediately
returns without calling hid_hw_close() and hid_hw_stop(). This prevents
the driver from tearing down the hardware properly, leaving active interfaces
unrestored and leaking open counts when the device is unregistered.

[Severity: High]
This is a pre-existing issue, but can hid_go_cfg_remove() deadlock with
concurrent sysfs operations?

In hid_go_cfg_remove():

    guard(mutex)(&drvdata.cfg_mutex);
    sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);

If userspace writes to a sysfs attribute, kernfs acquires an active reference
for the file and calls mcu_property_out(), which blocks waiting for
drvdata.cfg_mutex. During device removal, hid_go_cfg_remove() acquires
drvdata.cfg_mutex first and then calls sysfs_remove_groups(), which blocks
waiting for all active kernfs references to drain, causing an ABBA deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788096666.git.mradityadash@gmail.com?part=3

[PATCH v2 4/6] HID: hid-lenovo-go: normalize calibration failure status

[Aditya Dash]

The driver stores the firmware result byte as an index into the
calibration status text. A Legion Go 1 returned 0x08 after an idle Stop.
The status table has only three entries, so a later read returns -EINVAL.

Keep the defined values 0x00 through 0x02. Treat larger result values as
failure before storing them.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index dbfacbb70394..c7a2e621a4ad 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -648,17 +648,22 @@ static int hid_go_light_event(struct command_report *cmd_rep)
 
 static int hid_go_device_status_event(struct command_report *cmd_rep)
 {
+	u8 status = cmd_rep->data[1];
+
+	if (status > CAL_STAT_FAILURE)
+		status = CAL_STAT_FAILURE;
+
 	switch (cmd_rep->device_type) {
 	case LEFT_CONTROLLER:
 		switch (cmd_rep->data[0]) {
 		case CALDEV_GYROSCOPE:
-			drvdata.gp_left_gyro_cal_status = cmd_rep->data[1];
+			drvdata.gp_left_gyro_cal_status = status;
 			return 0;
 		case CALDEV_JOYSTICK:
-			drvdata.gp_left_joy_cal_status = cmd_rep->data[1];
+			drvdata.gp_left_joy_cal_status = status;
 			return 0;
 		case CALDEV_TRIGGER:
-			drvdata.gp_left_trigg_cal_status = cmd_rep->data[1];
+			drvdata.gp_left_trigg_cal_status = status;
 			return 0;
 		default:
 			return -EINVAL;
@@ -667,13 +672,13 @@ static int hid_go_device_status_event(struct command_report *cmd_rep)
 	case RIGHT_CONTROLLER:
 		switch (cmd_rep->data[0]) {
 		case CALDEV_GYROSCOPE:
-			drvdata.gp_right_gyro_cal_status = cmd_rep->data[1];
+			drvdata.gp_right_gyro_cal_status = status;
 			return 0;
 		case CALDEV_JOYSTICK:
-			drvdata.gp_right_joy_cal_status = cmd_rep->data[1];
+			drvdata.gp_right_joy_cal_status = status;
 			return 0;
 		case CALDEV_TRIGGER:
-			drvdata.gp_right_trigg_cal_status = cmd_rep->data[1];
+			drvdata.gp_right_trigg_cal_status = status;
 			return 0;
 		default:
 			return -EINVAL;
-- 
2.55.0

Re: [PATCH v2 4/6] HID: hid-lenovo-go: normalize calibration failure status

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The drivers use static global variables for per-device state and LED registration, preventing support for multiple devices and causing state corruption.
- [High] Missing `hid_hw_stop()` and `hid_hw_close()` in probe error paths.
- [High] LED devres unregistration happens after `hid_hw_stop()`, leaving a window for illegal hardware access.
- [High] The LED `brightness_set` callback illegally sleeps.
- [High] Hardware stop leaked during device removal if endpoint address retrieval fails.
--

commit 6f9c422af24bce1a678acc087e7d364da24c0f75
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: normalize calibration failure status

The driver stores the firmware result byte as an index into the
calibration status text. A Legion Go 1 returned 0x08 after an idle Stop.
The status table has only three entries, so a later read returns -EINVAL.

Keep the defined values 0x00 through 0x02. Treat larger result values as
failure before storing them.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index dbfacbb70394b..c7a2e621a4ade 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak
the HID hardware state?

drivers/hid/hid-lenovo-go.c:hid_go_probe() {
	...
	ret = hid_go_cfg_probe(hdev, id);
	if (ret)
		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");

	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);

	return ret;
}

If hid_go_cfg_probe() fails, the function returns an error without unwinding
the started hardware with hid_hw_stop() and hid_hw_close(). Since the custom
remove callback overrides the core's default unbind behavior, could this lead
to a use-after-free?

The hid-lenovo-go-s driver has the identical omission in its nested probe
error path:

drivers/hid/hid-lenovo-go-s.c:hid_gos_probe() {
	...
	ret = hid_gos_cfg_probe(hdev, id);
	if (ret)
		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface");
	...
}

[Severity: Critical]
This is a pre-existing issue, but does using a static global variable for
per-device state prevent support for multiple devices?

drivers/hid/hid-lenovo-go.c:
static struct hid_go_cfg {
	struct delayed_work go_cfg_setup;
	struct hid_go_cmd cmd;
...

If a second Lenovo Go controller is connected, the second probe overwrites
the global drvdata.hdev pointer in hid_go_cfg_probe():

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
	...
	mutex_init(&drvdata.cfg_mutex);
	init_completion(&drvdata.cmd.done);
	spin_lock_init(&drvdata.cmd.lock);
	drvdata.cmd.pending = false;
	hid_set_drvdata(hdev, &drvdata);
	drvdata.hdev = hdev;
	...
	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
	...
}

Does registering the exact same static go_cdev_rgb struct with devres
corrupt the LED core lists and crash the kernel?

[Severity: High]
This is a pre-existing issue, but does manually stopping the hardware here
leave a window for illegal hardware access?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
	...
	hid_hw_close(hdev);
	hid_hw_stop(hdev);
	hid_set_drvdata(hdev, NULL);
	...
}

Because the LED is cleaned up asynchronously by devres after remove()
returns, could a user writing to the LED brightness sysfs file in this
window invoke hid_go_brightness_set(), sending an output report to a
stopped HID device using a dangling drvdata.hdev pointer?

[Severity: High]
This is a pre-existing issue, but does the LED brightness_set callback
illegally sleep?

drivers/hid/hid-lenovo-go.c:hid_go_brightness_set() {
	...
	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
	...
}

The brightness_set LED callback is expected to be non-blocking, but it calls
rgb_cfg_call() which waits on a completion. If the LED is controlled by a
timer trigger, could this callback execute in a softirq/timer context and
cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but does this early return leak the hardware
start state?

drivers/hid/hid-lenovo-go.c:hid_go_remove() {
	...
	int ep = get_endpoint_address(hdev);

	if (ep <= 0)
		return;
	...
}

If a USB HID interface with no endpoints successfully passes probe() by
bypassing the config probe, does returning early here during removal entirely
bypass hid_hw_close() and hid_hw_stop(), leading to unbalanced HID core
reference counts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788096666.git.mradityadash@gmail.com?part=4

[PATCH v2 5/6] HID: hid-lenovo-go: reject unknown calibration action

[Aditya Dash]

The calibration action table includes 'unknown' at index zero. The options
attribute skips that entry and lists only 'start' and 'stop', but the
store searches the full table. Writing 'unknown' therefore sends a request
without an action byte.

Reject CAL_UNKNOWN before building the request. Leave Start and Stop
unchanged.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index c7a2e621a4ad..a4231d188b7a 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -1419,6 +1419,8 @@ static ssize_t calibrate_config_store(struct device *dev,
 	ret = sysfs_match_string(cal_enabled_text, buf);
 	if (ret < 0)
 		return ret;
+	if (ret == CAL_UNKNOWN)
+		return -EINVAL;
 
 	val = ret;
 	if (!val)
-- 
2.55.0

Re: [PATCH v2 5/6] HID: hid-lenovo-go: reject unknown calibration action

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Low] The patch leaves behind dead code (`if (!val) size = 0;`) after explicitly rejecting `CAL_UNKNOWN`.

Pre-existing issues:
- [Critical] The driver incorrectly uses static global structs to store per-device state.
- [High] Missing hid_hw_close() and hid_hw_stop() in hid_go_probe() error paths.
- [High] Early return in hid_go_remove() bypasses hid_hw_stop() cleanup.
- [High] Missing sysfs_remove_groups() on the error paths in hid_go_cfg_probe().
- [High] AB-BA deadlock in hid_go_cfg_remove() with sysfs operations.
- [High] LED brightness_set callback performs sleeping operations.
--

commit a78ed68c3272ef95327340abb6446f933e2d3711
Author: Aditya Dash <mradityadash@gmail.com>
HID: hid-lenovo-go: reject unknown calibration action

This patch explicitly rejects CAL_UNKNOWN in the store handler for the
calibration configuration. It prevents requests without an action byte from
being sent by returning -EINVAL before building the request.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index c7a2e621a4ade..a4231d188b7af 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1419,6 +1419,8 @@ static ssize_t calibrate_config_store(struct device *dev,
>  	ret = sysfs_match_string(cal_enabled_text, buf);
>  	if (ret < 0)
>  		return ret;
> +	if (ret == CAL_UNKNOWN)
> +		return -EINVAL;
>  
>  	val = ret;
>  	if (!val)
>  		size = 0;

[Severity: Low]
Does this change leave behind dead code? Since CAL_UNKNOWN maps to 0 and is now
explicitly rejected above, the subsequent assignment val = ret guarantees that
val is non-zero.

Because of this, if (!val) will evaluate to false under all possible
conditions, making size = 0 dead code.

[ ... ]

> 	ret = hid_go_cfg_probe(hdev, id);
> 	if (ret)
> 		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");
> 
> 	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);
> 
> 	return ret;

[Severity: High]
This isn't a regression introduced by this patch, but can this error path lead
to a resource leak and potential use-after-free?

In hid_go_probe(), if hid_go_cfg_probe() returns an error, the function
immediately returns the error without rolling back prior successful calls to
hid_hw_start() and hid_hw_open(). This leaves the device in an active hardware
state with input interfaces registered, violating HID subsystem lifecycle rules.

[ ... ]

> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;

[Severity: High]
This isn't a regression introduced by this patch, but does this early return
bypass the default case needed to clean up generic HID devices?

If a device without the expected endpoint is unbound, get_endpoint_address()
returns an error (< 0) and this early return in hid_go_remove() triggers.
Omitting hid_hw_stop() here leaves the HID hardware, input devices, and hidraw
nodes active and registered after the driver has unbound.

When userspace interacts with these lingering nodes, it will access freed devres
data, causing a use-after-free.

[ ... ]

> static struct hid_go_cfg {
> ... 
> } drvdata;

[ ... ]

> static struct led_classdev_mc go_cdev_rgb = {

[ ... ]

> 	mutex_init(&drvdata.cfg_mutex);
> 	init_completion(&drvdata.cmd.done);
> 	spin_lock_init(&drvdata.cmd.lock);
> 	drvdata.cmd.pending = false;
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;

[Severity: Critical]
This is a pre-existing issue, but does this incorrectly use static global
structs to store per-device state?

If multiple supported devices are plugged in simultaneously, hid_go_cfg_probe()
is called multiple times and unconditionally re-initializes the global
drvdata.cfg_mutex, drvdata.cmd.done, and drvdata.cmd.lock, and overwrites
drvdata.hdev.

Re-initializing active synchronization primitives corrupts lockdep and
waitqueues, leading to kernel panics. Additionally, registering the same static
global led_classdev_mc multiple times corrupts the LED subsystem's linked
lists, and sharing drvdata.hdev means one device's interrupts or sysfs
operations will randomly operate on the other device.

[ ... ]

> 	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
> 	if (ret) {
> 		dev_err_probe(&hdev->dev, ret, "Failed to create RGB device\n");
> 		return ret;
> 	}

[Severity: High]
This isn't a regression introduced by this patch, but does this error path
permanently leak sysfs attributes?

Earlier in hid_go_cfg_probe(), sysfs_create_groups() is called to create
attributes. If devm_led_classdev_multicolor_register() fails here, the
function returns the error directly without calling sysfs_remove_groups().

This leaves orphaned sysfs files on the device's kobject. If accessed, they
will reference uninitialized or invalid driver state, and reloading or re-binding
the driver will fail because the files already exist.

[ ... ]

> 	guard(mutex)(&drvdata.cfg_mutex);
> 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);

[Severity: High]
This is a pre-existing issue, but can this cause an AB-BA deadlock?

In hid_go_cfg_remove(), the driver acquires drvdata.cfg_mutex and then calls
sysfs_remove_groups(), which blocks waiting for the kernfs active reference
to drain.

Concurrently, any sysfs read/write handler (like feature_status_show()) first
acquires the kernfs active reference, and then calls mcu_property_out(), which
blocks trying to acquire drvdata.cfg_mutex. This forms an AB-BA deadlock that
will hang driver removal and userspace sysfs access.

[ ... ]

> 		.brightness_set = hid_go_brightness_set,

[ ... ]

> 	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);

[Severity: High]
This is a pre-existing issue, but does this callback perform sleeping operations
in an atomic context?

The driver assigns hid_go_brightness_set to the .brightness_set field. The LED
subsystem invokes this callback directly from atomic contexts, such as timer
interrupts for the timer trigger.

However, hid_go_brightness_set() calls mcu_property_out(), which acquires a
mutex, allocates memory with GFP_KERNEL, and sleeps on a completion timeout.
Triggering this callback from an atomic context will cause a panic. The driver
must use .brightness_set_blocking instead.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788096666.git.mradityadash@gmail.com?part=5

[PATCH v2 6/6] HID: hid-lenovo-go: clear calibration status on start

[Aditya Dash]

A completed calibration leaves success or failure in its status file. If
the next calibration has the same result, userspace sees no change and
cannot tell when the new attempt starts or finishes.

Link each calibration action attribute to its status field. Set only that
field to unknown before submitting Start. Stop leaves the status unchanged,
and the next firmware result replaces unknown with success or failure.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index a4231d188b7a..2ed367abd4cc 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -111,6 +111,11 @@ struct go_cfg_attr {
 	u8 index;
 };
 
+struct go_cal_cfg_attr {
+	u8 index;
+	u8 *status;
+};
+
 struct command_report {
 	u8 report_id;
 	u8 id;
@@ -1410,7 +1415,8 @@ static ssize_t device_status_show(struct device *dev,
 static ssize_t calibrate_config_store(struct device *dev,
 				      struct device_attribute *attr,
 				      const char *buf, u8 cmd, u8 sub_cmd,
-				      size_t count, enum dev_type device_type)
+				      size_t count, enum dev_type device_type,
+				      u8 *cal_status)
 {
 	size_t size = 1;
 	u8 val = 0;
@@ -1425,6 +1431,8 @@ static ssize_t calibrate_config_store(struct device *dev,
 	val = ret;
 	if (!val)
 		size = 0;
+	if (val == CAL_START)
+		*cal_status = CAL_STAT_UNKNOWN;
 
 	ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA, cmd, sub_cmd,
 			       device_type, &val, size);
@@ -1822,7 +1830,8 @@ static void hid_go_brightness_set(struct led_classdev *led_cdev,
 				     const char *buf, size_t count)           \
 	{                                                                     \
 		return calibrate_config_store(dev, attr, buf, _name.index,    \
-					      _scmd, count, _dtype);          \
+					      _scmd, count, _dtype,           \
+					      _name.status);                   \
 	}                                                                     \
 	static ssize_t _name##_##_rtype##_show(                               \
 		struct device *dev, struct device_attribute *attr, char *buf) \
@@ -1982,17 +1991,23 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification",
 static DEVICE_ATTR_RO_NAMED(rumble_notification_left_index,
 			    "rumble_notification_index");
 
-static struct go_cfg_attr cal_trigg_left = { SET_TRIGGER_CFG };
+static struct go_cal_cfg_attr cal_trigg_left = {
+	SET_TRIGGER_CFG, &drvdata.gp_left_trigg_cal_status
+};
 LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", TRIGGER_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_trigg_left_index, "calibrate_trigger_index");
 
-static struct go_cfg_attr cal_joy_left = { SET_JOYSTICK_CFG };
+static struct go_cal_cfg_attr cal_joy_left = {
+	SET_JOYSTICK_CFG, &drvdata.gp_left_joy_cal_status
+};
 LEGO_CAL_DEVICE_ATTR(cal_joy_left, "calibrate_joystick", JOYSTICK_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_joy_left_index, "calibrate_joystick_index");
 
-static struct go_cfg_attr cal_gyro_left = { SET_GYRO_CFG };
+static struct go_cal_cfg_attr cal_gyro_left = {
+	SET_GYRO_CFG, &drvdata.gp_left_gyro_cal_status
+};
 LEGO_CAL_DEVICE_ATTR(cal_gyro_left, "calibrate_gyro", GYRO_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_gyro_left_index, "calibrate_gyro_index");
@@ -2089,17 +2104,23 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_right, "rumble_notification",
 static DEVICE_ATTR_RO_NAMED(rumble_notification_right_index,
 			    "rumble_notification_index");
 
-static struct go_cfg_attr cal_trigg_right = { SET_TRIGGER_CFG };
+static struct go_cal_cfg_attr cal_trigg_right = {
+	SET_TRIGGER_CFG, &drvdata.gp_right_trigg_cal_status
+};
 LEGO_CAL_DEVICE_ATTR(cal_trigg_right, "calibrate_trigger", TRIGGER_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_trigg_right_index, "calibrate_trigger_index");
 
-static struct go_cfg_attr cal_joy_right = { SET_JOYSTICK_CFG };
+static struct go_cal_cfg_attr cal_joy_right = {
+	SET_JOYSTICK_CFG, &drvdata.gp_right_joy_cal_status
+};
 LEGO_CAL_DEVICE_ATTR(cal_joy_right, "calibrate_joystick", JOYSTICK_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_joy_right_index, "calibrate_joystick_index");
 
-static struct go_cfg_attr cal_gyro_right = { SET_GYRO_CFG };
+static struct go_cal_cfg_attr cal_gyro_right = {
+	SET_GYRO_CFG, &drvdata.gp_right_gyro_cal_status
+};
 LEGO_CAL_DEVICE_ATTR(cal_gyro_right, "calibrate_gyro", GYRO_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_gyro_right_index, "calibrate_gyro_index");
-- 
2.55.0

Re: [PATCH v2 6/6] HID: hid-lenovo-go: clear calibration status on start

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in hid_go_probe() error paths.
- [High] Missing hid_hw_stop() in hid_go_remove() on early return.
- [High] Sysfs attribute groups are leaked if hid_go_cfg_probe() fails midway.
- [High] Singleton driver state (`drvdata`) causes state corruption, lock re-initialization, and UAF when multiple devices are probed.
- [High] Deadlock in driver removal path between sysfs_remove_groups and mcu_property_out.
- [High] UAF vulnerability due to devm-managed LED class device unregistration ordering.
--

commit 6b546e272fefac54ad5bf113ee4d2d91e487fb03
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: clear calibration status on start
    
A completed calibration leaves success or failure in its status file. If
the next calibration has the same result, userspace sees no change and
cannot tell when the new attempt starts or finishes.

Link each calibration action attribute to its status field. Set only that
field to unknown before submitting Start. Stop leaves the status unchanged,
and the next firmware result replaces unknown with success or failure.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index a4231d188b7af..2ed367abd4ccc 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

drivers/hid/hid-lenovo-go.c:hid_go_probe() {
    ...
	ret = hid_go_cfg_probe(hdev, id);
	if (ret)
		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");

	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);

	return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but does this error path cause a
use-after-free?

If hid_go_cfg_probe() fails, hid_go_probe() returns the error code without
cleaning up. By omitting hid_hw_close() and hid_hw_stop(), the input and hidraw
interfaces remain registered and active while the driver binding fails and
devres resources are freed. Userspace interaction can then trigger a UAF.

drivers/hid/hid-lenovo-go.c:hid_go_remove() {
	int ep = get_endpoint_address(hdev);

	if (ep <= 0)
		return;
    ...
}

[Severity: High]
This is a pre-existing issue, but can this early return cause a use-after-free
if a malicious device with no endpoints is unplugged?

When a device is unplugged or unbound and get_endpoint_address() returns
-ENODEV, this early return completely bypasses the hid_hw_stop() call, leaving
input interfaces registered while driver devres resources are freed.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
	if (ret) {
		dev_err_probe(&hdev->dev, ret,
			      "Failed to create gamepad configuration attributes\n");
		return ret;
	}

	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
	if (ret) {
		dev_err_probe(&hdev->dev, ret, "Failed to create RGB device\n");
		return ret;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but do the sysfs attribute groups get leaked
here?

If devm_led_classdev_multicolor_register() fails, the manually created sysfs
groups are never removed because sysfs_remove_groups() is missing in the error
path. This leaves dangling sysfs files that can be accessed by userspace after
the driver fails to bind.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
	mutex_init(&drvdata.cfg_mutex);
	init_completion(&drvdata.cmd.done);
	spin_lock_init(&drvdata.cmd.lock);
	drvdata.cmd.pending = false;
	hid_set_drvdata(hdev, &drvdata);
	drvdata.hdev = hdev;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the use of a single global
static drvdata struct cause state corruption if multiple physical devices are
connected?

Probing a second device unconditionally overwrites drvdata.hdev and
re-initializes cfg_mutex and cmd.lock while they might be held, which could
lead to deadlocks or a use-after-free.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
    ...
	guard(mutex)(&drvdata.cfg_mutex);
	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
    ...
}

[Severity: High]
This is a pre-existing issue, but could this lock ordering cause an AB-BA
deadlock during device removal?

hid_go_cfg_remove() acquires drvdata.cfg_mutex and calls sysfs_remove_groups(),
which waits to drain active kernfs references. Meanwhile, an active sysfs
callback holds a kernfs reference and attempts to acquire drvdata.cfg_mutex
inside mcu_property_out(), which would cause a deadlock.

drivers/hid/hid-lenovo-go.c:hid_go_brightness_set() {
    ...
	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
    ...
}

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
    ...
	hid_hw_stop(hdev);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free window
here during device removal?

Devres cleans up resources (unregistering the LED class device) after the
driver's remove() callback returns. Inside hid_go_cfg_remove(), hid_hw_stop()
is called.

During the window before devres runs, userspace can still invoke
hid_go_brightness_set(), which accesses drvdata.hdev and calls
hid_hw_output_report() on a stopped hardware device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788096666.git.mradityadash@gmail.com?part=6

Re: [PATCH v2 0/6] HID: hid-lenovo-go: fix controller configuration

[Derek John Clark]

On Sun, Aug 30, 2026 at 7:57 AM Aditya Dash <mradityadash@gmail.com> wrote:
>
> This series fixes several issues in the controller configuration and
> calibration paths in hid-lenovo-go.
>
> 1/6 fixes the calibration attributes, where the command and sub-command
> are swapped. 2/6 fixes two right-controller attributes: imu_enabled
> selects the bypass feature, and reset targets the left controller.
>
> Before 3/6, any decoded configuration reply completes the shared
> completion. The driver then ignores the result of the wait, so an
> unrelated reply, a firmware error, an interrupted wait, or a timeout can
> still be reported as success.
>
> 3/6 tracks one pending request in the driver data and matches replies by
> command ID, command, sub-command, and device. It also returns request
> errors to the caller.
>
> The firmware has no sequence number, so a late reply with the same tuple
> can still match a newer request.
>
> The remaining patches fix calibration status handling.
>
> The calibration status table defines 0x00 as unknown, 0x01 as success,
> and 0x02 as failure. A Legion Go 1 returned 0x08 after a Stop was sent
> while no calibration was active. The driver stores the raw value as a
> status-text index, so a later read returns -EINVAL.
>
> 4/6 preserves the defined values and maps larger status values to
> failure.
>
> 5/6 rejects 'unknown' as an action. The action table contains it at index
> zero, but the options attribute advertises only 'start' and 'stop'.
>
> A previous result can also hide a new calibration attempt:
>
> failure -> start -> failure
>
> The status appears unchanged for the whole operation. 6/6 clears the
> selected status to unknown before submitting a Start. Stop leaves the
> status unchanged.
>
> ---
> Changes in v2:
> - Drop the FPS DPI patch; the existing request is correct.
> - Drop the v1 Boolean calibration-result mapping. Preserve values 0x00
>   through 0x02 and map only larger failure values.
> - Move the existing pending command state into the driver data and
>   initialize it in cfg_probe().
> - Use scoped IRQ-save guards and validate the report size and ID together.
> - Drop the v1 no-wait calibration path. Correct requests receive their
>   matching SET replies within the existing 50 ms timeout.
> - Reject 'unknown' and clear only the selected status on Start.
>
> v1: https://lore.kernel.org/all/20260821214810.87826-1-mradityadash@gmail.com/
>
Hi Aditya,

I tested the series and have no complaints, everything seems to work
well. Thank you for working on these bugs. You can ignore the existing
issues that sashiko-bot identified for your contribution, I'll take
care of those separately.
There were 2 new issues that need to be fixed:
- [High] The newly added spinlock and completion are accessed by the
raw event handler before they are initialized, causing a potential
kernel crash.
- [Low] The patch leaves behind dead code (`if (!val) size = 0;`)
after explicitly rejecting `CAL_UNKNOWN`.

The first one is more pressing, and will require a reordering of probe
in an additional patch to ensure drvdata is init before hid_hw_start()
and hid_hw_open() are called. The bug is admittedly very unlikely to
occur, but it is the kind of bug that could cause spurious crashes
during boot that would be very difficult to trace.

The second should be trivial to fix

Once fixed I'll add by review and tested by for the series.

Thanks,
Derek

> Aditya Dash (6):
>   HID: hid-lenovo-go: use the correct calibration commands
>   HID: hid-lenovo-go: use the right controller selectors
>   HID: hid-lenovo-go: return configuration request errors
>   HID: hid-lenovo-go: normalize calibration failure status
>   HID: hid-lenovo-go: reject unknown calibration action
>   HID: hid-lenovo-go: clear calibration status on start
>
>  drivers/hid/hid-lenovo-go.c | 179 +++++++++++++++++++++++++++---------
>  1 file changed, 136 insertions(+), 43 deletions(-)
>
>
> base-commit: 1292bca0f8d835d2ad96d309595b2e97f3106d3d
> --
> 2.55.0
>