[PATCH v4 0/4] HID: wiimote: new LED behavior on connect + scoped_guards

[PATCH v4 0/4] 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.
This was trivial for the LED probe.
The wiimote_probe required a new state tracker bitmask, now the wiimote_destroy
is used both on disconnect (hid_remove) and in the probe cleanup.

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

As we discussed in the v3, tell me if you like the changes in patch 4/4.
If you prefer not to apply it, I can send a new patchset revision, or
just drop it from the set if nothing else needs changes.

Thanks!


Rafael Passos (4):
  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
  HID: wiimote: wiimote_probe with scoped cleanup

 drivers/hid/hid-wiimote-core.c    | 346 ++++++++++++++++--------------
 drivers/hid/hid-wiimote-debug.c   |  71 +++---
 drivers/hid/hid-wiimote-modules.c |  24 +--
 drivers/hid/hid-wiimote.h         |  10 +
 4 files changed, 240 insertions(+), 211 deletions(-)

-- 
2.55.0

[PATCH v4 1/4] 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  | 52 +++++++++++++++++++++++++++++----
 drivers/hid/hid-wiimote-debug.c |  5 +++-
 drivers/hid/hid-wiimote.h       |  1 +
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 63c4fa8fbb9b..acf31d8b6991 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)
 {
@@ -671,6 +678,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);
+
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
+		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
+	}
+
 	return;
 
 error:
@@ -855,11 +868,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 +1765,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 unasigned id
 
 	INIT_WORK(&wdata->init_worker, wiimote_init_worker);
 	timer_setup(&wdata->timer, wiimote_init_timeout, 0);
@@ -1759,12 +1773,17 @@ 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);
 
+	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 +1853,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_min(&wiimote_ida, 1, 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", ret);
 
 	/* schedule device detection */
 	wiimote_schedule(wdata);
@@ -1887,7 +1913,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 5f74917781f2..fc847c2a1c1f 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.h b/drivers/hid/hid-wiimote.h
index 9c12f63f6dd2..8e5002f515e2 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 v4 2/4] 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    | 224 +++++++++++++-----------------
 drivers/hid/hid-wiimote-debug.c   |  62 ++++-----
 drivers/hid/hid-wiimote-modules.c |   7 +-
 3 files changed, 127 insertions(+), 166 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index acf31d8b6991..05f8ddb7909b 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,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 			goto error;
 	}
 
-	spin_lock_irq(&wdata->state.lock);
-	wdata->state.devtype = devtype;
-	spin_unlock_irq(&wdata->state.lock);
-
 	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.devtype = devtype;
 		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
 		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
 	}
@@ -703,13 +698,11 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)
 {
 	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)
@@ -736,7 +729,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;
 
@@ -748,22 +740,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);
@@ -771,7 +761,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;
@@ -783,14 +772,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)
@@ -798,10 +785,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);
@@ -885,19 +872,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;
@@ -910,11 +897,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);
+	}
 }
 
 /*
@@ -962,9 +949,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);
 
@@ -980,11 +966,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");
@@ -1005,10 +991,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");
@@ -1031,11 +1017,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");
@@ -1061,11 +1047,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");
@@ -1120,16 +1106,15 @@ 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);
@@ -1152,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]);
@@ -1192,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);
 }
@@ -1244,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)
@@ -1638,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;
@@ -1646,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;
 		}
 	}
@@ -1666,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:
@@ -1719,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:
@@ -1778,16 +1751,13 @@ static DEFINE_IDA(wiimote_ida);
 
 static void wiimote_destroy(struct wiimote_data *wdata)
 {
-	unsigned long flags;
-
 	wiidebug_deinit(wdata);
 
 	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 fc847c2a1c1f..b8027bb23608 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 dccb78bb3afd..3cd614466740 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;
 
@@ -364,9 +364,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);
-		wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
-		spin_unlock_irqrestore(&wdata->state.lock, flags);
+		scoped_guard(spinlock_irqsave, &wdata->state.lock)
+			wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
 	}
 
 	return 0;
-- 
2.55.0

[PATCH v4 3/4] 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>
---
 drivers/hid/hid-wiimote-modules.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index 3cd614466740..47fa6a8ecdae 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -341,11 +341,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;
 
@@ -359,8 +359,12 @@ 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);
 
 	/* enable LED1 to stop initial LED-blinking */
 	if (ops->arg == 0) {
@@ -369,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.55.0

[PATCH v4 4/4] HID: wiimote: wiimote_probe with scoped cleanup

[Rafael Passos]

Use the safer scoped cleanup with a single destroy function.
A new bitmask was introduced to track probing state.
This is needed because the hid_hw calls cannot be made with null.

A few other functions are safe to call without checking.
These cases are annotated with comments above them.

Also, a new debugfs entry was added tracking this new state (bitmask).

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c  | 78 +++++++++++++++++++--------------
 drivers/hid/hid-wiimote-debug.c |  4 ++
 drivers/hid/hid-wiimote.h       |  9 ++++
 3 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 05f8ddb7909b..044da4daa010 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -679,6 +679,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
 	}
 
