Re: [PATCH v2 08/12] HID: asus: add support for response curve

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Inconsistent capability check for response curve sysfs read operations.
--

--- Patch [8]: [PATCH v2 08/12] HID: asus: add support for response curve ---
commit 64bba5ccd81bdede00727b014f07c871c53e11d2
Author: Denis Benato <[email protected]>

HID: asus: add support for response curve

This commit adds the ability for userspace to modify 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 050d979f4315d..6f43b7317168f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static ssize_t response_curve_pct_show(struct device *dev,
> +				       char *buf,
> +				       struct ally_config *cfg, bool is_left,
> +				       unsigned int idx)
> +{
> +	struct ally_joystick_resp_curve *curve;
> +	int ret = -EINVAL;
> +
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		curve = is_left ? &cfg->left_curve : &cfg->right_curve;
> +
> +		switch (idx) {

[Severity: Medium]
Should there be a capability check here before returning values?

The corresponding store operations check for !cfg->resp_curve_support and
return -EOPNOTSUPP, but this show function unconditionally returns the locally
initialized defaults (0, 33, 66, 100) even if the device doesn't support
response curves. Can this cause userspace feature-detection logic to
erroneously believe response curves are supported until a write fails?

> +		case 1:
> +			ret = sysfs_emit(buf, "%u\n", curve->entry_1.resp);
> +			break;
> +		case 2:
> +			ret = sysfs_emit(buf, "%u\n", curve->entry_2.resp);
> +			break;

[ ... ]

> +static ssize_t response_curve_move_show(struct device *dev,
> +					char *buf,
> +					struct ally_config *cfg, bool is_left,
> +					unsigned int idx)
> +{
> +	struct ally_joystick_resp_curve *curve;
> +	int ret = -EINVAL;
> +
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		curve = is_left ? &cfg->left_curve : &cfg->right_curve;
> +
> +		switch (idx) {

[Severity: Medium]
Similarly, should this show function also check !cfg->resp_curve_support
before returning values?

> +		case 1:
> +			ret = sysfs_emit(buf, "%u\n", curve->entry_1.move);
> +			break;
> +		case 2:

[ ... ]

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