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

[Denis Benato <denis.benato@linux.dev>]

On 10/31/26 19:00, sashiko-bot@kernel.org wrote:
> 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?

Not good. And now that I think about it I also forgot the documentation entry.

>> 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?
>