+
+	wdata->init_state |= WIIMOTE_MODULES_LOADED;
 	return;
 
 error:
@@ -742,6 +744,8 @@ static void wiimote_ext_load(struct wiimote_data *wdata, unsigned int ext)
 
 	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 		wdata->state.exttype = ext;
+
+	wdata->init_state |= WIIMOTE_EXT_LOADED;
 }
 
 static void wiimote_ext_unload(struct wiimote_data *wdata)
@@ -774,6 +778,8 @@ static void wiimote_mp_load(struct wiimote_data *wdata)
 
 	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 		wdata->state.mp = mode;
+
+	wdata->init_state |= WIIMOTE_MP_LOADED;
 }
 
 static void wiimote_mp_unload(struct wiimote_data *wdata)
@@ -1751,39 +1757,56 @@ static DEFINE_IDA(wiimote_ida);
 
 static void wiimote_destroy(struct wiimote_data *wdata)
 {
+	if (!wdata)
+		return;
+
+	// safe, debugfs checks IS_ERR_OR_NULL
 	wiidebug_deinit(wdata);
 
-	ida_free(&wiimote_ida, wdata->player_id);
+	if (wdata->player_id)
+		ida_free(&wiimote_ida, wdata->player_id);
 
 	/* prevent init_worker from being scheduled again */
 	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
 
-	cancel_work_sync(&wdata->init_worker);
-	timer_shutdown_sync(&wdata->timer);
+	if (wdata->init_state & WIIMOTE_PROBE_READY) {
+		cancel_work_sync(&wdata->init_worker);
+		timer_shutdown_sync(&wdata->timer);
+	}
 
+	// safe, checks dev for NULL
 	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
 	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
 
-	wiimote_mp_unload(wdata);
-	wiimote_ext_unload(wdata);
-	wiimote_modules_unload(wdata);
+	if (wdata->init_state & WIIMOTE_MP_LOADED)
+		wiimote_mp_unload(wdata);
+	if (wdata->init_state & WIIMOTE_EXT_LOADED)
+		wiimote_ext_unload(wdata);
+	if (wdata->init_state & WIIMOTE_MODULES_LOADED)
+		wiimote_modules_unload(wdata);
+
 	cancel_work_sync(&wdata->queue.worker);
-	hid_hw_close(wdata->hdev);
-	hid_hw_stop(wdata->hdev);
+
+	if (wdata->init_state & WIIMOTE_PROBE_HW_OPENED)
+		hid_hw_close(wdata->hdev);
+	if (wdata->init_state & WIIMOTE_PROBE_HW_STARTED)
+		hid_hw_stop(wdata->hdev);
 
 	kfree(wdata);
 }
 
+DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
+	wiimote_destroy(_T))
+
 static int wiimote_hid_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
-	struct wiimote_data *wdata;
 	int ret;
 
 	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
-	wdata = wiimote_create(hdev);
+	struct wiimote_data *wdata __free(wiimote_probe_cleanup) = wiimote_create(hdev);
 	if (!wdata) {
 		hid_err(hdev, "Can't alloc device\n");
 		return -ENOMEM;
@@ -1792,41 +1815,43 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "HID parse failed\n");
-		goto err;
+		return ret;
 	}
 
 	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
 	if (ret) {
 		hid_err(hdev, "HW start failed\n");
-		goto err;
+		return ret;
 	}
+	wdata->init_state |= WIIMOTE_PROBE_HW_STARTED;
 
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "cannot start hardware I/O\n");
-		goto err_stop;
+		return ret;
 	}
+	wdata->init_state |= WIIMOTE_PROBE_HW_OPENED;
 
 	ret = device_create_file(&hdev->dev, &dev_attr_extension);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
-		goto err_close;
+		return ret;
 	}
 
 	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
