[PATCH 0/3] HID: wiimote: new LED behavior on connect, and scoped guards

[PATCH 0/3] HID: wiimote: new LED behavior on connect, and scoped guards

[Rafael Passos]

Hi,
This patchset contains one feature change, and two cleanup patches.

The feature is turning different LEDs for each of the first 4 wiimotes connected.
From id 5 forward, the LED will cycle back to 1, and so on.
This uses the ida struct, so its quite simple and lightweight.
The hid_info log message prints out the controller id.
I was considering adding this number to the controller name too,
but was unsure if this could impact applications using it.

While implementing this feature, I decided to cleanup the code using
scoped_guard for the many spinlocks in the driver. There are two places
where the original lock/unlock version fits best, and I left them
untouched.
Lastly, I used the __free scope cleanup in the LED probe function.
The wiimote_hid_probe call can also be changed in this way, but I
left it because it uses the wiimote_destroy in one of the err cases (w/ kfree inside).
But I can apply the change there too, to make this more complete.

It was really fun working with this driver.
I tested it with 4 Wii Motion Plus remotes (gen2).
Video recording of my tests (48s video).
https://rcpassos.me/video/wiimote-led-linux-driver

Thanks,
Rafael Passos

Rafael Passos (3):
  HID: wiimote: turn on the LEDs indicating the controller id
  HID: wiimote: replace spinlock pairs with scoped_guard
  HID: wiimote: use scoped cleanup in led creation

 drivers/hid/hid-wiimote-core.c    | 252 ++++++++++++++++--------------
 drivers/hid/hid-wiimote-debug.c   |  41 ++---
 drivers/hid/hid-wiimote-modules.c |  22 ++-
 drivers/hid/hid-wiimote.h         |   8 +
 4 files changed, 165 insertions(+), 158 deletions(-)

-- 
2.53.0

[PATCH 1/3] HID: wiimote: turn on the LEDs indicating the controller id

[Rafael Passos]

The behavior in a Wii/Wii U console is to have each controller turn on
a different LED indicating the controller id.
This commit implements the same behavior using the ida struct.
Unlike switch controllers, each ID only turns one LED (from 1 to 4).

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c | 46 +++++++++++++++++++++++++++++-----
 drivers/hid/hid-wiimote.h      |  8 ++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 63c4fa8fbb9b..0d36a38bdcda 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -671,6 +671,12 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 	spin_lock_irq(&wdata->state.lock);
 	wdata->state.devtype = devtype;
 	spin_unlock_irq(&wdata->state.lock);
+
+	/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wiiproto_req_leds(wdata, player_leds[(wdata->instance_id - 1) % 4]);
+	}
+
 	return;
 
 error:
@@ -855,11 +861,11 @@ static void wiimote_init_set_type(struct wiimote_data *wdata,
 
 done:
 	if (devtype == WIIMOTE_DEV_GENERIC)
-		hid_info(wdata->hdev, "cannot detect device; NAME: %s VID: %04x PID: %04x EXT: %04x\n",
-			name, vendor, product, exttype);
+		hid_info(wdata->hdev, "cannot detect device; NAME: %s VID: %04x PID: %04x EXT: %04x (%d)\n",
+			name, vendor, product, exttype, wdata->instance_id);
 	else
-		hid_info(wdata->hdev, "detected device: %s\n",
-			 wiimote_devtype_names[devtype]);
+		hid_info(wdata->hdev, "detected device: %s (%d)\n",
+			 wiimote_devtype_names[devtype], wdata->instance_id);
 
 	wiimote_modules_load(wdata, devtype);
 }
@@ -1786,11 +1792,15 @@ static void wiimote_destroy(struct wiimote_data *wdata)
 	kfree(wdata);
 }
 
+/* Global id allocator for wii remotes */
+static DEFINE_IDA(wiimote_ida);
+
 static int wiimote_hid_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
 	struct wiimote_data *wdata;
 	int ret;
+	int instance_id;
 
 	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
@@ -1834,7 +1844,15 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 	if (ret)
 		goto err_free;
 
-	hid_info(hdev, "New device registered\n");
+	instance_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
+	if (!instance_id) {
+		hid_err(hdev, "cannot allocate controller id, defaulting to 1\n");
+		instance_id = 1;
+	}
+
+	wdata->instance_id = instance_id;
+
+	hid_info(hdev, "New device registered (Wiimote %d)\n", instance_id);
 
 	/* schedule device detection */
 	wiimote_schedule(wdata);
