Re: [PATCH v13 1/4] HID: hid-msi: Add MSI Claw configuration driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Delayed generic ACKs from a previous timed-out command can falsely complete a subsequent command expecting a specific ACK, leading to silent state corruption.
- [Medium] Mixing `goto`-based error handling with scope-based `__free()` cleanup in the same function.
- [Medium] Sysfs attributes can spuriously return `-ENODEV` immediately after creation because they are exposed before `gp_registered` is set to true.
--

--- Patch [1]: [PATCH v13 1/4] HID: hid-msi: Add MSI Claw configuration driver ---
commit 12a4a941e97290e1a15080092de490d5782eef1c
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add MSI Claw configuration driver

Adds configuration HID driver for the MSI Claw series of handheld PC's.
In this initial patch add the initial driver outline and attributes for
changing the gamepad mode, M-key behavior, and add a WO reset function.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
> +			  u8 *data, int size)
> +{
[ ... ]
> +	switch (cmd_rep->cmd) {
> +	case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> +		ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> +		if (drvdata->waiting_cmd == CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE) {
> +			drvdata->cmd_status = ret;
> +			complete(&drvdata->send_cmd_complete);
> +		}
> +
> +		break;
> +	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: Medium]
When claw_raw_event() receives a generic ACK, it only verifies that
waiting_cmd is not CLAW_COMMAND_TYPE_NONE before calling
complete(&drvdata->send_cmd_complete).

If a previous command with timeout == 0 timed out, __claw_hw_output_report()
clears orphan_ack_pending.

If the generic ACK for that command arrives later while a new command is
waiting (e.g., waiting for GAMEPAD_MODE_ACK), this logic will accept the
generic ACK and falsely complete the new command.

Could this lead to silent state corruption where commands finish early without
receiving their actual payloads?

[ ... ]
> +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;
[ ... ]
> +	ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> +	if (ret)
> +		goto err;

[Severity: Medium]
This function uses the scope-based __free(kfree) cleanup for dmabuf, but also
uses manual goto err labels for error handling.

The cleanup subsystem guidelines specify that using goto and cleanup helpers
like __free() in the same function creates confusing ownership semantics and is
an anti-pattern.

Could we use a scope-based guard for the state machine cleanup, or stick
entirely to manual goto cleanup?

[ ... ]
> +static void cfg_setup_fn(struct work_struct *work)
> +{
[ ... ]
> +	/* Add sysfs attributes after we get the device state */
> +	ret = device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
> +	if (ret) {
> +		dev_err(&drvdata->hdev->dev,
> +			"Failed to setup device, can't create gamepad attrs: %d\n", ret);
> +		return;
> +	}
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
> +		/* Pairs with smp_load_acquire in attribute show/store functions */
> +		smp_store_release(&drvdata->gp_registered, true);

[Severity: Medium]
The sysfs attributes are exposed to userspace by device_add_group() before
gp_registered is set to true.

If an automated userspace tool reacts to the sysfs creation event and accesses
the attributes in this small timing window, the attribute show/store functions
will observe gp_registered == false and return -ENODEV.

Would it be better to set gp_registered to true before calling
device_add_group(), and revert it to false if the addition fails?

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