Re: [PATCH v5 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:
- [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble.
- [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition.
- [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers.
- [Low] Implicit type promotion in the max() macro causes a compiler warning.
- [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors.
--

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

This commit introduces rumble support for Switch 2 controllers, handling both HD
Rumble linear resonant actuators and eccentric rotating mass haptics. It adds
state machines for rumble scheduling and output reporting via hdev.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 26ef589f652c6..a7fe74d982475 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -850,10 +850,10 @@ config NINTENDO_FF
>  	depends on HID_NINTENDO
>  	select INPUT_FF_MEMLESS
>  	help
> -	Say Y here if you have a Nintendo Switch controller and want to enable
> -	force feedback support for it. This works for both joy-cons, the pro
> -	controller, and the NSO N64 controller. For the pro controller, both
> -	rumble motors can be controlled individually.
> +	Say Y here if you have a Nintendo Switch or Switch 2 controller and want
> +	to enable force feedback support for it. This works for Joy-Cons, the Pro
> +	Controllers, and the NSO N64 and GameCube controller. For the Pro
> +	Controller, both rumble motors can be controlled individually.

This isn't a bug, but is the help text accurate regarding individual motor
control? The implementation in switch2_rumble_work() appears to encode the
exact same rumble configuration to both the left and right motor offsets,
which would make both motors vibrate identically.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 2193afcdefabc..9801799a90758 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c

[ ... ]

> @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type)
>  	return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR;
>  }
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5])
> +{
> +	buffer[0] = rumble->hi_freq;
> +	buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2);
> +	buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4);
> +	buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6);
> +	buffer[4] = rumble->lo_amp >> 2;
> +}
> +
> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> +{
> +	struct switch2_controller *ns2 = input_get_drvdata(dev);
> +
> +	if (effect->type != FF_RUMBLE)
> +		return 0;
> +
> +	guard(spinlock_irqsave)(&ns2->rumble_lock);
> +	if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) {
> +		ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude,
> +			effect->u.rumble.weak_magnitude >> 1);

Will this max calculation trigger a compiler warning? The strict type checking
in the max macro might fail because effect->u.rumble.weak_magnitude >> 1
promotes to an int, while strong_magnitude is a u16.

> +	} else {
> +		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;
> +	}
> +
> +	if (ns2->hdev)
> +		schedule_delayed_work(&ns2->rumble_work, 0);
> +
> +	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);
> +	unsigned long current_ms = jiffies_to_msecs(get_jiffies_64());
> +	unsigned long flags;
> +	bool active;
> +	int ret;

[ ... ]

> +	if (active) {
> +		unsigned long interval = msecs_to_jiffies(4);
> +
> +		if (!ns2->last_rumble_work)
> +			ns2->last_rumble_work = current_ms;
> +		else
> +			ns2->last_rumble_work += interval;
> +		schedule_delayed_work(&ns2->rumble_work,
> +			ns2->last_rumble_work + interval - current_ms);

Does this code underflow the scheduled delay?

The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(),
but interval is calculated in jiffies using msecs_to_jiffies(). Since
ns2->last_rumble_work is initialized to milliseconds but incremented by
jiffies, subtracting current_ms could yield a negative value.

On systems where HZ is not 1000, could this underflow the delay parameter and
schedule the next work millions of jiffies in the future?

> +	} else {
> +		ns2->last_rumble_work = 0;
> +	}
> +	spin_unlock_irqrestore(&ns2->rumble_lock, flags);
> +
> +	if (!ns2->hdev) {
> +		cancel_delayed_work(&ns2->rumble_work);
> +		ret = -ENODEV;
> +	} else {
> +		ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);
> +	}
> +
> +	if (ret < 0)
> +		hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret);

If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call.
Could this result in a NULL pointer dereference, as the hid_dbg macro passes
ns2->hdev to dev_dbg()?

Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there
a time-of-check to time-of-use race where the compiler might reload the pointer
between the check and hid_hw_output_report()?

[ ... ]

> @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id
>  
>  	switch2_leds_create(ns2);
>  
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) {
> +		ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ;
> +		ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ;
> +	}

Are we checking ns2->ctlr_type before it is dynamically initialized?

During probe, ns2->ctlr_type appears to be 0 since the struct is freshly
allocated and the type is discovered later during the init state machine.
This would cause the high and low frequencies to always be written.

Since hd and sd share a union, could this corrupt the sd.error and
sd.amplitude fields for actual GameCube controllers?

> +	spin_lock_init(&ns2->rumble_lock);
> +	INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work);
> +#endif
>  	hid_set_drvdata(hdev, ns2);

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