@@ -1862,6 +1880,8 @@ static void wiimote_hid_remove(struct hid_device *hdev)
 {
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
 
+	ida_free(&wiimote_ida, wdata->instance_id);
+
 	hid_info(hdev, "Device removed\n");
 	wiimote_destroy(wdata);
 }
@@ -1887,7 +1907,21 @@ static struct hid_driver wiimote_hid_driver = {
 	.remove = wiimote_hid_remove,
 	.raw_event = wiimote_hid_event,
 };
-module_hid_driver(wiimote_hid_driver);
+
+
+static int __init wiimote_init(void)
+{
+	return hid_register_driver(&wiimote_hid_driver);
+}
+
+static void __exit wiimote_exit(void)
+{
+	hid_unregister_driver(&wiimote_hid_driver);
+	ida_destroy(&wiimote_ida);
+}
+
+module_init(wiimote_init);
+module_exit(wiimote_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index 9c12f63f6dd2..639f67c24f15 100644
--- a/drivers/hid/hid-wiimote.h
+++ b/drivers/hid/hid-wiimote.h
@@ -50,6 +50,13 @@
 #define WIIPROTO_FLAGS_IR (WIIPROTO_FLAG_IR_BASIC | WIIPROTO_FLAG_IR_EXT | \
 							WIIPROTO_FLAG_IR_FULL)
 
+static const __u8 player_leds[] = {
+        WIIPROTO_FLAG_LED1,
+        WIIPROTO_FLAG_LED2,
+        WIIPROTO_FLAG_LED3,
+        WIIPROTO_FLAG_LED4
+};
+
 /* return flag for led \num */
 #define WIIPROTO_FLAG_LED(num) (WIIPROTO_FLAG_LED1 << (num - 1))
 
