Re: [PATCH 09/12] HID: asus: add support to force feedback
From: sashiko-bot@kernel.org
Date: 2026-08-13 15:10:17
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Ignoring the return value of `input_ff_create_memless` and proceeding with `input_register_device` leads to a guaranteed NULL pointer dereference in `input_ff_upload()`. - [High] Disabling the force-feedback workqueue in `hid_asus_ally_remove()` before stopping the hardware leaves the device actively rumbling. - [Medium] Force feedback output report is incorrectly sent as a Feature report, and during initialization it is sent to the wrong USB interface. --
--- Patch [9]: [PATCH 09/12] HID: asus: add support to force feedback ---commit 84e3caa9147b2fcf6cc7ac8459e2b9e55841248a Author: Denis Benato [off-list ref] Subject: HID: asus: add support to force feedback This commit adds force feedback (FF_RUMBLE) support for the ROG Ally X by implementing a work-queue based approach for sending vendor-specific DInput protocol rumble packets.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index 8018e61cf0264..301fe33f66b1f 100644 --- a/drivers/hid/hid-asus.c +++ b/drivers/hid/hid-asus.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2254,6 +2282,59 @@ static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev, return true; } +static void ally_x_ff_work_fn(struct work_struct *work) +{
[ ... ]
+ if (!update || !hdev) + return; + + ret = ally_gamepad_send_packet(ally, hdev, (u8 *)&report, sizeof(report));
[Severity: Medium] Does ally_gamepad_send_packet() send this data as a Feature report instead of an Output report? The ff_data struct is documented as an XInput force-feedback output report (0x0d), but if ally_gamepad_send_packet() internally hardcodes the transfer as a HID_FEATURE_REPORT, will the hardware reject this packet?
+ if (ret < 0) + hid_err(hdev, "Failed to send force-feedback: %d\n", ret); +}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2308,6 +2389,16 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY); input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1); + memcpy(&ally->ff_packet, ALLY_FORCE_FEEDBACK_OFF, sizeof(ally->ff_packet)); + spin_lock_init(&ally->ff_lock); + INIT_WORK(&ally->ff_work, ally_x_ff_work_fn); + ally->ff_work_initialized = true; + + input_set_capability(input, EV_FF, FF_RUMBLE); + ret = input_ff_create_memless(input, NULL, ally_x_play_effect); + if (ret) + hid_warn(hdev, "Failed to create force-feedback: %d\n", ret); + ret = input_register_device(input);
[Severity: High] If input_ff_create_memless() fails, will this leave dev->ff as NULL while EV_FF is still advertised? Since the warning is printed but input_register_device() is still called on failure, could this cause a NULL pointer dereference in input_ff_upload() when userspace invokes the EVIOCSFF ioctl on the registered device?
if (ret) {
hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);[ ... ]
quoted hunk ↗ jump to hunk
@@ -2460,6 +2551,9 @@ static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld * scoped_guard(mutex, &ally_data_mutex) { if (ally->ally_x_hdev == hdev) { + scoped_guard(spinlock_irqsave, &ally->ff_lock) + ally->ff_work_initialized = false; + cancel_work_sync(&ally->ff_work);
[Severity: High] Will this leave the device vibrating indefinitely if it is unbound while rumbling? By setting ff_work_initialized to false and synchronously canceling the workqueue here, before the devres action has a chance to unregister the input device, won't the input subsystem's subsequent stop commands be silently discarded by the disabled workqueue?
ally->ally_x_input = NULL; ally->ally_x_hdev = NULL; }
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260813144736.2477941-1-denis.benato@linux.dev?part=9