* Re: [PATCH 3/3] HID: magicmouse: report charge status over Bluetooth
From: Alec Hall @ 2026-07-15 22:18 UTC (permalink / raw)
To: Jose Villaseñor Montfort, linux-input
Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel, Alec Hall
In-Reply-To: <20260715200354.1307250-1-pepemontfort@gmail.com>
Hi Jose,
Great -- doing it in magicmouse_probe() is the better fix. Moving the
"input not registered" check ahead of the early return rejects the bad
device up front and covers both ->raw_event and ->event, which is cleaner
than a guard in the hot path. No need for me to take it; please keep it.
I'll review both patches and give them a run on my hardware (Magic Keyboard
2021 and Magic Trackpad 2 over Bluetooth) to confirm the normal
enumeration and battery path is unaffected, and follow up with
Reviewed-by/Tested-by on the respective threads.
Agreed on the rebase too -- if yours land first I'll just relocate the 3/3
report-check hunk into the new wrapper.
Thanks for taking both of these,
Alec
^ permalink raw reply
* Re: [PATCH v2 4/4] HID: wiimote: fix uaf when hid events are handled during destroy
From: sashiko-bot @ 2026-07-15 21:57 UTC (permalink / raw)
To: Rafael Passos; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715212501.3920033-5-rafael@rcpassos.me>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Module cleanup commands (e.g., rumble off, LEDs off) are dropped during driver unload, leaving the controller hardware in an incorrect or stuck state.
--
--- Patch [4]: [PATCH v2 4/4] HID: wiimote: fix uaf when hid events are handled during destroy ---
commit 62bea240ecf4b4da704b1580ca426a1f9c83501a
Author: Rafael Passos <rafael@rcpassos.me>
HID: wiimote: fix uaf when hid events are handled during destroy
This commit adds a flag check to wiimote_queue() to drop events
if the driver is in the process of unloading. This mitigates a
use-after-free issue when handling events during destroy.
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index b549a7d58d512..c9638ec352e44 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -92,6 +92,9 @@ static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
> */
>
> spin_lock_irqsave(&wdata->queue.lock, flags);
> + /* Do not schedule work if controller is exiting */
> + if ((wdata->state.flags & WIIPROTO_FLAG_EXITING))
> + goto out_unlock;
[Severity: Medium]
Will this aggressively drop hardware cleanup commands during driver unload?
Looking at wiimote_destroy(), the exiting flag is set before calling
wiimote_modules_unload():
drivers/hid/hid-wiimote-core.c:wiimote_destroy() {
...
/* prevent init_worker from being scheduled again */
scoped_guard(spinlock_irqsave, &wdata->state.lock)
wdata->state.flags |= WIIPROTO_FLAG_EXITING;
...
wiimote_modules_unload(wdata);
...
}
If a user unbinds the driver or disconnects the controller while rumble is
active, wiimote_modules_unload() invokes wiimod_rumble_remove() to send
a command to turn the rumble off.
Will this check in wiimote_queue() unexpectedly drop the rumble-off
packet, leaving the physical controller vibrating indefinitely?
>
> memcpy(wdata->queue.outq[wdata->queue.head].data, buffer, count);
> wdata->queue.outq[wdata->queue.head].size = count;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=4
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: wiimote: turn on the LEDs indicating the controller id
From: sashiko-bot @ 2026-07-15 21:48 UTC (permalink / raw)
To: Rafael Passos; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715212501.3920033-2-rafael@rcpassos.me>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer truncation of IDA allocation results in out-of-bounds read, IDA resource leak, and kernel WARN.
- [Medium] Race condition causing use-before-initialization of `player_id`, leading to an out-of-bounds read.
- [Medium] Mixing `goto`-based error handling and `scoped_guard()` in the same function violates kernel cleanup subsystem rules.
--
--- Patch [1]: [PATCH v2 1/4] HID: wiimote: turn on the LEDs indicating the controller id ---
commit 49992cb763819a5276c65ada6594868205443ccd
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..91c2cec35f6eb 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);
> +
> + /* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
> + scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> + wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
> + }
> +
> return;
>
> error:
[Severity: Medium]
Does using scoped_guard() in this function violate the cleanup subsystem
guidelines?
The wiimote_modules_load() function appears to use a goto-based failure path
(goto error;), but introduces a scoped_guard() scope-based cleanup macro.
The cleanup subsystem documentation indicates that mixing goto-based error
handling and scoped_guard() in the same function can create confusing
ownership semantics and should be avoided.
[ ... ]
> @@ -1834,7 +1851,16 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> if (ret)
> goto err_free;
>
> - hid_info(hdev, "New device registered\n");
> + player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> + if (player_id < 1) {
> + hid_err(hdev, "cannot allocate controller id\n");
> + ret = player_id;
> + goto err_free;
> + }
> +
> + wdata->player_id = player_id;
[Severity: High]
Can this assignment truncate the allocated ID?
The player_id variable returned by ida_alloc_min() is an int, but
wdata->player_id is defined as a __u8 in struct wiimote_data.
If enough devices are created (e.g., via /dev/uhid) to allocate ID 256,
the value would truncate to 0.
When this 0 is later evaluated in wiimote_modules_load():
player_leds[(wdata->player_id - 1) % 4]
The expression (0 - 1) % 4 evaluates to -1, which would lead to an
out-of-bounds array read on player_leds[-1].
Additionally, during device removal in wiimote_hid_remove(), calling
ida_free(&wiimote_ida, wdata->player_id) with the truncated 0 would
trigger a WARN_ON_ONCE in the IDA core (since ID 0 was never allocated)
and permanently leak the originally allocated ID.
[Severity: Medium]
Is there a race condition where wdata->player_id is used before it is
initialized?
The hardware I/O is started and opened via hid_hw_open() earlier in
wiimote_hid_probe(), before the player_id is allocated and assigned here.
If the device immediately sends an event, could a background worker
executing wiimote_modules_load() read wdata->player_id while it is still 0?
This would cause the same out-of-bounds read on player_leds[-1] described
above.
[ ... ]
> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index 9c12f63f6dd2d..a53f72d5077ef 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;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=1
^ permalink raw reply
* Re: [PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes
From: sashiko-bot @ 2026-07-15 21:42 UTC (permalink / raw)
To: Rafael Passos; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715212501.3920033-4-rafael@rcpassos.me>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new scoped cleanup function `__wiimote_probe_cleanup()` fails to cancel async workers, timers, and loaded modules on error, leading to a Use-After-Free and dangling registered devices.
- [Medium] Truncation of `player_id` from `int` to `__u8` causes IDA double frees, leaked IDs, and an out-of-bounds array access.
--
--- Patch [3]: [PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes ---
commit 29027bf7f674068d4630217f5005fa7c42f22f18
Author: Rafael Passos <rafael@rcpassos.me>
Subject: HID: wiimote: use scoped cleanup in wiimote and led probes
This patch refactors the cleanup paths in the wiimote and led probe functions
to use scoped cleanup macros. It also introduces a state mask to track the
hardware probing progress.
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index d37d740a7162f..b549a7d58d512 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1774,16 +1774,40 @@ static void wiimote_destroy(struct wiimote_data *wdata)
> /* Global id allocator for wii remotes */
> static DEFINE_IDA(wiimote_ida);
>
> +#define WIIMOTE_PROBE_HW_STARTED BIT(0) // hid_hw_start succeeded
> +#define WIIMOTE_PROBE_HW_OPENED BIT(1) // hid_hw_open succeeded
> +
> +static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
> +{
> + if (!wdata)
> + return;
> +
> + if (wdata->player_id)
> + ida_free(&wiimote_ida, wdata->player_id);
> +
> + // safe, debugfs checks IS_ERR_OR_NULL
> + wiidebug_deinit(wdata);
> + // safe, checks dev for NULL
> + device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
> + device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> + if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
> + hid_hw_close(wdata->hdev);
> + if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
> + hid_hw_stop(wdata->hdev);
> + kfree(wdata);
> +}
[Severity: High]
Does this new cleanup function safely handle asynchronous teardown?
In the original code, failures later in the probe function would route to
the err_free label which called wiimote_destroy(). That function cleanly
cancelled wdata->init_worker and unloaded any registered modules.
If a hardware event occurs during probe:
wiimote_hid_event()
queues wdata->init_worker
And then an allocation fails (e.g. in wiidebug_init or ida_alloc_min):
__wiimote_probe_cleanup()
hid_hw_stop(wdata->hdev)
kfree(wdata)
Will the queued worker execute after wdata has been freed?
[ ... ]
> @@ -1792,68 +1816,54 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> ret = wiidebug_init(wdata);
> if (ret)
> - goto err_free;
> + return ret;
>
> - player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> + int player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> if (player_id < 1) {
> hid_err(hdev, "cannot allocate controller id\n");
> ret = player_id;
> - goto err_free;
> + return ret;
> }
> -
> wdata->player_id = player_id;
[Severity: Medium]
Could this assignment result in truncation if the allocated ID is larger
than 255?
ida_alloc_min() returns an int, but wdata->player_id is defined as a __u8.
If an ID exceeds 255, the truncation could result in 0.
If player_id becomes 0, could it cause an out-of-bounds read later?
Looking at wiimote_modules_load():
wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
This might evaluate to player_leds[-1].
Additionally, truncation could lead to multiple devices sharing the same
truncated ID, which might cause IDA double frees when devices are removed.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=3
^ permalink raw reply
* Re: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard
From: sashiko-bot @ 2026-07-15 21:39 UTC (permalink / raw)
To: Rafael Passos; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715212501.3920033-3-rafael@rcpassos.me>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] Functions mix `goto`-based error handling cleanup labels with scope-based cleanup (`scoped_guard()`), violating the cleanup subsystem architectural rule.
- [Low] Incorrect 4-space indentation within `scoped_guard` blocks instead of standard kernel tabs.
Pre-existing issues:
- [High] Use-after-free of `wdata` because `queue.worker` can be rescheduled after it is cancelled in `wiimote_destroy()`.
--
--- Patch [2]: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit 343c163a7f9f1368f62e298a4bac0953c054d9f3
Author: Rafael Passos <rafael@rcpassos.me>
HID: wiimote: replace spinlock pairs with scoped_guard
Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 91c2cec35f6eb..d37d740a7162f 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)
> {
[ ... ]
> - 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 did the indentation get changed to 4 spaces here instead
of standard kernel tabs?
[ ... ]
> @@ -885,19 +874,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
>
> wiimote_cmd_acquire_noint(wdata);
>
> - spin_lock_irq(&wdata->state.lock);
> - wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> - wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> - wiiproto_req_status(wdata);
> - spin_unlock_irq(&wdata->state.lock);
> + scoped_guard(spinlock_irq, &wdata->state.lock) {
> + wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> + wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> + wiiproto_req_status(wdata);
> + }
> +
>
> ret = wiimote_cmd_wait_noint(wdata);
> if (ret)
> goto out_release;
[Severity: Medium]
Does this mix goto-based cleanup labels with scope-based cleanup?
The cleanup guidelines state that for a given routine, all resources requiring
cleanup should be converted to scope-based cleanup, or none of them should be.
This function (as well as wiimote_init_check() and wiimote_modules_load())
still uses goto-based cleanup like goto out_release; and goto error;
combined with the new scoped_guard() declarations.
[ ... ]
> @@ -1120,16 +1108,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;
[Severity: Low]
this isn't a bug, but it looks like the contents of this scoped_guard block
are also indented with 4 spaces instead of a tab.
[ ... ]
> @@ -1774,14 +1749,11 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>
> static void wiimote_destroy(struct wiimote_data *wdata)
> {
> - unsigned long flags;
> -
> wiidebug_deinit(wdata);
>
> /* prevent init_worker from being scheduled again */
> - spin_lock_irqsave(&wdata->state.lock, flags);
> - wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> - spin_unlock_irqrestore(&wdata->state.lock, flags);
> + scoped_guard(spinlock_irqsave, &wdata->state.lock)
> + wdata->state.flags |= WIIPROTO_FLAG_EXITING;
>
> cancel_work_sync(&wdata->init_worker);
> timer_shutdown_sync(&wdata->timer);
[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
in wiimote_destroy() have a use-after-free race condition?
Looking at the rest of wiimote_destroy(), cancel_work_sync(&wdata->queue.worker)
is called before hid_hw_stop(wdata->hdev). If a HID interrupt arrives
immediately after the worker is cancelled:
HID core calls wiimote_hid_event() ->
handler_status_K() ->
wiiproto_req_drm() ->
wiimote_queue()
wiimote_queue() then calls schedule_work(&wdata->queue.worker) without
checking if the device is exiting.
Since hid_hw_stop() hasn't been called yet, the new worker is queued. After
hid_hw_stop() and kfree(wdata) complete, the queued worker can execute and
dereference the freed wdata.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=2
^ permalink raw reply
* [TEST] Patch used for testing the wiimote_probe_cleanup
From: Rafael Passos @ 2026-07-15 21:35 UTC (permalink / raw)
To: rafael; +Cc: bentiss, david, jikos, jkoolstra, linux-input
In-Reply-To: <20260715212501.3920033-4-rafael@rcpassos.me>
[DO NOT APPLY THIS PATCH] :)
This is the patch I used to simulate each possible failure case
in the probing function.
I can say going it was worth it, as I was originally not gating
ida_free, and the struct would corrupt.
Below are the logs from my last run with this.
I added a space between each iteration, to help reading:
[ 76.680204] wiimote 0005:057E:0330.0006: Probing wiimote iteration 1
[ 76.680216] __wiimote_probe_cleanup: wdata NULL
[ 76.680224] wiimote 0005:057E:0330.0006: probe with driver wiimote failed with error -12
[ 81.745168] wiimote 0005:057E:0330.0007: Probing wiimote iteration 2
[ 81.745312] wiimote 0005:057E:0330.0007: probe with driver wiimote failed with error -12
[ 86.869452] wiimote 0005:057E:0330.0008: Probing wiimote iteration 3
[ 86.869935] wiimote 0005:057E:0330.0008: unknown main item tag 0x0
[ 86.869983] wiimote 0005:057E:0330.0008: probe with driver wiimote failed with error -12
[ 92.001194] wiimote 0005:057E:0330.0009: Probing wiimote iteration 4
[ 92.001684] wiimote 0005:057E:0330.0009: unknown main item tag 0x0
[ 92.001905] wiimote 0005:057E:0330.0009: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 92.002075] __wiimote_probe_cleanup: hid_hw_stop cleanup
[ 92.002105] wiimote 0005:057E:0330.0009: probe with driver wiimote failed with error -12
[ 97.129059] wiimote 0005:057E:0330.000A: Probing wiimote iteration 5
[ 97.129528] wiimote 0005:057E:0330.000A: unknown main item tag 0x0
[ 97.129757] wiimote 0005:057E:0330.000A: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 97.129774] __wiimote_probe_cleanup: hid_hw_close cleanup
[ 97.129934] __wiimote_probe_cleanup: hid_hw_stop cleanup
[ 97.129962] wiimote 0005:057E:0330.000A: probe with driver wiimote failed with error -12
[ 102.265442] wiimote 0005:057E:0330.000B: Probing wiimote iteration 6
[ 102.265925] wiimote 0005:057E:0330.000B: unknown main item tag 0x0
[ 102.266521] wiimote 0005:057E:0330.000B: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 102.266538] __wiimote_probe_cleanup: hid_hw_close cleanup
[ 102.266814] __wiimote_probe_cleanup: hid_hw_stop cleanup
[ 102.266844] wiimote 0005:057E:0330.000B: probe with driver wiimote failed with error -12
[ 107.405172] wiimote 0005:057E:0330.000C: Probing wiimote iteration 7
[ 107.405602] wiimote 0005:057E:0330.000C: unknown main item tag 0x0
[ 107.405776] wiimote 0005:057E:0330.000C: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 107.405794] __wiimote_probe_cleanup: hid_hw_close cleanup
[ 107.405953] __wiimote_probe_cleanup: hid_hw_stop cleanup
[ 107.405981] wiimote 0005:057E:0330.000C: probe with driver wiimote failed with error -12
[ 112.525411] wiimote 0005:057E:0330.000D: Probing wiimote iteration 8
[ 112.525883] wiimote 0005:057E:0330.000D: unknown main item tag 0x0
[ 112.526323] wiimote 0005:057E:0330.000D: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 112.526362] __wiimote_probe_cleanup: hid_hw_close cleanup
[ 112.526511] __wiimote_probe_cleanup: hid_hw_stop cleanup
[ 112.526535] wiimote 0005:057E:0330.000D: probe with driver wiimote failed with error -12
[ 117.673238] wiimote 0005:057E:0330.000E: Probing wiimote iteration 9
[ 117.673700] wiimote 0005:057E:0330.000E: unknown main item tag 0x0
[ 117.673885] wiimote 0005:057E:0330.000E: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 117.673909] __wiimote_probe_cleanup: ida cleanup
[ 117.673929] __wiimote_probe_cleanup: hid_hw_close cleanup
[ 117.674081] __wiimote_probe_cleanup: hid_hw_stop cleanup
[ 117.674105] wiimote 0005:057E:0330.000E: probe with driver wiimote failed with error -12
[ 122.805206] wiimote 0005:057E:0330.000F: Probing wiimote iteration 10
[ 122.805669] wiimote 0005:057E:0330.000F: unknown main item tag 0x0
[ 122.805895] wiimote 0005:057E:0330.000F: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 122.805934] wiimote 0005:057E:0330.000F: New device registered (Wiimote 1)
[ 122.805940] __wiimote_probe_cleanup: wdata NULL
[ 122.841287] wiimote 0005:057E:0330.000F: detected device: Nintendo Wii Remote Plus (Gen 2) (1)
[ 122.841759] input: Nintendo Wii Remote Accelerometer as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input33
[ 122.844724] input: Nintendo Wii Remote IR as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input34
[ 122.845091] input: Nintendo Wii Remote as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input32
[ 122.965128] wiimote 0005:057E:0330.000F: detected extension: Nintendo Wii Motion Plus
[ 122.965276] input: Nintendo Wii Remote Motion Plus as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input35
[ 127.877355] wiimote 0005:057E:0330.000F: Device removed
[ 130.497046] wiimote 0005:057E:0330.0010: Probing wiimote iteration 11
[ 130.497524] wiimote 0005:057E:0330.0010: unknown main item tag 0x0
[ 130.497838] wiimote 0005:057E:0330.0010: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[ 130.497869] wiimote 0005:057E:0330.0010: New device registered (Wiimote 1)
[ 130.497874] __wiimote_probe_cleanup: wdata NULL
[ 130.533040] wiimote 0005:057E:0330.0010: detected device: Nintendo Wii Remote Plus (Gen 2) (1)
[ 130.533424] input: Nintendo Wii Remote Accelerometer as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input37
[ 130.533579] input: Nintendo Wii Remote IR as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input38
[ 130.533699] input: Nintendo Wii Remote as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input36
[ 130.641239] wiimote 0005:057E:0330.0010: detected extension: Nintendo Wii Motion Plus
[ 130.641385] input: Nintendo Wii Remote Motion Plus as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input39
[ 134.317450] wiimote 0005:057E:0330.0010: Device removed
Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
drivers/hid/hid-wiimote-core.c | 45 ++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 5 deletions(-)
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
@@ -1779,52 +1779,77 @@ static DEFINE_IDA(wiimote_ida);
static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
{
- if (!wdata)
+ if (!wdata) {
+ printk(KERN_INFO "%s: wdata NULL\n", __func__);
return;
+ }
- if (wdata->player_id)
+ if (wdata->player_id){
+ printk(KERN_INFO "%s: ida cleanup\n", __func__);
ida_free(&wiimote_ida, wdata->player_id);
+ }
// safe, debugfs checks IS_ERR_OR_NULL
wiidebug_deinit(wdata);
// safe, checks dev for NULL
device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
- if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
+ if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED){
+ printk(KERN_INFO "%s: hid_hw_close cleanup\n", __func__);
hid_hw_close(wdata->hdev);
- if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
+ }
+ if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED){
hid_hw_stop(wdata->hdev);
+ printk(KERN_INFO "%s: hid_hw_stop cleanup\n", __func__);
+ }
kfree(wdata);
}
DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
__wiimote_probe_cleanup(_T))
+static int debug_connect_iterate = 0;
+
static int wiimote_hid_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
int ret;
+ debug_connect_iterate++;
+
+ hid_info(hdev, "Probing wiimote iteration %d\n", debug_connect_iterate);
hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
- struct wiimote_data *wdata __free(wiimote_probe_cleanup) = wiimote_create(hdev);
+ struct wiimote_data *wdata __free(wiimote_probe_cleanup) = NULL;
+ if (debug_connect_iterate == 1)
+ return -ENOMEM;
+
+ wdata = wiimote_create(hdev);
if (!wdata) {
hid_err(hdev, "Can't alloc device\n");
return -ENOMEM;
}
+ if (debug_connect_iterate == 2)
+ return -ENOMEM;
+
ret = hid_parse(hdev);
if (ret) {
hid_err(hdev, "HID parse failed\n");
return ret;
}
+ if (debug_connect_iterate == 3)
+ return -ENOMEM;
+
ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
if (ret) {
hid_err(hdev, "HW start failed\n");
return ret;
}
wdata->probe_state |= WIIMOTE_PROBE_HW_STARTED;
+ if (debug_connect_iterate == 4)
+ return -ENOMEM;
ret = hid_hw_open(hdev);
if (ret) {
@@ -1832,22 +1857,30 @@ static int wiimote_hid_probe(struct hid_device *hdev,
return ret;
}
wdata->probe_state |= WIIMOTE_PROBE_HW_OPENED;
+ if (debug_connect_iterate == 5)
+ return -ENOMEM;
ret = device_create_file(&hdev->dev, &dev_attr_extension);
if (ret) {
hid_err(hdev, "cannot create sysfs attribute\n");
return ret;
}
+ if (debug_connect_iterate == 6)
+ return -ENOMEM;
ret = device_create_file(&hdev->dev, &dev_attr_devtype);
if (ret) {
hid_err(hdev, "cannot create sysfs attribute\n");
return ret;
}
+ if (debug_connect_iterate == 7)
+ return -ENOMEM;
ret = wiidebug_init(wdata);
if (ret)
return ret;
+ if (debug_connect_iterate == 8)
+ return -ENOMEM;
int player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
if (player_id < 1) {
@@ -1856,6 +1889,8 @@ static int wiimote_hid_probe(struct hid_device *hdev,
return ret;
}
wdata->player_id = player_id;
+ if (debug_connect_iterate == 9)
+ return -ENOMEM;
hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
--
2.53.0
^ permalink raw reply
* [PATCH v2 4/4] HID: wiimote: fix uaf when hid events are handled during destroy
From: Rafael Passos @ 2026-07-15 21:24 UTC (permalink / raw)
To: david, jikos, bentiss; +Cc: jkoolstra, Rafael Passos, linux-input
In-Reply-To: <20260715212501.3920033-1-rafael@rcpassos.me>
In wiimote_destroy, in the time between setting WIIPROTO_FLAG_EXITING
and hid_hw_stop, new events could be handled.
The new behaviour mirrors hid-playstation's output_worker_initialized
flag-gate (source of inspiration).
This issue was reported as a pre-existign race condition by sashiko.
I confirmed it is possible, but very unlikely. It could only happen if
shutdown is initiated by Linux, and the controller is being used.
Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
drivers/hid/hid-wiimote-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index b549a7d58d51..c9638ec352e4 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -92,6 +92,9 @@ static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
*/
spin_lock_irqsave(&wdata->queue.lock, flags);
+ /* Do not schedule work if controller is exiting */
+ if ((wdata->state.flags & WIIPROTO_FLAG_EXITING))
+ goto out_unlock;
memcpy(wdata->queue.outq[wdata->queue.head].data, buffer, count);
wdata->queue.outq[wdata->queue.head].size = count;
--
2.53.0
^ permalink raw reply related
* [PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes
From: Rafael Passos @ 2026-07-15 21:24 UTC (permalink / raw)
To: david, jikos, bentiss; +Cc: jkoolstra, Rafael Passos, linux-input
In-Reply-To: <20260715212501.3920033-1-rafael@rcpassos.me>
Cleanup code in wiimote/led probe function, using the scoped cleanup.
This prevents mistakes in future changes to this function.
In wiimote_probe_clenaup, a few functions are safe to call without
checking. For the hid_hw calls, a new bit mask was introduced to track
probing state.
Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
drivers/hid/hid-wiimote-core.c | 68 ++++++++++++++++++-------------
drivers/hid/hid-wiimote-modules.c | 17 ++++----
drivers/hid/hid-wiimote.h | 1 +
3 files changed, 48 insertions(+), 38 deletions(-)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index d37d740a7162..b549a7d58d51 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1774,16 +1774,40 @@ static void wiimote_destroy(struct wiimote_data *wdata)
/* Global id allocator for wii remotes */
static DEFINE_IDA(wiimote_ida);
+#define WIIMOTE_PROBE_HW_STARTED BIT(0) // hid_hw_start succeeded
+#define WIIMOTE_PROBE_HW_OPENED BIT(1) // hid_hw_open succeeded
+
+static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
+{
+ if (!wdata)
+ return;
+
+ if (wdata->player_id)
+ ida_free(&wiimote_ida, wdata->player_id);
+
+ // safe, debugfs checks IS_ERR_OR_NULL
+ wiidebug_deinit(wdata);
+ // safe, checks dev for NULL
+ device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
+ device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
+ if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
+ hid_hw_close(wdata->hdev);
+ if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
+ hid_hw_stop(wdata->hdev);
+ kfree(wdata);
+}
+
+DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
+ __wiimote_probe_cleanup(_T))
+
static int wiimote_hid_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
- struct wiimote_data *wdata;
int ret;
- int player_id;
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,68 +1816,54 @@ 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->probe_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->probe_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;
- player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
+ int player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
if (player_id < 1) {
hid_err(hdev, "cannot allocate controller id\n");
ret = player_id;
- goto err_free;
+ return ret;
}
-
wdata->player_id = player_id;
+
hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
/* schedule device detection */
wiimote_schedule(wdata);
-
+ 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)
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,
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index a53f72d5077e..6812efa589c9 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 probe_state;
union {
struct input_dev *input;
--
2.53.0
^ permalink raw reply related
* [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard
From: Rafael Passos @ 2026-07-15 21:24 UTC (permalink / raw)
To: david, jikos, bentiss; +Cc: jkoolstra, Rafael Passos, linux-input
In-Reply-To: <20260715212501.3920033-1-rafael@rcpassos.me>
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 | 222 +++++++++++++-----------------
drivers/hid/hid-wiimote-debug.c | 50 +++----
drivers/hid/hid-wiimote-modules.c | 7 +-
3 files changed, 121 insertions(+), 158 deletions(-)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 91c2cec35f6e..d37d740a7162 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)
@@ -680,9 +678,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
spin_unlock_irq(&wdata->state.lock);
/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
- scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+ scoped_guard(spinlock_irqsave, &wdata->state.lock)
wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
- }
return;
@@ -703,13 +700,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 +731,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 +742,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 +763,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 +774,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 +787,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 +874,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 +899,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 +951,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 +968,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 +993,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 +1019,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 +1049,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 +1108,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
wiimote_cmd_acquire_noint(wdata);
- spin_lock_irq(&wdata->state.lock);
-
- /* get state snapshot that we will then work on */
- flags = wdata->state.flags;
+ scoped_guard(spinlock_irq, &wdata->state.lock) {
- /* disable event forwarding temporarily */
- wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
- wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
+ /* get state snapshot that we will then work on */
+ flags = wdata->state.flags;
- spin_unlock_irq(&wdata->state.lock);
+ /* disable event forwarding temporarily */
+ wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
+ wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
+ }
/* init extension and MP (deactivates current extension or MP) */
wiimote_cmd_init_ext(wdata);
@@ -1152,9 +1139,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,27 +1178,25 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
mod_timer(&wdata->timer, jiffies + HZ * 4);
}
- spin_lock_irq(&wdata->state.lock);
-
- /* enable data forwarding again and set expected hotplug state */
- if (mp) {
- wdata->state.flags |= WIIPROTO_FLAG_MP_ACTIVE;
- if (wdata->state.exttype == WIIMOTE_EXT_NONE) {
- wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
- wdata->state.flags &= ~WIIPROTO_FLAG_MP_PLUGGED;
- } else {
- wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
- wdata->state.flags |= WIIPROTO_FLAG_MP_PLUGGED;
+ scoped_guard(spinlock_irq, &wdata->state.lock) {
+ /* enable data forwarding again and set expected hotplug state */
+ if (mp) {
+ wdata->state.flags |= WIIPROTO_FLAG_MP_ACTIVE;
+ if (wdata->state.exttype == WIIMOTE_EXT_NONE) {
+ wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
+ wdata->state.flags &= ~WIIPROTO_FLAG_MP_PLUGGED;
+ } else {
+ wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
+ wdata->state.flags |= WIIPROTO_FLAG_MP_PLUGGED;
+ wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
+ }
+ } else if (wdata->state.exttype != WIIMOTE_EXT_NONE) {
wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
}
- } else if (wdata->state.exttype != WIIMOTE_EXT_NONE) {
- wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
- }
-
- /* request status report for hotplug state updates */
- wiiproto_req_status(wdata);
- spin_unlock_irq(&wdata->state.lock);
+ /* request status report for hotplug state updates */
+ wiiproto_req_status(wdata);
+ }
hid_dbg(wdata->hdev, "detected extensions: MP: %d EXT: %d\n",
wdata->state.mp, wdata->state.exttype);
@@ -1244,11 +1228,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 +1619,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 +1626,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 +1645,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 +1696,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:
@@ -1774,14 +1749,11 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
static void wiimote_destroy(struct wiimote_data *wdata)
{
- unsigned long flags;
-
wiidebug_deinit(wdata);
/* prevent init_worker from being scheduled again */
- spin_lock_irqsave(&wdata->state.lock, flags);
- wdata->state.flags |= WIIPROTO_FLAG_EXITING;
- spin_unlock_irqrestore(&wdata->state.lock, flags);
+ scoped_guard(spinlock_irqsave, &wdata->state.lock)
+ wdata->state.flags |= WIIPROTO_FLAG_EXITING;
cancel_work_sync(&wdata->init_worker);
timer_shutdown_sync(&wdata->timer);
diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
index 5f74917781f2..df6f830ee791 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -7,6 +7,7 @@
/*
*/
+#include <linux/cleanup.h>
#include <linux/debugfs.h>
#include <linux/module.h>
#include <linux/seq_file.h>
@@ -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)
@@ -186,9 +181,8 @@ int wiidebug_init(struct wiimote_data *wdata)
dbg->drm = debugfs_create_file("drm", S_IRUSR,
dbg->wdata->hdev->debug_dir, dbg, &wiidebug_drm_fops);
- spin_lock_irqsave(&wdata->state.lock, flags);
- wdata->debug = dbg;
- spin_unlock_irqrestore(&wdata->state.lock, flags);
+ scoped_guard(spinlock_irqsave, &wdata->state.lock)
+ wdata->debug = dbg;
return 0;
@@ -197,14 +191,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.53.0
^ permalink raw reply related
* [PATCH v2 1/4] HID: wiimote: turn on the LEDs indicating the controller id
From: Rafael Passos @ 2026-07-15 21:24 UTC (permalink / raw)
To: david, jikos, bentiss; +Cc: jkoolstra, Rafael Passos, linux-input
In-Reply-To: <20260715212501.3920033-1-rafael@rcpassos.me>
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 | 54 ++++++++++++++++++++++++++++++----
drivers/hid/hid-wiimote.h | 1 +
2 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 63c4fa8fbb9b..91c2cec35f6e 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);
+
+ /* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
+ scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+ wiiproto_req_leds(wdata, player_leds[(wdata->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);
}
@@ -1786,11 +1799,15 @@ static void wiimote_destroy(struct wiimote_data *wdata)
kfree(wdata);
}
+/* Global id allocator for wii remotes */
+static DEFINE_IDA(wiimote_ida);
+
static int wiimote_hid_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
struct wiimote_data *wdata;
int ret;
+ int player_id;
hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
@@ -1834,7 +1851,16 @@ static int wiimote_hid_probe(struct hid_device *hdev,
if (ret)
goto err_free;
- hid_info(hdev, "New device registered\n");
+ player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
+ if (player_id < 1) {
+ hid_err(hdev, "cannot allocate controller id\n");
+ ret = player_id;
+ goto err_free;
+ }
+
+ wdata->player_id = player_id;
+
+ hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
/* schedule device detection */
wiimote_schedule(wdata);
@@ -1862,6 +1888,8 @@ static void wiimote_hid_remove(struct hid_device *hdev)
{
struct wiimote_data *wdata = hid_get_drvdata(hdev);
+ ida_free(&wiimote_ida, wdata->player_id);
+
hid_info(hdev, "Device removed\n");
wiimote_destroy(wdata);
}
@@ -1887,7 +1915,21 @@ static struct hid_driver wiimote_hid_driver = {
.remove = wiimote_hid_remove,
.raw_event = wiimote_hid_event,
};
-module_hid_driver(wiimote_hid_driver);
+
+
+static int __init wiimote_init(void)
+{
+ return hid_register_driver(&wiimote_hid_driver);
+}
+
+static void __exit wiimote_exit(void)
+{
+ hid_unregister_driver(&wiimote_hid_driver);
+ ida_destroy(&wiimote_ida);
+}
+
+module_init(wiimote_init);
+module_exit(wiimote_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index 9c12f63f6dd2..a53f72d5077e 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.53.0
^ permalink raw reply related
* [PATCH v2 0/4] HID: wiimote: new LED behavior on connect, scoped guards, uaf
From: Rafael Passos @ 2026-07-15 21:24 UTC (permalink / raw)
To: bentiss, david, jikos; +Cc: jkoolstra, Rafael Passos, linux-input
Hi,
This patchset contains one feature change, and two cleanup patches.
The feature is turning different LEDs for each of the first 4 wiimotes connected.
From id 5 forward, the LED will cycle back to 1, and so on.
This uses the ida struct, so its quite simple and lightweight.
The hid_info log message prints out the controller id.
While implementing this feature, I decided to cleanup the code using
scoped_guard for the many spinlocks in the driver. There are two places
where the original lock/unlock version fits best, and I left them
untouched.
Lastly, I used the __free scope cleanup in the wiimote and LED probe functions.
The LED is trivial. The wiimote_probe required a new state tracker bitmask.
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
(I will also send in response to this patch, what I used to test it).
(4/4) *new patch* :
- sashiko found a pre-existing uaf. Unlikely, but correct.
implemented using the playstation driver as an inspiration
Rafael Passos (4):
HID: wiimote: turn on the LEDs indicating the controller id
HID: wiimote: replace spinlock pairs with scoped_guard
HID: wiimote: use scoped cleanup in wiimote and led probes
HID: wiimote: fix uaf when hid events are handled during destroy
drivers/hid/hid-wiimote-core.c | 335 ++++++++++++++++--------------
drivers/hid/hid-wiimote-debug.c | 50 ++---
drivers/hid/hid-wiimote-modules.c | 24 +--
drivers/hid/hid-wiimote.h | 2 +
4 files changed, 215 insertions(+), 196 deletions(-)
--
2.53.0
^ permalink raw reply
* Re: [PATCH] Input: tca8418_keypad - enable overflow mode per datasheet (SCPS215G)
From: Dmitry Torokhov @ 2026-07-15 20:58 UTC (permalink / raw)
To: Zhian Liang; +Cc: linux-input, linux-kernel, stable
In-Reply-To: <20260529013900.43854-1-liangzhan5dev@gmail.com>
On Fri, May 29, 2026 at 09:39:00AM +0800, Zhian Liang wrote:
> The driver currently sets only the overflow interrupt enable bit
> (OVR_FLOW_IEN) in the configuration register, leaving the overflow
> mode bit (OVR_FLOW_M) at its default value of 0.
>
> According to the TCA8418 datasheet (SCPS215G, Section 8.6.4.1
> "Overflow Errata - Description"), both OVR_FLOW_M (Bit_5) and
> OVR_FLOW_IEN (Bit_3) must be set high for the overflow interrupt
> to be generated. If only OVR_FLOW_IEN is set, FIFO overflow events
> are silently lost without notifying the host.
>
> Fix this by setting OVR_FLOW_M alongside OVR_FLOW_IEN in the
> configuration register.
>
> Note: I do not have access to hardware to test this change.
> Testing by generating 11+ key events without reading the FIFO
> would be appreciated. Full handling of overflow events in the
> interrupt handler is left for future improvement.
>
> Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 3/3] HID: magicmouse: report charge status over Bluetooth
From: Jose Villaseñor Montfort @ 2026-07-15 20:03 UTC (permalink / raw)
To: Alec Hall
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Jose Villaseñor Montfort
In-Reply-To: <20260714101235.99447-4-signshop.alec@gmail.com>
Hi Alec,
Thanks, that all sounds good -- if mine lands first, great; the wrapper
split is exactly as you describe, so rebasing 3/3 is just relocating your
report-check hunk.
On the NULL msc->input deref: good timing -- I actually already have a fix
for that one ready, from digging into the same Sashiko review on your
series. Rather than a guard in the callbacks, I went at the root cause in
magicmouse_probe(): the USB Magic Mouse 2 / Trackpad 2 path returns 0
before the existing "input not registered" check, so I moved that check
ahead of the early return. That rejects a device that bound without an
input and covers both ->raw_event and ->event in one go.
I just sent it as a separate patch so we don't duplicate -- happy for you
to review, and shout if you'd rather take it instead.
Thanks,
Jose
^ permalink raw reply
* [PATCH] HID: magicmouse: reject devices that bind without an input device
From: Jose Villaseñor Montfort @ 2026-07-15 19:58 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Jose Villaseñor Montfort
magicmouse_raw_event() and magicmouse_event() dereference msc->input
(e.g. input->id.product, and via magicmouse_emit_touch() and
magicmouse_emit_buttons()) without checking it for NULL. hid-input only
sets msc->input when the device exposes a usable input device.
magicmouse_probe() guards against this with an "input not registered"
check that fails the probe when msc->input is NULL -- but the USB Magic
Mouse 2 / Magic Trackpad 2 path returns 0 before reaching that check.
A device that binds this driver on that path (for example a malicious
one spoofing an Apple VID/PID) with a report descriptor that does not
produce an input device therefore ends up bound with msc->input == NULL.
A subsequent input report then dereferences the NULL pointer in the
->raw_event / ->event callbacks and panics the kernel.
Move the msc->input check ahead of the early return so it covers every
bind path. Legitimate devices register an input during hid_hw_start()
and are unaffected.
Fixes: 0b91b4e4dae6 ("HID: magicmouse: Report battery level over USB")
Link: https://lore.kernel.org/linux-input/20260714102540.3EB2E1F000E9@smtp.kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Jose Villaseñor Montfort <pepemontfort@gmail.com>
---
Surfaced by an automated review of Alec Hall's parallel battery series
(the Link: above), independent of that work. This is a sibling hardening
fix to "HID: magicmouse: prevent unbounded recursion in
magicmouse_raw_event()" [1], which touches the same driver.
I went with fixing the probe path (rejecting a bind without an input)
rather than adding per-callback "if (!msc->input) return 0;" guards,
since a single check at probe covers both ->raw_event and ->event and
addresses the root asymmetry. Happy to switch to per-callback guards if
reviewers prefer that.
[1] https://lore.kernel.org/linux-input/20260715053526.574725-1-pepemontfort@gmail.com/
drivers/hid/hid-magicmouse.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 97562765a..bd6a12e40 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -923,17 +923,25 @@ static int magicmouse_probe(struct hid_device *hdev,
magicmouse_fetch_battery(hdev);
}
- if (is_usb_magicmouse2(id->vendor, id->product) ||
- (is_usb_magictrackpad2(id->vendor, id->product) &&
- hdev->type != HID_TYPE_USBMOUSE))
- return 0;
-
+ /*
+ * The ->raw_event and ->event callbacks dereference msc->input, which
+ * hid-input only populates when the device exposes a usable input.
+ * Reject a device that bound without one -- including on the USB Magic
+ * Mouse 2 / Trackpad 2 path that returns early below -- so a device
+ * (e.g. one spoofing an Apple VID/PID) cannot drive those callbacks
+ * into a NULL pointer dereference.
+ */
if (!msc->input) {
hid_err(hdev, "magicmouse input not registered\n");
ret = -ENOMEM;
goto err_stop_hw;
}
+ if (is_usb_magicmouse2(id->vendor, id->product) ||
+ (is_usb_magictrackpad2(id->vendor, id->product) &&
+ hdev->type != HID_TYPE_USBMOUSE))
+ return 0;
+
switch (id->product) {
case USB_DEVICE_ID_APPLE_MAGICMOUSE:
report = hid_register_report(hdev, HID_INPUT_REPORT, MOUSE_REPORT_ID, 0);
--
2.55.0
^ permalink raw reply related
* Re: [PATCH 3/3] HID: magicmouse: report charge status over Bluetooth
From: Alec Hall @ 2026-07-15 19:22 UTC (permalink / raw)
To: Jose Villaseñor Montfort, linux-input
Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel, Alec Hall
In-Reply-To: <20260715062056.616625-1-pepemontfort@gmail.com>
Hi Jose,
Thanks for the heads-up, and for fixing this.
> whichever goes in first the other needs a trivial rebase [...] I'm happy to
> rebase mine on top of your series, or the maintainers can take them in
> either order
Agreed, it's trivial either way. My 3/3 only adds a battery-report check and
its call near the top of magicmouse_raw_event(), so rebasing it onto your
__magicmouse_raw_event()/wrapper split is just moving that hunk. I'm fine with
whatever order is least work for the maintainers -- if yours lands first I'll
respin 3/3 on top.
For what it's worth, your recursion bound is one of two issues an automated
review (Sashiko) raised on my series; the other was a potential NULL
msc->input dereference in the same function (a device that takes the early
return in magicmouse_probe() leaves msc->input NULL, then a raw report
dereferences it). I'm happy to send a small guard for that as a follow-up so
between us both are covered -- and I'll coordinate the context so we don't
collide again.
Thanks,
Alec
^ permalink raw reply
* Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: Dmitry Torokhov @ 2026-07-15 18:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Hari Mishal, Amit Shah, Arnd Bergmann, Greg Kroah-Hartman,
Gerd Hoffmann, Jason Wang, David Hildenbrand, Henrik Rydberg,
Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input
In-Reply-To: <20260715120911-mutt-send-email-mst@kernel.org>
On Wed, Jul 15, 2026 at 12:11:12PM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2026 at 06:07:56PM +0200, Hari Mishal wrote:
> > On Wed, Jul 15, 2026 at 5:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> > > > nslots is derived from the ABS_MT_SLOT maximum reported by the
> > > > virtio device. A device could report a bogus maximum (e.g. -1)
> > > > making nslots = 0, which input_mt_init_slots() does not reject;
> > > > it returns success without allocating any slot storage, silently
> > > > leaving the device registered as multitouch capable with no
> > > > backing state.
> > >
> > > So let's disable multitouch instead?
> > >
> >
> > So rather than failing the whole probe, just warn and clear
> > ABS_MT_SLOT from absbit in that case, so the rest of the device
> > still registers? I took my lead from input_mt_init_slots(), which
> > has its own internal cap and returns -EINVAL when the device
> > reports more than 1024 slots.
> > Shall I modify that case to get the same "warn and disable
> > multitouch" for consistency, or is a hard failure better there since
> > it's a different type of bad device data?
>
> I don't really know enough for sure but generally if the device can
> kinda work it's better than not working, and adding
> a capability to a device that driver can't use should
> generally just ignore the capability, not fail probe.
What is the failure mode if we keep the ABS_MT_SLOT capability? Does the
kernel crash? And if this can cause crash then we should fix
input_mt_init_slots() to reject requests for 0 slots with -EINVAL.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v2 2/4] virtio_input: validate device-reported multitouch slot count
From: Hari Mishal @ 2026-07-15 16:41 UTC (permalink / raw)
To: Gerd Hoffmann, Michael S . Tsirkin, Jason Wang
Cc: Greg Kroah-Hartman, Xuan Zhuo, Eugenio Pérez, Henrik Rydberg,
virtualization, linux-kernel, linux-input, Hari Mishal
In-Reply-To: <20260715142337.22811-3-harimishal1@gmail.com>
nslots is derived from the ABS_MT_SLOT maximum reported by the virtio
device. A device could report a bogus maximum (e.g. -1) making nslots <=
0, and input_mt_init_slots() can independently fail too (e.g. it rejects
slot counts over 1024). Neither case was handled here, and
input_mt_init_slots() failing took down the whole probe, losing the rest
of the input device over a problem isolated to multitouch.
Instead of failing the probe, warn and disable the ABS_MT_SLOT
capability in either case, and let the rest of the device register
normally. This recovery assumes input_mt_init_slots() is called with
flags == 0: with nonzero flags it can fail after already mutating the
input_dev (e.g. via input_set_abs_params()), which this recovery does
not account for.
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
v2: per review, disable multitouch instead of failing the whole
probe when the reported slot count is invalid or
input_mt_init_slots() otherwise fails.
drivers/virtio/virtio_input.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index deec24e8e682..786f5150724f 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -312,9 +312,25 @@ static int virtinput_probe(struct virtio_device *vdev)
if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
- err = input_mt_init_slots(vi->idev, nslots, 0);
- if (err)
- goto err_mt_init_slots;
+ if (nslots <= 0) {
+ dev_warn(&vdev->dev,
+ "invalid multitouch slot count %d, disabling multitouch\n",
+ nslots);
+ __clear_bit(ABS_MT_SLOT, vi->idev->absbit);
+ } else {
+ /*
+ * flags is 0: input_mt_init_slots() can only fail
+ * before touching *idev, so the recovery below is
+ * a full rollback.
+ */
+ err = input_mt_init_slots(vi->idev, nslots, 0);
+ if (err) {
+ dev_warn(&vdev->dev,
+ "failed to init %d multitouch slots (%d), disabling multitouch\n",
+ nslots, err);
+ __clear_bit(ABS_MT_SLOT, vi->idev->absbit);
+ }
+ }
}
}
@@ -331,7 +347,6 @@ static int virtinput_probe(struct virtio_device *vdev)
spin_lock_irqsave(&vi->lock, flags);
vi->ready = false;
spin_unlock_irqrestore(&vi->lock, flags);
-err_mt_init_slots:
input_free_device(vi->idev);
err_input_alloc:
vdev->config->del_vqs(vdev);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: Michael S. Tsirkin @ 2026-07-15 16:11 UTC (permalink / raw)
To: Hari Mishal
Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Jason Wang, David Hildenbrand, Henrik Rydberg, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, linux-input
In-Reply-To: <CAMmC+=AnE-p7B=DbBPYeNzJdcdiBf7q=5bfcuV0VQrPRK+pmug@mail.gmail.com>
On Wed, Jul 15, 2026 at 06:07:56PM +0200, Hari Mishal wrote:
> On Wed, Jul 15, 2026 at 5:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> > > nslots is derived from the ABS_MT_SLOT maximum reported by the
> > > virtio device. A device could report a bogus maximum (e.g. -1)
> > > making nslots = 0, which input_mt_init_slots() does not reject;
> > > it returns success without allocating any slot storage, silently
> > > leaving the device registered as multitouch capable with no
> > > backing state.
> >
> > So let's disable multitouch instead?
> >
>
> So rather than failing the whole probe, just warn and clear
> ABS_MT_SLOT from absbit in that case, so the rest of the device
> still registers? I took my lead from input_mt_init_slots(), which
> has its own internal cap and returns -EINVAL when the device
> reports more than 1024 slots.
> Shall I modify that case to get the same "warn and disable
> multitouch" for consistency, or is a hard failure better there since
> it's a different type of bad device data?
I don't really know enough for sure but generally if the device can
kinda work it's better than not working, and adding
a capability to a device that driver can't use should
generally just ignore the capability, not fail probe.
> Happy to fix it either way! Just want to confirm before sending V2.
>
> Cheers,
> Hari
^ permalink raw reply
* Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: Hari Mishal @ 2026-07-15 16:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Jason Wang, David Hildenbrand, Henrik Rydberg, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, linux-input
In-Reply-To: <20260715115018-mutt-send-email-mst@kernel.org>
On Wed, Jul 15, 2026 at 5:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> > nslots is derived from the ABS_MT_SLOT maximum reported by the
> > virtio device. A device could report a bogus maximum (e.g. -1)
> > making nslots = 0, which input_mt_init_slots() does not reject;
> > it returns success without allocating any slot storage, silently
> > leaving the device registered as multitouch capable with no
> > backing state.
>
> So let's disable multitouch instead?
>
So rather than failing the whole probe, just warn and clear
ABS_MT_SLOT from absbit in that case, so the rest of the device
still registers? I took my lead from input_mt_init_slots(), which
has its own internal cap and returns -EINVAL when the device
reports more than 1024 slots.
Shall I modify that case to get the same "warn and disable
multitouch" for consistency, or is a hard failure better there since
it's a different type of bad device data?
Happy to fix it either way! Just want to confirm before sending V2.
Cheers,
Hari
^ permalink raw reply
* Re: [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend
From: Frank Li @ 2026-07-15 16:01 UTC (permalink / raw)
To: joy.zou
Cc: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li,
imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260715-b4-pwrkey-v5-4-07e7353c319e@oss.nxp.com>
On Wed, Jul 15, 2026 at 05:33:54PM +0800, joy.zou@oss.nxp.com wrote:
> From: Joy Zou <joy.zou@nxp.com>
>
> The driver implements debounce protection using a timer-based mechanism:
> when a key interrupt occurs, a timer is scheduled to verify the key state
> after DEBOUNCE_TIME before reporting the event. This works well during
> normal operation.
>
> However, key press events can be lost during system resume on platforms
> like i.MX8MQ-EVK because:
> 1. During the no_irq resume phase, PCIe driver restoration can take up to
> 200ms with IRQs disabled.
> 2. The power key interrupt remains pending during the no_irq phase.
> 3. If the key is released before IRQs are re-enabled, the timer eventually
> runs but sees the key as released and skips reporting the event.
>
> To prevent event loss during system suspend, set a pending_press flag in
> the interrupt handler and report the press event from the timer callback
> when the flag is set. This avoids out-of-order event delivery and keeps
> the existing timer-based debounce mechanism for normal operation.
>
> Signed-off-by: Joy Zou <joy.zou@nxp.com>
> ---
> Changes for v5:
> 1. Replace SIMPLE_DEV_PM_OPS with DEFINE_SIMPLE_DEV_PM_OPS and remove
> __maybe_unused from suspend/resume callbacks.
> 2. Use pm_ptr() to wrap pm_ops pointer in platform_driver.
> 3. Replace suspended flag check in interrupt handler with a pending_press
> latch: set pending_press in hardirq context, consume and report the
> press event from the timer callback in softirq context.
>
> Changes for v3:
> 1. Add spinlock for pdata->keystate and pdata->suspended per AI review
> comments.
> 2. Replace hardcode value 1 with local variable keystate in input_report_key()
> under suspended.
>
> Changes for v2:
> 1. Add a boolean variable suspended and PM callback functions to replace
> the use of the is_suspended field per AI review comments.
> 2. Move event report handle to else branch in suspended state, since the
> pdata->minor_rev == 0 branch has no debounce detection per AI review
> comments.
> 3. Modify the commit message.
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> drivers/input/keyboard/snvs_pwrkey.c | 72 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index cbe44a38d2b3..970c7d65bd57 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -39,6 +39,9 @@ struct pwrkey_drv_data {
> int keycode;
> int keystate; /* 1:pressed */
> int wakeup;
> + bool suspended; /* Track suspend state */
> + bool pending_press; /* Key pressed during suspend, report from timer callback */
> + spinlock_t lock; /* Protects keystate, suspended and pending_press */
> struct timer_list check_timer;
> struct input_dev *input;
> u8 minor_rev;
> @@ -49,14 +52,38 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
> struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
> check_timer);
> struct input_dev *input = pdata->input;
> + bool state_changed = false;
> + bool pending_press;
> u32 state;
>
> regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
> state = state & SNVS_HPSR_BTN ? 1 : 0;
>
> - /* only report new event if status changed */
> - if (state ^ pdata->keystate) {
> - pdata->keystate = state;
> + scoped_guard(spinlock_irqsave, &pdata->lock) {
> + pending_press = pdata->pending_press;
> + if (pending_press) {
> + pdata->pending_press = false;
> + pdata->keystate = 1;
> + }
> + /* only report new event if status changed */
> + if (state ^ pdata->keystate) {
> + pdata->keystate = state;
> + state_changed = true;
> + }
> + }
> +
> + /*
> + * Report a press event latched during suspend. If the key is still
> + * held, state_changed will be 0 (keystate already set to 1 above),
> + * so no duplicate press is reported. If already released,
> + * state_changed will fire next to report the release.
> + */
> + if (pending_press) {
> + input_report_key(input, pdata->keycode, 1);
> + input_sync(input);
> + }
> +
> + if (state_changed) {
> input_event(input, EV_KEY, pdata->keycode, state);
> input_sync(input);
> pm_relax(pdata->input->dev.parent);
> @@ -92,8 +119,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
> input_sync(input);
> pm_relax(input->dev.parent);
> } else {
> + /*
> + * If the key is pressed during suspend, latch it so
> + * the timer callback can report the press event in
> + * softirq context, avoiding out-of-order events.
> + */
> + scoped_guard(spinlock_irqsave, &pdata->lock) {
> + if (pdata->suspended)
> + pdata->pending_press = true;
> + }
> mod_timer(&pdata->check_timer,
> - jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> + jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> }
> }
>
> @@ -151,6 +187,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> if (pdata->irq < 0)
> return pdata->irq;
>
> + spin_lock_init(&pdata->lock);
> error = of_property_read_u32(np, "power-off-time-sec", &val);
> if (!error) {
> switch (val) {
> @@ -217,6 +254,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return 0;
> }
>
> +static int imx_snvs_pwrkey_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +
> + scoped_guard(spinlock_irqsave, &pdata->lock)
> + pdata->suspended = true;
> +
> + return 0;
> +}
> +
> +static int imx_snvs_pwrkey_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +
> + scoped_guard(spinlock_irqsave, &pdata->lock)
> + pdata->suspended = false;
> +
> + return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
> + imx_snvs_pwrkey_suspend,
> + imx_snvs_pwrkey_resume);
> +
> static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> { .compatible = "fsl,sec-v4.0-pwrkey" },
> { /* sentinel */ }
> @@ -227,6 +290,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
> .driver = {
> .name = "snvs_pwrkey",
> .of_match_table = imx_snvs_pwrkey_ids,
> + .pm = pm_ptr(&imx_snvs_pwrkey_pm_ops),
> },
> .probe = imx_snvs_pwrkey_probe,
> };
>
> --
> 2.34.1
>
>
^ permalink raw reply
* Re: [PATCH 1/4] virtio-mem: validate device-reported block size
From: Michael S. Tsirkin @ 2026-07-15 15:57 UTC (permalink / raw)
To: Hari Mishal
Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Jason Wang, David Hildenbrand, Henrik Rydberg, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, linux-input
In-Reply-To: <20260715142337.22811-2-harimishal1@gmail.com>
On Wed, Jul 15, 2026 at 04:22:40PM +0200, Hari Mishal wrote:
> The device_block_size read from the virtio-mem config space is used
> as a divisor and also in ALIGN_DOWN() further down the code path in
> the driver without further validation. A zero value leads to a division
> by zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
> arithmetic leading to a misreporting of guest usable guest ram, post
> crash. Reject both at init time instead of trusting the device.
>
> Assisted-by: gkh_clanker:t1000
> Signed-off-by: Hari Mishal <harimishal1@gmail.com>
> ---
> drivers/virtio/virtio_mem.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 11c441501582..43d12ec7c323 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2847,6 +2847,13 @@ static int virtio_mem_init(struct virtio_mem *vm)
> &vm->plugged_size);
> virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
> &vm->device_block_size);
> + if (!vm->device_block_size ||
> + !is_power_of_2(vm->device_block_size)) {
0 is not a power of 2, why do we need to check twice?
> + dev_err(&vm->vdev->dev,
> + "invalid device block size: 0x%llx\n",
> + (unsigned long long)vm->device_block_size);
> + return -EINVAL;
> + }
> virtio_cread_le(vm->vdev, struct virtio_mem_config, node_id,
> &node_id);
> vm->nid = virtio_mem_translate_node_id(vm, node_id);
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: Michael S. Tsirkin @ 2026-07-15 15:50 UTC (permalink / raw)
To: Hari Mishal
Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, Gerd Hoffmann,
Jason Wang, David Hildenbrand, Henrik Rydberg, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, linux-input
In-Reply-To: <20260715142337.22811-3-harimishal1@gmail.com>
On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> nslots is derived from the ABS_MT_SLOT maximum reported by the
> virtio device. A device could report a bogus maximum (e.g. -1)
> making nslots = 0, which input_mt_init_slots() does not reject;
> it returns success without allocating any slot storage, silently
> leaving the device registered as multitouch capable with no
> backing state.
So let's disable multitouch instead?
> Reject non-positive slot counts before calling
> input_mt_init_slots().
>
> Assisted-by: gkh_clanker:t1000
> Signed-off-by: Hari Mishal <harimishal1@gmail.com>
> ---
> drivers/virtio/virtio_input.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e682..2cc19782cdd3 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev)
>
> if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
> nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
> + if (nslots <= 0) {
> + err = -EINVAL;
> + goto err_mt_init_slots;
> + }
> err = input_mt_init_slots(vi->idev, nslots, 0);
> if (err)
> goto err_mt_init_slots;
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH 1/4] virtio-mem: validate device-reported block size
From: sashiko-bot @ 2026-07-15 14:43 UTC (permalink / raw)
To: Hari Mishal; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715142337.22811-2-harimishal1@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing minimum threshold check for `device_block_size` leads to silent truncation of `nb_vm_blocks` to 16 bits, causing guest memory corruption.
--
--- Patch [1]: [PATCH 1/4] virtio-mem: validate device-reported block size ---
commit 1886b5144897e316d9afb5456f7874735bcb4c19
Author: Hari Mishal <harimishal1@gmail.com>
virtio-mem: validate device-reported block size
This commit adds validation to ensure the device-reported block size is
non-zero and a power of two, preventing division by zero and miscalculated
alignments.
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 11c4415015829..43d12ec7c3230 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2847,6 +2847,13 @@ static int virtio_mem_init(struct virtio_mem *vm)
> &vm->plugged_size);
> virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
> &vm->device_block_size);
> + if (!vm->device_block_size ||
> + !is_power_of_2(vm->device_block_size)) {
> + dev_err(&vm->vdev->dev,
> + "invalid device block size: 0x%llx\n",
> + (unsigned long long)vm->device_block_size);
> + return -EINVAL;
> + }
[Severity: High]
This is a pre-existing issue, but does this validation also need to enforce
a minimum threshold for device_block_size?
If a small power-of-two value like 16 or 32 is provided by the device, the
calculated block count can easily exceed 65535.
For example, a minimum sub-block size of 2MB divided by 16 yields 131,072
blocks. When this is passed to virtio_mem_send_plug_request(), the value is
silently truncated to 16 bits:
drivers/virtio/virtio_mem.c:virtio_mem_send_plug_request() {
...
.u.plug.nb_blocks = cpu_to_virtio16(vm->vdev, nb_vm_blocks),
...
}
This truncation also happens in virtio_mem_send_unplug_request() and
virtio_mem_send_state_request() via the .u.unplug.nb_blocks and
.u.state.nb_blocks assignments.
Could this lead to the host processing a truncated request (e.g., plugging 0
blocks) while the guest kernel proceeds to add the full memory region,
causing a crash when the unbacked memory is accessed?
> virtio_cread_le(vm->vdev, struct virtio_mem_config, node_id,
> &node_id);
> vm->nid = virtio_mem_translate_node_id(vm, node_id);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142337.22811-1-harimishal1@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 4/4] virtio_console: take a kref in find_port_by_vq() to fix port UAF
From: sashiko-bot @ 2026-07-15 14:38 UTC (permalink / raw)
To: Hari Mishal; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715142337.22811-5-harimishal1@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix: `in_intr()` can still dereference a freed `hvc_struct` (`port->cons.hvc`) due to a race with `unplug_port()`.
- [High] A TOCTOU data race on `port->portdev` between `in_intr()` and `unplug_port()` can lead to a NULL pointer dereference.
- [High] File operations (e.g., `port_fops_read`, `port_fops_write`) can access freed virtqueue and portdev memory if device removal occurs concurrently.
--
--- Patch [4]: [PATCH 4/4] virtio_console: take a kref in find_port_by_vq() to fix port UAF ---
commit 135dcd5e5260baeaa35a46f85c302542d073ddaf
Author: Hari Mishal <harimishal1@gmail.com>
virtio_console: take a kref in find_port_by_vq() to fix port UAF
This commit fixes a use-after-free of the port structure by taking a
kref in find_port_by_vq() under the ports_lock. It matches the behavior
of find_port_by_devt_in_portdev() and safely drops the reference at the
end of the interrupt handlers.
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index faef362dae853..1b7593684ed9d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
[ ... ]
> @@ -1723,6 +1731,7 @@ static void in_intr(struct virtqueue *vq)
> if (!port->portdev) {
> /* Port is being unplugged, ignore further data. */
> spin_unlock_irqrestore(&port->inbuf_lock, flags);
> + kref_put(&port->kref, remove_port);
> return;
> }
[Severity: High]
This is a pre-existing issue, but is there a time-of-check to time-of-use
(TOCTOU) data race on port->portdev here?
In unplug_port(), port->portdev is locklessly set to NULL at the end of the
function. Concurrently, in_intr() checks if (!port->portdev) while holding
inbuf_lock. If the check passes before unplug_port() sets it to NULL,
in_intr() will proceed and later execute this check:
if (!port->guest_connected && !is_rproc_serial(port->portdev->vdev))
Could this force a reload of port->portdev, read NULL, and cause a kernel
panic when accessing ->vdev?
[ ... ]
> @@ -1756,6 +1765,8 @@ static void in_intr(struct virtqueue *vq)
>
> if (is_console_port(port) && hvc_poll(port->cons.hvc))
> hvc_kick();
> +
> + kref_put(&port->kref, remove_port);
> }
[Severity: High]
This is a pre-existing issue, but could in_intr() still dereference a freed
hvc_struct due to a race with unplug_port()?
When the host sends a VIRTIO_CONSOLE_PORT_REMOVE control message,
unplug_port() calls hvc_remove(port->cons.hvc), freeing the hvc_struct.
Concurrently, if the host kicks the data virtqueue, in_intr() passes the
portdev check, drops inbuf_lock, and evaluates:
if (is_console_port(port) && hvc_poll(port->cons.hvc))
Since port->cons.hvc is never cleared and this check is lockless, can this
dereference the freed memory and lead to memory corruption?
[Severity: High]
This is also a pre-existing issue, but can file operations like
port_fops_write() access freed virtqueue and portdev memory if device removal
occurs concurrently?
If a user-space process initiates port_fops_write(), it eventually executes:
buf = alloc_buf(port->portdev->vdev, count, 0);
Concurrently, if the host removes the virtio device, virtcons_remove() is
called. It unplugs the ports, calls remove_vqs(portdev), and frees portdev
via kfree(portdev) without waiting for existing open file descriptors to be
closed.
Even with the kref protecting struct port, could the file operation resume,
pass checks, and access port->portdev->vdev or call functions on freed
memory like virtqueue_add_outbuf() on port->out_vq?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142337.22811-1-harimishal1@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr()
From: sashiko-bot @ 2026-07-15 14:37 UTC (permalink / raw)
To: Hari Mishal; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260715142337.22811-4-harimishal1@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The newly introduced `!port->portdev` check in `in_intr()` suffers from a Time-Of-Check to Time-Of-Use (TOCTOU) race condition and fails to robustly prevent the NULL pointer dereference.
- [Medium] Bailing out early from `in_intr()` without consuming the used virtqueue buffers leaves stale data in the queue, contaminating future port instances that reuse the same ID.
Pre-existing issues:
- [High] The patch fails to address a fundamental Use-After-Free (UAF) vulnerability in `in_intr()` and `out_intr()`. The helper `find_port_by_vq()` returns a `port` without taking a reference count, allowing concurrent hot-unplug to free the `port` while interrupt handlers are executing.
--
--- Patch [3]: [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr() ---
commit 53028db078d1834cd8a61fa35f5ddb4495c447b8
Author: Hari Mishal <harimishal1@gmail.com>
virtio_console: avoid NULL portdev dereference in in_intr()
This commit attempts to fix a null pointer dereference in in_intr() caused
when a port is hot-unplugged. It adds an early exit check for portdev being
null under the inbuf_lock to ignore further data during the unplug window.
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 198b973141680..faef362dae853 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
[ ... ]
> @@ -1720,6 +1720,11 @@ static void in_intr(struct virtqueue *vq)
> }
>
> spin_lock_irqsave(&port->inbuf_lock, flags);
[Severity: High]
This is a pre-existing issue, but does find_port_by_vq() return the port
pointer without acquiring a reference via kref_get()?
If unplug_port() runs concurrently, can it drop the final reference and free
the port structure right before we try to take the lock here?
CPU0 CPU1
in_intr() unplug_port()
port = find_port_by_vq(...)
kref_put(&port->kref, remove_port);
spin_lock_irqsave(...)
Would this result in a use-after-free on the port structure?
> + if (!port->portdev) {
[Severity: High]
Does this check reliably prevent a null pointer dereference?
Since unplug_port() sets port->portdev = NULL locklessly outside of
inbuf_lock, could a concurrent unplug set it to null immediately after this
check passes?
Because get_inbuf() is called next, might the compiler reload port->portdev
from memory for the later is_rproc_serial(port->portdev->vdev) check,
crashing on the newly null pointer?
> + /* Port is being unplugged, ignore further data. */
> + spin_unlock_irqrestore(&port->inbuf_lock, flags);
> + return;
[Severity: Medium]
Does returning here without consuming the virtqueue buffers leave stale data
in the queue?
Since virtqueues persist across hot-unplugs for the same port ID, could a
newly hot-plugged port reuse this virtqueue and read leaked data from the
previous session?
Should this path consume the buffers, perhaps by calling
flush_bufs(vq, false), before returning?
> + }
> port->inbuf = get_inbuf(port);
>
> /*
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142337.22811-1-harimishal1@gmail.com?part=3
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox