Re: [PATCH v6 10/13] HID: asus: add support to force feedback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Initialization race: Force-feedback is explicitly disabled after the input device is registered, which can overwrite initial userspace effects.
- [Medium] The patch modifies `ally_config_remove` to check `cfg->resp_curve_support`, which is an unrelated silent fix.
--

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

HID: asus: add support to force feedback

Unlike ROG ally the X version and following ones uses DInput protocol
and the force feedback needs to be implemented as its protocol is
vendor-specific, therefore add support for FF_RUMBLE with magnitude
scaling on a work-queue based approach to avoid using possibly
sleeping calls in atomic context.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f338c0eed54b3..a75e6bd947907 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2544,7 +2580,8 @@ static void ally_config_remove(struct hid_device *hdev, struct ally_config *cfg)
>  	 * create them, so a device without those capabilities does not
>  	 * trigger a "not found" warning.
>  	 */
> -	if (cfg->user_cal_support || cfg->anti_deadzone_support) {
> +	if (cfg->user_cal_support || cfg->anti_deadzone_support ||
> +	    cfg->resp_curve_support) {

[Severity: Medium]
This modifies the sysfs group cleanup behavior for joystick response curves,
which appears unrelated to the stated goal of adding force-feedback support.

Should this change be split into a separate patch to keep changes focused and
match the commit message?

>  		for (i = 0; i < ARRAY_SIZE(ally_cal_attr_groups); i++)
>  			sysfs_remove_group(&hdev->dev.kobj,
>  						   ally_cal_attr_groups[i]);

[ ... ]

> @@ -3074,9 +3264,17 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  			return ERR_PTR(ret);
>  		}
>  
> -		spin_lock_irqsave(&ally_data_lock, flags);
> -		ally_drvdata.ally_x_hdev = hdev;
> -		spin_unlock_irqrestore(&ally_data_lock, flags);
> +		/*
> +		 * Make sure rumble starts disabled: this is the interface that
> +		 * owns the force-feedback output report. Failure is non-critical.
> +		 */
> +		scoped_guard(mutex, &ally_drvdata.intf_mutex) {
> +			ret = ally_x_send_ff_off(hdev);

[Severity: Medium]
Because ally_x_setup_input() runs just before this block and registers the
input device, the device node is exposed to userspace before this hardware
initialization packet is sent.

Can this result in a race where a userspace daemon immediately opens the device
and sends an initial rumble effect, which then gets silently canceled by this
OFF packet?

> +			if (ret < 0)
> +				hid_warn(hdev, "Failed to disable force-feedback: %d\n",
> +					  ret);
> +		}
> +
>  		break;
>  	case HID_ALLY_INTF_KEYBOARD_IN:

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