On May 28, 2026 9:43:39 AM GMT+09:00, sashiko-bot@kernel.org wrote:
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 [off-list ref]
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.
quoted
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.
... What the heck.
This must have somehow dropped during rebase and I didn't notice.
I suppose I'll send a v11 tomorrow. Sorry for all the noise.
- Derek
quoted
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
[ ... ]
quoted
@@ -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?
quoted
+ bool rumble_support;
bool gp_registered;
bool bmap_support;