[PATCH v4 00/10] HID: steam: General cleanup and improvements

[PATCH v4 00/10] HID: steam: General cleanup and improvements

[Vicki Pfau]

This is the first half of a patch series improving the hid-steam driver.

This series contains a few notable changes:
- As I've been maintaining it downstream in SteamOS as well as writing
  general improvements, I'm marking myself as the maintainer of the driver.
- Renames some stuff, notably marking the original Steam Controller as
  Steam Controller (2015) in preparation of adding support for the Steam
  Controller (2026), which will be in the second half of this series once
  it's ready for submission.
- Adds IMU support for the Steam Controller (2015).
- Some reliability improvements, especially surrounding edge cases
  regarding scheduled work during teardown.
- Fixes a short read issue reported by syzkaller.

Hopefully the second half of the series will be ready soon, but I am
waiting on more testing by SteamOS users before I submit it.

This is a minor interation on v3 that fixes a few issues that came up
during review of v3, as well as reverting a false positive from sashiko
that was added in v3.

Vicki Pfau (11):
  HID: steam: Update documentation
  HID: steam: Refactor and clean up report parsing
  HID: steam: Rename some constants that got renamed upstream
  HID: steam: Zero-initialize reply in serial lookup
  HID: steam: Add support for sensor events on the Steam Controller
    (2015)
  HID: steam: Coalesce rumble packets
  HID: steam: Fully unregister controller when hidraw is opened
  HID: steam: Rearrange teardown sequence
  HID: steam: Improve logging and other cleanup
  HID: steam: Reject short reads
  HID: steam: Retry send/recv reports if stale

 MAINTAINERS             |   6 +
 drivers/hid/hid-steam.c | 723 +++++++++++++++++++++++++++-------------
 2 files changed, 491 insertions(+), 238 deletions(-)

-- 
2.54.0

[PATCH v4 01/11] HID: steam: Update documentation

[Vicki Pfau]

Mark myself as the maintainer, as well as adding myself as an author.

It also makes some minor updates to comments, such as correcly calling the
left menu key view and retroactively renaming the original Steam Controller
as Steam Controller (2015), in preparation for support for the 2026 model.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 MAINTAINERS             |  6 ++++++
 drivers/hid/hid-steam.c | 13 +++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index f37a81950e25..8f2c582f77e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11566,6 +11566,12 @@ F:	drivers/hid/hid-sensor-*
 F:	drivers/iio/*/hid-*
 F:	include/linux/hid-sensor-*
 
+HID STEAM CONTROLLER
+M:	Vicki Pfau <vi@endrift.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/hid/hid-steam.c
+
 HID VRC-2 CAR CONTROLLER DRIVER
 M:	Marcus Folkesson <marcus.folkesson@gmail.com>
 L:	linux-input@vger.kernel.org
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 197126d6e081..a854d6360a0e 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -48,6 +48,7 @@
 MODULE_DESCRIPTION("HID driver for Valve Steam Controller");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
+MODULE_AUTHOR("Vicki Pfau <vi@endrift.com>");
 
 static bool lizard_mode = true;
 
@@ -1413,9 +1414,9 @@ static inline s16 steam_le16(u8 *data)
  *  9.1  | BTN_DPAD_RIGHT | left-pad right
  *  9.2  | BTN_DPAD_LEFT  | left-pad left
  *  9.3  | BTN_DPAD_DOWN  | left-pad down
- *  9.4  | BTN_SELECT | menu left
+ *  9.4  | BTN_SELECT | view
  *  9.5  | BTN_MODE   | steam logo
- *  9.6  | BTN_START  | menu right
+ *  9.6  | BTN_START  | menu
  *  9.7  | BTN_GRIPL  | left back lever
  * 10.0  | BTN_GRIPR  | right back lever
  * 10.1  | --         | left-pad clicked
@@ -1541,9 +1542,9 @@ static void steam_do_input_event(struct steam_device *steam,
  *  9.1  | BTN_DPAD_RIGHT | left-pad right
  *  9.2  | BTN_DPAD_LEFT  | left-pad left
  *  9.3  | BTN_DPAD_DOWN  | left-pad down
- *  9.4  | BTN_SELECT | menu left
+ *  9.4  | BTN_SELECT | view
  *  9.5  | BTN_MODE   | steam logo
- *  9.6  | BTN_START  | menu right
+ *  9.6  | BTN_START  | menu
  *  9.7  | BTN_GRIPL2 | left bottom grip button
  *  10.0 | BTN_GRIPR2 | right bottom grip button
  *  10.1 | BTN_THUMB  | left pad pressed
@@ -1850,11 +1851,11 @@ MODULE_PARM_DESC(lizard_mode,
 	"Enable mouse and keyboard emulation (lizard mode) when the gamepad is not in use");
 
 static const struct hid_device_id steam_controllers[] = {
-	{ /* Wired Steam Controller */
+	{ /* Wired Steam Controller (2015) */
 	  HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 		USB_DEVICE_ID_STEAM_CONTROLLER)
 	},
