Re: [PATCH v5 3/4] HID: hid-msi: Add RGB control interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Holding `cfg_mutex` while removing sysfs attribute groups in `claw_remove()` deadlocks with concurrent sysfs operations.
- [High] Teardown race condition allows `rgb_queue` to be re-queued after cancellation, leading to a Use-After-Free.
- [High] Unconditionally canceling the `rgb_queue` delayed work causes a crash if the setup worker has not executed.
- [Medium] Spurious timeouts due to an 8ms wait limit can break command-response synchronization.
- [Medium] Error handling paths in device initialization log errors using a NULL device pointer.
- [Medium] RGB work can be incorrectly scheduled while the device is suspended.
- [Low] Using a `break` statement inside a `scoped_guard` block creates confusing control flow.
--

commit 4a725c6bd4a5cd260e20d40567af5406130e6b41
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add RGB control interface

Adds RGB control interface for MSI Claw devices, supporting 9 zones
and up to 8 sequential frames of RGB data written to MCU memory.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 991d5a25d3de0..4443f53b54cb1 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -311,15 +375,45 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
>  		for (i = 0; i < CLAW_KEYS_MAX; i++)
>  			codes[i] = (cmd_rep->data[6 + i]);
>  		break;
> +	case CLAW_RGB_PENDING:
> +		frame = (struct rgb_report *)cmd_rep->data;
> +		rgb_addr = drvdata->rgb_addr;
> +		read_addr = be16_to_cpu(frame->read_addr);
> +		frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
> +		if (frame_calc >= CLAW_RGB_MAX_FRAMES) {
> +			dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> +				frame_calc);

Since this function is called during the initial setup in
claw_read_rgb_config() before devm_led_classdev_multicolor_register() has
run, is led_cdev.dev potentially NULL here? Will it print an obscured
"(NULL device *)" log message instead of the device name? 

Could we use &drvdata->hdev->dev instead?

> +			drvdata->profile_pending = CLAW_NO_PENDING;
> +			return -EINVAL;
> +		}
> +		f_idx = frame_calc;
> +
> +		scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
> +			memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> +			       sizeof(struct rgb_frame));
> +
> +			/* Only use frame 0 for remaining variable assignment */
> +			if (f_idx != 0)
> +				break;

This isn't a bug, but using a break inside a scoped_guard escapes the
internal hidden for loop used to implement the macro. It works correctly
here because the lock is dropped and the next line is the switch statement's
own break, but it creates confusing control flow. 

Should we use an if (f_idx == 0) { ... } block instead?

> +
> +			drvdata->rgb_speed = frame->speed;
> +			drvdata->led_mc.led_cdev.brightness = frame->brightness;
> +			drvdata->led_mc.subled_info[0].intensity = frame->zone_data.zone[0].red;
> +			drvdata->led_mc.subled_info[1].intensity = frame->zone_data.zone[0].green;
> +			drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
> +		}
> +
> +		break;

[ ... ]

