Re: [PATCH v15 2/4] HID: nintendo: Add rumble support for Switch 2 controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: nintendo: Infinite catch-up loop in switch2_rumble_work
- [High] hid: nintendo: AB-BA Deadlock in switch2_init_input error path
--

commit f2f111d17b13153234806561e2b816ddf7f4e353
Author: Vicki Pfau <vi@endrift.com>

HID: nintendo: Add rumble support for Switch 2 controllers

This commit introduces rumble support for Nintendo Switch 2 controllers,
handling both HD Rumble (linear resonant actuators) and eccentric rotating
mass haptics. It manages rumble states using a delayed workqueue to stream
reports.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 81d0a25d9846b..c185ad4a27c97 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> @@ -3251,6 +3305,128 @@ static void switch2_kref_put(struct kref *refcount)
[ ... ]
> +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]
Could this create an unbounded catch-up loop under load or after a suspend?

If switch2_rumble_work is delayed (for example, by lock contention or a
suspend/resume cycle) and falls significantly behind current_jiffies, it
reschedules itself with a 0 delay while only advancing last_rumble_work
by 4ms. If the execution of the work itself takes longer than 4ms,
current_jiffies will grow faster than last_rumble_work can catch up.

Can this lead to a situation where the worker spins constantly, pegging a
CPU core and flooding the controller with stale rumble packets?

> +	} else {
> +		ns2->last_rumble_work = 0;
> +	}
> +
> +	mutex_lock(&ns2->lock);
> +	if (!ns2->hdev) {
> +		cancel_delayed_work(&ns2->rumble_work);
> +	} else {
> +		ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);
[ ... ]
> @@ -3374,6 +3550,26 @@ 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;
> +	}
> +	ret = devm_delayed_work_autocancel(&input->dev, &ns2->rumble_work, switch2_rumble_work);
[ ... ]
> +	ns2->leds = devm_kcalloc(&input->dev, JC_NUM_LEDS, sizeof(*ns2->leds), GFP_KERNEL);
> +	if (!ns2->leds) {
> +		hid_err(ns2->hdev, "Failed to allocate LEDs\n");
> +		input_unregister_device(input);
> +		return -ENOMEM;
> +	}

[Severity: High]
Does this error path introduce an AB-BA deadlock with the rumble worker?

switch2_receive_command() acquires ns2->lock before calling
switch2_init_input(). Because the input device is registered before this
allocation, userspace can immediately open the evdev node and issue a rumble
effect, queuing switch2_rumble_work.

If devm_kcalloc() fails, it calls input_unregister_device(input). Since
userspace might have already closed the node, dropping the last reference
triggers synchronous devm cleanup (because the work is registered via
devm_delayed_work_autocancel). This cleanup will invoke
cancel_delayed_work_sync(&ns2->rumble_work) while this thread still holds
ns2->lock.

However, switch2_rumble_work() needs to acquire ns2->lock to complete, as
seen in the earlier snippet. Can this result in a permanent deadlock?

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