Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] Race condition in `claw_profile_event` causes stale data to be returned for profile reads.
- [Low] Compilation error due to invalid C syntax for embedded tagged struct.
- [Low] Subsystem guide violation: Mixing `goto`-based cleanup labels with `__free()` automated cleanup.
Pre-existing issues:
- [High] Generic ACK responses improperly complete commands waiting for specific data payloads.
- [High] Gamepad sysfs attributes are permanently hidden and never created.
--
--- Patch [2]: [PATCH v13 2/4] HID: hid-msi: Add M-key mapping attributes ---
commit 4c48f895b57157a8a075f47f1218b8052e864f1b
Author: Derek J. Clark [off-list ref]
HID: hid-msi: Add M-key mapping attributes
This patch adds attributes that allow for remapping the M-keys on MSI
devices when in macro mode. It routes mapped buttons to appropriate event
devices for gamepad, keyboard, or mouse events.
Link: https://patch.msgid.link/20260529072111.7565-3-derekjohn.clark@gmail.com (local)
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 6687e4579faa1..023d63cee6b06 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -255,6 +255,15 @@ struct claw_command_report {
u8 data[59];
} __packed;
+struct claw_profile_report {
+ u8 profile;
+ __be16 read_addr;
+} __packed;
+
+struct claw_mkey_report {
+ struct claw_profile_report;
+ u8 padding_0;
[Severity: Low]
Will this compile successfully, or does this introduce a regression?
Standard C11 only permits anonymous structs if they do not have a tag. Using
a tagged struct (like struct claw_profile_report;) without a field name acts
as a forward declaration.
This makes padding_0 the first member, causing the initialization in
claw_mkey_store() to fail, and attempts to access mkeys->read_addr in
claw_profile_event() will fail with a missing member error.
[ ... ]
quoted hunk ↗ jump to hunk
+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_irqsave, &drvdata->profile_lock)
+ profile = drvdata->profile_pending;
+
+ switch (profile) {
+ case CLAW_M1_PENDING:
[ ... ]
quoted hunk ↗ jump to hunk
+ default:
+ dev_dbg(&drvdata->hdev->dev,
+ "Got profile event without changes pending from command: %x\n",
+ cmd_rep->cmd);
+ return -EINVAL;
+ }
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->profile_pending = CLAW_NO_PENDING;
+
+ return 0;
+}
[Severity: High]
Is there a race condition here that introduces a regression where stale
data is returned?
The lock is dropped after reading profile_pending, opening a race window
where another thread could update it. When this thread resumes and
unconditionally writes CLAW_NO_PENDING, it could destroy pending profile
requests initiated during the race window.
For example, if Thread 1 times out leaving CLAW_M1_PENDING, and Thread 2
sets CLAW_M2_PENDING, a delayed hardware response for M1 could cause this
function to overwrite Thread 2's state with CLAW_NO_PENDING. This would
cause Thread 2 to return unmodified M2 data.
[ ... ]
static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
u8 *data, int size)
{
[ ... ]
case CLAW_COMMAND_TYPE_ACK:
if (drvdata->orphan_ack_pending) {
drvdata->orphan_ack_pending = false;
complete(&drvdata->orphan_ack_complete);
break;
}
if (drvdata->waiting_cmd == CLAW_COMMAND_TYPE_NONE) {
dev_warn(&drvdata->hdev->dev, "Got unexpected ACK from MCU, ignoring\n");
break;
}
drvdata->cmd_status = 0;
complete(&drvdata->send_cmd_complete);
[Severity: High]
This is a pre-existing issue, but do generic ACKs improperly complete
commands waiting for specific data payloads?
If a previous command (like SYNC_TO_ROM with a timeout of 0) takes longer
than the timeout to send its ACK, the orphan_ack_pending flag might clear
before the generic ACK arrives.
When the delayed generic ACK finally arrives while waiting_cmd is set for
a new command (like READ_PROFILE), this branch skips the orphan check and
unconditionally triggers complete(&drvdata->send_cmd_complete). This
prematurely completes the new command's wait, potentially returning stale
or uninitialized data.
[ ... ]
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;
[ ... ]
err:
scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
drvdata->orphan_ack_pending = false;
}
return ret;
}
[Severity: Low]
This is a pre-existing issue, but does this function mix goto-based cleanup
labels with __free() automated cleanup?
The function declares unsigned char *dmabuf __free(kfree) = NULL; but also
utilizes a goto err; label for state cleanup. The cleanup and guard
subsystem guidelines state that functions should either convert all
resources to scope-based cleanup or convert none of them.
[ ... ]
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);
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->gp_registered))
return 0;
}
[Severity: High]
This is a pre-existing issue, but are the gamepad sysfs attributes ever
created and made visible?
In cfg_setup_fn(), device_add_group() is called before gp_registered is
set to true. During sysfs group creation, this callback evaluates
!smp_load_acquire(&drvdata->gp_registered) and returns 0.
Because it returns 0 during creation, the files are never created. Even
though gp_registered is set to true immediately afterward, the attributes
will remain permanently hidden since sysfs_update_group is not called.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720031549.2272658-1-derekjohn.clark@gmail.com?part=2