Re: [PATCH v9 2/3] HID: nintendo: Add rumble support for Switch 2 controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free on `ns2->rumble_buffer` during device teardown due to devres lifecycle mismatch.
- [Medium] Rumble work scheduling math error adds 2ms instead of subtracting, causing it to run late and risk buffer underruns.
- [Medium] Pro Controller rumble motors receive identical signals, failing to control them individually as claimed.
--

commit 0899ea0ef032bf60e7abf4c07b73da1a806a6406
Author: Vicki Pfau <vi@endrift.com>
Date:   Wed Jul 1 06:56:03 2026 +0000

    HID: nintendo: Add rumble support for Switch 2 controllers
    
    This adds rumble support for both the "HD Rumble" linear resonant actuator
    type as used in the Joy-Cons and Pro Controller, as well as the eccentric
    rotating mass type used in the GameCube controller. Note that since there's
    currently no API for exposing full control of LRAs with evdev, it only
    simulates a basic rumble for now.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 5aacb8e88b704..96463e1586bbd 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -3205,6 +3259,125 @@ static void switch2_kref_put(struct kref *refcount)
[ ... ]
> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> +{
[ ... ]
> +	if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) {
> +		ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude,
> +			(uint16_t) (effect->u.rumble.weak_magnitude >> 1));
> +	} else {
> +		ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ;
> +		ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ;
> +		ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16;
> +		ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16;
> +	}
> +	spin_unlock_irqrestore(&ns2->rumble_lock, flags);
> +
> +	schedule_delayed_work(&ns2->rumble_work, 0);
> +
> +	return 0;
> +}
> +
> +static void switch2_rumble_work(struct work_struct *work)
> +{
[ ... ]
> +	} else {
> +		ns2->rumble_buffer[0] = 1;
> +		switch2_encode_rumble(&ns2->rumble.hd, &ns2->rumble_buffer[0x2]);
> +		active = ns2->rumble.hd.hi_amp || ns2->rumble.hd.lo_amp;
> +		if (ns2->ctlr_type == NS2_CTLR_TYPE_PRO) {
> +			/*
> +			 * The Pro Controller contains separate LRAs on each
> +			 * side that can be controlled individually.
> +			 */
> +			ns2->rumble_buffer[0] = 2;
> +			ns2->rumble_buffer[0x11] = 0x50 | ns2->rumble_seq;
> +			switch2_encode_rumble(&ns2->rumble.hd, &ns2->rumble_buffer[0x12]);
> +		}
> +	}

[Severity: Medium]
Does this code accurately provide independent control of the two rumble motors?

The commit message and the comment above mention that the Pro Controller's left
and right rumble motors can be controlled individually.

However, in switch2_play_effect(), the strong and weak magnitudes are mixed
into a single switch2_hd_rumble structure (ns2->rumble.hd). 

Then in switch2_rumble_work(), this exact same structure is encoded into both
the left (offset 0x2) and right (offset 0x12) rumble buffers. 

Since both physical motors receive the exact same mixed signal (both high and 
low frequencies simultaneously), doesn't this result in identical rumble on 
both sides rather than individual control?

> +	ns2->rumble_seq = (ns2->rumble_seq + 1) & 0xF;
> +	spin_unlock_irqrestore(&ns2->rumble_lock, flags);
> +
> +	if (active) {
> +		unsigned long interval = msecs_to_jiffies(4);
> +		uint64_t current_jiffies = get_jiffies_64();
> +
> +		if (!ns2->last_rumble_work)
> +			ns2->last_rumble_work = current_jiffies;
> +		else
> +			ns2->last_rumble_work += interval;
> +
> +		/* Reschedule a little early to make sure the buffer never underruns */
> +		interval += msecs_to_jiffies(2);
> +		if (ns2->last_rumble_work + interval >= current_jiffies)
> +			schedule_delayed_work(&ns2->rumble_work,
> +				ns2->last_rumble_work + interval - current_jiffies);

[Severity: Medium]
Does this schedule the work early or late?

The comment mentions rescheduling slightly early to prevent buffer underruns.

Since interval starts at 4ms, adding 2ms to it with
interval += msecs_to_jiffies(2) makes the new interval 6ms. 

When scheduling for ns2->last_rumble_work + interval, this appears to target
a time 6ms after the last theoretical tick instead of the intended earlier
target (which would be 2ms after, or interval - 2ms).

Could this delay cause the work to run later than intended and lead to the 
buffer underruns the comment warns about?

[ ... ]
> @@ -3346,6 +3522,20 @@ static int switch2_init_input(struct switch2_controller *ns2)
[ ... ]
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	ns2->rumble_buffer = devm_kzalloc(&input->dev, 64, GFP_KERNEL);
> +	if (!ns2->rumble_buffer) {
> +		input_free_device(input);
> +		return -ENOMEM;
> +	}

[Severity: High]
Can this lead to a use-after-free during device removal?

The rumble_buffer is allocated with devm_kzalloc() tied to the input device, 
so it gets freed when the input device is unregistered and its final reference
is dropped.

However, looking at the teardown sequence, if the input device is unregistered
(for example via switch2_controller_put() calling input_unregister_device())
before the delayed work is completely synced and canceled, the worker could run
and access the freed rumble_buffer.

Would it be safer to allocate this buffer without devm, or ensure the delayed
work is fully canceled prior to unregistering the input device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701065606.4169999-1-vi@endrift.com?part=2