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 [off-list ref]
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.

[ ... ]

[ ... ]

+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.
+		 */

[ ... ]

[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);
 }

[ ... ]

[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,

[ ... ]

[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