Re: [PATCH v4 2/4] HID: wiimote: replace spinlock pairs with scoped_guard

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 <[email protected]>

    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.

> 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
[ ... ]
> @@ -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)
[ ... ]
> @@ -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.

[ ... ]
> @@ -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/[email protected]?part=2