Re: [PATCH v5 12/13] HID: asus: add support for turbo buttons

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing cleanup of user calibration sysfs groups (`ally_cal_attr_groups`) in the probe error path of `ally_config_create()`.
--

commit 64475826d55426435b32a4fe5bb92d505b7ed1b7
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for turbo buttons
    
ROG ally devices supports a feature called turbo buttons that allows
the user to quickly toggle button state by keeping a button pressed.
This commit allows userspace to configure the feature by exposing
relevant sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 63efda8938386..7c10e6de438b7 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2697,6 +3126,14 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  		}
>  	}
>  
> +	if (cfg->turbo_support) {
> +		ret = ally_create_button_attributes(hdev, ally, cfg);
> +		if (ret < 0) {
> +			hid_err(hdev, "Failed to create button attributes: %d\n", ret);
> +			goto ally_config_create_sysfs_err;

[Severity: High]
If ally_create_button_attributes() fails here, does this jump leak the
sysfs groups that were previously created for ally_cal_attr_groups?

This newly added code introduces a failure point after ally_cal_attr_groups
has already been successfully created, making the missing cleanup reachable.

> +		}
> +	}
> +
>  	/* So far the only hardware this is supported is the Ally 1 */
>  	if (cfg->xbox_controller_support) {
>  		ret = ally_set_xbox_controller(hdev, ally, cfg, true);
> @@ -2717,6 +3154,9 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  
>  	return cfg;
>  ally_config_create_sysfs_err:
> +	if (cfg->button_entries)
> +		ally_remove_button_attributes(hdev, cfg);
> +

[Severity: High]
Does this error recovery path completely omit the sysfs_remove_group()
cleanup loop for ally_cal_attr_groups?

Leaking these sysfs group directories and attributes can cause subsequent
attempts to probe the device to trigger a kernel warning for duplicate sysfs
nodes (-EEXIST). This would render the device permanently unusable without a
reboot.

>  	/* The sysfs groups are not devm-managed: undo the created ones. */
>  	while (sysfs_i-- > 0)
>  		sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[sysfs_i]);

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