[PATCH v5 0/3] HID: wiimote: new LED behavior on connect + scoped_guards

[PATCH v5 0/3] HID: wiimote: new LED behavior on connect + scoped_guards

[Rafael Passos]

Hi,
This patchset contains one feature change, and 3 patches appying 
scoped cleanup to locking and to the initialization functions for
the led probing and the main wiimote probe call.

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.

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.
I also used the __free scope cleanup in the wiimote_probe and LED probe.

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

---
V1: https://lore.kernel.org/linux-input/20260710153456.2093889-1-rafael@rcpassos.me/
Changes from v1:
    (1/3):
    - fix ida_alloc_min error handling to consider negative values
    - remove fallback to 1 on ida_alloc_min failure
    - move player_leds static array to hid-wiimote-core.c
    - s/instance_id/player_id/g
    - store player_id on an u8
    (2/3):
    - add header include for cleanup.h
    - add identation to one-liner scoped_guards
    (3/3):
    - add scoped cleanup function to wiimote_probe, with a bitmask to track state
      Patch used for testing this:
      https://lore.kernel.org/linux-input/20260715213513.3929001-1-rafael@rcpassos.me/
    (4/4) *new patch* :
    - sashiko found a pre-existing uaf. Unlikely, but correct.
      implemented using the playstation driver as an inspiration

V2: https://lore.kernel.org/linux-input/20260710153456.2093889-1-rafael@rcpassos.me/
Changes from v2:
    (2/4):
    - join the last two locks into a single scoped_guard lock in wiimote_modules_load

V3: https://lore.rcpassos.me/wiimote/20260729164928.1138468-1-rafael@rcpassos.me/
Changes from v3:
    - dropped false uaf patch (previous 4/4). It was a false alarm.
      Discussion in:
      https://lore.kernel.org/linux-input/20260729164928.1138468-1-rafael@rcpassos.me/T/#t

    (1/4):
    - I tested how the changes looked if using ida from 0 instead of 1,
      and I believe it ended up less clean. I decided to keep them as is,
      with a few additional comments.
    - explicit initialization of player_id to 0 before allocating an id
    - avoid using a new int during ida_alloc (use ret instead)
    - use u8 instead of __u8
    - move ida_remove from wiimote_hid_remove to wiimote_destroy
    - add debugfs entry for the player_id entry
    (3/4):
    - move changes to wiimote_probe from this patch to the next
    - update patch title
    (4/4): *new patch*
    - scoped cleanup in the wiimote_probe call, using a bitmask to keep
      track of state during initialization
    - update wiimote_destroy so it can be the cleanup function
    - add a debugfs entry for this new entry

V4: https://lore.kernel.org/linux-input/20260817213840.1053216-1-rafael@rcpassos.me/
Changes from v4:
    - dropped wiimote probe with scoped guards (previous 4/4). It was too complex.
      Maybe we can still find a simpler way, outside of this patchset.
    
    (1/3):
    - drop previous wiiproto_req_leds call from wiimod_led_probe
      (covered by the new one in wiimote_modules_load)
    - On the call to wiiproto_req_leds in wiimote_modules_load:
        - Use U1 and to avoid integer promotion rules.
        - Use ARRAY_SIZE(player_leds) to avoid the hard-coded size
        - move call into spin-lock above
    - add guard to ida_free (was places in thw wrong patch)
    - replace ida_alloc_min by ida_alloc_range
    - change hid_info "registered" log to use the wdata->player_id value
    (2/3):
    - fix some whitespace/tab indentation mixup
    (3/3):
    - split declaration and assignment in the kzalloc

Thanks!


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

 drivers/hid/hid-wiimote-core.c    | 277 ++++++++++++++++--------------
 drivers/hid/hid-wiimote-debug.c   |  67 ++++----
 drivers/hid/hid-wiimote-modules.c |  22 +--
 drivers/hid/hid-wiimote.h         |   1 +
 4 files changed, 183 insertions(+), 184 deletions(-)

-- 
2.55.0

[PATCH v5 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>
Reviewed-by: David Rheinsberg <david@readahead.eu>
---
 drivers/hid/hid-wiimote-core.c    | 50 +++++++++++++++++++++++++++----
 drivers/hid/hid-wiimote-debug.c   |  5 +++-
 drivers/hid/hid-wiimote-modules.c |  7 -----
 drivers/hid/hid-wiimote.h         |  1 +
 4 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 63c4fa8fbb9b6..d86ddcdb240ba 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -621,6 +621,13 @@ static const __u8 * const wiimote_devtype_mods[WIIMOTE_DEV_NUM] = {
 	},
 };
 
