Re: [PATCH] HID: microsoft: normalize Xbox Series X|S controller axes over Bluetooth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: microsoft: swapped left and right trigger axes in
  ms_input_mapping()
- [High] HID: microsoft: breaking evdev ABI and trigger mapping in
  ms_input_mapping()
- [Medium] HID: microsoft: unfiltered stick events in ms_event() cause
  defuzz drift
--

commit b00037d015a7b7ec287323be001d24d5b680b0c0
Author: Adolf Hirthe <ohirthe@proton.me>

HID: microsoft: normalize Xbox Series X|S controller axes over Bluetooth

This commit adds an MS_QUIRK_BLE_XBOX_AXES quirk to hid-microsoft to remap
the Bluetooth LE stick and trigger axes of the Xbox Series X|S controller
(045e:0b13) to the xpad axis layout and signed stick range.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=222266

> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 226733718c488..7e426ee85a289 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c

[ ... ]

> @@ -186,6 +187,41 @@ static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	struct ms_data *ms = hid_get_drvdata(hdev);
>  	unsigned long quirks = ms->quirks;
>  
> +	/*
> +	 * Xbox controllers over Bluetooth report the right stick as Z/Rz and
> +	 * the analog triggers as Accelerator/Brake (ABS_GAS/ABS_BRAKE), and
> +	 * expose the sticks in the 0..65535 range centered on 32767 instead
> +	 * of the usual +-32767 around 0. Remap everything to the xpad-style
> +	 * layout so that consumers get the same axes regardless of the
> +	 * connection type.
> +	 */
> +	if (quirks & MS_QUIRK_BLE_XBOX_AXES) {
> +		switch (usage->hid) {
> +		case HID_GD_X:
> +			hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_X);
> +			return 1;
> +		case HID_GD_Y:
> +			hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_Y);
> +			return 1;
> +		case HID_GD_Z:
> +			hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_RX);
> +			return 1;
> +		case HID_GD_RZ:
> +			hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_RY);
> +			return 1;
> +		}
> +		if ((usage->hid & HID_USAGE_PAGE) == HID_UP_SIMULATION) {
> +			switch (usage->hid & HID_USAGE) {
> +			case 0xc5: /* Brake -> right trigger */
> +				hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_RZ);
> +				return 1;
> +			case 0xc4: /* Accelerator -> left trigger */
> +				hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_Z);
> +				return 1;
> +			}
> +		}
> +	}

[Severity: High]
Are the left and right triggers swapped here relative to xpad?

In the Xbox Bluetooth HID report descriptor, Simulation Controls usage
0xc5 (Brake) is the physical left trigger and 0xc4 (Accelerator) is the
physical right trigger.  In xpad, the left trigger is reported on ABS_Z
and the right trigger on ABS_RZ:

drivers/input/joystick/xpad.c:xpadone_process_packet() {
    ...
		/* triggers left/right */
		if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
			input_report_key(dev, BTN_TL2,
					(__u16) le16_to_cpup((__le16 *)(data + 6)));
			input_report_key(dev, BTN_TR2,
					(__u16) le16_to_cpup((__le16 *)(data + 8)));
		} else {
			input_report_abs(dev, ABS_Z,
					(__u16) le16_to_cpup((__le16 *)(data + 6)));
			input_report_abs(dev, ABS_RZ,
					(__u16) le16_to_cpup((__le16 *)(data + 8)));
		}
    ...
}

Mapping 0xc5 to ABS_RZ and 0xc4 to ABS_Z inverts the left and right
triggers on every pull.

[ ... ]

> @@ -232,6 +268,22 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>  
>  	input = field->hidinput->input;
>  
> +	/*
> +	 * The sticks are reported as unsigned 0..65535 values centered on
> +	 * 32767; convert them to the +-32767 range we advertise.
> +	 */
> +	if (quirks & MS_QUIRK_BLE_XBOX_AXES && usage->type == EV_ABS) {
> +		switch (usage->code) {
> +		case ABS_X:
> +		case ABS_Y:
> +		case ABS_RX:
> +		case ABS_RY:
> +			input_report_abs(input, usage->code,
> +					 clamp(value - 32768, -32767, 32767));
> +			return 1;
> +		}
> +	}

[Severity: Medium]
Can calling input_report_abs() unconditionally in ms_event() cause
spurious stick drift events when other buttons or triggers are pressed?

hid_process_report() calls hid_process_event() for every variable field in
an incoming report before updating field->value.  Normally,
hidinput_hid_event() drops unchanged values when value ==
field->value[usage->usage_index].  Because ms_event() intercepts the stick
axes and returns 1 without that check, input_report_abs() runs for all
four stick axes on every button or trigger report.  With fuzz set to 255
in ms_input_configured(), each repeated call with an unchanged raw stick
value within the defuzz window re-runs input_defuzz_abs_event() against
dev->absinfo[code].value, stepping the reported stick position and
emitting EV_ABS events while the stick is stationary.

[ ... ]

> @@ -428,7 +490,7 @@ static const struct hid_device_id ms_devices[] = {
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
>  		.driver_data = MS_QUIRK_FF },
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
> -		.driver_data = MS_QUIRK_FF },
> +		.driver_data = MS_QUIRK_FF | MS_QUIRK_BLE_XBOX_AXES },
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
>  		.driver_data = MS_QUIRK_FF },
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),

[Severity: High]
Won't changing the evdev axis codes and ranges for
USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914 (045e:0b13) cause a
regression for existing userspace input stacks?

This device ID has been bound by hid-microsoft since commit f5554725f304
("HID: microsoft: Add rumble support to latest xbox controllers") with the
standard 1:1 HID mapping (ABS_Z/ABS_RZ for the right stick in 0..65535 and
ABS_GAS/ABS_BRAKE for triggers).  Userspace libraries such as SDL,
gamecontrollerdb, Steam Input, and joydev match (BUS_BLUETOOTH, 0x045e,
0x0b13) and expect those axis codes, ranges, and ordinal axis positions.
In addition, 0xc5 (Brake) and 0xc4 (Accelerator) are mapped to ABS_RZ and
ABS_Z backwards relative to xpad, and sibling BLE models
USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE (045e:0b20) and
USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE (045e:0b22) added in the
same commit are left with the old layout.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-hid-ms-xbox-ble-v1-1-0b114dc5dcf3@proton.me?part=1