Re: [PATCH v11 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] 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 <derekjohn.clark@gmail.com>

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.

> 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

[ ... ]

> @@ -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?

> +		profile = drvdata->profile_pending;
> +
> +	switch (profile) {
> +	case CLAW_M1_PENDING:

[ ... ]

> +		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?

> +		drvdata->profile_pending = CLAW_NO_PENDING;
> +
> +	return 0;
> +}

[ ... ]

> +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]
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.

> +	}
> +
> +	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);

[ ... ]

> +	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.

> +
> +	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