-	{ /* Wireless Steam Controller */
+	{ /* Wireless Steam Controller (2015) */
 	  HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 		USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
 	  .driver_data = STEAM_QUIRK_WIRELESS
-- 
2.54.0

[PATCH v4 02/11] HID: steam: Refactor and clean up report parsing

[Vicki Pfau]

This switches from a parsing style where each button or axis is parsed
individually out of a report using !!(byte & BIT(x)) style. This commit
switches it to a mostly unified approach of defining a list of individual
mappings in an array and passing it to a function that handles all of the
extraction. Theoretically this is more lines, but in practice it results in
(subjectively) cleaner code. Some exceptions still need to be made for
things like handling the lizard mode toggle key, but in general there's a
lot less manual code.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 211 ++++++++++++++++++++++++----------------
 1 file changed, 128 insertions(+), 83 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index a854d6360a0e..75d6be0be0a2 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -43,6 +43,7 @@
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
 #include <linux/power_supply.h>
+#include <linux/unaligned.h>
 #include "hid-ids.h"
 
 MODULE_DESCRIPTION("HID driver for Valve Steam Controller");
@@ -1355,13 +1356,45 @@ static void steam_do_connect_event(struct steam_device *steam, bool connected)
  * Clamp the values to 32767..-32767 so that the range is
  * symmetrical and can be negated safely.
  */
-static inline s16 steam_le16(u8 *data)
+static inline s16 steam_le16(const u8 *data)
 {
-	s16 x = (s16) le16_to_cpup((__le16 *)data);
+	s16 x = (s16) get_unaligned_le16((const __le16 *)data);
 
 	return x == -32768 ? -32767 : x;
 }
 
+struct steam_button_mapping {
+	int code;
+	u8 byte;
+	u8 bit;
+};
+
+struct steam_axis_mapping {
+	int code;
+	s8 sign;
+	u8 byte;
+};
+
+static void steam_map_buttons(struct input_dev *input,
+		const struct steam_button_mapping *mappings, const u8 *data)
+{
+	const struct steam_button_mapping *mapping;
+
+	for (mapping = mappings; mapping->code; mapping++)
+		input_report_key(input, mapping->code,
+			data[mapping->byte] & BIT(mapping->bit));
+}
+
+static void steam_map_axes(struct input_dev *input,
+		const struct steam_axis_mapping *mappings, const u8 *data)
+{
+	const struct steam_axis_mapping *mapping;
+
+	for (mapping = mappings; mapping->sign; mapping++)
+		input_report_abs(input, mapping->code,
+			mapping->sign * steam_le16(&data[mapping->byte]));
+}
+
 /*
  * The size for this message payload is 60.
  * The known values are:
@@ -1428,18 +1461,42 @@ static inline s16 steam_le16(u8 *data)
  * 10.7  | --         | lpad_and_joy
  */
 
+static const struct steam_button_mapping steam_controller_button_mappings[] = {
+	{ BTN_TR2,		 8, 0 },
+	{ BTN_TL2,		 8, 1 },
+	{ BTN_TR,		 8, 2 },
+	{ BTN_TL,		 8, 3 },
+	{ BTN_Y,		 8, 4 },
+	{ BTN_B,		 8, 5 },
+	{ BTN_X,		 8, 6 },
+	{ BTN_A,		 8, 7 },
+	{ BTN_SELECT,		 9, 4 },
+	{ BTN_MODE,		 9, 5 },
+	{ BTN_START,		 9, 6 },
+	{ BTN_GRIPL,		 9, 7 },
+	{ BTN_GRIPR,		10, 0 },
+	{ BTN_THUMBR,		10, 2 },
+	{ BTN_THUMBL,		10, 6 },
+	{ BTN_THUMB2,		10, 4 },
+	{ BTN_DPAD_UP,		 9, 0 },
+	{ BTN_DPAD_RIGHT,	 9, 1 },
+	{ BTN_DPAD_LEFT,	 9, 2 },
+	{ BTN_DPAD_DOWN,	 9, 3 },
+	{ /* sentinel */ },
+};
+
+static const struct steam_axis_mapping steam_controller_axis_mappings[] = {
+	{ ABS_RX,  1, 20 },
+	{ ABS_RY, -1, 22 },
+	{ /* sentinel */ },
+};
+
 static void steam_do_input_event(struct steam_device *steam,
 		struct input_dev *input, u8 *data)
 {
-	/* 24 bits of buttons */
-	u8 b8, b9, b10;
 	s16 x, y;
 	bool lpad_touched, lpad_and_joy;
 
-	b8 = data[8];
-	b9 = data[9];
-	b10 = data[10];
-
 	input_report_abs(input, ABS_HAT2Y, data[11]);
 	input_report_abs(input, ABS_HAT2X, data[12]);
 
@@ -1451,8 +1508,8 @@ static void steam_do_input_event(struct steam_device *steam,
 	 * joystick values.
 	 * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
 	 */
-	lpad_touched = b10 & BIT(3);
-	lpad_and_joy = b10 & BIT(7);
+	lpad_touched = data[10] & BIT(3);
+	lpad_and_joy = data[10] & BIT(7);
 	x = steam_le16(data + 16);
 	y = -steam_le16(data + 18);
 
@@ -1468,31 +1525,10 @@ static void steam_do_input_event(struct steam_device *steam,
 		input_report_abs(input, ABS_HAT0X, 0);
 		input_report_abs(input, ABS_HAT0Y, 0);
 	}
+	input_report_key(input, BTN_THUMB, lpad_touched || lpad_and_joy);
 
-	input_report_abs(input, ABS_RX, steam_le16(data + 20));
-	input_report_abs(input, ABS_RY, -steam_le16(data + 22));
-
-	input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
-	input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
-	input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
-	input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
-	input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
-	input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
-	input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
-	input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
-	input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
-	input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
-	input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
-	input_event(input, EV_KEY, BTN_GRIPL, !!(b9 & BIT(7)));
-	input_event(input, EV_KEY, BTN_GRIPR, !!(b10 & BIT(0)));
-	input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
-	input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
-	input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
-	input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
-	input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
-	input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
-	input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
-	input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
+	steam_map_buttons(input, steam_controller_button_mappings, data);
+	steam_map_axes(input, steam_controller_axis_mappings, data);
 
 	input_sync(input);
 }
@@ -1595,23 +1631,67 @@ static void steam_do_input_event(struct steam_device *steam,
  *  15.6 | --         | unknown
  *  15.7 | --         | unknown
  */
+
+static const struct steam_button_mapping steam_deck_button_mappings[] = {
+	{ BTN_TR2,		 8, 0 },
+	{ BTN_TL2,		 8, 1 },
+	{ BTN_TR,		 8, 2 },
+	{ BTN_TL,		 8, 3 },
+	{ BTN_Y,		 8, 4 },
+	{ BTN_B,		 8, 5 },
+	{ BTN_X,		 8, 6 },
+	{ BTN_A,		 8, 7 },
+	{ BTN_SELECT,		 9, 4 },
+	{ BTN_MODE,		 9, 5 },
+	{ BTN_START,		 9, 6 },
+	{ BTN_GRIPL2,		 9, 7 },
+	{ BTN_GRIPR2,		10, 0 },
+	{ BTN_THUMBL,		10, 6 },
+	{ BTN_THUMBR,		11, 2 },
+	{ BTN_DPAD_UP,		 9, 0 },
+	{ BTN_DPAD_RIGHT,	 9, 1 },
+	{ BTN_DPAD_LEFT,	 9, 2 },
+	{ BTN_DPAD_DOWN,	 9, 3 },
+	{ BTN_THUMB,		10, 1 },
+	{ BTN_THUMB2,		10, 2 },
+	{ BTN_GRIPL,		13, 1 },
+	{ BTN_GRIPR,		13, 2 },
+	{ BTN_BASE,		14, 2 },
+	{ /* sentinel */ },
+};
+
+static const struct steam_axis_mapping steam_deck_axis_mappings[] = {
+	{ ABS_X,	 1, 48 },
+	{ ABS_Y,	-1, 50 },
+	{ ABS_RX,	 1, 52 },
+	{ ABS_RY,	-1, 54 },
+	{ ABS_HAT2Y,	 1, 44 },
+	{ ABS_HAT2X,	 1, 46 },
+	{ /* sentinel */ },
+};
+
+static const struct steam_axis_mapping steam_deck_imu_mappings[] = {
+	{ ABS_X,   1, 24 },
+	{ ABS_Z,  -1, 26 },
+	{ ABS_Y,   1, 28 },
+	{ ABS_RX,  1, 30 },
+	{ ABS_RZ, -1, 32 },
+	{ ABS_RY,  1, 34 },
+	{ /* sentinel */ },
+};
+
 static void steam_do_deck_input_event(struct steam_device *steam,
 		struct input_dev *input, u8 *data)
 {
-	u8 b8, b9, b10, b11, b13, b14;
+	bool start_pressed;
 	bool lpad_touched, rpad_touched;
 
-	b8 = data[8];
-	b9 = data[9];
-	b10 = data[10];
-	b11 = data[11];
-	b13 = data[13];
-	b14 = data[14];
+	start_pressed = data[9] & BIT(6);
 
-	if (!(b9 & BIT(6)) && steam->did_mode_switch) {
+	if (!start_pressed && steam->did_mode_switch) {
 		steam->did_mode_switch = false;
 		cancel_delayed_work(&steam->mode_switch);
-	} else if (!steam->client_opened && (b9 & BIT(6)) && !steam->did_mode_switch) {
+	} else if (!steam->client_opened && start_pressed && !steam->did_mode_switch) {
 		steam->did_mode_switch = true;
 		schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
 	}
@@ -1619,8 +1699,8 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 	if (!steam->gamepad_mode && lizard_mode)
 		return;
 
-	lpad_touched = b10 & BIT(3);
-	rpad_touched = b10 & BIT(4);
+	lpad_touched = data[10] & BIT(3);
+	rpad_touched = data[10] & BIT(4);
 
 	if (lpad_touched) {
 		input_report_abs(input, ABS_HAT0X, steam_le16(data + 16));
@@ -1638,38 +1718,8 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 		input_report_abs(input, ABS_HAT1Y, 0);
 	}
 
-	input_report_abs(input, ABS_X, steam_le16(data + 48));
-	input_report_abs(input, ABS_Y, -steam_le16(data + 50));
-	input_report_abs(input, ABS_RX, steam_le16(data + 52));
-	input_report_abs(input, ABS_RY, -steam_le16(data + 54));
-
-	input_report_abs(input, ABS_HAT2Y, steam_le16(data + 44));
-	input_report_abs(input, ABS_HAT2X, steam_le16(data + 46));
-
-	input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
-	input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
-	input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
-	input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
-	input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
-	input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
-	input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
-	input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
-	input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
-	input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
-	input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
-	input_event(input, EV_KEY, BTN_GRIPL2, !!(b9 & BIT(7)));
-	input_event(input, EV_KEY, BTN_GRIPR2, !!(b10 & BIT(0)));
-	input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
-	input_event(input, EV_KEY, BTN_THUMBR, !!(b11 & BIT(2)));
-	input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
-	input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
-	input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
-	input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
-	input_event(input, EV_KEY, BTN_THUMB, !!(b10 & BIT(1)));
-	input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(2)));
-	input_event(input, EV_KEY, BTN_GRIPL, !!(b13 & BIT(1)));
-	input_event(input, EV_KEY, BTN_GRIPR, !!(b13 & BIT(2)));
-	input_event(input, EV_KEY, BTN_BASE, !!(b14 & BIT(2)));
+	steam_map_buttons(input, steam_deck_button_mappings, data);
+	steam_map_axes(input, steam_deck_axis_mappings, data);
 
 	input_sync(input);
 }
@@ -1690,12 +1740,7 @@ static void steam_do_deck_sensors_event(struct steam_device *steam,
 		return;
 
 	input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
-	input_report_abs(sensors, ABS_X, steam_le16(data + 24));
-	input_report_abs(sensors, ABS_Z, -steam_le16(data + 26));
-	input_report_abs(sensors, ABS_Y, steam_le16(data + 28));
-	input_report_abs(sensors, ABS_RX, steam_le16(data + 30));
-	input_report_abs(sensors, ABS_RZ, -steam_le16(data + 32));
-	input_report_abs(sensors, ABS_RY, steam_le16(data + 34));
+	steam_map_axes(sensors, steam_deck_imu_mappings, data);
 
 	input_sync(sensors);
 }
-- 
2.54.0

[PATCH v4 03/11] HID: steam: Rename some constants that got renamed upstream

[Vicki Pfau]

SETTING_MOUSE_POINTER_ENABLED was renamed to SETTING_LIZARD_MODE upstream.
SETTING_GYRO_MODE was renamed to SETTING_IMU_MODE in an older commit, but
the associated enum was overlooked.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 75d6be0be0a2..983d18d1de4f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -151,7 +151,7 @@ enum {
 	SETTING_USB_DEBUG_MODE,
 	SETTING_LEFT_TRACKPAD_MODE,
 	SETTING_RIGHT_TRACKPAD_MODE,
-	SETTING_MOUSE_POINTER_ENABLED,
+	SETTING_LIZARD_MODE,
 
 	/* 10 */
 	SETTING_DPAD_DEADZONE,
@@ -261,14 +261,14 @@ enum {
 	ATTRIB_STR_UNIT_SERIAL,
 };
 
-/* Values for GYRO_MODE (bitmask) */
+/* Values for IMU_MODE (bitmask) */
 enum {
-	SETTING_GYRO_MODE_OFF			= 0,
-	SETTING_GYRO_MODE_STEERING		= BIT(0),
-	SETTING_GYRO_MODE_TILT			= BIT(1),
-	SETTING_GYRO_MODE_SEND_ORIENTATION	= BIT(2),
-	SETTING_GYRO_MODE_SEND_RAW_ACCEL	= BIT(3),
-	SETTING_GYRO_MODE_SEND_RAW_GYRO		= BIT(4),
+	SETTING_IMU_MODE_OFF			= 0,
+	SETTING_IMU_MODE_STEERING		= BIT(0),
+	SETTING_IMU_MODE_TILT			= BIT(1),
+	SETTING_IMU_MODE_SEND_ORIENTATION	= BIT(2),
+	SETTING_IMU_MODE_SEND_RAW_ACCEL		= BIT(3),
+	SETTING_IMU_MODE_SEND_RAW_GYRO		= BIT(4),
 };
 
 /* Trackpad modes */
-- 
2.54.0

[PATCH v4 04/11] HID: steam: Zero-initialize reply in serial lookup

[Vicki Pfau]

When requesting the serial number from a controller, the function will do
some basic bounds checking to make sure the reply is valid, as well as
capping off the reply with a null byte before copying. However, the error
logging can leak uninitialized memory in some cases. We can simplify and
solve this by just zero-initalizing the reply memory eagerly instead.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 983d18d1de4f..8bad79205b57 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -447,7 +447,7 @@ static int steam_get_serial(struct steam_device *steam)
 	 */
 	int ret = 0;
 	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
-	u8 reply[3 + STEAM_SERIAL_LEN + 1];
+	u8 reply[3 + STEAM_SERIAL_LEN + 1] = {0};
 
 	mutex_lock(&steam->report_mutex);
 	ret = steam_send_report(steam, cmd, sizeof(cmd));
@@ -461,7 +461,6 @@ static int steam_get_serial(struct steam_device *steam)
 		ret = -EIO;
 		goto out;
 	}
-	reply[3 + STEAM_SERIAL_LEN] = 0;
 	strscpy(steam->serial_no, reply + 3, reply[1]);
 out:
 	mutex_unlock(&steam->report_mutex);
-- 
2.54.0

[PATCH v4 05/11] HID: steam: Add support for sensor events on the Steam Controller (2015)

[Vicki Pfau]

Sensor support was added for the Steam Deck previously, but Steam
Controller sensor events were never added. This adds that missing support,
bringing Steam Controller support much closer to feature parity with things
like SDL and Steam itself.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 214 ++++++++++++++++++++++++++++++++--------
 1 file changed, 175 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 8bad79205b57..cabecd250072 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -70,13 +70,14 @@ static LIST_HEAD(steam_devices);
 /* Joystick runs are about 5 mm and 32768 units */
 #define STEAM_DECK_JOYSTICK_RESOLUTION 6553
 /* Accelerometer has 16 bit resolution and a range of +/- 2g */
-#define STEAM_DECK_ACCEL_RES_PER_G 16384
-#define STEAM_DECK_ACCEL_RANGE 32768
+#define STEAM_ACCEL_RES_PER_G 16384
+#define STEAM_ACCEL_RANGE 32768
+#define STEAM_ACCEL_FUZZ 128
 #define STEAM_DECK_ACCEL_FUZZ 32
 /* Gyroscope has 16 bit resolution and a range of +/- 2000 dps */
-#define STEAM_DECK_GYRO_RES_PER_DPS 16
-#define STEAM_DECK_GYRO_RANGE 32768
-#define STEAM_DECK_GYRO_FUZZ 1
+#define STEAM_GYRO_RES_PER_DPS 16
+#define STEAM_GYRO_RANGE 32768
+#define STEAM_GYRO_FUZZ 0
 
 #define STEAM_PAD_FUZZ 256
 
@@ -255,6 +256,31 @@ enum
 	ID_CONTROLLER_DECK_STATE = 9
 };
 
+/* Read-only attributes */
+enum {
+	ATTRIB_UNIQUE_ID, // deprecated
+	ATTRIB_PRODUCT_ID,
+	ATTRIB_PRODUCT_REVISON, // deprecated
+	ATTRIB_CAPABILITIES = ATTRIB_PRODUCT_REVISON, // intentional aliasing
+	ATTRIB_FIRMWARE_VERSION, // deprecated
+	ATTRIB_FIRMWARE_BUILD_TIME,
+	ATTRIB_RADIO_FIRMWARE_BUILD_TIME,
+	ATTRIB_RADIO_DEVICE_ID0,
+	ATTRIB_RADIO_DEVICE_ID1,
+	ATTRIB_DONGLE_FIRMWARE_BUILD_TIME,
+	ATTRIB_HW_ID, // AKA BOARD_REVISION,
+	ATTRIB_BOOTLOADER_BUILD_TIME,
+	ATTRIB_CONNECTION_INTERVAL_IN_US,
+	ATTRIB_SECONDARY_FIRMWARE_BUILD_TIME,
+	ATTRIB_SECONDARY_BOOTLOADER_BUILD_TIME,
+	ATTRIB_SECONDARY_HW_ID, // AKA BOARD_REVISION,
+	ATTRIB_STREAMING,
+	ATTRIB_TRACKPAD_ID,
+	ATTRIB_SECONDARY_TRACKPAD_ID,
+
+	ATTRIB_COUNT
+};
+
 /* String attribute identifiers */
 enum {
 	ATTRIB_STR_BOARD_SERIAL,
@@ -284,6 +310,11 @@ enum {
 	TRACKPAD_GESTURE_KEYBOARD,
 };
 
+struct steam_controller_attribute {
+	unsigned char tag;
+	__le32 value;
+} __packed;
+
 /* Pad identifiers for the deck */
 #define STEAM_PAD_LEFT 0
 #define STEAM_PAD_RIGHT 1
@@ -315,6 +346,7 @@ struct steam_device {
 	u16 rumble_left;
 	u16 rumble_right;
 	unsigned int sensor_timestamp_us;
+	unsigned int sensor_update_rate_us;
 	struct work_struct unregister_work;
 };
 
@@ -467,6 +499,38 @@ static int steam_get_serial(struct steam_device *steam)
 	return ret;
 }
 
+static int steam_get_attributes(struct steam_device *steam)
+{
+	int ret = 0;
+	u8 cmd[] = {ID_GET_ATTRIBUTES_VALUES, 0};
+	u8 reply[64] = {};
+	u8 size;
+	int i;
+	struct steam_controller_attribute *attr;
+
+	guard(mutex)(&steam->report_mutex);
+	ret = steam_send_report(steam, cmd, sizeof(cmd));
+	if (ret < 0)
+		return ret;
+	ret = steam_recv_report(steam, reply, sizeof(reply));
+	if (ret < 0)
+		return ret;
+	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2)
+		return -EIO;
+
+	size = min(reply[1], sizeof(reply) - 2);
+	for (i = 0; i + sizeof(*attr) <= size; i += sizeof(*attr)) {
+		attr = (struct steam_controller_attribute *)&reply[i + 2];
+		if (attr->tag == ATTRIB_CONNECTION_INTERVAL_IN_US) {
+			steam->sensor_update_rate_us = get_unaligned_le32(&attr->value);
+			hid_dbg(steam->hdev, "Sensor update rate: %uus\n",
+				steam->sensor_update_rate_us);
+		}
+	}
+
+	return 0;
+}
+
 /*
  * This command requests the wireless adaptor to post an event
  * with the connection status. Useful if this driver is loaded when
@@ -625,6 +689,42 @@ static void steam_input_close(struct input_dev *dev)
 	}
 }
 
+static int steam_sensor_open(struct input_dev *dev)
+{
+	struct steam_device *steam = input_get_drvdata(dev);
+	unsigned long flags;
+	bool client_opened;
+
+	spin_lock_irqsave(&steam->lock, flags);
+	client_opened = steam->client_opened;
+	spin_unlock_irqrestore(&steam->lock, flags);
+	if (client_opened)
+		return 0;
+
+	guard(mutex)(&steam->report_mutex);
+	steam_write_settings(steam, SETTING_IMU_MODE,
+			SETTING_IMU_MODE_SEND_RAW_ACCEL | SETTING_IMU_MODE_SEND_RAW_GYRO,
+			0);
+
+	return 0;
+}
+
+static void steam_sensor_close(struct input_dev *dev)
+{
+	struct steam_device *steam = input_get_drvdata(dev);
+	unsigned long flags;
+	bool client_opened;
+
+	spin_lock_irqsave(&steam->lock, flags);
+	client_opened = steam->client_opened;
+	spin_unlock_irqrestore(&steam->lock, flags);
+	if (client_opened)
+		return;
+
+	guard(mutex)(&steam->report_mutex);
+	steam_write_settings(steam, SETTING_IMU_MODE, 0, 0);
+}
+
 static enum power_supply_property steam_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_SCOPE,
@@ -838,9 +938,6 @@ static int steam_sensors_register(struct steam_device *steam)
 	struct input_dev *sensors;
 	int ret;
 
-	if (!(steam->quirks & STEAM_QUIRK_DECK))
-		return 0;
-
 	rcu_read_lock();
 	sensors = rcu_dereference(steam->sensors);
 	rcu_read_unlock();
@@ -855,8 +952,14 @@ static int steam_sensors_register(struct steam_device *steam)
 
 	input_set_drvdata(sensors, steam);
 	sensors->dev.parent = &hdev->dev;
+	if (!(steam->quirks & STEAM_QUIRK_DECK)) {
+		sensors->open = steam_sensor_open;
+		sensors->close = steam_sensor_close;
+	}
 
-	sensors->name = "Steam Deck Motion Sensors";
+	sensors->name = steam->quirks & STEAM_QUIRK_DECK ?
+		"Steam Deck Motion Sensors" :
+		"Steam Controller Motion Sensors";
 	sensors->phys = hdev->phys;
 	sensors->uniq = steam->serial_no;
 	sensors->id.bustype = hdev->bus;
@@ -868,25 +971,34 @@ static int steam_sensors_register(struct steam_device *steam)
 	__set_bit(EV_MSC, sensors->evbit);
 	__set_bit(MSC_TIMESTAMP, sensors->mscbit);
 
-	input_set_abs_params(sensors, ABS_X, -STEAM_DECK_ACCEL_RANGE,
-			STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_Y, -STEAM_DECK_ACCEL_RANGE,
-			STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_Z, -STEAM_DECK_ACCEL_RANGE,
-			STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
-	input_abs_set_res(sensors, ABS_X, STEAM_DECK_ACCEL_RES_PER_G);
-	input_abs_set_res(sensors, ABS_Y, STEAM_DECK_ACCEL_RES_PER_G);
-	input_abs_set_res(sensors, ABS_Z, STEAM_DECK_ACCEL_RES_PER_G);
-
-	input_set_abs_params(sensors, ABS_RX, -STEAM_DECK_GYRO_RANGE,
-			STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_RY, -STEAM_DECK_GYRO_RANGE,
-			STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_RZ, -STEAM_DECK_GYRO_RANGE,
-			STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
-	input_abs_set_res(sensors, ABS_RX, STEAM_DECK_GYRO_RES_PER_DPS);
-	input_abs_set_res(sensors, ABS_RY, STEAM_DECK_GYRO_RES_PER_DPS);
-	input_abs_set_res(sensors, ABS_RZ, STEAM_DECK_GYRO_RES_PER_DPS);
+	if (steam->quirks & STEAM_QUIRK_DECK) {
+		input_set_abs_params(sensors, ABS_X, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Y, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Z, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
+	} else {
+		input_set_abs_params(sensors, ABS_X, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Y, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Z, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_ACCEL_FUZZ, 0);
+	}
+	input_abs_set_res(sensors, ABS_X, STEAM_ACCEL_RES_PER_G);
+	input_abs_set_res(sensors, ABS_Y, STEAM_ACCEL_RES_PER_G);
+	input_abs_set_res(sensors, ABS_Z, STEAM_ACCEL_RES_PER_G);
+
+	input_set_abs_params(sensors, ABS_RX, -STEAM_GYRO_RANGE,
+			STEAM_GYRO_RANGE, STEAM_GYRO_FUZZ, 0);
+	input_set_abs_params(sensors, ABS_RY, -STEAM_GYRO_RANGE,
+			STEAM_GYRO_RANGE, STEAM_GYRO_FUZZ, 0);
+	input_set_abs_params(sensors, ABS_RZ, -STEAM_GYRO_RANGE,
+			STEAM_GYRO_RANGE, STEAM_GYRO_FUZZ, 0);
+	input_abs_set_res(sensors, ABS_RX, STEAM_GYRO_RES_PER_DPS);
+	input_abs_set_res(sensors, ABS_RY, STEAM_GYRO_RES_PER_DPS);
+	input_abs_set_res(sensors, ABS_RZ, STEAM_GYRO_RES_PER_DPS);
 
 	ret = input_register_device(sensors);
 	if (ret)
@@ -917,9 +1029,6 @@ static void steam_sensors_unregister(struct steam_device *steam)
 {
 	struct input_dev *sensors;
 
-	if (!(steam->quirks & STEAM_QUIRK_DECK))
-		return;
-
 	rcu_read_lock();
 	sensors = rcu_dereference(steam->sensors);
 	rcu_read_unlock();
@@ -967,6 +1076,12 @@ static int steam_register(struct steam_device *steam)
 			strscpy(steam->serial_no, "XXXXXXXXXX",
 					sizeof(steam->serial_no));
 
+		ret = steam_get_attributes(steam);
+		if (ret < 0)
+			hid_err(steam->hdev,
+				"%s:steam_get_attributes failed with error %d\n",
+				__func__, ret);
+
 		hid_info(steam->hdev, "Steam Controller '%s' connected",
 				steam->serial_no);
 
@@ -1245,6 +1360,10 @@ static int steam_probe(struct hid_device *hdev,
 	INIT_LIST_HEAD(&steam->list);
 	INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
 	steam->sensor_timestamp_us = 0;
+	if (steam->quirks & STEAM_QUIRK_DECK)
+		steam->sensor_update_rate_us = 4000;
+	else
+		steam->sensor_update_rate_us = 9000;
 	INIT_WORK(&steam->unregister_work, steam_work_unregister_cb);
 
 	/*
@@ -1490,6 +1609,16 @@ static const struct steam_axis_mapping steam_controller_axis_mappings[] = {
 	{ /* sentinel */ },
 };
 
