Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Typographical error checking `gamepad_registered` instead of `gp_registered` causes newly added sysfs attributes to unconditionally return `-ENODEV`.
- [High] Dangling asynchronous ACK from `SYNC_TO_ROM` can prematurely complete subsequent commands, corrupting the device state machine.
- [Medium] Spurious or late `READ_PROFILE_ACK` packets incorrectly abort subsequent profile reads with `-EINVAL`.
- [Medium] If `claw_hw_output_report()` fails in `claw_mkey_show()`, the function exits without clearing the `profile_pending` state.
--
commit fe7de95a59af02c44e638aba1fcc1763f3b5354f
Author: Derek J. Clark [off-list ref]
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. Mapped events will fire from gamepad, keyboard, or mouse evdevs.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 3a176f44f2c43..5f20102eae69d 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -87,24 +249,47 @@ struct claw_command_report {
u8 data[59];
} __packed;
[ ... ]
quoted hunk ↗ jump to hunk
struct claw_drvdata {
/* MCU General Variables */
+ enum claw_profile_ack_pending profile_pending;
struct completion send_cmd_complete;
[ ... ]
quoted hunk ↗ jump to hunk
/* Gamepad Variables */
enum claw_mkeys_function_index mkeys_function;
enum claw_gamepad_mode_index gamepad_mode;
+ u8 m1_codes[CLAW_KEYS_MAX];
+ u8 m2_codes[CLAW_KEYS_MAX];
+ bool gamepad_registered;
spinlock_t mode_lock; /* Lock for mode data read/write */
+ const u16 *bmap_addr;
+ bool bmap_support;
bool gp_registered;
};
[ ... ]
quoted hunk ↗ jump to hunk
@@ -165,9 +383,20 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
complete(&drvdata->send_cmd_complete);
}
+ break;
+ case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
+ ret = claw_profile_event(drvdata, cmd_rep);
+ if (drvdata->waiting_for_ack &&
+ drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE) {
+ drvdata->cmd_status = ret;
+ drvdata->waiting_for_ack = false;
+ complete(&drvdata->send_cmd_complete);
+ }
[Severity: Medium]
If a spurious or late READ_PROFILE_ACK packet arrives, claw_profile_event()
will return -EINVAL. Does this unconditionally pass the error to cmd_status
and prematurely complete the wait for a valid, currently pending READ_PROFILE
command?
quoted hunk ↗ jump to hunk
+
+ break;
case CLAW_COMMAND_TYPE_ACK:
[ ... ]
quoted hunk ↗ jump to hunk
@@ -442,6 +671,177 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
[ ... ]
quoted hunk ↗ jump to hunk
+static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[mkey])},
+ 0x07, 0x04, 0x00, {0xff, 0xff, 0xff, 0xff, 0xff} };
+ char **raw_keys __free(argv_free) = NULL;
+ int ret, key_count, i;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gamepad_registered))
+ return -ENODEV;
+ }
[Severity: High]
Does this check the newly added gamepad_registered field instead of the
already existing gp_registered field?
Since gamepad_registered is never set to true, it looks like this will
unconditionally return -ENODEV, rendering the sysfs attributes inaccessible.
+
+ raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
[ ... ]
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]
Since this command uses a timeout of 0, could the asynchronous ACK from
SYNC_TO_ROM arrive while the driver is waiting for a subsequent command
(like another WRITE_PROFILE_DATA)?
If the generic ACK arrives after the driver starts waiting for the next
command, it seems it could falsely complete the next command prematurely
and break the lock-step protocol between host and MCU.
quoted hunk ↗ jump to hunk
+ }
+
+ return ret;
+}
+
+static int claw_mkey_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);
+ struct claw_mkey_report report = { {0x01, cpu_to_be16(drvdata->bmap_addr[m_key])}, 0x07 };
+ int i, ret, count = 0;
+ const char *name;
+ u8 *codes;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gamepad_registered))
+ return -ENODEV;
+ }
+
+ codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
+
+ 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: Medium]
If claw_hw_output_report() fails or times out, this returns immediately.
Does this leave drvdata->profile_pending set to a pending state?
If the state is left dangling after the mutex is released, could a subsequent
late or spurious profile packet be incorrectly accepted and parsed into the
local codes arrays?
+
+ for (i = 0; i < CLAW_KEYS_MAX; i++) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=2