@@ -153,6 +160,7 @@ struct wiimote_data {
 	struct input_dev *mp;
 	struct timer_list timer;
 	struct wiimote_debug *debug;
+	int instance_id;
 
 	union {
 		struct input_dev *input;
-- 
2.53.0

Re: [PATCH 1/3] HID: wiimote: turn on the LEDs indicating the controller id

[sashiko-bot]

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

New issues:
- [High] Incorrect error check for `ida_alloc_min()` fails to catch negative error codes, leading to an out-of-bounds array read on `player_leds`.
- [Medium] The fallback logic for IDA allocation failure incorrectly claims ID 1, leading to ID collisions and corruption of the IDA state upon removal.
- [Medium] Mixing of `goto`-based error handling and scope-based cleanup helpers (`scoped_guard`) in the same function.
- [Low] Defining a static array in a header file duplicates it across multiple translation units.

Pre-existing issues:
- [High] Missing `state.lock` when calling `wiimote_cmd_abort` in `wiimote_queue`.
--

--- Patch [1]: [PATCH 1/3] HID: wiimote: turn on the LEDs indicating the controller id ---
commit 48125f73deb74f22332d54c849af1f5b48042ab2
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: turn on the LEDs indicating the controller id

This commit introduces a new feature to assign an ID to each connected
Wiimote using an IDA struct. It also updates the module loading sequence
to cycle the LEDs from 1 to 4 to visually indicate the assigned player ID.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 63c4fa8fbb9b6..0d36a38bdcdad 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c

[Severity: High]
This isn't a bug introduced by this patch, but does wiimote_queue() call
wiimote_cmd_abort() while holding queue.lock instead of state.lock?

drivers/hid/hid-wiimote-core.c:wiimote_queue() {
...
out_error:
	wiimote_cmd_abort(wdata);
out_unlock:
	spin_unlock_irqrestore(&wdata->queue.lock, flags);
}

The documentation for wiimote_cmd_abort() specifies that it requires
state.lock:

drivers/hid/hid-wiimote.h:
/* requires the state.lock spinlock to be held */
static inline void wiimote_cmd_abort(struct wiimote_data *wdata)

Could this cause a data race with other command state accesses?

[ ... ]

> @@ -671,6 +671,12 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  	spin_lock_irq(&wdata->state.lock);
>  	wdata->state.devtype = devtype;
>  	spin_unlock_irq(&wdata->state.lock);
> +
> +	/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		wiiproto_req_leds(wdata, player_leds[(wdata->instance_id - 1) % 4]);
> +	}
> +
>  	return;
>  
>  error:

[Severity: Medium]
Is it acceptable to mix scope-based cleanup helpers like scoped_guard()
with goto-based error handling in the same function?

The cleanup documentation states that goto-based cleanup and scope-based
cleanup should not be mixed within the same routine.

[ ... ]

> @@ -1834,7 +1844,15 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_free;
>  
> -	hid_info(hdev, "New device registered\n");
> +	instance_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> +	if (!instance_id) {
> +		hid_err(hdev, "cannot allocate controller id, defaulting to 1\n");
> +		instance_id = 1;
> +	}

[Severity: High]
Since ida_alloc_min() returns a negative error code on failure rather than 0,
does this check fail to catch allocation errors?

If ida_alloc_min() fails and returns a negative error code, the later
calculation in wiimote_modules_load():

    player_leds[(wdata->instance_id - 1) % 4]

would result in a negative modulo and an out-of-bounds array read.

[Severity: Medium]
Additionally, if the IDA allocation fails and this fallback logic is reached,
does assigning instance_id = 1 without actually reserving it in the IDA
risk corrupting the state?

[ ... ]

> @@ -1862,6 +1880,8 @@ static void wiimote_hid_remove(struct hid_device *hdev)
>  {
>  	struct wiimote_data *wdata = hid_get_drvdata(hdev);
>  
> +	ida_free(&wiimote_ida, wdata->instance_id);
> +
>  	hid_info(hdev, "Device removed\n");
>  	wiimote_destroy(wdata);
>  }

[Severity: Medium]
If the fallback logic in wiimote_hid_probe() set instance_id to 1 without
reserving it, will this unconditional free corrupt the IDA state if another
device had legitimately claimed ID 1?

[ ... ]

> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index 9c12f63f6dd2d..639f67c24f15f 100644
> --- a/drivers/hid/hid-wiimote.h
> +++ b/drivers/hid/hid-wiimote.h
> @@ -50,6 +50,13 @@
>  #define WIIPROTO_FLAGS_IR (WIIPROTO_FLAG_IR_BASIC | WIIPROTO_FLAG_IR_EXT | \
>  							WIIPROTO_FLAG_IR_FULL)
>  
> +static const __u8 player_leds[] = {
> +        WIIPROTO_FLAG_LED1,
> +        WIIPROTO_FLAG_LED2,
> +        WIIPROTO_FLAG_LED3,
> +        WIIPROTO_FLAG_LED4
> +};
> +

[Severity: Low]
Does defining a static array in a header file cause it to be unnecessarily
duplicated in the .rodata section for every translation unit that includes
hid-wiimote.h?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710153456.2093889-1-rafael@rcpassos.me?part=1

[PATCH 2/3] HID: wiimote: replace spinlock pairs with scoped_guard

[Rafael Passos]

Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.

The multi line spinlock blocks were replaced by braced scoped_guard,
and one-liners by a scoped_guard without braces nor indentation.

There are two cases left in this driver using lock/unlock, because
guard would make the code more complex than current implementation.

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c    | 206 +++++++++++++-----------------
 drivers/hid/hid-wiimote-debug.c   |  41 +++---
 drivers/hid/hid-wiimote-modules.c |   4 +-
 3 files changed, 108 insertions(+), 143 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 0d36a38bdcda..b0403b750d6b 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -362,13 +362,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);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 	if (!ret && wdata->state.cmd_err)
@@ -381,21 +380,19 @@ int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset,
 ssize_t wiimote_cmd_read(struct wiimote_data *wdata, __u32 offset, __u8 *rmem,
 								__u8 size)
 {
-	unsigned long flags;
 	ssize_t ret;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.cmd_read_size = size;
-	wdata->state.cmd_read_buf = rmem;
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, offset & 0xffff);
-	wiiproto_req_rreg(wdata, offset, size);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.cmd_read_size = size;
+		wdata->state.cmd_read_buf = rmem;
+		wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, offset & 0xffff);
+		wiiproto_req_rreg(wdata, offset, size);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->state.cmd_read_buf = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 
 	if (!ret) {
 		if (wdata->state.cmd_read_size == 0)
@@ -696,13 +693,11 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)
 {
 	const __u8 *mods, *iter;
 	const struct wiimod_ops *ops;
-	unsigned long flags;
 
 	mods = wiimote_devtype_mods[wdata->state.devtype];
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 
 	/* find end of list */
 	for (iter = mods; *iter != WIIMOD_NULL; ++iter)
@@ -729,7 +724,6 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)
 
 static void wiimote_ext_load(struct wiimote_data *wdata, unsigned int ext)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 	int ret;
 
