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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] HID: asus: mix of cleanup helpers and goto in
  ally_create_button_attributes()
- [Low] HID: asus: per-call stack lookup table in ally_btn_param()
--

commit 41d2b47a8b98d8711a6cf26beaa8f9fb7b5092d2
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for turbo buttons

This commit adds sysfs attributes to configure per-button turbo and toggle
periods on ROG Ally devices and restores those parameters on device
initialization.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index cf33ccefcc2b3..c0c337a5fcba9 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2143,8 +2222,261 @@ 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]
This isn't a bug, but table[] holds fixed pointers into the static
ally_prefs.turbo struct and is rebuilt on the stack on every call.  Could it
be declared static const like the other lookup tables in this file?

[ ... ]

> +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);
> +}

[Severity: Low]
This isn't a bug, but ally_create_button_attributes() mixes __free(kfree)
with goto ally_btn_err_cleanup, contrary to the cleanup.h guideline against
combining scope-based cleanup helpers and goto error handling in the same
function.  Since ally_btn_err_cleanup already unwinds the loop on failure,
entries could be freed explicitly with kfree(entries) there instead of using
__free(kfree) and no_free_ptr().

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