[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

[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

[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 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

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

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

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

[David Rheinsberg]

Hi

On Fri, Jul 10, 2026, at 9:35 PM, Rafael Passos wrote:
> 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

What device name are you referring to exactly?

>> 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

Ideally, you'd be able to call wiimote_destroy() on partially initialized devices. Not sure this is doable, but I would love to see it. I don't think using __free is worth it, if you retain the other goto labels in that function.

I mean, this is an old driver, lots of the cleanup-handlers were not around back then. But if I were to chose, I would either keep it as it is, or ensure that the destructors work with any partially initialized device just fine, and correctly checks that it only touches stuff that was initialized (or ensures the functions it calls do so).

Anyway, I like the series. I will review your v2 properly, so please go ahead!

Thanks
David