* Re: [PATCH] HID: nintendo: relax subcommand limiter until cadence is proven
2026-09-08 22:45 [PATCH] HID: nintendo: relax subcommand limiter until cadence is proven Mike Lothian
@ 2026-09-08 22:58 ` sashiko-bot
2026-09-08 23:43 ` [PATCH v2] " Mike Lothian
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-09-08 22:58 UTC (permalink / raw)
To: Mike Lothian; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Concurrent unsynchronized access to `subcmd_rate_exhaustions` and `subcmd_rate_relaxed` permanently breaks the rate limiter fallback mechanism.
- [Medium] USB controllers permanently bypass the transition to the strict rate limiter due to an unconditional override of `consecutive_valid_report_deltas`.
Pre-existing issues:
- [High] Probe error paths fail to set `ctlr_state` to `REMOVED`, leading to unsafe I/O on stopped hardware during workqueue flush.
--
commit 4cc815d1cbd63a9a8e0cd2d3dbc8b1bf1a713dfe
Author: Mike Lothian <mike@fireburn.co.uk>
HID: nintendo: relax subcommand limiter until cadence is proven
This commit modifies the initialization of the nintendo HID driver to
start each connection on the legacy flat-delay throttle instead of the
cadence-gated one. It transitions controllers to the strict limiter once
they demonstrate a valid reporting cadence, preventing warnings caused by
early rumble packets being sent before cadence is established.
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 43e0f2aaea3b5..0a169d0aa7195 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -1797,8 +1797,12 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
> */
> if (report_delta_ms >= JC_INPUT_REPORT_MIN_DELTA &&
> report_delta_ms <= JC_INPUT_REPORT_MAX_DELTA) {
> - if (ctlr->consecutive_valid_report_deltas < JC_SUBCMD_VALID_DELTA_REQ)
> + if (ctlr->consecutive_valid_report_deltas < JC_SUBCMD_VALID_DELTA_REQ) {
> ctlr->consecutive_valid_report_deltas++;
[Severity: Medium]
Will this permanently bypass the transition to the strict rate limiter for
USB controllers?
For USB devices, a preexisting block at the end of joycon_parse_report()
unconditionally forces consecutive_valid_report_deltas to
JC_SUBCMD_VALID_DELTA_REQ (which is 3).
On all subsequent input reports, the delta count is already 3, causing the
if check above to fail. The logic that sets subcmd_rate_relaxed to false
would be entirely bypassed, leaving USB controllers stuck on the legacy
flat-delay rate limiter.
> + if (ctlr->consecutive_valid_report_deltas == JC_SUBCMD_VALID_DELTA_REQ &&
> + ctlr->subcmd_rate_exhaustions < JC_SUBCMD_RATE_MAX_FAILURES)
> + ctlr->subcmd_rate_relaxed = false;
[Severity: High]
Could this result in a data race that permanently breaks the rate limiter
fallback mechanism?
joycon_enforce_subcmd_rate_strict() modifies subcmd_rate_exhaustions and
subcmd_rate_relaxed from a worker thread holding output_mutex but not
ctlr->lock:
joycon_enforce_subcmd_rate_strict() {
...
if (++ctlr->subcmd_rate_exhaustions == JC_SUBCMD_RATE_MAX_FAILURES) {
ctlr->subcmd_rate_relaxed = true;
...
}
Meanwhile, the new code here in joycon_parse_report() executes in softirq
context holding ctlr->lock but not output_mutex.
If joycon_parse_report() reads a stale value of exhaustions (e.g., 3) just
before the strict limiter increments it to the maximum (4), the softirq
evaluates the condition as true. If the softirq then writes false to
relaxed immediately after the strict limiter wrote true, the flag becomes
false while exhaustions is 4. Subsequent failures will increment exhaustions
past the maximum, meaning relaxed = true will never be executed again.
> + }
> } else {
> ctlr->consecutive_valid_report_deltas = 0;
> }
[ ... ]
> @@ -2730,6 +2734,10 @@ static int nintendo_hid_probe(struct hid_device *hdev,
>
> ctlr->hdev = hdev;
> ctlr->ctlr_state = JOYCON_CTLR_STATE_INIT;
> + /* Promoted to the strict limiter once cadence is proven, see
> + * joycon_parse_report().
> + */
> + ctlr->subcmd_rate_relaxed = true;
[Severity: High]
This is a pre-existing issue, but does the probe error path fail to set
ctlr_state to JOYCON_CTLR_STATE_REMOVED, leading to unsafe I/O on stopped
hardware during the workqueue flush?
If nintendo_hid_probe() fails after hid_device_io_start() (e.g. inside
joycon_leds_create()), an incoming input report can queue the rumble worker.
The error path then calls hid_hw_stop(), tearing down the transport.
The error path subsequently calls destroy_workqueue(ctlr->rumble_queue),
which synchronously flushes and executes the queued worker:
nintendo_hid_probe() {
...
err_stop:
hid_hw_stop(hdev);
err_wq:
destroy_workqueue(ctlr->rumble_queue);
...
}
Because the error path never updated ctlr_state, the worker bypasses the
safety guard in joycon_send_rumble_data() and unconditionally calls
__joycon_hid_send(), invoking hid_hw_output_report() on the already
stopped hardware. Executing hardware I/O on a stopped and partially
dismantled HID low-level transport can cause Use-After-Free, NULL pointer
dereferences, or kernel panics depending on the underlying transport.
> ctlr->rumble_queue_head = 0;
> ctlr->rumble_queue_tail = 0;
> hid_set_drvdata(hdev, ctlr);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908224532.114357-1-mike@fireburn.co.uk?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v2] HID: nintendo: relax subcommand limiter until cadence is proven
2026-09-08 22:45 [PATCH] HID: nintendo: relax subcommand limiter until cadence is proven Mike Lothian
2026-09-08 22:58 ` sashiko-bot
@ 2026-09-08 23:43 ` Mike Lothian
1 sibling, 0 replies; 3+ messages in thread
From: Mike Lothian @ 2026-09-08 23:43 UTC (permalink / raw)
To: Daniel J . Ogorchock
Cc: Jiri Kosina, Benjamin Tissoires, Silvan Jegen, Dmitry Torokhov,
linux-input, linux-kernel, Mike Lothian
joycon_config_rumble() queues a zero-rumble packet during probe. The
rumble worker sends it once ctlr_state becomes READ, and
joycon_handle_rumble_report() retries it a few more times. This is the
first subcommand sent on every connection and it goes out before any
input report has been seen, so consecutive_valid_report_deltas is still
0. joycon_enforce_subcmd_rate_strict() needs 3 consecutive reports in
the 8-17ms window before it releases a subcommand, so the send exhausts
all 25 attempts and warns on every connection.
Add a subcmd_rate_unproven flag, set until the cadence has been
observed, and use the legacy flat-delay throttle while it is set.
joycon_parse_report() clears it after JC_SUBCMD_VALID_DELTA_REQ
consecutive reports at a valid cadence. USB is excluded, as its
consecutive_valid_report_deltas is forced to the requirement anyway.
The flag is separate from subcmd_rate_relaxed so that the exhaustion
fallback added by commit 781f8e020a78 ("HID: nintendo: fix rumble
starved by the input report cadence gate") keeps its semantics.
subcmd_rate_relaxed is written from the subcommand worker under
output_mutex, subcmd_rate_unproven only from joycon_parse_report().
Found with btmon over Bluetooth on a MediaTek mt7921e: the three
JC_SUBCMD_RATE_MAX_ATTEMPTS warnings on connect match three
JC_OUTPUT_RUMBLE_ONLY (0x10) frames from the rumble worker's
zero-countdown retries. Tested on a Pro Controller, which now connects
reliably, where before it dropped within the first ~70 seconds of a
fresh connection about half the time.
Fixes: d750d1480362 ("HID: nintendo: fix rumble rate limiter")
Signed-off-by: Mike Lothian <mike@fireburn.co.uk>
Assisted-by: Claude:Opus-5 [Claude Code]
---
v2:
- Use a separate subcmd_rate_unproven flag rather than reusing
subcmd_rate_relaxed. In v1 joycon_parse_report() cleared
subcmd_rate_relaxed from softirq under ctlr->lock, while
joycon_enforce_subcmd_rate_strict() sets it from the subcommand
worker under output_mutex. Clearing it after the exhaustion counter
had passed JC_SUBCMD_RATE_MAX_FAILURES would stop the
++subcmd_rate_exhaustions == JC_SUBCMD_RATE_MAX_FAILURES test from
ever matching again, permanently disabling the fallback added by
781f8e020a78.
- Start USB controllers proven. consecutive_valid_report_deltas is
forced to JC_SUBCMD_VALID_DELTA_REQ for USB at the end of
joycon_parse_report(), so the v1 promotion could never run and USB
would have been left on the legacy throttle.
- v1 was assisted by Claude Sonnet 5, v2 by Claude Opus 5.
Both issues in v1 were spotted by the Sashiko AI review:
https://lore.kernel.org/linux-input/20260908225815.952CE1F00A3A@smtp.kernel.org/
That review also flagged a pre-existing issue: that the probe error
path does not set ctlr_state to JOYCON_CTLR_STATE_REMOVED, so a queued
rumble worker could run against stopped hardware during
destroy_workqueue(). As far as I can tell that is not reachable. The
worker can only be queued from joycon_handle_rumble_report(), which
runs via joycon_ctlr_read_handler() and is gated on ctlr_state ==
JOYCON_CTLR_STATE_READ, and from joycon_play_effect(), which needs the
input device registered. input_register_device() is the last call in
joycon_input_create() that can fail, and ctlr_state is set to READ
immediately after it returns, so there is no window in which the worker
is queued and probe can still fail.
v1: https://lore.kernel.org/linux-input/20260908224532.114357-1-mike@fireburn.co.uk/
drivers/hid/hid-nintendo.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 43e0f2aaea3b..5b3d97c0ad39 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -611,6 +611,7 @@ struct joycon_ctlr {
unsigned int consecutive_valid_report_deltas;
unsigned int subcmd_rate_exhaustions;
bool subcmd_rate_relaxed;
+ bool subcmd_rate_unproven;
/* factory calibration data */
struct joycon_stick_cal left_stick_cal_x;
@@ -917,7 +918,7 @@ static void joycon_enforce_subcmd_rate_legacy(struct joycon_ctlr *ctlr)
static void joycon_enforce_subcmd_rate(struct joycon_ctlr *ctlr)
{
- if (ctlr->subcmd_rate_relaxed)
+ if (ctlr->subcmd_rate_relaxed || READ_ONCE(ctlr->subcmd_rate_unproven))
joycon_enforce_subcmd_rate_legacy(ctlr);
else
joycon_enforce_subcmd_rate_strict(ctlr);
@@ -1797,8 +1798,11 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
*/
if (report_delta_ms >= JC_INPUT_REPORT_MIN_DELTA &&
report_delta_ms <= JC_INPUT_REPORT_MAX_DELTA) {
- if (ctlr->consecutive_valid_report_deltas < JC_SUBCMD_VALID_DELTA_REQ)
+ if (ctlr->consecutive_valid_report_deltas < JC_SUBCMD_VALID_DELTA_REQ) {
ctlr->consecutive_valid_report_deltas++;
+ if (ctlr->consecutive_valid_report_deltas == JC_SUBCMD_VALID_DELTA_REQ)
+ WRITE_ONCE(ctlr->subcmd_rate_unproven, false);
+ }
} else {
ctlr->consecutive_valid_report_deltas = 0;
}
@@ -2730,6 +2734,7 @@ static int nintendo_hid_probe(struct hid_device *hdev,
ctlr->hdev = hdev;
ctlr->ctlr_state = JOYCON_CTLR_STATE_INIT;
+ ctlr->subcmd_rate_unproven = hdev->bus != BUS_USB;
ctlr->rumble_queue_head = 0;
ctlr->rumble_queue_tail = 0;
hid_set_drvdata(hdev, ctlr);
--
2.55.0
^ permalink raw reply related [flat|nested] 3+ messages in thread