+static const __u8 player_leds[] = {
+	WIIPROTO_FLAG_LED1,
+	WIIPROTO_FLAG_LED2,
+	WIIPROTO_FLAG_LED3,
+	WIIPROTO_FLAG_LED4
+};
+
 static void wiimote_modules_load(struct wiimote_data *wdata,
 				 unsigned int devtype)
 {
@@ -670,7 +677,10 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 
 	spin_lock_irq(&wdata->state.lock);
 	wdata->state.devtype = devtype;
+	/* after loading modules, set the Player ID LED cycling the available LEDs*/
+	wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1U) % ARRAY_SIZE(player_leds)]);
 	spin_unlock_irq(&wdata->state.lock);
+
 	return;
 
 error:
@@ -855,11 +865,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->player_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->player_id);
 
 	wiimote_modules_load(wdata, devtype);
 }
@@ -1752,6 +1762,7 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 	mutex_init(&wdata->state.sync);
 	wdata->state.drm = WIIPROTO_REQ_DRM_K;
 	wdata->state.cmd_battery = 0xff;
+	wdata->player_id = 0; // min 1, u8 0 is unassigned id
 
 	INIT_WORK(&wdata->init_worker, wiimote_init_worker);
 	timer_setup(&wdata->timer, wiimote_init_timeout, 0);
@@ -1759,12 +1770,18 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 	return wdata;
 }
 
+/* Global id allocator for wii remotes */
+static DEFINE_IDA(wiimote_ida);
+
 static void wiimote_destroy(struct wiimote_data *wdata)
 {
 	unsigned long flags;
 
 	wiidebug_deinit(wdata);
 
+	if (wdata->player_id > 0)
+		ida_free(&wiimote_ida, wdata->player_id);
+
 	/* prevent init_worker from being scheduled again */
 	spin_lock_irqsave(&wdata->state.lock, flags);
 	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
@@ -1834,7 +1851,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 	if (ret)
 		goto err_free;
 
-	hid_info(hdev, "New device registered\n");
+	ret = ida_alloc_range(&wiimote_ida, 1, 255, GFP_KERNEL);
+	if (ret < 1) {
+		hid_err(hdev, "cannot allocate controller id\n");
+		goto err_free;
+	}
+
+	wdata->player_id = ret;
+	hid_info(hdev, "New device registered (Wiimote %d)\n", wdata->player_id);
 
 	/* schedule device detection */
 	wiimote_schedule(wdata);
@@ -1887,7 +1911,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-debug.c b/drivers/hid/hid-wiimote-debug.c
index 5f74917781f28..fc847c2a1c1f1 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -186,12 +186,14 @@ int wiidebug_init(struct wiimote_data *wdata)
 	dbg->drm = debugfs_create_file("drm", S_IRUSR,
 			dbg->wdata->hdev->debug_dir, dbg, &wiidebug_drm_fops);
 
+	debugfs_create_u8("player_id", S_IRUSR,
+		   dbg->wdata->hdev->debug_dir, &wdata->player_id);
+
 	spin_lock_irqsave(&wdata->state.lock, flags);
 	wdata->debug = dbg;
 	spin_unlock_irqrestore(&wdata->state.lock, flags);
 
 	return 0;
-
 }
 
 void wiidebug_deinit(struct wiimote_data *wdata)
@@ -208,5 +210,6 @@ void wiidebug_deinit(struct wiimote_data *wdata)
 
 	debugfs_remove(dbg->drm);
 	debugfs_remove(dbg->eeprom);
+	debugfs_lookup_and_remove("player_id", dbg->wdata->hdev->debug_dir);
 	kfree(dbg);
 }
diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index dccb78bb3afd6..a3d3fda9703dc 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -362,13 +362,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 	if (ret)
 		goto err_free;
 
