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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Software mapping cache rollback logic introduces state desync when a sequential hardware update fails on first-generation devices.
- [Low] Default button mapping initialization uses highly brittle hardcoded array indices that will silently break if the button code map array changes.
--

--- Patch [12]: [PATCH v2 12/12] HID: asus: add support for btn remapping ---
commit a8628b197357f3dab376cf274ae63c642d78d4ff
Author: Denis Benato <[email protected]>

HID: asus: add support for btn remapping

This commit adds support for button remapping on ROG Ally devices, exposing
sysfs attributes to allow configuring gamepad and desktop mode button codes
for individual physical buttons.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d6ab906b7e782..06dceb77d1086 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -976,6 +984,325 @@ static int ally_set_gamepad_mode(struct ally_handheld *ally, struct hid_device *
>  	return 0;
>  }
>  
> +enum btn_map_type {
> +	BTN_TYPE_NONE = 0,
> +	BTN_TYPE_PAD = 0x01,
> +	BTN_TYPE_KB = 0x02,
> +	BTN_TYPE_MOUSE = 0x03,
> +	BTN_TYPE_MEDIA = 0x05,
> +};
> +
> +struct btn_code_map {
> +	unsigned char type;
> +	unsigned char value;
> +	const char *name;
> +};
> +
> +static const struct btn_code_map ally_btn_codes[] = {
> +	{ BTN_TYPE_NONE, 0x00, "NONE" },
> +	/* Gamepad button codes */
> +	{ BTN_TYPE_PAD, 0x01, "PAD_A" },
[ ... ]
> +	/* Media button codes */
> +	{ BTN_TYPE_MEDIA, 0x16, "MEDIA_SCREENSHOT" },
> +	{ BTN_TYPE_MEDIA, 0x19, "MEDIA_SHOW_KEYBOARD" },
> +	{ BTN_TYPE_MEDIA, 0x1C, "MEDIA_SHOW_DESKTOP" },
> +	{ BTN_TYPE_MEDIA, 0x1E, "MEDIA_START_RECORDING" },
> +	{ BTN_TYPE_MEDIA, 0x01, "MEDIA_MIC_OFF" },
> +	{ BTN_TYPE_MEDIA, 0x02, "MEDIA_VOL_DOWN" },
> +	{ BTN_TYPE_MEDIA, 0x03, "MEDIA_VOL_UP" },
> +};
> +
> +static const size_t keymap_len = ARRAY_SIZE(ally_btn_codes);
[ ... ]
> +static void ally_set_default_keyboard_mapping(struct ally_button_mapping *mappings)
> +{
> +	struct ally_button_mapping *map = &mappings[ALLY_GAMEPAD_MODE_KEYBOARD];
> +	int i;
> +
> +	/* Set all pair indexes and initialize to NONE */
> +	for (i = 0; i < 9; i++) {
> +		map->button_pairs[i].pair_index = i + 1;
> +		map->button_pairs[i].first.remap =
> +			&ally_btn_codes[0];
> +		map->button_pairs[i].first.macro =
> +			&ally_btn_codes[0];
> +		map->button_pairs[i].second.remap =
> +			&ally_btn_codes[0];
> +		map->button_pairs[i].second.macro =
> +			&ally_btn_codes[0];
> +	}
> +
> +	/*
> +	 * Desktop mode defaults: keyboard and mouse mappings.
> +	 * Buttons not listed here (dpad, view, menu) remain NONE.
> +	 */
> +
> +	/* btn_a => KB_SPACE, btn_b => KB_E */

[Severity: Low]
This isn't a bug, but does hardcoding these array indices create a risk of
breaking the mappings if the ally_btn_codes array is modified in the future? 
If new codes are added, removed, or reordered, explicit numbers like 91 and 53
will silently point to the wrong button codes, causing the default mappings
to silently regress.

> +	map->button_pairs[BTN_PAIR_AB - 1].first.remap =
> +		&ally_btn_codes[91]; /* KB_SPACE */
> +	map->button_pairs[BTN_PAIR_AB - 1].second.remap =
> +		&ally_btn_codes[53]; /* KB_E */
> +
> +	/* btn_x => KB_R, btn_y => KB_F */
> +	map->button_pairs[BTN_PAIR_XY - 1].first.remap =
> +		&ally_btn_codes[54]; /* KB_R */
> +	map->button_pairs[BTN_PAIR_XY - 1].second.remap =
> +		&ally_btn_codes[68]; /* KB_F */
[ ... ]
> @@ -2775,6 +3120,515 @@ static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
>  	entry->attrs[4] = NULL;
>  }
[ ... ]
> +static ssize_t button_remap_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
[ ... ]
> +		/*
> +		 * Send mapping to device with the caveat that first
> +		 * generation devices require individual button pair updates.
> +		 */
> +		if (cfg->is_ally_x) {
> +			ret = ally_set_button_mapping(hdev, ally, pair);
> +		} else {
> +			mode_map = &cfg->button_mappings[cfg->gamepad_mode];
> +			ret = 0;
> +			for (i = 0; i < 9; i++) {
> +				mode_map->button_pairs[i].pair_index = i + 1;
> +				ret = ally_set_button_mapping(hdev, ally,
> +					&mode_map->button_pairs[i]);
> +				if (ret < 0)
> +					break;
> +			}
> +		}
> +
> +		/*
> +		 * The hardware rejected the update: restore the previous
> +		 * mapping so that the software cache keeps describing the
> +		 * state the device is actually in.
> +		 */

[Severity: Medium]
Could this rollback logic cause a desynchronization between the software cache
and the hardware state?

When iterating over the 9 button pairs on older devices, if a USB transfer
fails on an iteration after the target pair was already successfully written
to the device (e.g. failure at iteration 5 when the target was pair 2), the
loop breaks and this rollback path triggers.

This unrolls old_code in the software cache, but the hardware has already
accepted and applied the new mapping, causing the sysfs cache to incorrectly
report the previous state.

> +		if (ret < 0) {
> +			if (btn_attr->is_macro)
> +				btn_map->macro = old_code;
> +			else
> +				btn_map->remap = old_code;
> +
> +			hid_err(hdev, "Failed to set button mapping: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return count;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12