Re: [PATCH v11 01/18] HID: asus: add support for ROG Ally handhelds
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-26 00:10:26
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ 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)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ 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)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ /* 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) {
[ ... ]
quoted hunk ↗ jump to hunk
+ 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