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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Lockless read-modify-write (TOCTOU) race condition in vibration sysfs store functions causes hardware-software state mismatch.
- [Low] The patch introduces an unused struct field `vibration_active` and an unused function parameter `cfg`.
--

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

HID: asus: add vibration strength configuration

This commit adds sysfs attributes to allow userspace to configure the
vibration intensity of the motors on ASUS ROG Ally handhelds.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6a15380488efb..48cc867aeea86 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -205,6 +205,11 @@ 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;
> +	bool vibration_active;

[Severity: Low]
This isn't a bug, but vibration_active is added here and initialized in
ally_config_create(), but never appears to be read or used elsewhere.

[ ... ]

> @@ -724,15 +729,177 @@ static ssize_t xbox_controller_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(xbox_controller);
>  
> +/**
> + * ally_set_vibration_intensity() - Set vibration intensity values
[ ... ]
> + */
> +static int ally_set_vibration_intensity(struct hid_device *hdev, struct ally_config *cfg,
> +					u8 left, u8 right)

[Severity: Low]
This isn't a bug, but the cfg parameter is never utilized inside this
function.

[ ... ]

> +static ssize_t left_vibration_intensity_store(struct device *dev, struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
[ ... ]
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	ret = ally_set_vibration_intensity(hdev, cfg, value, cfg->vibration_intensity_right);
> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		cfg->vibration_intensity_left = value;
> +
> +	return count;
> +}

[Severity: Medium]
Does reading cfg->vibration_intensity_right outside of cfg->config_mutex
create a race condition here?

If concurrent sysfs writes happen for the left and right intensities, could
they read stale complementary values, interleave their hardware I/O
requests, and leave the hardware state permanently desynchronized from the
driver's software cache?

[ ... ]

> +static ssize_t right_vibration_intensity_store(struct device *dev, struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
[ ... ]
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	ret = ally_set_vibration_intensity(hdev, cfg, cfg->vibration_intensity_left, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		cfg->vibration_intensity_right = value;
> +
> +	return count;
> +}

[Severity: Medium]
The same lockless read-modify-write pattern occurs here with
cfg->vibration_intensity_left. Would it be safer to take the lock before
reading the complementary intensity and hold it across the hardware request?

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