Re: [PATCH v4 2/4] HID: wiimote: replace spinlock pairs with scoped_guard
From: sashiko-bot@kernel.org
Date: 2026-08-17 21:59:11
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [Low] Functions improperly mix goto-based cleanup labels with scoped_guard usage. - [Low] Inconsistent space-based indentation utilized inside scoped_guard blocks. - [Low] The commit message inaccurately describes the formatting configuration of one-liner scoped_guards. Pre-existing issues: - [High] The `queue.worker` can be rescheduled after cancellation because hardware polling is stopped too late. --
--- Patch [2]: [PATCH v4 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---commit 7580c2db3c79e9b0eeae8d3525656cfd7d171c35
Author: Rafael Passos [off-list ref]
HID: wiimote: replace spinlock pairs with scoped_guard
This commit replaces manual spinlock lock/unlock pairs with scoped_guard
across the wiimote driver to improve safety and maintainability.
The multi line spinlock blocks were replaced by braced scoped_guard,
and one-liners by a scoped_guard without braces nor indentation.[Severity: Low] This isn't a bug, but there's a discrepancy in the commit message. The one-liners in the diff correctly retain their tab indentation for the guarded statement (e.g., in wiimote_cmd_read), contradicting the description here.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c index acf31d8b6991d..05f8ddb7909bc 100644 --- a/drivers/hid/hid-wiimote-core.c +++ b/drivers/hid/hid-wiimote-core.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -362,13 +363,12 @@ void wiiproto_req_rmem(struct wiimote_data *wdata, bool eeprom, __u32 offset, int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset, const __u8 *wmem, __u8 size) { - unsigned long flags; int ret; - spin_lock_irqsave(&wdata->state.lock, flags); - wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0); - wiiproto_req_wreg(wdata, offset, wmem, size); - spin_unlock_irqrestore(&wdata->state.lock, flags); + scoped_guard(spinlock_irqsave, &wdata->state.lock) { + wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0); + wiiproto_req_wreg(wdata, offset, wmem, size); + }
[Severity: Low] This isn't a bug, but does this inner block use spaces for indentation instead of standard tabs?
ret = wiimote_cmd_wait(wdata); if (!ret && wdata->state.cmd_err)
[ ... ]
quoted hunk ↗ jump to hunk
@@ -675,11 +673,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata, goto error; } - spin_lock_irq(&wdata->state.lock); - wdata->state.devtype = devtype; - spin_unlock_irq(&wdata->state.lock); - scoped_guard(spinlock_irqsave, &wdata->state.lock) { + wdata->state.devtype = devtype; /* after loading modules, set the Player ID LED cycling from 1 to 4*/ wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]); }@@ -703,13 +698,11 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)
[Severity: Low] Does this mix goto-based error handling (the goto error statements above) with cleanup helpers in the same function? The cleanup subsystem guidelines suggest that for a given routine, we should convert all resources that need a goto cleanup to scope-based cleanup, or convert none of them. This pattern also appears in wiimote_init_detect and wiimote_init_check. [ ... ]
quoted hunk ↗ jump to hunk
@@ -1120,16 +1106,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata) wiimote_cmd_acquire_noint(wdata); - spin_lock_irq(&wdata->state.lock); - - /* get state snapshot that we will then work on */ - flags = wdata->state.flags; + scoped_guard(spinlock_irq, &wdata->state.lock) { - /* disable event forwarding temporarily */ - wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE; - wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE; + /* get state snapshot that we will then work on */ + flags = wdata->state.flags; - spin_unlock_irq(&wdata->state.lock); + /* disable event forwarding temporarily */ + wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE; + wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE; + }
[Severity: Low] This isn't a bug, but it looks like spaces are used here instead of tabs for the indentation inside the scoped_guard block. [ ... ] [Severity: High] This is a pre-existing issue, but can the wdata->queue.worker be rescheduled after it is canceled during device teardown? Looking at wiimote_destroy (which is not modified in this patch but is in the same file): cancel_work_sync(&wdata->queue.worker); if (wdata->init_state & WIIMOTE_PROBE_HW_OPENED) hid_hw_close(wdata->hdev); if (wdata->init_state & WIIMOTE_PROBE_HW_STARTED) hid_hw_stop(wdata->hdev); kfree(wdata); If the hardware sends a report between the cancel_work_sync and hid_hw_stop calls, wiimote_hid_event could process it and queue a request via wiimote_queue, which calls schedule_work(&wdata->queue.worker). Since wdata is freed immediately after hid_hw_stop, would the rescheduled worker later access the freed wdata structure and cause a use-after-free? Should hid_hw_stop be called before canceling the worker? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260817213840.1053216-1-rafael@rcpassos.me?part=2