-		goto err_ext;
+		return ret;
 	}
 
 	ret = wiidebug_init(wdata);
 	if (ret)
-		goto err_free;
+		return ret;
 
 	ret = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
 	if (ret < 1) {
 		hid_err(hdev, "cannot allocate controller id\n");
-		goto err_free;
+		return ret;
 	}
 
 	wdata->player_id = ret;
@@ -1834,24 +1859,10 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 
 	/* schedule device detection */
 	wiimote_schedule(wdata);
+	wdata->init_state |= WIIMOTE_PROBE_READY;
 
+	retain_and_null_ptr(wdata);
 	return 0;
-
-err_free:
-	wiimote_destroy(wdata);
-	return ret;
-
-err_ext:
-	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
-err_close:
-	hid_hw_close(hdev);
-err_stop:
-	hid_hw_stop(hdev);
-err:
-	input_free_device(wdata->ir);
-	input_free_device(wdata->accel);
-	kfree(wdata);
-	return ret;
 }
 
 static void wiimote_hid_remove(struct hid_device *hdev)
@@ -1902,3 +1913,4 @@ module_exit(wiimote_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
 MODULE_DESCRIPTION("Driver for Nintendo Wii / Wii U peripherals");
+
diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
index b8027bb23608..1353ab022acb 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -184,6 +184,9 @@ int wiidebug_init(struct wiimote_data *wdata)
 	debugfs_create_u8("player_id", S_IRUSR,
 		   dbg->wdata->hdev->debug_dir, &wdata->player_id);
 
+	debugfs_create_u8("init_state", S_IRUSR,
+		   dbg->wdata->hdev->debug_dir, &wdata->init_state);
+
 	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 		wdata->debug = dbg;
 
@@ -203,5 +206,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);
+	debugfs_lookup_and_remove("init_state", dbg->wdata->hdev->debug_dir);
 	kfree(dbg);
 }
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index 8e5002f515e2..147751973702 100644
--- a/drivers/hid/hid-wiimote.h
+++ b/drivers/hid/hid-wiimote.h
@@ -154,6 +154,7 @@ struct wiimote_data {
 	struct timer_list timer;
 	struct wiimote_debug *debug;
 	u8 player_id;
+	u8 init_state;
 
 	union {
 		struct input_dev *input;
@@ -376,4 +377,12 @@ static inline int wiimote_cmd_wait_noint(struct wiimote_data *wdata)
 		return 0;
 }
 
+/* controller initialization tracker bits */
+#define WIIMOTE_PROBE_HW_STARTED  BIT(0)  // hid_hw_start succeeded
+#define WIIMOTE_PROBE_HW_OPENED   BIT(1)  // hid_hw_open succeeded
+#define WIIMOTE_PROBE_READY       BIT(2)  // wiimote_schedule succeeded
+#define WIIMOTE_MP_LOADED         BIT(3)  // wiimote_mp_load succeeded
+#define WIIMOTE_EXT_LOADED        BIT(4)  // wiimote_ext_load succeeded
+#define WIIMOTE_MODULES_LOADED    BIT(5)  // wiimote_modules_load succeeded
+
 #endif
-- 
2.55.0

Re: [PATCH v4 4/4] HID: wiimote: wiimote_probe with scoped cleanup

[sashiko-bot]

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

New issues:
- [Critical] Skipping cancellation of `init_worker` and `timer` on probe error paths due to a conditional check leads to a Use-After-Free.
- [High] Non-atomic bitwise updates to `wdata->init_state` across concurrent threads cause a data race, leading to lost state flags and resource leaks.

Pre-existing issues:
- [Critical] Incorrect teardown ordering in `wiimote_destroy()` cancels `queue.worker` before stopping hardware, allowing the worker to be re-queued after cancellation.
--

--- Patch [4]: [PATCH v4 4/4] HID: wiimote: wiimote_probe with scoped cleanup ---
commit b9fae511d83189a3adfc0824b46530dbaab4dda2
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: wiimote_probe with scoped cleanup

This commit introduces scoped cleanup to the wiimote driver, adding a
new bitmask to track the probing state. It updates the probe and destroy
functions to conditionally clean up resources based on this state.

> 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

[ ... ]

> @@ -1751,39 +1757,56 @@ static DEFINE_IDA(wiimote_ida);
>  
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
[ ... ]
>  	/* prevent init_worker from being scheduled again */
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock)
>  		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
>  
> -	cancel_work_sync(&wdata->init_worker);
> -	timer_shutdown_sync(&wdata->timer);
> +	if (wdata->init_state & WIIMOTE_PROBE_READY) {
> +		cancel_work_sync(&wdata->init_worker);
> +		timer_shutdown_sync(&wdata->timer);
> +	}

