Re: [PATCH 09/12] HID: asus: add support to force feedback

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 <[email protected]>
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.

> 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

[ ... ]

> @@ -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);
> +}

[ ... ]

> @@ -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);

[ ... ]

> @@ -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/[email protected]?part=9