Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Timeout of READ_PROFILE commands leaves profile_pending dirty and falsely aborts subsequent requests due to unhandled stale ACKs.
- [High] Abandoning the ACK for CLAW_COMMAND_TYPE_SYNC_TO_ROM leaves a stale ACK in the USB input queue, falsely completing subsequent commands.
- [High] Unsafe spinlock acquisition without disabling interrupts in HID raw_event callback causes deadlocks.
--
commit ae4697cc229899bfbbf1b0ea18f1f026bc1b15e7
Author: Derek J. Clark [off-list ref]
HID: hid-msi: Add M-key mapping attributes
This commit adds attributes to allow remapping the M-keys with up to 5 values
when the device is in macro mode. When mapped, these events can fire from
the xpad device or the evdev interfaces provided by the input core.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 3a176f44f2c43..f099d6cb7ea8d 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -137,6 +321,39 @@ 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)
+{
+ enum claw_profile_ack_pending profile;
+ struct claw_mkey_report *mkeys;
+ u8 *codes, key;
+ int i;
+
+ scoped_guard(spinlock, &drvdata->profile_lock)
^^^^^^^^
[Severity: High]
Could this cause a deadlock?
HID raw_event callbacks can be invoked from process context with IRQs
enabled (e.g., when input is simulated via the uhid character device).
If an actual hardware interrupt (such as a USB URB completion) occurs on
the same CPU while the lock is held in process context, the interrupt handler
will invoke raw_event and attempt to reacquire profile_lock, resulting
in a deadlock.
Should this use spinlock_irqsave instead?
quoted hunk ↗ jump to hunk
+ profile = drvdata->profile_pending;
+
+ switch (profile) {
+ case CLAW_M1_PENDING:
[ ... ]
quoted hunk ↗ jump to hunk
+ dev_dbg(&drvdata->hdev->dev,
+ "Got profile event without changes pending from command: %x\n",
+ cmd_rep->cmd);
+ return -EINVAL;
+ }
+ scoped_guard(spinlock, &drvdata->profile_lock)
^^^^^^^^
[Severity: High]
Same question here, could this deadlock if interrupted by a hardware
interrupt on the same CPU?
quoted hunk ↗ jump to hunk
+ drvdata->profile_pending = CLAW_NO_PENDING;
+
+ return 0;
+}
[ ... ]
+static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+set_buttons:
+ scoped_guard(mutex, &drvdata->rom_mutex) {
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+ /* MCU will not send ACK until the USB transaction completes. ACK is sent
+ * immediately after and will hit the stale state machine, before the next
+ * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
+ * for ACK that ill never come.
+ */
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
[Severity: High]
If the ACK for CLAW_COMMAND_TYPE_SYNC_TO_ROM is abandoned, could it leave a
stale ACK in the USB input queue?
If userspace rapidly writes to the sysfs attributes (e.g., configuring
button_m1 then button_m2), the second write would acquire rom_mutex, send
WRITE_PROFILE_DATA, and set waiting_for_ack = true.
When the late ACK from the preceding SYNC_TO_ROM command arrives,
claw_raw_event() might process it as a generic CLAW_COMMAND_TYPE_ACK and
prematurely complete the wait for the new WRITE_PROFILE_DATA command.
The driver might then immediately send the next SYNC_TO_ROM while the MCU
may still be processing the profile data, potentially leading to dropped
configurations or MCU instability.
quoted hunk ↗ jump to hunk
[ ... ]
quoted hunk ↗ jump to hunk
+static int claw_mkey_show(struct device *dev, char *buf, enum claw_key_index m_key)
+{
+ struct hid_device *hdev = to_hid_device(dev);
[ ... ]
quoted hunk ↗ jump to hunk
+ guard(mutex)(&drvdata->profile_mutex);
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ 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,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
[Severity: High]
If claw_hw_output_report() times out, could this leave profile_pending
dirty?
If it returns early without resetting drvdata->profile_pending to
CLAW_NO_PENDING, and another process subsequently invokes claw_mkey_show()
for a different key, it would overwrite profile_pending and begin waiting
for a new ACK.
If the late ACK from the initial timed-out request arrives during this
window, claw_profile_event() processes it, detects an address mismatch
(since it evaluates against the new profile_pending state), and returns
-EINVAL.
claw_raw_event() would then blindly propagate this error to cmd_status and
call complete(), falsely aborting the valid, ongoing read.
quoted hunk ↗ jump to hunk
+
+ for (i = 0; i < CLAW_KEYS_MAX; i++) {
+ name = mkey_mapping_code_to_name(codes[i]);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=2