* [PATCH 05/11] HID: hid-belkin: clean up usage of 'driver_data'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
The module is storing an integer inside the drvdata pointer, which is
confusing, lets fix this and set the whole of 'hid_device_id' struct
as the drvdata and then simply use its integer 'driver_data' field for
quirks, which shall make the code cleaner, type-safe, consistent and
more readable.
This makes the cast to (void *) during storage a bit safer (just to
suppress the const qualifier warning) and the cast to (unsigned long)
during retrieval is removed.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/hid-belkin.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-belkin.c b/drivers/hid/hid-belkin.c
index 75aaed35ee9f..84695115d37b 100644
--- a/drivers/hid/hid-belkin.c
+++ b/drivers/hid/hid-belkin.c
@@ -27,7 +27,8 @@ static int belkin_input_mapping(struct hid_device *hdev, struct hid_input *hi,
struct hid_field *field, struct hid_usage *usage,
unsigned long **bit, int *max)
{
- unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
+ const struct hid_device_id *id = hid_get_drvdata(hdev);
+ unsigned long quirks = id->driver_data;
if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER ||
!(quirks & BELKIN_WKBD))
@@ -48,7 +49,7 @@ static int belkin_probe(struct hid_device *hdev, const struct hid_device_id *id)
unsigned long quirks = id->driver_data;
int ret;
- hid_set_drvdata(hdev, (void *)quirks);
+ hid_set_drvdata(hdev, (void *)id);
ret = hid_parse(hdev);
if (ret) {
--
2.43.0
^ permalink raw reply related
* [PATCH 06/11] HID: hid-cypress: clean up usage of 'driver_data'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
The module is storing an integer inside the drvdata pointer, which is
confusing - furthermore this integer is mutable. When its value is
changed it is set again using the 'hid_set_drvdata' API within
the 'cp_event' function.
Let's fix this, create and allocate the 'cp_device' struct that is then
set as the drvdata and then simply use its integer 'quirks' field for
storing the quirks, which shall make the code cleaner, type-safe,
consistent and more readable.
This makes the cast to (void *) during storage unnecessary and the cast
to (unsigned long) during retrieval is also removed.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/hid-cypress.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-cypress.c b/drivers/hid/hid-cypress.c
index 98548201feec..f18fddc176d0 100644
--- a/drivers/hid/hid-cypress.c
+++ b/drivers/hid/hid-cypress.c
@@ -25,6 +25,10 @@
#define VA_INVAL_LOGICAL_BOUNDARY 0x08
+struct cp_device {
+ unsigned long quirks;
+};
+
/*
* Some USB barcode readers from cypress have usage min and usage max in
* the wrong order
@@ -70,7 +74,8 @@ static __u8 *va_logical_boundary_fixup(struct hid_device *hdev, __u8 *rdesc,
static const __u8 *cp_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
- unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
+ const struct cp_device *cp_device = hid_get_drvdata(hdev);
+ unsigned long quirks = cp_device->quirks;
if (quirks & CP_RDESC_SWAPPED_MIN_MAX)
rdesc = cp_rdesc_fixup(hdev, rdesc, rsize);
@@ -84,7 +89,8 @@ static int cp_input_mapped(struct hid_device *hdev, struct hid_input *hi,
struct hid_field *field, struct hid_usage *usage,
unsigned long **bit, int *max)
{
- unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
+ const struct cp_device *cp_device = hid_get_drvdata(hdev);
+ unsigned long quirks = cp_device->quirks;
if (!(quirks & CP_2WHEEL_MOUSE_HACK))
return 0;
@@ -100,22 +106,21 @@ static int cp_input_mapped(struct hid_device *hdev, struct hid_input *hi,
static int cp_event(struct hid_device *hdev, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
- unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
+ struct cp_device *cp_device = hid_get_drvdata(hdev);
if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput ||
- !usage->type || !(quirks & CP_2WHEEL_MOUSE_HACK))
+ !usage->type || !(cp_device->quirks & CP_2WHEEL_MOUSE_HACK))
return 0;
if (usage->hid == 0x00090005) {
if (value)
- quirks |= CP_2WHEEL_MOUSE_HACK_ON;
+ cp_device->quirks |= CP_2WHEEL_MOUSE_HACK_ON;
else
- quirks &= ~CP_2WHEEL_MOUSE_HACK_ON;
- hid_set_drvdata(hdev, (void *)quirks);
+ cp_device->quirks &= ~CP_2WHEEL_MOUSE_HACK_ON;
return 1;
}
- if (usage->code == REL_WHEEL && (quirks & CP_2WHEEL_MOUSE_HACK_ON)) {
+ if (usage->code == REL_WHEEL && (cp_device->quirks & CP_2WHEEL_MOUSE_HACK_ON)) {
struct input_dev *input = field->hidinput->input;
input_event(input, usage->type, REL_HWHEEL, value);
@@ -127,10 +132,17 @@ static int cp_event(struct hid_device *hdev, struct hid_field *field,
static int cp_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
- unsigned long quirks = id->driver_data;
int ret;
+ struct cp_device *cp_device;
- hid_set_drvdata(hdev, (void *)quirks);
+ cp_device = devm_kzalloc(&hdev->dev, sizeof(*cp_device), GFP_KERNEL);
+
+ if (!cp_device)
+ return -ENOMEM;
+
+ cp_device->quirks = id->driver_data;
+
+ hid_set_drvdata(hdev, cp_device);
ret = hid_parse(hdev);
if (ret) {
--
2.43.0
^ permalink raw reply related
* [PATCH 09/11] HID: mod_devicetable: 'hid_device_id::driver_data' add union
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
The 'driver_data' field can either be interpreted as an unsigned
numerical number or as a pointer - this depends on the driver within
HID subsystem. Unionise this unsigned value with a 'const void*'
pointer. This change allows to remove explicit casts to pointers
in the pointer-users case and as far as integer-users of the field
are concerned nothing has really changed.
The change doesn't affect the size and layout of struct hid_device_id
which is part of the userspace ABI.
This change aims to improve type-safety, readability - as the
intent is more clear - and portability of the code in the long term.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
include/linux/mod_devicetable.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 23ff24080dfd..90b2dae75109 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -175,7 +175,10 @@ struct hid_device_id {
__u16 group;
__u32 vendor;
__u32 product;
- kernel_ulong_t driver_data;
+ union {
+ kernel_ulong_t driver_data;
+ const void *driver_data_ptr;
+ };
};
/* s390 CCW devices */
--
2.43.0
^ permalink raw reply related
* [PATCH 04/11] HID: i2c-hid-dmi-quirks: use named initializer for 'i2c_hid_elan_flipped_quirks[]'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
This makes the code a little bit more readable and consistent
with other drivers in the HID subsystem that assign the
'driver_data' field in the 'hid_device_id' struct explicitly.
It was confirmed that i2c-hid-dmi-quirks.o pre/post patch are equal
using diffscope.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
index 210f17c3a0be..f3e3cf73641e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
+++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
@@ -420,7 +420,7 @@ static const struct dmi_system_id i2c_hid_dmi_desc_override_table[] = {
static const struct hid_device_id i2c_hid_elan_flipped_quirks = {
HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
- HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT
+ .driver_data = HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT,
};
/*
--
2.43.0
^ permalink raw reply related
* [PATCH 11/11] HID: wacom: use 'hid_device_id::driver_data_ptr'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
Remove the casts to (kernel_ulong_t) and (struct wacom_features *)
and use the 'drvier_data_ptr' field instead to improve readability
of the code.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/wacom_sys.c | 14 +++++++++-----
drivers/hid/wacom_wac.c | 10 +++++-----
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 0d1c6d90fe21..25408676c5f6 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2555,8 +2555,9 @@ static void wacom_wireless_work(struct work_struct *work)
}
/* Stylus interface */
- wacom_wac1->features =
- *((struct wacom_features *)id->driver_data);
+ const struct wacom_features *features = id->driver_data_ptr;
+
+ wacom_wac1->features = *features;
wacom_wac1->pid = wacom_wac->pid;
hid_hw_stop(hdev1);
@@ -2568,8 +2569,7 @@ static void wacom_wireless_work(struct work_struct *work)
if (wacom_wac1->features.touch_max ||
(wacom_wac1->features.type >= INTUOSHT &&
wacom_wac1->features.type <= BAMBOO_PT)) {
- wacom_wac2->features =
- *((struct wacom_features *)id->driver_data);
+ wacom_wac2->features = *features;
wacom_wac2->pid = wacom_wac->pid;
hid_hw_stop(hdev2);
error = wacom_parse_and_register(wacom2, true);
@@ -2839,7 +2839,11 @@ static int wacom_probe(struct hid_device *hdev,
wacom->hdev = hdev;
wacom_wac = &wacom->wacom_wac;
- wacom_wac->features = *((struct wacom_features *)id->driver_data);
+
+ const struct wacom_features *id_features = id->driver_data_ptr;
+
+ wacom_wac->features = *id_features;
+
features = &wacom_wac->features;
if (features->check_for_hid_type && features->hid_type != hdev->type)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index da1f0ea85625..034bde5af2df 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -4947,23 +4947,23 @@ static const struct wacom_features wacom_features_0x94 =
#define USB_DEVICE_WACOM(prod) \
HID_DEVICE(BUS_USB, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
- .driver_data = (kernel_ulong_t)&wacom_features_##prod
+ .driver_data_ptr = &wacom_features_##prod
#define BT_DEVICE_WACOM(prod) \
HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
- .driver_data = (kernel_ulong_t)&wacom_features_##prod
+ .driver_data_ptr = &wacom_features_##prod
#define I2C_DEVICE_WACOM(prod) \
HID_DEVICE(BUS_I2C, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
- .driver_data = (kernel_ulong_t)&wacom_features_##prod
+ .driver_data_ptr = &wacom_features_##prod
#define PCI_DEVICE_WACOM(prod) \
HID_DEVICE(BUS_PCI, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
- .driver_data = (kernel_ulong_t)&wacom_features_##prod
+ .driver_data_ptr = &wacom_features_##prod
#define USB_DEVICE_LENOVO(prod) \
HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, prod), \
- .driver_data = (kernel_ulong_t)&wacom_features_##prod
+ .driver_data_ptr = &wacom_features_##prod
const struct hid_device_id wacom_ids[] = {
{ USB_DEVICE_WACOM(0x00) },
--
2.43.0
^ permalink raw reply related
* [PATCH 08/11] HID: hid-ite: clean up usage of 'driver_data'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
The module is storing an integer inside the drvdata pointer, which is
confusing, lets fix this and set the whole of 'hid_device_id' struct
as the drvdata and then simply use its integer 'driver_data' field for
quirks, which shall make the code cleaner, type-safe, consistent and
more readable.
This makes the cast to (void *) during storage a bit safer (just to
suppress the const qualifier warning) and the cast to (unsigned long)
during retrieval is removed.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/hid-ite.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c
index 8e42780a2663..63908f24b524 100644
--- a/drivers/hid/hid-ite.c
+++ b/drivers/hid/hid-ite.c
@@ -15,7 +15,9 @@
static const __u8 *ite_report_fixup(struct hid_device *hdev, __u8 *rdesc, unsigned int *rsize)
{
- unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
+
+ const struct hid_device_id *id = hid_get_drvdata(hdev);
+ unsigned long quirks = id->driver_data;
if (quirks & QUIRK_TOUCHPAD_ON_OFF_REPORT) {
/* For Acer Aspire Switch 10 SW5-012 keyboard-dock */
@@ -44,7 +46,8 @@ static int ite_input_mapping(struct hid_device *hdev,
int *max)
{
- unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
+ const struct hid_device_id *id = hid_get_drvdata(hdev);
+ unsigned long quirks = id->driver_data;
if ((quirks & QUIRK_TOUCHPAD_ON_OFF_REPORT) &&
(usage->hid & HID_USAGE_PAGE) == 0x00880000) {
@@ -94,7 +97,7 @@ static int ite_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
int ret;
- hid_set_drvdata(hdev, (void *)id->driver_data);
+ hid_set_drvdata(hdev, (void *)id);
ret = hid_open_report(hdev);
if (ret)
--
2.43.0
^ permalink raw reply related
* [PATCH 07/11] HID: hid-gfrm: clean up usage of 'driver_data'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
The module is storing an integer inside the drvdata pointer, which is
confusing, lets fix this and set the whole of 'hid_device_id' struct
as the drvdata and then simply use its integer 'driver_data' field for
quirks, which shall make the code cleaner, type-safe, consistent and
more readable.
This makes the cast to (void *) during storage a bit safer (just to
suppress the const qualifier warning) and the cast to (unsigned long)
during retrieval is removed.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/hid-gfrm.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index 699186ff2349..0be49a93d28b 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -28,7 +28,8 @@ static int gfrm_input_mapping(struct hid_device *hdev, struct hid_input *hi,
struct hid_field *field, struct hid_usage *usage,
unsigned long **bit, int *max)
{
- unsigned long hdev_type = (unsigned long) hid_get_drvdata(hdev);
+ const struct hid_device_id *id = hid_get_drvdata(hdev);
+ unsigned long hdev_type = id->driver_data;
if (hdev_type == GFRM100) {
if (usage->hid == (HID_UP_CONSUMER | 0x4)) {
@@ -50,7 +51,8 @@ static int gfrm_input_mapping(struct hid_device *hdev, struct hid_input *hi,
static int gfrm_raw_event(struct hid_device *hdev, struct hid_report *report,
u8 *data, int size)
{
- unsigned long hdev_type = (unsigned long) hid_get_drvdata(hdev);
+ const struct hid_device_id *id = hid_get_drvdata(hdev);
+ unsigned long hdev_type = id->driver_data;
int ret = 0;
if (hdev_type != GFRM100)
@@ -99,7 +101,7 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
int ret;
- hid_set_drvdata(hdev, (void *) id->driver_data);
+ hid_set_drvdata(hdev, (void *)id);
ret = hid_parse(hdev);
if (ret)
--
2.43.0
^ permalink raw reply related
* [PATCH 10/11] HID: hid-tmff: use 'hid_device_id::driver_data_ptr'
From: Pawel Zalewski (The Capable Hub) @ 2026-05-18 16:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
Cc: linux-kernel, linux-input, Christian A. Ehrhardt,
Christian A. Ehrhardt, Pawel Zalewski (The Capable Hub)
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk>
Remove the casts to (unsigned long) and instead assign the
'ff_rumble' and 'ff_joystick' variables as they should, as pointers.
Remove the cast to (void *) during probe - tmff_init() second parameter
already accepts a const pointer, so no further action was needed here.
These changes improve readability of the code.
Signed-off-by: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
---
drivers/hid/hid-tmff.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-tmff.c b/drivers/hid/hid-tmff.c
index 423f395d01ac..0d7e73fba0fa 100644
--- a/drivers/hid/hid-tmff.c
+++ b/drivers/hid/hid-tmff.c
@@ -226,7 +226,7 @@ static int tm_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto err;
}
- tmff_init(hdev, (void *)id->driver_data);
+ tmff_init(hdev, id->driver_data_ptr);
return 0;
err:
@@ -235,25 +235,25 @@ static int tm_probe(struct hid_device *hdev, const struct hid_device_id *id)
static const struct hid_device_id tm_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb300),
- .driver_data = (unsigned long)ff_rumble },
+ .driver_data_ptr = ff_rumble },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb304), /* FireStorm Dual Power 2 (and 3) */
- .driver_data = (unsigned long)ff_rumble },
+ .driver_data_ptr = ff_rumble },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, THRUSTMASTER_DEVICE_ID_2_IN_1_DT), /* Dual Trigger 2-in-1 */
- .driver_data = (unsigned long)ff_rumble },
+ .driver_data_ptr = ff_rumble },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb323), /* Dual Trigger 3-in-1 (PC Mode) */
- .driver_data = (unsigned long)ff_rumble },
+ .driver_data_ptr = ff_rumble },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb324), /* Dual Trigger 3-in-1 (PS3 Mode) */
- .driver_data = (unsigned long)ff_rumble },
+ .driver_data_ptr = ff_rumble },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb605), /* NASCAR PRO FF2 Wheel */
- .driver_data = (unsigned long)ff_joystick },
+ .driver_data_ptr = ff_joystick },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb651), /* FGT Rumble Force Wheel */
- .driver_data = (unsigned long)ff_rumble },
+ .driver_data_ptr = ff_rumble },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb653), /* RGT Force Feedback CLUTCH Raging Wheel */
- .driver_data = (unsigned long)ff_joystick },
+ .driver_data_ptr = ff_joystick },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb654), /* FGT Force Feedback Wheel */
- .driver_data = (unsigned long)ff_joystick },
+ .driver_data_ptr = ff_joystick },
{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb65a), /* F430 Force Feedback Wheel */
- .driver_data = (unsigned long)ff_joystick },
+ .driver_data_ptr = ff_joystick },
{ }
};
MODULE_DEVICE_TABLE(hid, tm_devices);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards
From: sashiko-bot @ 2026-05-18 16:12 UTC (permalink / raw)
To: Xavier Bestel; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260518153519.605627-1-xav@bes.tel>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing workqueue cancellation on module unload or device removal causes a Use-After-Free
- [High] Missing edge detection for Game Mode and brightness buttons leads to USB bus spam and state desynchronization
- [High] Data race on `g15->transfer_buf` during device probe
- [Medium] LED brightness callbacks violate the API contract by returning success on failure
- [Medium] Hardware events arriving before LED registration trigger a kernel WARN_ON
--
commit f98ec9abc4841148689435f75f93ed341ba49d7d
Author: Xavier Bestel <xav@bes.tel>
HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards
Add support for the Logitech G710 and G710+ gaming keyboards to the
hid-lg-g15 driver, exposing the G-keys, M-keys, game mode toggle, and
brightness cycle buttons.
> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
[ ... ]
> @@ -597,6 +605,234 @@ static int lg_g510_mkey_led_set(struct led_classdev *led_cdev,
> return ret;
> }
>
> +/******** G710 LED functions ********/
[ ... ]
> +static int lg_g710_mkey_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
[ ... ]
> + ret = hid_hw_raw_request(g15->hdev, LG_G710_FEATURE_M_KEYS_LEDS,
> + g15->transfer_buf, 2,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> + if (ret == 2) {
> + /* Success */
> + g15_led->brightness = brightness;
> + ret = 0;
> + } else {
> + if (ret != -EPIPE)
> + hid_err(g15->hdev, "Error setting LED brightness: %d\n", ret);
> + /* -EPIPE is transient (USB stall), cache is unchanged, retry next time */
> + ret = 0;
> + }
> +
> + mutex_unlock(&g15->mutex);
> +
> + return ret;
> +}
Does returning 0 here break the LED class API contract when a hard error
like -EIO or -ENODEV occurs?
If we return success despite hardware failure, won't the internal software
cache update while the physical hardware remains unchanged, leading to a
permanent desynchronization between sysfs and the device?
[ ... ]
> @@ -890,6 +1136,71 @@ static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
> return 0;
> }
>
> +static int lg_g710_event(struct lg_g15_data *g15, u8 *data, int size)
> +{
[ ... ]
> + /*
> + * Detect brightness key presses and schedule the work function
> + * to cycle cached brightness and notify userspace.
> + * Bit 10 = WASD backlight (maps to LG_G15_LCD_BRIGHTNESS slot).
> + * Bit 11 = Kbd backlight (maps to LG_G15_KBD_BRIGHTNESS slot).
> + */
> + if (pressed_keys & BIT(10)) {
> + set_bit(LG_G15_LCD_BRIGHTNESS, &g15->brightness_changed);
> + schedule_work(&g15->work);
> + }
> + if (pressed_keys & BIT(11)) {
> + set_bit(LG_G15_KBD_BRIGHTNESS, &g15->brightness_changed);
> + schedule_work(&g15->work);
> + }
> +
> + /* Game mode toggle — bit 12 is a state bit, trigger on any change */
> + if (pressed_keys & BIT(12)) {
> + set_bit(LG_G15_GAMEMODE, &g15->brightness_changed);
> + schedule_work(&g15->work);
> + }
Could these bitwise checks cause continuous triggering on every HID report
received?
Since the game mode toggle check evaluates the absolute state, if game mode
is enabled (for instance, while holding a macro key), won't this continuously
schedule the workqueue, spam the USB bus with synchronous get-report requests,
and flood userspace with duplicate notifications?
Also, since the condition evaluates to false when game mode is disabled, will
userspace ever be notified of the disabled state?
Similarly for the brightness buttons, does checking the absolute state mean
that holding the button down will rapidly decrement the software brightness
cache and permanently desynchronize it from the physical hardware?
Should the driver track previous states and trigger actions only on edge
transitions instead?
[ ... ]
> @@ -1212,6 +1562,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
> case LG_Z10:
> connect_mask = HID_CONNECT_HIDRAW;
> break;
> + case LG_G710:
> + INIT_WORK(&g15->work, lg_g710_leds_changed_work);
> + hdev->quirks |= HID_QUIRK_NOGET;
> + connect_mask = HID_CONNECT_DEFAULT;
> + gkeys_settings_feature_report = LG_G710_FEATURE_EXTRA_KEYS;
> + gkeys = 6;
> + break;
> }
>
> ret = hid_hw_start(hdev, connect_mask);
This isn't a bug introduced exclusively by this patch, but does the lack of a
remove callback or devres action to cancel the scheduled work risk a
use-after-free?
The driver allocates the context structure using devm_kzalloc and initializes
the work struct, but nothing calls cancel_work_sync on shutdown. If the work
is pending or running on the system workqueue when devm frees the memory,
won't it dereference freed memory?
[ ... ]
> @@ -1234,11 +1591,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
>
> if (gkeys_settings_feature_report) {
> + int report_size = (g15->model == LG_G710) ? gkeys * 2 : gkeys;
>
> g15->transfer_buf[0] = gkeys_settings_feature_report;
> - memset(g15->transfer_buf + 1, 0, gkeys);
> + memset(g15->transfer_buf + 1, 0, report_size);
> ret = hid_hw_raw_request(g15->hdev,
> gkeys_settings_feature_report,
> - g15->transfer_buf, gkeys + 1,
> + g15->transfer_buf, report_size + 1,
> HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> }
This isn't a new bug, but since this lockless access to the transfer buffer
occurs right after hid_hw_start, could an incoming HID report concurrently
schedule the work queue?
The asynchronous worker safely locks the mutex before mutating the exact same
transfer buffer. Does this allow the workqueue to overwrite the buffer while
the probe function is actively using it, resulting in malformed requests?
[ ... ]
> @@ -1327,12 +1686,18 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto error_hw_stop;
>
> /* Register LED devices */
> - for (i = 0; i < LG_G15_LED_MAX; i++) {
> + for (i = 0; i <= LG_G15_MACRO_RECORD; i++) {
> ret = lg_g15_register_led(g15, i, led_names[i]);
> if (ret)
> goto error_hw_stop;
> }
>
> + if (g15->model == LG_G710) {
> + ret = lg_g15_register_led(g15, LG_G15_GAMEMODE, "g15::gamemode");
> + if (ret)
> + goto error_hw_stop;
> + }
Because hid_hw_start is called earlier in the probe sequence, is it possible
for a hardware event to trigger lg_g710_event before these LEDs are actually
registered?
If the worker thread runs and invokes the brightness hardware changed
notification on zero-initialized, unregistered LED devices, won't the
internal sysfs nodes be NULL, triggering a stack trace warning in the LED
core?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518153519.605627-1-xav@bes.tel?part=1
^ permalink raw reply
* Re: [PATCH 11/11] HID: wacom: use 'hid_device_id::driver_data_ptr'
From: sashiko-bot @ 2026-05-18 16:23 UTC (permalink / raw)
Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-11-a08e3989c283@thegoodpenguin.co.uk>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Incomplete refactoring of `id->driver_data` to `id->driver_data_ptr`.
- [Low] Mixed declarations and code in `wacom_probe()` and `wacom_wireless_work()`.
- [Low] Typo in the commit message.
--
commit 83b980c631eb908ccc27340ea394dd0fc86c568e
Author: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
HID: wacom: use 'hid_device_id::driver_data_ptr'
This commit updates the wacom driver to use the driver_data_ptr field of
struct hid_device_id instead of casting the driver_data field. It improves
code readability by avoiding explicit casts to kernel_ulong_t and
struct wacom_features *.
> Remove the casts to (kernel_ulong_t) and (struct wacom_features *)
> and use the 'drvier_data_ptr' field instead to improve readability
> of the code.
This isn't a bug, but there's a typo (drvier_data_ptr) in the commit message.
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 0d1c6d90fe21c..25408676c5f61 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2555,8 +2555,9 @@ static void wacom_wireless_work(struct work_struct *work)
> }
>
> /* Stylus interface */
> - wacom_wac1->features =
> - *((struct wacom_features *)id->driver_data);
> + const struct wacom_features *features = id->driver_data_ptr;
> +
> + wacom_wac1->features = *features;
Does this introduce a mixed declaration and code warning? The variable
declaration is placed in the middle of a block after executable statements.
[ ... ]
> @@ -2839,7 +2839,11 @@ static int wacom_probe(struct hid_device *hdev,
> wacom->hdev = hdev;
>
> wacom_wac = &wacom->wacom_wac;
> - wacom_wac->features = *((struct wacom_features *)id->driver_data);
> +
> + const struct wacom_features *id_features = id->driver_data_ptr;
> +
> + wacom_wac->features = *id_features;
Similarly, does this placement of id_features violate the coding style by
mixing declarations and code?
Additionally, earlier in wacom_probe() there is this check:
if (!id->driver_data)
return -EINVAL;
Since the goal is to transition to driver_data_ptr, should that check also be
updated to use id->driver_data_ptr for consistency?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk?part=11
^ permalink raw reply
* Re: [PATCH 10/11] HID: hid-tmff: use 'hid_device_id::driver_data_ptr'
From: sashiko-bot @ 2026-05-18 16:26 UTC (permalink / raw)
Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-10-a08e3989c283@thegoodpenguin.co.uk>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Potential NULL pointer dereference in `tmff_init()` when dynamically binding devices via sysfs `new_id`.
--
commit 773ba3555b1e1205e4c7dd4ff010a1b6be4f15fa
Author: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
HID: hid-tmff: use 'hid_device_id::driver_data_ptr'
This commit updates the Thrustmaster HID driver to use the driver_data_ptr
field in the hid_device_id structure. It removes unsigned long and void
pointer casts to improve readability.
> diff --git a/drivers/hid/hid-tmff.c b/drivers/hid/hid-tmff.c
> index 423f395d01ac5..0d7e73fba0fa3 100644
> --- a/drivers/hid/hid-tmff.c
> +++ b/drivers/hid/hid-tmff.c
[ ... ]
> @@ -226,7 +226,7 @@ static int tm_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto err;
> }
>
> - tmff_init(hdev, (void *)id->driver_data);
> + tmff_init(hdev, id->driver_data_ptr);
If a new device ID is dynamically added to the HID driver via the sysfs
new_id interface without explicitly providing driver_data, does
id->driver_data_ptr default to NULL?
If id->driver_data_ptr is NULL, and the dynamically bound device contains
a THRUSTMASTER_USAGE_FF usage in its HID report descriptor, can this lead
to a NULL pointer dereference in tmff_init()?
Looking at tmff_init(), it appears to unconditionally dereference the
ff_bits pointer in this situation:
for (i = 0; ff_bits[i] >= 0; i++)
Could this regression allow a standard administrative sysfs action to crash
the system? While the missing NULL check might predate these changes,
modifying the pointer extraction here seems like a good time to address it.
>
> return 0;
> err:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk?part=10
^ permalink raw reply
* Re: [PATCH 09/11] HID: mod_devicetable: 'hid_device_id::driver_data' add union
From: sashiko-bot @ 2026-05-18 16:35 UTC (permalink / raw)
Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260518-mod-devicetable-hid_device_id-v1-9-a08e3989c283@thegoodpenguin.co.uk>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Adding a pointer variant to `hid_device_id` union encourages a dangerous architectural anti-pattern involving `driver_data`.
- [Low] The commit message inaccurately claims that `struct hid_device_id` is part of the userspace ABI.
--
commit c7768796e9018aff03a8509ee8a35c62af4417cf
Author: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
HID: mod_devicetable: 'hid_device_id::driver_data' add union
This commit adds a union to the driver_data field in struct hid_device_id
to provide a direct pointer representation.
> The change doesn't affect the size and layout of struct hid_device_id
> which is part of the userspace ABI.
Is this description accurate regarding the userspace ABI?
The structure struct hid_device_id is defined in
include/linux/mod_devicetable.h, which is an internal kernel header and is
not exported to include/uapi/.
While modpost uses the binary layout during the kernel build to generate
the text-based modules.alias file, userspace tools interact with the
generated text interface rather than the binary structure itself.
Could the commit message be updated to clarify that this layout is an
internal build ABI rather than a userspace ABI?
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 23ff24080dfd9..90b2dae75109c 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -175,7 +175,10 @@ struct hid_device_id {
> __u16 group;
> __u32 vendor;
> __u32 product;
> - kernel_ulong_t driver_data;
> + union {
> + kernel_ulong_t driver_data;
> + const void *driver_data_ptr;
> + };
> };
Does this code introduce a regression by encouraging an architectural
pattern where drivers assume they can dereference driver_data as a pointer?
The HID core supports dynamic device ID injection via the sysfs new_id
interface. In drivers/hid/hid-core.c, new_id_store() parses user input as
an unsigned long and assigns it to driver_data.
By adding driver_data_ptr, it explicitly blesses treating this field as a
kernel pointer at the core API level.
If a driver dereferences this user-provided value as a pointer during
probe, could it lead to arbitrary memory dereferences or lockdown bypasses?
Does it make more sense to keep driver_data restricted to an index or enum
to avoid exposing an API that facilitates this pattern?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk?part=9
^ permalink raw reply
* Re: [PATCH v2 5/7] drm/panel-edp: Add panel for Surface Pro 12in
From: Doug Anderson @ 2026-05-18 17:14 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, Herbert Xu,
davem, neil.armstrong, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, jikos, bentiss, luzmaximilian, hansg,
ilpo.jarvinen, Jessica Zhang, linux-arm-msm, devicetree,
linux-kernel, linux-crypto, dri-devel, linux-input,
platform-driver-x86
In-Reply-To: <9e749a3a483e4a3c684eac3ee6a4b241c94a0362.1778822464.git.harrison.vanderbyl@gmail.com>
Hi,
On Thu, May 14, 2026 at 10:43 PM Harrison Vanderbyl
<harrison.vanderbyl@gmail.com> wrote:
>
> Add an entry for the BOE NE120DRM-N28 panel,
> used in the Microsoft Surface Pro 12-inch.
>
> The values chosen were tested to be working fine
> for wake from sleep and hibernation.
>
> Panel edid:
>
> 00 ff ff ff ff ff ff 00 09 e5 c9 0c a0 06 00 07
> 0a 22 01 04 a5 19 11 78 07 9f 15 a6 55 4c 9b 25
> 0e 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 62 53 94 a0 80 b8 2e 50 18 10
> 3a 00 fe a9 00 00 00 1a 13 7d 94 a0 80 b8 2e 50
> 18 10 3a 00 fe a9 00 00 00 1a 00 00 00 fd 00 18
> 5a 5b 88 20 01 0a 20 20 20 20 20 20 00 00 00 fc
> 00 4e 45 31 32 30 44 52 4d 2d 4e 32 38 0a 00 0a
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
> ---
> drivers/gpu/drm/panel/panel-edp.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Pushed to drm-misc-next:
[5/7] drm/panel-edp: Add panel for Surface Pro 12in
commit: 02f48ffdf96c83ca3e6600fe5dec872b34b68775
^ permalink raw reply
* Re: [PATCH v2 0/7] Add support for the Microsoft Surface Pro 12in 1st Edition (Snapdragon),
From: Dmitry Baryshkov @ 2026-05-18 17:51 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, Herbert Xu,
davem, neil.armstrong, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, jikos, bentiss, luzmaximilian, hansg,
ilpo.jarvinen, Douglas Anderson, Jessica Zhang, linux-arm-msm,
devicetree, linux-kernel, linux-crypto, dri-devel, linux-input,
platform-driver-x86
In-Reply-To: <cover.1778822464.git.harrison.vanderbyl@gmail.com>
On Fri, May 15, 2026 at 03:41:45PM +1000, Harrison Vanderbyl wrote:
> Changes in v2:
Please don't send new iterations as a reply to the previous iteration.
Always start new thread for the new iteration.
> Ice device tree:
> - Updated to use 0x0 formatting
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: core: Fix size_t specifier in hid_report_raw_event()
From: Nathan Chancellor @ 2026-05-18 19:08 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman, Johan Hovold,
linux-input, linux-kernel, stable, Miguel Ojeda
In-Reply-To: <CAMuHMdVzWL0WZe6u-uY2U+uCNUKB1aNTYM3kaYkX=OJBCY9G0w@mail.gmail.com>
Hi Geert,
On Mon, May 18, 2026 at 11:10:49AM +0200, Geert Uytterhoeven wrote:
> On Sun, 17 May 2026 at 06:51, Nathan Chancellor <nathan@kernel.org> wrote:
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2050,7 +2050,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
> > return 0;
> >
> > if (unlikely(bsize < csize)) {
> > - hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n",
> > + hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %zu)\n",
> > report->id, csize, bsize);
>
> Both report->id and csize are unsigned, so should use %u.
>
> > return -EINVAL;
> > }
> > @@ -2072,7 +2072,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
> > rsize = max_buffer_size;
> >
> > if (bsize < rsize) {
> > - hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %ld)\n",
> > + hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %zu)\n",
> > report->id, rsize, bsize);
>
> Same here.
>
> > return -EINVAL;
> > }
>
> And more incorrect %d outside the context!
Yeah, I had noticed this as well but I decided to keep the patch
contained to only address the instances that the compiler warned about.
I do think this is worth addressing in a follow up patch, which either I
or the HID maintainers can do.
Thanks for the review!
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH] HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards
From: Xavier Bestel @ 2026-05-18 19:24 UTC (permalink / raw)
To: sashiko-reviews, Hans de Goede; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260518161227.BCAF6C2BCB7@smtp.kernel.org>
Le lundi 18 mai 2026 à 16:12 +0000, sashiko-bot@kernel.org a écrit :
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [Critical] Missing workqueue cancellation on module unload or device removal causes a Use-After-Free
> - [High] Missing edge detection for Game Mode and brightness buttons leads to USB bus spam and state desynchronization
> - [High] Data race on `g15->transfer_buf` during device probe
> - [Medium] LED brightness callbacks violate the API contract by returning success on failure
> - [Medium] Hardware events arriving before LED registration trigger a kernel WARN_ON
> --
>
> commit f98ec9abc4841148689435f75f93ed341ba49d7d
> Author: Xavier Bestel <xav@bes.tel>
>
> HID: lg-g15: Add support for the Logitech G710/G710+ gaming keyboards
>
> Add support for the Logitech G710 and G710+ gaming keyboards to the
> hid-lg-g15 driver, exposing the G-keys, M-keys, game mode toggle, and
> brightness cycle buttons.
>
> > diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> > --- a/drivers/hid/hid-lg-g15.c
> > +++ b/drivers/hid/hid-lg-g15.c
>
> [ ... ]
>
> > @@ -597,6 +605,234 @@ static int lg_g510_mkey_led_set(struct led_classdev *led_cdev,
> > return ret;
> > }
> >
> > +/******** G710 LED functions ********/
>
> [ ... ]
>
> > +static int lg_g710_mkey_led_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
>
> [ ... ]
>
> > + ret = hid_hw_raw_request(g15->hdev, LG_G710_FEATURE_M_KEYS_LEDS,
> > + g15->transfer_buf, 2,
> > + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > + if (ret == 2) {
> > + /* Success */
> > + g15_led->brightness = brightness;
> > + ret = 0;
> > + } else {
> > + if (ret != -EPIPE)
> > + hid_err(g15->hdev, "Error setting LED brightness: %d\n", ret);
> > + /* -EPIPE is transient (USB stall), cache is unchanged, retry next time */
> > + ret = 0;
> > + }
> > +
> > + mutex_unlock(&g15->mutex);
> > +
> > + return ret;
> > +}
>
> Does returning 0 here break the LED class API contract when a hard error
> like -EIO or -ENODEV occurs?
>
> If we return success despite hardware failure, won't the internal software
> cache update while the physical hardware remains unchanged, leading to a
> permanent desynchronization between sysfs and the device?
Right, will fix.
>
> [ ... ]
>
> > @@ -890,6 +1136,71 @@ static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
> > return 0;
> > }
> >
> > +static int lg_g710_event(struct lg_g15_data *g15, u8 *data, int size)
> > +{
>
> [ ... ]
>
> > + /*
> > + * Detect brightness key presses and schedule the work function
> > + * to cycle cached brightness and notify userspace.
> > + * Bit 10 = WASD backlight (maps to LG_G15_LCD_BRIGHTNESS slot).
> > + * Bit 11 = Kbd backlight (maps to LG_G15_KBD_BRIGHTNESS slot).
> > + */
> > + if (pressed_keys & BIT(10)) {
> > + set_bit(LG_G15_LCD_BRIGHTNESS, &g15->brightness_changed);
> > + schedule_work(&g15->work);
> > + }
> > + if (pressed_keys & BIT(11)) {
> > + set_bit(LG_G15_KBD_BRIGHTNESS, &g15->brightness_changed);
> > + schedule_work(&g15->work);
> > + }
> > +
> > + /* Game mode toggle — bit 12 is a state bit, trigger on any change */
> > + if (pressed_keys & BIT(12)) {
> > + set_bit(LG_G15_GAMEMODE, &g15->brightness_changed);
> > + schedule_work(&g15->work);
> > + }
>
> Could these bitwise checks cause continuous triggering on every HID report
> received?
>
> Since the game mode toggle check evaluates the absolute state, if game mode
> is enabled (for instance, while holding a macro key), won't this continuously
> schedule the workqueue, spam the USB bus with synchronous get-report requests,
> and flood userspace with duplicate notifications?
>
> Also, since the condition evaluates to false when game mode is disabled, will
> userspace ever be notified of the disabled state?
>
> Similarly for the brightness buttons, does checking the absolute state mean
> that holding the button down will rapidly decrement the software brightness
> cache and permanently desynchronize it from the physical hardware?
>
> Should the driver track previous states and trigger actions only on edge
> transitions instead?
Right again, will add edge detection.
> [ ... ]
>
> > @@ -1212,6 +1562,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > case LG_Z10:
> > connect_mask = HID_CONNECT_HIDRAW;
> > break;
> > + case LG_G710:
> > + INIT_WORK(&g15->work, lg_g710_leds_changed_work);
> > + hdev->quirks |= HID_QUIRK_NOGET;
> > + connect_mask = HID_CONNECT_DEFAULT;
> > + gkeys_settings_feature_report = LG_G710_FEATURE_EXTRA_KEYS;
> > + gkeys = 6;
> > + break;
> > }
> >
> > ret = hid_hw_start(hdev, connect_mask);
>
> This isn't a bug introduced exclusively by this patch, but does the lack of a
> remove callback or devres action to cancel the scheduled work risk a
> use-after-free?
>
> The driver allocates the context structure using devm_kzalloc and initializes
> the work struct, but nothing calls cancel_work_sync on shutdown. If the work
> is pending or running on the system workqueue when devm frees the memory,
> won't it dereference freed memory?
Sure, but it probably ought to be changed in a separate patch, if ever.
> [ ... ]
>
> > @@ -1234,11 +1591,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > }
> >
> > if (gkeys_settings_feature_report) {
> > + int report_size = (g15->model == LG_G710) ? gkeys * 2 : gkeys;
> >
> > g15->transfer_buf[0] = gkeys_settings_feature_report;
> > - memset(g15->transfer_buf + 1, 0, gkeys);
> > + memset(g15->transfer_buf + 1, 0, report_size);
> > ret = hid_hw_raw_request(g15->hdev,
> > gkeys_settings_feature_report,
> > - g15->transfer_buf, gkeys + 1,
> > + g15->transfer_buf, report_size + 1,
> > HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > }
>
> This isn't a new bug, but since this lockless access to the transfer buffer
> occurs right after hid_hw_start, could an incoming HID report concurrently
> schedule the work queue?
>
> The asynchronous worker safely locks the mutex before mutating the exact same
> transfer buffer. Does this allow the workqueue to overwrite the buffer while
> the probe function is actively using it, resulting in malformed requests?
Same. I'll defer to Hans for better judgement.
> [ ... ]
>
> > @@ -1327,12 +1686,18 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > goto error_hw_stop;
> >
> > /* Register LED devices */
> > - for (i = 0; i < LG_G15_LED_MAX; i++) {
> > + for (i = 0; i <= LG_G15_MACRO_RECORD; i++) {
> > ret = lg_g15_register_led(g15, i, led_names[i]);
> > if (ret)
> > goto error_hw_stop;
> > }
> >
> > + if (g15->model == LG_G710) {
> > + ret = lg_g15_register_led(g15, LG_G15_GAMEMODE, "g15::gamemode");
> > + if (ret)
> > + goto error_hw_stop;
> > + }
>
> Because hid_hw_start is called earlier in the probe sequence, is it possible
> for a hardware event to trigger lg_g710_event before these LEDs are actually
> registered?
>
> If the worker thread runs and invokes the brightness hardware changed
> notification on zero-initialized, unregistered LED devices, won't the
> internal sysfs nodes be NULL, triggering a stack trace warning in the LED
> core?
Same again.
Hans, I'll fix the 2 issues I introduced (brightness/game-mode edge
detection and error filtering). What about the pre-existing errors
reported by sashiko ?
^ permalink raw reply
* [PATCH] HID: logitech-hidpp: sync wheel multiplier on wheel mode changes
From: Lauri Saurus @ 2026-05-18 19:28 UTC (permalink / raw)
To: linux-input
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-kernel, Lauri Saurus
The hid-logitech-hidpp driver enables high resolution scrolling on
device connect for capable HID++ 2.0 devices. Driver also reads the
wheel capability and caches the returned high resolution wheel scroll
multiplier, that is used for scroll scaling when handling wheel scroll
events.
Wheel mode can also be set externally through HID++ requests, which
can leave the cached multiplier stale and cause incorrect scroll
scaling. If external SetWheelMode HID++ request sets the mode to
low resolution, the cached multiplier is not updated accordingly. This
causes extremely slow scrolling since driver expects multiple wheel
scroll events per detent but is only getting one.
The fix listens for HID++ SetWheelMode request responses and updates
the wheel scroll multiplier based on the set high resolution scroll
mode. The fix has been tested with Logitech G502X lightspeed mouse.
Signed-off-by: Lauri Saurus <saurla@saurla.com>
---
drivers/hid/hid-logitech-hidpp.c | 37 ++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ccbf28869a96..e80c4aa6115b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -207,6 +207,9 @@ struct hidpp_device {
u8 wireless_feature_index;
+ int hires_wheel_multiplier;
+ u8 hires_wheel_feature_index;
+
bool connected_once;
};
@@ -3710,6 +3713,7 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
multiplier = 1;
}
+ hidpp->hires_wheel_multiplier = multiplier;
hidpp->vertical_wheel_counter.wheel_multiplier = multiplier;
hid_dbg(hidpp->hid_dev, "wheel multiplier = %d\n", multiplier);
return 0;
@@ -3720,6 +3724,7 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
int ret;
unsigned long capabilities;
+ hidpp->hires_wheel_feature_index = 0xff;
capabilities = hidpp->capabilities;
if (hidpp->protocol_major >= 2) {
@@ -3729,6 +3734,7 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
&feature_index);
if (!ret) {
hidpp->capabilities |= HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL;
+ hidpp->hires_wheel_feature_index = feature_index;
hid_dbg(hidpp->hid_dev, "Detected HID++ 2.0 hi-res scroll wheel\n");
return 0;
}
@@ -3751,6 +3757,31 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
return 0;
}
+static int hidpp20_hires_wheel_raw_event(struct hidpp_device *hidpp,
+ u8 *data, int size)
+{
+ if (hidpp->hires_wheel_feature_index == 0xff)
+ return 0;
+
+ if (size < 5)
+ return 0;
+
+ if (data[0] != REPORT_ID_HIDPP_LONG ||
+ data[2] != hidpp->hires_wheel_feature_index)
+ return 0;
+
+ if ((data[3] & 0xf0) == CMD_HIRES_WHEEL_SET_WHEEL_MODE) {
+ u8 mode = data[4];
+ bool hires = (mode & 0x02) != 0;
+ int new_multiplier = (hires && hidpp->hires_wheel_multiplier > 0)
+ ? hidpp->hires_wheel_multiplier : 1;
+ hidpp->vertical_wheel_counter.wheel_multiplier = new_multiplier;
+ return 1;
+ }
+
+ return 0;
+}
+
/* -------------------------------------------------------------------------- */
/* Generic HID++ devices */
/* -------------------------------------------------------------------------- */
@@ -3947,6 +3978,12 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
return ret;
}
+ if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL) {
+ ret = hidpp20_hires_wheel_raw_event(hidpp, data, size);
+ if (ret != 0)
+ return ret;
+ }
+
return 0;
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: sync wheel multiplier on wheel mode changes
From: sashiko-bot @ 2026-05-18 20:10 UTC (permalink / raw)
To: Lauri Saurus; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260518192649.245691-1-saurla@saurla.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The logic incorrectly assumes that the device's response to the `CMD_HIRES_WHEEL_SET_WHEEL_MODE` command will echo the requested mode in its payload (`data[4]`), violating HID++ 2.0 protocol behavior.
- [Medium] The function `hidpp20_hires_wheel_raw_event` strictly checks for `REPORT_ID_HIDPP_LONG` (0x11), completely ignoring responses sent via Very Long (0x12) reports.
- [Low] Data race on `wheel_multiplier` and `hires_wheel_multiplier` due to unprotected concurrent accesses across interrupt and process contexts.
--
commit 83eeb25697adad15bd7ac874fb7119dc29516cf1
Author: Lauri Saurus <saurla@saurla.com>
HID: logitech-hidpp: sync wheel multiplier on wheel mode changes
The hid-logitech-hidpp driver enables high resolution scrolling on
device connect for capable HID++ 2.0 devices. Driver also reads the
wheel capability and caches the returned high resolution wheel scroll
multiplier, that is used for scroll scaling when handling wheel scroll
events.
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index b1330d23bd2d0..3afd8caf02911 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
[ ... ]
> +static int hidpp20_hires_wheel_raw_event(struct hidpp_device *hidpp,
> + u8 *data, int size)
> +{
> + if (hidpp->hires_wheel_feature_index == 0xff)
> + return 0;
> +
> + if (size < 5)
> + return 0;
> +
> + if (data[0] != REPORT_ID_HIDPP_LONG ||
> + data[2] != hidpp->hires_wheel_feature_index)
> + return 0;
Does this code need to handle VERY_LONG reports as well?
Userspace tools or newer devices might transmit FAP commands using
REPORT_ID_HIDPP_VERY_LONG (0x12). If the request is a VERY_LONG report, the
response will also be VERY_LONG, and this check would drop it, failing to
update the multiplier.
> +
> + if ((data[3] & 0xf0) == CMD_HIRES_WHEEL_SET_WHEEL_MODE) {
> + u8 mode = data[4];
> + bool hires = (mode & 0x02) != 0;
Does the HID++ 2.0 protocol guarantee that SET command responses will echo
the requested mode in the payload?
In the FAP protocol, responses to SET commands often return an empty
(zero-filled) payload. If the device returns a zero payload, data[4] would
be 0x00. The code would then evaluate hires = false and forcefully set the
multiplier to 1, even if high-resolution mode was successfully requested.
> + int new_multiplier = (hires && hidpp->hires_wheel_multiplier > 0)
> + ? hidpp->hires_wheel_multiplier : 1;
> + hidpp->vertical_wheel_counter.wheel_multiplier = new_multiplier;
Is there a potential data race on wheel_multiplier and
hires_wheel_multiplier here?
Looking at hi_res_scroll_enable(), it writes to these variables from
process context via the workqueue. Concurrently, this event handler reads
and writes to them from the HID raw event delivery path in interrupt context.
There does not appear to be any synchronization protecting these shared
variables across contexts.
> + return 1;
> + }
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518192649.245691-1-saurla@saurla.com?part=1
^ permalink raw reply
* [PATCH v6 0/4] Add MSI Claw HID Configuration Driver
From: Derek J. Clark @ 2026-05-18 22:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
This series adds an HID Configuration driver for the MSI Claw line of
Handheld Gaming PC's. The MSI Claw HID interface provides multiple
features, such as the ability to switch between xinput, dinput, and a
desktop mode, RGB control, rumble intensity, and mapping of the rear "M"
keys. There are additional gamepad modes that are not included in this
driver as they appear to be used in assembly line testing or are
incomplete in the firmware. During my testing I found them to be unstable.
The initial version of this driver was written by Denis Benato, which
contained the initial reverse-engineering and implementation for the
gamepad mode switching. This work was later expanded by Zhouwang Huang
to include more gamepad modes and additional features. Finally, I
refactored the entire driver, fixed multiple bugs, and refined the overall
format to conform to kernel driver best practices and style guide.
Claude was used initially by Zhouwang Huang to quickly parse HID captures
during the reverse-engineering of some of the features. Since Claude had
already been used, as a test of its capabilities I had it implement the
rumble intensity attribute after I had already rewritten most of the
driver, which I then manually edited to fix some mistakes. I also used
Claude to review the driver and these patches for any mistakes and bugs.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v6:
- Add send/ack pattern to ensure synchronous acks.
- Use spinlock_irqsave instead of mutex for read/write MODE event
data.
- add select NEW_LEDS to kconfig.
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Gate all attribute show/store functions with gamepad_registered or
rgb_registered, enabling use of devm_device_add_group and ending
the need to hold a mutex during remove.
- Don't set gamepad_mode on resume, MCU preserves state.
- Ensure all count variables are checked for > 0 characters before
setting buf - 1 to \n.
- Re-arm cfg_setup in resume if it was canceled in an early suspend.
- Remove duplicated argv_free macro.
- Add spinlock_irqsave vice mutex for read/write access on attribute
variables.
v5: https://lore.kernel.org/linux-input/20260517013925.3120314-1-derekjohn.clark@gmail.com/
- Swap disabled & combination mkeys_function enum values.
- Fix bug introduced in v5 where claw_buttons_store would return
-EINVAL on all valid key entries.
- Ensure mode_mutex is properly init.
- Ensure claw_remove is calling hid_hw_close and not hid_hw_stop for
all paths.
- Ensure adding "DISABLED" key to valid entries is done in the correct
patch.
- Re-enable sending an empty string to clear button mappings in
addition to setting DISABLED.
- Move adding the RGB device into cfg_setup to prevent led core
attributes from being written to prior to setup completing.
- Ensure frame_lock is properly init.
- Change variable names in RGB functions from frame and zone to f and
z respectively to fit all scoped_guard actions in 100 columns.
v4: https://lore.kernel.org/linux-input/20260516042841.500299-1-derekjohn.clark@gmail.com/
- Add msi_suspend/claw_suspend.
- Reorder claw_remove to cancel all work before removing sysfs.
- Add mutex lock for removing sysfs attributes.
- Add mutex lock for MODE command data read/write.
- Change dev_warn to dev_dbg in claw_profile_event.
- use __free with DEFINE_FREE macro for argv instead of manually
running argv_free, cleaining up scoped_guard goto.
- Fix frame_calc validity check to use >=.
- Use spinlock instead of mutex in raw_event and related attribute
_store function.
- Ensure delayed work is canceled in suspend & canceled before sysfs
attribute removal.
v3: https://lore.kernel.org/linux-input/20260515033622.2095277-1-derekjohn.clark@gmail.com/
- Add mutex for read/write if rgb frame data.
- Ensure claw_hw_output_report is properly guarded.
- Remove setting rgb_frame_count when reading rgb profiles as it always
returns garbage data.
- Ensure rgb_speed is getting drvdata from a valid lookup (not hdev).
- Use scoped_guard where necessary.
- Reoder claw_probe to ensure all mutex, completion, and variable
assignments are in place prior to setting drvdata.
- Ensure gamepad_mode is set to a valid enum value in claw_probe.
v2: https://lore.kernel.org/linux-input/20260513231445.3213501-1-derekjohn.clark@gmail.com/
- Use mutexes to guard SYNC_TO_ROM calls and pending_profile calls.
- Rename driver to hid-msi and add generic entrypoints for
probe/resume/remove that call claw specific functions in order to
future proof the driver for other MSI HID interfaces.
- Fix various bugs and formatting issues.
v1: https://lore.kernel.org/linux-input/20260510043510.442807-1-derekjohn.clark@gmail.com/
Derek J. Clark (4):
HID: hid-msi: Add MSI Claw configuration driver
HID: hid-msi: Add M-key mapping attributes
HID: hid-msi: Add RGB control interface
HID: hid-msi: Add Rumble Intensity Attributes
MAINTAINERS | 6 +
drivers/hid/Kconfig | 13 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 5 +
drivers/hid/hid-msi.c | 1766 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 1791 insertions(+)
create mode 100644 drivers/hid/hid-msi.c
--
2.53.0
^ permalink raw reply
* [PATCH v6 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: Derek J. Clark @ 2026-05-18 22:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260518222935.1802071-1-derekjohn.clark@gmail.com>
Adds configuration HID driver for the MSI Claw series of handheld PC's.
In this initial patch add the initial driver outline and attributes for
changing the gamepad mode, M-key behavior, and add a WO reset function.
Sending the SWITCH_MODE and RESET commands causes a USB disconnect in
the device. The completion will therefore never get hit and would trigger
an -EIO. To avoid showing the user an error for every write to these
attrs a bypass for the completion handling is introduced when timeout ==
0.
The initial version of this patch was written by Denis Benato, which
contained the initial reverse-engineering and implementation for the
gamepad mode switching. This work was later expanded by Zhouwang Huang
to include more gamepad modes. Finally, I refactored the drivers data
in/out flow and overall format to conform to kernel driver best
practices and style guides. Claude was used as an initial reviewer of
this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v6:
- Add send/ack pattern to ensure synchronous acks.
- Use spinlock_irqsave instead of mutex for read/write MODE event
data.
- add select NEW_LEDS to kconfig.
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Gate all attribute show/store functions with gamepad_registered,
enabling use of devm_device_add_group.
- Re-arm cfg_setup in resume if it was canceled in an early suspend.
- Don't set gamepad_mode on resume, MCU preserves state.
- Ensure all count variables are checked for > 0 characters before
setting buf - 1 to \n.
v5:
- Swap disabled & combination mkeys_function enum values.
- Ensure mode_mutex is properly init.
- Ensure claw_remove is calling hid_hw_close and not hid_hw_stop for
all paths.
v4:
- Add msi_suspend/claw_suspend.
- Reorder claw_remove to cancel all work before removing sysfs.
- Add mutex lock for removing sysfs attributes.
- Add mutex lock for MODE command data read/write.
v3:
- Ensure claw_hw_output_report is properly guarded.
- Reoder claw_probe to ensure all mutex, completion, and variable
assignments are in place prior to setting drvdata.
- Ensure gamepad_mode is set to a valid enum value in claw_probe.
v2:
- Rename driver to hid-msi from hid-msi-claw.
- Rename reusable/generic functions to msi_* from claw_*, retaining
claw specific functions.
- Add generic entrypoints for probe, remove, and raw event that route
to claw specific functions.
---
MAINTAINERS | 6 +
drivers/hid/Kconfig | 13 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 5 +
drivers/hid/hid-msi.c | 675 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 700 insertions(+)
create mode 100644 drivers/hid/hid-msi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 6f6517bf4f970..8e2de98b768f7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17965,6 +17965,12 @@ S: Odd Fixes
F: Documentation/devicetree/bindings/net/ieee802154/mrf24j40.txt
F: drivers/net/ieee802154/mrf24j40.c
+MSI HID DRIVER
+M: Derek J. Clark <derekjohn.clark@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: drivers/hid/hid-msi.c
+
MSI EC DRIVER
M: Nikita Kravets <teackot@gmail.com>
L: platform-driver-x86@vger.kernel.org
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 10c12d8e65579..7766676051a52 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -492,6 +492,19 @@ config HID_GT683R
Currently the following devices are know to be supported:
- MSI GT683R
+config HID_MSI
+ tristate "MSI Claw Gamepad Support"
+ depends on USB_HID
+ select NEW_LEDS
+ select LEDS_CLASS
+ select LEDS_CLASS_MULTICOLOR
+ help
+ Support for the MSI Claw RGB and controller configuration
+
+ Say Y here to include configuration interface support for the MSI Claw Line
+ of Handheld Console Controllers. Say M here to compile this driver as a
+ module. The module will be called hid-msi.
+
config HID_KEYTOUCH
tristate "Keytouch HID devices"
help
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 07dfdb6a49c59..80925a17b059c 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
+obj-$(CONFIG_HID_MSI) += hid-msi.o
obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
obj-$(CONFIG_HID_NINTENDO) += hid-nintendo.o
obj-$(CONFIG_HID_NTI) += hid-nti.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 933b7943bdb50..94a9b89dc240a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1047,7 +1047,12 @@
#define USB_DEVICE_ID_MOZA_R16_R21_2 0x0010
#define USB_VENDOR_ID_MSI 0x1770
+#define USB_VENDOR_ID_MSI_2 0x0db0
#define USB_DEVICE_ID_MSI_GT683R_LED_PANEL 0xff00
+#define USB_DEVICE_ID_MSI_CLAW_XINPUT 0x1901
+#define USB_DEVICE_ID_MSI_CLAW_DINPUT 0x1902
+#define USB_DEVICE_ID_MSI_CLAW_DESKTOP 0x1903
+#define USB_DEVICE_ID_MSI_CLAW_BIOS 0x1904
#define USB_VENDOR_ID_NATIONAL_SEMICONDUCTOR 0x0400
#define USB_DEVICE_ID_N_S_HARMONY 0xc359
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
new file mode 100644
index 0000000000000..c79eb0bfeb776
--- /dev/null
+++ b/drivers/hid/hid-msi.c
@@ -0,0 +1,675 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * HID driver for MSI Claw Handheld PC gamepads.
+ *
+ * Provides configuration support for the MSI Claw series of handheld PC
+ * gamepads. Multiple iterations of the device firmware has led to some
+ * quirks for how certain attributes are handled. The original firmware
+ * did not support remapping of the M1 (right) and M2 (left) rear paddles.
+ * Additionally, the MCU RAM address for writing configuration data has
+ * changed twice. Checks are done during probe to enumerate these variances.
+ *
+ * Copyright (c) 2026 Zhouwang Huang <honjow311@gmail.com>
+ * Copyright (c) 2026 Denis Benato <denis.benato@linux.dev>
+ * Copyright (c) 2026 Valve Corporation
+ */
+
+#include <linux/array_size.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/kobject.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/spinlock.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+#include <linux/usb.h>
+#include <linux/workqueue.h>
+
+#include "hid-ids.h"
+
+#define CLAW_OUTPUT_REPORT_ID 0x0f
+#define CLAW_INPUT_REPORT_ID 0x10
+
+#define CLAW_PACKET_SIZE 64
+
+#define CLAW_DINPUT_CFG_INTF_IN 0x82
+#define CLAW_XINPUT_CFG_INTF_IN 0x83
+
+enum claw_command_index {
+ CLAW_COMMAND_TYPE_READ_PROFILE = 0x04,
+ CLAW_COMMAND_TYPE_READ_PROFILE_ACK = 0x05,
+ CLAW_COMMAND_TYPE_ACK = 0x06,
+ CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA = 0x21,
+ CLAW_COMMAND_TYPE_SYNC_TO_ROM = 0x22,
+ CLAW_COMMAND_TYPE_SWITCH_MODE = 0x24,
+ CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE = 0x26,
+ CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK = 0x27,
+ CLAW_COMMAND_TYPE_RESET_DEVICE = 0x28,
+};
+
+enum claw_gamepad_mode_index {
+ CLAW_GAMEPAD_MODE_XINPUT = 0x01,
+ CLAW_GAMEPAD_MODE_DINPUT = 0x02,
+ CLAW_GAMEPAD_MODE_DESKTOP = 0x04,
+};
+
+static const char * const claw_gamepad_mode_text[] = {
+ [CLAW_GAMEPAD_MODE_XINPUT] = "xinput",
+ [CLAW_GAMEPAD_MODE_DINPUT] = "dinput",
+ [CLAW_GAMEPAD_MODE_DESKTOP] = "desktop",
+};
+
+enum claw_mkeys_function_index {
+ CLAW_MKEY_FUNCTION_MACRO,
+ CLAW_MKEY_FUNCTION_DISABLED,
+ CLAW_MKEY_FUNCTION_COMBO,
+};
+
+static const char * const claw_mkeys_function_text[] = {
+ [CLAW_MKEY_FUNCTION_MACRO] = "macro",
+ [CLAW_MKEY_FUNCTION_DISABLED] = "disabled",
+ [CLAW_MKEY_FUNCTION_COMBO] = "combination",
+};
+
+struct claw_command_report {
+ u8 report_id;
+ u8 padding[2];
+ u8 header_tail;
+ u8 cmd;
+ u8 data[59];
+} __packed;
+
+struct claw_drvdata {
+ /* MCU General Variables */
+ struct completion send_cmd_complete;
+ struct delayed_work cfg_resume;
+ struct delayed_work cfg_setup;
+ struct hid_device *hdev;
+ struct mutex cfg_mutex; /* mutex for synchronous data */
+ bool waiting_for_ack;
+ spinlock_t cmd_lock; /* Lock for cmd data read/write */
+ u8 waiting_cmd;
+ int cmd_status;
+ u8 ep;
+
+ /* Gamepad Variables */
+ enum claw_mkeys_function_index mkeys_function;
+ enum claw_gamepad_mode_index gamepad_mode;
+ bool gamepad_registered;
+ spinlock_t mode_lock; /* Lock for mode data read/write */
+};
+
+static int get_endpoint_address(struct hid_device *hdev)
+{
+ struct usb_host_endpoint *ep;
+ struct usb_interface *intf;
+
+ intf = to_usb_interface(hdev->dev.parent);
+ ep = intf->cur_altsetting->endpoint;
+ if (ep)
+ return ep->desc.bEndpointAddress;
+
+ return -ENODEV;
+}
+
+static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
+ struct claw_command_report *cmd_rep)
+{
+ if (cmd_rep->data[0] >= ARRAY_SIZE(claw_gamepad_mode_text) ||
+ !claw_gamepad_mode_text[cmd_rep->data[0]] ||
+ cmd_rep->data[1] >= ARRAY_SIZE(claw_mkeys_function_text))
+ return -EINVAL;
+
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock) {
+ drvdata->gamepad_mode = cmd_rep->data[0];
+ drvdata->mkeys_function = cmd_rep->data[1];
+ }
+
+ return 0;
+}
+
+static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct claw_command_report *cmd_rep;
+ int ret = 0;
+
+ if (size != CLAW_PACKET_SIZE)
+ return 0;
+
+ cmd_rep = (struct claw_command_report *)data;
+
+ if (cmd_rep->report_id != CLAW_INPUT_REPORT_ID || cmd_rep->header_tail != 0x3c)
+ return 0;
+
+ dev_dbg(&drvdata->hdev->dev, "Rx data as raw input report: [%*ph]\n",
+ CLAW_PACKET_SIZE, data);
+
+ switch (cmd_rep->cmd) {
+ case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
+ ret = claw_gamepad_mode_event(drvdata, cmd_rep);
+
+ scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
+ if (drvdata->waiting_for_ack &&
+ drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE) {
+ drvdata->cmd_status = ret;
+ drvdata->waiting_for_ack = false;
+ complete(&drvdata->send_cmd_complete);
+ }
+ }
+
+ break;
+ case CLAW_COMMAND_TYPE_ACK:
+ scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
+ if (drvdata->waiting_for_ack) {
+ drvdata->cmd_status = 0;
+ drvdata->waiting_for_ack = false;
+ complete(&drvdata->send_cmd_complete);
+ }
+ dev_dbg(&drvdata->hdev->dev, "Waiting CMD: %x\n", drvdata->waiting_cmd);
+ }
+
+ break;
+ default:
+ dev_dbg(&drvdata->hdev->dev, "Unknown command: %x\n", cmd_rep->cmd);
+ return 0;
+ }
+
+ return ret;
+}
+
+static int msi_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (!drvdata || (drvdata->ep != CLAW_XINPUT_CFG_INTF_IN &&
+ drvdata->ep != CLAW_DINPUT_CFG_INTF_IN))
+ return 0;
+
+ return claw_raw_event(drvdata, report, data, size);
+}
+
+static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
+ size_t len, unsigned int timeout)
+{
+ unsigned char *dmabuf __free(kfree) = NULL;
+ u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ size_t header_size = ARRAY_SIZE(header);
+ int ret;
+
+ if (header_size + len > CLAW_PACKET_SIZE)
+ return -EINVAL;
+
+ /* We can't use a devm_alloc reusable buffer without side effects during suspend */
+ dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ memcpy(dmabuf, header, header_size);
+ if (data && len)
+ memcpy(dmabuf + header_size, data, len);
+
+ guard(mutex)(&drvdata->cfg_mutex);
+ if (timeout) {
+ scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
+ drvdata->waiting_cmd = index;
+ drvdata->waiting_for_ack = true;
+ drvdata->cmd_status = -ETIMEDOUT;
+ }
+ reinit_completion(&drvdata->send_cmd_complete);
+ }
+
+ dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
+ CLAW_PACKET_SIZE, dmabuf);
+
+ ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
+ if (ret < 0)
+ return ret;
+
+ ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
+ if (ret)
+ return ret;
+
+ if (timeout) {
+ ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
+ msecs_to_jiffies(timeout));
+
+ dev_dbg(&hdev->dev, "Remaining timeout: %u\n", ret);
+ ret = ret > 0 ? drvdata->cmd_status : ret ?: -EBUSY;
+ scoped_guard(spinlock_irqsave, &drvdata->cmd_lock)
+ drvdata->waiting_for_ack = false;
+ }
+
+ return ret;
+}
+
+static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int i, ret = -EINVAL;
+ u8 data[2];
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ for (i = 0; i < ARRAY_SIZE(claw_gamepad_mode_text); i++) {
+ if (claw_gamepad_mode_text[i] && sysfs_streq(buf, claw_gamepad_mode_text[i])) {
+ ret = i;
+ break;
+ }
+ }
+ if (ret < 0)
+ return ret;
+
+ data[0] = ret;
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
+ data[1] = drvdata->mkeys_function;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t gamepad_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret, i;
+
+ if (drvdata->gamepad_registered)
+ return -ENODEV;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE, NULL, 0, 25);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
+ i = drvdata->gamepad_mode;
+
+ if (!claw_gamepad_mode_text[i] || claw_gamepad_mode_text[i][0] == '\0')
+ return sysfs_emit(buf, "unsupported\n");
+
+ return sysfs_emit(buf, "%s\n", claw_gamepad_mode_text[i]);
+}
+static DEVICE_ATTR_RW(gamepad_mode);
+
+static ssize_t gamepad_mode_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ ssize_t count = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(claw_gamepad_mode_text); i++) {
+ if (!claw_gamepad_mode_text[i] || claw_gamepad_mode_text[i][0] == '\0')
+ continue;
+ count += sysfs_emit_at(buf, count, "%s ", claw_gamepad_mode_text[i]);
+ }
+
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(gamepad_mode_index);
+
+static ssize_t mkeys_function_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int i, ret = -EINVAL;
+ u8 data[2];
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ for (i = 0; i < ARRAY_SIZE(claw_mkeys_function_text); i++) {
+ if (claw_mkeys_function_text[i] && sysfs_streq(buf, claw_mkeys_function_text[i])) {
+ ret = i;
+ break;
+ }
+ }
+ if (ret < 0)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
+ data[0] = drvdata->gamepad_mode;
+ data[1] = ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t mkeys_function_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret, i;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE, NULL, 0, 25);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
+ i = drvdata->mkeys_function;
+
+ if (i >= ARRAY_SIZE(claw_mkeys_function_text))
+ return sysfs_emit(buf, "unsupported\n");
+
+ return sysfs_emit(buf, "%s\n", claw_mkeys_function_text[i]);
+}
+static DEVICE_ATTR_RW(mkeys_function);
+
+static ssize_t mkeys_function_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i, count = 0;
+
+ for (i = 0; i < ARRAY_SIZE(claw_mkeys_function_text); i++)
+ count += sysfs_emit_at(buf, count, "%s ", claw_mkeys_function_text[i]);
+
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(mkeys_function_index);
+
+static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ bool val;
+ int ret;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ ret = kstrtobool(buf, &val);
+ if (ret)
+ return ret;
+
+ if (!val)
+ return -EINVAL;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_RESET_DEVICE, NULL, 0, 0);
+ if (ret)
+ return ret;
+
+ return count;
+}
+static DEVICE_ATTR_WO(reset);
+
+static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribute *attr,
+ int n)
+{
+ struct hid_device *hdev = to_hid_device(kobj_to_dev(kobj));
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (!drvdata) {
+ dev_warn(&hdev->dev,
+ "Failed to get drvdata from kobj. Gamepad attributes are not available.\n");
+ return 0;
+ }
+
+ return attr->mode;
+}
+
+static struct attribute *claw_gamepad_attrs[] = {
+ &dev_attr_gamepad_mode.attr,
+ &dev_attr_gamepad_mode_index.attr,
+ &dev_attr_mkeys_function.attr,
+ &dev_attr_mkeys_function_index.attr,
+ &dev_attr_reset.attr,
+ NULL,
+};
+
+static const struct attribute_group claw_gamepad_attr_group = {
+ .attrs = claw_gamepad_attrs,
+ .is_visible = claw_gamepad_attr_is_visible,
+};
+
+static void cfg_setup_fn(struct work_struct *work)
+{
+ struct delayed_work *dwork = container_of(work, struct delayed_work, work);
+ struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_setup);
+ int ret;
+
+ ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE,
+ NULL, 0, 25);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to setup device, can't read gamepad mode: %d\n", ret);
+ return;
+ }
+
+ /* Add sysfs attributes after we get the device state */
+ ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to setup device, can't create gamepad attrs: %d\n", ret);
+ return;
+ }
+ drvdata->gamepad_registered = true;
+
+ kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
+}
+
+static void cfg_resume_fn(struct work_struct *work)
+{
+ struct delayed_work *dwork = container_of(work, struct delayed_work, work);
+ struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
+
+ if (!drvdata->gamepad_registered)
+ schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
+}
+
+static int claw_probe(struct hid_device *hdev, u8 ep)
+{
+ struct claw_drvdata *drvdata;
+ int ret;
+
+ drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->gamepad_mode = CLAW_GAMEPAD_MODE_XINPUT;
+ drvdata->hdev = hdev;
+ drvdata->ep = ep;
+
+ mutex_init(&drvdata->cfg_mutex);
+ spin_lock_init(&drvdata->cmd_lock);
+ spin_lock_init(&drvdata->mode_lock);
+ init_completion(&drvdata->send_cmd_complete);
+ INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
+ INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
+
+ /* For control interface: open the HID transport for sending commands. */
+ ret = hid_hw_open(hdev);
+ if (ret)
+ return ret;
+
+ hid_set_drvdata(hdev, drvdata);
+ schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
+
+ return 0;
+}
+
+static int msi_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ u8 ep;
+
+ if (!hid_is_usb(hdev)) {
+ ret = -ENODEV;
+ goto err_probe;
+ }
+
+ ret = hid_parse(hdev);
+ if (ret)
+ goto err_probe;
+
+ /* Set quirk to create separate input devices per HID application */
+ hdev->quirks |= HID_QUIRK_INPUT_PER_APP | HID_QUIRK_MULTI_INPUT;
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret)
+ goto err_probe;
+
+ /* For non-control interfaces (keyboard/mouse), allow userspace to grab the devices. */
+ ret = get_endpoint_address(hdev);
+ if (ret < 0)
+ goto err_stop_hw;
+
+ ep = ret;
+ if (ep == CLAW_XINPUT_CFG_INTF_IN || ep == CLAW_DINPUT_CFG_INTF_IN) {
+ ret = claw_probe(hdev, ep);
+ if (ret)
+ goto err_stop_hw;
+ }
+
+ return 0;
+
+err_stop_hw:
+ hid_hw_stop(hdev);
+err_probe:
+ return dev_err_probe(&hdev->dev, ret, "Failed to init device\n");
+}
+
+static void claw_remove(struct hid_device *hdev)
+{
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (!drvdata) {
+ hid_hw_close(hdev);
+ return;
+ }
+
+ cancel_delayed_work_sync(&drvdata->cfg_setup);
+ cancel_delayed_work_sync(&drvdata->cfg_resume);
+
+ drvdata->gamepad_registered = false;
+
+ hid_hw_close(hdev);
+}
+
+static void msi_remove(struct hid_device *hdev)
+{
+ int ret;
+ u8 ep;
+
+ ret = get_endpoint_address(hdev);
+ if (ret <= 0)
+ goto hw_stop;
+
+ ep = ret;
+ if (ep == CLAW_XINPUT_CFG_INTF_IN || ep == CLAW_DINPUT_CFG_INTF_IN)
+ claw_remove(hdev);
+
+hw_stop:
+ hid_hw_stop(hdev);
+}
+
+static int claw_resume(struct hid_device *hdev)
+{
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (!drvdata)
+ return -ENODEV;
+
+ /* MCU can take up to 500ms to be ready after resume */
+ schedule_delayed_work(&drvdata->cfg_resume, msecs_to_jiffies(500));
+ return 0;
+}
+
+static int msi_resume(struct hid_device *hdev)
+{
+ int ret;
+ u8 ep;
+
+ ret = get_endpoint_address(hdev);
+ if (ret <= 0)
+ return 0;
+
+ ep = ret;
+ if (ep == CLAW_XINPUT_CFG_INTF_IN || ep == CLAW_DINPUT_CFG_INTF_IN)
+ return claw_resume(hdev);
+
+ return 0;
+}
+
+static int claw_suspend(struct hid_device *hdev)
+{
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (!drvdata)
+ return -ENODEV;
+
+ cancel_delayed_work_sync(&drvdata->cfg_setup);
+ cancel_delayed_work_sync(&drvdata->cfg_resume);
+
+ return 0;
+}
+
+static int msi_suspend(struct hid_device *hdev, pm_message_t msg)
+{
+ int ret;
+ u8 ep;
+
+ ret = get_endpoint_address(hdev);
+ if (ret <= 0)
+ return 0;
+
+ ep = ret;
+ if (ep == CLAW_XINPUT_CFG_INTF_IN || ep == CLAW_DINPUT_CFG_INTF_IN)
+ return claw_suspend(hdev);
+
+ return 0;
+}
+
+static const struct hid_device_id msi_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_MSI_2, USB_DEVICE_ID_MSI_CLAW_XINPUT) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_MSI_2, USB_DEVICE_ID_MSI_CLAW_DINPUT) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_MSI_2, USB_DEVICE_ID_MSI_CLAW_DESKTOP) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_MSI_2, USB_DEVICE_ID_MSI_CLAW_BIOS) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, msi_devices);
+
+static struct hid_driver msi_driver = {
+ .name = "hid-msi",
+ .id_table = msi_devices,
+ .raw_event = msi_raw_event,
+ .probe = msi_probe,
+ .remove = msi_remove,
+ .resume = msi_resume,
+ .suspend = pm_ptr(msi_suspend),
+};
+module_hid_driver(msi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Denis Benato <denis.benato@linux.dev>");
+MODULE_AUTHOR("Zhouwang Huang <honjow311@gmail.com>");
+MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
+MODULE_DESCRIPTION("HID driver for MSI Claw Handheld PC gamepads");
--
2.53.0
^ permalink raw reply related
* [PATCH v6 2/4] HID: hid-msi: Add M-key mapping attributes
From: Derek J. Clark @ 2026-05-18 22:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260518222935.1802071-1-derekjohn.clark@gmail.com>
Adds attributes that allow for remapping the M-keys with up to 5 values
when in macro mode. There are 2 mappable buttons on the rear of the
device, M1 on the right and M2 on the left. When mapped, the events will
fire from one of three event devices: gamepad buttons will fire from the
device handled by xpad, while keyboard and mouse events will fire from
respectively typed evdevs provided by the input core. Names of each
mapping have been kept as close to the event that will fire from the evdev
as possible, with context added to the ABS_ events on the direction of the
movement.
Initial reverse-engineering and implementation of this feature was done
by Zhouwang Huang. I refactored the overall format to conform to kernel
driver best practices and style guides. Claude was used as an initial
reviewer of this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v6:
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Gate all attribute show/store functions with gamepad_registered.
- Remove duplicated argv_free macro.
v5:
- Ensure adding "DISABLED" key to valid entries is done in the correct
patch.
- Re-enable sending an empty string to clear button mappings in
addition to setting DISABLED.
v4:
- Change dev_warn to dev_dbg in claw_profile_event.
- use __free with DEFINE_FREE macro for argv instead of manually
running argv_free, cleaining up scoped_guard goto.
v3:
- Use scoped_guard where necessary.
v2:
- Add mutex for SYNC_TO_ROM commands to ensure every SYNC is completed
before more data is written to the MCU volatile memory.
- Add mutex for profile_pending to ensure every profile action
response is serialized to the generating command.
---
drivers/hid/hid-msi.c | 415 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 414 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index c79eb0bfeb776..e58d35dba5b40 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -42,6 +42,8 @@
#define CLAW_DINPUT_CFG_INTF_IN 0x82
#define CLAW_XINPUT_CFG_INTF_IN 0x83
+#define CLAW_KEYS_MAX 5
+
enum claw_command_index {
CLAW_COMMAND_TYPE_READ_PROFILE = 0x04,
CLAW_COMMAND_TYPE_READ_PROFILE_ACK = 0x05,
@@ -66,6 +68,17 @@ static const char * const claw_gamepad_mode_text[] = {
[CLAW_GAMEPAD_MODE_DESKTOP] = "desktop",
};
+enum claw_profile_ack_pending {
+ CLAW_NO_PENDING,
+ CLAW_M1_PENDING,
+ CLAW_M2_PENDING,
+};
+
+enum claw_key_index {
+ CLAW_KEY_M1,
+ CLAW_KEY_M2,
+};
+
enum claw_mkeys_function_index {
CLAW_MKEY_FUNCTION_MACRO,
CLAW_MKEY_FUNCTION_DISABLED,
@@ -78,6 +91,155 @@ static const char * const claw_mkeys_function_text[] = {
[CLAW_MKEY_FUNCTION_COMBO] = "combination",
};
+static const struct {
+ u8 code;
+ const char *name;
+} claw_button_mapping_key_map[] = {
+ /* Gamepad buttons */
+ { 0x01, "ABS_HAT0Y_UP" },
+ { 0x02, "ABS_HAT0Y_DOWN" },
+ { 0x03, "ABS_HAT0X_LEFT" },
+ { 0x04, "ABS_HAT0X_RIGHT" },
+ { 0x05, "BTN_TL" },
+ { 0x06, "BTN_TR" },
+ { 0x07, "BTN_THUMBL" },
+ { 0x08, "BTN_THUMBR" },
+ { 0x09, "BTN_SOUTH" },
+ { 0x0a, "BTN_EAST" },
+ { 0x0b, "BTN_NORTH" },
+ { 0x0c, "BTN_WEST" },
+ { 0x0d, "BTN_MODE" },
+ { 0x0e, "BTN_SELECT" },
+ { 0x0f, "BTN_START" },
+ { 0x13, "BTN_TL2"},
+ { 0x14, "BTN_TR2"},
+ { 0x15, "ABS_Y_UP"},
+ { 0x16, "ABS_Y_DOWN"},
+ { 0x17, "ABS_X_LEFT"},
+ { 0x18, "ABS_X_RIGHT"},
+ { 0x19, "ABS_RY_UP"},
+ { 0x1a, "ABS_RY_DOWN"},
+ { 0x1b, "ABS_RX_LEFT"},
+ { 0x1c, "ABS_RX_RIGHT"},
+ /* Keyboard keys */
+ { 0x32, "KEY_ESC" },
+ { 0x33, "KEY_F1" },
+ { 0x34, "KEY_F2" },
+ { 0x35, "KEY_F3" },
+ { 0x36, "KEY_F4" },
+ { 0x37, "KEY_F5" },
+ { 0x38, "KEY_F6" },
+ { 0x39, "KEY_F7" },
+ { 0x3a, "KEY_F8" },
+ { 0x3b, "KEY_F9" },
+ { 0x3c, "KEY_F10" },
+ { 0x3d, "KEY_F11" },
+ { 0x3e, "KEY_F12" },
+ { 0x3f, "KEY_GRAVE" },
+ { 0x40, "KEY_1" },
+ { 0x41, "KEY_2" },
+ { 0x42, "KEY_3" },
+ { 0x43, "KEY_4" },
+ { 0x44, "KEY_5" },
+ { 0x45, "KEY_6" },
+ { 0x46, "KEY_7" },
+ { 0x47, "KEY_8" },
+ { 0x48, "KEY_9" },
+ { 0x49, "KEY_0" },
+ { 0x4a, "KEY_MINUS" },
+ { 0x4b, "KEY_EQUAL" },
+ { 0x4c, "KEY_BACKSPACE" },
+ { 0x4d, "KEY_TAB" },
+ { 0x4e, "KEY_Q" },
+ { 0x4f, "KEY_W" },
+ { 0x50, "KEY_E" },
+ { 0x51, "KEY_R" },
+ { 0x52, "KEY_T" },
+ { 0x53, "KEY_Y" },
+ { 0x54, "KEY_U" },
+ { 0x55, "KEY_I" },
+ { 0x56, "KEY_O" },
+ { 0x57, "KEY_P" },
+ { 0x58, "KEY_LEFTBRACE" },
+ { 0x59, "KEY_RIGHTBRACE" },
+ { 0x5a, "KEY_BACKSLASH" },
+ { 0x5b, "KEY_CAPSLOCK" },
+ { 0x5c, "KEY_A" },
+ { 0x5d, "KEY_S" },
+ { 0x5e, "KEY_D" },
+ { 0x5f, "KEY_F" },
+ { 0x60, "KEY_G" },
+ { 0x61, "KEY_H" },
+ { 0x62, "KEY_J" },
+ { 0x63, "KEY_K" },
+ { 0x64, "KEY_L" },
+ { 0x65, "KEY_SEMICOLON" },
+ { 0x66, "KEY_APOSTROPHE" },
+ { 0x67, "KEY_ENTER" },
+ { 0x68, "KEY_LEFTSHIFT" },
+ { 0x69, "KEY_Z" },
+ { 0x6a, "KEY_X" },
+ { 0x6b, "KEY_C" },
+ { 0x6c, "KEY_V" },
+ { 0x6d, "KEY_B" },
+ { 0x6e, "KEY_N" },
+ { 0x6f, "KEY_M" },
+ { 0x70, "KEY_COMMA" },
+ { 0x71, "KEY_DOT" },
+ { 0x72, "KEY_SLASH" },
+ { 0x73, "KEY_RIGHTSHIFT" },
+ { 0x74, "KEY_LEFTCTRL" },
+ { 0x75, "KEY_LEFTMETA" },
+ { 0x76, "KEY_LEFTALT" },
+ { 0x77, "KEY_SPACE" },
+ { 0x78, "KEY_RIGHTALT" },
+ { 0x79, "KEY_RIGHTCTRL" },
+ { 0x7a, "KEY_INSERT" },
+ { 0x7b, "KEY_HOME" },
+ { 0x7c, "KEY_PAGEUP" },
+ { 0x7d, "KEY_DELETE" },
+ { 0x7e, "KEY_END" },
+ { 0x7f, "KEY_PAGEDOWN" },
+ { 0x8a, "KEY_KPENTER" },
+ { 0x8b, "KEY_KP0" },
+ { 0x8c, "KEY_KP1" },
+ { 0x8d, "KEY_KP2" },
+ { 0x8e, "KEY_KP3" },
+ { 0x8f, "KEY_KP4" },
+ { 0x90, "KEY_KP5" },
+ { 0x91, "KEY_KP6" },
+ { 0x92, "KEY_KP7" },
+ { 0x93, "KEY_KP8" },
+ { 0x94, "KEY_KP9" },
+ { 0x95, "MD_PLAY" },
+ { 0x96, "MD_STOP" },
+ { 0x97, "MD_NEXT" },
+ { 0x98, "MD_PREV" },
+ { 0x99, "MD_VOL_UP" },
+ { 0x9a, "MD_VOL_DOWN" },
+ { 0x9b, "MD_VOL_MUTE" },
+ { 0x9c, "KEY_F23" },
+ /* Mouse events */
+ { 0xc8, "BTN_LEFT" },
+ { 0xc9, "BTN_MIDDLE" },
+ { 0xca, "BTN_RIGHT" },
+ { 0xcb, "BTN_SIDE" },
+ { 0xcc, "BTN_EXTRA" },
+ { 0xcd, "REL_WHEEL_UP" },
+ { 0xce, "REL_WHEEL_DOWN" },
+ { 0xff, "DISABLED" },
+};
+
+static const u16 button_mapping_addr_old[] = {
+ 0x007a, /* M1 */
+ 0x011f, /* M2 */
+};
+
+static const u16 button_mapping_addr_new[] = {
+ 0x00bb, /* M1 */
+ 0x0164, /* M2 */
+};
+
struct claw_command_report {
u8 report_id;
u8 padding[2];
@@ -88,22 +250,30 @@ struct claw_command_report {
struct claw_drvdata {
/* MCU General Variables */
+ enum claw_profile_ack_pending profile_pending;
struct completion send_cmd_complete;
struct delayed_work cfg_resume;
struct delayed_work cfg_setup;
+ struct mutex profile_mutex; /* mutex for profile_pending calls */
struct hid_device *hdev;
struct mutex cfg_mutex; /* mutex for synchronous data */
+ struct mutex rom_mutex; /* mutex for SYNC_TO_ROM calls */
bool waiting_for_ack;
spinlock_t cmd_lock; /* Lock for cmd data read/write */
u8 waiting_cmd;
int cmd_status;
+ u16 bcd_device;
u8 ep;
/* Gamepad Variables */
enum claw_mkeys_function_index mkeys_function;
enum claw_gamepad_mode_index gamepad_mode;
+ u8 m1_codes[CLAW_KEYS_MAX];
+ u8 m2_codes[CLAW_KEYS_MAX];
bool gamepad_registered;
spinlock_t mode_lock; /* Lock for mode data read/write */
+ const u16 *bmap_addr;
+ bool bmap_support;
};
static int get_endpoint_address(struct hid_device *hdev)
@@ -135,6 +305,30 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
return 0;
}
+static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
+{
+ u8 *codes;
+ int i;
+
+ switch (drvdata->profile_pending) {
+ case CLAW_M1_PENDING:
+ case CLAW_M2_PENDING:
+ codes = (drvdata->profile_pending == CLAW_M1_PENDING) ?
+ drvdata->m1_codes : drvdata->m2_codes;
+ for (i = 0; i < CLAW_KEYS_MAX; i++)
+ codes[i] = (cmd_rep->data[6 + i]);
+ break;
+ default:
+ dev_dbg(&drvdata->hdev->dev,
+ "Got profile event without changes pending from command: %x\n",
+ cmd_rep->cmd);
+ return -EINVAL;
+ }
+ drvdata->profile_pending = CLAW_NO_PENDING;
+
+ return 0;
+}
+
static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
u8 *data, int size)
{
@@ -165,6 +359,19 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
}
}
+ break;
+ case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
+ ret = claw_profile_event(drvdata, cmd_rep);
+
+ scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
+ if (drvdata->waiting_for_ack &&
+ drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE) {
+ drvdata->cmd_status = ret;
+ drvdata->waiting_for_ack = false;
+ complete(&drvdata->send_cmd_complete);
+ }
+ }
+
break;
case CLAW_COMMAND_TYPE_ACK:
scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
@@ -422,6 +629,168 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_WO(reset);
+static int button_mapping_name_to_code(const char *name)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(claw_button_mapping_key_map); i++) {
+ if (!strcmp(name, claw_button_mapping_key_map[i].name))
+ return claw_button_mapping_key_map[i].code;
+ }
+
+ return -EINVAL;
+}
+
+static const char *button_mapping_code_to_name(u8 code)
+{
+ int i;
+
+ if (code == 0xff)
+ return NULL;
+
+ for (i = 0; i < ARRAY_SIZE(claw_button_mapping_key_map); i++) {
+ if (claw_button_mapping_key_map[i].code == code)
+ return claw_button_mapping_key_map[i].name;
+ }
+
+ return NULL;
+}
+
+static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 data[] = { 0x01, (drvdata->bmap_addr[mkey_idx] >> 8) & 0xff,
+ drvdata->bmap_addr[mkey_idx] & 0xff, 0x07,
+ 0x04, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff };
+ char **raw_keys __free(argv_free) = NULL;
+ size_t len = ARRAY_SIZE(data);
+ int ret, key_count, i;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
+ if (!raw_keys)
+ return -ENOMEM;
+
+ if (key_count > CLAW_KEYS_MAX)
+ return -EINVAL;
+
+ if (key_count == 0)
+ goto set_buttons;
+
+ for (i = 0; i < key_count; i++) {
+ ret = button_mapping_name_to_code(raw_keys[i]);
+ if (ret < 0)
+ return ret;
+
+ data[6 + i] = ret;
+ }
+
+set_buttons:
+ scoped_guard(mutex, &drvdata->rom_mutex) {
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ data, len, 25);
+ if (ret)
+ return ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 25);
+ }
+
+ return ret;
+}
+
+static int claw_buttons_show(struct device *dev, char *buf, enum claw_key_index m_key)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 data[] = { 0x01, (drvdata->bmap_addr[m_key] >> 8) & 0xff,
+ drvdata->bmap_addr[m_key] & 0xff, 0x07 };
+ size_t len = ARRAY_SIZE(data);
+ int i, ret, count = 0;
+ const char *name;
+ u8 *codes;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
+
+ guard(mutex)(&drvdata->profile_mutex);
+ drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING : CLAW_M2_PENDING;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 25);
+ if (ret) {
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return ret;
+ }
+ for (i = 0; i < CLAW_KEYS_MAX; i++) {
+ name = button_mapping_code_to_name(codes[i]);
+ if (name)
+ count += sysfs_emit_at(buf, count, "%s ", name);
+ }
+
+ if (!count)
+ return sysfs_emit(buf, "(not set)\n");
+
+ buf[count - 1] = '\n';
+
+ return count;
+}
+
+static ssize_t button_m1_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+
+ ret = claw_buttons_store(dev, buf, CLAW_KEY_M1);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t button_m1_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return claw_buttons_show(dev, buf, CLAW_KEY_M1);
+}
+static DEVICE_ATTR_RW(button_m1);
+
+static ssize_t button_m2_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+
+ ret = claw_buttons_store(dev, buf, CLAW_KEY_M2);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t button_m2_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return claw_buttons_show(dev, buf, CLAW_KEY_M2);
+}
+static DEVICE_ATTR_RW(button_m2);
+
+static ssize_t button_mapping_options_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i, count = 0;
+
+ for (i = 0; i < ARRAY_SIZE(claw_button_mapping_key_map); i++)
+ count += sysfs_emit_at(buf, count, "%s ", claw_button_mapping_key_map[i].name);
+
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(button_mapping_options);
+
static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
{
@@ -434,10 +803,22 @@ static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribu
return 0;
}
- return attr->mode;
+ /* Always show attrs available on all firmware */
+ if (attr == &dev_attr_gamepad_mode.attr ||
+ attr == &dev_attr_gamepad_mode_index.attr ||
+ attr == &dev_attr_mkeys_function.attr ||
+ attr == &dev_attr_mkeys_function_index.attr ||
+ attr == &dev_attr_reset.attr)
+ return attr->mode;
+
+ /* Hide button mapping attrs if it isn't supported */
+ return drvdata->bmap_support ? attr->mode : 0;
}
static struct attribute *claw_gamepad_attrs[] = {
+ &dev_attr_button_m1.attr,
+ &dev_attr_button_m2.attr,
+ &dev_attr_button_mapping_options.attr,
&dev_attr_gamepad_mode.attr,
&dev_attr_gamepad_mode_index.attr,
&dev_attr_mkeys_function.attr,
@@ -486,8 +867,31 @@ static void cfg_resume_fn(struct work_struct *work)
schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
}
+static void claw_features_supported(struct claw_drvdata *drvdata)
+{
+ u8 major = (drvdata->bcd_device >> 8) & 0xff;
+ u8 minor = drvdata->bcd_device & 0xff;
+
+ if (major == 0x01) {
+ drvdata->bmap_support = true;
+ if (minor >= 0x66)
+ drvdata->bmap_addr = button_mapping_addr_new;
+ else
+ drvdata->bmap_addr = button_mapping_addr_old;
+ return;
+ }
+
+ if ((major == 0x02 && minor >= 0x17) || major >= 0x03) {
+ drvdata->bmap_support = true;
+ drvdata->bmap_addr = button_mapping_addr_new;
+ return;
+ }
+}
+
static int claw_probe(struct hid_device *hdev, u8 ep)
{
+ struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+ struct usb_device *udev = interface_to_usbdev(intf);
struct claw_drvdata *drvdata;
int ret;
@@ -499,7 +903,16 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
drvdata->hdev = hdev;
drvdata->ep = ep;
+ /* Determine feature level from firmware version */
+ drvdata->bcd_device = le16_to_cpu(udev->descriptor.bcdDevice);
+ claw_features_supported(drvdata);
+
+ if (!drvdata->bmap_support)
+ dev_dbg(&hdev->dev, "M-Key mapping is not supported. Update firmware to enable.\n");
+
mutex_init(&drvdata->cfg_mutex);
+ mutex_init(&drvdata->profile_mutex);
+ mutex_init(&drvdata->rom_mutex);
spin_lock_init(&drvdata->cmd_lock);
spin_lock_init(&drvdata->mode_lock);
init_completion(&drvdata->send_cmd_complete);
--
2.53.0
^ permalink raw reply related
* [PATCH v6 3/4] HID: hid-msi: Add RGB control interface
From: Derek J. Clark @ 2026-05-18 22:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260518222935.1802071-1-derekjohn.clark@gmail.com>
Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
fairly unique RGB interface. It has 9 total zones (4 per joystick ring
and 1 for the ABXY buttons), and supports up to 8 sequential frames of
RGB zone data. Each frame is written to a specific area of MCU memory by
the profile command, the value of which changes based on the firmware of
the device. Unlike other devices (such as the Legion Go or the OneXPlayer
devices), there are no hard coded effects built into the MCU. Instead,
the basic effects are provided as a series of frame data. I have
mirrored the effects available in Windows in this driver, while keeping
the effect names consistent with the Lenovo drivers for the effects that
are similar.
Initial reverse-engineering and implementation of this feature was done
by Zhouwang Huang. I refactored the overall format to conform to kernel
driver best practices and style guides. Claude was used as an initial
reviewer of this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v6:
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Gate all attribute show/store functions with rgb_registered,
enabling use of devm_device_add_group.
v5:
- Move adding the RGB device into cfg_setup to prevent led core
attributes from being written to prior to setup completing.
- Ensure frame_lock is properly init.
- Change variable names in RGB functions from frame and zone to f and
z respectively to fit all scoped_guard actions in 100 columns.
v4:
- Fix frame_calc validity check to use >=.
- USe spinlock instead of mutex in raw_event and related attribute
_store function.
- Ensure delayed work is canceled in suspend & canceled before sysfs
attribute removal.
v3:
- Add mutex for read/write of rgb frame data.
- Remove setting rgb_frame_count when reading rgb profiles as it always
returns garbage data.
- Ensure rgb_speed is getting drvdata from a valid lookup (not hdev).
v2:
- Use pending_profile mutex
---
drivers/hid/hid-msi.c | 596 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 587 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index e58d35dba5b40..4d267b896da72 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -21,6 +21,7 @@
#include <linux/device.h>
#include <linux/hid.h>
#include <linux/kobject.h>
+#include <linux/led-class-multicolor.h>
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -44,6 +45,10 @@
#define CLAW_KEYS_MAX 5
+#define CLAW_RGB_ZONES 9
+#define CLAW_RGB_MAX_FRAMES 8
+#define CLAW_RGB_FRAME_OFFSET 0x24
+
enum claw_command_index {
CLAW_COMMAND_TYPE_READ_PROFILE = 0x04,
CLAW_COMMAND_TYPE_READ_PROFILE_ACK = 0x05,
@@ -72,6 +77,7 @@ enum claw_profile_ack_pending {
CLAW_NO_PENDING,
CLAW_M1_PENDING,
CLAW_M2_PENDING,
+ CLAW_RGB_PENDING,
};
enum claw_key_index {
@@ -230,6 +236,22 @@ static const struct {
{ 0xff, "DISABLED" },
};
+enum claw_rgb_effect_index {
+ CLAW_RGB_EFFECT_MONOCOLOR,
+ CLAW_RGB_EFFECT_BREATHE,
+ CLAW_RGB_EFFECT_CHROMA,
+ CLAW_RGB_EFFECT_RAINBOW,
+ CLAW_RGB_EFFECT_FROSTFIRE,
+};
+
+static const char * const claw_rgb_effect_text[] = {
+ [CLAW_RGB_EFFECT_MONOCOLOR] = "monocolor",
+ [CLAW_RGB_EFFECT_BREATHE] = "breathe",
+ [CLAW_RGB_EFFECT_CHROMA] = "chroma",
+ [CLAW_RGB_EFFECT_RAINBOW] = "rainbow",
+ [CLAW_RGB_EFFECT_FROSTFIRE] = "frostfire",
+};
+
static const u16 button_mapping_addr_old[] = {
0x007a, /* M1 */
0x011f, /* M2 */
@@ -240,6 +262,9 @@ static const u16 button_mapping_addr_new[] = {
0x0164, /* M2 */
};
+static const u16 rgb_addr_old = 0x01fa;
+static const u16 rgb_addr_new = 0x024a;
+
struct claw_command_report {
u8 report_id;
u8 padding[2];
@@ -248,6 +273,28 @@ struct claw_command_report {
u8 data[59];
} __packed;
+struct rgb_zone {
+ u8 red;
+ u8 green;
+ u8 blue;
+};
+
+struct rgb_frame {
+ struct rgb_zone zone[CLAW_RGB_ZONES];
+};
+
+struct rgb_report {
+ u8 profile;
+ __be16 read_addr;
+ u8 frame_bytes;
+ u8 padding;
+ u8 frame_count;
+ u8 state; /* Always 0x09 */
+ u8 speed;
+ u8 brightness;
+ struct rgb_frame zone_data;
+} __packed;
+
struct claw_drvdata {
/* MCU General Variables */
enum claw_profile_ack_pending profile_pending;
@@ -274,6 +321,18 @@ struct claw_drvdata {
spinlock_t mode_lock; /* Lock for mode data read/write */
const u16 *bmap_addr;
bool bmap_support;
+
+ /* RGB Variables */
+ struct rgb_frame rgb_frames[CLAW_RGB_MAX_FRAMES];
+ enum claw_rgb_effect_index rgb_effect;
+ struct led_classdev_mc led_mc;
+ struct delayed_work rgb_queue;
+ spinlock_t frame_lock; /* lock for rgb_frames read/write */
+ bool rgb_registered;
+ u8 rgb_frame_count;
+ bool rgb_enabled;
+ u8 rgb_speed;
+ u16 rgb_addr;
};
static int get_endpoint_address(struct hid_device *hdev)
@@ -307,8 +366,11 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
{
- u8 *codes;
- int i;
+ struct rgb_report *frame;
+ u16 rgb_addr, read_addr;
+ u8 *codes, f_idx;
+ u16 frame_calc;
+ int i, ret = 0;
switch (drvdata->profile_pending) {
case CLAW_M1_PENDING:
@@ -318,6 +380,35 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
for (i = 0; i < CLAW_KEYS_MAX; i++)
codes[i] = (cmd_rep->data[6 + i]);
break;
+ case CLAW_RGB_PENDING:
+ frame = (struct rgb_report *)cmd_rep->data;
+ rgb_addr = drvdata->rgb_addr;
+ read_addr = be16_to_cpu(frame->read_addr);
+ frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
+ if (frame_calc >= CLAW_RGB_MAX_FRAMES) {
+ dev_err(&drvdata->hdev->dev, "Got unsupported frame index: %x\n",
+ frame_calc);
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return -EINVAL;
+ }
+ f_idx = frame_calc;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
+ sizeof(struct rgb_frame));
+
+ /* Only use frame 0 for remaining variable assignment */
+ if (f_idx != 0)
+ break;
+
+ drvdata->rgb_speed = frame->speed;
+ drvdata->led_mc.led_cdev.brightness = frame->brightness;
+ drvdata->led_mc.subled_info[0].intensity = frame->zone_data.zone[0].red;
+ drvdata->led_mc.subled_info[1].intensity = frame->zone_data.zone[0].green;
+ drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
+ }
+
+ break;
default:
dev_dbg(&drvdata->hdev->dev,
"Got profile event without changes pending from command: %x\n",
@@ -326,7 +417,7 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
}
drvdata->profile_pending = CLAW_NO_PENDING;
- return 0;
+ return ret;
}
static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
@@ -832,6 +923,441 @@ static const struct attribute_group claw_gamepad_attr_group = {
.is_visible = claw_gamepad_attr_is_visible,
};
+/* Read RGB config from device */
+static int claw_read_rgb_config(struct hid_device *hdev)
+{
+ u8 data[4] = { 0x01, 0x00, 0x00, CLAW_RGB_FRAME_OFFSET };
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u16 read_addr = drvdata->rgb_addr;
+ size_t len = ARRAY_SIZE(data);
+ int ret, i;
+
+ if (!drvdata->rgb_addr)
+ return -ENODEV;
+
+ /* Loop through all 8 pages of RGB data */
+ guard(mutex)(&drvdata->profile_mutex);
+ for (i = 0; i < 8; i++) {
+ drvdata->profile_pending = CLAW_RGB_PENDING;
+ data[1] = (read_addr >> 8) & 0xff;
+ data[2] = read_addr & 0x00ff;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 25);
+ if (ret) {
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return ret;
+ }
+ read_addr += CLAW_RGB_FRAME_OFFSET;
+ }
+
+ return 0;
+}
+
+/* Send RGB configuration to device */
+static int claw_write_rgb_state(struct claw_drvdata *drvdata)
+{
+ struct rgb_report report = { 0x01, 0x0000, CLAW_RGB_FRAME_OFFSET, 0x00,
+ drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
+ drvdata->led_mc.led_cdev.brightness };
+ u16 write_addr = drvdata->rgb_addr;
+ size_t len = sizeof(report);
+ int f, ret;
+
+ if (!drvdata->rgb_addr)
+ return -ENODEV;
+
+ if (!drvdata->rgb_frame_count)
+ return -EINVAL;
+
+ guard(mutex)(&drvdata->rom_mutex);
+ /* Loop through (up to) 8 pages of RGB data */
+ for (f = 0; f < drvdata->rgb_frame_count; f++) {
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock)
+ report.zone_data = drvdata->rgb_frames[f];
+
+ /* Set the MCU address to write the frame data to */
+ report.read_addr = cpu_to_be16(write_addr);
+
+ /* Serialize the rgb_report and write it to MCU */
+ ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ (u8 *)&report, len, 25);
+ if (ret)
+ return ret;
+
+ /* Increment the write addr by the offset for the next frame */
+ write_addr += CLAW_RGB_FRAME_OFFSET;
+ }
+
+ ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 25);
+
+ return ret;
+}
+
+/* Fill all zones with the same color */
+static void claw_frame_fill_solid(struct rgb_frame *frame, struct rgb_zone zone)
+{
+ int z;
+
+ for (z = 0; z < CLAW_RGB_ZONES; z++)
+ frame->zone[z] = zone;
+}
+
+/* Apply solid effect (1 frame, no color) */
+static int claw_apply_disabled(struct claw_drvdata *drvdata)
+{
+ struct rgb_zone off = { 0x00, 0x00, 0x00};
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = 1;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], off);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply solid effect (1 frame, all zones same color) */
+static int claw_apply_monocolor(struct claw_drvdata *drvdata)
+{
+ struct mc_subled *subleds = drvdata->led_mc.subled_info;
+ struct rgb_zone zone = { subleds[0].intensity, subleds[1].intensity,
+ subleds[2].intensity };
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = 1;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], zone);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply breathe effect (2 frames: color -> off) */
+static int claw_apply_breathe(struct claw_drvdata *drvdata)
+{
+ struct mc_subled *subleds = drvdata->led_mc.subled_info;
+ struct rgb_zone zone = { subleds[0].intensity, subleds[1].intensity,
+ subleds[2].intensity };
+ static const struct rgb_zone off = { 0, 0, 0 };
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = 2;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], zone);
+ claw_frame_fill_solid(&drvdata->rgb_frames[1], off);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply chroma effect (6 frames: rainbow cycle, all zones sync) */
+static int claw_apply_chroma(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone colors[] = {
+ {255, 0, 0}, /* red */
+ {255, 255, 0}, /* yellow */
+ { 0, 255, 0}, /* green */
+ { 0, 255, 255}, /* cyan */
+ { 0, 0, 255}, /* blue */
+ {255, 0, 255}, /* magenta */
+ };
+ u8 frame_count = ARRAY_SIZE(colors);
+ int f;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = frame_count;
+
+ for (f = 0; f < frame_count; f++)
+ claw_frame_fill_solid(&drvdata->rgb_frames[f], colors[f]);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply rainbow effect (4 frames: rotating colors around joysticks) */
+static int claw_apply_rainbow(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone colors[] = {
+ {255, 0, 0}, /* red */
+ { 0, 255, 0}, /* green */
+ { 0, 255, 255}, /* cyan */
+ { 0, 0, 255}, /* blue */
+ };
+ u8 frame_count = ARRAY_SIZE(colors);
+ int f, z;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = frame_count;
+
+ for (f = 0; f < frame_count; f++) {
+ for (z = 0; z < 4; z++) {
+ drvdata->rgb_frames[f].zone[z] = colors[(z + f) % 4];
+ drvdata->rgb_frames[f].zone[z + 4] = colors[(z + f) % 4];
+ }
+ drvdata->rgb_frames[f].zone[8] = colors[f];
+ }
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/*
+ * Apply frostfire effect (4 frames: fire vs ice rotating)
+ * Right joystick: fire red -> dark -> ice blue -> dark (clockwise)
+ * Left joystick: ice blue -> dark -> fire red -> dark (counter-clockwise)
+ * ABXY: fire red -> dark -> ice blue -> dark
+ */
+static int claw_apply_frostfire(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone colors[] = {
+ {255, 0, 0}, /* fire red */
+ { 0, 0, 0}, /* dark */
+ { 0, 0, 255}, /* ice blue */
+ { 0, 0, 0}, /* dark */
+ };
+ u8 frame_count = ARRAY_SIZE(colors);
+ int f, z;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = frame_count;
+
+ for (f = 0; f < frame_count; f++) {
+ for (z = 0; z < 4; z++) {
+ drvdata->rgb_frames[f].zone[z] = colors[(z + f) % 4];
+ drvdata->rgb_frames[f].zone[z + 4] = colors[(z - f + 6) % 4];
+ }
+ drvdata->rgb_frames[f].zone[8] = colors[f];
+ }
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply current state to device */
+static int claw_apply_rgb_state(struct claw_drvdata *drvdata)
+{
+ if (!drvdata->rgb_enabled)
+ return claw_apply_disabled(drvdata);
+
+ switch (drvdata->rgb_effect) {
+ case CLAW_RGB_EFFECT_MONOCOLOR:
+ return claw_apply_monocolor(drvdata);
+ case CLAW_RGB_EFFECT_BREATHE:
+ return claw_apply_breathe(drvdata);
+ case CLAW_RGB_EFFECT_CHROMA:
+ return claw_apply_chroma(drvdata);
+ case CLAW_RGB_EFFECT_RAINBOW:
+ return claw_apply_rainbow(drvdata);
+ case CLAW_RGB_EFFECT_FROSTFIRE:
+ return claw_apply_frostfire(drvdata);
+ default:
+ dev_err(drvdata->led_mc.led_cdev.dev,
+ "No supported rgb_effect selected\n");
+ return -EINVAL;
+ }
+}
+
+static void claw_rgb_queue_fn(struct work_struct *work)
+{
+ struct delayed_work *dwork = container_of(work, struct delayed_work, work);
+ struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, rgb_queue);
+ int ret;
+
+ if (!drvdata->rgb_registered)
+ return;
+
+ ret = claw_apply_rgb_state(drvdata);
+ if (ret)
+ dev_err(drvdata->led_mc.led_cdev.dev,
+ "Failed to apply RGB state: %d\n", ret);
+}
+
+static ssize_t effect_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ int ret;
+
+ if (!drvdata->rgb_registered)
+ return -ENODEV;
+
+ ret = sysfs_match_string(claw_rgb_effect_text, buf);
+ if (ret < 0)
+ return ret;
+
+ drvdata->rgb_effect = ret;
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t effect_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+
+ if (!drvdata->rgb_registered)
+ return -ENODEV;
+
+ if (drvdata->rgb_effect >= ARRAY_SIZE(claw_rgb_effect_text))
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%s\n", claw_rgb_effect_text[drvdata->rgb_effect]);
+}
+
+static DEVICE_ATTR_RW(effect);
+
+static ssize_t effect_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i, count = 0;
+
+ for (i = 0; i < ARRAY_SIZE(claw_rgb_effect_text); i++)
+ count += sysfs_emit_at(buf, count, "%s ", claw_rgb_effect_text[i]);
+
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(effect_index);
+
+static ssize_t enabled_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ bool val;
+ int ret;
+
+ if (!drvdata->rgb_registered)
+ return -ENODEV;
+
+ ret = kstrtobool(buf, &val);
+ if (ret)
+ return ret;
+
+ drvdata->rgb_enabled = val;
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t enabled_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+
+ if (!drvdata->rgb_registered)
+ return -ENODEV;
+
+ return sysfs_emit(buf, "%s\n", drvdata->rgb_enabled ? "true" : "false");
+}
+static DEVICE_ATTR_RW(enabled);
+
+static ssize_t enabled_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "true false\n");
+}
+static DEVICE_ATTR_RO(enabled_index);
+
+static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ unsigned int val, speed;
+ int ret;
+
+ if (!drvdata->rgb_registered)
+ return -ENODEV;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 20)
+ return -EINVAL;
+
+ /* 0 is fastest, invert value for intuitive userspace speed */
+ speed = 20 - val;
+
+ drvdata->rgb_speed = speed;
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ u8 speed = 20 - drvdata->rgb_speed;
+
+ if (!drvdata->rgb_registered)
+ return -ENODEV;
+
+ return sysfs_emit(buf, "%u\n", speed);
+}
+static DEVICE_ATTR_RW(speed);
+
+static ssize_t speed_range_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "0-20\n");
+}
+static DEVICE_ATTR_RO(speed_range);
+
+static void claw_led_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness _brightness)
+{
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+
+ if (!drvdata->rgb_registered)
+ return;
+
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+}
+
+static struct attribute *claw_rgb_attrs[] = {
+ &dev_attr_effect.attr,
+ &dev_attr_effect_index.attr,
+ &dev_attr_enabled.attr,
+ &dev_attr_enabled_index.attr,
+ &dev_attr_speed.attr,
+ &dev_attr_speed_range.attr,
+ NULL,
+};
+
+static const struct attribute_group claw_rgb_attr_group = {
+ .attrs = claw_rgb_attrs,
+};
+
+static struct mc_subled claw_rgb_subled_info[] = {
+ {
+ .color_index = LED_COLOR_ID_RED,
+ .channel = 0x1,
+ },
+ {
+ .color_index = LED_COLOR_ID_GREEN,
+ .channel = 0x2,
+ },
+ {
+ .color_index = LED_COLOR_ID_BLUE,
+ .channel = 0x3,
+ },
+};
+
static void cfg_setup_fn(struct work_struct *work)
{
struct delayed_work *dwork = container_of(work, struct delayed_work, work);
@@ -846,16 +1372,44 @@ static void cfg_setup_fn(struct work_struct *work)
return;
}
- /* Add sysfs attributes after we get the device state */
- ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
+ ret = claw_read_rgb_config(drvdata->hdev);
if (ret) {
dev_err(&drvdata->hdev->dev,
- "Failed to setup device, can't create gamepad attrs: %d\n", ret);
+ "Failed to setup device, can't read RGB config: %d\n", ret);
return;
}
- drvdata->gamepad_registered = true;
+
+ /* Add sysfs attributes after we get the device state */
+ if (!drvdata->gamepad_registered) {
+ ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to setup device, can't create gamepad attrs: %d\n", ret);
+ return;
+ }
+ drvdata->gamepad_registered = true;
+ }
+
+ /* Add and enable RGB interface once we have the device state */
+ if (!drvdata->rgb_registered) {
+ ret = devm_led_classdev_multicolor_register(&drvdata->hdev->dev, &drvdata->led_mc);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to setup device, can't create led device: %d\n", ret);
+ return;
+ }
+
+ ret = devm_device_add_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to setup device, can't create led attributes: %d\n", ret);
+ return;
+ }
+ drvdata->rgb_registered = true;
+ }
kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
+ kobject_uevent(&drvdata->led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
}
static void cfg_resume_fn(struct work_struct *work)
@@ -874,18 +1428,24 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
if (major == 0x01) {
drvdata->bmap_support = true;
- if (minor >= 0x66)
+ if (minor >= 0x66) {
drvdata->bmap_addr = button_mapping_addr_new;
- else
+ drvdata->rgb_addr = rgb_addr_new;
+ } else {
drvdata->bmap_addr = button_mapping_addr_old;
+ drvdata->rgb_addr = rgb_addr_old;
+ }
return;
}
if ((major == 0x02 && minor >= 0x17) || major >= 0x03) {
drvdata->bmap_support = true;
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rgb_addr = rgb_addr_new;
return;
}
+
+ drvdata->rgb_addr = rgb_addr_old;
}
static int claw_probe(struct hid_device *hdev, u8 ep)
@@ -900,6 +1460,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
return -ENOMEM;
drvdata->gamepad_mode = CLAW_GAMEPAD_MODE_XINPUT;
+ drvdata->rgb_enabled = true;
drvdata->hdev = hdev;
drvdata->ep = ep;
@@ -910,14 +1471,27 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
if (!drvdata->bmap_support)
dev_dbg(&hdev->dev, "M-Key mapping is not supported. Update firmware to enable.\n");
+ drvdata->led_mc.led_cdev.name = "msi_claw:rgb:joystick_rings";
+ drvdata->led_mc.led_cdev.brightness = 0x50;
+ drvdata->led_mc.led_cdev.max_brightness = 0x64;
+ drvdata->led_mc.led_cdev.color = LED_COLOR_ID_RGB;
+ drvdata->led_mc.led_cdev.brightness_set = claw_led_brightness_set;
+ drvdata->led_mc.num_colors = 3;
+ drvdata->led_mc.subled_info = devm_kmemdup(&hdev->dev, claw_rgb_subled_info,
+ sizeof(claw_rgb_subled_info), GFP_KERNEL);
+ if (!drvdata->led_mc.subled_info)
+ return -ENOMEM;
+
mutex_init(&drvdata->cfg_mutex);
mutex_init(&drvdata->profile_mutex);
mutex_init(&drvdata->rom_mutex);
spin_lock_init(&drvdata->cmd_lock);
spin_lock_init(&drvdata->mode_lock);
+ spin_lock_init(&drvdata->frame_lock);
init_completion(&drvdata->send_cmd_complete);
INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
+ INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);
/* For control interface: open the HID transport for sending commands. */
ret = hid_hw_open(hdev);
@@ -979,10 +1553,14 @@ static void claw_remove(struct hid_device *hdev)
return;
}
+ /* Block writes to brightness/multi_intensity during teardown */
+ drvdata->led_mc.led_cdev.brightness_set = NULL;
cancel_delayed_work_sync(&drvdata->cfg_setup);
cancel_delayed_work_sync(&drvdata->cfg_resume);
+ cancel_delayed_work_sync(&drvdata->rgb_queue);
drvdata->gamepad_registered = false;
+ drvdata->rgb_enabled = false;
hid_hw_close(hdev);
}
--
2.53.0
^ permalink raw reply related
* [PATCH v6 4/4] HID: hid-msi: Add Rumble Intensity Attributes
From: Derek J. Clark @ 2026-05-18 22:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260518222935.1802071-1-derekjohn.clark@gmail.com>
Adds intensity adjustment for the left and right rumble motors.
Claude was used during the reverse-engineering data gathering for this
feature done by Zhouwang Huang. As the code had already been affected,
I used Claude to create the initial framing for the feature, then did
manual cleanup of the _show and _store functions afterwards to fix bugs
and keep the coding style consistent. Claude was also used as an initial
reviewer of this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v6:
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Add spinlock_irqsave for read/write access on rumble_intensity
variables.
- Gate all attribute show/store functions with gamepad_registered.
v5:
- Remove mkey related changes.
v2:
- Use pending_profile and sync to rom mutexes.
---
drivers/hid/hid-msi.c | 167 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 167 insertions(+)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 4d267b896da72..339f256b60dbe 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -78,6 +78,8 @@ enum claw_profile_ack_pending {
CLAW_M1_PENDING,
CLAW_M2_PENDING,
CLAW_RGB_PENDING,
+ CLAW_RUMBLE_LEFT_PENDING,
+ CLAW_RUMBLE_RIGHT_PENDING,
};
enum claw_key_index {
@@ -265,6 +267,11 @@ static const u16 button_mapping_addr_new[] = {
static const u16 rgb_addr_old = 0x01fa;
static const u16 rgb_addr_new = 0x024a;
+static const u16 rumble_addr[] = {
+ 0x0022, /* left */
+ 0x0023, /* right */
+};
+
struct claw_command_report {
u8 report_id;
u8 padding[2];
@@ -317,9 +324,13 @@ struct claw_drvdata {
enum claw_gamepad_mode_index gamepad_mode;
u8 m1_codes[CLAW_KEYS_MAX];
u8 m2_codes[CLAW_KEYS_MAX];
+ u8 rumble_intensity_right;
+ u8 rumble_intensity_left;
bool gamepad_registered;
+ spinlock_t rumble_lock; /* lock for rumble_intensity read/write */
spinlock_t mode_lock; /* Lock for mode data read/write */
const u16 *bmap_addr;
+ bool rumble_support;
bool bmap_support;
/* RGB Variables */
@@ -409,6 +420,14 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
}
break;
+ case CLAW_RUMBLE_LEFT_PENDING:
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ drvdata->rumble_intensity_left = cmd_rep->data[4];
+ break;
+ case CLAW_RUMBLE_RIGHT_PENDING:
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ drvdata->rumble_intensity_right = cmd_rep->data[4];
+ break;
default:
dev_dbg(&drvdata->hdev->dev,
"Got profile event without changes pending from command: %x\n",
@@ -882,6 +901,142 @@ static ssize_t button_mapping_options_show(struct device *dev,
}
static DEVICE_ATTR_RO(button_mapping_options);
+static ssize_t rumble_intensity_left_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 data[] = { 0x01, (rumble_addr[0] >> 8) & 0xff, rumble_addr[0] & 0xff, 0x01, 0x00 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 val;
+ int ret;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 100)
+ return -EINVAL;
+
+ data[4] = val;
+
+ guard(mutex)(&drvdata->rom_mutex);
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ data, ARRAY_SIZE(data), 25);
+ if (ret)
+ return ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 25);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t rumble_intensity_left_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ u8 data[4] = { 0x01, (rumble_addr[0] >> 8) & 0xff, rumble_addr[0] & 0xff, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+ u8 val;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ guard(mutex)(&drvdata->profile_mutex);
+ drvdata->profile_pending = CLAW_RUMBLE_LEFT_PENDING;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data,
+ ARRAY_SIZE(data), 25);
+ if (ret) {
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return ret;
+ }
+
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ val = drvdata->rumble_intensity_left;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+static DEVICE_ATTR_RW(rumble_intensity_left);
+
+static ssize_t rumble_intensity_right_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 data[] = { 0x01, (rumble_addr[1] >> 8) & 0xff, rumble_addr[1] & 0xff, 0x01, 0x00 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 val;
+ int ret;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 100)
+ return -EINVAL;
+
+ data[4] = val;
+
+ guard(mutex)(&drvdata->rom_mutex);
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ data, ARRAY_SIZE(data), 25);
+ if (ret)
+ return ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 25);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t rumble_intensity_right_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ u8 data[4] = { 0x01, (rumble_addr[1] >> 8) & 0xff, rumble_addr[1] & 0xff, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+ u8 val;
+
+ if (!drvdata->gamepad_registered)
+ return -ENODEV;
+
+ guard(mutex)(&drvdata->profile_mutex);
+ drvdata->profile_pending = CLAW_RUMBLE_RIGHT_PENDING;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data,
+ ARRAY_SIZE(data), 25);
+ if (ret) {
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return ret;
+ }
+
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ val = drvdata->rumble_intensity_right;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+static DEVICE_ATTR_RW(rumble_intensity_right);
+
+static ssize_t rumble_intensity_range_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "0-100\n");
+}
+static DEVICE_ATTR_RO(rumble_intensity_range);
+
static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
{
@@ -902,6 +1057,12 @@ static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribu
attr == &dev_attr_reset.attr)
return attr->mode;
+ /* Hide rumble attrs if not supported */
+ if (attr == &dev_attr_rumble_intensity_left.attr ||
+ attr == &dev_attr_rumble_intensity_right.attr ||
+ attr == &dev_attr_rumble_intensity_range.attr)
+ return drvdata->rumble_support ? attr->mode : 0;
+
/* Hide button mapping attrs if it isn't supported */
return drvdata->bmap_support ? attr->mode : 0;
}
@@ -915,6 +1076,9 @@ static struct attribute *claw_gamepad_attrs[] = {
&dev_attr_mkeys_function.attr,
&dev_attr_mkeys_function_index.attr,
&dev_attr_reset.attr,
+ &dev_attr_rumble_intensity_left.attr,
+ &dev_attr_rumble_intensity_right.attr,
+ &dev_attr_rumble_intensity_range.attr,
NULL,
};
@@ -1430,6 +1594,7 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
drvdata->bmap_support = true;
if (minor >= 0x66) {
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rumble_support = true;
drvdata->rgb_addr = rgb_addr_new;
} else {
drvdata->bmap_addr = button_mapping_addr_old;
@@ -1441,6 +1606,7 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
if ((major == 0x02 && minor >= 0x17) || major >= 0x03) {
drvdata->bmap_support = true;
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rumble_support = true;
drvdata->rgb_addr = rgb_addr_new;
return;
}
@@ -1488,6 +1654,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
spin_lock_init(&drvdata->cmd_lock);
spin_lock_init(&drvdata->mode_lock);
spin_lock_init(&drvdata->frame_lock);
+ spin_lock_init(&drvdata->rumble_lock);
init_completion(&drvdata->send_cmd_complete);
INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v6 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: sashiko-bot @ 2026-05-18 22:57 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260518222935.1802071-2-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Inverted registration check in `gamepad_mode_show` breaks functionality and allows access after device removal.
- [High] TOCTOU race in sysfs attributes leading to Use-After-Free during device teardown.
- [High] Race condition with `reinit_completion` and unprotected state on error paths in `claw_hw_output_report`.
- [High] Incorrect cancellation order of interdependent delayed works causes a Use-After-Free.
- [High] Brittle endpoint check in `msi_remove` bypasses teardown and causes unbalanced `hid_hw_close`.
--
commit bae529985c447afb063f1f68fb0ebbfec812a32f
Author: Derek J. Clark <derekjohn.clark@gmail.com>
Subject: HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PC's. It provides an initial driver outline and attributes for
changing the gamepad mode, M-key behavior, and handling resets.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..c79eb0bfeb776
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> + size_t len, unsigned int timeout)
> +{
> + unsigned char *dmabuf __free(kfree) = NULL;
> + u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + size_t header_size = ARRAY_SIZE(header);
> + int ret;
> +
> + if (header_size + len > CLAW_PACKET_SIZE)
> + return -EINVAL;
> +
> + /* We can't use a devm_alloc reusable buffer without side effects during suspend */
> + dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + memcpy(dmabuf, header, header_size);
> + if (data && len)
> + memcpy(dmabuf + header_size, data, len);
> +
> + guard(mutex)(&drvdata->cfg_mutex);
> + if (timeout) {
> + scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
> + drvdata->waiting_cmd = index;
> + drvdata->waiting_for_ack = true;
> + drvdata->cmd_status = -ETIMEDOUT;
> + }
> + reinit_completion(&drvdata->send_cmd_complete);
Could this race with an incoming ACK? Since reinit_completion() is called
outside cmd_lock after setting waiting_for_ack = true, an ACK arriving in
this window could call complete(), which would then be overwritten, causing
a command timeout.
> + }
> +
> + dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
> + CLAW_PACKET_SIZE, dmabuf);
> +
> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> + if (ret < 0)
> + return ret;
> +
> + ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> + if (ret)
> + return ret;
If hid_hw_output_report() fails or returns a partial write, the function
returns early without resetting waiting_for_ack to false. Could this corrupt
the state machine and cause stray ACKs to trigger spurious completions later?
[ ... ]
> +static ssize_t gamepad_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + int ret, i;
> +
> + if (drvdata->gamepad_registered)
> + return -ENODEV;
Should this be if (!drvdata->gamepad_registered)? Without the logical NOT,
this prevents reading the mode during normal operation and allows access
during teardown, which could lead to a use-after-free since the sysfs
attribute can be accessed after device removal.
[ ... ]
> +static ssize_t mkeys_function_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + int i, ret = -EINVAL;
> + u8 data[2];
> +
> + if (!drvdata->gamepad_registered)
> + return -ENODEV;
Is there a race condition here during device teardown? The check for
gamepad_registered happens locklessly before cfg_mutex is acquired in
claw_hw_output_report().
If claw_remove() executes concurrently, sets gamepad_registered to false,
and closes the hardware, could this result in calling hid_hw_output_report()
on a stopped device and causing a use-after-free when the sysfs thread
resumes?
[ ... ]
> +static void claw_remove(struct hid_device *hdev)
> +{
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> + if (!drvdata) {
> + hid_hw_close(hdev);
> + return;
> + }
> +
> + cancel_delayed_work_sync(&drvdata->cfg_setup);
> + cancel_delayed_work_sync(&drvdata->cfg_resume);
Should cfg_resume be cancelled before cfg_setup? Since cfg_resume_fn() can
schedule cfg_setup, cancelling cfg_setup first might allow cfg_resume_fn()
to queue it again. If this happens, the newly scheduled cfg_setup work could
access freed drvdata memory after claw_remove() finishes.
> +
> + drvdata->gamepad_registered = false;
> +
> + hid_hw_close(hdev);
> +}
> +
> +static void msi_remove(struct hid_device *hdev)
> +{
> + int ret;
> + u8 ep;
> +
> + ret = get_endpoint_address(hdev);
> + if (ret <= 0)
> + goto hw_stop;
> +
> + ep = ret;
> + if (ep == CLAW_XINPUT_CFG_INTF_IN || ep == CLAW_DINPUT_CFG_INTF_IN)
> + claw_remove(hdev);
Is it safe to rely on get_endpoint_address() during removal?
If userspace modifies the USB alternate setting via usbfs while the driver
is bound, the endpoint could change, causing claw_remove() to be skipped
and leaving the workqueues running, leading to a use-after-free.
Conversely, if the endpoint didn't match during probe but does match during
removal, claw_remove() might be called with a NULL drvdata and execute
hid_hw_close() without a prior hid_hw_open(), underflowing ll_open_count.
[ ... ]
> +static int claw_suspend(struct hid_device *hdev)
> +{
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> + if (!drvdata)
> + return -ENODEV;
> +
> + cancel_delayed_work_sync(&drvdata->cfg_setup);
> + cancel_delayed_work_sync(&drvdata->cfg_resume);
Does this have the same cancellation order issue as claw_remove(), where
cfg_resume_fn() could reschedule cfg_setup after it has been cancelled?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518222935.1802071-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v6 2/4] HID: hid-msi: Add M-key mapping attributes
From: sashiko-bot @ 2026-05-18 23:29 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260518222935.1802071-3-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race and state desynchronization in `profile_pending` allows delayed ACKs to corrupt profile data.
- [High] Race window in `claw_hw_output_report()` causes spurious command timeouts.
- [High] Use-After-Free during module teardown due to incorrect delayed work cancellation order.
--
commit d45c51fc226fdd946c2d70a93c254be199300dc7
Author: Derek J. Clark <derekjohn.clark@gmail.com>
Subject: HID: hid-msi: Add M-key mapping attributes
This commit introduces M-key mapping attributes for the MSI Claw device.
It adds the ability to read and write profile mappings for two back buttons
(M1 and M2) via sysfs, using custom HID commands to communicate with the
hardware.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index c79eb0bfeb776..e58d35dba5b40 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> struct claw_drvdata {
> /* MCU General Variables */
> + enum claw_profile_ack_pending profile_pending;
> struct completion send_cmd_complete;
> struct delayed_work cfg_resume;
> struct delayed_work cfg_setup;
This problem wasn't introduced by this patch, but does the cancellation order
of these delayed works cause a use-after-free regression during module
teardown?
In cfg_resume_fn(), cfg_setup is unconditionally queued via
schedule_delayed_work(). During module removal, if
cancel_delayed_work_sync(&drvdata->cfg_setup) is executed before
cancel_delayed_work_sync(&drvdata->cfg_resume), and cfg_resume_fn is
actively executing:
The first cancellation would return immediately because cfg_setup is not
yet pending. cfg_resume_fn would then queue cfg_setup. The second
cancellation would wait for cfg_resume_fn to finish.
Once the removal completes and drvdata is freed by devres, could the
newly queued cfg_setup fire 500ms later and dereference the freed
drvdata pointer?
[ ... ]
> +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> +{
> + u8 *codes;
> + int i;
> +
> + switch (drvdata->profile_pending) {
> + case CLAW_M1_PENDING:
> + case CLAW_M2_PENDING:
> + codes = (drvdata->profile_pending == CLAW_M1_PENDING) ?
> + drvdata->m1_codes : drvdata->m2_codes;
> + for (i = 0; i < CLAW_KEYS_MAX; i++)
> + codes[i] = (cmd_rep->data[6 + i]);
> + break;
> + default:
> + dev_dbg(&drvdata->hdev->dev,
> + "Got profile event without changes pending from command: %x\n",
> + cmd_rep->cmd);
> + return -EINVAL;
> + }
> + drvdata->profile_pending = CLAW_NO_PENDING;
> +
> + return 0;
> +}
Is there a data race and potential state desynchronization with
profile_pending here?
drvdata->profile_pending is written under profile_mutex in process context
in claw_buttons_show(), but it is read and cleared locklessly here in the
atomic HID event handler.
If a request (like an M1 read) times out, profile_pending is cleared by the
sysfs thread. If a subsequent request then sets profile_pending to
CLAW_M2_PENDING, could a concurrently arriving delayed ACK for the original
M1 read cause this lockless handler to mistakenly parse the M1 payload into
drvdata->m2_codes?
This would reset profile_pending and prematurely wake the waiting thread,
corrupting the profile data.
[ ... ]
> +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
> +{
[ ... ]
> +set_buttons:
> + scoped_guard(mutex, &drvdata->rom_mutex) {
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + data, len, 25);
This problem wasn't introduced by this patch, but is there a race window in
claw_hw_output_report() that could cause spurious command timeouts?
Looking at claw_hw_output_report(), drvdata->waiting_for_ack is set to
true inside a cmd_lock critical section. Immediately after releasing the
spinlock, it calls reinit_completion(&drvdata->send_cmd_complete).
If the hardware responds exceptionally quickly, or if a delayed ACK from a
previous timed-out command arrives in the small window between the spinlock
release and the reinit_completion() call:
Could the interrupt handler clear waiting_for_ack and call complete(),
only for the main thread to immediately erase the completion event with
reinit_completion()?
This would cause wait_for_completion_interruptible_timeout() to block and
incorrectly return -ETIMEDOUT to userspace.
Should reinit_completion() be called before setting waiting_for_ack?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518222935.1802071-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox