Re: [PATCH v15 12/18] HID: asus: add support for btn remapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] HID: asus: inconsistent pair update and rollback in
  button_remap_store()
- [Low] HID: asus: unlocked read of turbo_support in
  ally_create_button_attributes()
- [Low] HID: asus: mixed cleanup helpers and goto in
  button_remap_store()
--

commit 6cb0aa69d532f45691d60b1b758a8ac7ec0bc31a
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for btn remapping

This commit adds sysfs attributes and MCU command support for remapping
physical buttons and macro assignments on ROG Ally handhelds.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index b5a1466d327bf..8f9695b54e2a5 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -998,10 +1206,586 @@ static struct ally_prefs {
[ ... ]
> +static ssize_t button_remap_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
> +	enum ally_button_id button_id = btn_attr->button_id;
> +	const struct btn_code_map *old_code, *code;
> +	const struct btn_code_map **target_map;
> +	struct ally_button_mapping *mode_map;
> +	enum btn_pair_index pair_idx;
> +	struct button_pair_map *pair;
> +	struct button_map *btn_map;
> +	bool is_first;
> +	int ret, i;
> +
> +	code = find_button_by_name(buf);
> +	if (!code)
> +		return -EINVAL;
> +
> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	/*
> +	 * Validate the remap target while holding the lock: the
> +	 * gamepad mode can change concurrently, and the check must
> +	 * be done against the very mode the mapping is written to.
> +	 */
> +	if (!ally_remap_code_valid(button_id, ally_prefs.gamepad_mode, code,
> +				   btn_attr->is_macro))
> +		return -EINVAL;
> +
> +	/* Access the mapping for current gamepad mode */
> +	pair = &ally_prefs.button_mappings.button_pairs[pair_idx - 1];
> +	btn_map = is_first ? &pair->first : &pair->second;
> +
> +	target_map = btn_attr->is_macro ? &btn_map->macro : &btn_map->remap;
> +	old_code = *target_map;
> +	*target_map = code;
> +
> +	/* Update pair index */
> +	pair->pair_index = pair_idx;
> +
> +	/*
> +	 * Send mapping to device with the caveat that first
> +	 * generation devices require individual button pair updates.
> +	 */
> +	if (ally_prefs.is_ally_x) {
> +		ret = ally_set_button_mapping(hdev, pair);
> +		if (ret < 0)
> +			goto err_restore;
> +	} else {
> +		mode_map = &ally_prefs.button_mappings;
> +		for (i = 0; i < ARRAY_SIZE(mode_map->button_pairs); i++) {
> +			mode_map->button_pairs[i].pair_index = i + 1;
> +			ret = ally_set_button_mapping(hdev,
> +						      &mode_map->button_pairs[i]);
> +			if (ret < 0) {
> +				/*
> +				 * Pairs are pushed one at a time. This pair
> +				 * already reached the hardware when a later
> +				 * pair is the one that failed.
> +				 */
> +				if (i > pair_idx - 1)
> +					goto err_restore_hw;
> +				goto err_restore;
> +			}
> +		}
> +	}
> +	sysfs_notify(&dev->kobj, ally_button_names[btn_attr->button_id],
> +		     btn_attr->is_macro ? "macro" : "remap");
> +	return count;
> +
> +err_restore_hw:
> +	*target_map = old_code;
> +
> +	/* Try to recover the mapping presented to the user. */
> +	ally_set_button_mapping(hdev, pair);
> +	goto err_out;
> +
> +err_restore:
> +	*target_map = old_code;
> +
> +err_out:
> +	hid_err(hdev, "Failed to set button mapping: %d\n", ret);
> +	return ret;
> +}

[Severity: Medium]
Is the ally_prefs.is_ally_x condition or the err_restore_hw rollback
inverted here?

The comment says first-generation devices require individual button pair
updates, yet the ally_prefs.is_ally_x branch sends a single pair while the
!ally_prefs.is_ally_x branch loops over all 9 pairs.  And if first-gen
devices actually require all 9 pairs to be sent in sequence, err_restore_hw
(which is only reachable when !ally_prefs.is_ally_x) only sends the single
pair instead of all 9, leaving the hardware with the failed mapping.

[Severity: Low]
This isn't a bug, but button_remap_store() mixes guard(mutex) with goto
error labels and holds ally_prefs.lock across sysfs_notify().  Using
scoped_guard(mutex, &ally_prefs.lock) like gamepad_mode_store() does would
avoid mixing cleanup styles and move sysfs_notify() outside the lock.

[ ... ]
> @@ -2420,11 +3220,11 @@ static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
>  
>  static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_device *hdev)
>  {
> +	size_t entries_attr_index;
>  	int i, ret;
>  
>  	scoped_guard(mutex, &ally_prefs.lock)
> -		if (!ally_prefs.turbo_support)
> -			return NULL;
> +		entries_attr_index = ally_prefs.turbo_support ? 4 : 0;
>  
>  	struct ally_btn_sysfs_entry *entries __free(kfree) = kzalloc_objs(*entries, ALLY_BTN_MAX);
>  	if (!entries)
> @@ -2433,13 +3233,33 @@ static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_dev
>  	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;
> +		if (ally_prefs.turbo_support) {
> +			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]);
> +		}

[Severity: Low]
Should this check entries_attr_index (or a local bool snapshotted under
ally_prefs.lock) instead of re-reading ally_prefs.turbo_support without
holding ally_prefs.lock?

[ ... ]
> @@ -2457,6 +3277,11 @@ static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_dev
>  	while (i--)
>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
>  
> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
> +		kfree(entries[i].remap_attr);
> +		kfree(entries[i].macro_attr);
> +	}
> +
>  	return ERR_PTR(ret);
>  }

[Severity: Low]
This isn't a bug, but ally_create_button_attributes() also mixes
__free(kfree) on entries with goto ally_btn_err_cleanup.

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