Re: [PATCH v7 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] The delay calculation in `switch2_rumble_work` causes an unbounded timer catch-up loop, flooding the USB bus with back-to-back output reports if the worker is delayed.
- [High] Sending an embedded, non-cacheline-aligned struct member (`ns2->rumble_buffer`) to `hid_hw_output_report()` can cause memory corruption during DMA.
- [High] The teardown sequence calls `hid_hw_stop()` before cancelling the rumble work and unregistering the input device, allowing output reports to be sent to a stopped device.
--

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

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 b9e2e708e28f3..a53c554a54ae3 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> @@ -3043,8 +3056,45 @@ struct switch2_controller {
>  
>  	uint32_t player_id;
>  	struct led_classdev leds[4];
> +
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	spinlock_t rumble_lock;
> +	uint8_t rumble_seq;
> +	union {
> +		struct switch2_hd_rumble hd;
> +		struct switch2_erm_rumble sd;
> +	} rumble;
> +	uint64_t last_rumble_work;
> +	struct delayed_work rumble_work;
> +	uint8_t rumble_buffer[64];

[Severity: High]
Does this pass an embedded, non-cacheline-aligned array to the DMA layer?

The USB DMA API requires that data buffers passed to it are dynamically
allocated (e.g., via kmalloc()) to ensure they are cache-line aligned.
Because rumble_buffer is embedded directly in struct switch2_controller, it
shares a cache line with other members like rumble_work.

Could this lead to memory corruption of adjacent struct members during
DMA cache invalidation on non-coherent architectures when passed to
hid_hw_output_report() in switch2_rumble_work()?

> +#endif
>  };
[ ... ]
> +static void switch2_rumble_work(struct work_struct *work)
> +{
[ ... ]
> +	if (active) {
> +		unsigned long interval = msecs_to_jiffies(2);
> +		uint64_t current_jiffies = get_jiffies_64();
> +
> +		if (!ns2->last_rumble_work)
> +			ns2->last_rumble_work = current_jiffies;
> +		else
> +			ns2->last_rumble_work += interval;
> +
> +		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 delay calculation result in an unbounded timer catch-up loop?

If the switch2_rumble_work() worker is delayed (e.g., due to system load
or slow synchronous transfers), last_rumble_work will fall behind
current_jiffies. The function appears to attempt to catch up by continually
adding the fixed 2ms interval to last_rumble_work and scheduling the next
execution with 0 delay.

For example, a 100ms stall might result in 50 back-to-back executions.
Could this flood the USB bus with zero-delay output reports, wasting CPU
resources and potentially degrading the device or bus?

> +	} 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);
[ ... ]
> @@ -4009,6 +4210,9 @@ static void switch2_remove(struct hid_device *hdev)
>  
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	cancel_delayed_work_sync(&ns2->rumble_work);
> +#endif
>  	mutex_lock(&ns2->lock);
>  	ida_free(&nintendo_player_id_allocator, ns2->player_id);
>  	WARN_ON(ns2->hdev != hdev);
> +	ns2->hdev = NULL;

[Severity: High]
Does this teardown sequence allow output reports to be sent to a stopped
device?

Here, hid_hw_stop() is called to stop the underlying HID transport,
followed by cancel_delayed_work_sync(&ns2->rumble_work). If the worker
thread was preempted just before acquiring ns2->lock in
switch2_rumble_work():

Worker thread:
    mutex_lock(&ns2->lock);
    // preempted before lock acquisition

Teardown:
    hid_hw_stop(hdev);
    cancel_delayed_work_sync(&ns2->rumble_work); // waits for worker

Worker thread resumes:
    if (!ns2->hdev) // hdev is still valid here, set to NULL later
        ...
    else
        ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);

Could the worker thread then execute hid_hw_output_report() on the stopped
device, potentially causing undefined behavior in the transport drivers?

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