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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] HID: asus: mixed cleanup helpers and goto in
  button_remap_store()
--

commit 0fe51bd4cd86b318227ccb9c127184422216243c
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for btn remapping

This commit adds sysfs attributes and firmware command support for button
and macro remapping on ASUS ROG Ally handheld devices.

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

[ ... ]

> +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: Low]
This isn't a bug, but button_remap_store() mixes guard(mutex) with
goto-based error handling (err_restore_hw, err_restore, err_out).
Likewise, ally_create_button_attributes() below mixes __free(kfree) on
entries with goto ally_btn_err_cleanup.  The cleanup.h convention is to
avoid mixing scope-based cleanup helpers and goto unwinding within the
same function.

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