Re: [PATCH v12 12/17] HID: asus: add support for btn remapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] HID: asus: ABBA deadlock between ally_prefs.lock and sysfs active refs
- [Low] HID: asus: mixed cleanup helpers and goto in button_remap_store()
- [Low] HID: asus: commit message refers to remap_index instead of remap_available
--

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

HID: asus: add support for btn remapping

This commit adds button and macro remapping support for ASUS ROG Ally
devices via per-button sysfs attributes (remap, macro, and
remap_available), validating mappings against the active gamepad mode and
restoring or refreshing the cached mappings across mode switches and MCU
resets.

>     HID: asus: add support for btn remapping
>     
>     Add support for buttons remapping: the ability to assign different
>     button events to each one of the physical button on the device.
>     
>     Each button exposes a remap_index sysfs attribute listing the valid
>     remap target names for the current gamepad mode: M1 and M2 can be
>     remapped to anything at any time, while other buttons may only be
>     remapped to gamepad codes in gamepad mode and keyboard/mouse/media
>     codes in desktop mode.
>     
>     When the gamepad mode is changed the firmware automatically restores
>     its internal defaults, so the software cache is refreshed to reflect
>     the hardware state.
>     
>     Co-developed-by: Luke Jones <luke@ljones.dev>
>     Signed-off-by: Luke Jones <luke@ljones.dev>
>     Signed-off-by: Denis Benato <denis.benato@linux.dev>
>     Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>

[Severity: Low]
This isn't a bug, but the commit message refers to a remap_index sysfs
attribute, whereas ally_create_button_attributes() in drivers/hid/hid-asus.c
registers this attribute as remap_available alongside the remap and macro
attributes created in button_remap_attr_create(). Should the commit message
be updated to mention remap, macro, and remap_available instead of
remap_index?

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d20e433af5740..b94cb8c50b28b 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;
> +	struct ally_button_mapping *mode_map;
> +	const struct btn_code_map *code;
> +	const struct btn_code_map *old_code;
> +	enum btn_pair_index pair_idx;
> +	struct button_pair_map *pair;
> +	struct button_map *btn_map;
> +	char btn_name[32];
> +	bool is_first;
> +	int ret, i;
> +
> +	if (sscanf(buf, "%31s", btn_name) != 1)
> +		return -EINVAL;
> +
> +	code = find_button_by_name(btn_name);
> +	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;
> +
> +	if (btn_attr->is_macro) {
> +		old_code = btn_map->macro;
> +		btn_map->macro = code;
> +	} else {
> +		old_code = btn_map->remap;
> +		btn_map->remap = 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:
> +	if (btn_attr->is_macro)
> +		btn_map->macro = old_code;
> +	else
> +		btn_map->remap = old_code;
> +
> +	/* Try to recover the mapping presented to the user. */
> +	ally_set_button_mapping(hdev, pair);
> +	goto err_out;
> +
> +err_restore:
> +	if (btn_attr->is_macro)
> +		btn_map->macro = old_code;
> +	else
> +		btn_map->remap = 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() in drivers/hid/hid-asus.c mixes
scope-based cleanup via guard(mutex)(&ally_prefs.lock) with goto-based error
labels (err_restore_hw, err_restore, and err_out), and
ally_create_button_attributes() similarly mixes __free(kfree) on entries
with goto ally_btn_err_cleanup. Could these functions avoid mixing cleanup
helpers and goto error paths in the same routine?

[ ... ]

> @@ -2429,11 +3248,9 @@ 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)
>  {
> +	const size_t entries_attr_index = ally_prefs.turbo_support ? 4 : 0;
>  	int i, ret;
>  
> -	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);
> @@ -2441,13 +3258,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]);
> +		}
> +
> +		entries[i].remap_attr = button_remap_attr_create(i, false);
> +		entries[i].macro_attr = button_remap_attr_create(i, true);
> +		if (!entries[i].remap_attr || !entries[i].macro_attr) {
> +			ret = -ENOMEM;
>  			goto ally_btn_err_cleanup;
>  		}
>  
> -		ally_btn_turbo_init_attrs(&entries[i]);
> +		sysfs_attr_init(&entries[i].attr_remap_available.attr);
> +		entries[i].attr_remap_available.attr.name = "remap_available";
> +		entries[i].attr_remap_available.attr.mode = 0444;
> +		entries[i].attr_remap_available.show = btn_remap_available_show;
> +
> +		entries[i].attrs[entries_attr_index + 0] = &entries[i].remap_attr->dev_attr.attr;
> +		entries[i].attrs[entries_attr_index + 1] = &entries[i].macro_attr->dev_attr.attr;
> +		entries[i].attrs[entries_attr_index + 2] = &entries[i].attr_remap_available.attr;
> +		entries[i].attrs[entries_attr_index + 3] = NULL;
> +
>  		entries[i].group.name = ally_button_names[i];
>  		entries[i].group.attrs = entries[i].attrs;
>  
> @@ -2465,6 +3302,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);
>  }
>  
> @@ -2477,11 +3319,19 @@ static void ally_remove_button_attributes(struct hid_device *hdev)
>  	if (!entries)
>  		return;
>  
> -	for (i = 0; i < ALLY_BTN_MAX; i++)
> +	/*
> +	 * Unpublish first. A mode-store notify holds the prefs lock and must
> +	 * see either the live array or NULL.
> +	 */
> +	ally_prefs.button_entries = NULL;
> +
> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
> +		kfree(entries[i].remap_attr);
> +		kfree(entries[i].macro_attr);
> +	}
>  
>  	kfree(entries);
> -	ally_prefs.button_entries = NULL;
>  }

[Severity: High]
Can this cause a lock ordering inversion and deadlock between
ally_prefs.lock and the sysfs kernfs active references on the button and
gamepad_mode attributes?

When userspace reads or writes the remap, macro, remap_available, or
gamepad_mode sysfs attributes, kernfs acquires an active reference on the
kernfs node before invoking button_remap_show(), button_remap_store(),
btn_remap_available_show(), or gamepad_mode_store(), which then acquire
ally_prefs.lock:

  kernfs_fop_read_iter() / kernfs_fop_write_iter()
    kernfs_get_active()                 /* acquires kn->active */
    button_remap_show() / button_remap_store() /
    btn_remap_available_show() / gamepad_mode_store()
      guard(mutex)(&ally_prefs.lock);   /* acquires ally_prefs.lock */

In the opposite direction, during device removal or error rollback in
ally_prefs_setup(), ally_prefs.lock is held while calling
sysfs_remove_group(), which drains active kernfs references:

  ally_sysfs_remove() / ally_prefs_setup()
    guard(mutex)(&ally_prefs.lock);     /* acquires ally_prefs.lock */
    ally_remove_button_attributes() / ally_create_button_attributes()
      sysfs_remove_group()
        __kernfs_remove()
          kernfs_drain()                /* waits for kn->active */

If a sysfs read or write is waiting to acquire ally_prefs.lock while
ally_sysfs_remove() or ally_prefs_setup() holds ally_prefs.lock and enters
kernfs_drain(), won't both threads block on each other?

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