Re: [PATCH v2 05/10] HID: steam: Coalesce rumble packets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The rate-limiting logic inadvertently creates a continuous polling loop that actively induces the haptic discontinuities it was intended to fix.
--

--- Patch [5]: [PATCH v2 05/10] HID: steam: Coalesce rumble packets ---
commit 57ea23e2a38271a41e31886938cdef03c430a55f
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Coalesce rumble packets

This commit adds rate limiting to Steam Deck haptic rumble packets to
avoid resetting the haptic pattern too frequently when userspace sends
many rapid updates. It introduces a delayed work queue to throttle updates
to 20Hz.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 6029c055a6cf3..5537488139011 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -603,10 +604,24 @@ static void steam_haptic_rumble_cb(struct work_struct *work)
>  {
>  	struct steam_device *steam = container_of(work, struct steam_device,
>  							rumble_work);
> +
>  	steam_haptic_rumble(steam, 0, steam->rumble_left,
>  		steam->rumble_right, 2, 0);
>  }
>  
> +static void steam_coalesce_rumble_cb(struct work_struct *work)
> +{
> +	struct steam_device *steam = container_of(to_delayed_work(work),
> +							struct steam_device,
> +							coalesce_rumble_work);
> +
> +	steam_haptic_rumble(steam, 0, steam->rumble_left,
> +		steam->rumble_right, 2, 0);
> +
> +	if (steam->rumble_left || steam->rumble_right)
> +		schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
> +}

[Severity: High]
Does this logic inadvertently create a continuous loop that actively induces
the haptic discontinuities it was intended to fix?

Since the callback unconditionally reschedules itself every 50ms as long as
the stored rumble values are non-zero, it will continuously transmit identical
rumble packets to the device at 20Hz, even when userspace has stopped
updating the effect.

Because the commit message mentions the Steam Deck resets its haptic pattern
on every packet, won't this perpetual loop guarantee the pattern resets every
50ms and unnecessarily spam the USB bus?

> +
>  #ifdef CONFIG_STEAM_FF
>  static int steam_play_effect(struct input_dev *dev, void *data,
>  				struct ff_effect *effect)
> @@ -616,6 +631,14 @@ static int steam_play_effect(struct input_dev *dev, void *data,
>  	steam->rumble_left = effect->u.rumble.strong_magnitude;
>  	steam->rumble_right = effect->u.rumble.weak_magnitude;
>  
> +	/*
> +	 * The interface gets somewhat overloaded when too many rumble
> +	 * packets are sent in a row, so Steam throttles it to 20 Hz
> +	 */
> +	if (delayed_work_pending(&steam->coalesce_rumble_work))
> +		return 0;

[Severity: High]
Could this condition unnecessarily stall valid userspace updates?

Because the loop in steam_coalesce_rumble_cb() keeps the delayed work pending
continuously while the effect is active, valid userspace updates may be
stalled by up to 50ms instead of executing immediately when the system is
otherwise idle.

> +
> +	schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
>  	return schedule_work(&steam->rumble_work);
>  }
>  #endif

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