Re: [PATCH v6 09/13] HID: asus: add support for response curve

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Suspend/resume silently drops the active response curve or incorrectly applies an unapplied staged curve.
--

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

    HID: asus: add support for response curve
    
    This commit adds support for configuring the response curve of both
    joysticks on ROG ally devices by exposing relevant sysfs attributes.

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

[ ... ]

> +static ssize_t response_curve_move_store(const char *buf, size_t count,
> +					 struct ally_config *cfg, bool is_left,
> +					 unsigned int idx)
> +{
> +	struct ally_joystick_resp_curve *curve;
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (value > 100)
> +		return -EINVAL;
> +
> +	guard(mutex)(&cfg->config_mutex);
> +
> +	if (!cfg->resp_curve_support)
> +		return -EOPNOTSUPP;
> +
> +	curve = is_left ? &cfg->left_curve : &cfg->right_curve;
> +
> +	switch (idx) {
> +	case 1:
> +		curve->entry_1.move = value;

[Severity: Medium]
Since this directly mutates the active configuration structure (cfg->left_curve
or cfg->right_curve), does this cause issues with suspend and resume if a user
has written uncommitted changes?

If userspace stages partial sysfs modifications to response_curve_move_X without
triggering the apply attribute, and a suspend/resume cycle occurs, could the
resume path be impacted?

Could we store the staged curve separately from the active curve to prevent
uncommitted edits from interfering with the system resume path?

> +		break;
> +	case 2:
> +		curve->entry_2.move = value;

[ ... ]

> @@ -2421,6 +2879,31 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>  					 ret);
>  	}
>  
> +	if (cfg->resp_curve_support) {
> +		/*
> +		 * sysfs stages the entries without validating them: do not
> +		 * push a curve to the MCU until userspace has applied one
> +		 * that is valid.
> +		 */
> +		if (ally_resp_curve_is_valid(&cfg->left_curve)) {
> +			ret = ally_set_joystick_resp_curve(ally, hdev, JOYSTICK_LEFT,
> +							   &cfg->left_curve);

[Severity: Medium]
Will this restore an uncommitted partially edited state if it happens to be valid,
or silently drop the previously valid state if the uncommitted edits made
cfg->left_curve invalid?

> +			if (ret < 0)
> +				hid_warn(hdev,
> +					 "Failed to restore left response curve: %d\n",
> +					 ret);
> +		}
> +
> +		if (ally_resp_curve_is_valid(&cfg->right_curve)) {
> +			ret = ally_set_joystick_resp_curve(ally, hdev, JOYSTICK_RIGHT,
> +							   &cfg->right_curve);
> +			if (ret < 0)
> +				hid_warn(hdev,
> +					 "Failed to restore right response curve: %d\n",
> +					 ret);
> +		}
> +	}
> +
>  	return 0;
>  }

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