Re: [PATCH v13 11/18] HID: asus: add support for turbo buttons

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] HID: asus: unlocked read of turbo period in ally_btn_period_show()
- [Low] HID: asus: mix of cleanup helpers and goto in ally_create_button_attributes()
- [Low] HID: asus: non-static lookup table on stack in ally_btn_param()
--

commit 3795dba8be2afc39142aa94e1d2e669448280abb
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for turbo buttons

This commit adds support for configuring ROG Ally turbo and toggle button
periods via per-button sysfs attribute groups and restores these settings on
controller initialization.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d3dbeba529c12..70c9d71fc5fa9 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2132,8 +2211,252 @@ static const struct attribute_group ally_attr_groups[] = {
>  	},
>  };
>  
> +static struct ally_btn_turbo_params *ally_btn_param(enum ally_button_id btn)
> +{
> +	struct ally_btn_turbo_params *table[ALLY_BTN_MAX] = {
> +		[ALLY_BTN_A] = &ally_prefs.turbo.btn_a,
> +		[ALLY_BTN_B] = &ally_prefs.turbo.btn_b,
> +		[ALLY_BTN_X] = &ally_prefs.turbo.btn_x,
> +		[ALLY_BTN_Y] = &ally_prefs.turbo.btn_y,
> +		[ALLY_BTN_LB] = &ally_prefs.turbo.btn_lb,
> +		[ALLY_BTN_RB] = &ally_prefs.turbo.btn_rb,
> +		[ALLY_BTN_DU] = &ally_prefs.turbo.btn_du,
> +		[ALLY_BTN_DD] = &ally_prefs.turbo.btn_dd,
> +		[ALLY_BTN_DL] = &ally_prefs.turbo.btn_dl,
> +		[ALLY_BTN_DR] = &ally_prefs.turbo.btn_dr,
> +		[ALLY_BTN_J0B] = &ally_prefs.turbo.btn_j0b,
> +		[ALLY_BTN_J1B] = &ally_prefs.turbo.btn_j1b,
> +		[ALLY_BTN_MENU] = &ally_prefs.turbo.btn_menu,
> +		[ALLY_BTN_VIEW] = &ally_prefs.turbo.btn_view,
> +		[ALLY_BTN_M1] = &ally_prefs.turbo.btn_m1,
> +		[ALLY_BTN_M2] = &ally_prefs.turbo.btn_m2,
> +	};
> +
> +	if (btn >= ALLY_BTN_MAX)
> +		return NULL;
> +
> +	return table[btn];
> +}

[Severity: Low]
As reported by Sashiko on v12 [1], in ally_btn_param() in
drivers/hid/hid-asus.c, table[ALLY_BTN_MAX] maps enum ally_button_id
values to fixed addresses inside the file-scope ally_prefs.turbo struct,
but it is not declared static const. As a result, the 16-pointer lookup
array is reconstructed on the stack on every sysfs show, store, and setup
call.

Could table be declared static struct ally_btn_turbo_params *const to
match the other lookup tables in this driver?

[ ... ]

> +static ssize_t ally_btn_period_show(struct ally_btn_sysfs_entry *entry, char *buf,
> +				    enum ally_btn_period which)
> +{
> +	struct ally_btn_turbo_params *params;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.turbo_support)
> +			return -EOPNOTSUPP;
> +
> +		params = ally_btn_param(entry->btn);
> +		if (!params)
> +			return -ENODEV;
> +	}
> +
> +	return sysfs_emit(buf, "%hhu\n", *ally_btn_period_field(params, which));
> +}
> +
> +static ssize_t ally_btn_period_store(struct device *dev,
> +				     struct ally_btn_sysfs_entry *entry,
> +				     const char *buf, size_t count,
> +				     enum ally_btn_period which, u8 max,
> +				     const char *name)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct ally_btn_turbo_params *params;
> +	u8 old_value, value, *field;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret)
> +		return ret;
> +	if (value > max)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.turbo_support)
> +			return -EOPNOTSUPP;
> +
> +		params = ally_btn_param(entry->btn);
> +		if (!params)
> +			return -ENODEV;
> +
> +		field = ally_btn_period_field(params, which);
> +		old_value = *field;
> +		*field = value;
> +
> +		ret = ally_set_turbo_params(hdev);
> +		if (ret) {
> +			*field = old_value;
> +			return ret;
> +		}
> +	}

