Re: [PATCH v13 16/18] HID: asus: document ROG Ally sysfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] HID: asus: inverted sysfs_streq() check in find_button_by_name()
- [Low] HID: asus: response_curve_index ABI doc mismatches multi-line output
--

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

HID: asus: document ROG Ally sysfs attributes

This commit documents the sysfs attributes exposed by hid-asus for configuring
the ASUS ROG Ally and ROG Ally X gamepad controller, including emulation mode,
vibration, joystick calibration, trigger limits, and button mappings.

> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-asus b/Documentation/ABI/testing/sysfs-driver-hid-asus
> index 30e0d17d4cecc..515eb7c550fc5 100644
> --- a/Documentation/ABI/testing/sysfs-driver-hid-asus
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-asus
> @@ -93,3 +93,392 @@ Description:
>  
>  		Access: Read only
>  		Valid values: a single line of text
> +

[ ... ]

> +What:		/sys/bus/hid/drivers/asus/<dev>/left_joystick_axis/response_curve_index
> +		/sys/bus/hid/drivers/asus/<dev>/right_joystick_axis/response_curve_index
> +Date:		October 2026
> +Contact:	linux-input@vger.kernel.org
> +Description:
> +		Explanation of the format of the sibling response_curve
> +		file.
> +
> +		The files are created on the configuration interface of
> +		a bound ASUS ROG Ally or ROG Ally X controller.
> +
> +		Access: Read only
> +		Valid values: a single line of text

[Severity: Low]
Does response_curve_index actually return a single line of text?

In drivers/hid/hid-asus.c, response_curve_index_show() emits six lines of text
with embedded newlines:

drivers/hid/hid-asus.c:response_curve_index_show() {
	return sysfs_emit(buf,
			  "move_1 resp_1 move_2 resp_2 move_3 resp_3 move_4 resp_4\n"
			  "\n"
			  "move_N: joystick deflection percentage at which curve point N\n"
			  "is placed (0-100, each move_N must be greater than the\n"
			  "previous one). resp_N: response percentage applied from\n"
			  "point N on (0-100).\n");
}

Should this entry describe the multi-line output, or should
response_curve_index_show() emit a single line?

[ ... ]

> +What:		/sys/bus/hid/drivers/asus/<dev>/<btn>/remap
> +Date:		October 2026
> +Contact:	linux-input@vger.kernel.org
> +Description:
> +		Target the button is remapped to, by name. A write
> +		sends the mapping to the controller and is rejected
> +		when the name is unknown or not valid for the button
> +		in the current mode; the recorded mapping is restored
> +		when the controller rejects it.
> +
> +		"NONE" is always valid. In "gamepad" mode only gamepad
> +		codes are valid, in "desktop" mode only keyboard,
> +		mouse and media codes are; btn_m1 and btn_m2 accept
> +		every code.
> +
> +		The file is created in one directory per button of the
> +		configuration interface of a bound ASUS ROG Ally or
> +		ROG Ally X controller. <btn> is one of: btn_a, btn_b,
> +		btn_x, btn_y, btn_lb, btn_rb, dpad_up, dpad_down,
> +		dpad_left, dpad_right, btn_l3, btn_r3, btn_menu,
> +		btn_view, btn_m1 and btn_m2.
> +
> +		Access: Read/Write
> +		Valid values: one name from the sibling
> +		remap_available file

[Severity: High]
Does the driver implementation match this documented behavior for <btn>/remap
and <btn>/macro?

In drivers/hid/hid-asus.c, find_button_by_name() negates sysfs_streq():

drivers/hid/hid-asus.c:find_button_by_name() {
	for (i = 0; i < ARRAY_SIZE(ally_btn_codes); i++) {
		if (!sysfs_streq(ally_btn_codes[i].name, name))
			return &ally_btn_codes[i];
	}

	return NULL;
}

Since sysfs_streq() returns true when the strings match, !sysfs_streq()
returns the first entry in ally_btn_codes[] whose name does not match name.

Because ally_btn_codes[0] is "NONE" and ally_btn_codes[1] is "BTN_SOUTH",
calling find_button_by_name("NONE") returns "BTN_SOUTH", while calling it with
any other string (including unknown names in button_remap_store()) matches
index 0 and returns "NONE" instead of NULL.

Consequently, ally_set_button_pair_mappings_default() initializes every button
and macro slot to "BTN_SOUTH", ally_set_default_gamepad_mapping() and
ally_set_default_keyboard_mapping() set explicitly mapped buttons to "NONE",
and writes to <btn>/remap or <btn>/macro never reject unknown names with
-EINVAL.

Should the negation in find_button_by_name() be removed?

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