@@ -741,22 +735,20 @@ static void wiimote_ext_load(struct wiimote_data *wdata, unsigned int ext)
 			ext = WIIMOTE_EXT_UNKNOWN;
 	}
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->state.exttype = ext;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 }
 
 static void wiimote_ext_unload(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 
 	ops = wiimod_ext_table[wdata->state.exttype];
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.exttype = WIIMOTE_EXT_UNKNOWN;
-	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_USED;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	    wdata->state.exttype = WIIMOTE_EXT_UNKNOWN;
+	    wdata->state.flags &= ~WIIPROTO_FLAG_EXT_USED;
+	}
 
 	if (ops->remove)
 		ops->remove(ops, wdata);
@@ -764,7 +756,6 @@ static void wiimote_ext_unload(struct wiimote_data *wdata)
 
 static void wiimote_mp_load(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 	int ret;
 	__u8 mode = 2;
@@ -776,14 +767,12 @@ static void wiimote_mp_load(struct wiimote_data *wdata)
 			mode = 1;
 	}
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->state.mp = mode;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 }
 
 static void wiimote_mp_unload(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 
 	if (wdata->state.mp < 2)
@@ -791,10 +780,10 @@ static void wiimote_mp_unload(struct wiimote_data *wdata)
 
 	ops = &wiimod_mp;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.mp = 0;
-	wdata->state.flags &= ~WIIPROTO_FLAG_MP_USED;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	    wdata->state.mp = 0;
+	    wdata->state.flags &= ~WIIPROTO_FLAG_MP_USED;
+	}
 
 	if (ops->remove)
 		ops->remove(ops, wdata);
@@ -878,19 +867,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
 
 	wiimote_cmd_acquire_noint(wdata);
 
-	spin_lock_irq(&wdata->state.lock);
-	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
-	wiiproto_req_status(wdata);
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
+		wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
+		wiiproto_req_status(wdata);
+	}
+
 
 	ret = wiimote_cmd_wait_noint(wdata);
 	if (ret)
 		goto out_release;
 
-	spin_lock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock)
 	ext = wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED;
-	spin_unlock_irq(&wdata->state.lock);
 
 	if (!ext)
 		goto out_release;
@@ -903,11 +892,11 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
 	wiimote_init_set_type(wdata, exttype);
 
 	/* schedule MP timer */
-	spin_lock_irq(&wdata->state.lock);
-	if (!(wdata->state.flags & WIIPROTO_FLAG_BUILTIN_MP) &&
-	    !(wdata->state.flags & WIIPROTO_FLAG_NO_MP))
-		mod_timer(&wdata->timer, jiffies + HZ * 4);
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		if (!(wdata->state.flags & WIIPROTO_FLAG_BUILTIN_MP) &&
+		!(wdata->state.flags & WIIPROTO_FLAG_NO_MP))
+			mod_timer(&wdata->timer, jiffies + HZ * 4);
+	}
 }
 
 /*
@@ -955,9 +944,8 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 	__u8 type, data[6];
 	bool ret, poll_mp;
 
-	spin_lock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock)
 	flags = wdata->state.flags;
-	spin_unlock_irq(&wdata->state.lock);
 
 	wiimote_cmd_acquire_noint(wdata);
 
@@ -973,11 +961,11 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		type = wiimote_cmd_read_mp_mapped(wdata);
 		ret = type == WIIMOTE_MP_SINGLE;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_PLUGGED);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+		    ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+		    ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_PLUGGED);
+		    ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: !EXT && MP\n");
@@ -998,10 +986,10 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		type = wiimote_cmd_read_ext(wdata, data);
 		ret = type == wdata->state.exttype;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+		    ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+		    ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: EXT && !MP\n");
@@ -1024,11 +1012,11 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		type = wiimote_cmd_read_ext(wdata, data);
 		ret = type == wdata->state.exttype;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+		    ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+		    ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+		    ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: !EXT && !MP\n");
@@ -1054,11 +1042,11 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		ret = ret && type != WIIMOTE_MP_UNKNOWN;
 		ret = ret && type != WIIMOTE_MP_SINGLE;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+		    ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
+		    ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+		    ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: EXT && MP\n");
@@ -1113,16 +1101,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;
+	}
 
 	/* init extension and MP (deactivates current extension or MP) */
 	wiimote_cmd_init_ext(wdata);