+static const struct steam_axis_mapping steam_controller_imu_mappings[] = {
+	{ ABS_X,   1, 28 },
+	{ ABS_Z,  -1, 30 },
+	{ ABS_Y,   1, 32 },
+	{ ABS_RX,  1, 34 },
+	{ ABS_RZ,  1, 36 },
+	{ ABS_RY,  1, 38 },
+	{ /* sentinel */ },
+};
+
 static void steam_do_input_event(struct steam_device *steam,
 		struct input_dev *input, u8 *data)
 {
@@ -1532,6 +1661,17 @@ static void steam_do_input_event(struct steam_device *steam,
 	input_sync(input);
 }
 
+static void steam_do_sensors_event(struct steam_device *steam,
+		struct input_dev *sensors, u8 *data)
+{
+	steam->sensor_timestamp_us += steam->sensor_update_rate_us;
+
+	input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
+	steam_map_axes(sensors, steam_controller_imu_mappings, data);
+
+	input_sync(sensors);
+}
+
 /*
  * The size for this message payload is 56.
  * The known values are:
@@ -1726,14 +1866,7 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 static void steam_do_deck_sensors_event(struct steam_device *steam,
 		struct input_dev *sensors, u8 *data)
 {
-	/*
-	 * The deck input report is received every 4 ms on average,
-	 * with a jitter of +/- 4 ms even though the USB descriptor claims
-	 * that it uses 1 kHz.
-	 * Since the HID report does not include a sensor timestamp,
-	 * use a fixed increment here.
-	 */
-	steam->sensor_timestamp_us += 4000;
+	steam->sensor_timestamp_us += steam->sensor_update_rate_us;
 
 	if (!steam->gamepad_mode && lizard_mode)
 		return;
@@ -1818,6 +1951,9 @@ static int steam_raw_event(struct hid_device *hdev,
 		input = rcu_dereference(steam->input);
 		if (likely(input))
 			steam_do_input_event(steam, input, data);
+		sensors = rcu_dereference(steam->sensors);
+		if (likely(sensors))
+			steam_do_sensors_event(steam, sensors, data);
 		rcu_read_unlock();
 		break;
 	case ID_CONTROLLER_DECK_STATE:
-- 
2.54.0

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

[Vicki Pfau]

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.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index cabecd250072..a200878db8ac 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -343,6 +343,7 @@ struct steam_device {
 	bool did_mode_switch;
 	bool gamepad_mode;
 	struct work_struct rumble_work;
+	struct delayed_work coalesce_rumble_work;
 	u16 rumble_left;
 	u16 rumble_right;
 	unsigned int sensor_timestamp_us;
@@ -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);
+
+	if (steam->rumble_left || steam->rumble_right)
+		schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
+}
+
 #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;
 
+	/*
+	 * 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);
 }
 #endif
@@ -1359,6 +1382,7 @@ static int steam_probe(struct hid_device *hdev,
 	INIT_DELAYED_WORK(&steam->mode_switch, steam_mode_switch_cb);
 	INIT_LIST_HEAD(&steam->list);
 	INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
+	INIT_DELAYED_WORK(&steam->coalesce_rumble_work, steam_coalesce_rumble_cb);
 	steam->sensor_timestamp_us = 0;
 	if (steam->quirks & STEAM_QUIRK_DECK)
 		steam->sensor_update_rate_us = 4000;
@@ -1425,6 +1449,7 @@ static int steam_probe(struct hid_device *hdev,
 	cancel_work_sync(&steam->work_connect);
 	cancel_delayed_work_sync(&steam->mode_switch);
 	cancel_work_sync(&steam->rumble_work);
+	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
 	cancel_work_sync(&steam->unregister_work);
 
 	return ret;
@@ -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;
-- 
2.54.0

[PATCH v4 07/11] HID: steam: Fully unregister controller when hidraw is opened

[Vicki Pfau]

To avoid conflicts between anything touching the hidraw and the driver we
had previously detached the evdev nodes when the hidraw is opened. However,
this isn't sufficient to avoid FEATURE reports from conflicting, so we
change to fully unregistering the controller internally, leaving only the
hidraw active until it's closed.

This also unifies the unregister and connect callbacks, as now the logic
between these two callbacks is identical.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 60 +++++++++++++----------------------------
 1 file changed, 18 insertions(+), 42 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index a200878db8ac..ff112aa1b0e9 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -348,7 +348,6 @@ struct steam_device {
 	u16 rumble_right;
 	unsigned int sensor_timestamp_us;
 	unsigned int sensor_update_rate_us;
-	struct work_struct unregister_work;
 };
 
 static int steam_recv_report(struct steam_device *steam,
@@ -817,6 +816,7 @@ static int steam_battery_register(struct steam_device *steam)
 			&steam->battery_desc, &battery_cfg);
 	if (IS_ERR(battery)) {
 		ret = PTR_ERR(battery);
+		devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
 		hid_err(steam->hdev,
 				"%s:power_supply_register failed with error %d\n",
 				__func__, ret);
@@ -1076,6 +1076,7 @@ static void steam_battery_unregister(struct steam_device *steam)
 	RCU_INIT_POINTER(steam->battery, NULL);
 	synchronize_rcu();
 	power_supply_unregister(battery);
+	devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
 }
 
 static int steam_register(struct steam_device *steam)
@@ -1141,38 +1142,41 @@ static int steam_register(struct steam_device *steam)
 
 static void steam_unregister(struct steam_device *steam)
 {
+	if (!steam->serial_no[0])
+		return;
+
+	hid_info(steam->hdev, "Steam Controller '%s' disconnected",
+			steam->serial_no);
 	steam_battery_unregister(steam);
 	steam_sensors_unregister(steam);
 	steam_input_unregister(steam);
-	if (steam->serial_no[0]) {
-		hid_info(steam->hdev, "Steam Controller '%s' disconnected",
-				steam->serial_no);
-		mutex_lock(&steam_devices_lock);
-		list_del_init(&steam->list);
-		mutex_unlock(&steam_devices_lock);
-		steam->serial_no[0] = 0;
-	}
+	mutex_lock(&steam_devices_lock);
+	list_del_init(&steam->list);
+	mutex_unlock(&steam_devices_lock);
+	steam->serial_no[0] = 0;
 }
 
 static void steam_work_connect_cb(struct work_struct *work)
 {
 	struct steam_device *steam = container_of(work, struct steam_device,
 							work_connect);
+
 	unsigned long flags;
 	bool connected;
+	bool opened;
 	int ret;
 
 	spin_lock_irqsave(&steam->lock, flags);
+	opened = steam->client_opened;
 	connected = steam->connected;
 	spin_unlock_irqrestore(&steam->lock, flags);
 
-	if (connected) {
+	if (connected && !opened) {
 		ret = steam_register(steam);
-		if (ret) {
+		if (ret)
 			hid_err(steam->hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-		}
 	} else {
 		steam_unregister(steam);
 	}
@@ -1206,31 +1210,6 @@ static void steam_mode_switch_cb(struct work_struct *work)
 	}
 }
 
-static void steam_work_unregister_cb(struct work_struct *work)
-{
-	struct steam_device *steam = container_of(work, struct steam_device,
-							unregister_work);
-	unsigned long flags;
-	bool connected;
-	bool opened;
-
-	spin_lock_irqsave(&steam->lock, flags);
-	opened = steam->client_opened;
-	connected = steam->connected;
-	spin_unlock_irqrestore(&steam->lock, flags);
-
-	if (connected) {
-		if (opened) {
-			steam_sensors_unregister(steam);
-			steam_input_unregister(steam);
-		} else {
-			steam_set_lizard_mode(steam, lizard_mode);
-			steam_input_register(steam);
-			steam_sensors_register(steam);
-		}
-	}
-}
-
 static bool steam_is_valve_interface(struct hid_device *hdev)
 {
 	struct hid_report_enum *rep_enum;
@@ -1276,7 +1255,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
 	steam->client_opened++;
 	spin_unlock_irqrestore(&steam->lock, flags);
 
-	schedule_work(&steam->unregister_work);
+	schedule_work(&steam->work_connect);
 
 	return 0;
 }
@@ -1291,7 +1270,7 @@ static void steam_client_ll_close(struct hid_device *hdev)
 	steam->client_opened--;
 	spin_unlock_irqrestore(&steam->lock, flags);
 
-	schedule_work(&steam->unregister_work);
+	schedule_work(&steam->work_connect);
 }
 
 static int steam_client_ll_raw_request(struct hid_device *hdev,
@@ -1388,7 +1367,6 @@ static int steam_probe(struct hid_device *hdev,
 		steam->sensor_update_rate_us = 4000;
 	else
 		steam->sensor_update_rate_us = 9000;
-	INIT_WORK(&steam->unregister_work, steam_work_unregister_cb);
 
 	/*
 	 * With the real steam controller interface, do not connect hidraw.
@@ -1450,7 +1428,6 @@ static int steam_probe(struct hid_device *hdev,
 	cancel_delayed_work_sync(&steam->mode_switch);
 	cancel_work_sync(&steam->rumble_work);
 	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
-	cancel_work_sync(&steam->unregister_work);
 
 	return ret;
 }
@@ -1469,7 +1446,6 @@ static void steam_remove(struct hid_device *hdev)
 	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;
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
-- 
2.54.0

[PATCH v4 08/11] HID: steam: Rearrange teardown sequence

[Vicki Pfau]

This fixes a narrow window during the teardown sequence where callbacks
could still be scheduled during cleanup that would then have a dangling
pointer to the now-freed steam struct.

This also puts work canceling for rumble and mode switch in
steam_unregister, as that shouldn't persist while the client hdev is open.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 67 ++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index ff112aa1b0e9..5ab4396a241e 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1150,6 +1150,9 @@ static void steam_unregister(struct steam_device *steam)
 	steam_battery_unregister(steam);
 	steam_sensors_unregister(steam);
 	steam_input_unregister(steam);
+	cancel_work_sync(&steam->rumble_work);
+	cancel_delayed_work_sync(&steam->mode_switch);
+	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
 	mutex_lock(&steam_devices_lock);
 	list_del_init(&steam->list);
 	mutex_unlock(&steam_devices_lock);
@@ -1327,6 +1330,7 @@ static int steam_probe(struct hid_device *hdev,
 {
 	struct steam_device *steam;
 	int ret;
+	unsigned long flags;
 
 	ret = hid_parse(hdev);
 	if (ret) {
@@ -1352,6 +1356,14 @@ static int steam_probe(struct hid_device *hdev,
 	if (!steam)
 		return -ENOMEM;
 
+	/*
+	 * With the real steam controller interface, do not connect hidraw.
+	 * Instead, create the client_hid and connect that.
+	 */
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
+	if (ret)
+		return ret;
+
 	steam->hdev = hdev;
 	hid_set_drvdata(hdev, steam);
 	spin_lock_init(&steam->lock);
@@ -1368,22 +1380,6 @@ static int steam_probe(struct hid_device *hdev,
 	else
 		steam->sensor_update_rate_us = 9000;
 
-	/*
-	 * With the real steam controller interface, do not connect hidraw.
-	 * Instead, create the client_hid and connect that.
-	 */
-	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
-	if (ret)
-		goto err_cancel_work;
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev,
-			"%s:hid_hw_open\n",
-			__func__);
-		goto err_hw_stop;
-	}
-
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver connected");
 		/* If using a wireless adaptor ask for connection status */
