Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing `goto`-based error handling with scope-based cleanup (`__free()` and `guard()`) in the same function.
--
commit 99c97f725ca1423e8aa76228aadfc2b109079d67
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 provides attributes for changing the gamepad mode, M-key
behavior, and resetting the device, using a bypass for completion handling
to handle expected USB disconnects.
quoted hunk ↗ jump to hunk
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 hunk ↗ jump to hunk
+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;
+ u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ size_t header_size = ARRAY_SIZE(header);
+ int ret;
+
+ if (header_size + len > CLAW_PACKET_SIZE)
+ return -EINVAL;
+
+ /* We can't use a devm_alloc reusable buffer without side effects during suspend */
+ dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ memcpy(dmabuf, header, header_size);
+ if (data && len)
+ memcpy(dmabuf + header_size, data, len);
+
+ guard(mutex)(&drvdata->cfg_mutex);
+ if (timeout) {
+ reinit_completion(&drvdata->send_cmd_complete);
+ scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
+ drvdata->waiting_cmd = index;
+ drvdata->waiting_for_ack = true;
+ drvdata->cmd_status = -ETIMEDOUT;
+ }
+ }
+
+ dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
+ CLAW_PACKET_SIZE, dmabuf);
+
+ ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
+ if (ret < 0)
+ goto err_clear_state;
+
+ 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:
[Severity: Medium]
Is it intentional to mix goto-based error handling with scope-based cleanup
helpers in the same function?
This function uses __free(kfree) for dmabuf and guard(mutex) for the
cfg_mutex, but also uses a goto err_clear_state label for error handling.
The kernel cleanup subsystem guidelines recommend that usage of goto and
cleanup helpers never be mixed in the same function to avoid confusing
ownership semantics.
quoted hunk ↗ jump to hunk
+ if (timeout) {
+ guard(spinlock_irqsave)(&drvdata->cmd_lock);
+ drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
+ drvdata->waiting_for_ack = false;
+ }
+
+ return ret;
+}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260527222122.10620-1-derekjohn.clark@gmail.com?part=1