@@ -1145,9 +1132,8 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 			hid_info(wdata->hdev, "cannot detect extension; %6phC\n",
 				 extdata);
 		} else if (exttype == WIIMOTE_EXT_NONE) {
-			spin_lock_irq(&wdata->state.lock);
+			scoped_guard(spinlock_irq, &wdata->state.lock)
 			wdata->state.exttype = WIIMOTE_EXT_NONE;
-			spin_unlock_irq(&wdata->state.lock);
 		} else {
 			hid_info(wdata->hdev, "detected extension: %s\n",
 				 wiimote_exttype_names[exttype]);
@@ -1185,27 +1171,25 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 			mod_timer(&wdata->timer, jiffies + HZ * 4);
 	}
 
-	spin_lock_irq(&wdata->state.lock);
-
-	/* enable data forwarding again and set expected hotplug state */
-	if (mp) {
-		wdata->state.flags |= WIIPROTO_FLAG_MP_ACTIVE;
-		if (wdata->state.exttype == WIIMOTE_EXT_NONE) {
-			wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
-			wdata->state.flags &= ~WIIPROTO_FLAG_MP_PLUGGED;
-		} else {
-			wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
-			wdata->state.flags |= WIIPROTO_FLAG_MP_PLUGGED;
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		/* enable data forwarding again and set expected hotplug state */
+		if (mp) {
+			wdata->state.flags |= WIIPROTO_FLAG_MP_ACTIVE;
+			if (wdata->state.exttype == WIIMOTE_EXT_NONE) {
+				wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
+				wdata->state.flags &= ~WIIPROTO_FLAG_MP_PLUGGED;
+			} else {
+				wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
+				wdata->state.flags |= WIIPROTO_FLAG_MP_PLUGGED;
+				wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
+			}
+		} else if (wdata->state.exttype != WIIMOTE_EXT_NONE) {
 			wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
 		}
-	} else if (wdata->state.exttype != WIIMOTE_EXT_NONE) {
-		wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
-	}
 
-	/* request status report for hotplug state updates */
-	wiiproto_req_status(wdata);
-
-	spin_unlock_irq(&wdata->state.lock);
+		/* request status report for hotplug state updates */
+		wiiproto_req_status(wdata);
+	}
 
 	hid_dbg(wdata->hdev, "detected extensions: MP: %d EXT: %d\n",
 		wdata->state.mp, wdata->state.exttype);
@@ -1237,11 +1221,8 @@ void __wiimote_schedule(struct wiimote_data *wdata)
 
 static void wiimote_schedule(struct wiimote_data *wdata)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	__wiimote_schedule(wdata);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 }
 
 static void wiimote_init_timeout(struct timer_list *t)
@@ -1631,7 +1612,6 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
 	const struct wiiproto_handler *h;
 	int i;
-	unsigned long flags;
 
 	if (size < 1)
 		return -EINVAL;
@@ -1639,9 +1619,9 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	for (i = 0; handlers[i].id; ++i) {
 		h = &handlers[i];
 		if (h->id == raw_data[0] && h->size < size) {
-			spin_lock_irqsave(&wdata->state.lock, flags);
-			h->func(wdata, &raw_data[1]);
-			spin_unlock_irqrestore(&wdata->state.lock, flags);
+			scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+			    h->func(wdata, &raw_data[1]);
+			}
 			break;
 		}
 	}
