Re: [PATCH v4 06/11] HID: steam: Coalesce rumble packets

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The coalescing logic unintentionally creates an infinite loop of redundant rumble packets and always sends an immediate duplicate, violating the stated goal.
- [Medium] Data race on rumble magnitude variables `rumble_left` and `rumble_right`.

Pre-existing issues:
- [High] Incorrect teardown order in `steam_remove()` leads to a Use-After-Free of `steam_device` by requeued work items.
--

--- Patch [6]: [PATCH v4 06/11] HID: steam: Coalesce rumble packets ---
commit 49ddfbfccce223ca7429a2e5dad77eb7b4ef0fbe
Author: Vicki Pfau <[email protected]>

HID: steam: Coalesce rumble packets

The Steam Deck resets the haptic pattern every time it receives a rumble
packet, leading to weird discontinuities or sometimes cutting out entirely.
Instead of overloading the interface, Steam interally rate-limits sending
these packets, so we should too.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -602,10 +603,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);
> +

[Severity: Medium]
Can this lead to stale magnitude values being read? The rumble_left and
rumble_right variables are read here by the asynchronous worker thread without
acquiring a lock or using READ_ONCE().

> +	if (steam->rumble_left || steam->rumble_right)
> +		schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
> +}

[Severity: High]
Does this create an infinite polling loop? The delayed work reschedules itself
unconditionally based only on non-zero magnitudes. It appears this will
repeatedly spam the interface with identical values, causing the haptic
pattern to reset continuously at 20Hz.

> +
>  #ifdef CONFIG_STEAM_FF
>  static int steam_play_effect(struct input_dev *dev, void *data,
>  				struct ff_effect *effect)
> @@ -615,6 +630,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;

[Severity: Medium]
When delayed_work_pending() returns true below, this function returns early.
Does this mean the magnitudes are written locklessly without generating a
memory barrier, potentially causing the worker thread to read incorrect rumble
intensities?

> +	/*
> +	 * 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;
> +
> +	schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
>  	return schedule_work(&steam->rumble_work);
>  }

[Severity: High]
When a new effect starts, both an immediate work and a delayed work are
queued here. Doesn't this guarantee a duplicate packet is sent 50ms later
even if no changes occurred?

[ ... ]

> @@ -1443,6 +1468,7 @@ static void steam_remove(struct hid_device *hdev)
>  	cancel_delayed_work_sync(&steam->mode_switch);
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_work_sync(&steam->rumble_work);
> +	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
>  	cancel_work_sync(&steam->unregister_work);
>  	steam->client_hdev = NULL;
>  	steam->client_opened = 0;

[Severity: High]
This is a pre-existing issue, but does this teardown order in steam_remove()
lead to a use-after-free of the steam_device struct?

During device removal, the work items are cancelled before unregistering the
input device. If a userspace process still holds the device open when
steam_unregister() is subsequently called, it will synchronously flush active
effects via input_ff_flush().

This flush ultimately calls steam_play_effect() to stop the rumble, which
re-arms both rumble_work and coalesce_rumble_work after they were already
cancelled here. Will those requeued work items execute after the device memory
has been freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6