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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unconditional self-rescheduling causes severe haptic stutter by spamming redundant packets at 20Hz.

Pre-existing issues:
- [High] Use-after-free during device removal when a force-feedback effect is active.
--

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

HID: steam: Coalesce rumble packets

The Steam Deck resets the haptic pattern every time it receives a rumble
packet. This patch rate-limits sending these packets to 20Hz to prevent
discontinuities and cutting out.

> 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: Medium]
Will this unconditionally self-reschedule and send identical packets at 20Hz
as long as a continuous force-feedback effect is active?

If a constant rumble effect is playing, this worker will continuously send
hardware reports and reschedule itself at 20Hz even if the magnitude has not
changed.

Since the commit message notes that the Steam Deck resets its haptic pattern
upon receiving a rumble packet, won't spamming these redundant packets force
continuous resets and cause the exact haptic stutter this patch is trying to
fix?

Could we add a check to only send packets and reschedule if the requested
rumble magnitudes have actually changed?

[ ... ]

> @@ -1444,6 +1469,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 canceling these work items before
unregistering the input device cause a use-after-free on device removal?

If the device is physically unplugged while a force-feedback effect is
playing, the removal path cancels the rumble works and then proceeds to call
steam_unregister().

When the input device is unregistered, the input core flushes and stops all
active effects. This operation routes through to steam_play_effect() with a
zero magnitude.

Since steam_play_effect() unconditionally re-arms the work items by calling
schedule_work() and schedule_delayed_work(), the work items we just cancelled
will be queued again. When device removal completes, the steam_device
structure memory is freed by devres.

The re-armed works will execute later and dereference the freed memory. Should
the work items be cancelled after steam_unregister() instead to prevent this?

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