@@ -1659,11 +1639,10 @@ static ssize_t wiimote_ext_show(struct device *dev,
 {
 	struct wiimote_data *wdata = dev_to_wii(dev);
 	__u8 type;
-	unsigned long flags;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	type = wdata->state.exttype;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	    type = wdata->state.exttype;
+	}
 
 	switch (type) {
 	case WIIMOTE_EXT_NONE:
@@ -1712,11 +1691,10 @@ static ssize_t wiimote_dev_show(struct device *dev,
 {
 	struct wiimote_data *wdata = dev_to_wii(dev);
 	__u8 type;
-	unsigned long flags;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	type = wdata->state.devtype;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	    type = wdata->state.devtype;
+	}
 
 	switch (type) {
 	case WIIMOTE_DEV_GENERIC:
@@ -1767,14 +1745,12 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 
 static void wiimote_destroy(struct wiimote_data *wdata)
 {
-	unsigned long flags;
-
 	wiidebug_deinit(wdata);
 
 	/* prevent init_worker from being scheduled again */
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	    wdata->state.flags |= WIIPROTO_FLAG_EXITING;
+	}
 
 	cancel_work_sync(&wdata->init_worker);
 	timer_shutdown_sync(&wdata->timer);
diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
index 5f74917781f2..b174fa030c71 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -25,7 +25,6 @@ static ssize_t wiidebug_eeprom_read(struct file *f, char __user *u, size_t s,
 {
 	struct wiimote_debug *dbg = f->private_data;
 	struct wiimote_data *wdata = dbg->wdata;
-	unsigned long flags;
 	ssize_t ret;
 	char buf[16];
 	__u16 size = 0;
@@ -41,20 +40,19 @@ static ssize_t wiidebug_eeprom_read(struct file *f, char __user *u, size_t s,
 	if (ret)
 		return ret;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.cmd_read_size = s;
-	wdata->state.cmd_read_buf = buf;
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, *off & 0xffff);
-	wiiproto_req_reeprom(wdata, *off, s);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.cmd_read_size = s;
+		wdata->state.cmd_read_buf = buf;
+		wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, *off & 0xffff);
+		wiiproto_req_reeprom(wdata, *off, s);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 	if (!ret)
 		size = wdata->state.cmd_read_size;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->state.cmd_read_buf = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 
 	wiimote_cmd_release(wdata);
 
@@ -99,12 +97,10 @@ static int wiidebug_drm_show(struct seq_file *f, void *p)
 {
 	struct wiimote_debug *dbg = f->private;
 	const char *str = NULL;
-	unsigned long flags;
 	__u8 drm;
 
-	spin_lock_irqsave(&dbg->wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &dbg->wdata->state.lock)
 	drm = dbg->wdata->state.drm;
-	spin_unlock_irqrestore(&dbg->wdata->state.lock, flags);
 
 	if (drm < WIIPROTO_REQ_MAX)
 		str = wiidebug_drmmap[drm];
@@ -126,7 +122,6 @@ static ssize_t wiidebug_drm_write(struct file *f, const char __user *u,
 {
 	struct seq_file *sf = f->private_data;
 	struct wiimote_debug *dbg = sf->private;
-	unsigned long flags;
 	char buf[16];
 	ssize_t len;
 	int i;
@@ -150,12 +145,12 @@ static ssize_t wiidebug_drm_write(struct file *f, const char __user *u,
 	if (i == WIIPROTO_REQ_MAX)
 		i = simple_strtoul(buf, NULL, 16);
 
-	spin_lock_irqsave(&dbg->wdata->state.lock, flags);
-	dbg->wdata->state.flags &= ~WIIPROTO_FLAG_DRM_LOCKED;
-	wiiproto_req_drm(dbg->wdata, (__u8) i);
-	if (i != WIIPROTO_REQ_NULL)
-		dbg->wdata->state.flags |= WIIPROTO_FLAG_DRM_LOCKED;
-	spin_unlock_irqrestore(&dbg->wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &dbg->wdata->state.lock) {
+		dbg->wdata->state.flags &= ~WIIPROTO_FLAG_DRM_LOCKED;
+		wiiproto_req_drm(dbg->wdata, (__u8) i);
+		if (i != WIIPROTO_REQ_NULL)
+			dbg->wdata->state.flags |= WIIPROTO_FLAG_DRM_LOCKED;
+	}
 
 	return len;
 }
@@ -172,7 +167,6 @@ static const struct file_operations wiidebug_drm_fops = {
 int wiidebug_init(struct wiimote_data *wdata)
 {
 	struct wiimote_debug *dbg;
-	unsigned long flags;
 
 	dbg = kzalloc_obj(*dbg);
 	if (!dbg)
@@ -186,9 +180,8 @@ int wiidebug_init(struct wiimote_data *wdata)
 	dbg->drm = debugfs_create_file("drm", S_IRUSR,
 			dbg->wdata->hdev->debug_dir, dbg, &wiidebug_drm_fops);
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->debug = dbg;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 
 	return 0;
 
@@ -197,14 +190,12 @@ int wiidebug_init(struct wiimote_data *wdata)
 void wiidebug_deinit(struct wiimote_data *wdata)
 {
 	struct wiimote_debug *dbg = wdata->debug;
-	unsigned long flags;
 
 	if (!dbg)
 		return;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 	wdata->debug = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
 
 	debugfs_remove(dbg->drm);
 	debugfs_remove(dbg->eeprom);
diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index dccb78bb3afd..f42f9aa7b2d4 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -341,7 +341,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 	struct device *dev = &wdata->hdev->dev;
 	size_t namesz = strlen(dev_name(dev)) + 9;
 	struct led_classdev *led;
-	unsigned long flags;
 	char *name;
 	int ret;
 
@@ -364,9 +363,8 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 
 	/* enable LED1 to stop initial LED-blinking */
 	if (ops->arg == 0) {
-		spin_lock_irqsave(&wdata->state.lock, flags);
+		scoped_guard(spinlock_irqsave, &wdata->state.lock)
 		wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
-		spin_unlock_irqrestore(&wdata->state.lock, flags);
 	}
 
 	return 0;
-- 
2.53.0

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

[sashiko-bot]

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

New issues:
- [Low] Functions wiimote_init_detect() and wiimote_init_check() mix goto-based cleanup labels with cleanup.h helpers (scoped_guard).

Pre-existing issues:
- [High] Race condition during device teardown allows `queue.worker` to be re-scheduled after cancellation, leading to a Use-After-Free on `wdata`.
--

--- Patch [2]: [PATCH 2/3] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit b72167f4bfaf6c447e5bff82a564df82f7c2a54d
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: replace spinlock pairs with scoped_guard

Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -878,19 +867,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> -	wiiproto_req_status(wdata);
> -	spin_unlock_irq(&wdata->state.lock);
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
> +		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> +		wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> +		wiiproto_req_status(wdata);
> +	}
> +
>  
>  	ret = wiimote_cmd_wait_noint(wdata);
>  	if (ret)
>  		goto out_release;

[Severity: Low]
Is it acceptable to mix goto-based error handling with scoped_guard() in the
same function?

According to the cleanup.h documentation, the expectation is that a function
either uses goto-based cleanup or scope-based cleanup exclusively. Since
this patch introduces scoped_guard() while leaving goto out_release, does
this violate the subsystem guidelines for mixing cleanup styles? This same
pattern also appears further down in wiimote_init_check().

[ ... ]

> @@ -1767,14 +1745,12 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>  
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
> -	unsigned long flags;
> -
>  	wiidebug_deinit(wdata);
>  
>  	/* prevent init_worker from being scheduled again */
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +	    wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> +	}
>  
>  	cancel_work_sync(&wdata->init_worker);
>  	timer_shutdown_sync(&wdata->timer);
[ ... ]
> 	wiimote_mp_unload(wdata);
> 	wiimote_ext_unload(wdata);
> 	wiimote_modules_unload(wdata);
> 	cancel_work_sync(&wdata->queue.worker);
> 	hid_hw_close(wdata->hdev);
> 	hid_hw_stop(wdata->hdev);
> 
> 	kfree(wdata);
>  }

[Severity: High]
This is a pre-existing issue, but could this teardown sequence result in a
use-after-free on wdata?

Currently, cancel_work_sync(&wdata->queue.worker) is called before
hid_hw_stop(wdata->hdev). If an incoming status report arrives after the work
is canceled but before the HID device is stopped, wiimote_hid_event() could
handle it and call handler_status_K() -> wiiproto_req_drm() ->
wiimote_queue(), which unconditionally re-arms the work queue via
schedule_work(&wdata->queue.worker).

If the queue worker executes after wdata is freed by kfree(wdata) here,
would it dereference freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710153456.2093889-1-rafael@rcpassos.me?part=2

[PATCH 3/3] HID: wiimote: use scoped cleanup in led creation

[Rafael Passos]

Cleanup code in led probe function, using the modern scoped cleanup.
This prevents mistakes in future changes to this function.

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-modules.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index f42f9aa7b2d4..7c305099c82d 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -340,11 +340,11 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 {
 	struct device *dev = &wdata->hdev->dev;
 	size_t namesz = strlen(dev_name(dev)) + 9;
-	struct led_classdev *led;
 	char *name;
 	int ret;
 
-	led = kzalloc(sizeof(struct led_classdev) + namesz, GFP_KERNEL);
+	struct led_classdev *led __free(kfree) =
+		kzalloc(sizeof(struct led_classdev) + namesz, GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;
 
@@ -358,8 +358,13 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 
 	wdata->leds[ops->arg] = led;
 	ret = led_classdev_register(dev, led);
-	if (ret)
-		goto err_free;
+	if (ret) {
+		wdata->leds[ops->arg] = NULL;
+		return ret;
+	}
+
+	/* Ownership transferred to wdata – prevent cleanup from freeing */
+	led = NULL;
 
 	/* enable LED1 to stop initial LED-blinking */
 	if (ops->arg == 0) {
@@ -368,11 +373,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 	}
 
 	return 0;
-
-err_free:
-	wdata->leds[ops->arg] = NULL;
-	kfree(led);
-	return ret;
 }
 
 static void wiimod_led_remove(const struct wiimod_ops *ops,
-- 
2.53.0

Re: [PATCH 3/3] HID: wiimote: use scoped cleanup in led creation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The check `if (!instance_id)` after `ida_alloc_min()` incorrectly assumes 0 on failure, missing negative error codes and leading to an out-of-bounds array access and kernel BUG.
--

--- Patch [3]: [PATCH 3/3] HID: wiimote: use scoped cleanup in led creation ---
commit 2a6971d40f61933b6a7890df0931f14066de90e7
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: use scoped cleanup in led creation

This commit updates the wiimod_led_probe function to use modern scoped
cleanup for led allocation. It ensures the led structure is safely freed
on error paths while transferring ownership appropriately on success.

> diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
> index f42f9aa7b2d49..7c305099c82d6 100644
> --- a/drivers/hid/hid-wiimote-modules.c
> +++ b/drivers/hid/hid-wiimote-modules.c
> @@ -340,11 +340,11 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
>  {
>  	struct device *dev = &wdata->hdev->dev;
>  	size_t namesz = strlen(dev_name(dev)) + 9;

[ ... ]

[Severity: High]
Does this driver handle negative error codes from ida_alloc_min() correctly?

Looking at wiimote_hid_probe(), the return value is checked with a boolean
negation:

drivers/hid/hid-wiimote-core.c:wiimote_hid_probe() {
    ...
	instance_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
	if (!instance_id) {
		hid_err(hdev, "cannot allocate controller id, defaulting to 1\n");
		instance_id = 1;
	}
    ...
}

If ida_alloc_min() fails due to memory pressure or IDA exhaustion, it
returns a negative error code like -ENOMEM. The check if (!instance_id)
evaluates to false, bypassing the fallback logic and allowing the
negative error code to be stored in wdata->instance_id.

This negative value is later used in an array index calculation:

drivers/hid/hid-wiimote-core.c:wiimote_modules_load() {
    ...
	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
		wiiproto_req_leds(wdata, player_leds[(wdata->instance_id - 1) % 4]);
	}
    ...
}

Can this cause an out-of-bounds array read, since the modulo operator
in C retains the sign of the dividend?

Additionally, when the device is removed or rolled back, this negative ID
is freed:

drivers/hid/hid-wiimote-core.c:wiimote_hid_remove() {
    ...
	ida_free(&wiimote_ida, wdata->instance_id);
    ...
}

Could passing the negative ID to ida_free() explicitly trigger a BUG_ON
and crash the kernel during teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710153456.2093889-1-rafael@rcpassos.me?part=3

Re: [PATCH 0/3] HID: wiimote: new LED behavior on connect, and scoped guards

[Rafael Passos]

I checked sashiko reviews, and there are a few needed fixes.
I will delay sending the v2 by a few days, in case there are any other reviews.

Specially regarding these two questions:

On Fri Jul 10, 2026 at 12:32 PM -03, Rafael Passos wrote:
> [...]
> The hid_info log message prints out the controller id.
> I was considering adding this number to the controller name too,
> but was unsure if this could impact applications using it.
- device naming

> The wiimote_hid_probe call can also be changed in this way, but I
> left it because it uses the wiimote_destroy in one of the err cases (w/ kfree inside).
> But I can apply the change there too, to make this more complete.
- wiimote_hid_probe scope cleanup

Thanks,
Rafael