-	/* enable LED1 to stop initial LED-blinking */
-	if (ops->arg == 0) {
-		spin_lock_irqsave(&wdata->state.lock, flags);
-		wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
-		spin_unlock_irqrestore(&wdata->state.lock, flags);
-	}
-
 	return 0;
 
 err_free:
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index 9c12f63f6dd2d..8e5002f515e24 100644
--- a/drivers/hid/hid-wiimote.h
+++ b/drivers/hid/hid-wiimote.h
@@ -153,6 +153,7 @@ struct wiimote_data {
 	struct input_dev *mp;
 	struct timer_list timer;
 	struct wiimote_debug *debug;
+	u8 player_id;
 
 	union {
 		struct input_dev *input;
-- 
2.55.0

[PATCH v5 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>
Reviewed-by: David Rheinsberg <david@readahead.eu>
---
 drivers/hid/hid-wiimote-core.c    | 231 +++++++++++++-----------------
 drivers/hid/hid-wiimote-debug.c   |  62 ++++----
 drivers/hid/hid-wiimote-modules.c |   2 +-
 3 files changed, 130 insertions(+), 165 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index d86ddcdb240ba..9168b85029183 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -7,6 +7,7 @@
 /*
  */
 
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/hid.h>
@@ -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);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 	if (!ret && wdata->state.cmd_err)
@@ -381,21 +381,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);
-	wdata->state.cmd_read_buf = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.cmd_read_buf = NULL;
 
 	if (!ret) {
 		if (wdata->state.cmd_read_size == 0)
@@ -675,11 +673,12 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 			goto error;
 	}
 
-	spin_lock_irq(&wdata->state.lock);
-	wdata->state.devtype = devtype;
-	/* after loading modules, set the Player ID LED cycling the available LEDs*/
-	wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1U) % ARRAY_SIZE(player_leds)]);
-	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 the available LEDs*/
+		wiiproto_req_leds(wdata,
+		    player_leds[(wdata->player_id - 1U) % ARRAY_SIZE(player_leds)]);
+	}
 
 	return;
 
@@ -700,13 +699,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);
-	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
 
 	/* find end of list */
 	for (iter = mods; *iter != WIIMOD_NULL; ++iter)
@@ -733,7 +730,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;
 
@@ -745,22 +741,20 @@ static void wiimote_ext_load(struct wiimote_data *wdata, unsigned int ext)
 			ext = WIIMOTE_EXT_UNKNOWN;
 	}
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.exttype = ext;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.exttype = ext;
 }
 
 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);
@@ -768,7 +762,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;
@@ -780,14 +773,12 @@ static void wiimote_mp_load(struct wiimote_data *wdata)
 			mode = 1;
 	}
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.mp = mode;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.mp = mode;
 }
 
 static void wiimote_mp_unload(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 
 	if (wdata->state.mp < 2)
@@ -795,10 +786,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);
@@ -882,19 +873,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);
-	ext = wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED;
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock)
+		ext = wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED;
 
 	if (!ext)
 		goto out_release;
@@ -907,11 +898,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);
+	}
 }
 
 /*
@@ -959,9 +950,8 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 	__u8 type, data[6];
 	bool ret, poll_mp;
 
-	spin_lock_irq(&wdata->state.lock);
-	flags = wdata->state.flags;
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock)
+		flags = wdata->state.flags;
 
 	wiimote_cmd_acquire_noint(wdata);
 
@@ -977,11 +967,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");
@@ -1002,10 +992,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");
@@ -1028,11 +1018,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");
@@ -1058,11 +1048,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");
@@ -1117,16 +1107,14 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 
 	wiimote_cmd_acquire_noint(wdata);
 
-	spin_lock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		/* get state snapshot that we will then work on */
+		flags = wdata->state.flags;
 
-	/* get state snapshot that we will then work on */
-	flags = wdata->state.flags;
-
-	/* disable event forwarding temporarily */
-	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
-	wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
-
-	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);
@@ -1149,9 +1137,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);
-			wdata->state.exttype = WIIMOTE_EXT_NONE;
-			spin_unlock_irq(&wdata->state.lock);
+			scoped_guard(spinlock_irq, &wdata->state.lock)
+				wdata->state.exttype = WIIMOTE_EXT_NONE;
 		} else {
 			hid_info(wdata->hdev, "detected extension: %s\n",
 				 wiimote_exttype_names[exttype]);
@@ -1189,28 +1176,26 @@ 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);
 	}
 
-	/* request status report for hotplug state updates */
-	wiiproto_req_status(wdata);
-
-	spin_unlock_irq(&wdata->state.lock);
-
 	hid_dbg(wdata->hdev, "detected extensions: MP: %d EXT: %d\n",
 		wdata->state.mp, wdata->state.exttype);
 }
