Re: [PATCH v5 05/11] HID: steam: Coalesce rumble packets

From: sashiko-bot@kernel.org
Date: 2026-07-30 04:33:46

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

New issues:
- [High] Dynamic device unregistration fails to cancel `coalesce_rumble_work`, causing an infinite loop and resource leak.
- [Medium] Lockless `delayed_work_pending()` check bypasses rate limiting, causing haptic reset bugs.

Pre-existing issues:
- [High] Teardown order in `steam_remove()` causes Use-After-Free by cancelling work items before unregistering the input device.
--

--- Patch [5]: [PATCH v5 05/11] HID: steam: Coalesce rumble packets ---

commit d8418fccc5859b59417d02482c0b768383aa61ef
Author: Vicki Pfau [off-list ref]

HID: steam: Coalesce rumble packets

This commit introduces a 20Hz rate limit for rumble packets sent to the Steam
Deck to prevent the hardware from resetting haptic patterns on every packet.
It implements this rate limiting by using a delayed work queue to coalesce 
rumble updates.

quoted hunk ↗ jump to hunk

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

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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]
Will this work item reschedule itself indefinitely if the device is dynamically
unregistered while a haptic effect is playing?

When the device is dynamically unregistered, such as during a wireless
disconnect or a mode switch, steam_unregister() destroys the input device
via steam_input_unregister().

However, steam_unregister() does not cancel coalesce_rumble_work, and the
input core does not proactively clear the rumble_left and rumble_right
magnitudes when an input device is destroyed. Because the input device is
gone, userspace can never send a 0-magnitude effect to stop the rumble,
leaving this worker to loop indefinitely every 50ms and leak CPU cycles
and USB bandwidth.

quoted hunk ↗ jump to hunk

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

[Severity: Medium]
Could this lockless check of delayed_work_pending() defeat the intended
rate limiting?

The workqueue core clears the pending bit immediately before executing the
worker callback. Because steam_coalesce_rumble_cb() acquires a mutex and
performs synchronous USB I/O, its execution time can be relatively large.

If a userspace process updates force feedback effects while the callback is
actively executing, delayed_work_pending() will evaluate to false. This
causes steam_play_effect() to immediately queue another rumble_work, which
then executes right after the callback finishes, sending two packets
back-to-back and bypassing the 50ms throttle.

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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 the teardown order in steam_remove()
allow a use-after-free?

The works are cancelled before the input device is unregistered:

steam_remove() {
    ...
    cancel_work_sync(&steam->rumble_work);
    cancel_delayed_work_sync(&steam->coalesce_rumble_work);
    ...
    steam_unregister(steam);
}

If a userspace process continuously calls the EVIOCSFF ioctl to play
force-feedback effects while the device is being removed, the ioctl
path in steam_play_effect() can queue new rumble_work and
coalesce_rumble_work items after they have been cancelled.

Once steam_remove() completes, the devm-managed steam structure is freed,
but the newly queued work items will eventually execute and dereference
the freed memory.

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help