* [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver
2026-05-13 23:14 [PATCH v2 0/4] Add MSI Claw HID Configuration Driver Derek J. Clark
@ 2026-05-13 23:14 ` Derek J. Clark
2026-05-14 11:51 ` sashiko-bot
2026-05-13 23:14 ` [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes Derek J. Clark
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Derek J. Clark @ 2026-05-13 23:14 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
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>
---
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.
- Remove deadlock in cfg_setup, return on errors.
---
MAINTAINERS | 6 +
drivers/hid/Kconfig | 12 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 6 +
drivers/hid/hid-msi.c | 582 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 607 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..af146691bd481 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -492,6 +492,18 @@ 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 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..6d0d34806931f 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1047,7 +1047,13 @@
#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..8915942af15e6
--- /dev/null
+++ b/drivers/hid/hid-msi.c
@@ -0,0 +1,582 @@
+// 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/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_COMBO,
+ CLAW_MKEY_FUNCTION_DISABLED,
+};
+
+static const char * const claw_mkeys_function_text[] = {
+ [CLAW_MKEY_FUNCTION_MACRO] = "macro",
+ [CLAW_MKEY_FUNCTION_COMBO] = "combination",
+ [CLAW_MKEY_FUNCTION_DISABLED] = "disabled",
+};
+
+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 */
+ u8 ep;
+
+ /* Gamepad Variables */
+ enum claw_mkeys_function_index mkeys_function;
+ enum claw_gamepad_mode_index gamepad_mode;
+};
+
+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;
+
+ 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);
+ break;
+ case CLAW_COMMAND_TYPE_ACK:
+ break;
+ default:
+ dev_dbg(&drvdata->hdev->dev, "Unknown command: %x\n", cmd_rep->cmd);
+ return 0;
+ }
+
+ complete(&drvdata->send_cmd_complete);
+
+ 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);
+
+ /* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
+ if (timeout) {
+ guard(mutex)(&drvdata->cfg_mutex);
+ 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);
+ if (ret >= 0) /* preserve errors */
+ ret = ret == 0 ? -EBUSY : 0; /* timeout occurred : time remained */
+ }
+
+ 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);
+ u8 data[2] = { 0x00, drvdata->mkeys_function };
+ int i, ret = -EINVAL;
+
+ 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;
+
+ 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;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE, NULL, 0, 8);
+ if (ret)
+ return ret;
+
+ 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]);
+ }
+
+ 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);
+ u8 data[2] = { drvdata->gamepad_mode, 0x00 };
+ int i, ret = -EINVAL;
+
+ 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;
+
+ 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;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE, NULL, 0, 8);
+ if (ret)
+ return ret;
+
+ 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]);
+
+ 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);
+ bool val;
+ int ret;
+
+ 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 < 0)
+ 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, 8);
+ 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;
+ }
+
+ 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);
+ u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
+ int ret;
+
+ ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
+ ARRAY_SIZE(data), 0);
+ if (ret)
+ dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
+}
+
+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;
+
+ hid_set_drvdata(hdev, drvdata);
+ drvdata->hdev = hdev;
+ drvdata->ep = ep;
+
+ mutex_init(&drvdata->cfg_mutex);
+ init_completion(&drvdata->send_cmd_complete);
+
+ /* For control interface: open the HID transport for sending commands. */
+ ret = hid_hw_open(hdev);
+ if (ret)
+ return ret;
+
+ INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
+ INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
+ 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_stop(hdev);
+ return;
+ }
+
+ cancel_delayed_work_sync(&drvdata->cfg_setup);
+ cancel_delayed_work_sync(&drvdata->cfg_resume);
+ 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);
+
+ /* 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 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,
+};
+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 [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver
2026-05-13 23:14 ` [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver Derek J. Clark
@ 2026-05-14 11:51 ` sashiko-bot
2026-05-14 13:43 ` Derek John Clark
0 siblings, 1 reply; 12+ messages in thread
From: sashiko-bot @ 2026-05-14 11:51 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `guard(mutex)` macro in `claw_hw_output_report` is scoped to an `if` block, prematurely releasing the lock before the critical section.
- [High] The driver publishes `drvdata` via `hid_set_drvdata()` before fully initializing its mutex and completion objects, creating a race condition.
- [Medium] An invalid, uninitialized gamepad mode (0) may be sent to the hardware during system resume.
- [Medium] The 8ms timeout used in `claw_hw_output_report()` is vulnerable to jiffies rounding, leading to spurious read failures.
- [Medium] The commit message claims probe checks MCU RAM address variances, but the implementation is missing.
--
commit 13b388de96d7484eee908adeed127e8086fc289e
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It provides support for switching the gamepad mode and changing
the behavior of the M-keys.
> Additionally, the MCU RAM address for writing configuration data has
> changed twice. Checks are done during probe to enumerate these variances.
The commit message states that checks are done during probe to enumerate MCU
RAM address variances.
Looking at claw_probe() and msi_probe(), there does not appear to be any code
that checks firmware versions or handles these variances.
Was this functionality dropped during refactoring, or should the commit message
be updated?
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..8915942af15e6
> --- /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);
> +
> + /* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
> + if (timeout) {
> + guard(mutex)(&drvdata->cfg_mutex);
> + reinit_completion(&drvdata->send_cmd_complete);
> + }
Does this drop the lock too early?
The guard() macro releases the lock at the end of its enclosing scope.
Because it is declared inside the if (timeout) block, the mutex will be
released immediately at the closing brace, leaving hid_hw_output_report() and
the subsequent wait unprotected against concurrent access.
> +
> + 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));
Could this timeout evaluate to 1 jiffy on systems configured with HZ=100?
Several callers, such as gamepad_mode_show(), pass an 8ms timeout to this
function. A 1-jiffy timeout can expire almost instantaneously if the operation
begins near the end of the current tick boundary, potentially leading to
spurious -EBUSY timeouts.
[ ... ]
> +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);
> + u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
If the system suspends before cfg_setup_fn() executes, or if the device read in
cfg_setup_fn() fails, could drvdata->gamepad_mode still be 0 here?
Since 0 is not a valid mode (valid modes like CLAW_GAMEPAD_MODE_XINPUT start
at 1), will this send an uninitialized configuration state to the hardware?
> + int ret;
> +
> + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> + ARRAY_SIZE(data), 0);
> + if (ret)
> + dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> +}
> +
> +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;
> +
> + hid_set_drvdata(hdev, drvdata);
> + drvdata->hdev = hdev;
> + drvdata->ep = ep;
> +
> + mutex_init(&drvdata->cfg_mutex);
> + init_completion(&drvdata->send_cmd_complete);
Is it safe to publish drvdata via hid_set_drvdata() before initializing the
mutex and completion object?
Since hid_hw_start() was already called earlier in msi_probe(), the hardware
is active. If an input report arrives right after hid_set_drvdata(), the
interrupt handler msi_raw_event() will process it and call complete() on an
uninitialized waitqueue.
> +
> + /* For control interface: open the HID transport for sending commands. */
> + ret = hid_hw_open(hdev);
> + if (ret)
> + return ret;
> +
> + INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> + INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> + schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver
2026-05-14 11:51 ` sashiko-bot
@ 2026-05-14 13:43 ` Derek John Clark
0 siblings, 0 replies; 12+ messages in thread
From: Derek John Clark @ 2026-05-14 13:43 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
On Thu, May 14, 2026 at 4:51 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] The `guard(mutex)` macro in `claw_hw_output_report` is scoped to an `if` block, prematurely releasing the lock before the critical section.
> - [High] The driver publishes `drvdata` via `hid_set_drvdata()` before fully initializing its mutex and completion objects, creating a race condition.
> - [Medium] An invalid, uninitialized gamepad mode (0) may be sent to the hardware during system resume.
> - [Medium] The 8ms timeout used in `claw_hw_output_report()` is vulnerable to jiffies rounding, leading to spurious read failures.
> - [Medium] The commit message claims probe checks MCU RAM address variances, but the implementation is missing.
> --
>
> commit 13b388de96d7484eee908adeed127e8086fc289e
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add MSI Claw configuration driver
>
> This commit adds a configuration HID driver for the MSI Claw series of
> handheld PCs. It provides support for switching the gamepad mode and changing
> the behavior of the M-keys.
>
> > Additionally, the MCU RAM address for writing configuration data has
> > changed twice. Checks are done during probe to enumerate these variances.
>
> The commit message states that checks are done during probe to enumerate MCU
> RAM address variances.
>
> Looking at claw_probe() and msi_probe(), there does not appear to be any code
> that checks firmware versions or handles these variances.
>
> Was this functionality dropped during refactoring, or should the commit message
> be updated?
>
> > diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> > new file mode 100644
> > index 0000000000000..8915942af15e6
> > --- /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);
> > +
> > + /* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
> > + if (timeout) {
> > + guard(mutex)(&drvdata->cfg_mutex);
> > + reinit_completion(&drvdata->send_cmd_complete);
> > + }
>
> Does this drop the lock too early?
>
Hi robot,
Will fix.
> The guard() macro releases the lock at the end of its enclosing scope.
> Because it is declared inside the if (timeout) block, the mutex will be
> released immediately at the closing brace, leaving hid_hw_output_report() and
> the subsequent wait unprotected against concurrent access.
>
> > +
> > + 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));
>
> Could this timeout evaluate to 1 jiffy on systems configured with HZ=100?
This seems like an unlikely config for a performance gaming system.
> Several callers, such as gamepad_mode_show(), pass an 8ms timeout to this
> function. A 1-jiffy timeout can expire almost instantaneously if the operation
> begins near the end of the current tick boundary, potentially leading to
> spurious -EBUSY timeouts.
>
> [ ... ]
>
> > +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);
> > + u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
>
> If the system suspends before cfg_setup_fn() executes, or if the device read in
> cfg_setup_fn() fails, could drvdata->gamepad_mode still be 0 here?
>
> Since 0 is not a valid mode (valid modes like CLAW_GAMEPAD_MODE_XINPUT start
> at 1), will this send an uninitialized configuration state to the hardware?
I'll add a valid default value.
> > + int ret;
> > +
> > + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> > + ARRAY_SIZE(data), 0);
> > + if (ret)
> > + dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> > +}
> > +
> > +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;
> > +
> > + hid_set_drvdata(hdev, drvdata);
> > + drvdata->hdev = hdev;
> > + drvdata->ep = ep;
> > +
> > + mutex_init(&drvdata->cfg_mutex);
> > + init_completion(&drvdata->send_cmd_complete);
>
> Is it safe to publish drvdata via hid_set_drvdata() before initializing the
> mutex and completion object?
>
> Since hid_hw_start() was already called earlier in msi_probe(), the hardware
> is active. If an input report arrives right after hid_set_drvdata(), the
> interrupt handler msi_raw_event() will process it and call complete() on an
> uninitialized waitqueue.
>
I'll reorder this.
> > +
> > + /* For control interface: open the HID transport for sending commands. */
> > + ret = hid_hw_open(hdev);
> > + if (ret)
> > + return ret;
> > +
> > + INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> > + INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> > + schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
> > +
> > + return 0;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes
2026-05-13 23:14 [PATCH v2 0/4] Add MSI Claw HID Configuration Driver Derek J. Clark
2026-05-13 23:14 ` [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver Derek J. Clark
@ 2026-05-13 23:14 ` Derek J. Clark
2026-05-14 12:22 ` sashiko-bot
2026-05-13 23:14 ` [PATCH v2 3/4] HID: hid-msi: Add RGB control interface Derek J. Clark
2026-05-13 23:14 ` [PATCH v2 4/4] HID: hid-msi: Add Rumble Intensity Attributes Derek J. Clark
3 siblings, 1 reply; 12+ messages in thread
From: Derek J. Clark @ 2026-05-13 23:14 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
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>
---
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 | 398 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 397 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 8915942af15e6..13ba2747fdb66 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -40,6 +40,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,
@@ -64,6 +66,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_COMBO,
@@ -76,6 +89,154 @@ static const char * const claw_mkeys_function_text[] = {
[CLAW_MKEY_FUNCTION_DISABLED] = "disabled",
};
+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_LEFT_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" },
+};
+
+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];
@@ -86,16 +247,24 @@ 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 */
+ 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];
+ const u16 *bmap_addr;
+ bool bmap_support;
};
static int get_endpoint_address(struct hid_device *hdev)
@@ -125,6 +294,31 @@ 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;
+ /* Extract key codes; replace disabled (0xff) with 0x00, which is (null) in _show */
+ for (i = 0; i < CLAW_KEYS_MAX; i++)
+ codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
+ break;
+ default:
+ dev_warn(&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)
{
@@ -146,6 +340,9 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
ret = claw_gamepad_mode_event(drvdata, cmd_rep);
break;
+ case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
+ ret = claw_profile_event(drvdata, cmd_rep);
+ break;
case CLAW_COMMAND_TYPE_ACK:
break;
default:
@@ -366,6 +563,161 @@ 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;
+
+ 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 };
+ size_t len = ARRAY_SIZE(data);
+ int ret, key_count, i;
+ char **raw_keys;
+
+ raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
+ if (!raw_keys)
+ return -ENOMEM;
+
+ guard(mutex)(&drvdata->rom_mutex); /* all err_free paths must be in scope */
+ if (key_count > CLAW_KEYS_MAX) {
+ ret = -EINVAL;
+ goto err_free;
+ }
+
+ 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)
+ goto err_free;
+
+ data[6 + i] = ret;
+ }
+
+set_buttons:
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);
+ if (ret < 0)
+ goto err_free;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
+
+err_free:
+ argv_free(raw_keys);
+ 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;
+
+ 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, 8);
+ 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)
{
@@ -378,10 +730,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,
@@ -432,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
}
+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;
@@ -446,8 +833,17 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
drvdata->ep = ep;
mutex_init(&drvdata->cfg_mutex);
+ mutex_init(&drvdata->profile_mutex);
+ mutex_init(&drvdata->rom_mutex);
init_completion(&drvdata->send_cmd_complete);
+ /* 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");
+
/* For control interface: open the HID transport for sending commands. */
ret = hid_hw_open(hdev);
if (ret)
--
2.53.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes
2026-05-13 23:14 ` [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes Derek J. Clark
@ 2026-05-14 12:22 ` sashiko-bot
2026-05-14 13:48 ` Derek John Clark
0 siblings, 1 reply; 12+ messages in thread
From: sashiko-bot @ 2026-05-14 12:22 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `cfg_mutex` guard in `claw_hw_output_report()` is scoped to an `if` block, releasing the lock before the critical section executes.
- [High] Lockless concurrent access to `profile_pending` between process and interrupt contexts causes data corruption and sequence mismatches on delayed responses.
- [High] Unsafe assumption of USB transport in `claw_probe()` causes type confusion and invalid pointer dereference for non-USB devices.
- [High] Returning an error from `claw_profile_event()` for a spurious event unconditionally triggers `complete()`, causing premature wakeups of unrelated commands.
- [Medium] The function `claw_buttons_store()` illegally mixes scope-based `guard()` cleanup with `goto`-based error handling.
--
commit 42be71c046dc9b6b31d28598f5e0bb568d78b7a1
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add M-key mapping attributes
Adds attributes that allow for remapping the M-keys with up to 5 values
when in macro mode.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 8915942af15e6..13ba2747fdb66 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -125,6 +294,31 @@ 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;
> + /* Extract key codes; replace disabled (0xff) with 0x00, which is (null) in _show */
> + for (i = 0; i < CLAW_KEYS_MAX; i++)
> + codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> + break;
Is it safe to read and modify profile_pending locklessly here in the
interrupt handler?
Since claw_buttons_show() modifies this state in process context under
profile_mutex, if a sysfs request times out and a subsequent request starts,
could the delayed ACK from the first request cause this handler to see the
new profile_pending state and corrupt the new request's buffer?
> + default:
> + dev_warn(&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)
> {
[ ... ]
> @@ -146,6 +340,9 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> break;
> + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> + ret = claw_profile_event(drvdata, cmd_rep);
> + break;
> case CLAW_COMMAND_TYPE_ACK:
> break;
> default:
When claw_profile_event() returns -EINVAL for a spurious event,
claw_raw_event() breaks from the switch and unconditionally calls
complete(&drvdata->send_cmd_complete).
Could this prematurely wake up a thread waiting on a completely different
command, such as WRITE_PROFILE_DATA?
[ ... ]
> @@ -366,6 +563,161 @@ 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)
> +{
[ ... ]
> +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 };
> + size_t len = ARRAY_SIZE(data);
> + int ret, key_count, i;
> + char **raw_keys;
> +
> + raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> + if (!raw_keys)
> + return -ENOMEM;
> +
> + guard(mutex)(&drvdata->rom_mutex); /* all err_free paths must be in scope */
> + if (key_count > CLAW_KEYS_MAX) {
> + ret = -EINVAL;
> + goto err_free;
> + }
> +
> + 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)
> + goto err_free;
> +
> + data[6 + i] = ret;
> + }
> +
> +set_buttons:
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);
While reviewing the addition of these sysfs attributes, I noticed a
pre-existing issue in claw_hw_output_report() that is exacerbated by
the concurrent sysfs requests added here:
drivers/hid/hid-msi.c:claw_hw_output_report() {
...
if (timeout) {
guard(mutex)(&drvdata->cfg_mutex);
reinit_completion(&drvdata->send_cmd_complete);
}
...
}
Since guard() is scoped to the if block, the mutex is immediately released at
the closing brace.
Doesn't this mean hid_hw_output_report() and
wait_for_completion_interruptible_timeout() execute entirely without
the lock, defeating the intended serialization and allowing concurrent
requests to race?
> + if (ret < 0)
> + goto err_free;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
> +
> +err_free:
> + argv_free(raw_keys);
> + return ret;
> +}
Is it safe to mix goto-based error handling with scope-based guard() cleanup
in the same function?
According to the cleanup guidelines, functions should either convert
entirely to scope-based cleanup, for example by using a DEFINE_FREE
wrapper for argv_free, or not use guard() at all to avoid confusing
ownership semantics.
[ ... ]
> @@ -432,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
> dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> }
>
> +static void claw_features_supported(struct claw_drvdata *drvdata)
> +{
[ ... ]
> 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);
Is it guaranteed that this device is always connected via USB?
If this driver is bound to a device connected over a non-USB transport like
I2C, Bluetooth, or UHID, hdev->dev.parent would not be a valid USB
interface, which could cause an invalid pointer dereference here.
Could this code check hid_is_usb(hdev) first or read the version from
hdev->version instead?
> struct claw_drvdata *drvdata;
> int ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes
2026-05-14 12:22 ` sashiko-bot
@ 2026-05-14 13:48 ` Derek John Clark
0 siblings, 0 replies; 12+ messages in thread
From: Derek John Clark @ 2026-05-14 13:48 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
On Thu, May 14, 2026 at 5:22 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] The `cfg_mutex` guard in `claw_hw_output_report()` is scoped to an `if` block, releasing the lock before the critical section executes.
> - [High] Lockless concurrent access to `profile_pending` between process and interrupt contexts causes data corruption and sequence mismatches on delayed responses.
> - [High] Unsafe assumption of USB transport in `claw_probe()` causes type confusion and invalid pointer dereference for non-USB devices.
> - [High] Returning an error from `claw_profile_event()` for a spurious event unconditionally triggers `complete()`, causing premature wakeups of unrelated commands.
> - [Medium] The function `claw_buttons_store()` illegally mixes scope-based `guard()` cleanup with `goto`-based error handling.
> --
>
> commit 42be71c046dc9b6b31d28598f5e0bb568d78b7a1
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add M-key mapping attributes
>
> Adds attributes that allow for remapping the M-keys with up to 5 values
> when in macro mode.
>
> > diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> > index 8915942af15e6..13ba2747fdb66 100644
> > --- a/drivers/hid/hid-msi.c
> > +++ b/drivers/hid/hid-msi.c
>
> [ ... ]
>
> > @@ -125,6 +294,31 @@ 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;
> > + /* Extract key codes; replace disabled (0xff) with 0x00, which is (null) in _show */
> > + for (i = 0; i < CLAW_KEYS_MAX; i++)
> > + codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> > + break;
>
> Is it safe to read and modify profile_pending locklessly here in the
> interrupt handler?
>
> Since claw_buttons_show() modifies this state in process context under
> profile_mutex, if a sysfs request times out and a subsequent request starts,
> could the delayed ACK from the first request cause this handler to see the
> new profile_pending state and corrupt the new request's buffer?
The lock covers this case as it is only released after the completion
fires or time out, after this.
> > + default:
> > + dev_warn(&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)
> > {
>
> [ ... ]
>
> > @@ -146,6 +340,9 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> > case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> > ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> > break;
> > + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> > + ret = claw_profile_event(drvdata, cmd_rep);
> > + break;
> > case CLAW_COMMAND_TYPE_ACK:
> > break;
> > default:
>
> When claw_profile_event() returns -EINVAL for a spurious event,
> claw_raw_event() breaks from the switch and unconditionally calls
> complete(&drvdata->send_cmd_complete).
That case is unlikely as the profile events are serialized by a mutex.
Holding the completion until timeout would only result in delaying
subsequent calls.
> Could this prematurely wake up a thread waiting on a completely different
> command, such as WRITE_PROFILE_DATA?
>
> [ ... ]
>
> > @@ -366,6 +563,161 @@ 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)
> > +{
>
> [ ... ]
>
> > +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 };
> > + size_t len = ARRAY_SIZE(data);
> > + int ret, key_count, i;
> > + char **raw_keys;
> > +
> > + raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> > + if (!raw_keys)
> > + return -ENOMEM;
> > +
> > + guard(mutex)(&drvdata->rom_mutex); /* all err_free paths must be in scope */
> > + if (key_count > CLAW_KEYS_MAX) {
> > + ret = -EINVAL;
> > + goto err_free;
> > + }
> > +
> > + 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)
> > + goto err_free;
> > +
> > + data[6 + i] = ret;
> > + }
> > +
> > +set_buttons:
> > + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);
>
> While reviewing the addition of these sysfs attributes, I noticed a
> pre-existing issue in claw_hw_output_report() that is exacerbated by
> the concurrent sysfs requests added here:
>
> drivers/hid/hid-msi.c:claw_hw_output_report() {
> ...
> if (timeout) {
> guard(mutex)(&drvdata->cfg_mutex);
> reinit_completion(&drvdata->send_cmd_complete);
> }
> ...
> }
>
> Since guard() is scoped to the if block, the mutex is immediately released at
> the closing brace.
>
> Doesn't this mean hid_hw_output_report() and
> wait_for_completion_interruptible_timeout() execute entirely without
> the lock, defeating the intended serialization and allowing concurrent
> requests to race?
>
> > + if (ret < 0)
> > + goto err_free;
> > +
> > + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
> > +
> > +err_free:
> > + argv_free(raw_keys);
> > + return ret;
> > +}
>
> Is it safe to mix goto-based error handling with scope-based guard() cleanup
> in the same function?
I'll make this a scoped guard over claw_hw_output_report.
> According to the cleanup guidelines, functions should either convert
> entirely to scope-based cleanup, for example by using a DEFINE_FREE
> wrapper for argv_free, or not use guard() at all to avoid confusing
> ownership semantics.
>
> [ ... ]
>
> > @@ -432,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
> > dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> > }
> >
> > +static void claw_features_supported(struct claw_drvdata *drvdata)
> > +{
>
> [ ... ]
>
> > 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);
>
> Is it guaranteed that this device is always connected via USB?
This function is only called by msi_probe, which checks for a valid
USB device as a first step.
> If this driver is bound to a device connected over a non-USB transport like
> I2C, Bluetooth, or UHID, hdev->dev.parent would not be a valid USB
> interface, which could cause an invalid pointer dereference here.
>
> Could this code check hid_is_usb(hdev) first or read the version from
> hdev->version instead?
>
> > struct claw_drvdata *drvdata;
> > int ret;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] HID: hid-msi: Add RGB control interface
2026-05-13 23:14 [PATCH v2 0/4] Add MSI Claw HID Configuration Driver Derek J. Clark
2026-05-13 23:14 ` [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver Derek J. Clark
2026-05-13 23:14 ` [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes Derek J. Clark
@ 2026-05-13 23:14 ` Derek J. Clark
2026-05-14 12:53 ` sashiko-bot
2026-05-13 23:14 ` [PATCH v2 4/4] HID: hid-msi: Add Rumble Intensity Attributes Derek J. Clark
3 siblings, 1 reply; 12+ messages in thread
From: Derek J. Clark @ 2026-05-13 23:14 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
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>
---
v2:
- Use pending_profile mutex
- Remove deadlock in cfg_setup, return on errors.
---
drivers/hid/hid-msi.c | 548 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 542 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 13ba2747fdb66..a628b77bfb7b5 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>
@@ -42,6 +43,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,
@@ -70,6 +75,7 @@ enum claw_profile_ack_pending {
CLAW_NO_PENDING,
CLAW_M1_PENDING,
CLAW_M2_PENDING,
+ CLAW_RGB_PENDING,
};
enum claw_key_index {
@@ -227,6 +233,22 @@ static const struct {
{ 0xce, "REL_WHEEL_DOWN" },
};
+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 */
@@ -237,6 +259,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];
@@ -245,6 +270,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;
@@ -265,6 +312,16 @@ struct claw_drvdata {
u8 m2_codes[CLAW_KEYS_MAX];
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;
+ u8 rgb_frame_count;
+ bool rgb_enabled;
+ u8 rgb_speed;
+ u16 rgb_addr;
};
static int get_endpoint_address(struct hid_device *hdev)
@@ -296,8 +353,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:
@@ -308,15 +368,52 @@ 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] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
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 > U8_MAX) {
+ dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
+ frame_calc);
+ ret = -EINVAL;
+ goto err_pending;
+ }
+ f_idx = frame_calc;
+
+ if (f_idx >= CLAW_RGB_MAX_FRAMES) {
+ dev_err(drvdata->led_mc.led_cdev.dev, "Got illegal frame index: %x\n",
+ f_idx);
+ ret = -EINVAL;
+ goto err_pending;
+ }
+
+ /* Always treat the first frame as the truth for these constants */
+ if (f_idx == 0) {
+ drvdata->rgb_frame_count = frame->frame_count;
+ /* Invert device speed (20-0) to sysfs speed (0-20) */
+ 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;
+ }
+
+ memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
+ sizeof(struct rgb_frame));
+
+ break;
default:
dev_warn(&drvdata->hdev->dev,
"Got profile event without changes pending from command: %x\n",
cmd_rep->cmd);
- return -EINVAL;
+ ret = -EINVAL;
}
+
+err_pending:
drvdata->profile_pending = CLAW_NO_PENDING;
- return 0;
+ return ret;
}
static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
@@ -759,6 +856,397 @@ 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, 8);
+ 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++) {
+ 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, 8);
+ 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, 8);
+
+ 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, 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 };
+
+ 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 };
+
+ 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 frame;
+
+ drvdata->rgb_frame_count = frame_count;
+
+ for (frame = 0; frame < frame_count; frame++)
+ claw_frame_fill_solid(&drvdata->rgb_frames[frame], colors[frame]);
+
+ 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 frame, zone;
+
+ drvdata->rgb_frame_count = frame_count;
+
+ for (frame = 0; frame < frame_count; frame++) {
+ for (zone = 0; zone < 4; zone++) {
+ drvdata->rgb_frames[frame].zone[zone] = colors[(zone + frame) % 4];
+ drvdata->rgb_frames[frame].zone[zone + 4] = colors[(zone + frame) % 4];
+ }
+ drvdata->rgb_frames[frame].zone[8] = colors[frame];
+ }
+
+ 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 frame, zone;
+
+ drvdata->rgb_frame_count = frame_count;
+
+ for (frame = 0; frame < frame_count; frame++) {
+ for (zone = 0; zone < 4; zone++) {
+ drvdata->rgb_frames[frame].zone[zone] = colors[(zone + frame) % 4];
+ drvdata->rgb_frames[frame].zone[zone + 4] = colors[(zone - frame + 6) % 4];
+ }
+ drvdata->rgb_frames[frame].zone[8] = colors[frame];
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply current state to device */
+static int claw_apply_rgb_state(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone off = { 0, 0, 0 };
+
+ if (!drvdata->rgb_enabled) {
+ drvdata->rgb_frame_count = 1;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], off);
+ return claw_write_rgb_state(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;
+
+ 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;
+
+ 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_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;
+
+ 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);
+
+ 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;
+
+ 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 hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 speed = 20 - drvdata->rgb_speed;
+
+ 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);
+
+ 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 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);
@@ -772,6 +1260,13 @@ static void cfg_setup_fn(struct work_struct *work)
return;
}
+ ret = claw_read_rgb_config(drvdata->hdev);
+ if (ret) {
+ dev_err(drvdata->led_mc.led_cdev.dev,
+ "Failed to setup device, can't read RGB config: %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) {
@@ -780,7 +1275,15 @@ static void cfg_setup_fn(struct work_struct *work)
return;
}
+ ret = devm_device_add_group(drvdata->led_mc.led_cdev.dev, &rgb_attr_group);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to setup device, can't create led attributes: %d\n", ret);
+ return;
+ }
+
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)
@@ -790,6 +1293,10 @@ static void cfg_resume_fn(struct work_struct *work)
u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
int ret;
+ ret = claw_read_rgb_config(drvdata->hdev);
+ if (ret)
+ dev_err(drvdata->led_mc.led_cdev.dev, "Failed to read RGB config: %d\n", ret);
+
ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
ARRAY_SIZE(data), 0);
if (ret)
@@ -803,18 +1310,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)
@@ -844,6 +1357,26 @@ 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");
+ /* Initialize RGB LED */
+ INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);
+
+ 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;
+
+ drvdata->rgb_enabled = true;
+
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, &drvdata->led_mc);
+ if (ret)
+ return ret;
+
/* For control interface: open the HID transport for sending commands. */
ret = hid_hw_open(hdev);
if (ret)
@@ -905,6 +1438,9 @@ 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->rgb_queue);
cancel_delayed_work_sync(&drvdata->cfg_setup);
cancel_delayed_work_sync(&drvdata->cfg_resume);
hid_hw_close(hdev);
--
2.53.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/4] HID: hid-msi: Add RGB control interface
2026-05-13 23:14 ` [PATCH v2 3/4] HID: hid-msi: Add RGB control interface Derek J. Clark
@ 2026-05-14 12:53 ` sashiko-bot
2026-05-14 13:54 ` Derek John Clark
0 siblings, 1 reply; 12+ messages in thread
From: sashiko-bot @ 2026-05-14 12:53 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] The `speed_show` function incorrectly casts the LED class device to a HID device, resulting in an invalid pointer dereference and a guaranteed kernel panic.
- [Critical] Missing bounds check on `frame->frame_count` from the USB device leads to an out-of-bounds read of kernel memory.
- [High] Re-arming `rgb_queue` from sysfs attributes after it has been explicitly cancelled in `claw_remove` leads to a Use-After-Free.
- [High] The `guard(mutex)` macro in `claw_hw_output_report()` is incorrectly used inside a block scope, providing absolutely no synchronization.
- [High] Concurrent modification of shared RGB state without lock protection causes data races and memory corruption.
- [High] Every RGB state change triggers a `SYNC_TO_ROM` command, causing severe flash memory degradation.
- [Medium] An early return in `cfg_setup_fn` silently drops core gamepad sysfs attributes if RGB initialization fails.
--
commit a90ff4a0f03b6df7827bfbcd222f977138b9d280
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add RGB control interface
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.
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -308,15 +368,52 @@ 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] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> 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 > U8_MAX) {
> + dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> + frame_calc);
> + ret = -EINVAL;
> + goto err_pending;
> + }
> + f_idx = frame_calc;
> +
> + if (f_idx >= CLAW_RGB_MAX_FRAMES) {
> + dev_err(drvdata->led_mc.led_cdev.dev, "Got illegal frame index: %x\n",
> + f_idx);
> + ret = -EINVAL;
> + goto err_pending;
> + }
> +
> + /* Always treat the first frame as the truth for these constants */
> + if (f_idx == 0) {
> + drvdata->rgb_frame_count = frame->frame_count;
drvdata->rgb_frame_count is assigned directly from the hardware packet.
Should there be a bounds check here to ensure it is less than or equal to
CLAW_RGB_MAX_FRAMES?
Later, claw_write_rgb_state() uses drvdata->rgb_frame_count as the upper
bound for a loop reading from drvdata->rgb_frames. If a device sends a
frame count greater than 8, could this read out-of-bounds kernel memory
and send it back to the USB device?
> + /* Invert device speed (20-0) to sysfs speed (0-20) */
> + 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;
> + }
> +
> + memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> + sizeof(struct rgb_frame));
claw_profile_event() modifies drvdata->rgb_frames and
drvdata->rgb_frame_count from the HID raw event handler context.
Simultaneously, claw_rgb_queue_fn() runs in a workqueue and calls
functions that overwrite these exact same fields.
Since there are no locks protecting these updates, could a background
worker read torn state or corrupt the frame arrays while they are being
actively updated by the asynchronous device response?
[ ... ]
> @@ -759,6 +856,397 @@ 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)
> +{
[ ... ]
> +/* 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++) {
> + 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, 8);
> + 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, 8);
Does sending CLAW_COMMAND_TYPE_SYNC_TO_ROM on every RGB state change
cause excessive flash memory degradation?
Because claw_apply_rgb_state() is triggered by unthrottled sysfs writes
(such as rapidly changing a brightness slider), this might cause hundreds
of NVRAM writes per second and potentially degrade the MCU's flash
endurance.
[ ... ]
> +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;
> +
> + 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;
> +}
Is there a potential use-after-free here if sysfs attributes are written
after the device is removed?
In claw_remove(), the driver attempts to tear down the worker using
cancel_delayed_work_sync(&drvdata->rgb_queue). However, the sysfs files
and LED endpoints remain active because devm cleanup executes after
claw_remove() completes.
A concurrent write to a sysfs attribute like effect_store could call
mod_delayed_work() and re-arm the work item, causing it to execute after
drvdata has been freed.
[ ... ]
> +static ssize_t speed_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);
Is the dev parameter passed to speed_show actually a hid_device?
The rgb_attr_group sysfs group is attached to the LED class device
(led_cdev.dev), not the parent HID device. The macro to_hid_device(dev)
blindly performs a container_of cast, which will yield an invalid pointer
for hdev.
Dereferencing it with hid_get_drvdata(hdev) will likely cause a kernel
panic when this world-readable attribute is accessed.
[ ... ]
> @@ -772,6 +1260,13 @@ static void cfg_setup_fn(struct work_struct *work)
> return;
> }
>
> + ret = claw_read_rgb_config(drvdata->hdev);
> + if (ret) {
> + dev_err(drvdata->led_mc.led_cdev.dev,
> + "Failed to setup device, can't read RGB config: %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 claw_read_rgb_config() fails due to a temporary USB error or
unsupported firmware, this early return skips the subsequent
devm_device_add_group call.
Does this unintentionally break the device's core gamepad configuration
interface simply because the RGB initialization failed?
This isn't a new bug introduced by this patch, but does the guard() in
claw_hw_output_report() provide any synchronization?
claw_hw_output_report() {
if (timeout) {
guard(mutex)(&drvdata->cfg_mutex);
reinit_completion(&drvdata->send_cmd_complete);
}
...
}
Because the guard is declared inside the block, the mutex is immediately
released at the closing brace before hid_hw_output_report() and
wait_for_completion_interruptible_timeout() are ever called. This seems
to leave the I/O operations unprotected against concurrent threads, which
this patch introduces via the new RGB workers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/4] HID: hid-msi: Add RGB control interface
2026-05-14 12:53 ` sashiko-bot
@ 2026-05-14 13:54 ` Derek John Clark
0 siblings, 0 replies; 12+ messages in thread
From: Derek John Clark @ 2026-05-14 13:54 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
On Thu, May 14, 2026 at 5:53 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [Critical] The `speed_show` function incorrectly casts the LED class device to a HID device, resulting in an invalid pointer dereference and a guaranteed kernel panic.
> - [Critical] Missing bounds check on `frame->frame_count` from the USB device leads to an out-of-bounds read of kernel memory.
> - [High] Re-arming `rgb_queue` from sysfs attributes after it has been explicitly cancelled in `claw_remove` leads to a Use-After-Free.
> - [High] The `guard(mutex)` macro in `claw_hw_output_report()` is incorrectly used inside a block scope, providing absolutely no synchronization.
> - [High] Concurrent modification of shared RGB state without lock protection causes data races and memory corruption.
> - [High] Every RGB state change triggers a `SYNC_TO_ROM` command, causing severe flash memory degradation.
> - [Medium] An early return in `cfg_setup_fn` silently drops core gamepad sysfs attributes if RGB initialization fails.
> --
>
> commit a90ff4a0f03b6df7827bfbcd222f977138b9d280
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add RGB control interface
>
> 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.
>
> > --- a/drivers/hid/hid-msi.c
> > +++ b/drivers/hid/hid-msi.c
>
> [ ... ]
>
> > @@ -308,15 +368,52 @@ 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] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> > 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 > U8_MAX) {
> > + dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> > + frame_calc);
> > + ret = -EINVAL;
> > + goto err_pending;
> > + }
> > + f_idx = frame_calc;
> > +
> > + if (f_idx >= CLAW_RGB_MAX_FRAMES) {
> > + dev_err(drvdata->led_mc.led_cdev.dev, "Got illegal frame index: %x\n",
> > + f_idx);
> > + ret = -EINVAL;
> > + goto err_pending;
> > + }
> > +
> > + /* Always treat the first frame as the truth for these constants */
> > + if (f_idx == 0) {
> > + drvdata->rgb_frame_count = frame->frame_count;
>
> drvdata->rgb_frame_count is assigned directly from the hardware packet.
> Should there be a bounds check here to ensure it is less than or equal to
> CLAW_RGB_MAX_FRAMES?
>
> Later, claw_write_rgb_state() uses drvdata->rgb_frame_count as the upper
> bound for a loop reading from drvdata->rgb_frames. If a device sends a
> frame count greater than 8, could this read out-of-bounds kernel memory
> and send it back to the USB device?
I'll add a bounds check, though again it is unlikely with the previous checks.
> > + /* Invert device speed (20-0) to sysfs speed (0-20) */
> > + 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;
> > + }
> > +
> > + memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> > + sizeof(struct rgb_frame));
>
> claw_profile_event() modifies drvdata->rgb_frames and
> drvdata->rgb_frame_count from the HID raw event handler context.
> Simultaneously, claw_rgb_queue_fn() runs in a workqueue and calls
> functions that overwrite these exact same fields.
>
> Since there are no locks protecting these updates, could a background
> worker read torn state or corrupt the frame arrays while they are being
> actively updated by the asynchronous device response?
I'll add another mutex I suppose.
> [ ... ]
>
> > @@ -759,6 +856,397 @@ 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)
> > +{
>
> [ ... ]
>
> > +/* 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++) {
> > + 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, 8);
> > + 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, 8);
>
> Does sending CLAW_COMMAND_TYPE_SYNC_TO_ROM on every RGB state change
> cause excessive flash memory degradation?
>
> Because claw_apply_rgb_state() is triggered by unthrottled sysfs writes
> (such as rapidly changing a brightness slider), this might cause hundreds
> of NVRAM writes per second and potentially degrade the MCU's flash
> endurance.
This is throttled by mod_delayed_work, where each subsequent call
under 50ms resets the timer.
> [ ... ]
>
> > +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;
> > +
> > + 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;
> > +}
>
> Is there a potential use-after-free here if sysfs attributes are written
> after the device is removed?
>
> In claw_remove(), the driver attempts to tear down the worker using
> cancel_delayed_work_sync(&drvdata->rgb_queue). However, the sysfs files
> and LED endpoints remain active because devm cleanup executes after
> claw_remove() completes.
>
> A concurrent write to a sysfs attribute like effect_store could call
> mod_delayed_work() and re-arm the work item, causing it to execute after
> drvdata has been freed.
> [ ... ]
>
> > +static ssize_t speed_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);
>
> Is the dev parameter passed to speed_show actually a hid_device?
>
I forgot to fix this.
> The rgb_attr_group sysfs group is attached to the LED class device
> (led_cdev.dev), not the parent HID device. The macro to_hid_device(dev)
> blindly performs a container_of cast, which will yield an invalid pointer
> for hdev.
>
> Dereferencing it with hid_get_drvdata(hdev) will likely cause a kernel
> panic when this world-readable attribute is accessed.
>
> [ ... ]
>
> > @@ -772,6 +1260,13 @@ static void cfg_setup_fn(struct work_struct *work)
> > return;
> > }
> >
> > + ret = claw_read_rgb_config(drvdata->hdev);
> > + if (ret) {
> > + dev_err(drvdata->led_mc.led_cdev.dev,
> > + "Failed to setup device, can't read RGB config: %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 claw_read_rgb_config() fails due to a temporary USB error or
> unsupported firmware, this early return skips the subsequent
> devm_device_add_group call.
>
> Does this unintentionally break the device's core gamepad configuration
> interface simply because the RGB initialization failed?
The expectation is that if any of these error there is a problem with
the kernel or the device hardware. This is intentional.
> This isn't a new bug introduced by this patch, but does the guard() in
> claw_hw_output_report() provide any synchronization?
>
> claw_hw_output_report() {
> if (timeout) {
> guard(mutex)(&drvdata->cfg_mutex);
> reinit_completion(&drvdata->send_cmd_complete);
> }
> ...
> }
>
> Because the guard is declared inside the block, the mutex is immediately
> released at the closing brace before hid_hw_output_report() and
> wait_for_completion_interruptible_timeout() are ever called. This seems
> to leave the I/O operations unprotected against concurrent threads, which
> this patch introduces via the new RGB workers.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] HID: hid-msi: Add Rumble Intensity Attributes
2026-05-13 23:14 [PATCH v2 0/4] Add MSI Claw HID Configuration Driver Derek J. Clark
` (2 preceding siblings ...)
2026-05-13 23:14 ` [PATCH v2 3/4] HID: hid-msi: Add RGB control interface Derek J. Clark
@ 2026-05-13 23:14 ` Derek J. Clark
2026-05-14 16:29 ` sashiko-bot
3 siblings, 1 reply; 12+ messages in thread
From: Derek J. Clark @ 2026-05-13 23:14 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
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>
---
v2:
- Use pending_profile and sync to rom mutexes.
---
drivers/hid/hid-msi.c | 147 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 147 insertions(+)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index a628b77bfb7b5..cffd6ed253ec9 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -76,6 +76,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 {
@@ -262,6 +264,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];
@@ -310,7 +317,10 @@ 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;
const u16 *bmap_addr;
+ bool rumble_support;
bool bmap_support;
/* RGB Variables */
@@ -402,6 +412,12 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
sizeof(struct rgb_frame));
+ break;
+ case CLAW_RUMBLE_LEFT_PENDING:
+ drvdata->rumble_intensity_left = cmd_rep->data[4];
+ break;
+ case CLAW_RUMBLE_RIGHT_PENDING:
+ drvdata->rumble_intensity_right = cmd_rep->data[4];
break;
default:
dev_warn(&drvdata->hdev->dev,
@@ -815,6 +831,126 @@ 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;
+
+ 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), 8);
+ if (ret)
+ return ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
+ if (ret)
+ return ret;
+
+ drvdata->rumble_intensity_left = val;
+
+ 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;
+
+ 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), 8);
+ if (ret) {
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return ret;
+ }
+
+ return sysfs_emit(buf, "%u\n", drvdata->rumble_intensity_left);
+}
+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;
+
+ 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), 8);
+ if (ret)
+ return ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
+ if (ret)
+ return ret;
+
+ drvdata->rumble_intensity_right = val;
+
+ 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;
+
+ 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), 8);
+ if (ret) {
+ drvdata->profile_pending = CLAW_NO_PENDING;
+ return ret;
+ }
+
+ return sysfs_emit(buf, "%u\n", drvdata->rumble_intensity_right);
+}
+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)
{
@@ -835,6 +971,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;
}
@@ -848,6 +990,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,
};
@@ -1312,6 +1457,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;
@@ -1323,6 +1469,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;
}
--
2.53.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] HID: hid-msi: Add Rumble Intensity Attributes
2026-05-13 23:14 ` [PATCH v2 4/4] HID: hid-msi: Add Rumble Intensity Attributes Derek J. Clark
@ 2026-05-14 16:29 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-05-14 16:29 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent sysfs read and write operations interleave commands and corrupt hardware state due to a block-scoped guard() bug on cfg_mutex.
- [High] State machine corruption due to failure to defensively clear profile_pending on unexpected generic ACKs.
--
commit f1baff465a5e284998680961eb3326992b856326
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add Rumble Intensity Attributes
This commit adds intensity adjustment for the left and right rumble motors.
The code was co-developed with AI assistance for data gathering and initial
framing of the sysfs show and store functions.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index a628b77bfb7b5..cffd6ed253ec9 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -815,6 +831,126 @@ 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)
> +{
[ ... ]
> + guard(mutex)(&drvdata->rom_mutex);
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + data, ARRAY_SIZE(data), 8);
[ ... ]
> +static ssize_t rumble_intensity_left_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
[ ... ]
> + 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), 8);
> + if (ret) {
> + drvdata->profile_pending = CLAW_NO_PENDING;
> + return ret;
> + }
> +
> + return sysfs_emit(buf, "%u\n", drvdata->rumble_intensity_left);
> +}
Since rumble_intensity_left_store uses rom_mutex and
rumble_intensity_left_show uses profile_mutex, these functions can be
called concurrently.
Does claw_hw_output_report serialize these hardware requests?
Looking at claw_hw_output_report, it attempts to protect the transaction
with a block-scoped guard:
static int claw_hw_output_report(...)
{
...
if (timeout) {
guard(mutex)(&drvdata->cfg_mutex);
reinit_completion(&drvdata->send_cmd_complete);
}
// drvdata->cfg_mutex is released here
...
ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
...
if (timeout) {
ret = wait_for_completion_interruptible_timeout(
&drvdata->send_cmd_complete, ...);
...
}
Because the guard is scoped only to the if statement, the lock is released
before the USB transmission and wait occur.
Will concurrent sysfs operations issue uncoordinated USB commands and wait
on the exact same send_cmd_complete object, resulting in desynchronized
responses?
If a concurrent store operation triggers a generic ACK that satisfies the
wait in the show thread, ret will be 0, but claw_profile_event will not
have executed.
In this success path, does drvdata->profile_pending remain stuck at
CLAW_RUMBLE_LEFT_PENDING instead of being defensively cleared?
If an unrelated READ_PROFILE_ACK arrives later, could claw_profile_event
incorrectly demultiplex it based on the stuck state, copying arbitrary
payload bytes into drvdata->rumble_intensity_left?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=4
^ permalink raw reply [flat|nested] 12+ messages in thread