@@ -1397,14 +1393,22 @@ static int steam_probe(struct hid_device *hdev,
 			hid_err(hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-			goto err_hw_close;
+			goto err_hw_stop;
 		}
 	}
 
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev,
+			"%s:hid_hw_open\n",
+			__func__);
+		goto err_steam_unregister;
+	}
+
 	steam->client_hdev = steam_create_client_hid(hdev);
 	if (IS_ERR(steam->client_hdev)) {
 		ret = PTR_ERR(steam->client_hdev);
-		goto err_steam_unregister;
+		goto err_hw_close;
 	}
 	steam->client_hdev->driver_data = steam;
 
@@ -1416,18 +1420,20 @@ static int steam_probe(struct hid_device *hdev,
 
 err_destroy:
 	hid_destroy_device(steam->client_hdev);
-err_steam_unregister:
-	if (steam->connected)
-		steam_unregister(steam);
 err_hw_close:
 	hid_hw_close(hdev);
-err_hw_stop:
-	hid_hw_stop(hdev);
-err_cancel_work:
+err_steam_unregister:
+	spin_lock_irqsave(&steam->lock, flags);
+	steam->client_opened = 0;
+	spin_unlock_irqrestore(&steam->lock, flags);
 	cancel_work_sync(&steam->work_connect);
+	if (steam->connected)
+		steam_unregister(steam);
 	cancel_delayed_work_sync(&steam->mode_switch);
+err_hw_stop:
 	cancel_work_sync(&steam->rumble_work);
 	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
+	hid_hw_stop(hdev);
 
 	return ret;
 }