> @@ -769,6 +863,420 @@ static const struct attribute_group claw_gamepad_attr_group = {
>  	.is_visible = claw_gamepad_attr_is_visible,
>  };
>  
> +/* Read RGB config from device */
> +static int claw_read_rgb_config(struct hid_device *hdev)
> +{
> +	u8 data[4] = { 0x01, 0x00, 0x00, CLAW_RGB_FRAME_OFFSET };
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	u16 read_addr = drvdata->rgb_addr;
> +	size_t len = ARRAY_SIZE(data);
> +	int ret, i;
> +
> +	if (!drvdata->rgb_addr)
> +		return -ENODEV;
> +
> +	/* Loop through all 8 pages of RGB data */
> +	guard(mutex)(&drvdata->profile_mutex);
> +	for (i = 0; i < 8; i++) {
> +		drvdata->profile_pending = CLAW_RGB_PENDING;
> +		data[1] = (read_addr >> 8) & 0xff;
> +		data[2] = read_addr & 0x00ff;
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 8);

Does an 8ms timeout pose a risk of spurious timeouts on systems where HZ=100?

With HZ=100, msecs_to_jiffies(8) evaluates to 1 jiffy. If the system timer
ticks immediately after the wait begins,
wait_for_completion_interruptible_timeout() can return 0 in less than 1ms.
If this happens, does it break command synchronization since the hardware
will eventually ACK the first command?

[ ... ]

> +static void cfg_setup_fn(struct work_struct *work)
>  {
>  	struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> @@ -782,6 +1290,13 @@ static void cfg_setup_fn(struct work_struct *work)
>  		return;
>  	}
>  
> +	ret = claw_read_rgb_config(drvdata->hdev);
> +	if (ret) {
> +		dev_err(drvdata->led_mc.led_cdev.dev,
> +			"Failed to setup device, can't read RGB config: %d\n", ret);

Is it possible led_cdev.dev is NULL here since
devm_led_classdev_multicolor_register() has not been called yet?

> +		return;
> +	}
> +
>  	/* Add sysfs attributes after we get the device state */
>  	ret = device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
>  	if (ret) {
> @@ -791,7 +1306,25 @@ static void cfg_setup_fn(struct work_struct *work)
>  	}
>  	drvdata->gamepad_registered = true;
>  
> +	/* Add and enable RGB interface once we have the device state */
> +	INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);

If INIT_DELAYED_WORK is called asynchronously 500ms after probe, what
happens if the device is unplugged or suspended before this timer expires?

Will the unconditional cancel_delayed_work_sync(&drvdata->rgb_queue) in
claw_remove() or claw_suspend() be called on a zero-initialized work
structure, causing undefined behavior and a kernel crash?

[ ... ]

> @@ -918,10 +1475,15 @@ static void claw_remove(struct hid_device *hdev)
>  		return;
>  	}
>  
> +	/* Block writes to brightness/multi_intensity during teardown */
> +	drvdata->led_mc.led_cdev.brightness_set = NULL;
>  	cancel_delayed_work_sync(&drvdata->cfg_setup);
>  	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);

Can rgb_queue be re-queued after this point, leading to a Use-After-Free?

Since cancel_delayed_work_sync() is called before the custom sysfs
attributes are removed via device_remove_group() and before the devm-managed
LED class device is unregistered, a user could write to an RGB sysfs
attribute (like effect or speed) concurrently with or immediately after the
work is canceled. The sysfs callback executes mod_delayed_work() and
re-queues the delayed work. 

When claw_remove() finishes, devres frees the drvdata memory. Could the
re-queued work execute later on the freed memory?

>  
>  	guard(mutex)(&drvdata->cfg_mutex);
> +	if (drvdata->rgb_registered)
> +		device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
>  	if (drvdata->gamepad_registered)
>  		device_remove_group(&hdev->dev, &claw_gamepad_attr_group);

Can holding cfg_mutex across these device_remove_group() calls cause an
ABBA deadlock?

device_remove_group() blocks until all active sysfs operations for the
group finish. However, if a sysfs callback (like mkeys_function_store() for
the gamepad group) is executing concurrently, it will call
claw_hw_output_report() which attempts to acquire cfg_mutex.

If this occurs, the sysfs callback will block waiting for cfg_mutex, and
claw_remove() will block waiting for the sysfs callback to complete,
hanging device teardown.

[ ... ]

> @@ -982,6 +1544,7 @@ static int claw_suspend(struct hid_device *hdev)
>  
>  	cancel_delayed_work_sync(&drvdata->cfg_setup);
>  	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);

Can the rgb_queue work be incorrectly re-scheduled while the device is
suspended?

Since the LED class device and custom sysfs attributes remain active, if an
active LED trigger or a userspace sysfs write occurs during suspend, could
mod_delayed_work() re-queue the work and cause the driver to attempt USB
communication with a suspended device?

>  
>  	return 0;
>  }

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