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

- From: Derek J. Clark \<derekjohn.clark@gmail.com\>
- Date: 2026-05-25 03:11:11 
- List: linux-input
- Page: https://ratatoskr.run/linux-input/2026/05/17025337
- Thread: https://ratatoskr.run/sashiko-reviews/2026/05/17025337/t.md

```text
On May 25, 2026 11:37:12 AM GMT+09:00, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>- [Medium] Mixing goto-based error handling with scope-based cleanups (__free, guard) in claw_hw_output_report.
>--
>
>commit 279964e3a8cf0a3835903d6bad7357f0ca42f523
>Author: Derek J. Clark [ref]
>
>HID: hid-msi: Add MSI Claw configuration driver
>
>This commit adds a configuration HID driver for the MSI Claw series of
>handheld PCs. It introduces the initial driver outline and attributes for
>changing gamepad mode, M-key behavior, and resetting the device. It also
>adds a completion bypass when the timeout is 0 to handle USB disconnects
>gracefully.
>
>> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
>> new file mode 100644
>> index 0000000000000..3a176f44f2c43
>> --- /dev/null
>> +++ b/drivers/hid/hid-msi.c
>
>[ ... ]
>
>> +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;
>
>[ ... ]
>
>> +	guard(mutex)(&drvdata->cfg_mutex);
>
>[ ... ]
>
>> +	ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
>> +	if (ret < 0)
>> +		goto err_clear_state;
>
>[Severity: Medium]
Hi robot,

Interesting that this same issue was Severity: Low in v8...

>Is it intended to mix scope-based cleanups with goto statements in this
>function? The subsystem guidelines recommend either converting all resources
>that need a goto cleanup to scope-based cleanups, or converting none of them.
>Mixing __free() and guard() declarations with goto-based error handling in the
>same function can create confusing ownership semantics.
>

The goto's don't return early or break in/out of guarded sections. It simply skips waiting for the completion if there was an error sending the payload. Using this goto removes the need for 8 lines of duplicated code and the use of cmd_lock in 2 additional places. Unless Jiri or Benjamin specifically ask for it I don't intend to change this.

- Derek

>> +
>> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
>> +	if (ret)
>> +		goto err_clear_state;
>> +
>> +	if (timeout) {
>> +		ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
>> +								msecs_to_jiffies(timeout));
>> +
>> +		dev_dbg(&hdev->dev, "Remaining timeout: %u\n", ret);
>> +		ret = ret > 0 ? drvdata->cmd_status : ret ?: -EBUSY;
>> +	}
>> +
>> +err_clear_state:
>> +	if (timeout) {
>> +		guard(spinlock_irqsave)(&drvdata->cmd_lock);
>> +		drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
>> +		drvdata->waiting_for_ack = false;
>> +	}
>> +
>> +	return ret;
>> +}
>

```