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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Non-atomic update of drvdata->profile_pending can overwrite concurrent state changes, leading to sysfs read failures.
- [High] Orphan ACKs from SYNC_TO_ROM commands are ignored and leave the `orphan_ack_pending` flag set if they arrive while waiting for a READ command, causing the next WRITE command to time out.
--

--- Patch [2]: [PATCH v12 2/4] HID: hid-msi: Add M-key mapping attributes ---
commit 21a823af0d7845d277f06cccaf5818c1b1be1ce7
Author: Derek J. Clark <derekjohn.clark@gmail.com>

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.

Link: https://patch.msgid.link/20260529072111.7565-3-derekjohn.clark@gmail.com

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 8a48ce099c1ca..9d3249fa50291 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -143,6 +327,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_irqsave, &drvdata->profile_lock)
> +		profile = drvdata->profile_pending;
> +
> +	switch (profile) {
> +	case CLAW_M1_PENDING:
> +	case CLAW_M2_PENDING:
> +		key = (profile == CLAW_M1_PENDING) ? CLAW_KEY_M1 : CLAW_KEY_M2;
> +		mkeys = (struct claw_mkey_report *)cmd_rep->data;
> +		if (be16_to_cpu(mkeys->read_addr) != drvdata->bmap_addr[key])
> +			return -EAGAIN;
> +		codes = (profile == CLAW_M1_PENDING) ? drvdata->m1_codes : drvdata->m2_codes;
> +		for (i = 0; i < CLAW_KEYS_MAX; i++)
> +			codes[i] = (mkeys->codes[i]);
> +		break;
> +	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;

[Severity: High]
Can this unconditionally clearing profile_pending overwrite concurrent
state changes?

If Thread A times out waiting for M1, and Thread B starts waiting for M2
by setting profile_pending = CLAW_M2_PENDING. If the delayed USB ACK
for M1 then arrives, claw_profile_event processes it and executes:

    scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
        drvdata->profile_pending = CLAW_NO_PENDING;

Doesn't this clobber Thread B's pending M2 state, causing the M2 ACK to
be rejected and Thread B to permanently time out?

> +
> +	return 0;
> +}
> +
>  static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
>  			  u8 *data, int size)
>  {

[ ... ]

> @@ -171,9 +388,23 @@ 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);
> +		/* Stale address received, ignore and keep waiting */
> +		if (ret == -EAGAIN)
> +			return 0;
> +		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);
> +		}
> +
>  		break;
>  	case CLAW_COMMAND_TYPE_ACK:
> -		if (drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE)
> +		if (drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_PROFILE ||
> +		    drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE)
>  			break;
>  
>  		if (drvdata->orphan_ack_pending) {

[Severity: High]
Does this early break for READ commands permanently skip processing
the orphan ACK?

If a WRITE_PROFILE_DATA command is sent followed by a SYNC_TO_ROM
command, orphan_ack_pending is set to true. If userspace then
immediately issues a sysfs read, setting waiting_cmd to
CLAW_COMMAND_TYPE_READ_PROFILE, and the generic orphan ACK arrives:

claw_raw_event() will evaluate the condition above, execute the early
break, and bypass clearing orphan_ack_pending.

Later, when another write is issued, won't its real generic ACK be
incorrectly consumed as the leftover orphan ACK, causing the new write
to time out since its completion is never signaled?

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