[Severity: Critical]
If probe fails after creating the sysfs nodes but before setting
WIIMOTE_PROBE_READY, can this lead to a use-after-free?

In wiimote_hid_probe(), the sysfs node for extension is created before
initialization finishes:

wiimote_hid_probe() {
        ...
        ret = device_create_file(&hdev->dev, &dev_attr_extension);
        ...
}

If userspace writes to this node, it can trigger wiimote_schedule(), placing
init_worker on the workqueue. If a subsequent step in probe fails,
wiimote_destroy() is called. Since WIIMOTE_PROBE_READY is not yet set, the
worker and timer cancellations are skipped, and wdata is freed while the
worker is still queued.

[ ... ]

> +	if (wdata->init_state & WIIMOTE_MODULES_LOADED)
> +		wiimote_modules_unload(wdata);
> +
>  	cancel_work_sync(&wdata->queue.worker);
> -	hid_hw_close(wdata->hdev);
> -	hid_hw_stop(wdata->hdev);
> +
> +	if (wdata->init_state & WIIMOTE_PROBE_HW_OPENED)
> +		hid_hw_close(wdata->hdev);
> +	if (wdata->init_state & WIIMOTE_PROBE_HW_STARTED)
> +		hid_hw_stop(wdata->hdev);
>  
>  	kfree(wdata);
>  }

[Severity: Critical]
This is a pre-existing issue, but does this teardown order allow
queue.worker to be re-queued after it is cancelled?

Here, cancel_work_sync(&wdata->queue.worker) is called before hid_hw_stop().
Because the hardware is still active, incoming HID reports can trigger
wiimote_hid_event(). Event handlers can queue output commands via
wiimote_queue(), which calls schedule_work(&wdata->queue.worker).

This could place the worker back on the system workqueue just before wdata
is freed, leading to a use-after-free when the worker executes.

[ ... ]

> @@ -1834,24 +1859,10 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  
>  	/* schedule device detection */
>  	wiimote_schedule(wdata);
> +	wdata->init_state |= WIIMOTE_PROBE_READY;
>  
> +	retain_and_null_ptr(wdata);
>  	return 0;

[Severity: High]
Can these non-atomic bitwise updates to wdata->init_state cause data races?

Here, wiimote_schedule(wdata) queues init_worker. Immediately after, this
thread updates wdata->init_state |= WIIMOTE_PROBE_READY.

Concurrently, init_worker executes on another CPU and calls functions like
wiimote_modules_load(), which perform their own non-atomic updates:

wiimote_modules_load() {
        ...
        wdata->init_state |= WIIMOTE_MODULES_LOADED;
}

Since init_state is a u8, these non-atomic read-modify-write operations can
race. If the worker's update overwrites the probe thread's update,
WIIMOTE_PROBE_READY is lost, skipping worker cancellation during teardown.
If the probe thread overwrites the worker's update, WIIMOTE_MODULES_LOADED
is lost, potentially leaking devices on unbind.

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

Re: [PATCH v4 1/4] 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] Out-of-bounds memory read and IDA leak due to integer truncation of `player_id`.
- [Medium] Unconditional `ida_free(0)` in probe error paths triggers a kernel WARN splat.
--

--- Patch [1]: [PATCH v4 1/4] HID: wiimote: turn on the LEDs indicating the controller id ---
commit ee8a446b2fee34f66eb9e5ae9f8fb5e25ff963be
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
> index 63c4fa8fbb9b6..acf31d8b6991d 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
[ ... ]
> @@ -671,6 +678,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);
> +
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
> +		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);

[Severity: High]
Does this calculation cause an out-of-bounds read if player_id is 0?

Because of C integer promotion rules, if player_id is 0, (0 - 1) % 4
evaluates to -1, which results in reading player_leds[-1].

> +	}
> +
>  	return;
[ ... ]
> @@ -1759,12 +1773,17 @@ 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);
>  
> +	ida_free(&wiimote_ida, wdata->player_id);
> +

[Severity: Medium]
Will this unconditionally free ID 0 if early probe steps fail?

If an error occurs in wiimote_hid_probe() before an ID is successfully
allocated, the error path jumps to err_free and calls wiimote_destroy().
Because player_id is initialized to 0, calling ida_free(&wiimote_ida, 0)
for an unallocated ID triggers a WARN(1) splat in the IDA core.

