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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `guard(mutex)` macro in `claw_hw_output_report` is scoped to an `if` block, prematurely releasing the lock before the critical section.
- [High] The driver publishes `drvdata` via `hid_set_drvdata()` before fully initializing its mutex and completion objects, creating a race condition.
- [Medium] An invalid, uninitialized gamepad mode (0) may be sent to the hardware during system resume.
- [Medium] The 8ms timeout used in `claw_hw_output_report()` is vulnerable to jiffies rounding, leading to spurious read failures.
- [Medium] The commit message claims probe checks MCU RAM address variances, but the implementation is missing.
--

commit 13b388de96d7484eee908adeed127e8086fc289e
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 support for switching the gamepad mode and changing
the behavior of the M-keys.

>     Additionally, the MCU RAM address for writing configuration data has
>     changed twice. Checks are done during probe to enumerate these variances.

The commit message states that checks are done during probe to enumerate MCU
RAM address variances.

Looking at claw_probe() and msi_probe(), there does not appear to be any code
that checks firmware versions or handles these variances.

Was this functionality dropped during refactoring, or should the commit message
be updated?

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..8915942af15e6
> --- /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);
> +
> +	/* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
> +	if (timeout) {
> +		guard(mutex)(&drvdata->cfg_mutex);
> +		reinit_completion(&drvdata->send_cmd_complete);
> +	}

Does this drop the lock too early?

The guard() macro releases the lock at the end of its enclosing scope.
Because it is declared inside the if (timeout) block, the mutex will be
released immediately at the closing brace, leaving hid_hw_output_report() and
the subsequent wait unprotected against concurrent access.

> +
> +	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)
> +		return ret;
> +
> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> +	if (ret)
> +		return ret;
> +
> +	if (timeout) {
> +		ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> +								msecs_to_jiffies(timeout));

Could this timeout evaluate to 1 jiffy on systems configured with HZ=100?

Several callers, such as gamepad_mode_show(), pass an 8ms timeout to this
function. A 1-jiffy timeout can expire almost instantaneously if the operation
begins near the end of the current tick boundary, potentially leading to
spurious -EBUSY timeouts.

[ ... ]

> +static void cfg_resume_fn(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> +	struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
> +	u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };

If the system suspends before cfg_setup_fn() executes, or if the device read in
cfg_setup_fn() fails, could drvdata->gamepad_mode still be 0 here?

Since 0 is not a valid mode (valid modes like CLAW_GAMEPAD_MODE_XINPUT start
at 1), will this send an uninitialized configuration state to the hardware?

> +	int ret;
> +
> +	ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> +				    ARRAY_SIZE(data), 0);
> +	if (ret)
> +		dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> +}
> +
> +static int claw_probe(struct hid_device *hdev, u8 ep)
> +{
> +	struct claw_drvdata *drvdata;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	hid_set_drvdata(hdev, drvdata);
> +	drvdata->hdev = hdev;
> +	drvdata->ep = ep;
> +
> +	mutex_init(&drvdata->cfg_mutex);
> +	init_completion(&drvdata->send_cmd_complete);

Is it safe to publish drvdata via hid_set_drvdata() before initializing the
mutex and completion object?

Since hid_hw_start() was already called earlier in msi_probe(), the hardware
is active. If an input report arrives right after hid_set_drvdata(), the
interrupt handler msi_raw_event() will process it and call complete() on an
uninitialized waitqueue.

> +
> +	/* For control interface: open the HID transport for sending commands. */
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		return ret;
> +
> +	INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> +	INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> +	schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
> +
> +	return 0;
> +}

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