@@ -1435,25 +1441,24 @@ static int steam_probe(struct hid_device *hdev,
 static void steam_remove(struct hid_device *hdev)
 {
 	struct steam_device *steam = hid_get_drvdata(hdev);
+	unsigned long flags;
 
 	if (!steam || hdev->group == HID_GROUP_STEAM) {
 		hid_hw_stop(hdev);
 		return;
 	}
 
+	hid_hw_close(hdev);
 	hid_destroy_device(steam->client_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);
-	steam->client_hdev = NULL;
+	spin_lock_irqsave(&steam->lock, flags);
 	steam->client_opened = 0;
+	spin_unlock_irqrestore(&steam->lock, flags);
+	cancel_work_sync(&steam->work_connect);
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver disconnected");
 	}
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 	steam_unregister(steam);
+	hid_hw_stop(hdev);
 }
 
 static void steam_do_connect_event(struct steam_device *steam, bool connected)
-- 
2.54.0

[PATCH v4 09/11] HID: steam: Improve logging and other cleanup

[Vicki Pfau]

Adds more logging as appropriate, reindents an enum to match surrounding
style, as well as cleaning up some places where we can use guard() instead
of doing locking and unlocking manually.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 56 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 5ab4396a241e..3738a4de3bff 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -246,14 +246,14 @@ enum {
 /* Input report identifiers */
 enum
 {
-	ID_CONTROLLER_STATE = 1,
-	ID_CONTROLLER_DEBUG = 2,
-	ID_CONTROLLER_WIRELESS = 3,
-	ID_CONTROLLER_STATUS = 4,
-	ID_CONTROLLER_DEBUG2 = 5,
-	ID_CONTROLLER_SECONDARY_STATE = 6,
-	ID_CONTROLLER_BLE_STATE = 7,
-	ID_CONTROLLER_DECK_STATE = 9
+	ID_CONTROLLER_STATE		= 1,
+	ID_CONTROLLER_DEBUG		= 2,
+	ID_CONTROLLER_WIRELESS		= 3,
+	ID_CONTROLLER_STATUS		= 4,
+	ID_CONTROLLER_DEBUG2		= 5,
+	ID_CONTROLLER_SECONDARY_STATE	= 6,
+	ID_CONTROLLER_BLE_STATE		= 7,
+	ID_CONTROLLER_DECK_STATE	= 9,
 };
 
 /* Read-only attributes */
@@ -379,9 +379,16 @@ static int steam_recv_report(struct steam_device *steam,
 	ret = hid_hw_raw_request(steam->hdev, 0x00,
 			buf, hid_report_len(r) + 1,
 			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
-	if (ret > 0)
-		memcpy(data, buf + 1, min(size, ret - 1));
+	if (ret > 0) {
+		ret = min(size, ret - 1);
+		memcpy(data, buf + 1, ret);
+	}
 	kfree(buf);
+
+	if (ret < 0)
+		hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
+	else
+		hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
 	return ret;
 }
 
@@ -409,6 +416,8 @@ static int steam_send_report(struct steam_device *steam,
 	/* The report ID is always 0 */
 	memcpy(buf + 1, cmd, size);
 
+	hid_dbg(steam->hdev, "Sending report %*ph\n", size, cmd);
+
 	/*
 	 * Sometimes the wireless controller fails with EPIPE
 	 * when sending a feature report.
@@ -481,21 +490,20 @@ static int steam_get_serial(struct steam_device *steam)
 	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
 	u8 reply[3 + STEAM_SERIAL_LEN + 1] = {0};
 
-	mutex_lock(&steam->report_mutex);
+	guard(mutex)(&steam->report_mutex);
 	ret = steam_send_report(steam, cmd, sizeof(cmd));
 	if (ret < 0)
-		goto out;
+		return ret;
 	ret = steam_recv_report(steam, reply, sizeof(reply));
 	if (ret < 0)
-		goto out;
+		return ret;
 	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
 	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
-		ret = -EIO;
-		goto out;
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				(int)sizeof(reply), reply);
+		return -EIO;
 	}
 	strscpy(steam->serial_no, reply + 3, reply[1]);
-out:
-	mutex_unlock(&steam->report_mutex);
 	return ret;
 }
 
@@ -515,8 +523,11 @@ static int steam_get_attributes(struct steam_device *steam)
 	ret = steam_recv_report(steam, reply, sizeof(reply));
 	if (ret < 0)
 		return ret;
-	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2)
+	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2) {
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				(int)sizeof(reply), reply);
 		return -EIO;
+	}
 
 	size = min(reply[1], sizeof(reply) - 2);
 	for (i = 0; i + sizeof(*attr) <= size; i += sizeof(*attr)) {
@@ -538,11 +549,8 @@ static int steam_get_attributes(struct steam_device *steam)
  */
 static inline int steam_request_conn_status(struct steam_device *steam)
 {
-	int ret;
-	mutex_lock(&steam->report_mutex);
-	ret = steam_send_report_byte(steam, ID_DONGLE_GET_WIRELESS_STATE);
-	mutex_unlock(&steam->report_mutex);
-	return ret;
+	guard(mutex)(&steam->report_mutex);
+	return steam_send_report_byte(steam, ID_DONGLE_GET_WIRELESS_STATE);
 }
 
 /*
@@ -1195,6 +1203,7 @@ static void steam_mode_switch_cb(struct work_struct *work)
 		return;
 
 	steam->gamepad_mode = !steam->gamepad_mode;
+	hid_dbg(steam->hdev, "%s: switching gamepad mode to %i\n", __func__, steam->gamepad_mode);
 	if (steam->gamepad_mode)
 		steam_set_lizard_mode(steam, false);
 	else {
@@ -1838,6 +1847,7 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 		steam->did_mode_switch = false;
 		cancel_delayed_work(&steam->mode_switch);
 	} else if (!steam->client_opened && start_pressed && !steam->did_mode_switch) {
+		hid_dbg(steam->hdev, "%s: doing mode switch\n", __func__);
 		steam->did_mode_switch = true;
 		schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
 	}
-- 
2.54.0

[PATCH v4 10/11] HID: steam: Reject short reads

[Vicki Pfau]

Steam Controller FEATURE reports encode the size of the message in the
message itself. Previously we were trusting that the size reported matched
the size we actually read, leading to a potential issue with short reads.
Instead, we should actually verify the length of the read.

Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 3738a4de3bff..7c100d61495a 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -357,6 +357,13 @@ static int steam_recv_report(struct steam_device *steam,
 	u8 *buf;
 	int ret;
 
+	/*
+	 * All reports start with a two byte header.
+	 * We must read at least two bytes to get a sensible output.
+	 */
+	if (size < 2)
+		return -EINVAL;
+
 	r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
 	if (!r) {
 		hid_err(steam->hdev, "No HID_FEATURE_REPORT submitted -  nothing to read\n");
@@ -380,16 +387,30 @@ static int steam_recv_report(struct steam_device *steam,
 			buf, hid_report_len(r) + 1,
 			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 	if (ret > 0) {
-		ret = min(size, ret - 1);
-		memcpy(data, buf + 1, ret);
+		/* Remove the report ID from the return buffer */
+		ret--;
+		size = min(size, ret);
+		memcpy(data, buf + 1, size);
 	}
 	kfree(buf);
 
 	if (ret < 0)
 		hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
 	else
-		hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
-	return ret;
+		hid_dbg(steam->hdev, "Received report %*ph\n", size, data);
+	if (ret < 0)
+		return ret;
+
+	if (ret < 2) {
+		hid_err(steam->hdev, "%s: reply too short\n", __func__);
+		return -EPROTO;
+	}
+	if (ret < data[1] + 2) {
+		hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
+				__func__, data[1] + 2, ret);
+		return -EPROTO;
+	}
+	return size;
 }
 
 static int steam_send_report(struct steam_device *steam,
-- 
2.54.0

[PATCH v4 11/11] HID: steam: Retry send/recv reports if stale

[Vicki Pfau]

Sometimes recv report will reply with a stale result from a previous send
report. Instead of failing out, we should retry them, as they generally
reply correctly after three tries, give or take.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 54 +++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 7c100d61495a..3c7776aae6bf 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -501,6 +501,43 @@ static int steam_write_settings(struct steam_device *steam,
 	return steam_recv_report(steam, cmd, 2 + cmd[1]);
 }
 
+static int steam_exchange_report(struct steam_device *steam, u8 *cmd, int csize,
+		u8 *reply, int rsize)
+{
+	unsigned int retries = 5;
+	int ret;
+
+	guard(mutex)(&steam->report_mutex);
+	do {
+		ret = steam_send_report(steam, cmd, csize);
+		if (ret < 0)
+			return ret;
+		ret = steam_recv_report(steam, reply, rsize);
+		/*
+		 * Sometimes this can fail on the first few tries on the Steam
+		 * Controller (2015). It appears to be a firmware bug, and Steam
+		 * itself just retries, so we should also retry a few times to
+		 * see if we get it.
+		 */
+		if (ret == -EPROTO)
+			continue;
+		if (ret < 0) {
+			hid_err(steam->hdev, "%s: error reading reply (%*ph)\n",
+					__func__, csize, cmd);
+			return ret;
+		}
+		if (reply[0] == cmd[0] && reply[1] >= 1)
+			break;
+		if (retries > 0)
+			continue;
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				rsize, reply);
+		return -EPROTO;
+	} while (retries--);
+
+	return ret;
+}
+
 static int steam_get_serial(struct steam_device *steam)
 {
 	/*
@@ -511,15 +548,10 @@ static int steam_get_serial(struct steam_device *steam)
 	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
 	u8 reply[3 + STEAM_SERIAL_LEN + 1] = {0};
 
-	guard(mutex)(&steam->report_mutex);
-	ret = steam_send_report(steam, cmd, sizeof(cmd));
+	ret = steam_exchange_report(steam, cmd, sizeof(cmd), reply, sizeof(reply));
 	if (ret < 0)
 		return ret;
-	ret = steam_recv_report(steam, reply, sizeof(reply));
-	if (ret < 0)
-		return ret;
-	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
-	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
+	if (reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
 		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
 				(int)sizeof(reply), reply);
 		return -EIO;
@@ -537,14 +569,10 @@ static int steam_get_attributes(struct steam_device *steam)
 	int i;
 	struct steam_controller_attribute *attr;
 
-	guard(mutex)(&steam->report_mutex);
-	ret = steam_send_report(steam, cmd, sizeof(cmd));
-	if (ret < 0)
-		return ret;
-	ret = steam_recv_report(steam, reply, sizeof(reply));
+	ret = steam_exchange_report(steam, cmd, sizeof(cmd), reply, sizeof(reply));
 	if (ret < 0)
 		return ret;
-	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2) {
+	if (reply[1] < 2) {
 		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
 				(int)sizeof(reply), reply);
 		return -EIO;
-- 
2.54.0

Re: [PATCH v4 05/11] HID: steam: Add support for sensor events on the Steam Controller (2015)

[sashiko-bot]

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

New issues:
- [Medium] Enabling lizard mode via gamepad closure implicitly disables the IMU, breaking active sensor listeners.

Pre-existing issues:
- [High] If `steam_register()` fails for a wired controller in `steam_probe()`, the `steam` device is freed while remaining linked in the global `steam_devices` list.
- [High] Concurrent execution of workqueues causes double unregistration of the input and sensors devices due to a TOCTOU race condition.
--

--- Patch [5]: [PATCH v4 05/11] HID: steam: Add support for sensor events on the Steam Controller (2015) ---
commit bb1476f66b80150be67610fe42e5546b316d06ef
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Add support for sensor events on the Steam Controller (2015)

Sensor support was added for the Steam Deck previously, but Steam
Controller sensor events were never added. This adds that missing support,
bringing Steam Controller support much closer to feature parity with things
like SDL and Steam itself.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 8bad79205b570..cabecd250072e 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -625,6 +689,42 @@ static void steam_input_close(struct input_dev *dev)
>  	}
>  }
>  
> +static int steam_sensor_open(struct input_dev *dev)
> +{
> +	struct steam_device *steam = input_get_drvdata(dev);
> +	unsigned long flags;
> +	bool client_opened;
> +
> +	spin_lock_irqsave(&steam->lock, flags);
> +	client_opened = steam->client_opened;
> +	spin_unlock_irqrestore(&steam->lock, flags);
> +	if (client_opened)
> +		return 0;
> +
> +	guard(mutex)(&steam->report_mutex);
> +	steam_write_settings(steam, SETTING_IMU_MODE,
> +			SETTING_IMU_MODE_SEND_RAW_ACCEL | SETTING_IMU_MODE_SEND_RAW_GYRO,
> +			0);

[Severity: Medium]
Does closing the gamepad evdev node implicitly disable the IMU, breaking
active sensor listeners?

When the gamepad evdev node is closed, the driver automatically re-enables
"lizard mode" by calling steam_set_lizard_mode(steam, true) in
steam_input_close(). This function sends the ID_LOAD_DEFAULT_SETTINGS
command to the controller, which performs a hardware state reset.

This reset wipes out the SETTING_IMU_MODE configuration previously requested
by steam_sensor_open() here. Since the driver does not track or coordinate
the IMU state across these logically independent evdev nodes, closing the
gamepad node while the sensor node is still open will cause the controller
to silently stop sending motion data.

> +
> +	return 0;
> +}
> +
> +static void steam_sensor_close(struct input_dev *dev)
> +{

[ ... ]

> @@ -917,9 +1029,6 @@ static void steam_sensors_unregister(struct steam_device *steam)
>  {
>  	struct input_dev *sensors;
>  
> -	if (!(steam->quirks & STEAM_QUIRK_DECK))
> -		return;
> -
>  	rcu_read_lock();
>  	sensors = rcu_dereference(steam->sensors);
>  	rcu_read_unlock();

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent execution of
workqueues cause a double unregistration of the input and sensors devices
due to a TOCTOU race condition?

The workqueues work_connect and unregister_work cache the device state under
a spinlock, drop the lock, and then act on the cached state. Both can run
concurrently on different CPUs and call steam_sensors_unregister() or
steam_input_unregister() simultaneously.

In both unregister functions, the pointers (steam->sensors and steam->input)
are read under rcu_read_lock(), but the lock is dropped immediately before
checking if the pointer is NULL. Because there is no atomic swap or mutex
protecting the RCU_INIT_POINTER assignment, both threads can read the same
non-NULL pointer, wait for RCU, and call input_unregister_device() on the
same device, causing a double-free and list corruption in the input core.

> @@ -967,6 +1076,12 @@ static int steam_register(struct steam_device *steam)
>  			strscpy(steam->serial_no, "XXXXXXXXXX",
>  					sizeof(steam->serial_no));
>  
> +		ret = steam_get_attributes(steam);
> +		if (ret < 0)
> +			hid_err(steam->hdev,
> +				"%s:steam_get_attributes failed with error %d\n",
> +				__func__, ret);
> +

[Severity: High]
This is a pre-existing issue, but if steam_register() fails for a wired
controller in steam_probe(), is the steam device freed while remaining
linked in the global steam_devices list?

In steam_register(), the steam object is added to the global steam_devices
list under a lock. If registration subsequently fails (e.g., due to an
out-of-memory error in steam_input_register()), steam_register() returns an
error code without removing the object from the list.

For a wired controller, steam_probe() catches this error and jumps to
err_hw_close, bypassing err_steam_unregister and the steam_unregister()
function that would normally remove it from the list. When steam_probe()
returns the error, devres frees the steam object.

Any subsequent iteration over steam_devices (e.g., changing lizard mode via
sysfs) will dereference the freed memory, causing a use-after-free and
potential kernel panic.

>  		hid_info(steam->hdev, "Steam Controller '%s' connected",
>  				steam->serial_no);
>

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

Re: [PATCH v4 04/11] HID: steam: Zero-initialize reply in serial lookup

[sashiko-bot]

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

Pre-existing issues:
- [High] Improper teardown ordering in `steam_remove()` allows works to be re-scheduled and execute after `steam_device` is freed.
- [High] Race condition in `steam_probe()` error path allows a background work item to register a device and corrupt global state after error unwinding.
--

--- Patch [4]: [PATCH v4 04/11] HID: steam: Zero-initialize reply in serial lookup ---
commit 3fb5af0927e3da21c09244521c52f446d7eb0ba2
Author: Vicki Pfau <vi@endrift.com>

    HID: steam: Zero-initialize reply in serial lookup

    This patch eagerly zero-initializes the reply buffer in steam_get_serial()
    instead of just null-terminating it at the end. This prevents potential
    leaks of uninitialized memory in error logging paths.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 983d18d1de4fe..8bad79205b570 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -447,7 +447,7 @@ static int steam_get_serial(struct steam_device *steam)
>  	 */
>  	int ret = 0;
>  	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
> -	u8 reply[3 + STEAM_SERIAL_LEN + 1];
> +	u8 reply[3 + STEAM_SERIAL_LEN + 1] = {0};
>  
>  	mutex_lock(&steam->report_mutex);
>  	ret = steam_send_report(steam, cmd, sizeof(cmd));

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for works to
be rescheduled after they are cancelled in steam_remove()?

Looking at the code, it appears work items are cancelled before the hardware
events are stopped:

drivers/hid/hid-steam.c:steam_remove() {
    ...
	cancel_delayed_work_sync(&steam->mode_switch);
	cancel_work_sync(&steam->work_connect);
	cancel_work_sync(&steam->rumble_work);
	cancel_work_sync(&steam->unregister_work);
    ...
	hid_hw_close(hdev);
	hid_hw_stop(hdev);
}

If the hardware is still active, could incoming events trigger
steam_raw_event() and reschedule these works? 

Since steam_device is allocated via devm_kzalloc(), could this lead to a
use-after-free if the device is unplugged and a pending work later accesses
the freed memory?


[Severity: High]
This is a pre-existing issue, but I have a question about whether the error
handling in steam_probe() properly synchronizes with the background worker
thread.

If an error occurs during probe (for example, if steam_create_client_hid()
fails), the error path executes:

drivers/hid/hid-steam.c:steam_probe() {
    ...
err_steam_unregister:
	if (steam->connected)
		steam_unregister(steam);
err_hw_close:
	hid_hw_close(hdev);
err_hw_stop:
	hid_hw_stop(hdev);
err_cancel_work:
	cancel_work_sync(&steam->work_connect);
    ...
}

If the hardware concurrently triggers steam_work_connect_cb(), could the
worker thread be executing steam_register()?

In this scenario, steam_unregister() might do nothing if steam->serial_no
isn't set yet. Then cancel_work_sync() would block until the worker finishes
registering the input device and adding it to the global steam_devices list.

Once steam_probe() returns and the driver core frees the steam_device memory,
could this leave dangling pointers in the global list and input subsystem?

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

Re: [PATCH v4 09/11] HID: steam: Improve logging and other cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] The error path in `steam_register()` fails to remove the device from the global `steam_devices` list, leading to a Use-After-Free when probe fails.
--

--- Patch [9]: [PATCH v4 09/11] HID: steam: Improve logging and other cleanup ---
commit e2534775e852c76ea14b6b8605e36139ce7654b1
Author: Vicki Pfau <vi@endrift.com>

    HID: steam: Improve logging and other cleanup
    
    Adds more logging as appropriate, reindents an enum to match surrounding
    style, as well as cleaning up some places where we can use guard() instead
    of doing locking and unlocking manually.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 5ab4396a241e0..3738a4de3bffb 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -1838,6 +1847,7 @@ static void steam_do_deck_input_event(struct steam_device *steam,
>  		steam->did_mode_switch = false;
>  		cancel_delayed_work(&steam->mode_switch);
>  	} else if (!steam->client_opened && start_pressed && !steam->did_mode_switch) {
> +		hid_dbg(steam->hdev, "%s: doing mode switch\n", __func__);
>  		steam->did_mode_switch = true;
>  		schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
>  	}

[Severity: High]
This is a pre-existing issue, but the error path in steam_register() fails to
remove the device from the global steam_devices list, which could lead to a
use-after-free when probe fails.

If steam_register() fails during steam_probe() for a wired device (e.g., if
steam_sensors_register() returns an error), the steam object is added to the
global steam_devices list but is not removed on the error path:

drivers/hid/hid-steam.c:steam_register() {
    ...
steam_register_sensors_fail:
	steam_input_unregister(steam);
steam_register_input_fail:
	return ret;
}

steam_probe() then returns the error, jumping to the err_hw_stop label and
bypassing steam_unregister(), which would have removed it from the list:

drivers/hid/hid-steam.c:steam_probe() {
    ...
		ret = steam_register(steam);
		if (ret) {
			hid_err(hdev, "%s:steam_register failed with error %d\n", __func__, ret);
			goto err_hw_stop;
		}
    ...
}

Because the steam object was allocated with devm_kzalloc(), the kernel frees
it upon probe failure, leaving a dangling pointer in the steam_devices list.

Can this memory corruption be triggered by plugging in a second Steam
Controller, or by writing to the lizard_mode sysfs parameter?

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

Re: [PATCH v4 07/11] HID: steam: Fully unregister controller when hidraw is opened

[sashiko-bot]

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

New issues:
- [Medium] Unifying connect and unregister logic causes log spam and unnecessary I/O on hidraw open/close.

Pre-existing issues:
- [High] Dangling pointer in the global `steam_devices` list on probe failure.
--

--- Patch [7]: [PATCH v4 07/11] HID: steam: Fully unregister controller when hidraw is opened ---
commit d6b711a20faba4cc20306f07e01853b79872c545
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Fully unregister controller when hidraw is opened

To avoid conflicts between anything touching the hidraw and the driver, this
patch fully unregisters the controller internally when hidraw is opened,
leaving only the hidraw active until it's closed. It also unifies the
unregister and connect callbacks.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index a200878db8ac7..ff112aa1b0e91 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1141,38 +1142,41 @@ static int steam_register(struct steam_device *steam)
[ ... ]
>  static void steam_work_connect_cb(struct work_struct *work)
>  {
>  	struct steam_device *steam = container_of(work, struct steam_device,
>  							work_connect);
> +
>  	unsigned long flags;
>  	bool connected;
> +	bool opened;
>  	int ret;
>  
>  	spin_lock_irqsave(&steam->lock, flags);
> +	opened = steam->client_opened;
>  	connected = steam->connected;
>  	spin_unlock_irqrestore(&steam->lock, flags);
>  
> -	if (connected) {
> +	if (connected && !opened) {
>  		ret = steam_register(steam);

[Severity: High]
This is a pre-existing issue, but does steam_register() leak the list
entry if it fails?

If steam_register() fails (e.g., if input_allocate_device() fails inside
steam_input_register()), the device is added to the global steam_devices
list but not removed.

Also, in steam_probe(), if a wired Steam controller is probed but an error
occurs later, the error path jumps to err_hw_stop, bypassing
err_steam_unregister. Devres will free the steam_device structure, leaving a
dangling pointer in steam_devices. Could this lead to a use-after-free if the
list is later traversed (e.g., when a user modifies lizard_mode)?

> -		if (ret) {
> +		if (ret)
>  			hid_err(steam->hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -		}
>  	} else {
>  		steam_unregister(steam);
>  	}

[Severity: Medium]
By unifying the connect and unregister logic here, will this cause log spam
and unnecessary synchronous USB I/O when hidraw is opened and closed?

steam_unregister() clears the serial number and logs a disconnect message.
When hidraw is closed, steam_register() logs a connect message and performs
synchronous USB I/O to re-fetch the serial number and device attributes.

Could an unprivileged userspace application rapidly open and close the
hidraw node (which is typical for gamepads managed by udev), flooding the
kernel log and triggering localized DoS via USB I/O?

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

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

[sashiko-bot]

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 <vi@endrift.com>

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/20260729015243.1170573-1-vi@endrift.com?part=6

Re: [PATCH v4 08/11] HID: steam: Rearrange teardown sequence

[sashiko-bot]

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

New issues:
- [High] Calling `hid_hw_start()` before initializing driver data structures exposes the device to events early, leading to crashes from uninitialized locks and work_structs.
- [High] The modified error path in `steam_probe()` skips the cancellation of `work_connect` and `mode_switch`, leaving them dangling on probe failure.

Pre-existing issues:
- [High] Teardown ordering in `steam_remove()` allows `work_connect` to be re-scheduled by raw events after it has been explicitly cancelled.
- [High] `steam->client_hdev` is destroyed before the `client_opened` flag is cleared, allowing asynchronous events to dereference the freed device.
- [High] If `steam_register()` fails during initialization, the `steam` object is freed but remains linked in the global `steam_devices` list.
--

--- Patch [8]: [PATCH v4 08/11] HID: steam: Rearrange teardown sequence ---
commit 4778053b21e62ca367e7570ac85b4132006adade
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Rearrange teardown sequence

This fixes a narrow window during the teardown sequence where callbacks
could still be scheduled during cleanup that would then have a dangling
pointer to the now-freed steam struct. This also puts work canceling
for rumble and mode switch in steam_unregister, as that shouldn't persist
while the client hdev is open.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index ff112aa1b0e91..5ab4396a241e0 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1352,6 +1356,14 @@ static int steam_probe(struct hid_device *hdev,
>  	if (!steam)
>  		return -ENOMEM;
>  
> +	/*
> +	 * With the real steam controller interface, do not connect hidraw.
> +	 * Instead, create the client_hid and connect that.
> +	 */
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> +	if (ret)
> +		return ret;
> +
>  	steam->hdev = hdev;
>  	hid_set_drvdata(hdev, steam);
>  	spin_lock_init(&steam->lock);

[Severity: High]
Does calling hid_hw_start() before initializing driver data structures expose
the device to events early?

Moving hid_hw_start() here makes the input devices visible to userspace.
If a userspace process opens the input node, hid_hw_open() is called, starting
URB polling. An incoming hardware interrupt can then invoke steam_raw_event().

Because hid_set_drvdata() is called right after hid_hw_start(),
steam_raw_event() could access the non-NULL steam pointer and call
steam_do_connect_event(), which might attempt to acquire the uninitialized
spinlock and schedule the uninitialized work_connect, potentially leading to
a crash.

[ ... ]
> @@ -1397,14 +1393,22 @@ static int steam_probe(struct hid_device *hdev,
>  			hid_err(hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -			goto err_hw_close;
> +			goto err_hw_stop;

[Severity: High]
This is a pre-existing issue, but if steam_register() fails during
initialization, does the steam object remain linked in the global
steam_devices list after being freed?

During steam_register(), the device is added to the global steam_devices list.
If a subsequent step like steam_input_register() fails, the function returns
an error without removing the device from the list.

When this failure jumps to err_hw_stop in steam_probe(), it bypasses
steam_unregister(), which is responsible for removing it from the list.
If devres then frees the object, could a dangling pointer be left in the
global list?

[ ... ]
>  err_destroy:
>  	hid_destroy_device(steam->client_hdev);
> -err_steam_unregister:
> -	if (steam->connected)
> -		steam_unregister(steam);
>  err_hw_close:
>  	hid_hw_close(hdev);
> -err_hw_stop:
> -	hid_hw_stop(hdev);
> -err_cancel_work:
> +err_steam_unregister:
> +	spin_lock_irqsave(&steam->lock, flags);
> +	steam->client_opened = 0;
> +	spin_unlock_irqrestore(&steam->lock, flags);
>  	cancel_work_sync(&steam->work_connect);
> +	if (steam->connected)
> +		steam_unregister(steam);
>  	cancel_delayed_work_sync(&steam->mode_switch);
> +err_hw_stop:

[Severity: High]
Does the modified error path in steam_probe() skip the cancellation of
work_connect and mode_switch when a probe failure jumps to err_hw_stop?

Because hid_hw_start() was moved earlier in the probe, raw events might already
queue work_connect. If the probe fails and jumps directly to err_hw_stop,
bypassing err_steam_unregister, could it leave these work items dangling when
devres frees the steam object?

>  	cancel_work_sync(&steam->rumble_work);
>  	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> +	hid_hw_stop(hdev);
>  
>  	return ret;
>  }
> @@ -1435,25 +1441,24 @@ static int steam_probe(struct hid_device *hdev,
>  static void steam_remove(struct hid_device *hdev)
>  {
>  	struct steam_device *steam = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
>  	if (!steam || hdev->group == HID_GROUP_STEAM) {
>  		hid_hw_stop(hdev);
>  		return;
>  	}
>  
> +	hid_hw_close(hdev);
>  	hid_destroy_device(steam->client_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);
> -	steam->client_hdev = NULL;
> +	spin_lock_irqsave(&steam->lock, flags);
>  	steam->client_opened = 0;
> +	spin_unlock_irqrestore(&steam->lock, flags);

[Severity: High]
This is a pre-existing issue, but is steam->client_hdev destroyed before
the client_opened flag is cleared?

Because steam_raw_event() runs in softirq context without taking a lock around
the client_opened check, there could be a race condition.

If an interrupt sees client_opened > 0 right before hid_destroy_device()
completes, could it pass the freed client_hdev pointer to hid_input_report()?

> +	cancel_work_sync(&steam->work_connect);
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver disconnected");
>  	}
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  	steam_unregister(steam);
> +	hid_hw_stop(hdev);

[Severity: High]
This is a pre-existing issue, but does the teardown ordering in steam_remove()
allow work_connect to be re-scheduled by raw events after it has been explicitly
cancelled?

cancel_work_sync(&steam->work_connect) is called above, but URB polling is only
guaranteed to stop at the end of the function when hid_hw_stop() is called.

If a hardware event arrives after the cancellation but before hid_hw_stop(),
could it re-queue work_connect via steam_raw_event(), leaving the work pending
after the driver finishes removal and frees the device?

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