Re: [PATCH v3 04/12] HID: asus: add vibration strength configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing hardware state restoration for vibration intensity on device resume.
- [Medium] Partial failure of the sysfs initialization loop leaves dangling sysfs groups and leaks devres actions.
--

--- Patch [4]: [PATCH v3 04/12] HID: asus: add vibration strength configuration ---
commit bae59c4354c6c0f337acefd797d650cf39840ae7
Author: Denis Benato <denis.benato@linux.dev>

    HID: asus: add vibration strength configuration
    
    ASUS ROG Ally handhelds support the vibration strength to be configured:
    add sysfs attributes to allow userspace configure motors vibration
    intensity.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 04f41294edf67..dc1a4fb6c9e9e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -206,6 +206,10 @@ struct ally_config {
>  	u8 left_trigger_max;
>  	u8 right_trigger_min;
>  	u8 right_trigger_max;
> +
> +	/* Vibration settings */
> +	u8 vibration_intensity_left;
> +	u8 vibration_intensity_right;
>  };
>  

[Severity: Medium]
Will this newly added state be lost after a system suspend/resume cycle?

Looking at hid_asus_ally_init(), it restores the gamepad mode and Xbox
controller state after an MCU reset, but doesn't seem to restore these
vibration intensity values:

drivers/hid/hid-asus.c:hid_asus_ally_init() {
	...
	if (cfg->xbox_controller_enabled) {
		ret = ally_set_xbox_controller(hdev, ally, cfg, true);
	...
}

Could this lead to a state desynchronization? After resume, the controller
hardware resets its vibration intensity to the default 100%, but the sysfs
files might still report the user's previously set custom value until manually
overwritten.

[ ... ]

> @@ -811,15 +815,193 @@ static ssize_t xbox_controller_store(struct device *dev,

[ ... ]

>  static const struct attribute_group ally_attr_groups[] = {
>  	{
>  		.attrs = ally_config_attrs,
>  	},
> +	{
> +		.name = "left_vibration",
> +		.attrs = ally_left_vibration_attrs,
> +	},
> +	{
> +		.name = "right_vibration",
> +		.attrs = ally_right_vibration_attrs,
> +	},
>  };

[Severity: Medium]
Does adding new groups here expose a partial failure cleanup issue during
initialization?

If devm_device_add_group() fails for the second or third element in
ally_attr_groups, ally_config_create() returns an error without rolling
back the successfully added groups:

drivers/hid/hid-asus.c:ally_config_create() {
	...
	for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_attr_groups); sysfs_i++) {
		ret = devm_device_add_group(&hdev->dev, &ally_attr_groups[sysfs_i]);
		if (ret < 0) {
			hid_err(hdev, "Failed to create sysfs group '%s': %d\n",
				ally_attr_groups[sysfs_i].name ?: "", ret);
			goto ally_config_create_sysfs_err;
		}
	...
}

This error propagates up to asus_probe(), which explicitly ignores the
failure and returns 0. Since the overall device probe succeeds, the devres
rollback that would normally remove the successfully added groups is bypassed.

Could this leave dangling sysfs files registered on the device while the
cfg structure is freed via devm_kfree(), since the sysfs actions are never
cleaned up?

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