@@ -1241,11 +1226,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);
-	__wiimote_schedule(wdata);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		__wiimote_schedule(wdata);
 }
 
 static void wiimote_init_timeout(struct timer_list *t)
@@ -1635,7 +1617,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;
@@ -1643,9 +1624,8 @@ 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;
 		}
 	}
@@ -1663,11 +1643,9 @@ 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:
@@ -1716,11 +1694,9 @@ 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:
@@ -1775,17 +1751,14 @@ static DEFINE_IDA(wiimote_ida);
 
 static void wiimote_destroy(struct wiimote_data *wdata)
 {
-	unsigned long flags;
-
 	wiidebug_deinit(wdata);
 
 	if (wdata->player_id > 0)
 		ida_free(&wiimote_ida, wdata->player_id);
 
 	/* 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 fc847c2a1c1f1..b8027bb236089 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -7,12 +7,13 @@
 /*
  */
 
-#include <linux/debugfs.h>
-#include <linux/module.h>
-#include <linux/seq_file.h>
-#include <linux/spinlock.h>
-#include <linux/uaccess.h>
-#include "hid-wiimote.h"
+ #include <linux/cleanup.h>
+ #include <linux/debugfs.h>
+ #include <linux/module.h>
+ #include <linux/seq_file.h>
+ #include <linux/spinlock.h>
+ #include <linux/uaccess.h>
+ #include "hid-wiimote.h"
 
 struct wiimote_debug {
 	struct wiimote_data *wdata;
@@ -25,7 +26,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 +41,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);
-	wdata->state.cmd_read_buf = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.cmd_read_buf = NULL;
 
 	wiimote_cmd_release(wdata);
 
@@ -99,12 +98,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);
-	drm = dbg->wdata->state.drm;
-	spin_unlock_irqrestore(&dbg->wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &dbg->wdata->state.lock)
+		drm = dbg->wdata->state.drm;
 
 	if (drm < WIIPROTO_REQ_MAX)
 		str = wiidebug_drmmap[drm];
@@ -126,7 +123,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 +146,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 +168,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)
@@ -189,9 +184,8 @@ int wiidebug_init(struct wiimote_data *wdata)
 	debugfs_create_u8("player_id", S_IRUSR,
 		   dbg->wdata->hdev->debug_dir, &wdata->player_id);
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->debug = dbg;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->debug = dbg;
 
 	return 0;
 }
@@ -199,14 +193,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);
-	wdata->debug = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->debug = NULL;
 
 	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 a3d3fda9703dc..fa8c63eb6097d 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -30,6 +30,7 @@
  * input devices.
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input.h>
@@ -341,7 +342,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;
 
-- 
2.55.0

[PATCH v5 3/3] HID: wiimote: led_probe with scoped cleanup

[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>
Reviewed-by: David Rheinsberg <david@readahead.eu>
---
 drivers/hid/hid-wiimote-modules.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index fa8c63eb6097d..199589cbf2b85 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -339,9 +339,9 @@ static void wiimod_led_set(struct led_classdev *led_dev,
 static int wiimod_led_probe(const struct wiimod_ops *ops,
 			    struct wiimote_data *wdata)
 {
+	struct led_classdev *led __free(kfree) = NULL;
 	struct device *dev = &wdata->hdev->dev;
 	size_t namesz = strlen(dev_name(dev)) + 9;
-	struct led_classdev *led;
 	char *name;
 	int ret;
 
@@ -359,15 +359,14 @@ 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;
+	}
+
+	retain_and_null_ptr(led);
 
 	return 0;
-
-err_free:
-	wdata->leds[ops->arg] = NULL;
-	kfree(led);
-	return ret;
 }
 
 static void wiimod_led_remove(const struct wiimod_ops *ops,
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] hid: incorrect preprocessor directive indentation
--

commit 6b73789c825ec316d85d88975b55b4fcb3448d0c
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: replace spinlock pairs with scoped_guard

This commit replaces manual lock/unlock sequences with scoped guards in the
wiimote driver. This is a cleanup that does not change behavior but aims to
make the code safer to modify in the future.

> diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
> index fc847c2a1c1f1..b8027bb236089 100644
> --- a/drivers/hid/hid-wiimote-debug.c
> +++ b/drivers/hid/hid-wiimote-debug.c
> @@ -7,12 +7,13 @@
>  /*
>   */
>  
> -#include <linux/debugfs.h>
> -#include <linux/module.h>
> -#include <linux/seq_file.h>
> -#include <linux/spinlock.h>
> -#include <linux/uaccess.h>
> -#include "hid-wiimote.h"
> + #include <linux/cleanup.h>
> + #include <linux/debugfs.h>
> + #include <linux/module.h>

[Severity: Low]
This isn't a bug, but could we remove the leading spaces before these #include
directives? The extra indentation violates the standard kernel coding style.

> + #include <linux/seq_file.h>
> + #include <linux/spinlock.h>
> + #include <linux/uaccess.h>
> + #include "hid-wiimote.h"

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: wiimote: UAF on probe error path due to early sysfs exposure
- [Medium] hid: wiimote: premature ida_free() races with running workers
--

commit a053e1ef17e333e649ad6046f972b3ed52cf7844
Author: Rafael Passos <rafael@rcpassos.me>

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

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

> 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
> @@ -670,7 +677,10 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  
>  	spin_lock_irq(&wdata->state.lock);
>  	wdata->state.devtype = devtype;
> +	/* after loading modules, set the Player ID LED cycling the available LEDs*/
> +	wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1U) % ARRAY_SIZE(player_leds)]);
>  	spin_unlock_irq(&wdata->state.lock);
> +
>  	return;

