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

[sashiko-bot@kernel.org]

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 <derekjohn.clark@gmail.com>

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.

> 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;
> +	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.

> +	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