Re: [PATCH v10 4/4] HID: hid-msi: Add Rumble Intensity Attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Missing functional implementation for rumble intensity feature.

Pre-existing issues:
- [High] Race condition during device teardown leading to Use-After-Free of drvdata.
--

commit 902919289c1cf51c111590532df6640b717c3236
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add Rumble Intensity Attributes

Adds intensity adjustment for the left and right rumble motors. The patch 
introduces the data structures and driver variables for this feature.

>     Claude was used during the reverse-engineering data gathering for this
>     feature done by Zhouwang Huang. As the code had already been affected,
>     I used Claude to create the initial framing for the feature, then did
>     manual cleanup of the _show and _store functions afterwards to fix bugs
>     and keep the coding style consistent. Claude was also used as an initial
>     reviewer of this patch.

[Severity: Medium]
Are the _show and _store functions missing from this patch?

The commit message mentions manual cleanup of these functions, but the diff
only introduces the data structures (like struct claw_rumble_report and
rumble_addr) without any sysfs attributes or functional implementation.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 96710d80543d4..17b0707ecd46d 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -332,8 +343,12 @@ struct claw_drvdata {
>  	enum claw_gamepad_mode_index gamepad_mode;
>  	u8 m1_codes[CLAW_KEYS_MAX];
>  	u8 m2_codes[CLAW_KEYS_MAX];
> -	spinlock_t mode_lock; /* Lock for mode data read/write */
> +	u8 rumble_intensity_right;
> +	u8 rumble_intensity_left;
>  	const u16 *bmap_addr;
> +	spinlock_t rumble_lock; /* lock for rumble_intensity read/write */
> +	spinlock_t mode_lock; /* Lock for mode data read/write */

[Severity: High]
This is a pre-existing issue, but can the device teardown in claw_remove()
cause a use-after-free of this drvdata structure?

In claw_remove(), cancel_delayed_work_sync(&drvdata->rgb_queue) is called
before led_classdev_multicolor_unregister(&drvdata->led_mc).

If a concurrent sysfs write triggers claw_led_brightness_set(), it checks
rgb_registered under a lock but calls mod_delayed_work() after unlocking.

If preempted between these steps, claw_remove() could set the flag to false,
cancel the workqueue, and then block in led_classdev_multicolor_unregister()
waiting for the sysfs operation to finish:

CPU1                                CPU2
claw_led_brightness_set()
  scoped_guard(...) {
    if (!smp_load_acquire(...))
      return;
  }
  // preempted

                                    claw_remove()
                                      smp_store_release(..., false);
                                      cancel_delayed_work_sync(...);
                                      led_classdev_multicolor_unregister(...);
                                      // blocks waiting for CPU1

  mod_delayed_work(...);
  // finishes, CPU2 unblocks

                                      hid_hw_close(hdev);
                                      // drvdata is freed

When the sysfs thread resumes, it schedules the delayed work on the freed
drvdata.

Should the LED subsystem be unregistered before cancelling the associated
workqueues to flush active sysfs writes?

> +	bool rumble_support;
>  	bool gp_registered;
>  	bool bmap_support;
>

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