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 [off-list 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.
quoted
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
[ ... ]
quoted
+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;
[ ... ]
quoted
+ guard(mutex)(&drvdata->cfg_mutex);
[ ... ]
quoted
+ 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
quoted
+
+ 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;
+}