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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] `ally_set_vibration_intensity()` directly sends raw HID requests without acquiring the required `ally->intf_mutex`, leading to potential race conditions and device state corruption.
- [Low] If `devm_device_add_group()` fails for one of the newly added vibration sysfs groups, the previously registered groups are not rolled back if the overall device probe succeeds.
--

--- Patch [4]: [PATCH v2 04/12] HID: asus: add vibration strength configuration ---
commit fd873329418038e807b5d4833861817dc262fc2b
Author: Denis Benato <[email protected]>
Subject: HID: asus: add vibration strength configuration

This patch adds support for configuring the vibration strength of ASUS ROG
Ally handhelds. It introduces sysfs attributes to allow userspace to control
the intensity of the left and right motors.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 900914d21ca46..6b005afe01a24 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -771,15 +775,189 @@ static ssize_t xbox_controller_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(xbox_controller);
>  
> +/**
> + * ally_set_vibration_intensity() - Set vibration intensity values
> + * @hdev: HID device
> + * @left: Left motor intensity (0-100)
> + * @right: Right motor intensity (0-100)
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int ally_set_vibration_intensity(struct hid_device *hdev, u8 left, u8 right)
> +{
> +	const u8 data[] = { left, right };
> +	int ret;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_VIBRATION_INTENSITY, data, sizeof(data));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);

[Severity: Medium]
Does this direct call to ally_dev_set_report() need to be protected by 
ally->intf_mutex?

Other commands like CMD_SET_XBOX_CONTROLLER use ally_gamepad_send_packet() 
which takes the mutex:

static int ally_gamepad_send_packet(...)
{
        scoped_guard(mutex, &ally->intf_mutex)
                return ally_dev_set_report(hdev, buf, len);
}

Without the lock, could concurrent sysfs writes to the vibration attributes
race with other hardware communication on the IN interface, potentially breaking
the strict SET/GET pairing expected by initialization routines?

> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to set vibration intensity: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

>  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: Low]
If devm_device_add_group() fails for one of these newly added sysfs groups
during the registration loop in ally_config_create(), and the overall device
probe continues and succeeds, are the previously registered groups properly
cleaned up?

Since devres actions are only rolled back when the probe fails or the device
is unbound, it looks like a failure partway through the loop might leave
partially registered sysfs groups visible indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4