Re: [PATCH v8 2/4] HID: hid-msi: Add M-key mapping attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Command synchronization cross-talk due to unconsumed generic ACKs from `SYNC_TO_ROM`
- [Medium] TOCTOU race in `claw_profile_event` unconditionally overwrites `profile_pending`
- [Low] Invalid anonymous struct declaration in `struct claw_mkey_report`
--

commit b8dd002e01cbcbf34fdd54df7543cbafb1207a47
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add M-key mapping attributes

This commit adds attributes that allow for remapping the M-keys with up to 5 
values when in macro mode. It adds support for mappings from the gamepad, 
keyboard, or mouse event devices.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 40b16253abbb3..b9901869075f6 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -87,25 +249,47 @@ struct claw_command_report {
>  	u8 report_id;
>  	u8 padding[2];
>  	u8 header_tail;
>  	u8 cmd;
>  	u8 data[59];
>  } __packed;
>  
> +struct claw_profile_report {
> +	u8 profile;
> +	__be16 read_addr;
> +} __packed;
> +
> +struct claw_mkey_report {
> +	struct claw_profile_report;

[Severity: Low]
Does this act as a forward declaration rather than an embedded field? Under
standard C11 and GNU C, a struct with a tag cannot be an anonymous embedded
member without specific compiler extensions. Will this cause a compilation
error when accessing fields like mkeys->read_addr later?

> +	u8 padding_0;
> +	u8 padding_1;
> +	u8 padding_2;
> +	u8 codes[5];
> +} __packed;

[ ... ]

> @@ -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)
> +		profile = drvdata->profile_pending;
> +
> +	switch (profile) {
> +	case CLAW_M1_PENDING:
> +	case CLAW_M2_PENDING:

[ ... ]

> +	default:
> +		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)
> +		drvdata->profile_pending = CLAW_NO_PENDING;

[Severity: Medium]
Could this unconditionally overwrite a new state if a timeout occurs? If a
command times out, the waiting thread might drop profile_mutex, allowing a
new thread to enter claw_mkey_show() and set profile_pending for a new
operation. If the delayed ACK from the first command arrives at that moment,
will this clear the new thread's state and cause its operation to fail?

> +
> +	return 0;
> +}

[ ... ]

> @@ -442,6 +670,177 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,

[ ... ]

> +static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
> +{

[ ... ]

> +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]
Can this lead to command synchronization cross-talk? Since timeout is 0,
the driver won't wait for the ACK. If a user rapidly issues another sysfs
write, the next operation could take rom_mutex and send its own command.

Because generic ACKs appear to lack command IDs, will claw_raw_event()
blindly consume the orphaned ACK from this command and prematurely complete
the new command's wait before the MCU has actually processed it?

> +	}
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522015518.1111290-1-derekjohn.clark@gmail.com?part=2