[ ... ]
> 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;

[Severity: High]
Can this u8 declaration lead to integer truncation and an IDA leak?

The ida_alloc_min() function returns an unbounded int. If the allocated
ID reaches 256, it will silently truncate to 0 when assigned to this
u8 field.

If truncation occurs, the 0 value will later cause the out-of-bounds read
in wiimote_modules_load() mentioned above. Furthermore, wiimote_destroy()
will pass 0 to ida_free(), permanently leaking the originally allocated ID
(e.g., 256).

>  
>  	union {
>  		struct input_dev *input;

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

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

[sashiko-bot]

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

New issues:
- [Low] Functions improperly mix goto-based cleanup labels with scoped_guard usage.
- [Low] Inconsistent space-based indentation utilized inside scoped_guard blocks.
- [Low] The commit message inaccurately describes the formatting configuration of one-liner scoped_guards.

Pre-existing issues:
- [High] The `queue.worker` can be rescheduled after cancellation because hardware polling is stopped too late.
--

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

    HID: wiimote: replace spinlock pairs with scoped_guard

This commit replaces manual spinlock lock/unlock pairs with scoped_guard
across the wiimote driver to improve safety and maintainability.

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

[Severity: Low]
This isn't a bug, but there's a discrepancy in the commit message. The 
one-liners in the diff correctly retain their tab indentation for the 
guarded statement (e.g., in wiimote_cmd_read), contradicting the 
description here.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index acf31d8b6991d..05f8ddb7909bc 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
[ ... ]
> @@ -362,13 +363,12 @@ void wiiproto_req_rmem(struct wiimote_data *wdata, bool eeprom, __u32 offset,
>  int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset,
>  						const __u8 *wmem, __u8 size)
>  {
> -	unsigned long flags;
>  	int ret;
>  
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
> -	wiiproto_req_wreg(wdata, offset, wmem, size);
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +	    wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
> +	    wiiproto_req_wreg(wdata, offset, wmem, size);
> +	}

[Severity: Low]
This isn't a bug, but does this inner block use spaces for indentation 
instead of standard tabs?

>  	ret = wiimote_cmd_wait(wdata);
>  	if (!ret && wdata->state.cmd_err)
[ ... ]
> @@ -675,11 +673,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  			goto error;
>  	}
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = devtype;
> -	spin_unlock_irq(&wdata->state.lock);
> -
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		wdata->state.devtype = devtype;
>  		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
>  		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
>  	}
> @@ -703,13 +698,11 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)

[Severity: Low]
Does this mix goto-based error handling (the goto error statements above) 
with cleanup helpers in the same function?

The cleanup subsystem guidelines suggest that for a given routine, we 
should convert all resources that need a goto cleanup to scope-based 
cleanup, or convert none of them. This pattern also appears in 
wiimote_init_detect and wiimote_init_check.

[ ... ]
> @@ -1120,16 +1106,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -
> -	/* get state snapshot that we will then work on */
> -	flags = wdata->state.flags;
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
>  
> -	/* disable event forwarding temporarily */
> -	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
> -	wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
> +	    /* get state snapshot that we will then work on */
> +	    flags = wdata->state.flags;
>  
> -	spin_unlock_irq(&wdata->state.lock);
> +	    /* disable event forwarding temporarily */
> +	    wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
> +	    wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
> +	}

[Severity: Low]
This isn't a bug, but it looks like spaces are used here instead of tabs 
for the indentation inside the scoped_guard block.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the wdata->queue.worker be 
rescheduled after it is canceled during device teardown?

Looking at wiimote_destroy (which is not modified in this patch but 
is in the same file):

	cancel_work_sync(&wdata->queue.worker);

	if (wdata->init_state & WIIMOTE_PROBE_HW_OPENED)
		hid_hw_close(wdata->hdev);
	if (wdata->init_state & WIIMOTE_PROBE_HW_STARTED)
		hid_hw_stop(wdata->hdev);

	kfree(wdata);

If the hardware sends a report between the cancel_work_sync and 
hid_hw_stop calls, wiimote_hid_event could process it and queue 
a request via wiimote_queue, which calls 
schedule_work(&wdata->queue.worker).

Since wdata is freed immediately after hid_hw_stop, would the 
rescheduled worker later access the freed wdata structure and cause a 
use-after-free? Should hid_hw_stop be called before canceling the 
worker?

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