Re: [PATCH v11 01/18] HID: asus: add support for ROG Ally handhelds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] HID: asus: untracked endpoints claim udev in hid_asus_ally_probe()
- [Low] HID: asus: duplicate and unused BTN_TRIGGER_HAPPY1 capability in ally_x_setup_input()
--

commit 2082b890016be96a18a64dd50b259758d2e05629
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for ROG Ally handhelds

This commit adds ROG Ally handheld support to hid-asus, including device
readiness checks, Ally X gamepad input mapping, AC button long-press handling,
and shared state tracking across the controller's USB interfaces.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f6aa1be09ad53..9e6796bbbdcbd 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -240,12 +297,906 @@ static const struct asus_touchpad_info medion_e1239t_tp = {
[ ... ]
> +static bool ally_x_raw_event(struct input_dev *input, u8 *data, int size)
> +{
[ ... ]
> +	byte = in_report->buttons[1];
> +	input_report_key(input, BTN_THUMBL, byte & BIT(0));
> +	input_report_key(input, BTN_THUMBR, byte & BIT(1));
> +	input_report_key(input, BTN_MODE, byte & BIT(2));
[ ... ]
> +static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *ally)
> +{
[ ... ]
> +	input_set_capability(input, EV_KEY, KEY_PROG1);
> +	input_set_capability(input, EV_KEY, KEY_F16);
> +	input_set_capability(input, EV_KEY, KEY_F17);
> +	input_set_capability(input, EV_KEY, KEY_F18);
> +	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY);
> +	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);

[Severity: Low]
In include/uapi/linux/input-event-codes.h, BTN_TRIGGER_HAPPY and
BTN_TRIGGER_HAPPY1 both expand to 0x2c0, while the second button is
BTN_TRIGGER_HAPPY2 (0x2c1).

At the same time, ally_x_raw_event() only reports bits 0..2 from
in_report->buttons[1] and never emits any BTN_TRIGGER_HAPPY* events.

Were these two capabilities intended to be BTN_TRIGGER_HAPPY1 and
BTN_TRIGGER_HAPPY2 with corresponding reporting in ally_x_raw_event(), or
should they be dropped from ally_x_setup_input()?

> +
> +	ret = input_register_device(input);
[ ... ]
> +static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
> +{
[ ... ]
> +	/* Reserve the role for as long as the initialization below runs. */
> +	if (ep == HID_ALLY_INTF_CFG_IN)
> +		ally_drvdata.cfg_probe_in_progress = true;
> +	else if (ep == HID_ALLY_X_INTF_IN)
> +		ally_drvdata.ally_x_probe_in_progress = true;
> +	else if (ep == HID_ALLY_INTF_KEYBOARD_IN)
> +		ally_drvdata.keyboard_probe_in_progress = true;
> +
> +	if (!ally_drvdata.udev) {
> +		usb_get_dev(udev);
> +		ally_drvdata.udev = udev;
> +	}
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +	/*
> +	 * The interface initialization sleeps (it performs USB transfers),
> +	 * so it must not run under ally_data_lock: the role stays reserved
> +	 * while it runs, and the pointers are published only once it
> +	 * succeeded.
> +	 */
> +	switch (ep) {
[ ... ]
> +	default:
> +		/* This is normally supposed to happen */
> +		break;
> +	}
> +
> +	return &ally_drvdata;
> +}

[Severity: Medium]
Can an untracked endpoint on the composite USB device (such as the mouse
interface, which has an endpoint address other than HID_ALLY_INTF_CFG_IN,
HID_ALLY_X_INTF_IN, or HID_ALLY_INTF_KEYBOARD_IN) cause ally_drvdata.udev to
get out of sync with the bound interfaces?

In hid_asus_ally_probe(), any endpoint matching QUIRK_ROG_ALLY_XPAD acquires
ally_drvdata.udev before switching on ep, and then hits the default branch
without setting any *_hdev pointer or *_probe_in_progress flag, while still
returning &ally_drvdata to be stored in drvdata->rog_ally.

Because ally_put_udev_if_orphaned() only checks the three tracked roles:

	if (!ally_drvdata.udev || ally_drvdata.keyboard_hdev ||
	    ally_drvdata.cfg_hdev || ally_drvdata.ally_x_hdev ||
	    ally_drvdata.cfg_probe_in_progress ||
	    ally_drvdata.ally_x_probe_in_progress ||
	    ally_drvdata.keyboard_probe_in_progress) {
		spin_unlock_irqrestore(&ally_data_lock, flags);
		return;
	}

two regressions seem possible:

1. If a default-branch interface probes first and a tracked role interface
subsequently fails in hid_asus_ally_probe() (or if the three tracked role
interfaces are later unbound via sysfs while the default-branch interface
remains bound), ally_put_udev_if_orphaned() sees all tracked pointers and
reservation flags cleared and drops ally_drvdata.udev while the
default-branch interface still holds drvdata->rog_ally = &ally_drvdata.
Unbinding that interface later will run hid_asus_ally_remove() ->
cancel_delayed_work_sync() and ally_put_udev_if_orphaned() against global
state that may now belong to another connected controller.

2. Conversely, if only a default-branch interface binds (for example, if the
three tracked role interfaces fail in hid_parse() or hid_hw_start() before
reaching hid_asus_ally_probe(), or on a device with the Ally VID/PID exposing
only other endpoints), ally_drvdata.udev stays pinned with no tracked role
bound, preventing any other controller from probing.

Should hid_asus_ally_probe() return NULL (or avoid claiming ally_drvdata.udev
and &ally_drvdata) when ep is not one of HID_ALLY_INTF_CFG_IN,
HID_ALLY_X_INTF_IN, or HID_ALLY_INTF_KEYBOARD_IN?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025235232.129945-1-denis.benato@linux.dev?part=1