Re: [PATCH v7 12/12] Input: xbox_gip - Add wheel support

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

New issues:
- [Medium] Late length checks in input report handlers leave events unsynchronized on truncated packets.
- [Medium] Incorrect modulo arithmetic for relative dial causes erratic reverse jumps.
- [Medium] Static configuration of `max_angle` with 0 precision causes integer underflow, bypassing validation.

Pre-existing issues:
- [High] AB-BA deadlock between LED sysfs operations and device unregistration.
--

commit 533e8fbcd84078cc304b29f07232970fb0a6e7d7
Author: Vicki Pfau [off-list ref]

    Input: xbox_gip - Add wheel support
    
This adds preliminary support for racing wheel support in xbox_gip,
exposing them mapped to the newly added axes.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does adding these devices expose them to
the AB-BA deadlock between LED sysfs operations and device unregistration?

During device disconnect, gip_reset_attachment() acquires attachment->lock and
calls gip_free_devices(), which manually calls input_unregister_device(). If
there are no open evdev handles, this synchronously tears down the device and
its devres resources, invoking led_classdev_unregister(). This function blocks
in kernfs_drain waiting for active sysfs operations to complete. Concurrently,
a sysfs write to the LED brightness holds the kernfs active reference and calls
gip_guide_led_set(), which blocks trying to acquire attachment->lock.

[ ... ]

+static int gip_handle_wheel_report(struct gip_attachment *attachment,
+	struct input_dev *input, const uint8_t *bytes, int num_bytes)
+{
+	uint8_t connections;
+	struct gip_wheel_info *info = attachment->driver_data;
+	int rc = gip_driver_navigation.handle_input_report(attachment, input, bytes, num_bytes);
+
+	if (rc < 0)
+		return rc;
+
+	if (!info)
+		return -ENODEV;
+
+	if (num_bytes < 17)
+		return -EINVAL;

[Severity: Medium]
Will this length check leave partial events unsynchronized if the packet is
truncated?

If num_bytes is less than 17, this returns -EINVAL after
gip_driver_navigation.handle_input_report() has already reported events via
input_report_key(). The outer function will then skip input_sync(), leaving the
reported events stuck in the input core's buffers until the next sync.

[ ... ]

+static int gip_handle_wheel_ll_static_configuration(struct gip_attachment *attachment,
+	const struct gip_header *header, const uint8_t *bytes, int num_bytes)
+{
+	struct gip_wheel_info *info = attachment->driver_data;
+
+	if (!info)
+		return -ENODEV;
+
+	if (num_bytes < 11)
+		return -EINVAL;
+
+	info->max_angle = BIT(min(bytes[0], 16)) / 2 - 1;

[Severity: Medium]
Could this formula cause an integer underflow if bytes[0] is 0?

If a 0-bit precision configuration is received, BIT(0) is 1, and 1 / 2 - 1
evaluates to -1, which underflows the uint16_t info->max_angle to 65535. This
could bypass validation checks and assign corrupted bounds to ABS_WHEEL.

[ ... ]

+static int gip_handle_trueforce_wheel_report(struct gip_attachment *attachment,
+	struct input_dev *input, const uint8_t *bytes, int num_bytes)
+{
+	int rc = gip_driver_wheel.handle_input_report(attachment, input, bytes, num_bytes);
+	struct gip_trueforce_wheel_state *state = attachment->driver_data;
+	int dial;
+
+	if (rc < 0)
+		return rc;
+
+	if (num_bytes < 18)
+		return -EINVAL;

[Severity: Medium]
Will this length check leave partial events unsynchronized if the packet is
truncated?

Similar to the check in gip_handle_wheel_report(), this returns -EINVAL after
the base wheel handler has already injected state changes, causing the core loop
to abort before calling input_sync().

[ ... ]

+	/* Dial states only change one at a time, so we can easily check for wraparound */
+	if (dial == 0 && state->dial == 7)
+		input_report_rel(input, REL_DIAL, -1);
+	else if (dial == 7 && state->dial == 0)
+		input_report_rel(input, REL_DIAL, 1);
+	else if (state->dial >= 0)
+		input_report_rel(input, REL_DIAL,
+			state->dial - dial);

[Severity: Medium]
Could this cause erratic relative dial jumps if the dial skips a step?

If the dial moves rapidly across the 0-7 boundary and skips a step (e.g., from
previous state 6 to current dial 0), the hardcoded wraparound checks fail. It
then falls back to state->dial - dial (6 - 0 = 6), reporting +6 instead of the
actual shortest distance of -2.

Should standard modulo arithmetic be used here instead?

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