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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Explicitly calling input_free_device() on a devm-managed input device in error paths leads to a double-free.
- [High] ABBA deadlock between ns2->lock and ns2->rumble_work during device removal or probe failure.
- [High] Teardown race allows hid_hw_output_report() to execute on a stopped HID device.
- [High] The rumble delayed work loop fails to bound catch-up scheduling, causing permanent zero-delay spam.

Pre-existing issues:
- [Critical] Double spinlock acquisition causing a deadlock in the error path of switch2_usb_probe.
--

commit 8dfb25ac3f59c21072ff7262dbfd49faf2095c2d
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: nintendo: Add rumble support for Switch 2 controllers

This commit adds rumble support for Switch 2 controllers. It handles both
HD Rumble LRAs for Joy-Cons and Pro Controllers, and eccentric rotating
mass haptics for GameCube controllers. Since evdev lacks a full LRA API,
it simulates basic rumble effects.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 20e5b18039b85..00ab4bee3dabc 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)
>  	kfree(ns2);
>  }
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)

[ ... ]

> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> +{
> +	struct switch2_controller *ns2 = input_get_drvdata(dev);

[ ... ]

> +	schedule_delayed_work(&ns2->rumble_work, 0);

[Severity: High]
Does this create a race condition with device teardown?

During hot-unplug, switch2_remove() calls cancel_delayed_work_sync() and
hid_hw_stop(hdev). However, because the input device is not unregistered
until switch2_controller_put() is called under ns2->lock, userspace could
still send EV_FF events via ioctl.

If switch2_play_effect() is invoked during this window, it schedules
ns2->rumble_work unconditionally.

> +
> +	return 0;
> +}
> +
> +static void switch2_rumble_work(struct work_struct *work)
> +{
> +	struct switch2_controller *ns2 = container_of(to_delayed_work(work),
> +						      struct switch2_controller, rumble_work);

[ ... ]

> +	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);
> +		else
> +			schedule_delayed_work(&ns2->rumble_work, 0);

[Severity: High]
Can this loop get stuck in a permanent zero-delay spam?

If the worker gets delayed by system load or if hid_hw_output_report() takes
longer than 4ms, ns2->last_rumble_work falls behind current_jiffies. The
code adds a fixed 4ms to last_rumble_work and calls schedule_delayed_work()
with a 0 delay to catch up.

If the worker continuously takes more than 4ms to run, current_jiffies
advances alongside last_rumble_work. Without a cap or reset mechanism, could
this flood the hardware and monopolize a workqueue thread indefinitely?

> +	} else {
> +		ns2->last_rumble_work = 0;
> +	}
> +
> +	mutex_lock(&ns2->lock);

[Severity: High]
Could this lead to an ABBA deadlock?

In switch2_remove(), we take ns2->lock and then call
switch2_controller_put(), which calls input_unregister_device(). This will
synchronously invoke the driver close callback, switch2_input_deref().

If switch2_input_deref() calls cancel_delayed_work_sync(), it will wait
for this worker to finish while holding ns2->lock. If switch2_rumble_work()
is currently executing, it will attempt to acquire ns2->lock here, resulting
in a deadlock.

> +	if (!ns2->hdev) {
> +		cancel_delayed_work(&ns2->rumble_work);
> +	} else {
> +		ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);

[Severity: High]
Can this execute hid_hw_output_report() on a stopped device?

If switch2_play_effect() schedules rumble_work after switch2_remove()
called hid_hw_stop(), this worker will run, acquire ns2->lock, and pass the
!ns2->hdev check because ns2->hdev has not yet been set to NULL.

> +		/*
> +		 * Don't log on ENODEV, ESHUTDOWN, or EPROTO, which can happen
> +		 * mid-hotplug. Also cancel any further work on ENODEV or
> +		 * ESHUTDOWN as they're clear indications that the endpoint
> +		 * is dead.
> +		 */

[ ... ]

> @@ -3275,6 +3448,9 @@ static void switch2_input_deref(struct input_dev *input)
>  {
>  	struct switch2_controller *ns2 = input_get_drvdata(input);
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	cancel_delayed_work_sync(&ns2->rumble_work);

[Severity: High]
Can this cause a deadlock during device removal?

switch2_remove() calls mutex_lock(&ns2->lock) followed by
switch2_controller_put(ns2). This eventually triggers the input core to
synchronously call this callback (switch2_input_deref).

Calling cancel_delayed_work_sync() while the removal thread holds ns2->lock
will deadlock if switch2_rumble_work() is simultaneously trying to acquire
ns2->lock.

> +#endif
>  	kref_put(&ns2->refcount, switch2_kref_put);
>  }

[ ... ]

> @@ -3346,6 +3522,20 @@ static int switch2_init_input(struct switch2_controller *ns2)
>  		return -EINVAL;
>  	}
>  
> +#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;
> +	}
> +	input_set_capability(input, EV_FF, FF_RUMBLE);
> +	ret = input_ff_create_memless(input, NULL, switch2_play_effect);
> +	if (ret) {
> +		input_free_device(input);

[Severity: Critical]
Will calling input_free_device() here lead to a double-free?

The input device was allocated earlier using devm_input_allocate_device().
By explicitly calling input_free_device() in this error path, the reference
count drops and the memory is freed.

When the probe entirely fails and the devres cleanup triggers
(devm_input_device_release()), it will attempt to call input_put_device() on
the already freed memory.

> +		return ret;
> +	}
> +#endif
> +
>  	hid_info(ns2->hdev, "Firmware version %u.%u.%u (type %i)\n", ns2->version.major,

[ ... ]

> @@ -4018,6 +4226,9 @@ static void switch2_remove(struct hid_device *hdev)
>  {
>  	struct switch2_controller *ns2 = hid_get_drvdata(hdev);
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	cancel_delayed_work_sync(&ns2->rumble_work);
> +#endif
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

[Severity: High]
Does this cancellation leave a window for the worker to be rescheduled?

The input device is not unregistered until switch2_controller_put()
is called later in this function. Userspace can still send EV_FF events via
ioctl, which could invoke switch2_play_effect() and unconditionally
reschedule ns2->rumble_work after the device has been stopped.

>  	mutex_lock(&ns2->lock);

Additionally, there is a finding in a separate file:

[Severity: Critical]
This isn't a bug introduced by this patch, but could the error handling in
switch2_usb_probe() cause a deadlock?

During probe, if usb_alloc_urb() or usb_alloc_coherent() fails in the
NS2_IN_URBS loop, the code jumps to the err_free_in label:

drivers/input/joystick/nintendo-switch2-usb.c:switch2_usb_probe() {
    ...
    if (!urb) {
        spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
        continue;
    }
    ...
}

It incorrectly calls spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags) again
instead of spin_unlock_irqrestore(), causing an immediate double-lock deadlock
when trying to free the URBs.

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