[Severity: High]
Could wdata->player_id be 0 here if the initialization worker runs before
probe finishes allocating the ID?

If a sysfs write to dev_attr_extension triggers the initialization worker
before wiimote_hid_probe() has assigned player_id via ida_alloc_range(),
wdata->player_id will still be 0. This would cause an underflow in the
calculation (0 - 1U) % 4, incorrectly indexing player_leds[3].

[ ... ]

> @@ -1759,12 +1770,18 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>  	return wdata;
>  }
>  
> +/* Global id allocator for wii remotes */
> +static DEFINE_IDA(wiimote_ida);
> +
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
>  	unsigned long flags;
>  
>  	wiidebug_deinit(wdata);
>  
> +	if (wdata->player_id > 0)
> +		ida_free(&wiimote_ida, wdata->player_id);
> +
>  	/* 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);
>  
>  	cancel_work_sync(&wdata->init_worker);

[Severity: Medium]
Is it safe to free the player ID here before synchronously cancelling
wdata->init_worker?

If the asynchronous initialization worker or timer is actively executing (for
example, performing a periodic hotplug scan or LED setup), releasing the ID
early could allow a concurrently probing wiimote to claim the same player ID.

This would result in the still-executing worker of the tearing-down device
continuing to use the old wdata->player_id, which could potentially conflict
with the newly registered device. Should the ida_free() be moved to run
after cancel_work_sync() and timer_shutdown_sync()?

[ ... ]

> @@ -1834,7 +1851,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_free;
>  
> -	hid_info(hdev, "New device registered\n");
> +	ret = ida_alloc_range(&wiimote_ida, 1, 255, GFP_KERNEL);
> +	if (ret < 1) {
> +		hid_err(hdev, "cannot allocate controller id\n");
> +		goto err_free;
> +	}
> +
> +	wdata->player_id = ret;

[Severity: High]
Does this sequence create a use-after-free window if probe fails?

Prior to this point in wiimote_hid_probe(), device_create_file() exposes
sysfs nodes like dev_attr_extension. If userspace writes to "extension",
it can queue wdata->init_worker.

If a subsequent step fails (like device_create_file for dev_attr_devtype
failing before ida_alloc_range), the error path jumps to err_ext. The
err_ext path removes the file and eventually calls kfree(wdata) without
calling cancel_work_sync(&wdata->init_worker). If the init_worker was
already queued by userspace, it will execute and dereference the freed
wdata.

Could the sysfs nodes be created only after player_id is allocated and
all probe setup is complete, or should the err_ext path be updated to
cancel pending work?

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

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

[Rafael Passos]

Oh well, I dont know how this happened.

I will wait for reviews in the rest of the patchset before sending a new version...

On Thu Oct 8, 2026 at 11:44 PM -03, sashiko-bot wrote:
> [Severity: Low]
> This isn't a bug, but could we remove the leading spaces before these #include
> directives? The extra indentation violates the standard kernel coding style.
>
>> + #include <linux/seq_file.h>
>> + #include <linux/spinlock.h>
>> + #include <linux/uaccess.h>
>> + #include "hid-wiimote.h"