[Severity: Medium]
In ally_btn_period_show() in drivers/hid/hid-asus.c, scoped_guard(mutex,
&ally_prefs.lock) only covers checking ally_prefs.turbo_support and
fetching the params pointer into ally_prefs.turbo via ally_btn_param().
The mutex is released when the scoped_guard block exits, before
sysfs_emit() dereferences *ally_btn_period_field(params, which).

Meanwhile, ally_btn_period_store() modifies *field in-place inside
ally_prefs.turbo under ally_prefs.lock before blocking on the feature
report transfer in ally_set_turbo_params(hdev), and rolls *field back to
old_value if that transfer fails.

Can a concurrent read on turbo_period or toggle_period race with
ally_btn_period_store() and observe an uncommitted value that is
subsequently rolled back on error? Should ally_btn_period_show() use
guard(mutex)(&ally_prefs.lock) (or read the byte into a local variable
inside the guarded scope) so the read is serialized by ally_prefs.lock?

[ ... ]

> +static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_device *hdev)
> +{
> +	int i, ret;
> +
> +	scoped_guard(mutex, &ally_prefs.lock)
> +		if (!ally_prefs.turbo_support)
> +			return NULL;
> +
> +	struct ally_btn_sysfs_entry *entries __free(kfree) = kzalloc_objs(*entries, ALLY_BTN_MAX);
> +	if (!entries)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
> +		entries[i].btn = i;
> +
> +		if (!ally_btn_param(i)) {
> +			hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
> +			ret = -EINVAL;
> +			goto ally_btn_err_cleanup;
> +		}
> +
> +		ally_btn_turbo_init_attrs(&entries[i]);
> +		entries[i].group.name = ally_button_names[i];
> +		entries[i].group.attrs = entries[i].attrs;
> +
> +		ret = sysfs_create_group(&hdev->dev.kobj, &entries[i].group);
> +		if (ret < 0) {
> +			hid_err(hdev, "Failed to create sysfs group for %s: %d\n",
> +				ally_button_names[i], ret);
> +			goto ally_btn_err_cleanup;
> +		}
> +	}
> +
> +	return no_free_ptr(entries);
> +
> +ally_btn_err_cleanup:
> +	while (i--)
> +		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
> +
> +	return ERR_PTR(ret);
> +}

[ ... ]

> @@ -2192,7 +2515,15 @@ static int ally_prefs_setup(struct hid_device *hdev, struct ally_intf *intf)
>  		}
>  	}
>  
> +	entries = ally_create_button_attributes(hdev);
> +	if (IS_ERR(entries)) {
> +		ret = PTR_ERR(entries);
> +		hid_err(hdev, "Failed to create button attributes: %d\n", ret);
> +		goto err_cal_groups;
> +	}
> +
>  	scoped_guard(mutex, &ally_prefs.lock) {
> +		intf->button_entries = entries;
>  		intf->cal_sysfs_live = cal_live;
>  		intf->sysfs_live = true;
>  	}

[Severity: Low]
As reported by Sashiko on v12 [2], in ally_create_button_attributes() in
drivers/hid/hid-asus.c, scope-based cleanup helpers (scoped_guard and
entries __free(kfree)) are mixed with goto ally_btn_err_cleanup in the
same function, and ally_prefs_setup() similarly mixes scoped_guard with
goto err_cal_groups.

The kernel cleanup API conventions require not mixing scope-based cleanup
helpers with goto-based error handling within the same function. Could
ally_create_button_attributes() either avoid goto (for example by calling
 a helper to unwind the created groups before returning Directly) or manage
entries with explicit kfree() on the error path?

[1] https://lore.kernel.org/r/20261031174130.208240-12-denis.benato@linux.dev
[2] https://lore.kernel.org/r/20261031174130.208240-12-denis.benato@linux.dev

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