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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unhandled timeouts and spurious ACKs cause the READ_PROFILE state machine to desynchronize, causing legitimate reads to fail.
- [High] `claw_profile_event` unconditionally clears `profile_pending` after temporarily dropping the `profile_lock`, potentially overwriting concurrent state changes.
- [High] Fire-and-forget commands with `timeout = 0` generate unhandled generic ACKs that can spoof and prematurely complete unrelated blocking commands.
- [Low] The declaration of `struct claw_profile_report;` inside `struct claw_mkey_report` does not declare a member, causing build failures.
--

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

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

This patch adds sysfs attributes that allow remapping the M-keys on MSI Claw
devices when in macro mode. The mapping supports up to 5 values for the M1
and M2 buttons.

> 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

[ ... ]

> +struct claw_profile_report {
> +	u8 profile;
> +	__be16 read_addr;
> +} __packed;
> +
> +struct claw_mkey_report {
> +	struct claw_profile_report;

[Severity: Low]
Will this compile correctly?

In standard C11 and GCC (without -fms-extensions), declaring a struct tag
without a declarator inside a struct definition only declares the tag in that
scope, rather than creating an anonymous embedded struct member.

This means struct claw_mkey_report won't have a member named read_addr, which
would cause mkeys->read_addr to produce a compilation error later.

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

[ ... ]

> +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:
> +		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 -EINVAL;
> +		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, &drvdata->profile_lock)
> +		drvdata->profile_pending = CLAW_NO_PENDING;

[Severity: High]
Is it safe to unconditionally clear profile_pending here after dropping and
reacquiring the lock?

If a timeout occurred in claw_mkey_show() for M1 just before this function
drops the lock, the M1 thread would release profile_mutex. A concurrent
thread could then enter claw_mkey_show() for M2 and set profile_pending to
CLAW_M2_PENDING.

When this function reacquires the lock, it would unconditionally overwrite
the state set by the concurrent thread, which could cause the M2 command to
fail when its own ACK arrives.

> +
> +	return 0;
> +}

[ ... ]

> +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]
Does sending CLAW_COMMAND_TYPE_SYNC_TO_ROM with timeout 0 cause issues with
subsequent commands?

Because timeout is 0, the driver does not wait for the generic ACK that the
MCU will eventually send. This means the ACK will be left lingering and will
be processed asynchronously by claw_raw_event.

If another thread later sends a command that waits for a generic ACK (such
as CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA), could it falsely complete early
by consuming the lingering ACK before the MCU has actually finished processing
its request?

> +	}
> +
> +	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->gp_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: High]
If claw_hw_output_report times out and returns early, doesn't it leave
drvdata->profile_pending set?

If another thread then initiates a READ_PROFILE for the other M-key (e.g., M2),
it will overwrite profile_pending. When the late ACK from the timed-out M1
command eventually arrives, claw_profile_event() will read CLAW_M2_PENDING
and compare the ACK's address (which is for M1) against M2's expected address.
This would return -EINVAL and falsely complete the active M2 command with an
error.

> +
> +	for (i = 0; i < CLAW_KEYS_MAX; i++) {
> +		name = mkey_mapping_code_to_name(codes[i]);

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