Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mixing `goto`-based error handling with scope-based cleanup (`__free`/`guard`).
- [High] TOCTOU race condition causing lost device configuration updates.
--
commit 5c893bbc0296560ff4fc46c1a17db05f45daeaf6
Author: Derek J. Clark [off-list ref]
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It introduces sysfs attributes to change the gamepad mode,
M-key behavior, and perform a device reset.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
new file mode 100644
--- /dev/null
+++ b/drivers/hid/hid-msi.c
[ ... ]
quoted hunk ↗ jump to hunk
+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 };
[ ... ]
+ guard(mutex)(&drvdata->cfg_mutex);
+ if (timeout) {
[ ... ]
quoted hunk ↗ jump to hunk
+ ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
+ if (ret < 0)
+ goto err_clear_state;
[Severity: High]
Is it intended to mix goto-based error handling with scope-based cleanup
in this function?
The cleanup subsystem guidelines mention that functions should either
convert all resources that need cleanup to scope-based cleanup, or convert
none of them. Using guard and __free alongside a goto label can create
confusing ownership and lock lifetime semantics.
Could this be converted entirely to scope-based cleanup?
quoted hunk ↗ jump to hunk
+
+ ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
+ if (ret)
+ goto err_clear_state;
[ ... ]
quoted hunk ↗ jump to hunk
+err_clear_state:
+ if (timeout) {
+ guard(spinlock_irqsave)(&drvdata->cmd_lock);
+ drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
+ drvdata->waiting_for_ack = false;
+ }
+
+ return ret;
+}
[ ... ]
quoted hunk ↗ jump to hunk
+static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ data[0] = ret;
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
+ data[1] = drvdata->mkeys_function;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
[Severity: High]
Does this sequence introduce a time-of-check to time-of-use race condition?
The current state of mkeys_function is read while holding mode_lock, but the
lock is released before claw_hw_output_report() acquires cfg_mutex. If two
threads concurrently write to gamepad_mode and mkeys_function, both could
read the other's state before either acquires cfg_mutex.
This could result in the second thread sending a command that uses stale
data, silently reverting the first thread's update when the device
acknowledges the command.
Should the read of the current state and the device update be atomic under
a single lock?
quoted hunk ↗ jump to hunk
+ if (ret)
+ return ret;
+
+ return count;
+}
[ ... ]
quoted hunk ↗ jump to hunk
+static ssize_t mkeys_function_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
+ data[0] = drvdata->gamepad_mode;
+ data[1] = ret;
+
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
[Severity: High]
A similar sequence happens here. Can concurrent writes to these attributes
lead to lost device configuration updates due to the locks being acquired and
released separately?
quoted hunk ↗ jump to hunk
+ if (ret)
+ return ret;
+
+ return count;
+}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=1