[PATCH 00/12] HID: asus: add support for ROG Ally handhelds

[PATCH 00/12] HID: asus: add support for ROG Ally handhelds

[Denis Benato]

Hi all,

This patch series adds support for ROG Ally handhelds in the HID driver for ASUS devices.

The changes include reinitialization of the device after exiting a sleep state,
gamepad configuration, vibration strength configuration, joysticks and triggers
range configuration, anti-deadzone configuration, response curve support,
force feedback support, gamepad mode support, turbo buttons support,
and button remapping support.

This driver is based on the original Luke's work, which was a great starting point
and I have reworked it to be upstreamable, while keeping the original functionality
intact and extending hid-asus to avoid regressions in functionality such as the
backlight control.

The ally support has been a surprisingly long task, spanning multiple years and
being used in Steam OS and well as other distributions for quite a while,
and I am happy to finally be submitting it.

At this time LEDs are not part of the patch series as I have work in progress to
unify LEDs handling across all ASUS devices, and I will submit that in a separate
patch series when the userspace interface will be ready to express capabilities
of these devices, but that will take some more time.

This driver is the culmination of a lot of work, from many different people,
and I want to thank Luke for his original work, as well as the many people that have
helped me test and provide feedback on the driver: Khamunetri, Jonathan, Derek,
Matthew and others that have helped during these years.

Best regards,
Denis Benato

Denis Benato (12):
  HID: asus: reinitialize the device after exiting a sleep state
  HID: asus: add support for ROG Ally handhelds
  HID: asus: add gamepad configuration
  HID: asus: add vibration strength configuration
  HID: asus: add joysticks inner and outer range configuration
  HID: asus: add triggers inner and outer range configuration
  HID: asus: add joysticks anti-deadzone configuration
  HID: asus: add support for response curve
  HID: asus: add support to force feedback
  HID: asus: add support for gamepad mode
  HID: asus: add support for turbo buttons
  HID: asus: add support for btn remapping

 drivers/hid/hid-asus.c | 4171 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 4011 insertions(+), 160 deletions(-)

-- 
2.47.3

[PATCH 01/12] HID: asus: reinitialize the device after exiting a sleep state

[Denis Benato]

The ROG ally needs to have the EC string sent back after resuming from
s2idle since the USB device can be turned completely off by the firmware
when mcu_powersave firmware-attribute is set to 1.

This may also be true for other laptops and certain features might stop
working after the device exit from sleep.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index ec966fc0a411..897b04d06b46 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1360,6 +1360,22 @@ static int asus_start_multitouch(struct hid_device *hdev)
 	return 0;
 }
 
+static int asus_initialize_reports(struct hid_device *hdev)
+{
+	int ret;
+
+	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
+		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
+			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
+			if (ret < 0)
+				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
+					 asus_report_id_init[r], ret);
+		}
+	}
+
+	return 0;
+}
+
 static int __maybe_unused asus_resume(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
@@ -1378,11 +1394,20 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
 static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	int ret;
+
+	ret = asus_initialize_reports(hdev);
+	if (ret) {
+		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
+		goto asus_reset_resume_err;
+	}
 
 	if (drvdata->tp)
 		return asus_start_multitouch(hdev);
 
 	return 0;
+asus_reset_resume_err:
+	return ret;
 }
 
 static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
@@ -1493,13 +1518,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
-		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
-			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
-			if (ret < 0)
-				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
-					 asus_report_id_init[r], ret);
-		}
+	ret = asus_initialize_reports(hdev);
+	if (ret) {
+		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
+		goto err_stop_hw;
 	}
 
 	/* Laptops keyboard backlight is always at 0x5a */
-- 
2.47.3

Re: [PATCH 01/12] HID: asus: reinitialize the device after exiting a sleep state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added error handling blocks in `asus_probe()` and `asus_reset_resume()` are completely dead code because `asus_initialize_reports()` unconditionally returns 0.
--

--- Patch [1]: [PATCH 01/12] HID: asus: reinitialize the device after exiting a sleep state ---
commit 0ca493a924cc77202a10af1860055debe0b725bc
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: reinitialize the device after exiting a sleep state

This patch extracts keyboard initialization into a helper function and
calls it during reset resume, ensuring devices like the ROG ally have
their EC string sent back after s2idle.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1360,6 +1360,22 @@ static int asus_start_multitouch(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static int asus_initialize_reports(struct hid_device *hdev)
> +{
> +	int ret;
> +
> +	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
> +		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
> +			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
> +			if (ret < 0)
> +				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
> +					 asus_report_id_init[r], ret);
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Does this helper function need to return the error code? 

Because it unconditionally returns 0, the error codes captured in the ret
variable from asus_kbd_init() are dropped after the warning is printed.

>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
> +
> +	ret = asus_initialize_reports(hdev);
> +	if (ret) {
> +		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
> +		goto asus_reset_resume_err;
> +	}

[Severity: Low]
Since asus_initialize_reports() unconditionally returns 0, does that make
this error path dead code? 

If the intent was to abort on initialization failure, should the helper return
the error code instead?

[ ... ]

> @@ -1493,13 +1518,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> -	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
> -		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
> -			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
> -			if (ret < 0)
> -				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
> -					 asus_report_id_init[r], ret);
> -		}
> +	ret = asus_initialize_reports(hdev);
> +	if (ret) {
> +		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
> +		goto err_stop_hw;
>  	}

[Severity: Low]
Similar to the check in asus_reset_resume(), is this error handling reachable?

The original code in asus_probe() only printed a warning on failure and
continued, so returning 0 preserves the old behavior, but it leaves this new
goto err_stop_hw path inaccessible.

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

[PATCH 02/12] HID: asus: add support for ROG Ally handhelds

[Denis Benato]

Implement the core functionality for ASUS handhelds:
- extend functionality of hid-asus if driver is enabled
- initialise the device to a "ready" state
- implement Ally X input mapping
- emit a single proper event for AC button long press

Assisted-by: Claude:claude-fable-5
Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
---
 drivers/hid/hid-asus.c | 729 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 725 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 897b04d06b46..f4b2c4a616b7 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -24,11 +24,16 @@
 #include <linux/cleanup.h>
 #include <linux/dmi.h>
 #include <linux/hid.h>
+#include <linux/jiffies.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_data/x86/asus-wmi.h>
+#include <linux/types.h>
 #include <linux/input/mt.h>
 #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
 #include <linux/power_supply.h>
+#include <linux/stddef.h>
 #include <linux/leds.h>
 
 #include "hid-ids.h"
@@ -37,6 +42,9 @@ MODULE_AUTHOR("Yusuke Fujimaki <usk.fujimaki@gmail.com>");
 MODULE_AUTHOR("Brendan McGrath <redmcg@redmandi.dyndns.org>");
 MODULE_AUTHOR("Victor Vlasenko <victor.vlasenko@sysgears.com>");
 MODULE_AUTHOR("Frederik Wenigwieser <frederik.wenigwieser@gmail.com>");
+MODULE_AUTHOR("Denis Benato <denis.benato@linux.dev>");
+MODULE_AUTHOR("Luke Jones <luke@ljones.dev>");
+MODULE_AUTHOR("Khamunetri Clark <khamunetriclark@gmail.com>");
 MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 
 #define T100_TPAD_INTF 2
@@ -55,6 +63,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define ROG_ALLY_X_MIN_MCU 313
 #define ROG_ALLY_MIN_MCU 319
 
+#define HID_ALLY_INTF_KEYBOARD_IN 0x81
+#define HID_ALLY_INTF_CFG_IN 0x83
+#define HID_ALLY_X_INTF_IN 0x87
+
+#define HID_ALLY_GET_REPORT_ID 0x0D
+#define HID_ALLY_SET_REPORT_ID 0x5A
+#define HID_ALLY_FEATURE_CODE_PAGE 0xD1
+
+#define HID_ALLY_X_INPUT_REPORT_SIZE 16
+#define HID_ALLY_X_INPUT_REPORT 0x0B
+
+#define HID_ALLY_READY_MAX_TRIES 6
+
 /* Spurious HID codes sent by QUIRK_ROG_NKEY_KEYBOARD devices */
 #define ASUS_SPURIOUS_CODE_0XEA 0xea
 #define ASUS_SPURIOUS_CODE_0XEC 0xec
@@ -154,6 +175,23 @@ struct asus_touchpad_info {
 	int report_size;
 };
 
+struct ally_handheld {
+	/* All read/write to IN interfaces must lock */
+	struct mutex intf_mutex;
+	struct hid_device *cfg_hdev;
+
+	struct input_dev *ally_x_input;
+	struct hid_device *ally_x_hdev;
+
+	struct hid_device *keyboard_hdev;
+	struct input_dev *keyboard_input;
+
+	u8 cad_sequence_state;
+	unsigned long cad_last_event_time;
+
+	struct delayed_work resume_work;
+};
+
 struct asus_drvdata {
 	unsigned long quirks;
 	struct hid_device *hdev;
@@ -161,6 +199,7 @@ struct asus_drvdata {
 	struct input_dev *tp_kbd_input;
 	struct asus_worker *worker;
 	unsigned int kbd_backlight_brightness;
+	struct ally_handheld *rog_ally;
 	const struct asus_touchpad_info *tp;
 	struct power_supply *battery;
 	struct power_supply_desc battery_desc;
@@ -232,12 +271,655 @@ static const struct asus_touchpad_info medion_e1239t_tp = {
 	.report_size = 32 /* 2 byte header + 5 * 5 + 5 byte footer */,
 };
 
+enum ally_command_codes {
+	CMD_SET_GAMEPAD_MODE            = 0x01,
+	CMD_SET_MAPPING                 = 0x02,
+	CMD_SET_JOYSTICK_MAPPING        = 0x03,
+	CMD_SET_JOYSTICK_DEADZONE       = 0x04,
+	CMD_SET_TRIGGER_RANGE           = 0x05,
+	CMD_SET_VIBRATION_INTENSITY     = 0x06,
+	CMD_LED_CONTROL                 = 0x08,
+	CMD_CHECK_READY                 = 0x0A,
+	CMD_SET_XBOX_CONTROLLER         = 0x0B,
+	CMD_CHECK_XBOX_SUPPORT          = 0x0C,
+	CMD_USER_CAL_DATA               = 0x0D,
+	CMD_CHECK_USER_CAL_SUPPORT      = 0x0E,
+	CMD_SET_TURBO_PARAMS            = 0x0F,
+	CMD_CHECK_TURBO_SUPPORT         = 0x10,
+	CMD_CHECK_RESP_CURVE_SUPPORT    = 0x12,
+	CMD_SET_RESP_CURVE              = 0x13,
+	CMD_CHECK_DIR_TO_BTN_SUPPORT    = 0x14,
+	CMD_SET_GYRO_PARAMS             = 0x15,
+	CMD_CHECK_GYRO_TO_JOYSTICK      = 0x16,
+	CMD_CHECK_ANTI_DEADZONE         = 0x17,
+	CMD_SET_ANTI_DEADZONE           = 0x18,
+};
+
+static const u8 ALLY_FORCE_FEEDBACK_OFF[] = {
+	0x0D, 0x0F, 0x00, 0x00, 0x00, 0x00, 0xFF, 0x00, 0xEB
+};
+
+/*
+ * The ROG Ally device presents multiple USB interfaces (keyboard, mouse, gamepad,
+ * and custom configuration interface) that bind to the same module. Since only
+ * one ROG Ally device can be connected at a time, we use a single global static
+ * ally_handheld structure to share state across these separate HID interfaces.
+ */
+static void ally_resume_work_fn(struct work_struct *work);
+
+/* Changes to ally_drvdata must lock */
+static DEFINE_MUTEX(ally_data_mutex);
+static struct ally_handheld ally_drvdata = {
+	.intf_mutex = __MUTEX_INITIALIZER(ally_drvdata.intf_mutex),
+	/*
+	 * Initialised statically so it is always safe to cancel, whichever
+	 * of the interfaces probed or failed to probe.
+	 */
+	.resume_work = __DELAYED_WORK_INITIALIZER(ally_drvdata.resume_work,
+						  ally_resume_work_fn, 0),
+};
+
 static const u8 asus_report_id_init[] = {
 	FEATURE_KBD_REPORT_ID,
 	FEATURE_KBD_LED_REPORT_ID1,
 	FEATURE_KBD_LED_REPORT_ID2
 };
 
+static inline int ally_dev_set_report(struct hid_device *hdev, const u8 *buf, size_t len)
+{
+	u8 *dmabuf __free(kfree) = kmemdup(buf, len, GFP_KERNEL);
+	if (!dmabuf)
+		return -ENOMEM;
+
+	return hid_hw_raw_request(hdev, buf[0], dmabuf, len,
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+}
+
+static inline int ally_dev_get_report(struct hid_device *hdev, u8 *out, size_t len)
+{
+	return hid_hw_raw_request(hdev, HID_ALLY_GET_REPORT_ID, out, len,
+		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+}
+
+static void ally_resume_work_fn(struct work_struct *work)
+{
+	struct ally_handheld *ally = container_of(work, struct ally_handheld,
+						  resume_work.work);
+	struct input_dev *keyboard_input, *x_input;
+
+	/*
+	 * Test the very pointers that get dereferenced: probe sets
+	 * keyboard_hdev even when the interface exposes no input_dev, and
+	 * removal clears the two fields one after the other.
+	 */
+	keyboard_input = READ_ONCE(ally->keyboard_input);
+	x_input = READ_ONCE(ally->ally_x_input);
+
+	/* Force release all vendor buttons to prevent "stuck" ghosting on resume
+	 * (workaround for Ally X USB re-probing during suspend/resume)
+	 */
+	if (keyboard_input) {
+		input_report_key(keyboard_input, KEY_F16, 0);
+		input_report_key(keyboard_input, KEY_F17, 0);
+		input_report_key(keyboard_input, KEY_F18, 0);
+		input_report_key(keyboard_input, KEY_PROG1, 0);
+		input_sync(keyboard_input);
+	}
+
+	if (x_input) {
+		input_report_key(x_input, KEY_F16, 0);
+		input_report_key(x_input, KEY_F17, 0);
+		input_report_key(x_input, KEY_F18, 0);
+		input_report_key(x_input, KEY_PROG1, 0);
+		input_sync(x_input);
+	}
+}
+
+/**
+ * handle_ctrl_alt_del() - detect a left button long press.
+ * @hdev: HID device the report arrived on
+ * @ally: ally handheld structure holding the sequence state
+ * @data: raw report buffer, rewritten in place when the sequence matches
+ * @size: length of @data in bytes
+ *
+ * The Ally left button emits a sequence of ctrl+alt+del events. Capture that
+ * and emit only a single code for that single event.
+ *
+ * Return: true iff the event has been managed
+ */
+static bool handle_ctrl_alt_del(struct hid_device *hdev,
+				struct ally_handheld *ally, u8 *data, int size)
+{
+	bool time_is_past = time_after(jiffies, ally->cad_last_event_time + msecs_to_jiffies(100));
+
+	if (size < 16 || data[0] != 0x01)
+		return false;
+
+	if (ally->cad_sequence_state > 0 && time_is_past)
+		ally->cad_sequence_state = 0;
+
+	ally->cad_last_event_time = jiffies;
+
+	switch (ally->cad_sequence_state) {
+	case 0:
+		if (data[1] == 0x01 && data[2] == 0x00 && data[3] == 0x00) {
+			ally->cad_sequence_state = 1;
+			data[1] = 0x00;
+			return true;
+		}
+		break;
+	case 1:
+		if (data[1] == 0x05 && data[2] == 0x00 && data[3] == 0x00) {
+			ally->cad_sequence_state = 2;
+			data[1] = 0x00;
+			return true;
+		}
+		break;
+	case 2:
+		if (data[1] == 0x05 && data[2] == 0x00 && data[3] == 0x4c) {
+			ally->cad_sequence_state = 3;
+			data[1] = 0x00;
+			data[3] = 0x6F; // F20;
+			return true;
+		}
+		break;
+	case 3:
+		if (data[1] == 0x04 && data[2] == 0x00 && data[3] == 0x4c) {
+			ally->cad_sequence_state = 4;
+			data[1] = data[3] = 0x00;
+			return true;
+		}
+		break;
+	case 4:
+		if (data[1] == 0x00 && data[2] == 0x00 && data[3] == 0x4c) {
+			ally->cad_sequence_state = 5;
+			data[3] = 0x00;
+			return true;
+		}
+		break;
+	}
+	ally->cad_sequence_state = 0;
+	return false;
+}
+
+static bool handle_ally_event(struct hid_device *hdev, struct ally_handheld *ally,
+			      u8 *data, int size)
+{
+	struct input_dev *keyboard_input;
+	int keycode = 0;
+
+	if (size < 2)
+		return false;
+
+	if (data[0] == 0x5A) {
+		switch (data[1]) {
+		case 0x38:
+			keycode = KEY_F19;
+			break;
+		case 0xA6:
+			keycode = KEY_F16;
+			break;
+		case 0xA7:
+			keycode = KEY_F17;
+			break;
+		case 0xA8:
+			keycode = KEY_F18;
+			break;
+		default:
+			return false;
+		}
+
+		scoped_guard(mutex, &ally_data_mutex) {
+			keyboard_input = ally->keyboard_input;
+			if (keyboard_input) {
+				input_report_key(keyboard_input, keycode, 1);
+				input_sync(keyboard_input);
+				input_report_key(keyboard_input, keycode, 0);
+				input_sync(keyboard_input);
+				return true;
+			}
+		}
+	}
+	return false;
+}
+
+/**
+ * ally_gamepad_send_packet() - Send a raw packet to the gamepad device.
+ *
+ * @ally: ally handheld structure
+ * @hdev: hid device
+ * @buf: Buffer containing the packet data
+ * @len: Length of data to send
+ *
+ * Return: count of data transferred, negative if error
+ */
+static int ally_gamepad_send_packet(struct ally_handheld *ally,
+			     struct hid_device *hdev, const u8 *buf, size_t len)
+{
+	scoped_guard(mutex, &ally->intf_mutex)
+		return ally_dev_set_report(hdev, buf, len);
+}
+
+/**
+ * ally_gamepad_send_receive_packet() - Send a packet and receive the response.
+ * @ally: ally handheld structure
+ * @hdev: hid device
+ * @buf: Buffer containing the packet data to send and receive response in
+ * @len: Length of buffer
+ *
+ * Return: count of data transferred, negative if error
+ */
+static int ally_gamepad_send_receive_packet(struct ally_handheld *ally,
+					    struct hid_device *hdev,
+					    u8 *buf, size_t len)
+{
+	int ret;
+
+	scoped_guard(mutex, &ally->intf_mutex) {
+		ret = ally_dev_set_report(hdev, buf, len);
+		if (ret >= 0) {
+			memset(buf, 0, len);
+			ret = ally_dev_get_report(hdev, buf, len);
+		}
+	}
+
+	return ret;
+}
+
+/**
+ * ally_alloc_cmd() - Construct a command buffer for the gamepad
+ * @cmd: Command code to send
+ * @payload: Optional payload data to include in the command
+ * @payload_size: Size of the payload data
+ *
+ * The constructed buffer is 64 bytes long, and it is the caller
+ * responsibility to free the buffer using kfree().
+ *
+ * Returns the pointer of newly allocated buffer containing the command,
+ * or NULL on allocation failure.
+ */
+static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
+{
+	u8 *hidbuf = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
+
+	if (!hidbuf)
+		return NULL;
+
+	hidbuf[0] = HID_ALLY_SET_REPORT_ID;
+	hidbuf[1] = HID_ALLY_FEATURE_CODE_PAGE;
+	hidbuf[2] = cmd;
+	hidbuf[3] = payload_size;
+
+	if (payload_size > 0 && payload)
+		memcpy(&hidbuf[4], payload, payload_size);
+
+	return hidbuf;
+}
+
+/*
+ * This should be called before any remapping attempts,
+ * and on driver init/resume, after the asus handshake
+ * has been performed on the configuration endpoint.
+ */
+static int ally_gamepad_check_ready(struct ally_handheld *ally, struct hid_device *hdev)
+{
+	u8 payload[] = { 0x00 };
+	int ret;
+
+	for (int i = 0; i < HID_ALLY_READY_MAX_TRIES; i++) {
+		u8 *buf __free(kfree) = ally_alloc_cmd(CMD_CHECK_READY, payload, sizeof(payload));
+		if (!buf)
+			return -ENOMEM;
+
+		ret = ally_gamepad_send_receive_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
+		if (ret < 0) {
+			hid_dbg(hdev, "ROG Ally check %d/%d failed: %d\n", i,
+				HID_ALLY_READY_MAX_TRIES, ret);
+			continue;
+		}
+
+		if (buf[2] == CMD_CHECK_READY)
+			return 0;
+
+		usleep_range(1000, 2000);
+	}
+
+	hid_err(hdev, "ROG Ally never responded with a ready\n");
+	return -ENODEV;
+}
+
+static int ally_get_endpoint_address(struct hid_device *hdev)
+{
+	struct usb_host_endpoint *ep;
+	struct usb_interface *intf;
+
+	if (!hid_is_usb(hdev))
+		return -ENODEV;
+
+	intf = to_usb_interface(hdev->dev.parent);
+	if (!intf || !intf->cur_altsetting)
+		return -ENODEV;
+
+	ep = intf->cur_altsetting->endpoint;
+	if (!ep)
+		return -ENODEV;
+
+	return ep->desc.bEndpointAddress;
+}
+
+struct ally_x_input_report {
+	uint16_t x, y;
+	uint16_t rx, ry;
+	uint16_t z, rz;
+	uint8_t buttons[3];
+} __packed;
+
+/* The hatswitch outputs integers, we use them to index this X|Y pair */
+static const int hat_values[][2] = {
+	{ 0, 0 }, { 0, -1 }, { 1, -1 }, { 1, 0 },   { 1, 1 },
+	{ 0, 1 }, { -1, 1 }, { -1, 0 }, { -1, -1 },
+};
+
+/* Return true if event was handled, otherwise false */
+static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev,
+			    struct hid_report *report, u8 *data, int size)
+{
+	struct ally_x_input_report *in_report;
+	u8 byte;
+
+	if (!input)
+		return false;
+
+	if (size < 1)
+		return false;
+
+	if (data[0] == 0x5A) {
+		input_report_key(input, KEY_PROG1, data[1] == 0x38);
+		input_report_key(input, KEY_F16, data[1] == 0xA6);
+		input_report_key(input, KEY_F17, data[1] == 0xA7);
+		input_report_key(input, KEY_F18, data[1] == 0xA8);
+		input_sync(input);
+
+		return data[1] == 0xA6 || data[1] == 0xA7 || data[1] == 0xA8 || data[1] == 0x38;
+	}
+
+	if (data[0] != HID_ALLY_X_INPUT_REPORT)
+		return false;
+
+	/*
+	 * hid-core only guarantees size >= 1 and does not zero-pad short
+	 * reports before ->raw_event, so a truncated transfer would leave the
+	 * payload below pointing at stale DMA buffer contents.
+	 */
+	if (size < 1 + sizeof(*in_report))
+		return false;
+
+	in_report = (struct ally_x_input_report *)&data[1];
+
+	input_report_abs(input, ABS_X, in_report->x - 32768);
+	input_report_abs(input, ABS_Y, in_report->y - 32768);
+	input_report_abs(input, ABS_RX, in_report->rx - 32768);
+	input_report_abs(input, ABS_RY, in_report->ry - 32768);
+	input_report_abs(input, ABS_Z, in_report->z);
+	input_report_abs(input, ABS_RZ, in_report->rz);
+
+	byte = in_report->buttons[0];
+	input_report_key(input, BTN_A, byte & BIT(0));
+	input_report_key(input, BTN_B, byte & BIT(1));
+	input_report_key(input, BTN_X, byte & BIT(2));
+	input_report_key(input, BTN_Y, byte & BIT(3));
+	input_report_key(input, BTN_TL, byte & BIT(4));
+	input_report_key(input, BTN_TR, byte & BIT(5));
+	input_report_key(input, BTN_SELECT, byte & BIT(6));
+	input_report_key(input, BTN_START, byte & BIT(7));
+
+	byte = in_report->buttons[1];
+	input_report_key(input, BTN_THUMBL, byte & BIT(0));
+	input_report_key(input, BTN_THUMBR, byte & BIT(1));
+	input_report_key(input, BTN_MODE, byte & BIT(2));
+
+	/* The hatswitch byte is device-controlled; treat anything the table
+	 * does not cover as centred rather than indexing out of bounds.
+	 */
+	byte = in_report->buttons[2];
+	if (byte >= ARRAY_SIZE(hat_values))
+		byte = 0;
+	input_report_abs(input, ABS_HAT0X, hat_values[byte][0]);
+	input_report_abs(input, ABS_HAT0Y, hat_values[byte][1]);
+
+	input_sync(input);
+
+	return true;
+}
+
+static struct input_dev *ally_x_alloc_input_dev(struct hid_device *hdev)
+{
+	struct input_dev *input_dev = devm_input_allocate_device(&hdev->dev);
+
+	if (!input_dev)
+		return ERR_PTR(-ENOMEM);
+
+	input_dev->id.bustype = hdev->bus;
+	input_dev->id.vendor = hdev->vendor;
+	input_dev->id.product = hdev->product;
+	input_dev->id.version = hdev->version;
+	input_dev->uniq = hdev->uniq;
+	input_dev->name = "ASUS ROG Ally X Gamepad";
+
+	input_set_drvdata(input_dev, hdev);
+
+	return input_dev;
+}
+
+static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *ally)
+{
+	struct input_dev *input = ally_x_alloc_input_dev(hdev);
+	int ret;
+
+	if (IS_ERR(input))
+		return PTR_ERR(input);
+
+	input_set_abs_params(input, ABS_X, -32768, 32767, 0, 0);
+	input_set_abs_params(input, ABS_Y, -32768, 32767, 0, 0);
+	input_set_abs_params(input, ABS_RX, -32768, 32767, 0, 0);
+	input_set_abs_params(input, ABS_RY, -32768, 32767, 0, 0);
+	input_set_abs_params(input, ABS_Z, 0, 1023, 0, 0);
+	input_set_abs_params(input, ABS_RZ, 0, 1023, 0, 0);
+	input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0);
+	input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0);
+	input_set_capability(input, EV_KEY, BTN_A);
+	input_set_capability(input, EV_KEY, BTN_B);
+	input_set_capability(input, EV_KEY, BTN_X);
+	input_set_capability(input, EV_KEY, BTN_Y);
+	input_set_capability(input, EV_KEY, BTN_TL);
+	input_set_capability(input, EV_KEY, BTN_TR);
+	input_set_capability(input, EV_KEY, BTN_SELECT);
+	input_set_capability(input, EV_KEY, BTN_START);
+	input_set_capability(input, EV_KEY, BTN_MODE);
+	input_set_capability(input, EV_KEY, BTN_THUMBL);
+	input_set_capability(input, EV_KEY, BTN_THUMBR);
+
+	input_set_capability(input, EV_KEY, KEY_PROG1);
+	input_set_capability(input, EV_KEY, KEY_F16);
+	input_set_capability(input, EV_KEY, KEY_F17);
+	input_set_capability(input, EV_KEY, KEY_F18);
+	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY);
+	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);
+
+	ret = input_register_device(input);
+	if (ret) {
+		hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);
+		goto ally_x_setup_input_err;
+	}
+
+	ally->ally_x_input = input;
+
+	return 0;
+ally_x_setup_input_err:
+	return ret;
+}
+
+static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *ally)
+{
+	int ret;
+
+	/*
+	 * This function assumes the asus-specific initialization
+	 * to have been performed already at this point.
+	 */
+	ret = ally_gamepad_check_ready(ally, hdev);
+	if (ret < 0) {
+		hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
+		return ret;
+	}
+
+	/* Failure at this point is non-critical */
+	ret = ally_gamepad_send_packet(ally, hdev, ALLY_FORCE_FEEDBACK_OFF,
+				       sizeof(ALLY_FORCE_FEEDBACK_OFF));
+	if (ret < 0)
+		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
+
+	return 0;
+}
+
+static bool hid_asus_ally_raw_event(struct hid_device *hdev, struct ally_handheld *ally,
+			struct hid_report *report, u8 *data, int size)
+{
+	struct input_dev *x_input;
+	struct hid_device *x_hdev;
+
+	if (!ally)
+		return false;
+
+	switch (ally_get_endpoint_address(hdev)) {
+	case HID_ALLY_X_INTF_IN:
+		scoped_guard(mutex, &ally_data_mutex) {
+			x_input = ally->ally_x_input;
+			x_hdev = ally->ally_x_hdev;
+		}
+		if (ally_x_raw_event(x_input, x_hdev, report, data, size))
+			return true;
+		break;
+	case HID_ALLY_INTF_CFG_IN:
+		if (handle_ally_event(hdev, ally, data, size))
+			return true;
+		break;
+	case HID_ALLY_INTF_KEYBOARD_IN:
+		if (handle_ctrl_alt_del(hdev, ally, data, size))
+			return false;
+		break;
+	default:
+		break;
+	}
+
+	return false;
+}
+
+/*
+ * Initialize ROG Ally HID extension: this module works alongside
+ * the main Asus HID driver to handle Ally-specific features
+ * and quirks.
+ *
+ * returns:
+ * Either an ally_handheld struct pointer on success, or an ERR_PTR on failure.
+ * The caller is not expected to use the returned pointer, but it should
+ * check for errors by using IS_ERR and PTR_ERR and pass to other functions
+ * NULL if there was an error.
+ */
+static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
+{
+	int ret = 0, ep = ally_get_endpoint_address(hdev);
+	struct hid_input *hidinput;
+
+	if (ep < 0)
+		return ERR_PTR(ep);
+
+	scoped_guard(mutex, &ally_data_mutex)
+		switch (ep) {
+		case HID_ALLY_INTF_CFG_IN:
+			ally_drvdata.cfg_hdev = hdev;
+			ret = hid_asus_ally_init(hdev, &ally_drvdata);
+			if (ret < 0) {
+				ally_drvdata.cfg_hdev = NULL;
+				return ERR_PTR(ret);
+			}
+
+			break;
+		case HID_ALLY_X_INTF_IN:
+			ally_drvdata.ally_x_hdev = hdev;
+			/* This will create and populate ally_x_input */
+			ret = ally_x_setup_input(hdev, &ally_drvdata);
+			if (ret) {
+				hid_err(hdev, "Failed to create Ally X gamepad device.\n");
+				ally_drvdata.ally_x_hdev = NULL;
+				return ERR_PTR(ret);
+			}
+			break;
+		case HID_ALLY_INTF_KEYBOARD_IN:
+			ally_drvdata.keyboard_hdev = hdev;
+			if (!list_empty(&hdev->inputs)) {
+				hidinput = list_first_entry(&hdev->inputs, struct hid_input, list);
+				ally_drvdata.keyboard_input = hidinput->input;
+			}
+			break;
+		default:
+			/* This is normally supposed to happen */
+			break;
+		}
+
+	return &ally_drvdata;
+}
+
+static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *ally)
+{
+	if (!ally)
+		return;
+
+	/*
+	 * Any of the three interfaces can own an input_dev the resume work
+	 * reports through, and they are torn down in an arbitrary order, so
+	 * drain it before clearing anything. Cancel outside ally_data_mutex so
+	 * a handler that wants the mutex cannot deadlock against us.
+	 */
+	cancel_delayed_work_sync(&ally->resume_work);
+
+	scoped_guard(mutex, &ally_data_mutex) {
+		if (ally->ally_x_hdev == hdev) {
+			ally->ally_x_input = NULL;
+			ally->ally_x_hdev = NULL;
+		}
+
+		/*
+		 * The keyboard interface is torn down before the config one, and
+		 * its input_dev is freed with it. handle_ally_event() and
+		 * ally_resume_work_fn() both report keys through it from the
+		 * config endpoint, so drop the references here or they dangle.
+		 */
+		if (ally->keyboard_hdev == hdev) {
+			ally->keyboard_input = NULL;
+			ally->keyboard_hdev = NULL;
+		}
+	}
+}
+
+static int hid_asus_ally_reset_resume(struct hid_device *hdev, struct ally_handheld *ally)
+{
+	int ep = ally_get_endpoint_address(hdev);
+	int ret;
+
+	if (!ally)
+		return -EINVAL;
+
+	if (ep != HID_ALLY_INTF_CFG_IN)
+		return 0;
+
+	ret = hid_asus_ally_init(hdev, ally);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 /*
  * Send events to asus-wmi driver for handling special keys
  */
@@ -505,6 +1187,17 @@ static int asus_raw_event(struct hid_device *hdev,
 	if (drvdata->quirks & QUIRK_MEDION_E1239T)
 		return asus_e1239t_event(drvdata, data, size);
 
+	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+		/*
+		 * Return -1 to suppress further processing by the generic HID
+		 * input parser for reports we fully handle for the Gamepad (0x0B).
+		 * If we let 0x0B fallthrough then the default parser creates a
+		 * generic gamepad causing Steam Input overlaps (i.e. L1 stuck on screenshot).
+		 */
+		if (hid_asus_ally_raw_event(hdev, drvdata->rog_ally, report, data, size))
+			return -1;
+	}
+
 	/*
 	 * Skip these report ID, the device emits a continuous stream associated
 	 * with the AURA mode it is in which looks like an 'echo'.
@@ -1254,6 +1947,7 @@ static int asus_input_mapping(struct hid_device *hdev,
 		case 0x5f: asus_map_key_clear(KEY_PROG2);	break; /* S-shaped programmable key */
 		case 0x6b: asus_map_key_clear(KEY_F21);		break; /* ASUS touchpad toggle */
 		case 0x38: asus_map_key_clear(KEY_PROG1);	break; /* ROG key */
+		case 0x93: asus_map_key_clear(KEY_PROG1);	break; /* ROG Ally X AC button */
 		case 0xba: asus_map_key_clear(KEY_PROG2);	break; /* Fn+C ASUS Splendid */
 		case 0x5c: asus_map_key_clear(KEY_PROG3);	break; /* Fn+Space Power4Gear */
 		case 0x99: asus_map_key_clear(KEY_PROG4);	break; /* Fn+F5 "fan" symbol */
@@ -1266,7 +1960,7 @@ static int asus_input_mapping(struct hid_device *hdev,
 		case 0xa5: asus_map_key_clear(KEY_F15);		break; /* ROG Ally left back */
 		case 0xa6: asus_map_key_clear(KEY_F16);		break; /* ROG Ally QAM button */
 		case 0xa7: asus_map_key_clear(KEY_F17);		break; /* ROG Ally ROG long-press */
-		case 0xa8: asus_map_key_clear(KEY_F18);		break; /* ROG Ally ROG long-press-release */
+		case 0xa8: asus_map_key_clear(KEY_F18);		break;
 
 		default:
 			/* ASUS lazily declares 256 usages, ignore the rest,
@@ -1379,6 +2073,8 @@ static int asus_initialize_reports(struct hid_device *hdev)
 static int __maybe_unused asus_resume(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	int ep;
 
 	/*
 	 * If we have a backlight listener registered, restore the previous state,
@@ -1388,6 +2084,12 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
 	if (drvdata->listener.brightness_set)
 		asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
 
+	if (ally && (drvdata->quirks & QUIRK_ROG_ALLY_XPAD)) {
+		ep = ally_get_endpoint_address(hdev);
+		if (ep == HID_ALLY_INTF_CFG_IN)
+			schedule_delayed_work(&ally->resume_work, msecs_to_jiffies(500));
+	}
+
 	return 0;
 }
 
@@ -1399,21 +2101,28 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
 	ret = asus_initialize_reports(hdev);
 	if (ret) {
 		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
-		goto asus_reset_resume_err;
+		return ret;
 	}
 
 	if (drvdata->tp)
 		return asus_start_multitouch(hdev);
 
+	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+		ret = hid_asus_ally_reset_resume(hdev, drvdata->rog_ally);
+		if (ret) {
+			hid_err(hdev, "Failed to resume ROG Ally HID extensions: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
-asus_reset_resume_err:
-	return ret;
 }
 
 static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct hid_report_enum *rep_enum;
 	struct asus_drvdata *drvdata;
+	struct ally_handheld *ally;
 	struct hid_report *rep;
 	bool is_vendor = false;
 	int ret;
@@ -1530,6 +2239,15 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		(asus_kbd_register_leds(hdev)))
 		hid_warn(hdev, "Failed to initialize backlight.\n");
 
+	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+		ally = hid_asus_ally_probe(hdev);
+		if (IS_ERR(ally))
+			hid_err(hdev, "Failed to initialize ROG Ally HID extensions: %ld\n",
+				PTR_ERR(ally));
+		else
+			drvdata->rog_ally = ally;
+	}
+
 	/*
 	 * For ROG keyboards, skip rename for consistency and ->input check as
 	 * some devices do not have inputs.
@@ -1573,6 +2291,9 @@ static void asus_remove(struct hid_device *hdev)
 	if (drvdata->listener.brightness_set)
 		asus_hid_unregister_listener(&drvdata->listener);
 
+	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD)
+		hid_asus_ally_remove(hdev, drvdata->rog_ally);
+
 	asus_worker_stop(drvdata->worker);
 	hid_hw_stop(hdev);
 }
-- 
2.47.3

Re: [PATCH 02/12] HID: asus: add support for ROG Ally handhelds

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Acquiring a sleeping lock (mutex) inside an atomic context (URB completion handler), causing a 'BUG: scheduling while atomic' kernel panic.
- [High] Missing rollback of global state pointers on probe failure leaves dangling pointers in a static global struct.
- [Medium] Hardware packet structures are defined using native types (`uint16_t`), breaking portability on Big Endian architectures.
- [Medium] Device fails to resume if the ROG Ally extensions failed to probe.
- [Medium] Missing bounds check before accessing `data[1]` in `ally_x_raw_event` allows reading stale buffer data.
--

--- Patch [2]: [PATCH 02/12] HID: asus: add support for ROG Ally handhelds ---
commit 5a6720dc4f3259dca06d1dbb804d6380655462ce
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for ROG Ally handhelds

This commit implements the core functionality for ASUS handhelds, extending
the hid-asus driver to initialize the device, implement Ally X input mapping,
and handle specific AC button events.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 897b04d06b464..f4b2c4a616b75 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -232,12 +271,655 @@ static const struct asus_touchpad_info medion_e1239t_tp = {
[ ... ]
> +struct ally_x_input_report {
> +	uint16_t x, y;
> +	uint16_t rx, ry;
> +	uint16_t z, rz;
> +	uint8_t buttons[3];
> +} __packed;

[Severity: Medium]
Are these values interpreted correctly on big-endian architectures?

Since USB HID payloads natively use little-endian byte ordering, defining
these as native uint16_t and using them without le16_to_cpu() conversion
can corrupt the analog stick values on big-endian systems.

[ ... ]
> +static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev,
> +			    struct hid_report *report, u8 *data, int size)
> +{
> +	struct ally_x_input_report *in_report;
> +	u8 byte;
> +
> +	if (!input)
> +		return false;
> +
> +	if (size < 1)
> +		return false;
> +
> +	if (data[0] == 0x5A) {
> +		input_report_key(input, KEY_PROG1, data[1] == 0x38);

[Severity: Medium]
Is it possible for data[1] to read stale memory here?

Since the size check above only ensures the size is at least 1, if the device
sends a 1-byte report starting with 0x5A, accessing data[1] will read
out-of-bounds from the previous transfer buffer contents.

[ ... ]
> +static bool hid_asus_ally_raw_event(struct hid_device *hdev, struct ally_handheld *ally,
> +			struct hid_report *report, u8 *data, int size)
> +{
> +	struct input_dev *x_input;
> +	struct hid_device *x_hdev;
> +
> +	if (!ally)
> +		return false;
> +
> +	switch (ally_get_endpoint_address(hdev)) {
> +	case HID_ALLY_X_INTF_IN:
> +		scoped_guard(mutex, &ally_data_mutex) {
> +			x_input = ally->ally_x_input;
> +			x_hdev = ally->ally_x_hdev;
> +		}

[Severity: Critical]
Can this cause a kernel panic due to scheduling while atomic?

The hid_asus_ally_raw_event() function is invoked from the driver's
raw_event callback (asus_raw_event). For USB HID devices, the core HID
subsystem invokes raw_event directly from the URB completion handler which
executes in atomic interrupt context.

Acquiring a sleeping lock like a mutex in atomic context violates kernel
scheduling rules and will trigger an immediate panic.

[ ... ]
> @@ -1399,21 +2101,28 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
[ ... ]
> +	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +		ret = hid_asus_ally_reset_resume(hdev, drvdata->rog_ally);
> +		if (ret) {
> +			hid_err(hdev, "Failed to resume ROG Ally HID extensions: %d\n", ret);
> +			return ret;
> +		}
> +	}

[Severity: Medium]
Could this break system resume if the optional extension failed to probe?

If hid_asus_ally_probe() fails during device probe, drvdata->rog_ally is left
as NULL, but the device continues to operate as a generic HID device.

During system resume, hid_asus_ally_reset_resume() will return -EINVAL when
ally is NULL, which gets propagated here and causes the entire device resume
to fail.

[ ... ]
> @@ -1530,6 +2239,15 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
[ ... ]
> +	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +		ally = hid_asus_ally_probe(hdev);
> +		if (IS_ERR(ally))
> +			hid_err(hdev, "Failed to initialize ROG Ally HID extensions: %ld\n",
> +				PTR_ERR(ally));
> +		else
> +			drvdata->rog_ally = ally;
> +	}

[Severity: High]
Might this lead to a use-after-free if probe fails later in this function?

The hid_asus_ally_probe() function populates the statically-allocated global
ally_drvdata struct with pointers to the current device's hdev and
devres-managed input_dev.

If a subsequent step in asus_probe() fails (such as
asus_initialize_reports()), the function jumps to the error path without
calling hid_asus_ally_remove(). The devres cleanup will free the input_dev,
but the global ally_drvdata will retain the dangling pointer. If another
interface successfully probes and triggers ally_resume_work_fn(), it will
dereference the freed pointer.

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

[PATCH 03/12] HID: asus: add gamepad configuration

[Denis Benato]

Add the base configuration structures for the gamepad configuration,
detect capabilities and initialize the device in a known state.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
---
 drivers/hid/hid-asus.c | 299 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 289 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index f4b2c4a616b7..6a15380488ef 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -22,6 +22,7 @@
 
 #include <linux/acpi.h>
 #include <linux/cleanup.h>
+#include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/hid.h>
 #include <linux/jiffies.h>
@@ -34,6 +35,7 @@
 #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
 #include <linux/power_supply.h>
 #include <linux/stddef.h>
+#include <linux/sysfs.h>
 #include <linux/leds.h>
 
 #include "hid-ids.h"
@@ -175,6 +177,36 @@ struct asus_touchpad_info {
 	int report_size;
 };
 
+struct ally_config {
+	/* Must be locked if the data is being changed */
+	struct mutex config_mutex;
+	bool initialized;
+
+	/* Device capabilities flags */
+	bool is_ally_x;
+	bool xbox_controller_support;
+	bool user_cal_support;
+	bool turbo_support;
+	bool resp_curve_support;
+	bool dir_to_btn_support;
+	bool gyro_support;
+	bool anti_deadzone_support;
+
+	/* Current settings */
+	bool xbox_controller_enabled;
+	u8 gamepad_mode;
+	u8 left_deadzone;
+	u8 left_outer_threshold;
+	u8 right_deadzone;
+	u8 right_outer_threshold;
+	u8 left_anti_deadzone;
+	u8 right_anti_deadzone;
+	u8 left_trigger_min;
+	u8 left_trigger_max;
+	u8 right_trigger_min;
+	u8 right_trigger_max;
+};
+
 struct ally_handheld {
 	/* All read/write to IN interfaces must lock */
 	struct mutex intf_mutex;
@@ -190,6 +222,8 @@ struct ally_handheld {
 	unsigned long cad_last_event_time;
 
 	struct delayed_work resume_work;
+
+	struct ally_config *config;
 };
 
 struct asus_drvdata {
@@ -556,6 +590,219 @@ static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
 	return hidbuf;
 }
 
+/**
+ * ally_check_capability - Check if a specific capability is supported
+ * @hdev: HID device
+ * @ally: ally handheld structure
+ * @check_cmd: Capability command code to query
+ *
+ * Returns true if capability is supported, false otherwise
+ */
+static bool ally_check_capability(struct hid_device *hdev, struct ally_handheld *ally,
+				  enum ally_command_codes check_cmd)
+{
+	u8 payload[] = { 0x00 };
+	bool result = false;
+	int ret;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(check_cmd, payload, sizeof(payload));
+	if (!buf) {
+		hid_err(hdev, "Failed to allocate buffer for capability check.\n");
+		goto ally_check_capability_err;
+	}
+
+	ret = ally_gamepad_send_receive_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to check capability 0x%02x: %d\n", check_cmd, ret);
+		goto ally_check_capability_err;
+	}
+
+	if (buf[1] == HID_ALLY_FEATURE_CODE_PAGE && buf[2] == check_cmd)
+		result = (buf[4] == 0x01);
+
+ally_check_capability_err:
+	return result;
+}
+
+static int ally_detect_capabilities(struct hid_device *hdev, struct ally_handheld *ally,
+				    struct ally_config *cfg)
+{
+	if (!hdev || !cfg || !ally)
+		return -EINVAL;
+
+	scoped_guard(mutex, &cfg->config_mutex) {
+		cfg->is_ally_x = (hdev->product == USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X);
+
+		cfg->xbox_controller_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_XBOX_SUPPORT);
+		cfg->user_cal_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_USER_CAL_SUPPORT);
+		cfg->turbo_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_TURBO_SUPPORT);
+		cfg->resp_curve_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_RESP_CURVE_SUPPORT);
+		cfg->dir_to_btn_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_DIR_TO_BTN_SUPPORT);
+		cfg->gyro_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_GYRO_TO_JOYSTICK);
+		cfg->anti_deadzone_support =
+			ally_check_capability(hdev, ally, CMD_CHECK_ANTI_DEADZONE);
+	}
+
+	return 0;
+}
+
+static int ally_set_xbox_controller(struct hid_device *hdev,
+				    struct ally_config *cfg, bool enabled)
+{
+	u8 payload[] = { enabled ? 0x01 : 0x00 };
+	int ret;
+
+	if (!cfg || !cfg->xbox_controller_support)
+		return -ENODEV;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_XBOX_CONTROLLER, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set Xbox controller mode: %d\n", ret);
+		return ret;
+	}
+
+	cfg->xbox_controller_enabled = enabled;
+	return 0;
+}
+
+static ssize_t xbox_controller_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+	if (!cfg->xbox_controller_support)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%d\n", cfg->xbox_controller_enabled ? 1 : 0);
+}
+
+static ssize_t xbox_controller_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+	bool enabled;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+	if (!cfg->xbox_controller_support)
+		return -ENODEV;
+
+	ret = kstrtobool(buf, &enabled);
+	if (ret)
+		return ret;
+
+	ret = ally_set_xbox_controller(hdev, cfg, enabled);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(xbox_controller);
+
+static struct attribute *ally_config_attrs[] = {
+	&dev_attr_xbox_controller.attr,
+	NULL
+};
+
+static const struct attribute_group ally_attr_groups[] = {
+	{
+		.attrs = ally_config_attrs,
+	},
+};
+
+/**
+ * ally_config_create() - Initialize configuration and create sysfs entries
+ * @hdev: HID device
+ * @ally: Non-NULL ally device data with uninitialized config pointer
+ *
+ * Returns valid pointer on success, error pointer on failure.
+ */
+static struct ally_config *ally_config_create(struct hid_device *hdev, struct ally_handheld *ally)
+{
+	struct ally_config *cfg;
+	int ret, sysfs_i;
+
+	cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return ERR_PTR(-ENOMEM);
+
+	ret = ally_detect_capabilities(hdev, ally, cfg);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to detect Ally capabilities: %d\n", ret);
+		goto ally_config_create_err;
+	}
+
+	for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_attr_groups); sysfs_i++) {
+		ret = devm_device_add_group(&hdev->dev, &ally_attr_groups[sysfs_i]);
+		if (ret < 0) {
+			hid_err(hdev, "Failed to create sysfs group '%s': %d\n",
+				ally_attr_groups[sysfs_i].name, ret);
+			goto ally_config_create_sysfs_err;
+		}
+	}
+
+	cfg->gamepad_mode = 0x01;
+	cfg->left_deadzone = 10;
+	cfg->left_outer_threshold = 90;
+	cfg->right_deadzone = 10;
+	cfg->right_outer_threshold = 90;
+
+	/* So far the only hardware this is supported is the Ally 1 */
+	if (cfg->xbox_controller_support) {
+		ret = ally_set_xbox_controller(hdev, cfg, true);
+		if (ret < 0)
+			hid_warn(hdev, "Failed to set default Xbox controller mode: %d\n",
+				ret);
+	}
+
+	cfg->initialized = true;
+
+	return cfg;
+ally_config_create_sysfs_err:
+ally_config_create_err:
+	ally->config = NULL;
+	devm_kfree(&hdev->dev, cfg);
+	return ERR_PTR(ret);
+}
+
+/**
+ * ally_config_remove() - Clean up configuration resources
+ * @hdev: HID device
+ * @ally: Non-NULL Ally device data
+ */
+static void ally_config_remove(struct hid_device *hdev, struct ally_handheld *ally)
+{
+	struct ally_config *cfg = ally->config;
+
+	if (!cfg || !cfg->initialized)
+		return;
+}
+
 /*
  * This should be called before any remapping attempts,
  * and on driver init/resume, after the asus handshake
@@ -607,6 +854,9 @@ static int ally_get_endpoint_address(struct hid_device *hdev)
 	return ep->desc.bEndpointAddress;
 }
 
+/* Matches the 15-byte payload of the 16-byte 0x0B wire report:
+ * buttons[0..1] are button bitmaps, buttons[2] is the hatswitch.
+ */
 struct ally_x_input_report {
 	uint16_t x, y;
 	uint16_t rx, ry;
@@ -763,16 +1013,6 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
 {
 	int ret;
 
-	/*
-	 * This function assumes the asus-specific initialization
-	 * to have been performed already at this point.
-	 */
-	ret = ally_gamepad_check_ready(ally, hdev);
-	if (ret < 0) {
-		hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
-		return ret;
-	}
-
 	/* Failure at this point is non-critical */
 	ret = ally_gamepad_send_packet(ally, hdev, ALLY_FORCE_FEEDBACK_OFF,
 				       sizeof(ALLY_FORCE_FEEDBACK_OFF));
@@ -829,6 +1069,7 @@ static bool hid_asus_ally_raw_event(struct hid_device *hdev, struct ally_handhel
 static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
 {
 	int ret = 0, ep = ally_get_endpoint_address(hdev);
+	struct ally_config *ally_cfg;
 	struct hid_input *hidinput;
 
 	if (ep < 0)
@@ -838,8 +1079,30 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
 		switch (ep) {
 		case HID_ALLY_INTF_CFG_IN:
 			ally_drvdata.cfg_hdev = hdev;
+
+			/*
+			 * This function assumes the asus-specific initialization
+			 * to have been performed already at this point.
+			 */
+			ret = ally_gamepad_check_ready(&ally_drvdata, hdev);
+			if (ret < 0) {
+				hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
+				return ERR_PTR(ret);
+			}
+
+			ally_cfg = ally_config_create(hdev, &ally_drvdata);
+			if (IS_ERR(ally_cfg)) {
+				hid_err(hdev, "Failed to create Ally cfg: %ld\n",
+					PTR_ERR(ally_cfg));
+				ally_drvdata.cfg_hdev = NULL;
+				return ERR_PTR(PTR_ERR(ally_cfg));
+			}
+			ally_drvdata.config = ally_cfg;
+
 			ret = hid_asus_ally_init(hdev, &ally_drvdata);
 			if (ret < 0) {
+				ally_config_remove(hdev, &ally_drvdata);
+				ally_drvdata.config = NULL;
 				ally_drvdata.cfg_hdev = NULL;
 				return ERR_PTR(ret);
 			}
@@ -899,6 +1162,12 @@ static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *
 			ally->keyboard_input = NULL;
 			ally->keyboard_hdev = NULL;
 		}
+
+		if (ally->cfg_hdev == hdev) {
+			ally_config_remove(hdev, ally);
+			ally->cfg_hdev = NULL;
+			ally->config = NULL;
+		}
 	}
 }
 
@@ -913,6 +1182,16 @@ static int hid_asus_ally_reset_resume(struct hid_device *hdev, struct ally_handh
 	if (ep != HID_ALLY_INTF_CFG_IN)
 		return 0;
 
+	/*
+	 * This function assumes the asus-specific initialization
+	 * to have been performed already at this point.
+	 */
+	ret = ally_gamepad_check_ready(ally, hdev);
+	if (ret < 0) {
+		hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
+		return ret;
+	}
+
 	ret = hid_asus_ally_init(hdev, ally);
 	if (ret < 0)
 		return ret;
-- 
2.47.3

Re: [PATCH 03/12] HID: asus: add gamepad configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Dangling global pointer `ally_drvdata.cfg_hdev` on probe failure.
- [High] Uninitialized mutex `cfg->config_mutex`.
- [High] TOCTOU NULL pointer dereference in sysfs callbacks reading `ally->config`.
- [Medium] Missing lock acquisitions in sysfs store callback (`xbox_controller_store`).
- [Low] Mixing `goto`-based error handling with the `__free()` scope-based cleanup helper in `ally_check_capability()`.

Pre-existing issues:
- [High] Sleeping in atomic context by acquiring a mutex in HID `raw_event` callback.
--

--- Patch [3]: [PATCH 03/12] HID: asus: add gamepad configuration ---
commit 4090231155d8c981f8463cdfdb5cbc51349b174f
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add gamepad configuration

Add the base configuration structures for the gamepad configuration,
detect capabilities and initialize the device in a known state.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f4b2c4a616b75..6a15380488efb 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -556,6 +590,219 @@ static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
>  	return hidbuf;
>  }
>  
> +/**
> + * ally_check_capability - Check if a specific capability is supported
> + * @hdev: HID device
> + * @ally: ally handheld structure
> + * @check_cmd: Capability command code to query
> + *
> + * Returns true if capability is supported, false otherwise
> + */
> +static bool ally_check_capability(struct hid_device *hdev, struct ally_handheld *ally,
> +				  enum ally_command_codes check_cmd)
> +{
> +	u8 payload[] = { 0x00 };
> +	bool result = false;
> +	int ret;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(check_cmd, payload, sizeof(payload));
> +	if (!buf) {
> +		hid_err(hdev, "Failed to allocate buffer for capability check.\n");
> +		goto ally_check_capability_err;

[Severity: Low]
Does this code mix goto-based error handling with the __free() scope-based 
cleanup helper? The cleanup guidelines suggest that goto and cleanup helpers 
should never be mixed in the same function to avoid confusing ownership 
semantics.

> +	}
> +
> +	ret = ally_gamepad_send_receive_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);

[ ... ]

> +static int ally_set_xbox_controller(struct hid_device *hdev,
> +				    struct ally_config *cfg, bool enabled)
> +{
> +	u8 payload[] = { enabled ? 0x01 : 0x00 };
> +	int ret;
> +
> +	if (!cfg || !cfg->xbox_controller_support)
> +		return -ENODEV;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_XBOX_CONTROLLER, payload, sizeof(payload));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to set Xbox controller mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	cfg->xbox_controller_enabled = enabled;

[Severity: Medium]
Are we modifying the configuration state and interacting with the hardware 
without acquiring the required locks? The sysfs handler modifies 
cfg->xbox_controller_enabled and sends device reports, but neither 
cfg->config_mutex nor ally->intf_mutex appear to be held here. Could this 
cause a data race if userspace writes to the xbox_controller sysfs attribute?

> +	return 0;
> +}
> +
> +static ssize_t xbox_controller_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct ally_config *cfg;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	cfg = ally->config;

[Severity: High]
Is it possible for ally->config to be freed between the check and this 
assignment? If hid_asus_ally_remove() runs concurrently and sets 
ally->config = NULL, could this result in a NULL pointer dereference?

> +	if (!cfg->xbox_controller_support)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%d\n", cfg->xbox_controller_enabled ? 1 : 0);
> +}
> +
> +static ssize_t xbox_controller_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct ally_config *cfg;
> +	bool enabled;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	cfg = ally->config;

[Severity: High]
Like in xbox_controller_show(), could a concurrent device unbind cause a 
NULL pointer dereference here if ally->config is cleared after the check?

> +	if (!cfg->xbox_controller_support)
> +		return -ENODEV;

[ ... ]

> +static struct ally_config *ally_config_create(struct hid_device *hdev, struct ally_handheld *ally)
> +{
> +	struct ally_config *cfg;
> +	int ret, sysfs_i;
> +
> +	cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
> +	if (!cfg)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = ally_detect_capabilities(hdev, ally, cfg);

[Severity: High]
Are we missing an initialization for cfg->config_mutex? The config_mutex in 
struct ally_config is allocated with devm_kzalloc, but it doesn't seem to be 
initialized with mutex_init() before ally_detect_capabilities() uses 
scoped_guard() to lock it. Could this lead to a lockdep splat or panic?

> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to detect Ally capabilities: %d\n", ret);
> +		goto ally_config_create_err;
> +	}

[ ... ]

> @@ -838,8 +1079,30 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  		switch (ep) {
>  		case HID_ALLY_INTF_CFG_IN:
>  			ally_drvdata.cfg_hdev = hdev;
> +
> +			/*
> +			 * This function assumes the asus-specific initialization
> +			 * to have been performed already at this point.
> +			 */
> +			ret = ally_gamepad_check_ready(&ally_drvdata, hdev);
> +			if (ret < 0) {
> +				hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
> +				return ERR_PTR(ret);

[Severity: High]
If ally_gamepad_check_ready() fails, do we leave ally_drvdata.cfg_hdev 
pointing to the hid_device? Since ally_drvdata is global state and the device 
will be freed on probe failure, could this cause a use-after-free later?

> +			}
> +

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed something concerning in 
handle_ally_event() when it is called from the raw_event path:

drivers/hid/hid-asus.c:handle_ally_event() {
	...
		scoped_guard(mutex, &ally_data_mutex) {
			keyboard_input = ally->keyboard_input;
			if (keyboard_input) {
	...
}

Since handle_ally_event() is called from the HID raw_event callback, it 
runs in an atomic context where sleeping is prohibited. Could acquiring 
ally_data_mutex here lead to a kernel panic by sleeping in atomic context?

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

[PATCH 04/12] HID: asus: add vibration strength configuration

[Denis Benato]

ASUS ROG Ally handhelds support the vibration strength to be configured:
add sysfs attributes to allow userspace configure motors vibration
intensity.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c | 170 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 170 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 6a15380488ef..48cc867aeea8 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -205,6 +205,11 @@ struct ally_config {
 	u8 left_trigger_max;
 	u8 right_trigger_min;
 	u8 right_trigger_max;
+
+	/* Vibration settings */
+	u8 vibration_intensity_left;
+	u8 vibration_intensity_right;
+	bool vibration_active;
 };
 
 struct ally_handheld {
@@ -724,15 +729,177 @@ static ssize_t xbox_controller_store(struct device *dev,
 
 static DEVICE_ATTR_RW(xbox_controller);
 
+/**
+ * ally_set_vibration_intensity() - Set vibration intensity values
+ * @hdev: HID device
+ * @cfg: Ally config
+ * @left: Left motor intensity (0-100)
+ * @right: Right motor intensity (0-100)
+ *
+ * Returns 0 on success, negative error code on failure
+ */
+static int ally_set_vibration_intensity(struct hid_device *hdev, struct ally_config *cfg,
+					u8 left, u8 right)
+{
+	const u8 data[] = { left, right };
+	int ret;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_VIBRATION_INTENSITY, data, sizeof(data));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set vibration intensity: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t left_vibration_intensity_show(struct device *dev, struct device_attribute *attr,
+					char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	return sysfs_emit(buf, "%u\n", cfg->vibration_intensity_left);
+}
+
+static ssize_t left_vibration_intensity_store(struct device *dev, struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_vibration_intensity(hdev, cfg, value, cfg->vibration_intensity_right);
+	if (ret < 0)
+		return ret;
+
+	scoped_guard(mutex, &cfg->config_mutex)
+		cfg->vibration_intensity_left = value;
+
+	return count;
+}
+
+static ssize_t left_vibration_intensity_range_show(struct device *dev,
+						   struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "0 100\n");
+}
+
+static ssize_t right_vibration_intensity_show(struct device *dev, struct device_attribute *attr,
+					char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	return sysfs_emit(buf, "%u\n", cfg->vibration_intensity_right);
+}
+
+static ssize_t right_vibration_intensity_store(struct device *dev, struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_vibration_intensity(hdev, cfg, cfg->vibration_intensity_left, value);
+	if (ret < 0)
+		return ret;
+
+	scoped_guard(mutex, &cfg->config_mutex)
+		cfg->vibration_intensity_right = value;
+
+	return count;
+}
+
+static ssize_t right_vibration_intensity_range_show(struct device *dev,
+						    struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "0 100\n");
+}
+
+static struct device_attribute dev_attr_left_vibration_intensity =
+	__ATTR(intensity, 0644, left_vibration_intensity_show, left_vibration_intensity_store);
+
+static struct device_attribute dev_attr_left_vibration_intensity_range =
+	__ATTR(intensity_range, 0444, left_vibration_intensity_range_show, NULL);
+
+static struct device_attribute dev_attr_right_vibration_intensity =
+	__ATTR(intensity, 0644, right_vibration_intensity_show, right_vibration_intensity_store);
+
+static struct device_attribute dev_attr_right_vibration_intensity_range =
+	__ATTR(intensity_range, 0444, right_vibration_intensity_range_show, NULL);
+
 static struct attribute *ally_config_attrs[] = {
 	&dev_attr_xbox_controller.attr,
 	NULL
 };
 
+static struct attribute *ally_left_vibration_attrs[] = {
+	&dev_attr_left_vibration_intensity.attr,
+	&dev_attr_left_vibration_intensity_range.attr,
+	NULL
+};
+
+static struct attribute *ally_right_vibration_attrs[] = {
+	&dev_attr_right_vibration_intensity.attr,
+	&dev_attr_right_vibration_intensity_range.attr,
+	NULL
+};
+
 static const struct attribute_group ally_attr_groups[] = {
 	{
 		.attrs = ally_config_attrs,
 	},
+	{
+		.name = "left_vibration",
+		.attrs = ally_left_vibration_attrs,
+	},
+	{
+		.name = "right_vibration",
+		.attrs = ally_right_vibration_attrs,
+	},
 };
 
 /**
@@ -771,6 +938,9 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
 	cfg->left_outer_threshold = 90;
 	cfg->right_deadzone = 10;
 	cfg->right_outer_threshold = 90;
+	cfg->vibration_intensity_left = 100;
+	cfg->vibration_intensity_right = 100;
+	cfg->vibration_active = false;
 
 	/* So far the only hardware this is supported is the Ally 1 */
 	if (cfg->xbox_controller_support) {
-- 
2.47.3

Re: [PATCH 04/12] HID: asus: add vibration strength configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Lockless read-modify-write (TOCTOU) race condition in vibration sysfs store functions causes hardware-software state mismatch.
- [Low] The patch introduces an unused struct field `vibration_active` and an unused function parameter `cfg`.
--

--- Patch [4]: [PATCH 04/12] HID: asus: add vibration strength configuration ---
commit 88d84618d4463bdab2563d7c2980a265cf17afa3
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add vibration strength configuration

This commit adds sysfs attributes to allow userspace to configure the
vibration intensity of the motors on ASUS ROG Ally handhelds.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6a15380488efb..48cc867aeea86 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -205,6 +205,11 @@ struct ally_config {
>  	u8 left_trigger_max;
>  	u8 right_trigger_min;
>  	u8 right_trigger_max;
> +
> +	/* Vibration settings */
> +	u8 vibration_intensity_left;
> +	u8 vibration_intensity_right;
> +	bool vibration_active;

[Severity: Low]
This isn't a bug, but vibration_active is added here and initialized in
ally_config_create(), but never appears to be read or used elsewhere.

[ ... ]

> @@ -724,15 +729,177 @@ static ssize_t xbox_controller_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(xbox_controller);
>  
> +/**
> + * ally_set_vibration_intensity() - Set vibration intensity values
[ ... ]
> + */
> +static int ally_set_vibration_intensity(struct hid_device *hdev, struct ally_config *cfg,
> +					u8 left, u8 right)

[Severity: Low]
This isn't a bug, but the cfg parameter is never utilized inside this
function.

[ ... ]

> +static ssize_t left_vibration_intensity_store(struct device *dev, struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
[ ... ]
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	ret = ally_set_vibration_intensity(hdev, cfg, value, cfg->vibration_intensity_right);
> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		cfg->vibration_intensity_left = value;
> +
> +	return count;
> +}

[Severity: Medium]
Does reading cfg->vibration_intensity_right outside of cfg->config_mutex
create a race condition here?

If concurrent sysfs writes happen for the left and right intensities, could
they read stale complementary values, interleave their hardware I/O
requests, and leave the hardware state permanently desynchronized from the
driver's software cache?

[ ... ]

> +static ssize_t right_vibration_intensity_store(struct device *dev, struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
[ ... ]
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	ret = ally_set_vibration_intensity(hdev, cfg, cfg->vibration_intensity_left, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		cfg->vibration_intensity_right = value;
> +
> +	return count;
> +}

[Severity: Medium]
The same lockless read-modify-write pattern occurs here with
cfg->vibration_intensity_left. Would it be safer to take the lock before
reading the complementary intensity and hold it across the hardware request?

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

[PATCH 05/12] HID: asus: add joysticks inner and outer range configuration

[Denis Benato]

ROG Ally devices supports configuring joysticks inner and outer range:
add sysfs attributes to allow userspace modifying the sensitivity
of those controllers.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c | 308 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 308 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 48cc867aeea8..010b33533068 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -133,6 +133,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 
 #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
 
+#define ALLY_DEVICE_ATTR_RO(_name, _sysfs_name)    \
+	struct device_attribute dev_attr_##_name = \
+		__ATTR(_sysfs_name, 0444, _name##_show, NULL)
+
+#define ALLY_DEVICE_CONST_ATTR_RO(fname, sysfs_name, value)			\
+	static ssize_t fname##_show(struct device *dev,				\
+				   struct device_attribute *attr, char *buf)	\
+	{									\
+		return sysfs_emit(buf, value);					\
+	}									\
+	struct device_attribute dev_attr_##fname =				\
+		__ATTR(sysfs_name, 0444, fname##_show, NULL)
+
 enum asus_work_action_type {
 	FN_LOCK_SYNC,
 	BRIGHTNESS_SET,
@@ -871,6 +884,277 @@ static struct device_attribute dev_attr_right_vibration_intensity =
 static struct device_attribute dev_attr_right_vibration_intensity_range =
 	__ATTR(intensity_range, 0444, right_vibration_intensity_range_show, NULL);
 
+/**
+ * ally_set_joystick_thresholds() - Generic function to set joystick ranges
+ *
+ * This function send the command to set both inner and outer threshold for
+ * the left and right joysticks.
+ *
+ * @hdev: HID device
+ * @cfg: Ally config
+ * @left_it: inner threshold (deadzone) of the left stick (0-50)
+ * @left_ot: outer threshold of the left stick (70-100)
+ * @right_it: inner threshold (deadzone) of the right stick (0-50)
+ * @right_ot: outer threshold of the right stick (70-100)
+ *
+ * Returns 0 on success, negative error code on failure
+ */
+static int ally_set_joystick_thresholds(struct hid_device *hdev, struct ally_config *cfg,
+					u8 left_it, u8 left_ot, u8 right_it, u8 right_ot)
+{
+	u8 payload[] = { left_it, left_ot, right_it, right_ot };
+	int ret;
+
+	if (!cfg->xbox_controller_support)
+		return -ENODEV;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_JOYSTICK_DEADZONE, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set joystick ranges: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t left_joystick_inner_threshold_show(struct device *dev, struct device_attribute *attr,
+				      char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->left_deadzone);
+}
+
+static ssize_t left_joystick_inner_threshold_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 50)
+		return -EINVAL;
+
+	ret = ally_set_joystick_thresholds(hdev, ally->config,
+					   value,
+					   ally->config->left_outer_threshold,
+					   ally->config->right_deadzone,
+					   ally->config->right_outer_threshold);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->left_deadzone = value;
+
+	return count;
+}
+
+static ssize_t left_joystick_inner_threshold_range_show(struct device *dev,
+							 struct device_attribute *attr,
+							 char *buf)
+{
+	return sysfs_emit(buf, "0 50\n");
+}
+
+static ssize_t left_joystick_outer_threshold_show(struct device *dev, struct device_attribute *attr,
+				      char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->left_outer_threshold);
+}
+
+static ssize_t left_joystick_outer_threshold_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value < 70 || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_joystick_thresholds(hdev, ally->config,
+					   ally->config->left_deadzone,
+					   value,
+					   ally->config->right_deadzone,
+					   ally->config->right_outer_threshold);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->left_outer_threshold = value;
+
+	return count;
+}
+
+static ssize_t left_joystick_outer_threshold_range_show(struct device *dev,
+							struct device_attribute *attr,
+							char *buf)
+{
+	return sysfs_emit(buf, "70 100\n");
+}
+
+static ssize_t right_joystick_inner_threshold_show(struct device *dev,
+						   struct device_attribute *attr,
+						   char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->right_deadzone);
+}
+
+static ssize_t right_joystick_inner_threshold_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 50)
+		return -EINVAL;
+
+	ret = ally_set_joystick_thresholds(hdev, ally->config,
+					   ally->config->left_deadzone,
+					   ally->config->left_outer_threshold,
+					   value,
+					   ally->config->right_outer_threshold);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->right_deadzone = value;
+
+	return count;
+}
+
+static ssize_t right_joystick_inner_threshold_range_show(struct device *dev,
+							  struct device_attribute *attr,
+							  char *buf)
+{
+	return sysfs_emit(buf, "0 50\n");
+}
+
+static ssize_t right_joystick_outer_threshold_show(struct device *dev,
+						   struct device_attribute *attr,
+						   char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->right_outer_threshold);
+}
+
+static ssize_t right_joystick_outer_threshold_store(struct device *dev,
+						    struct device_attribute *attr,
+						    const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value < 70 || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_joystick_thresholds(hdev, ally->config,
+					   ally->config->left_deadzone,
+					   ally->config->left_outer_threshold,
+					   ally->config->right_deadzone,
+					   value);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->right_outer_threshold = value;
+
+	return count;
+}
+
+static ssize_t right_joystick_outer_threshold_range_show(struct device *dev,
+							 struct device_attribute *attr,
+							 char *buf)
+{
+	return sysfs_emit(buf, "70 100\n");
+}
+
+static struct device_attribute dev_attr_left_joystick_inner_threshold =
+	__ATTR(inner_threshold, 0644, left_joystick_inner_threshold_show,
+	       left_joystick_inner_threshold_store);
+
+static struct device_attribute dev_attr_left_joystick_inner_threshold_range =
+	__ATTR(inner_threshold_range, 0444, left_joystick_inner_threshold_range_show, NULL);
+
+static struct device_attribute dev_attr_left_joystick_outer_threshold =
+	__ATTR(outer_threshold, 0644, left_joystick_outer_threshold_show,
+	       left_joystick_outer_threshold_store);
+
+static struct device_attribute dev_attr_left_joystick_outer_threshold_range =
+	__ATTR(outer_threshold_range, 0444, left_joystick_outer_threshold_range_show, NULL);
+
+static struct device_attribute dev_attr_right_joystick_inner_threshold =
+	__ATTR(inner_threshold, 0644, right_joystick_inner_threshold_show,
+	       right_joystick_inner_threshold_store);
+
+static struct device_attribute dev_attr_right_joystick_inner_threshold_range =
+	__ATTR(inner_threshold_range, 0444, right_joystick_inner_threshold_range_show, NULL);
+
+static struct device_attribute dev_attr_right_joystick_outer_threshold =
+	__ATTR(outer_threshold, 0644, right_joystick_outer_threshold_show,
+	       right_joystick_outer_threshold_store);
+
+static struct device_attribute dev_attr_right_joystick_outer_threshold_range =
+	__ATTR(outer_threshold_range, 0444, right_joystick_outer_threshold_range_show, NULL);
+
 static struct attribute *ally_config_attrs[] = {
 	&dev_attr_xbox_controller.attr,
 	NULL
@@ -888,6 +1172,22 @@ static struct attribute *ally_right_vibration_attrs[] = {
 	NULL
 };
 
+static struct attribute *left_joystick_axis_attrs[] = {
+	&dev_attr_left_joystick_inner_threshold.attr,
+	&dev_attr_left_joystick_outer_threshold.attr,
+	&dev_attr_left_joystick_inner_threshold_range.attr,
+	&dev_attr_left_joystick_outer_threshold_range.attr,
+	NULL
+};
+
+static struct attribute *right_joystick_axis_attrs[] = {
+	&dev_attr_right_joystick_inner_threshold.attr,
+	&dev_attr_right_joystick_outer_threshold.attr,
+	&dev_attr_right_joystick_inner_threshold_range.attr,
+	&dev_attr_right_joystick_outer_threshold_range.attr,
+	NULL
+};
+
 static const struct attribute_group ally_attr_groups[] = {
 	{
 		.attrs = ally_config_attrs,
@@ -900,6 +1200,14 @@ static const struct attribute_group ally_attr_groups[] = {
 		.name = "right_vibration",
 		.attrs = ally_right_vibration_attrs,
 	},
+	{
+		.name = "left_joystick_axis",
+		.attrs = left_joystick_axis_attrs,
+	},
+	{
+		.name = "right_joystick_axis",
+		.attrs = right_joystick_axis_attrs,
+	},
 };
 
 /**
-- 
2.47.3

Re: [PATCH 05/12] HID: asus: add joysticks inner and outer range configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Uninitialized Mutex `config_mutex` in `ally_config`
- [High] NULL Pointer Dereference due to TOCTOU on `ally->config`
- [Medium] Data Race and State Desynchronization in Sysfs Handlers
- [Low] Dead Code: Unused Macros
--

--- Patch [5]: [PATCH 05/12] HID: asus: add joysticks inner and outer range configuration ---
commit bd4086a93a46d9dd4b9c0379eb086b2a077f1489
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add joysticks inner and outer range configuration

ROG Ally devices supports configuring joysticks inner and outer range:
add sysfs attributes to allow userspace modifying the sensitivity
of those controllers.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 48cc867aeea86..010b33533068a 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -133,6 +133,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  
>  #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
>  
> +#define ALLY_DEVICE_ATTR_RO(_name, _sysfs_name)    \
> +	struct device_attribute dev_attr_##_name = \
> +		__ATTR(_sysfs_name, 0444, _name##_show, NULL)
> +
> +#define ALLY_DEVICE_CONST_ATTR_RO(fname, sysfs_name, value)			\
> +	static ssize_t fname##_show(struct device *dev,				\
> +				   struct device_attribute *attr, char *buf)	\
> +	{									\
> +		return sysfs_emit(buf, value);					\
> +	}									\
> +	struct device_attribute dev_attr_##fname =				\
> +		__ATTR(sysfs_name, 0444, fname##_show, NULL)

[Severity: Low]
Are these macros used anywhere in this patch or the rest of the codebase?

[ ... ]

> +static ssize_t left_joystick_inner_threshold_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	u8 value;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 50)
> +		return -EINVAL;
> +
> +	ret = ally_set_joystick_thresholds(hdev, ally->config,
> +					   value,
> +					   ally->config->left_outer_threshold,
> +					   ally->config->right_deadzone,
> +					   ally->config->right_outer_threshold);

[Severity: High]
Could ally->config become NULL after the initial check if a concurrent device
removal (like USB unplug) occurs while this sysfs write is executing?

If hid_asus_ally_remove() runs concurrently and sets ally->config to NULL:

hid_asus_ally_remove() {
    if (ally->cfg_hdev == hdev) {
        ally_config_remove(hdev, ally);
        ally->cfg_hdev = NULL;
        ally->config = NULL;
    }
}

Then this function could dereference a NULL pointer when reading
ally->config->left_outer_threshold. Would it be safer to cache the config
pointer locally?

[Severity: Medium]
Does reading ally->config->left_outer_threshold and other variables
locklessly create a race condition with other concurrent sysfs writes?

If two writes interleave, they could both read the same threshold values
before either updates the hardware, causing the hardware state to become
desynchronized with the sysfs cache. Should the mutex be acquired before
reading the current configuration and sending the hardware command?

> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally->config->config_mutex)

[Severity: High]
Is config_mutex initialized before being used here?

Looking at ally_config_create(), the configuration structure is allocated
with devm_kzalloc(), but mutex_init() is never called on config_mutex. Will
acquiring this all-zeros mutex trigger a crash or lockdep failure?

> +		ally->config->left_deadzone = value;
> +
> +	return count;
> +}

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

[PATCH 06/12] HID: asus: add triggers inner and outer range configuration

[Denis Benato]

ROG Ally devices allows configuring inner and outer ranges for triggers
buttons on the back: allow userspace to configure the sesitivity by
exposing sysfs attributes.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c | 298 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 298 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 010b33533068..eb735ec8f065 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1155,6 +1155,276 @@ static struct device_attribute dev_attr_right_joystick_outer_threshold =
 static struct device_attribute dev_attr_right_joystick_outer_threshold_range =
 	__ATTR(outer_threshold_range, 0444, right_joystick_outer_threshold_range_show, NULL);
 
+/**
+ * ally_set_trigger_ranges() - Generic function to set triggers ranges
+ *
+ * This function send the command to set both inner and outer threshold for
+ * the left and right triggers.
+ *
+ * @hdev: HID device
+ * @cfg: Ally config
+ * @left_it: lower limit of the left trigger range (0-50)
+ * @left_ot: upper limit of the left trigger range (70-100)
+ * @right_it: lower limit of the right trigger range (0-50)
+ * @right_ot: upper limit of the right trigger range (70-100)
+ *
+ * Returns 0 on success, negative error code on failure
+ */
+static int ally_set_trigger_ranges(struct hid_device *hdev, struct ally_config *cfg,
+				   u8 left_it, u8 left_ot, u8 right_it, u8 right_ot)
+{
+	const u8 payload[] = { left_it, left_ot, right_it, right_ot };
+	int ret;
+
+	if (!cfg->xbox_controller_support)
+		return -ENODEV;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_TRIGGER_RANGE, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set trigger ranges: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t left_trigger_range_lower_limit_show(struct device *dev,
+						   struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->left_trigger_min);
+}
+
+static ssize_t left_trigger_range_lower_limit_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 50)
+		return -EINVAL;
+
+	ret = ally_set_trigger_ranges(hdev, ally->config,
+					   value,
+					   ally->config->left_trigger_max,
+					   ally->config->right_trigger_min,
+					   ally->config->right_trigger_max);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->left_trigger_min = value;
+
+	return count;
+}
+
+static ssize_t left_trigger_range_lower_limit_range_show(struct device *dev,
+							 struct device_attribute *attr,
+							 char *buf)
+{
+	return sysfs_emit(buf, "0 50\n");
+}
+
+static ssize_t right_trigger_range_upper_limit_show(struct device *dev,
+						    struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->right_trigger_max);
+}
+
+static ssize_t right_trigger_range_upper_limit_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value < 70 || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_trigger_ranges(hdev, ally->config,
+					   ally->config->left_trigger_min,
+					   ally->config->left_trigger_max,
+					   ally->config->right_trigger_min,
+					   value);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->right_trigger_max = value;
+
+	return count;
+}
+
+static ssize_t right_trigger_range_upper_limit_range_show(struct device *dev,
+							 struct device_attribute *attr,
+							 char *buf)
+{
+	return sysfs_emit(buf, "70 100\n");
+}
+
+static ssize_t right_trigger_range_lower_limit_show(struct device *dev,
+						    struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->right_trigger_min);
+}
+
+static ssize_t right_trigger_range_lower_limit_store(struct device *dev,
+						     struct device_attribute *attr,
+						     const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 50)
+		return -EINVAL;
+
+	ret = ally_set_trigger_ranges(hdev, ally->config,
+					   ally->config->left_trigger_min,
+					   ally->config->left_trigger_max,
+					   value,
+					   ally->config->right_trigger_max);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->right_trigger_min = value;
+
+	return count;
+}
+
+static ssize_t right_trigger_range_lower_limit_range_show(struct device *dev,
+							  struct device_attribute *attr,
+							  char *buf)
+{
+	return sysfs_emit(buf, "0 50\n");
+}
+
+static ssize_t left_trigger_range_upper_limit_show(struct device *dev,
+						   struct device_attribute *attr,
+						   char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%u\n", ally->config->left_trigger_max);
+}
+
+static ssize_t left_trigger_range_upper_limit_store(struct device *dev,
+						    struct device_attribute *attr,
+						    const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value < 70 || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_trigger_ranges(hdev, ally->config,
+					   ally->config->left_trigger_min,
+					   value,
+					   ally->config->right_trigger_min,
+					   ally->config->right_trigger_max);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->left_trigger_max = value;
+
+	return count;
+}
+
+static ssize_t left_trigger_range_upper_limit_range_show(struct device *dev,
+							 struct device_attribute *attr,
+							 char *buf)
+{
+	return sysfs_emit(buf, "70 100\n");
+}
+
+static struct device_attribute dev_attr_left_trigger_range_lower_limit =
+	__ATTR(range_lower_limit, 0644, left_trigger_range_lower_limit_show,
+	       left_trigger_range_lower_limit_store);
+
+static struct device_attribute dev_attr_left_trigger_range_lower_limit_range =
+	__ATTR(range_lower_limit_range, 0444, left_trigger_range_lower_limit_range_show, NULL);
+
+static struct device_attribute dev_attr_left_trigger_range_upper_limit =
+	__ATTR(range_upper_limit, 0644, left_trigger_range_upper_limit_show,
+	       left_trigger_range_upper_limit_store);
+
+static struct device_attribute dev_attr_left_trigger_range_upper_limit_range =
+	__ATTR(range_upper_limit_range, 0444, left_trigger_range_upper_limit_range_show, NULL);
+
+static struct device_attribute dev_attr_right_trigger_range_lower_limit =
+	__ATTR(range_lower_limit, 0644, right_trigger_range_lower_limit_show,
+	       right_trigger_range_lower_limit_store);
+
+static struct device_attribute dev_attr_right_trigger_range_lower_limit_range =
+	__ATTR(range_lower_limit_range, 0444, right_trigger_range_lower_limit_range_show, NULL);
+
+static struct device_attribute dev_attr_right_trigger_range_upper_limit =
+	__ATTR(range_upper_limit, 0644, right_trigger_range_upper_limit_show,
+	       right_trigger_range_upper_limit_store);
+
+static struct device_attribute dev_attr_right_trigger_range_upper_limit_range =
+	__ATTR(range_upper_limit_range, 0444, right_trigger_range_upper_limit_range_show, NULL);
+
 static struct attribute *ally_config_attrs[] = {
 	&dev_attr_xbox_controller.attr,
 	NULL
@@ -1188,6 +1458,22 @@ static struct attribute *right_joystick_axis_attrs[] = {
 	NULL
 };
 
+static struct attribute *left_trigger_attrs[] = {
+	&dev_attr_left_trigger_range_lower_limit.attr,
+	&dev_attr_left_trigger_range_upper_limit.attr,
+	&dev_attr_left_trigger_range_lower_limit_range.attr,
+	&dev_attr_left_trigger_range_upper_limit_range.attr,
+	NULL
+};
+
+static struct attribute *right_trigger_attrs[] = {
+	&dev_attr_right_trigger_range_lower_limit.attr,
+	&dev_attr_right_trigger_range_upper_limit.attr,
+	&dev_attr_right_trigger_range_lower_limit_range.attr,
+	&dev_attr_right_trigger_range_upper_limit_range.attr,
+	NULL
+};
+
 static const struct attribute_group ally_attr_groups[] = {
 	{
 		.attrs = ally_config_attrs,
@@ -1208,6 +1494,14 @@ static const struct attribute_group ally_attr_groups[] = {
 		.name = "right_joystick_axis",
 		.attrs = right_joystick_axis_attrs,
 	},
+	{
+		.name = "left_trigger",
+		.attrs = left_trigger_attrs,
+	},
+	{
+		.name = "right_trigger",
+		.attrs = right_trigger_attrs,
+	},
 };
 
 /**
@@ -1246,6 +1540,10 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
 	cfg->left_outer_threshold = 90;
 	cfg->right_deadzone = 10;
 	cfg->right_outer_threshold = 90;
+	cfg->left_trigger_min = 0;
+	cfg->left_trigger_max = 100;
+	cfg->right_trigger_min = 0;
+	cfg->right_trigger_max = 100;
 	cfg->vibration_intensity_left = 100;
 	cfg->vibration_intensity_right = 100;
 	cfg->vibration_active = false;
-- 
2.47.3

Re: [PATCH 06/12] HID: asus: add triggers inner and outer range configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Time-of-check to time-of-use (TOCTOU) race condition in trigger threshold sysfs stores causes hardware state desynchronization.

Pre-existing issues:
- [High] The `config_mutex` in `struct ally_config` is dynamically allocated but never explicitly initialized.
--

--- Patch [6]: [PATCH 06/12] HID: asus: add triggers inner and outer range configuration ---
commit bf1ab6fc2d6df4d87a7b2cef0f0cf8628bb2f010
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add triggers inner and outer range configuration

This patch adds sysfs attributes to configure the inner and outer ranges for
the left and right triggers on ROG Ally devices. It implements the store and
show functions for these limits and sends the updated configuration to the
hardware.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 010b33533068a..eb735ec8f065c 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static ssize_t left_trigger_range_lower_limit_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	u8 value;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 50)
> +		return -EINVAL;
> +
> +	ret = ally_set_trigger_ranges(hdev, ally->config,
> +					   value,
> +					   ally->config->left_trigger_max,
> +					   ally->config->right_trigger_min,
> +					   ally->config->right_trigger_max);

[Severity: Medium]
Could this cause a TOCTOU data race if multiple threads write to the trigger
sysfs attributes concurrently?

Since the other limit values are read locklessly from `ally->config` before the
hardware is updated, concurrent writes to different attributes could interleave.

This might result in one thread sending a stale limit while overwriting the
other thread's hardware update, permanently desynchronizing the hardware
configuration from the driver's software cache.

Should the mutex be acquired before calling `ally_set_trigger_ranges()`?

> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally->config->config_mutex)
> +		ally->config->left_trigger_min = value;
> +
> +	return count;
> +}

[ ... ]

> @@ -1246,6 +1540,10 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  	cfg->left_outer_threshold = 90;
>  	cfg->right_deadzone = 10;
>  	cfg->right_outer_threshold = 90;
> +	cfg->left_trigger_min = 0;
> +	cfg->left_trigger_max = 100;
> +	cfg->right_trigger_min = 0;
> +	cfg->right_trigger_max = 100;

[Severity: High]
This is a pre-existing issue, but is `config_mutex` ever explicitly initialized?

In `ally_config_create()`, `cfg` is dynamically allocated via `devm_kzalloc()`
which zero-initializes the memory.  However, it appears
`mutex_init(&cfg->config_mutex)` is never called before the mutex is locked in
functions like `ally_detect_capabilities()` or the sysfs store operations.

Could this lead to a kernel panic or lockdep warning when the uninitialized
mutex is locked under contention?

>  	cfg->vibration_intensity_left = 100;
>  	cfg->vibration_intensity_right = 100;
>  	cfg->vibration_active = false;

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

[PATCH 07/12] HID: asus: add joysticks anti-deadzone configuration

[Denis Benato]

ROG Ally devices allow configuring the anti-deadzone parameter for
the resistive joysticks devices as over time those develops drift,
therefore allow userspace to configure the anti-deadzone by exposing
relevant sysfs attributes.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c | 163 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 163 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index eb735ec8f065..8ed10c5b22ab 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1155,6 +1155,165 @@ static struct device_attribute dev_attr_right_joystick_outer_threshold =
 static struct device_attribute dev_attr_right_joystick_outer_threshold_range =
 	__ATTR(outer_threshold_range, 0444, right_joystick_outer_threshold_range_show, NULL);
 
+/**
+ * ally_set_anti_deadzone - Set anti-deadzone values for joysticks
+ * @hdev: HID device
+ * @left_adz: Left joystick anti-deadzone value (0-100)
+ * @right_adz: Right joystick anti-deadzone value (0-100)
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int ally_set_anti_deadzone(struct hid_device *hdev, u8 left_adz, u8 right_adz)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	const u8 payload[] = { left_adz, right_adz };
+	int ret;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_ANTI_DEADZONE, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	if (!ally->config->anti_deadzone_support) {
+		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set anti-deadzone values: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t left_joystick_anti_deadzone_show(struct device *dev, struct device_attribute *attr,
+						char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	if (!ally->config->anti_deadzone_support) {
+		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
+		return -EOPNOTSUPP;
+	}
+
+	return sysfs_emit(buf, "%u\n", ally->config->left_anti_deadzone);
+}
+
+static ssize_t left_joystick_anti_deadzone_store(struct device *dev, struct device_attribute *attr,
+						 const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	if (!ally->config->anti_deadzone_support) {
+		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_anti_deadzone(hdev, value, ally->config->right_anti_deadzone);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->left_anti_deadzone = value;
+
+	return count;
+}
+
+static ssize_t left_joystick_anti_deadzone_range_show(struct device *dev,
+						       struct device_attribute *attr,
+						       char *buf)
+{
+	return sysfs_emit(buf, "0 100\n");
+}
+
+static ssize_t right_joystick_anti_deadzone_show(struct device *dev, struct device_attribute *attr,
+						char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	if (!ally->config->anti_deadzone_support) {
+		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
+		return -EOPNOTSUPP;
+	}
+
+	return sysfs_emit(buf, "%u\n", ally->config->right_anti_deadzone);
+}
+
+static ssize_t right_joystick_anti_deadzone_store(struct device *dev, struct device_attribute *attr,
+						 const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	u8 value;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	if (!ally->config->anti_deadzone_support) {
+		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret || value > 100)
+		return -EINVAL;
+
+	ret = ally_set_anti_deadzone(hdev, ally->config->left_anti_deadzone, value);
+	if (ret)
+		return ret;
+
+	scoped_guard(mutex, &ally->config->config_mutex)
+		ally->config->right_anti_deadzone = value;
+
+	return count;
+}
+
+static ssize_t right_joystick_anti_deadzone_range_show(struct device *dev,
+							struct device_attribute *attr,
+							char *buf)
+{
+	return sysfs_emit(buf, "0 100\n");
+}
+
+static struct device_attribute dev_attr_left_joystick_anti_deadzone =
+	__ATTR(anti_deadzone, 0644, left_joystick_anti_deadzone_show,
+	       left_joystick_anti_deadzone_store);
+
+static struct device_attribute dev_attr_left_joystick_anti_deadzone_range =
+	__ATTR(anti_deadzone_range, 0444, left_joystick_anti_deadzone_range_show, NULL);
+
+static struct device_attribute dev_attr_right_joystick_anti_deadzone =
+	__ATTR(anti_deadzone, 0644, right_joystick_anti_deadzone_show,
+	       right_joystick_anti_deadzone_store);
+
+static struct device_attribute dev_attr_right_joystick_anti_deadzone_range =
+	__ATTR(anti_deadzone_range, 0444, right_joystick_anti_deadzone_range_show, NULL);
+
 /**
  * ally_set_trigger_ranges() - Generic function to set triggers ranges
  *
@@ -1447,6 +1606,8 @@ static struct attribute *left_joystick_axis_attrs[] = {
 	&dev_attr_left_joystick_outer_threshold.attr,
 	&dev_attr_left_joystick_inner_threshold_range.attr,
 	&dev_attr_left_joystick_outer_threshold_range.attr,
+	&dev_attr_left_joystick_anti_deadzone.attr,
+	&dev_attr_left_joystick_anti_deadzone_range.attr,
 	NULL
 };
 
@@ -1455,6 +1616,8 @@ static struct attribute *right_joystick_axis_attrs[] = {
 	&dev_attr_right_joystick_outer_threshold.attr,
 	&dev_attr_right_joystick_inner_threshold_range.attr,
 	&dev_attr_right_joystick_outer_threshold_range.attr,
+	&dev_attr_right_joystick_anti_deadzone.attr,
+	&dev_attr_right_joystick_anti_deadzone_range.attr,
 	NULL
 };
 
-- 
2.47.3

Re: [PATCH 07/12] HID: asus: add joysticks anti-deadzone configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Concurrent sysfs writes to left and right anti-deadzone attributes can desynchronize the hardware state from the software state due to a Time-of-Check to Time-of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] The `config_mutex` inside `struct ally_config` is used without being initialized.
- [High] Sysfs attribute callbacks can dereference a NULL `ally->config` pointer if a concurrent device removal occurs.
--

--- Patch [7]: [PATCH 07/12] HID: asus: add joysticks anti-deadzone configuration ---
commit d78f09e927da53af9bf7a52457ad40ae819eb0ce
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add joysticks anti-deadzone configuration

ROG Ally devices allow configuring the anti-deadzone parameter for
the resistive joysticks devices as over time those develops drift.
This commit exposes relevant sysfs attributes to allow userspace to
configure the anti-deadzone.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index eb735ec8f065c..8ed10c5b22ab5 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1155,6 +1155,165 @@ static struct device_attribute dev_attr_right_joystick_outer_threshold =
>  static struct device_attribute dev_attr_right_joystick_outer_threshold_range =
>  	__ATTR(outer_threshold_range, 0444, right_joystick_outer_threshold_range_show, NULL);
>  
> +/**
> + * ally_set_anti_deadzone - Set anti-deadzone values for joysticks
> + * @hdev: HID device
> + * @left_adz: Left joystick anti-deadzone value (0-100)
> + * @right_adz: Right joystick anti-deadzone value (0-100)
> + *
> + * Return: 0 on success, negative on failure
> + */
> +static int ally_set_anti_deadzone(struct hid_device *hdev, u8 left_adz, u8 right_adz)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	const u8 payload[] = { left_adz, right_adz };
> +	int ret;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_ANTI_DEADZONE, payload, sizeof(payload));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (!ally->config->anti_deadzone_support) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but could this dereference a NULL pointer?

If a concurrent device removal occurs, the remove() callback explicitly sets
ally->config = NULL. Since the sysfs groups are managed by devres, they are
removed after the remove() callback completes, leaving a race window.

A concurrent sysfs access could pass the initial check in
left_joystick_anti_deadzone_store(), get interrupted while the remove path
nullifies ally->config, and then crash here when ally_set_anti_deadzone()
directly dereferences it.

> +		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to set anti-deadzone values: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static ssize_t left_joystick_anti_deadzone_store(struct device *dev, struct device_attribute *attr,
> +						 const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	u8 value;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	if (!ally->config->anti_deadzone_support) {
> +		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	ret = ally_set_anti_deadzone(hdev, value, ally->config->right_anti_deadzone);
> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally->config->config_mutex)
> +		ally->config->left_anti_deadzone = value;

[Severity: Medium]
Could this result in a Time-of-Check to Time-of-Use (TOCTOU) race condition?

If two threads concurrently write to the left and right anti-deadzone
attributes, they could locklessly read the opposite joystick's state from
ally->config, send conflicting commands to the hardware, and then sequentially
acquire config_mutex to update the software cache. Depending on which hardware
request reaches the device last, the hardware might drop one of the updates,
but the software cache would reflect both new values, causing a permanent
desynchronization.

[Severity: High]
This is a pre-existing issue, but I noticed config_mutex inside
struct ally_config is used here without being initialized.

Looking at ally_config_create(), it is allocated with devm_kzalloc():

    cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
    if (!cfg)
        return ERR_PTR(-ENOMEM);

Because the memory is zeroed but mutex_init() is never called, the wait
queue list head (wait_list) is not initialized. Could this cause a kernel
crash in the mutex slow path if there is contention or if
CONFIG_DEBUG_MUTEXES is enabled?

> +
> +	return count;
> +}

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

[PATCH 08/12] HID: asus: add support for response curve

[Denis Benato]

ROG ally devices allows configuring the response curve of both joysticks,
therefore add the ability of userspace to modify the response curve by
exposing relevant sysfs attributes.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c | 375 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 374 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8ed10c5b22ab..8018e61cf026 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -137,7 +137,15 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 	struct device_attribute dev_attr_##_name = \
 		__ATTR(_sysfs_name, 0444, _name##_show, NULL)
 
-#define ALLY_DEVICE_CONST_ATTR_RO(fname, sysfs_name, value)			\
+#define ALLY_DEVICE_ATTR_WO(_name, _sysfs_name)			\
+	struct device_attribute dev_attr_##_name =			\
+		__ATTR(_sysfs_name, 0200, NULL, _name##_store)
+
+#define ALLY_DEVICE_ATTR_RW(_name, _sysfs_name)			\
+	struct device_attribute dev_attr_##_name =			\
+		__ATTR(_sysfs_name, 0644, _name##_show, _name##_store)
+
+#define ALLY_DEVICE_CONST_ATTR_RO(fname, sysfs_name, value)		\
 	static ssize_t fname##_show(struct device *dev,				\
 				   struct device_attribute *attr, char *buf)	\
 	{									\
@@ -190,6 +198,18 @@ struct asus_touchpad_info {
 	int report_size;
 };
 
+struct ally_joystick_resp_curve_param {
+	u8 move;
+	u8 resp;
+} __packed;
+
+struct ally_joystick_resp_curve {
+	struct ally_joystick_resp_curve_param entry_1;
+	struct ally_joystick_resp_curve_param entry_2;
+	struct ally_joystick_resp_curve_param entry_3;
+	struct ally_joystick_resp_curve_param entry_4;
+} __packed;
+
 struct ally_config {
 	/* Must be locked if the data is being changed */
 	struct mutex config_mutex;
@@ -223,6 +243,9 @@ struct ally_config {
 	u8 vibration_intensity_left;
 	u8 vibration_intensity_right;
 	bool vibration_active;
+
+	struct ally_joystick_resp_curve left_curve;
+	struct ally_joystick_resp_curve right_curve;
 };
 
 struct ally_handheld {
@@ -1584,6 +1607,319 @@ static struct device_attribute dev_attr_right_trigger_range_upper_limit =
 static struct device_attribute dev_attr_right_trigger_range_upper_limit_range =
 	__ATTR(range_upper_limit_range, 0444, right_trigger_range_upper_limit_range_show, NULL);
 
+enum ally_joystick_side {
+	JOYSTICK_LEFT = 0,
+	JOYSTICK_RIGHT,
+};
+
+/**
+ * ally_set_joystick_resp_curve - Set joystick response curve parameters
+ * @hdev: HID device
+ * @side: Which joystick side (0=left, 1=right)
+ * @curve: Response curve parameter structure
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int ally_set_joystick_resp_curve(struct hid_device *hdev, enum ally_joystick_side side,
+					struct ally_joystick_resp_curve *curve)
+{
+	const u8 payload[] = { side,
+		curve->entry_1.move, curve->entry_1.resp,
+		curve->entry_2.move, curve->entry_2.resp,
+		curve->entry_3.move, curve->entry_3.resp,
+		curve->entry_4.move, curve->entry_4.resp
+	};
+	int ret;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_RESP_CURVE, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int response_curve_apply(struct hid_device *hdev, bool is_left)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	struct ally_config *cfg = ally->config;
+	struct ally_joystick_resp_curve curve;
+	int ret;
+
+	/*
+	 * Snapshot under the lock so a concurrent sysfs write cannot change an
+	 * entry between the monotonicity check and the packet being built.
+	 */
+	scoped_guard(mutex, &cfg->config_mutex)
+		curve = is_left ? cfg->left_curve : cfg->right_curve;
+
+	if (!(curve.entry_1.move < curve.entry_2.move &&
+	      curve.entry_2.move < curve.entry_3.move &&
+	      curve.entry_3.move < curve.entry_4.move))
+		return -EINVAL;
+
+	ret = ally_set_joystick_resp_curve(hdev,
+					    is_left ? JOYSTICK_LEFT : JOYSTICK_RIGHT,
+					    &curve);
+	if (ret) {
+		hid_err(hdev, "Failed to set joystick response curve: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t left_response_curve_apply_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	bool apply;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	if (!ally->config->resp_curve_support)
+		return -EOPNOTSUPP;
+
+	ret = kstrtobool(buf, &apply);
+	if (ret)
+		return ret;
+
+	if (!apply)
+		return count;
+
+	ret = response_curve_apply(hdev, true);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static ssize_t right_response_curve_apply_store(struct device *dev,
+						struct device_attribute *attr,
+						const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *const ally = drvdata->rog_ally;
+	bool apply;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	if (!ally->config->resp_curve_support)
+		return -EOPNOTSUPP;
+
+	ret = kstrtobool(buf, &apply);
+	if (ret)
+		return ret;
+
+	if (!apply)
+		return count;
+
+	ret = response_curve_apply(hdev, false);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static ALLY_DEVICE_ATTR_WO(left_response_curve_apply, response_curve_apply);
+static ALLY_DEVICE_ATTR_WO(right_response_curve_apply, response_curve_apply);
+
+static ssize_t response_curve_pct_show(struct device *dev,
+				       struct device_attribute *attr, char *buf,
+				       struct ally_joystick_resp_curve *curve,
+				       int idx)
+{
+	switch (idx) {
+	case 1: return sysfs_emit(buf, "%u\n", curve->entry_1.resp);
+	case 2: return sysfs_emit(buf, "%u\n", curve->entry_2.resp);
+	case 3: return sysfs_emit(buf, "%u\n", curve->entry_3.resp);
+	case 4: return sysfs_emit(buf, "%u\n", curve->entry_4.resp);
+	default: return -EINVAL;
+	}
+}
+
+static ssize_t response_curve_move_show(struct device *dev,
+					struct device_attribute *attr, char *buf,
+					struct ally_joystick_resp_curve *curve,
+					int idx)
+{
+	switch (idx) {
+	case 1: return sysfs_emit(buf, "%u\n", curve->entry_1.move);
+	case 2: return sysfs_emit(buf, "%u\n", curve->entry_2.move);
+	case 3: return sysfs_emit(buf, "%u\n", curve->entry_3.move);
+	case 4: return sysfs_emit(buf, "%u\n", curve->entry_4.move);
+	default: return -EINVAL;
+	}
+}
+
+static ssize_t response_curve_pct_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count,
+					bool is_left,
+					struct ally_handheld *ally, int idx)
+{
+	struct ally_config *cfg = ally->config;
+	struct ally_joystick_resp_curve *curve;
+	u8 value;
+	int ret;
+
+	if (!cfg->resp_curve_support)
+		return -EOPNOTSUPP;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret)
+		return ret;
+
+	if (value > 100)
+		return -EINVAL;
+
+	curve = is_left ? &cfg->left_curve : &cfg->right_curve;
+
+	scoped_guard(mutex, &cfg->config_mutex) {
+		switch (idx) {
+		case 1:
+			curve->entry_1.resp = value;
+			break;
+		case 2:
+			curve->entry_2.resp = value;
+			break;
+		case 3:
+			curve->entry_3.resp = value;
+			break;
+		case 4:
+			curve->entry_4.resp = value;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return count;
+}
+
+static ssize_t response_curve_move_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count,
+					 bool is_left,
+					 struct ally_handheld *ally, int idx)
+{
+	struct ally_config *cfg = ally->config;
+	struct ally_joystick_resp_curve *curve;
+	u8 value;
+	int ret;
+
+	if (!cfg->resp_curve_support)
+		return -EOPNOTSUPP;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret)
+		return ret;
+
+	if (value > 100)
+		return -EINVAL;
+
+	curve = is_left ? &cfg->left_curve : &cfg->right_curve;
+
+	scoped_guard(mutex, &cfg->config_mutex) {
+		switch (idx) {
+		case 1:
+			curve->entry_1.move = value;
+			break;
+		case 2:
+			curve->entry_2.move = value;
+			break;
+		case 3:
+			curve->entry_3.move = value;
+			break;
+		case 4:
+			curve->entry_4.move = value;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return count;
+}
+
+#define DEFINE_JS_CURVE_PCT_FOPS(region, side)					\
+	static ssize_t side##_response_curve_pct_##region##_show(		\
+		struct device *dev, struct device_attribute *attr, char *buf)	\
+	{									\
+		struct hid_device *hdev = to_hid_device(dev);			\
+		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
+		struct ally_handheld *ally = drvdata->rog_ally;			\
+		return response_curve_pct_show(					\
+			dev, attr, buf, &ally->config->side##_curve, region);\
+	}									\
+										\
+	static ssize_t side##_response_curve_pct_##region##_store(		\
+		struct device *dev, struct device_attribute *attr,		\
+		const char *buf, size_t count)					\
+	{									\
+		struct hid_device *hdev = to_hid_device(dev);			\
+		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
+		struct ally_handheld *ally = drvdata->rog_ally;			\
+		return response_curve_pct_store(dev, attr, buf, count,		\
+						side##_is_left, ally, region);	\
+	}
+
+#define DEFINE_JS_CURVE_MOVE_FOPS(region, side)					\
+	static ssize_t side##_response_curve_move_##region##_show(		\
+		struct device *dev, struct device_attribute *attr, char *buf)	\
+	{									\
+		struct hid_device *hdev = to_hid_device(dev);			\
+		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
+		struct ally_handheld *ally = drvdata->rog_ally;			\
+		return response_curve_move_show(					\
+			dev, attr, buf, &ally->config->side##_curve, region);\
+	}									\
+										\
+	static ssize_t side##_response_curve_move_##region##_store(		\
+		struct device *dev, struct device_attribute *attr,		\
+		const char *buf, size_t count)					\
+	{									\
+		struct hid_device *hdev = to_hid_device(dev);			\
+		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
+		struct ally_handheld *ally = drvdata->rog_ally;			\
+		return response_curve_move_store(dev, attr, buf, count,	        \
+						 side##_is_left, ally, region); \
+	}
+
+#define DEFINE_JS_CURVE_ATTRS(region, side)					\
+	DEFINE_JS_CURVE_PCT_FOPS(region, side)					\
+	DEFINE_JS_CURVE_MOVE_FOPS(region, side)					\
+	static ALLY_DEVICE_ATTR_RW(side##_response_curve_pct_##region,		\
+				   response_curve_pct_##region);		\
+	static ALLY_DEVICE_ATTR_RW(side##_response_curve_move_##region,		\
+				   response_curve_move_##region)
+
+/* Helper defines for "is_left" parameter in DEFINE_JS_CURVE_ATTRS macros */
+#define left_is_left true
+#define right_is_left false
+
+DEFINE_JS_CURVE_ATTRS(1, left);
+DEFINE_JS_CURVE_ATTRS(2, left);
+DEFINE_JS_CURVE_ATTRS(3, left);
+DEFINE_JS_CURVE_ATTRS(4, left);
+
+DEFINE_JS_CURVE_ATTRS(1, right);
+DEFINE_JS_CURVE_ATTRS(2, right);
+DEFINE_JS_CURVE_ATTRS(3, right);
+DEFINE_JS_CURVE_ATTRS(4, right);
+
 static struct attribute *ally_config_attrs[] = {
 	&dev_attr_xbox_controller.attr,
 	NULL
@@ -1608,6 +1944,15 @@ static struct attribute *left_joystick_axis_attrs[] = {
 	&dev_attr_left_joystick_outer_threshold_range.attr,
 	&dev_attr_left_joystick_anti_deadzone.attr,
 	&dev_attr_left_joystick_anti_deadzone_range.attr,
+	&dev_attr_left_response_curve_pct_1.attr,
+	&dev_attr_left_response_curve_pct_2.attr,
+	&dev_attr_left_response_curve_pct_3.attr,
+	&dev_attr_left_response_curve_pct_4.attr,
+	&dev_attr_left_response_curve_move_1.attr,
+	&dev_attr_left_response_curve_move_2.attr,
+	&dev_attr_left_response_curve_move_3.attr,
+	&dev_attr_left_response_curve_move_4.attr,
+	&dev_attr_left_response_curve_apply.attr,
 	NULL
 };
 
@@ -1618,6 +1963,15 @@ static struct attribute *right_joystick_axis_attrs[] = {
 	&dev_attr_right_joystick_outer_threshold_range.attr,
 	&dev_attr_right_joystick_anti_deadzone.attr,
 	&dev_attr_right_joystick_anti_deadzone_range.attr,
+	&dev_attr_right_response_curve_pct_1.attr,
+	&dev_attr_right_response_curve_pct_2.attr,
+	&dev_attr_right_response_curve_pct_3.attr,
+	&dev_attr_right_response_curve_pct_4.attr,
+	&dev_attr_right_response_curve_move_1.attr,
+	&dev_attr_right_response_curve_move_2.attr,
+	&dev_attr_right_response_curve_move_3.attr,
+	&dev_attr_right_response_curve_move_4.attr,
+	&dev_attr_right_response_curve_apply.attr,
 	NULL
 };
 
@@ -1711,6 +2065,25 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
 	cfg->vibration_intensity_right = 100;
 	cfg->vibration_active = false;
 
+	/* Initialize default response curve values (linear) */
+	cfg->left_curve.entry_1.move = 0;
+	cfg->left_curve.entry_1.resp = 0;
+	cfg->left_curve.entry_2.move = 33;
+	cfg->left_curve.entry_2.resp = 33;
+	cfg->left_curve.entry_3.move = 66;
+	cfg->left_curve.entry_3.resp = 66;
+	cfg->left_curve.entry_4.move = 100;
+	cfg->left_curve.entry_4.resp = 100;
+
+	cfg->right_curve.entry_1.move = 0;
+	cfg->right_curve.entry_1.resp = 0;
+	cfg->right_curve.entry_2.move = 33;
+	cfg->right_curve.entry_2.resp = 33;
+	cfg->right_curve.entry_3.move = 66;
+	cfg->right_curve.entry_3.resp = 66;
+	cfg->right_curve.entry_4.move = 100;
+	cfg->right_curve.entry_4.resp = 100;
+
 	/* So far the only hardware this is supported is the Ally 1 */
 	if (cfg->xbox_controller_support) {
 		ret = ally_set_xbox_controller(hdev, cfg, true);
-- 
2.47.3

Re: [PATCH 08/12] HID: asus: add support for response curve

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing NULL pointer checks in sysfs callbacks cause a reliable kernel panic during device probe (NULL pointer dereference).
--

--- Patch [8]: [PATCH 08/12] HID: asus: add support for response curve ---
commit 6ba0f4853fce7434b3f1730f265395260f85991d
Author: Denis Benato <denis.benato@linux.dev>
Subject: HID: asus: add support for response curve

This commit adds support for configuring the joystick response curves on ROG
ally devices. It introduces several sysfs attributes that allow userspace to
modify the response curve parameters.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8ed10c5b22ab5..8018e61cf0264 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1584,6 +1607,319 @@ static struct device_attribute dev_attr_right_trigger_range_upper_limit =
>  static struct device_attribute dev_attr_right_trigger_range_upper_limit_range =
>  	__ATTR(range_upper_limit_range, 0444, right_trigger_range_upper_limit_range_show, NULL);
>  
> +enum ally_joystick_side {
> +	JOYSTICK_LEFT = 0,
> +	JOYSTICK_RIGHT,
> +};
> +
[ ... ]
> +static int response_curve_apply(struct hid_device *hdev, bool is_left)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	struct ally_config *cfg = ally->config;

[Severity: Critical]
Could ally->config become NULL here if the device is concurrently unbound?

The check for !ally || !ally->config happens in
left_response_curve_apply_store and right_response_curve_apply_store before
calling this function, but it is performed outside of any lock. Could this
lead to a NULL pointer dereference if the state changes in between?

> +	struct ally_joystick_resp_curve curve;
> +	int ret;
> +
> +	/*
> +	 * Snapshot under the lock so a concurrent sysfs write cannot change an
> +	 * entry between the monotonicity check and the packet being built.
> +	 */
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		curve = is_left ? cfg->left_curve : cfg->right_curve;
> +
[ ... ]
> +#define DEFINE_JS_CURVE_PCT_FOPS(region, side)					\
> +	static ssize_t side##_response_curve_pct_##region##_show(		\
> +		struct device *dev, struct device_attribute *attr, char *buf)	\
> +	{									\
> +		struct hid_device *hdev = to_hid_device(dev);			\
> +		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
> +		struct ally_handheld *ally = drvdata->rog_ally;			\
> +		return response_curve_pct_show(					\
> +			dev, attr, buf, &ally->config->side##_curve, region);\
> +	}									\

[Severity: Critical]
Can ally or ally->config be NULL here when userspace (like udev) reads
these sysfs attributes during device probe?

If they are accessed before drvdata->rog_ally is fully populated, would
&ally->config->side##_curve cause a NULL pointer dereference? The older
sysfs attributes appear to include an if (!ally || !ally->config) check
before dereferencing these.

> +										\
> +	static ssize_t side##_response_curve_pct_##region##_store(		\
> +		struct device *dev, struct device_attribute *attr,		\
> +		const char *buf, size_t count)					\
> +	{									\
> +		struct hid_device *hdev = to_hid_device(dev);			\
> +		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
> +		struct ally_handheld *ally = drvdata->rog_ally;			\
> +		return response_curve_pct_store(dev, attr, buf, count,		\
> +						side##_is_left, ally, region);	\
> +	}
> +
> +#define DEFINE_JS_CURVE_MOVE_FOPS(region, side)					\
> +	static ssize_t side##_response_curve_move_##region##_show(		\
> +		struct device *dev, struct device_attribute *attr, char *buf)	\
> +	{									\
> +		struct hid_device *hdev = to_hid_device(dev);			\
> +		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
> +		struct ally_handheld *ally = drvdata->rog_ally;			\
> +		return response_curve_move_show(					\
> +			dev, attr, buf, &ally->config->side##_curve, region);\
> +	}									\

[Severity: Critical]
Does this also need a check to prevent a NULL pointer dereference if ally or
ally->config are accessed during probe?

> +										\
> +	static ssize_t side##_response_curve_move_##region##_store(		\
> +		struct device *dev, struct device_attribute *attr,		\
> +		const char *buf, size_t count)					\
> +	{									\
> +		struct hid_device *hdev = to_hid_device(dev);			\
> +		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
> +		struct ally_handheld *ally = drvdata->rog_ally;			\
> +		return response_curve_move_store(dev, attr, buf, count,	        \
> +						 side##_is_left, ally, region); \
> +	}

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

[PATCH 09/12] HID: asus: add support to force feedback

[Denis Benato]

Unlike ROG ally the X version and following ones uses DInput protocol
and the force feedback needs to be implemented as its protocol is
vendor-specific, therefore add support for FF_RUMBLE with magnitude
scaling on a work-queue based approach to avoid using possibly
sleeping calls in atomic context.

Assisted-by: VSCode:gpt-5.3-codex
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
Signed-off-by: Luke Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c | 94 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8018e61cf026..301fe33f66b1 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -248,6 +248,23 @@ struct ally_config {
 	struct ally_joystick_resp_curve right_curve;
 };
 
+/* XInput force-feedback report (output report 0x0d, gamepad interface) */
+struct ff_data {
+	u8 enable;
+	u8 magnitude_left;
+	u8 magnitude_right;
+	u8 magnitude_strong;
+	u8 magnitude_weak;
+	u8 pulse_sustain_10ms;
+	u8 pulse_release_10ms;
+	u8 loop_count;
+} __packed;
+
+struct ff_report {
+	u8 report_id;
+	struct ff_data ff;
+} __packed;
+
 struct ally_handheld {
 	/* All read/write to IN interfaces must lock */
 	struct mutex intf_mutex;
@@ -256,6 +273,13 @@ struct ally_handheld {
 	struct input_dev *ally_x_input;
 	struct hid_device *ally_x_hdev;
 
+	struct ff_report ff_packet;
+	struct work_struct ff_work;
+	/* Serializes ff_packet and update_ff between play_effect and ff_work */
+	spinlock_t ff_lock;
+	bool ff_work_initialized;
+	bool update_ff;
+
 	struct hid_device *keyboard_hdev;
 	struct input_dev *keyboard_input;
 
@@ -370,9 +394,13 @@ enum ally_command_codes {
 	CMD_SET_ANTI_DEADZONE           = 0x18,
 };
 
+/* XInput rumble magnitudes use the hardware's 0..100 intensity range. */
+#define ALLY_FF_MAX_INTENSITY 100
+
 static const u8 ALLY_FORCE_FEEDBACK_OFF[] = {
 	0x0D, 0x0F, 0x00, 0x00, 0x00, 0x00, 0xFF, 0x00, 0xEB
 };
+static_assert(sizeof(struct ff_report) == sizeof(ALLY_FORCE_FEEDBACK_OFF));
 
 /*
  * The ROG Ally device presents multiple USB interfaces (keyboard, mouse, gamepad,
@@ -2254,6 +2282,59 @@ static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev,
 	return true;
 }
 
+static void ally_x_ff_work_fn(struct work_struct *work)
+{
+	struct ally_handheld *ally =
+		container_of(work, struct ally_handheld, ff_work);
+	struct hid_device *hdev = NULL;
+	struct ff_report report;
+	bool update = false;
+	int ret;
+
+	scoped_guard(spinlock_irqsave, &ally->ff_lock) {
+		if (ally->update_ff) {
+			report = ally->ff_packet;
+			ally->update_ff = false;
+			update = true;
+			hdev = ally->ally_x_hdev;
+		}
+	}
+
+	if (!update || !hdev)
+		return;
+
+	ret = ally_gamepad_send_packet(ally, hdev, (u8 *)&report, sizeof(report));
+	if (ret < 0)
+		hid_err(hdev, "Failed to send force-feedback: %d\n", ret);
+}
+
+static int ally_x_play_effect(struct input_dev *idev, void *data,
+			     struct ff_effect *effect)
+{
+	struct ally_handheld *ally = &ally_drvdata;
+
+	if (effect->type != FF_RUMBLE)
+		return 0;
+
+	/*
+	 * Both the flag and the queueing must happen under ff_lock: removal
+	 * clears the flag under the same lock before cancel_work_sync(), so an
+	 * unlocked test here could queue work again after the cancel.
+	 */
+	scoped_guard(spinlock_irqsave, &ally->ff_lock) {
+		ally->ff_packet.ff.magnitude_strong =
+			effect->u.rumble.strong_magnitude * ALLY_FF_MAX_INTENSITY / 65535;
+		ally->ff_packet.ff.magnitude_weak =
+			effect->u.rumble.weak_magnitude * ALLY_FF_MAX_INTENSITY / 65535;
+		ally->update_ff = true;
+
+		if (ally->ff_work_initialized)
+			schedule_work(&ally->ff_work);
+	}
+
+	return 0;
+}
+
 static struct input_dev *ally_x_alloc_input_dev(struct hid_device *hdev)
 {
 	struct input_dev *input_dev = devm_input_allocate_device(&hdev->dev);
@@ -2308,6 +2389,16 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
 	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY);
 	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);
 
+	memcpy(&ally->ff_packet, ALLY_FORCE_FEEDBACK_OFF, sizeof(ally->ff_packet));
+	spin_lock_init(&ally->ff_lock);
+	INIT_WORK(&ally->ff_work, ally_x_ff_work_fn);
+	ally->ff_work_initialized = true;
+
+	input_set_capability(input, EV_FF, FF_RUMBLE);
+	ret = input_ff_create_memless(input, NULL, ally_x_play_effect);
+	if (ret)
+		hid_warn(hdev, "Failed to create force-feedback: %d\n", ret);
+
 	ret = input_register_device(input);
 	if (ret) {
 		hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);
@@ -2460,6 +2551,9 @@ static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *
 
 	scoped_guard(mutex, &ally_data_mutex) {
 		if (ally->ally_x_hdev == hdev) {
+			scoped_guard(spinlock_irqsave, &ally->ff_lock)
+				ally->ff_work_initialized = false;
+			cancel_work_sync(&ally->ff_work);
 			ally->ally_x_input = NULL;
 			ally->ally_x_hdev = NULL;
 		}
-- 
2.47.3

Re: [PATCH 09/12] HID: asus: add support to force feedback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Ignoring the return value of `input_ff_create_memless` and proceeding with `input_register_device` leads to a guaranteed NULL pointer dereference in `input_ff_upload()`.
- [High] Disabling the force-feedback workqueue in `hid_asus_ally_remove()` before stopping the hardware leaves the device actively rumbling.
- [Medium] Force feedback output report is incorrectly sent as a Feature report, and during initialization it is sent to the wrong USB interface.
--

--- Patch [9]: [PATCH 09/12] HID: asus: add support to force feedback ---
commit 84e3caa9147b2fcf6cc7ac8459e2b9e55841248a
Author: Denis Benato <denis.benato@linux.dev>
Subject: HID: asus: add support to force feedback

This commit adds force feedback (FF_RUMBLE) support for the ROG Ally X by 
implementing a work-queue based approach for sending vendor-specific DInput 
protocol rumble packets.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8018e61cf0264..301fe33f66b1f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2254,6 +2282,59 @@ static bool ally_x_raw_event(struct input_dev *input, struct hid_device *hdev,
>  	return true;
>  }
>  
> +static void ally_x_ff_work_fn(struct work_struct *work)
> +{

[ ... ]

> +	if (!update || !hdev)
> +		return;
> +
> +	ret = ally_gamepad_send_packet(ally, hdev, (u8 *)&report, sizeof(report));

[Severity: Medium]
Does ally_gamepad_send_packet() send this data as a Feature report instead 
of an Output report?

The ff_data struct is documented as an XInput force-feedback output report 
(0x0d), but if ally_gamepad_send_packet() internally hardcodes the transfer as 
a HID_FEATURE_REPORT, will the hardware reject this packet?

> +	if (ret < 0)
> +		hid_err(hdev, "Failed to send force-feedback: %d\n", ret);
> +}

[ ... ]

> @@ -2308,6 +2389,16 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
>  	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY);
>  	input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);
>  
> +	memcpy(&ally->ff_packet, ALLY_FORCE_FEEDBACK_OFF, sizeof(ally->ff_packet));
> +	spin_lock_init(&ally->ff_lock);
> +	INIT_WORK(&ally->ff_work, ally_x_ff_work_fn);
> +	ally->ff_work_initialized = true;
> +
> +	input_set_capability(input, EV_FF, FF_RUMBLE);
> +	ret = input_ff_create_memless(input, NULL, ally_x_play_effect);
> +	if (ret)
> +		hid_warn(hdev, "Failed to create force-feedback: %d\n", ret);
> +
>  	ret = input_register_device(input);

[Severity: High]
If input_ff_create_memless() fails, will this leave dev->ff as NULL while 
EV_FF is still advertised?

Since the warning is printed but input_register_device() is still called on 
failure, could this cause a NULL pointer dereference in input_ff_upload() 
when userspace invokes the EVIOCSFF ioctl on the registered device?

>  	if (ret) {
>  		hid_err(hdev, "Failed to register Ally X gamepad device: %d\n", ret);

[ ... ]

> @@ -2460,6 +2551,9 @@ static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *
>  
>  	scoped_guard(mutex, &ally_data_mutex) {
>  		if (ally->ally_x_hdev == hdev) {
> +			scoped_guard(spinlock_irqsave, &ally->ff_lock)
> +				ally->ff_work_initialized = false;
> +			cancel_work_sync(&ally->ff_work);

[Severity: High]
Will this leave the device vibrating indefinitely if it is unbound while 
rumbling?

By setting ff_work_initialized to false and synchronously canceling the 
workqueue here, before the devres action has a chance to unregister the 
input device, won't the input subsystem's subsequent stop commands be 
silently discarded by the disabled workqueue?

>  			ally->ally_x_input = NULL;
>  			ally->ally_x_hdev = NULL;
>  		}

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

[PATCH 10/12] HID: asus: add support for gamepad mode

[Denis Benato]

ROG Ally devices can emulate either a mouse+keyboard (desktop mode)
or an gamepad device (xbox360 controller in ROG ally and a custom
DInput device on newer models): add support for switching the current
controller mode.

Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 153 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 301fe33f66b1..8930a246e725 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -397,6 +397,20 @@ enum ally_command_codes {
 /* XInput rumble magnitudes use the hardware's 0..100 intensity range. */
 #define ALLY_FF_MAX_INTENSITY 100
 
+enum ally_gamepad_mode_index {
+	ALLY_GAMEPAD_MODE_GAMEPAD = 0x01,
+	ALLY_GAMEPAD_MODE_KEYBOARD = 0x02,
+};
+
+static const char *const ally_gamepad_mode_text[] = {
+	"gamepad", "desktop"
+};
+
+static const u8 ally_gamepad_mode[] = {
+	ALLY_GAMEPAD_MODE_GAMEPAD,
+	ALLY_GAMEPAD_MODE_KEYBOARD
+};
+
 static const u8 ALLY_FORCE_FEEDBACK_OFF[] = {
 	0x0D, 0x0F, 0x00, 0x00, 0x00, 0x00, 0xFF, 0x00, 0xEB
 };
@@ -793,6 +807,136 @@ static ssize_t xbox_controller_store(struct device *dev,
 
 static DEVICE_ATTR_RW(xbox_controller);
 
+/**
+ * ally_set_gamepad_mode - Set the gamepad operating mode
+ * @ally: ally handheld structure
+ * @hdev: HID device
+ * @mode: Gamepad mode to set
+ *
+ * Returns: 0 on success, negative on failure
+ */
+static int ally_set_gamepad_mode(struct ally_handheld *ally, struct hid_device *hdev, u8 mode)
+{
+	struct ally_config *cfg = ally->config;
+	u8 payload[] = { mode };
+	int ret;
+
+	if (!cfg)
+		return -EINVAL;
+
+	if (mode < ALLY_GAMEPAD_MODE_GAMEPAD ||
+	    mode > ALLY_GAMEPAD_MODE_KEYBOARD) {
+		hid_err(hdev, "Invalid gamepad mode: %u\n", mode);
+		return -EINVAL;
+	}
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_GAMEPAD_MODE, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set gamepad mode: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t gamepad_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+	u8 mode_byte;
+	int i;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+	mode_byte = cfg->gamepad_mode;
+
+	for (i = 0; i < ARRAY_SIZE(ally_gamepad_mode); i++) {
+		if (ally_gamepad_mode[i] == mode_byte)
+			return sysfs_emit(buf, "%s\n", ally_gamepad_mode_text[i]);
+	}
+
+	return sysfs_emit(buf, "unsupported\n");
+}
+
+static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_config *cfg;
+	u8 mode_byte;
+	int mode;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	mode = sysfs_match_string(ally_gamepad_mode_text, buf);
+	if (mode < 0) {
+		hid_err(hdev, "Unknown gamepad mode\n");
+		return mode;
+	}
+
+	/* Convert the index of the text mode array to the byte
+	 * that will be accepted by the ally MCU.
+	 */
+	mode_byte = ally_gamepad_mode[mode];
+
+	ret = ally_set_gamepad_mode(ally, hdev, mode_byte);
+	if (ret < 0)
+		return ret;
+
+	scoped_guard(mutex, &cfg->config_mutex)
+		cfg->gamepad_mode = mode_byte;
+
+	hid_dbg(hdev, "Set gamepad mode to %s\n", ally_gamepad_mode_text[mode]);
+
+	return count;
+}
+
+static ssize_t gamepad_mode_index_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	int i;
+	ssize_t len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(ally_gamepad_mode_text); i++) {
+		if (!ally_gamepad_mode_text[i] || ally_gamepad_mode_text[i][0] == '\0')
+			continue;
+		len += sysfs_emit_at(buf, len, "%s ", ally_gamepad_mode_text[i]);
+	}
+
+	/* Replace the last space with a newline */
+	if (len > 0)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static DEVICE_ATTR_RW(gamepad_mode);
+static DEVICE_ATTR_RO(gamepad_mode_index);
+
+static int ally_set_default_gamepad_mode(struct hid_device *hdev,
+					 struct ally_handheld *ally,
+					 struct ally_config *cfg)
+{
+	cfg->gamepad_mode = ALLY_GAMEPAD_MODE_GAMEPAD;
+
+	return ally_set_gamepad_mode(ally, hdev, cfg->gamepad_mode);
+}
+
 /**
  * ally_set_vibration_intensity() - Set vibration intensity values
  * @hdev: HID device
@@ -1950,6 +2094,8 @@ DEFINE_JS_CURVE_ATTRS(4, right);
 
 static struct attribute *ally_config_attrs[] = {
 	&dev_attr_xbox_controller.attr,
+	&dev_attr_gamepad_mode.attr,
+	&dev_attr_gamepad_mode_index.attr,
 	NULL
 };
 
@@ -2422,6 +2568,13 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
 	if (ret < 0)
 		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
 
+	/* Set the default gamepad mode now that the MCU is confirmed ready */
+	if (ally->config) {
+		ret = ally_set_default_gamepad_mode(hdev, ally, ally->config);
+		if (ret < 0)
+			hid_warn(hdev, "Failed to set default gamepad mode: %d\n", ret);
+	}
+
 	return 0;
 }
 
-- 
2.47.3

Re: [PATCH 10/12] HID: asus: add support for gamepad mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing `hid_asus_ally_remove()` in `asus_probe()` error path leads to Use-After-Free.
- [High] The dynamically allocated `cfg->config_mutex` is never initialized.
- [Medium] User-configured gamepad mode is unconditionally overwritten and lost during device reset/resume.
- [Medium] Hardware state desynchronization due to missing lock coverage over hardware I/O in `gamepad_mode_store`.
- [Medium] Lockless modification of shared state `cfg->gamepad_mode` in `ally_set_default_gamepad_mode()`.
--

--- Patch [10]: [PATCH 10/12] HID: asus: add support for gamepad mode ---
commit cfd28807a39c51dcfa8825ce30540ae3214394d4
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for gamepad mode

ROG Ally devices can emulate either a mouse+keyboard (desktop mode)
or an gamepad device (xbox360 controller in ROG ally and a custom
DInput device on newer models): add support for switching the current
controller mode.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 301fe33f66b1f..8930a246e7255 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -793,6 +807,136 @@ static ssize_t xbox_controller_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(xbox_controller);
>  
> +/**
> + * ally_set_gamepad_mode - Set the gamepad operating mode

[ ... ]

> +static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{

[ ... ]

> +	/* Convert the index of the text mode array to the byte
> +	 * that will be accepted by the ally MCU.
> +	 */
> +	mode_byte = ally_gamepad_mode[mode];
> +
> +	ret = ally_set_gamepad_mode(ally, hdev, mode_byte);
> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		cfg->gamepad_mode = mode_byte;

[Severity: Medium]
Can the hardware state become desynchronized from the software state here?

The hardware request ally_set_gamepad_mode() executes before acquiring
cfg->config_mutex, while the internal software state cfg->gamepad_mode
is updated only after the lock is acquired. 

If two threads write to the sysfs attribute concurrently, might their
hardware I/O requests interleave with the mutex-protected state updates,
resulting in the hardware being set to one mode while the driver records
the other?

[Severity: High]
Is cfg->config_mutex properly initialized before it is used here?

Looking at ally_config_create(), the structure is allocated and
zero-initialized:

drivers/hid/hid-asus.c:ally_config_create() {
    ...
	cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
	if (!cfg)
		return ERR_PTR(-ENOMEM);
    ...
}

However, mutex_init() is never called for cfg->config_mutex.

A zero-initialized mutex lacks proper lockdep linkage and wait_list
initialization. 

Could this trigger lockdep warnings or kernel crashes when scoped_guard()
is called?

> +
> +	hid_dbg(hdev, "Set gamepad mode to %s\n", ally_gamepad_mode_text[mode]);
> +
> +	return count;
> +}
> +

[ ... ]

> +static DEVICE_ATTR_RW(gamepad_mode);
> +static DEVICE_ATTR_RO(gamepad_mode_index);
> +
> +static int ally_set_default_gamepad_mode(struct hid_device *hdev,
> +					 struct ally_handheld *ally,
> +					 struct ally_config *cfg)
> +{
> +	cfg->gamepad_mode = ALLY_GAMEPAD_MODE_GAMEPAD;
> +
> +	return ally_set_gamepad_mode(ally, hdev, cfg->gamepad_mode);

[Severity: Medium]
Is it safe to modify cfg->gamepad_mode directly without holding
cfg->config_mutex?

This function is called from hid_asus_ally_init() after ally_config_create()
has already successfully registered the sysfs group. 

Since the sysfs attributes are active and exposed, could a concurrent write
from userspace interleave with this initialization?

> +}
> +
>  /**
>   * ally_set_vibration_intensity() - Set vibration intensity values

[ ... ]

> @@ -2422,6 +2568,13 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>  	if (ret < 0)
>  		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
>  
> +	/* Set the default gamepad mode now that the MCU is confirmed ready */
> +	if (ally->config) {
> +		ret = ally_set_default_gamepad_mode(hdev, ally, ally->config);
> +		if (ret < 0)
> +			hid_warn(hdev, "Failed to set default gamepad mode: %d\n", ret);
> +	}
> +

[Severity: Medium]
Will this unconditionally overwrite the user-configured gamepad mode?

During a USB reset resume, hid_asus_ally_init() is called and invokes
ally_set_default_gamepad_mode(), which sets the state to
ALLY_GAMEPAD_MODE_GAMEPAD and writes it to the hardware. 

Does this actively destroy the user's previously selected mode across
suspend/resume or device reset cycles?

>  	return 0;
>  }

[Severity: Critical]
Does the error path in asus_probe() leave a dangling pointer to freed memory?

If asus_probe() fails after hid_asus_ally_probe() succeeds, the driver
jumps to the err_stop_hw error path:

drivers/hid/hid-asus.c:asus_probe() {
    ...
err_stop_hw:
	if (drvdata->listener.brightness_set)
		asus_hid_unregister_listener(&drvdata->listener);

	asus_worker_stop(drvdata->worker);
	hid_hw_stop(hdev);
	return ret;
}

This path cleans up core resources but fails to tear down the ally
sub-driver state. 

The driver core's devres mechanism automatically frees ally_cfg, but the 
static global ally_drvdata.config retains a dangling pointer to this freed
memory.

Could this lead to a use-after-free if accessed by a subsequent interface
probe or a delayed work?

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

[PATCH 11/12] HID: asus: add support for turbo buttons

[Denis Benato]

ROG ally devices supports a feature called turbo buttons that allows
the user to quickly toggle button state by keeping a button pressed,
therefore allow userspace to configure the feature by exposing
relevant sysfs attributes.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
---
 drivers/hid/hid-asus.c | 421 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 421 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8930a246e725..359832395afe 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -210,6 +210,96 @@ struct ally_joystick_resp_curve {
 	struct ally_joystick_resp_curve_param entry_4;
 } __packed;
 
+/* Button identifiers for the turbo attribute system */
+enum ally_button_id {
+	ALLY_BTN_A,
+	ALLY_BTN_B,
+	ALLY_BTN_X,
+	ALLY_BTN_Y,
+	ALLY_BTN_LB,
+	ALLY_BTN_RB,
+	ALLY_BTN_DU,
+	ALLY_BTN_DD,
+	ALLY_BTN_DL,
+	ALLY_BTN_DR,
+	ALLY_BTN_J0B,
+	ALLY_BTN_J1B,
+	ALLY_BTN_MENU,
+	ALLY_BTN_VIEW,
+	ALLY_BTN_M1,
+	ALLY_BTN_M2,
+	ALLY_BTN_MAX
+};
+
+/* Names for the button directories in sysfs */
+static const char *const ally_button_names[ALLY_BTN_MAX] = {
+	[ALLY_BTN_A] = "btn_a",
+	[ALLY_BTN_B] = "btn_b",
+	[ALLY_BTN_X] = "btn_x",
+	[ALLY_BTN_Y] = "btn_y",
+	[ALLY_BTN_LB] = "btn_lb",
+	[ALLY_BTN_RB] = "btn_rb",
+	[ALLY_BTN_DU] = "dpad_up",
+	[ALLY_BTN_DD] = "dpad_down",
+	[ALLY_BTN_DL] = "dpad_left",
+	[ALLY_BTN_DR] = "dpad_right",
+	[ALLY_BTN_J0B] = "btn_l3",
+	[ALLY_BTN_J1B] = "btn_r3",
+	[ALLY_BTN_MENU] = "btn_menu",
+	[ALLY_BTN_VIEW] = "btn_view",
+	[ALLY_BTN_M1] = "btn_m1",
+	[ALLY_BTN_M2] = "btn_m2",
+};
+
+/*
+ * Button turbo parameters structure
+ * Each button can have:
+ * - turbo: Turbo press interval in multiples of 50ms (0 = disabled, 1-20 = 50ms-1000ms)
+ * - toggle: Toggle interval (0 = disabled)
+ */
+struct ally_btn_turbo_params {
+	u8 turbo;
+	u8 toggle;
+} __packed;
+
+#define ALLY_TURBO_PERIOD_MIN 0
+#define ALLY_TURBO_PERIOD_MAX 20
+#define ALLY_TOGGLE_PERIOD_MIN 0
+#define ALLY_TOGGLE_PERIOD_MAX 255
+
+/* Collection of all button turbo settings */
+struct ally_turbo_config {
+	struct ally_btn_turbo_params btn_du;
+	struct ally_btn_turbo_params btn_dd;
+	struct ally_btn_turbo_params btn_dl;
+	struct ally_btn_turbo_params btn_dr;
+	struct ally_btn_turbo_params btn_j0b;
+	struct ally_btn_turbo_params btn_j1b;
+	struct ally_btn_turbo_params btn_lb;
+	struct ally_btn_turbo_params btn_rb;
+	struct ally_btn_turbo_params btn_a;
+	struct ally_btn_turbo_params btn_b;
+	struct ally_btn_turbo_params btn_x;
+	struct ally_btn_turbo_params btn_y;
+	struct ally_btn_turbo_params btn_view;
+	struct ally_btn_turbo_params btn_menu;
+	struct ally_btn_turbo_params btn_m2;
+	struct ally_btn_turbo_params btn_m1;
+};
+
+struct ally_btn_turbo_attr;
+
+struct ally_btn_sysfs_entry {
+	struct attribute_group group;
+	struct attribute *attrs[5]; /* turbo_period + toggle_period + ranges + NULL */
+	struct ally_config *cfg;
+	struct hid_device *hdev;
+	enum ally_button_id btn;
+	struct device_attribute attr_turbo_period;
+	struct device_attribute attr_toggle_period;
+	struct ally_btn_turbo_attr *turbo_attr;
+};
+
 struct ally_config {
 	/* Must be locked if the data is being changed */
 	struct mutex config_mutex;
@@ -244,6 +334,9 @@ struct ally_config {
 	u8 vibration_intensity_right;
 	bool vibration_active;
 
+	struct ally_turbo_config turbo;
+	struct ally_btn_sysfs_entry *button_entries;
+
 	struct ally_joystick_resp_curve left_curve;
 	struct ally_joystick_resp_curve right_curve;
 };
@@ -2195,6 +2288,321 @@ static const struct attribute_group ally_attr_groups[] = {
 	},
 };
 
+/**
+ * ally_set_turbo_params - Set turbo parameters for all buttons
+ * @hdev: HID device
+ * @cfg: Ally config structure
+ *
+ * Returns: 0 on success, negative on failure
+ */
+static int ally_set_turbo_params(struct hid_device *hdev, struct ally_config *cfg)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct ally_turbo_config *turbo = &cfg->turbo;
+	const u8 payload[] = {
+		turbo->btn_du.turbo,
+		turbo->btn_du.toggle,
+		turbo->btn_dd.turbo,
+		turbo->btn_dd.toggle,
+		turbo->btn_dl.turbo,
+		turbo->btn_dl.toggle,
+		turbo->btn_dr.turbo,
+		turbo->btn_dr.toggle,
+		turbo->btn_j0b.turbo,
+		turbo->btn_j0b.toggle,
+		turbo->btn_j1b.turbo,
+		turbo->btn_j1b.toggle,
+		turbo->btn_lb.turbo,
+		turbo->btn_lb.toggle,
+		turbo->btn_rb.turbo,
+		turbo->btn_rb.toggle,
+		turbo->btn_a.turbo,
+		turbo->btn_a.toggle,
+		turbo->btn_b.turbo,
+		turbo->btn_b.toggle,
+		turbo->btn_x.turbo,
+		turbo->btn_x.toggle,
+		turbo->btn_y.turbo,
+		turbo->btn_y.toggle,
+		turbo->btn_view.turbo,
+		turbo->btn_view.toggle,
+		turbo->btn_menu.turbo,
+		turbo->btn_menu.toggle,
+		turbo->btn_m2.turbo,
+		turbo->btn_m2.toggle,
+		turbo->btn_m1.turbo,
+		turbo->btn_m1.toggle,
+	};
+	int ret;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_TURBO_PARAMS, payload, sizeof(payload));
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ally_gamepad_send_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to set turbo parameters: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+struct ally_btn_turbo_attr {
+	struct device_attribute dev_attr;
+	int button_id;
+};
+
+#define to_ally_btn_turbo_attr(x) container_of(x, struct ally_btn_turbo_attr, dev_attr)
+
+static struct ally_btn_turbo_params *ally_btn_get_turbo_params(struct ally_config *cfg,
+							       enum ally_button_id btn)
+{
+	switch (btn) {
+	case ALLY_BTN_DU: return &cfg->turbo.btn_du;
+	case ALLY_BTN_DD: return &cfg->turbo.btn_dd;
+	case ALLY_BTN_DL: return &cfg->turbo.btn_dl;
+	case ALLY_BTN_DR: return &cfg->turbo.btn_dr;
+	case ALLY_BTN_J0B: return &cfg->turbo.btn_j0b;
+	case ALLY_BTN_J1B: return &cfg->turbo.btn_j1b;
+	case ALLY_BTN_LB: return &cfg->turbo.btn_lb;
+	case ALLY_BTN_RB: return &cfg->turbo.btn_rb;
+	case ALLY_BTN_A: return &cfg->turbo.btn_a;
+	case ALLY_BTN_B: return &cfg->turbo.btn_b;
+	case ALLY_BTN_X: return &cfg->turbo.btn_x;
+	case ALLY_BTN_Y: return &cfg->turbo.btn_y;
+	case ALLY_BTN_VIEW: return &cfg->turbo.btn_view;
+	case ALLY_BTN_MENU: return &cfg->turbo.btn_menu;
+	case ALLY_BTN_M2: return &cfg->turbo.btn_m2;
+	case ALLY_BTN_M1: return &cfg->turbo.btn_m1;
+	default: return NULL;
+	}
+}
+
+static ssize_t btn_turbo_period_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
+							  attr_turbo_period);
+	struct ally_btn_turbo_params *params = ally_btn_get_turbo_params(entry->cfg,
+									 entry->btn);
+
+	if (!params)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%hhu\n", params->turbo);
+}
+
+static ssize_t btn_turbo_period_store(struct device *dev, struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
+							  attr_turbo_period);
+	struct ally_btn_turbo_params *params;
+	u8 value;
+	int ret;
+
+	if (!entry->cfg->turbo_support)
+		return -EOPNOTSUPP;
+
+	params = ally_btn_get_turbo_params(entry->cfg, entry->btn);
+	if (!params)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret)
+		return ret;
+
+	if (value < ALLY_TURBO_PERIOD_MIN || value > ALLY_TURBO_PERIOD_MAX)
+		return -EINVAL;
+
+	scoped_guard(mutex, &entry->cfg->config_mutex)
+		params->turbo = value;
+
+	ret = ally_set_turbo_params(entry->hdev, entry->cfg);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t btn_toggle_period_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
+							  attr_toggle_period);
+	struct ally_btn_turbo_params *params = ally_btn_get_turbo_params(entry->cfg, entry->btn);
+
+	if (!params)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%hhu\n", params->toggle);
+}
+
+static ssize_t btn_toggle_period_store(struct device *dev, struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
+							  attr_toggle_period);
+	struct ally_btn_turbo_params *params;
+	u8 value;
+	int ret;
+
+	if (!entry->cfg->turbo_support)
+		return -EOPNOTSUPP;
+
+	params = ally_btn_get_turbo_params(entry->cfg, entry->btn);
+	if (!params)
+		return -ENODEV;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret)
+		return ret;
+
+	if (value < ALLY_TOGGLE_PERIOD_MIN || value > ALLY_TOGGLE_PERIOD_MAX)
+		return -EINVAL;
+
+	scoped_guard(mutex, &entry->cfg->config_mutex)
+		params->toggle = value;
+
+	ret = ally_set_turbo_params(entry->hdev, entry->cfg);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+ALLY_DEVICE_CONST_ATTR_RO(btn_turbo_period_range, turbo_period_range, "0 20\n");
+ALLY_DEVICE_CONST_ATTR_RO(btn_toggle_period_range, toggle_period_range, "0 255\n");
+
+static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
+{
+	sysfs_attr_init(&entry->attr_turbo_period.attr);
+	entry->attr_turbo_period.attr.name = "turbo_period";
+	entry->attr_turbo_period.attr.mode = 0644;
+	entry->attr_turbo_period.show = btn_turbo_period_show;
+	entry->attr_turbo_period.store = btn_turbo_period_store;
+
+	sysfs_attr_init(&entry->attr_toggle_period.attr);
+	entry->attr_toggle_period.attr.name = "toggle_period";
+	entry->attr_toggle_period.attr.mode = 0644;
+	entry->attr_toggle_period.show = btn_toggle_period_show;
+	entry->attr_toggle_period.store = btn_toggle_period_store;
+}
+
+/* Helper to create button turbo attribute */
+static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device *hdev,
+							      struct ally_btn_sysfs_entry *entry)
+{
+	struct ally_btn_turbo_attr *attr __free(kfree) = kzalloc_obj(*attr);
+
+	if (!entry || !entry->cfg || !entry->cfg->turbo_support)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	if (!ally_btn_get_turbo_params(entry->cfg, entry->btn)) {
+		hid_err(hdev, "Invalid button id %d for turbo attributes\n", entry->btn);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (!attr)
+		return ERR_PTR(-ENOMEM);
+
+	ally_btn_turbo_init_attrs(entry);
+	entry->attrs[0] = &entry->attr_turbo_period.attr;
+	entry->attrs[1] = &entry->attr_toggle_period.attr;
+	entry->attrs[2] = &dev_attr_btn_turbo_period_range.attr;
+	entry->attrs[3] = &dev_attr_btn_toggle_period_range.attr;
+	entry->attrs[4] = NULL;
+
+	return no_free_ptr(attr);
+}
+
+/**
+ * ally_create_button_attributes - Create turbo button attributes
+ * @hdev: HID device
+ * @cfg: Ally config structure
+ *
+ * Returns: 0 on success, negative on failure
+ */
+static int ally_create_button_attributes(struct hid_device *hdev, struct ally_config *cfg)
+{
+	struct ally_btn_sysfs_entry *entries;
+	int i, ret;
+
+	if (!cfg->turbo_support)
+		return 0;
+
+	entries = devm_kcalloc(&hdev->dev, ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
+	cfg->button_entries = entries;
+
+	for (i = 0; i < ALLY_BTN_MAX; i++) {
+		entries[i].cfg = cfg;
+		entries[i].hdev = hdev;
+		entries[i].btn = i;
+
+		entries[i].turbo_attr = ally_btn_turbo_attr_create(hdev, &entries[i]);
+		if (IS_ERR(entries[i].turbo_attr)) {
+			ret = PTR_ERR(entries[i].turbo_attr);
+			entries[i].turbo_attr = NULL;
+			goto err_cleanup;
+		}
+
+		entries[i].group.name = ally_button_names[i];
+		entries[i].group.attrs = entries[i].attrs;
+
+		ret = sysfs_create_group(&hdev->dev.kobj, &entries[i].group);
+		if (ret < 0) {
+			hid_err(hdev, "Failed to create sysfs group for %s: %d\n",
+				ally_button_names[i], ret);
+			goto err_cleanup;
+		}
+	}
+
+	return 0;
+
+err_cleanup:
+	/* Only groups [0, i) were registered; the failure happened at i. */
+	while (--i >= 0)
+		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
+
+	for (i = 0; i < ALLY_BTN_MAX; i++) {
+		kfree(entries[i].turbo_attr);
+		entries[i].turbo_attr = NULL;
+	}
+
+	/* Nullify the entries and mappings to prevent use-after-free crashes */
+	cfg->button_entries = NULL;
+
+	return ret;
+}
+
+/**
+ * ally_remove_button_attributes - Remove turbo button attributes
+ * @hdev: HID device
+ * @cfg: Ally config structure
+ */
+static void ally_remove_button_attributes(struct hid_device *hdev, struct ally_config *cfg)
+{
+	struct ally_btn_sysfs_entry *entries;
+	int i;
+
+	if (!cfg || !cfg->button_entries)
+		return;
+
+	entries = cfg->button_entries;
+
+	for (i = 0; i < ALLY_BTN_MAX; i++) {
+		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
+		kfree(entries[i].turbo_attr);
+	}
+
+	cfg->button_entries = NULL;
+}
+
 /**
  * ally_config_create() - Initialize configuration and create sysfs entries
  * @hdev: HID device
@@ -2226,6 +2634,14 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
 		}
 	}
 
+	if (cfg->turbo_support) {
+		ret = ally_create_button_attributes(hdev, cfg);
+		if (ret < 0) {
+			hid_err(hdev, "Failed to create button attributes: %d\n", ret);
+			goto ally_config_create_sysfs_err;
+		}
+	}
+
 	cfg->gamepad_mode = 0x01;
 	cfg->left_deadzone = 10;
 	cfg->left_outer_threshold = 90;
@@ -2270,6 +2686,8 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
 
 	return cfg;
 ally_config_create_sysfs_err:
+	if (cfg->turbo_support && cfg->button_entries)
+		ally_remove_button_attributes(hdev, cfg);
 ally_config_create_err:
 	ally->config = NULL;
 	devm_kfree(&hdev->dev, cfg);
@@ -2287,6 +2705,9 @@ static void ally_config_remove(struct hid_device *hdev, struct ally_handheld *al
 
 	if (!cfg || !cfg->initialized)
 		return;
+
+	if (cfg->turbo_support && cfg->button_entries)
+		ally_remove_button_attributes(hdev, cfg);
 }
 
 /*
-- 
2.47.3

Re: [PATCH 11/12] HID: asus: add support for turbo buttons

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Lockless read of `cfg->turbo` in `ally_set_turbo_params` leads to data races and hardware state desynchronization.
- [Low] Redundant and completely unused dynamic allocation of `struct ally_btn_turbo_attr`.

Pre-existing issues:
- [High] Preexisting sysfs show/store callbacks for response curves dereference `ally->config` without checking if it is NULL, leading to a kernel crash if accessed during device unbind.
--

--- Patch [11]: [PATCH 11/12] HID: asus: add support for turbo buttons ---
commit 889fd7b803cb63ae392e549855086950d7932992
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for turbo buttons

ROG ally devices supports a feature called turbo buttons that allows
the user to quickly toggle button state by keeping a button pressed,
therefore allow userspace to configure the feature by exposing
relevant sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8930a246e7255..359832395afec 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -2195,6 +2288,321 @@ static const struct attribute_group ally_attr_groups[] = {
> +static int ally_set_turbo_params(struct hid_device *hdev, struct ally_config *cfg)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct ally_turbo_config *turbo = &cfg->turbo;
> +	const u8 payload[] = {
> +		turbo->btn_du.turbo,
> +		turbo->btn_du.toggle,

[Severity: Medium]
Can this lockless read result in stale or torn data being sent to the hardware?

In btn_turbo_period_store(), the config_mutex is released immediately before
calling ally_set_turbo_params():

    scoped_guard(mutex, &entry->cfg->config_mutex)
        params->turbo = value;

    ret = ally_set_turbo_params(entry->hdev, entry->cfg);

Since ally_set_turbo_params() locklessly reads the entire turbo array to
construct the payload, concurrent sysfs writes to other buttons could result
in the device receiving an inconsistent or desynchronized state. Could this
be prevented by holding config_mutex while building the payload?

> +		turbo->btn_dd.turbo,
> +		turbo->btn_dd.toggle,
[ ... ]
> +struct ally_btn_turbo_attr {
> +	struct device_attribute dev_attr;
> +	int button_id;
> +};

[Severity: Low]
Does this code allocate a redundant structure?

Later in ally_btn_turbo_attr_create(), this struct is allocated via
kzalloc_obj, but its fields are never populated. The actual sysfs attributes
used seem to be embedded within struct ally_btn_sysfs_entry instead.

> +
> +#define to_ally_btn_turbo_attr(x) container_of(x, struct ally_btn_turbo_attr, dev_attr)
[ ... ]


[Severity: High]
This is a pre-existing issue, but can this code cause a kernel panic if the
sysfs attribute is read during device unbind?

Looking at DEFINE_JS_CURVE_PCT_FOPS in drivers/hid/hid-asus.c:

	static ssize_t side##_response_curve_pct_##region##_show(		\
		struct device *dev, struct device_attribute *attr, char *buf)	\
	{									\
		struct hid_device *hdev = to_hid_device(dev);			\
		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
		struct ally_handheld *ally = drvdata->rog_ally;			\
		return response_curve_pct_show(					\
			dev, attr, buf, &ally->config->side##_curve, region);\
	}

When the device is unplugged, hid_asus_ally_remove() synchronously sets
ally->config = NULL. Since devres handles the sysfs removal later, a
concurrent read of the sysfs file unconditionally dereferences ally->config
without checking if it is NULL.

Should there be a NULL check before evaluating &ally->config->side_curve
in both DEFINE_JS_CURVE_PCT_FOPS and DEFINE_JS_CURVE_MOVE_FOPS?

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

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

[Denis Benato]

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.

Assisted-by: Claude:claude-fable-5
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>
---
 drivers/hid/hid-asus.c | 867 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 858 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 359832395afe..5b744dba98d3 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -288,16 +288,20 @@ struct ally_turbo_config {
 };
 
 struct ally_btn_turbo_attr;
+struct button_remap_attr;
 
 struct ally_btn_sysfs_entry {
 	struct attribute_group group;
-	struct attribute *attrs[5]; /* turbo_period + toggle_period + ranges + NULL */
+	struct attribute *attrs[8]; /* turbo + ranges + remap + macro + remap_index + NULL */
 	struct ally_config *cfg;
 	struct hid_device *hdev;
 	enum ally_button_id btn;
 	struct device_attribute attr_turbo_period;
 	struct device_attribute attr_toggle_period;
 	struct ally_btn_turbo_attr *turbo_attr;
+	struct button_remap_attr *remap_attr;
+	struct button_remap_attr *macro_attr;
+	struct device_attribute attr_remap_index;
 };
 
 struct ally_config {
@@ -336,6 +340,7 @@ struct ally_config {
 
 	struct ally_turbo_config turbo;
 	struct ally_btn_sysfs_entry *button_entries;
+	void *button_mappings; /* ally_button_mapping array indexed by gamepad_mode */
 
 	struct ally_joystick_resp_curve left_curve;
 	struct ally_joystick_resp_curve right_curve;
@@ -936,6 +941,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" },
+	{ BTN_TYPE_PAD, 0x02, "PAD_B" },
+	{ BTN_TYPE_PAD, 0x03, "PAD_X" },
+	{ BTN_TYPE_PAD, 0x04, "PAD_Y" },
+	{ BTN_TYPE_PAD, 0x05, "PAD_LB" },
+	{ BTN_TYPE_PAD, 0x06, "PAD_RB" },
+	{ BTN_TYPE_PAD, 0x07, "PAD_LS" },
+	{ BTN_TYPE_PAD, 0x08, "PAD_RS" },
+	{ BTN_TYPE_PAD, 0x09, "PAD_DPAD_UP" },
+	{ BTN_TYPE_PAD, 0x0A, "PAD_DPAD_DOWN" },
+	{ BTN_TYPE_PAD, 0x0B, "PAD_DPAD_LEFT" },
+	{ BTN_TYPE_PAD, 0x0C, "PAD_DPAD_RIGHT" },
+	{ BTN_TYPE_PAD, 0x0D, "PAD_LT" },
+	{ BTN_TYPE_PAD, 0x0E, "PAD_RT" },
+	{ BTN_TYPE_PAD, 0x11, "PAD_VIEW" },
+	{ BTN_TYPE_PAD, 0x12, "PAD_MENU" },
+	{ BTN_TYPE_PAD, 0x13, "PAD_XBOX" },
+
+	/* Keyboard button codes */
+	{ BTN_TYPE_KB, 0x8E, "FN_M2" },
+	{ BTN_TYPE_KB, 0x8F, "FN_M1" },
+	{ BTN_TYPE_KB, 0x76, "KEY_ESC" },
+	{ BTN_TYPE_KB, 0x05, "KEY_F1" },
+	{ BTN_TYPE_KB, 0x06, "KEY_F2" },
+	{ BTN_TYPE_KB, 0x04, "KEY_F3" },
+	{ BTN_TYPE_KB, 0x0C, "KEY_F4" },
+	{ BTN_TYPE_KB, 0x03, "KEY_F5" },
+	{ BTN_TYPE_KB, 0x0B, "KEY_F6" },
+	{ BTN_TYPE_KB, 0x80, "KEY_F7" },
+	{ BTN_TYPE_KB, 0x0A, "KEY_F8" },
+	{ BTN_TYPE_KB, 0x01, "KEY_F9" },
+	{ BTN_TYPE_KB, 0x09, "KEY_F10" },
+	{ BTN_TYPE_KB, 0x78, "KEY_F11" },
+	{ BTN_TYPE_KB, 0x07, "KEY_F12" },
+	{ BTN_TYPE_KB, 0x08, "KEY_F13" },
+	{ BTN_TYPE_KB, 0x10, "KEY_F14" },
+	{ BTN_TYPE_KB, 0x18, "KEY_F15" },
+	{ BTN_TYPE_KB, 0x0E, "KEY_GRAVE" }, // backtick
+	{ BTN_TYPE_KB, 0x16, "KEY_1" },
+	{ BTN_TYPE_KB, 0x1E, "KEY_2" },
+	{ BTN_TYPE_KB, 0x26, "KEY_3" },
+	{ BTN_TYPE_KB, 0x25, "KEY_4" },
+	{ BTN_TYPE_KB, 0x2E, "KEY_5" },
+	{ BTN_TYPE_KB, 0x36, "KEY_6" },
+	{ BTN_TYPE_KB, 0x3D, "KEY_7" },
+	{ BTN_TYPE_KB, 0x3E, "KEY_8" },
+	{ BTN_TYPE_KB, 0x46, "KEY_9" },
+	{ BTN_TYPE_KB, 0x45, "KEY_0" },
+	{ BTN_TYPE_KB, 0x4E, "KEY_MINUS" }, // hypen
+	{ BTN_TYPE_KB, 0x55, "KEY_EQUAL" },
+	{ BTN_TYPE_KB, 0x66, "KEY_BACKSPACE" },
+	{ BTN_TYPE_KB, 0x0D, "KEY_TAB" },
+	{ BTN_TYPE_KB, 0x15, "KEY_Q" },
+	{ BTN_TYPE_KB, 0x1D, "KEY_W" },
+	{ BTN_TYPE_KB, 0x24, "KEY_E" },
+	{ BTN_TYPE_KB, 0x2D, "KEY_R" },
+	{ BTN_TYPE_KB, 0x2C, "KEY_T" },
+	{ BTN_TYPE_KB, 0x35, "KEY_Y" },
+	{ BTN_TYPE_KB, 0x3C, "KEY_U" },
+	{ BTN_TYPE_KB, 0x43, "KEY_I" },
+	{ BTN_TYPE_KB, 0x44, "KEY_O" },
+	{ BTN_TYPE_KB, 0x4D, "KEY_P" },
+	{ BTN_TYPE_KB, 0x54, "KEY_LEFTBRACE" },
+	{ BTN_TYPE_KB, 0x5B, "KEY_RIGHTBRACE" },
+	{ BTN_TYPE_KB, 0x5D, "KEY_BACKSLASH" },
+	{ BTN_TYPE_KB, 0x58, "KEY_CAPSLOCK" },
+	{ BTN_TYPE_KB, 0x1C, "KEY_A" },
+	{ BTN_TYPE_KB, 0x1B, "KEY_S" },
+	{ BTN_TYPE_KB, 0x23, "KEY_D" },
+	{ BTN_TYPE_KB, 0x2B, "KEY_F" },
+	{ BTN_TYPE_KB, 0x34, "KEY_G" },
+	{ BTN_TYPE_KB, 0x33, "KEY_H" },
+	{ BTN_TYPE_KB, 0x3B, "KEY_J" },
+	{ BTN_TYPE_KB, 0x42, "KEY_K" },
+	{ BTN_TYPE_KB, 0x4B, "KEY_L" },
+	{ BTN_TYPE_KB, 0x4C, "KEY_SEMICOLON" },
+	{ BTN_TYPE_KB, 0x52, "KEY_APOSTROPHE" },
+	{ BTN_TYPE_KB, 0x5A, "KEY_ENTER" },
+	{ BTN_TYPE_KB, 0x88, "KEY_LEFTSHIFT" },
+	{ BTN_TYPE_KB, 0x1A, "KEY_Z" },
+	{ BTN_TYPE_KB, 0x22, "KEY_X" },
+	{ BTN_TYPE_KB, 0x21, "KEY_C" },
+	{ BTN_TYPE_KB, 0x2A, "KEY_V" },
+	{ BTN_TYPE_KB, 0x32, "KEY_B" },
+	{ BTN_TYPE_KB, 0x31, "KEY_N" },
+	{ BTN_TYPE_KB, 0x3A, "KEY_M" },
+	{ BTN_TYPE_KB, 0x41, "KEY_COMMA" },
+	{ BTN_TYPE_KB, 0x49, "KEY_DOT" },
+	{ BTN_TYPE_KB, 0x4A, "KEY_SLASH" },
+	{ BTN_TYPE_KB, 0x89, "KEY_RIGHTSHIFT" },
+	{ BTN_TYPE_KB, 0x82, "KEY_LEFTMETA" },
+	{ BTN_TYPE_KB, 0x8A, "KEY_LEFTALT" },
+	{ BTN_TYPE_KB, 0x29, "KEY_SPACE" },
+	{ BTN_TYPE_KB, 0x8B, "KEY_RIGHTALT" },
+	{ BTN_TYPE_KB, 0x84, "KEY_COMPOSE" },
+	{ BTN_TYPE_KB, 0x8D, "KEY_RIGHTCTRL" },
+	{ BTN_TYPE_KB, 0xC3, "KEY_SYSRQ" },
+	{ BTN_TYPE_KB, 0x7E, "KEY_SCROLLLOCK" },
+	{ BTN_TYPE_KB, 0x91, "KEY_PAUSE" },
+	{ BTN_TYPE_KB, 0xC2, "KEY_INSERT" },
+	{ BTN_TYPE_KB, 0x94, "KEY_HOME" },
+	{ BTN_TYPE_KB, 0x96, "KEY_PAGEUP" },
+	{ BTN_TYPE_KB, 0xC0, "KEY_DELETE" },
+	{ BTN_TYPE_KB, 0x95, "KEY_END" },
+	{ BTN_TYPE_KB, 0x97, "KEY_PAGEDOWN" },
+	{ BTN_TYPE_KB, 0x98, "KEY_UP" },
+	{ BTN_TYPE_KB, 0x99, "KEY_DOWN" },
+	{ BTN_TYPE_KB, 0x9A, "KEY_LEFT" },
+	{ BTN_TYPE_KB, 0x9B, "KEY_RIGHT" },
+
+	/* Numpad button codes */
+	{ BTN_TYPE_KB, 0x77, "KEY_NUMLOCK" },
+	{ BTN_TYPE_KB, 0x90, "KEY_KPSLASH" },
+	{ BTN_TYPE_KB, 0x7C, "KEY_KPASTERISK" },
+	{ BTN_TYPE_KB, 0x7B, "KEY_KPMINUS" },
+	{ BTN_TYPE_KB, 0x70, "KEY_KP0" },
+	{ BTN_TYPE_KB, 0x69, "KEY_KP1" },
+	{ BTN_TYPE_KB, 0x72, "KEY_KP2" },
+	{ BTN_TYPE_KB, 0x7A, "KEY_KP3" },
+	{ BTN_TYPE_KB, 0x6B, "KEY_KP4" },
+	{ BTN_TYPE_KB, 0x73, "KEY_KP5" },
+	{ BTN_TYPE_KB, 0x74, "KEY_KP6" },
+	{ BTN_TYPE_KB, 0x6C, "KEY_KP7" },
+	{ BTN_TYPE_KB, 0x75, "KEY_KP8" },
+	{ BTN_TYPE_KB, 0x7D, "KEY_KP9" },
+	{ BTN_TYPE_KB, 0x79, "KEY_KPPLUS" },
+	{ BTN_TYPE_KB, 0x81, "KEY_KPENTER" },
+	{ BTN_TYPE_KB, 0x71, "KEY_KPDOT" },
+
+	/* Mouse button codes */
+	{ BTN_TYPE_MOUSE, 0x01, "BTN_LEFT" },
+	{ BTN_TYPE_MOUSE, 0x02, "BTN_RIGHT" },
+	{ BTN_TYPE_MOUSE, 0x03, "BTN_MIDDLE" },
+	{ BTN_TYPE_MOUSE, 0x04, "REL_WHEEL_HI_RES_UP" },
+	{ BTN_TYPE_MOUSE, 0x05, "REL_WHEEL_HI_RES_DOWN" },
+
+	/* 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);
+
+/* Button pair indexes for mapping commands */
+enum btn_pair_index {
+	BTN_PAIR_DPAD_UPDOWN    = 0x01,
+	BTN_PAIR_DPAD_LEFTRIGHT = 0x02,
+	BTN_PAIR_STICK_LR       = 0x03,
+	BTN_PAIR_BUMPER_LR      = 0x04,
+	BTN_PAIR_AB             = 0x05,
+	BTN_PAIR_XY             = 0x06,
+	BTN_PAIR_VIEW_MENU      = 0x07,
+	BTN_PAIR_M1M2           = 0x08,
+	BTN_PAIR_TRIGGER_LR     = 0x09,
+};
+
+struct button_map {
+	struct btn_code_map *remap;
+	struct btn_code_map *macro;
+};
+
+struct button_pair_map {
+	enum btn_pair_index pair_index;
+	struct button_map first;
+	struct button_map second;
+};
+
+/* Store button mapping per gamepad mode */
+struct ally_button_mapping {
+	struct button_pair_map button_pairs[9]; /* 9 button pairs */
+};
+
+static void ally_set_default_gamepad_mapping(struct ally_button_mapping *mappings)
+{
+	struct ally_button_mapping *map = &mappings[ALLY_GAMEPAD_MODE_GAMEPAD];
+	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 =
+			(struct btn_code_map *)&ally_btn_codes[0];
+		map->button_pairs[i].first.macro =
+			(struct btn_code_map *)&ally_btn_codes[0];
+		map->button_pairs[i].second.remap =
+			(struct btn_code_map *)&ally_btn_codes[0];
+		map->button_pairs[i].second.macro =
+			(struct btn_code_map *)&ally_btn_codes[0];
+	}
+
+	/* Set direct mappings using array indices */
+	map->button_pairs[BTN_PAIR_AB - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[1]; /* PAD_A */
+	map->button_pairs[BTN_PAIR_AB - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[2]; /* PAD_B */
+
+	map->button_pairs[BTN_PAIR_XY - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[3]; /* PAD_X */
+	map->button_pairs[BTN_PAIR_XY - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[4]; /* PAD_Y */
+
+	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[5]; /* PAD_LB */
+	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[6]; /* PAD_RB */
+
+	map->button_pairs[BTN_PAIR_STICK_LR - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[7]; /* PAD_LS */
+	map->button_pairs[BTN_PAIR_STICK_LR - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[8]; /* PAD_RS */
+
+	map->button_pairs[BTN_PAIR_DPAD_UPDOWN - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[9]; /* PAD_DPAD_UP */
+	map->button_pairs[BTN_PAIR_DPAD_UPDOWN - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[10]; /* PAD_DPAD_DOWN */
+
+	map->button_pairs[BTN_PAIR_DPAD_LEFTRIGHT - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[11]; /* PAD_DPAD_LEFT */
+	map->button_pairs[BTN_PAIR_DPAD_LEFTRIGHT - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[12]; /* PAD_DPAD_RIGHT */
+
+	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[13]; /* PAD_LT */
+	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[14]; /* PAD_RT */
+
+	map->button_pairs[BTN_PAIR_VIEW_MENU - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[15]; /* PAD_VIEW */
+	map->button_pairs[BTN_PAIR_VIEW_MENU - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[16]; /* PAD_MENU */
+
+	map->button_pairs[BTN_PAIR_M1M2 - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[18]; /* FN_M2 */
+	map->button_pairs[BTN_PAIR_M1M2 - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[19]; /* FN_M1 */
+}
+
+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 =
+			(struct btn_code_map *)&ally_btn_codes[0];
+		map->button_pairs[i].first.macro =
+			(struct btn_code_map *)&ally_btn_codes[0];
+		map->button_pairs[i].second.remap =
+			(struct btn_code_map *)&ally_btn_codes[0];
+		map->button_pairs[i].second.macro =
+			(struct btn_code_map *)&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 */
+	map->button_pairs[BTN_PAIR_AB - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[91]; /* KB_SPACE */
+	map->button_pairs[BTN_PAIR_AB - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[53]; /* KB_E */
+
+	/* btn_x => KB_R, btn_y => KB_F */
+	map->button_pairs[BTN_PAIR_XY - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[54]; /* KB_R */
+	map->button_pairs[BTN_PAIR_XY - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[68]; /* KB_F */
+
+	/* LB => MOUSE_WHEEL_UP, RB => MOUSE_WHEEL_DOWN */
+	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[128]; /* MOUSE_WHEEL_UP */
+	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[129]; /* MOUSE_WHEEL_DOWN */
+
+	/* left stick click => MOUSE_LCLICK, right stick click => KB_LSHIFT */
+	map->button_pairs[BTN_PAIR_STICK_LR - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[125]; /* MOUSE_LCLICK */
+	map->button_pairs[BTN_PAIR_STICK_LR - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[77]; /* KB_LSHIFT */
+
+	/* LT => MOUSE_LCLICK, RT => MOUSE_RCLICK */
+	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[125]; /* MOUSE_LCLICK */
+	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[126]; /* MOUSE_RCLICK */
+
+	/* M2 => FN_M2, M1 => FN_M1 */
+	map->button_pairs[BTN_PAIR_M1M2 - 1].first.remap =
+		(struct btn_code_map *)&ally_btn_codes[18]; /* FN_M2 */
+	map->button_pairs[BTN_PAIR_M1M2 - 1].second.remap =
+		(struct btn_code_map *)&ally_btn_codes[19]; /* FN_M1 */
+}
+
 static ssize_t gamepad_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct hid_device *hdev = to_hid_device(dev);
@@ -990,9 +1314,28 @@ static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *a
 	if (ret < 0)
 		return ret;
 
-	scoped_guard(mutex, &cfg->config_mutex)
+	scoped_guard(mutex, &cfg->config_mutex) {
 		cfg->gamepad_mode = mode_byte;
 
+		/*
+		 * When the gamepad mode changes, the hardware automatically
+		 * resets all button mappings to its internal defaults for the
+		 * new mode.
+		 *
+		 * Refresh the software cache so that subsequent
+		 * reads from sysfs reflect what the firmware is actually
+		 * using.
+		 */
+		if (cfg->button_mappings) {
+			struct ally_button_mapping *mappings = cfg->button_mappings;
+
+			if (mode_byte == ALLY_GAMEPAD_MODE_GAMEPAD)
+				ally_set_default_gamepad_mapping(mappings);
+			else if (mode_byte == ALLY_GAMEPAD_MODE_KEYBOARD)
+				ally_set_default_keyboard_mapping(mappings);
+		}
+	}
+
 	hid_dbg(hdev, "Set gamepad mode to %s\n", ally_gamepad_mode_text[mode]);
 
 	return count;
@@ -2518,6 +2861,446 @@ static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device
 	return no_free_ptr(attr);
 }
 
+/* Find a button code map by its name */
+static const struct btn_code_map *find_button_by_name(const char *name)
+{
+	int i;
+
+	for (i = 0; i < keymap_len; i++) {
+		if (strcmp(ally_btn_codes[i].name, name) == 0)
+			return &ally_btn_codes[i];
+	}
+
+	return NULL;
+}
+
+/*
+ * Check whether a remap target is valid for a given button in the current
+ * gamepad mode.
+ *
+ * M1 and M2 can be remapped to anything at any time.
+ * All other buttons may only be remapped to gamepad buttons (BTN_TYPE_PAD)
+ * when in gamepad mode, and to keyboard/mouse/media codes when in desktop
+ * (keyboard) mode.  BTN_TYPE_NONE (unmapped) is always allowed.
+ */
+static bool ally_remap_code_valid(enum ally_button_id btn, u8 gamepad_mode,
+				  const struct btn_code_map *code)
+{
+	if (!code)
+		return false;
+
+	/* M1 and M2 can be remapped to everything */
+	if (btn == ALLY_BTN_M1 || btn == ALLY_BTN_M2)
+		return true;
+
+	/* NONE is always valid */
+	if (code->type == BTN_TYPE_NONE)
+		return true;
+
+	if (gamepad_mode == ALLY_GAMEPAD_MODE_GAMEPAD)
+		return code->type == BTN_TYPE_PAD;
+
+	/* Desktop / keyboard mode: allow keyboard, mouse and media */
+	return code->type == BTN_TYPE_KB ||
+	       code->type == BTN_TYPE_MOUSE ||
+	       code->type == BTN_TYPE_MEDIA;
+}
+
+/* Set button mapping for a button pair */
+static int ally_set_button_mapping(struct hid_device *hdev, struct ally_handheld *ally,
+				  struct button_pair_map *mapping)
+{
+	/* The MCU mapping block is four consecutive 11-byte entries starting at
+	 * buf[5]: first remap 5-15, first macro 16-26, second remap 27-37,
+	 * second macro 38-48 (see hid-asus-ally __btn_pair_to_pkt, BTN_CODE_LEN).
+	 */
+	u8 macro_bytes[11] = {0};
+	u8 btn_bytes[11] = {0};
+
+	if (!mapping)
+		return -EINVAL;
+
+	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_MAPPING, NULL, 0);
+	if (!buf)
+		return -ENOMEM;
+
+	/* This packet is slightly different from the other
+	 * as before the packet length there is an extra byte
+	 * which is the pair index.
+	 */
+	buf[3] = mapping->pair_index;
+	buf[4] = 0x2C; /* Length */
+
+	/* First button mapping */
+	buf[5] = mapping->first.remap->type;
+	/* Fill in bytes 6-14 with button code */
+	if (mapping->first.remap->type) {
+		memset(btn_bytes, 0, sizeof(btn_bytes));
+		btn_bytes[0] = mapping->first.remap->type;
+
+		/* Value byte position depends on type: pad=1, kb=2, media=3,
+		 * mouse=4 (see hid-asus-ally BTN_CODE definitions).
+		 */
+		switch (mapping->first.remap->type) {
+		case BTN_TYPE_NONE:
+			break;
+		case BTN_TYPE_PAD:
+			btn_bytes[1] = mapping->first.remap->value;
+			break;
+		case BTN_TYPE_KB:
+			btn_bytes[2] = mapping->first.remap->value;
+			break;
+		case BTN_TYPE_MEDIA:
+			btn_bytes[3] = mapping->first.remap->value;
+			break;
+		case BTN_TYPE_MOUSE:
+			btn_bytes[4] = mapping->first.remap->value;
+			break;
+		}
+		memcpy(&buf[5], btn_bytes, 11);
+	}
+
+	/* Macro mapping for first button if any */
+	buf[16] = mapping->first.macro->type;
+	if (mapping->first.macro->type) {
+		memset(macro_bytes, 0, sizeof(macro_bytes));
+		macro_bytes[0] = mapping->first.macro->type;
+
+		switch (mapping->first.macro->type) {
+		case BTN_TYPE_NONE:
+			break;
+		case BTN_TYPE_PAD:
+			macro_bytes[1] = mapping->first.macro->value;
+			break;
+		case BTN_TYPE_KB:
+			macro_bytes[2] = mapping->first.macro->value;
+			break;
+		case BTN_TYPE_MEDIA:
+			macro_bytes[3] = mapping->first.macro->value;
+			break;
+		case BTN_TYPE_MOUSE:
+			macro_bytes[4] = mapping->first.macro->value;
+			break;
+		}
+		memcpy(&buf[16], macro_bytes, 11);
+	}
+
+	/* Second button mapping */
+	buf[27] = mapping->second.remap->type;
+	/* Fill in bytes 28-36 with button code */
+	if (mapping->second.remap->type) {
+		memset(btn_bytes, 0, sizeof(btn_bytes));
+		btn_bytes[0] = mapping->second.remap->type;
+
+		switch (mapping->second.remap->type) {
+		case BTN_TYPE_NONE:
+			break;
+		case BTN_TYPE_PAD:
+			btn_bytes[1] = mapping->second.remap->value;
+			break;
+		case BTN_TYPE_KB:
+			btn_bytes[2] = mapping->second.remap->value;
+			break;
+		case BTN_TYPE_MEDIA:
+			btn_bytes[3] = mapping->second.remap->value;
+			break;
+		case BTN_TYPE_MOUSE:
+			btn_bytes[4] = mapping->second.remap->value;
+			break;
+		}
+		memcpy(&buf[27], btn_bytes, 11);
+	}
+
+	/* Macro mapping for second button if any */
+	buf[38] = mapping->second.macro->type;
+	if (mapping->second.macro->type) {
+		memset(macro_bytes, 0, sizeof(macro_bytes));
+		macro_bytes[0] = mapping->second.macro->type;
+
+		switch (mapping->second.macro->type) {
+		case BTN_TYPE_NONE:
+			break;
+		case BTN_TYPE_PAD:
+			macro_bytes[1] = mapping->second.macro->value;
+			break;
+		case BTN_TYPE_KB:
+			macro_bytes[2] = mapping->second.macro->value;
+			break;
+		case BTN_TYPE_MEDIA:
+			macro_bytes[3] = mapping->second.macro->value;
+			break;
+		case BTN_TYPE_MOUSE:
+			macro_bytes[4] = mapping->second.macro->value;
+			break;
+		}
+		memcpy(&buf[38], macro_bytes, 11);
+	}
+
+	return ally_gamepad_send_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
+}
+
+/* Button remap attribute structure */
+struct button_remap_attr {
+	struct device_attribute dev_attr;
+	enum ally_button_id button_id;
+	bool is_macro;
+};
+
+#define to_button_remap_attr(x) container_of(x, struct button_remap_attr, dev_attr)
+
+/* Get appropriate button pair index and position for a given button */
+static int get_button_pair_info(enum ally_button_id button_id,
+				enum btn_pair_index *pair_idx,
+				bool *is_first)
+{
+	switch (button_id) {
+	case ALLY_BTN_DU:
+		*pair_idx = BTN_PAIR_DPAD_UPDOWN;
+		*is_first = true;
+		break;
+	case ALLY_BTN_DD:
+		*pair_idx = BTN_PAIR_DPAD_UPDOWN;
+		*is_first = false;
+		break;
+	case ALLY_BTN_DL:
+		*pair_idx = BTN_PAIR_DPAD_LEFTRIGHT;
+		*is_first = true;
+		break;
+	case ALLY_BTN_DR:
+		*pair_idx = BTN_PAIR_DPAD_LEFTRIGHT;
+		*is_first = false;
+		break;
+	case ALLY_BTN_J0B:
+		*pair_idx = BTN_PAIR_STICK_LR;
+		*is_first = true;
+		break;
+	case ALLY_BTN_J1B:
+		*pair_idx = BTN_PAIR_STICK_LR;
+		*is_first = false;
+		break;
+	case ALLY_BTN_LB:
+		*pair_idx = BTN_PAIR_BUMPER_LR;
+		*is_first = true;
+		break;
+	case ALLY_BTN_RB:
+		*pair_idx = BTN_PAIR_BUMPER_LR;
+		*is_first = false;
+		break;
+	case ALLY_BTN_A:
+		*pair_idx = BTN_PAIR_AB;
+		*is_first = true;
+		break;
+	case ALLY_BTN_B:
+		*pair_idx = BTN_PAIR_AB;
+		*is_first = false;
+		break;
+	case ALLY_BTN_X:
+		*pair_idx = BTN_PAIR_XY;
+		*is_first = true;
+		break;
+	case ALLY_BTN_Y:
+		*pair_idx = BTN_PAIR_XY;
+		*is_first = false;
+		break;
+	case ALLY_BTN_VIEW:
+		*pair_idx = BTN_PAIR_VIEW_MENU;
+		*is_first = true;
+		break;
+	case ALLY_BTN_MENU:
+		*pair_idx = BTN_PAIR_VIEW_MENU;
+		*is_first = false;
+		break;
+	case ALLY_BTN_M1:
+		*pair_idx = BTN_PAIR_M1M2;
+		*is_first = false;
+		break;
+	case ALLY_BTN_M2:
+		*pair_idx = BTN_PAIR_M1M2;
+		*is_first = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t button_remap_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
+	struct ally_config *cfg;
+	enum ally_button_id button_id = btn_attr->button_id;
+	enum btn_pair_index pair_idx;
+	bool is_first;
+	struct button_pair_map *pair;
+	struct button_map *btn_map;
+	int ret;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
+	if (ret < 0)
+		return ret;
+
+	guard(mutex)(&cfg->config_mutex);
+	pair = &((struct ally_button_mapping
+			  *)(cfg->button_mappings))[cfg->gamepad_mode]
+			.button_pairs[pair_idx - 1];
+	btn_map = is_first ? &pair->first : &pair->second;
+
+	if (btn_attr->is_macro) {
+		if (btn_map->macro->type == BTN_TYPE_NONE)
+			return sysfs_emit(buf, "NONE\n");
+		else
+			return sysfs_emit(buf, "%s\n", btn_map->macro->name);
+	} else {
+		if (btn_map->remap->type == BTN_TYPE_NONE)
+			return sysfs_emit(buf, "NONE\n");
+		else
+			return sysfs_emit(buf, "%s\n", btn_map->remap->name);
+	}
+}
+
+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 asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	struct ally_handheld *ally = drvdata->rog_ally;
+	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;
+	enum btn_pair_index pair_idx;
+	struct button_pair_map *pair;
+	struct button_map *btn_map;
+	struct ally_config *cfg;
+	char btn_name[32];
+	bool is_first;
+	int ret, i;
+
+	if (!ally || !ally->config)
+		return -ENODEV;
+
+	cfg = ally->config;
+
+	if (sscanf(buf, "%31s", btn_name) != 1)
+		return -EINVAL;
+
+	/* Handle "NONE" specially */
+	if (strcmp(btn_name, "NONE") == 0) {
+		code = &ally_btn_codes[0]; /* NONE entry */
+	} else {
+		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;
+
+	/* Validate that the remap target is allowed for this button/mode */
+	if (!ally_remap_code_valid(button_id, cfg->gamepad_mode, code))
+		return -EINVAL;
+
+	scoped_guard(mutex, &cfg->config_mutex) {
+		/* Access the mapping for current gamepad mode */
+		pair = &((struct ally_button_mapping
+				  *)(cfg->button_mappings))[cfg->gamepad_mode]
+				.button_pairs[pair_idx - 1];
+		btn_map = is_first ? &pair->first : &pair->second;
+
+		if (btn_attr->is_macro)
+			btn_map->macro = (struct btn_code_map *)code;
+		else
+			btn_map->remap = (struct btn_code_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 (cfg->is_ally_x) {
+			ret = ally_set_button_mapping(hdev, ally, pair);
+			if (ret < 0)
+				return ret;
+		} else {
+			mode_map = &((struct ally_button_mapping
+					 *)(cfg->button_mappings))[cfg->gamepad_mode];
+			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;
+			}
+		}
+	}
+
+	return count;
+}
+
+/* Helper to create button remap attribute */
+static struct button_remap_attr *button_remap_attr_create(enum ally_button_id button_id,
+							  bool is_macro)
+{
+	struct button_remap_attr *attr __free(kfree) = kzalloc_obj(*attr);
+	if (!attr)
+		return NULL;
+
+	attr->button_id = button_id;
+	attr->is_macro = is_macro;
+	sysfs_attr_init(&attr->dev_attr.attr);
+	attr->dev_attr.attr.name = is_macro ? "macro" : "remap";
+	attr->dev_attr.attr.mode = 0644;
+	attr->dev_attr.show = button_remap_show;
+	attr->dev_attr.store = button_remap_store;
+
+	return no_free_ptr(attr);
+}
+
+/*
+ * Show the list of valid remap target names for a button in the current
+ * gamepad mode.  M1/M2 list everything; other buttons list only the codes
+ * valid for the active mode (gamepad buttons in gamepad mode, keyboard /
+ * mouse / media in desktop mode).
+ */
+static ssize_t btn_remap_index_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct ally_btn_sysfs_entry *entry = container_of(attr,
+		struct ally_btn_sysfs_entry, attr_remap_index);
+	enum ally_button_id btn = entry->btn;
+	u8 mode;
+	ssize_t len = 0;
+	int i;
+
+	scoped_guard(mutex, &entry->cfg->config_mutex)
+		mode = entry->cfg->gamepad_mode;
+
+	for (i = 0; i < keymap_len; i++) {
+		if (!ally_remap_code_valid(btn, mode, &ally_btn_codes[i]))
+			continue;
+		len += sysfs_emit_at(buf, len, "%s ", ally_btn_codes[i].name);
+	}
+
+	if (len > 0)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
 /**
  * ally_create_button_attributes - Create turbo button attributes
  * @hdev: HID device
@@ -2528,29 +3311,78 @@ static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device
 static int ally_create_button_attributes(struct hid_device *hdev, struct ally_config *cfg)
 {
 	struct ally_btn_sysfs_entry *entries;
+	struct ally_button_mapping *mappings;
 	int i, ret;
 
-	if (!cfg->turbo_support)
-		return 0;
-
 	entries = devm_kcalloc(&hdev->dev, ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);
 	if (!entries)
 		return -ENOMEM;
 
+	/* Allocate mappings for each gamepad mode (1-based indexing) */
+	mappings = devm_kcalloc(&hdev->dev, ALLY_GAMEPAD_MODE_KEYBOARD + 1,
+				sizeof(*mappings), GFP_KERNEL);
+	if (!mappings) {
+		ret = -ENOMEM;
+		goto err_free_entries;
+	}
+
 	cfg->button_entries = entries;
+	cfg->button_mappings = mappings;
+	ally_set_default_gamepad_mapping(mappings);
+	ally_set_default_keyboard_mapping(mappings);
 
 	for (i = 0; i < ALLY_BTN_MAX; i++) {
 		entries[i].cfg = cfg;
 		entries[i].hdev = hdev;
 		entries[i].btn = i;
 
-		entries[i].turbo_attr = ally_btn_turbo_attr_create(hdev, &entries[i]);
-		if (IS_ERR(entries[i].turbo_attr)) {
-			ret = PTR_ERR(entries[i].turbo_attr);
-			entries[i].turbo_attr = NULL;
+		if (cfg->turbo_support) {
+			entries[i].turbo_attr = ally_btn_turbo_attr_create(hdev, &entries[i]);
+			if (IS_ERR(entries[i].turbo_attr)) {
+				ret = PTR_ERR(entries[i].turbo_attr);
+				entries[i].turbo_attr = NULL;
+				goto err_cleanup;
+			}
+		}
+
+		entries[i].remap_attr = button_remap_attr_create(i, false);
+		if (!entries[i].remap_attr) {
+			ret = -ENOMEM;
 			goto err_cleanup;
 		}
 
+		entries[i].macro_attr = button_remap_attr_create(i, true);
+		if (!entries[i].macro_attr) {
+			ret = -ENOMEM;
+			goto err_cleanup;
+		}
+
+		/* Initialize the remap_index attribute */
+		sysfs_attr_init(&entries[i].attr_remap_index.attr);
+		entries[i].attr_remap_index.attr.name = "remap_index";
+		entries[i].attr_remap_index.attr.mode = 0444;
+		entries[i].attr_remap_index.show = btn_remap_index_show;
+		entries[i].attr_remap_index.store = NULL;
+
+		/* Set up attributes array based on what's supported */
+		if (cfg->turbo_support) {
+			entries[i].attrs[4] =
+				&entries[i].remap_attr->dev_attr.attr;
+			entries[i].attrs[5] =
+				&entries[i].macro_attr->dev_attr.attr;
+			entries[i].attrs[6] =
+				&entries[i].attr_remap_index.attr;
+			entries[i].attrs[7] = NULL;
+		} else {
+			entries[i].attrs[0] =
+				&entries[i].remap_attr->dev_attr.attr;
+			entries[i].attrs[1] =
+				&entries[i].macro_attr->dev_attr.attr;
+			entries[i].attrs[2] =
+				&entries[i].attr_remap_index.attr;
+			entries[i].attrs[3] = NULL;
+		}
+
 		entries[i].group.name = ally_button_names[i];
 		entries[i].group.attrs = entries[i].attrs;
 
@@ -2571,11 +3403,20 @@ static int ally_create_button_attributes(struct hid_device *hdev, struct ally_co
 
 	for (i = 0; i < ALLY_BTN_MAX; i++) {
 		kfree(entries[i].turbo_attr);
+		kfree(entries[i].remap_attr);
+		kfree(entries[i].macro_attr);
 		entries[i].turbo_attr = NULL;
+		entries[i].remap_attr = NULL;
+		entries[i].macro_attr = NULL;
 	}
+err_free_entries:
+	if (mappings)
+		devm_kfree(&hdev->dev, mappings);
+	devm_kfree(&hdev->dev, entries);
 
 	/* Nullify the entries and mappings to prevent use-after-free crashes */
 	cfg->button_entries = NULL;
+	cfg->button_mappings = NULL;
 
 	return ret;
 }
@@ -2598,8 +3439,16 @@ static void ally_remove_button_attributes(struct hid_device *hdev, struct ally_c
 	for (i = 0; i < ALLY_BTN_MAX; i++) {
 		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
 		kfree(entries[i].turbo_attr);
+		kfree(entries[i].remap_attr);
+		kfree(entries[i].macro_attr);
+	}
+
+	if (cfg->button_mappings) {
+		devm_kfree(&hdev->dev, cfg->button_mappings);
+		cfg->button_mappings = NULL;
 	}
 
+	devm_kfree(&hdev->dev, entries);
 	cfg->button_entries = NULL;
 }
 
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Use-After-Free (UAF) in sysfs store functions due to manual lockless deallocation of button mappings.
- [High] Sysfs attributes are exposed before `gamepad_mode` initialization, causing a reproducible NULL pointer dereference.
- [Medium] Hardware update errors are silently swallowed for non-Ally X devices, and the software cache is permanently corrupted on failure.
- [Medium] Button remapping sysfs attributes are entirely inaccessible for devices lacking turbo support.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in `button_remap_store` bypasses validation.
--

--- Patch [12]: [PATCH 12/12] HID: asus: add support for btn remapping ---
commit d94c33db2e53bcd63e5e7044bbdf330bf4916ffa
Author: Denis Benato <denis.benato@linux.dev>
Subject: 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.

[ ... ]

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 359832395afec..5b744dba98d3e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2528,29 +3311,78 @@ static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device
> +static ssize_t button_remap_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
> +	struct ally_config *cfg;
> +	enum ally_button_id button_id = btn_attr->button_id;
> +	enum btn_pair_index pair_idx;
> +	bool is_first;
> +	struct button_pair_map *pair;
> +	struct button_map *btn_map;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	cfg = ally->config;
> +
> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(mutex)(&cfg->config_mutex);
> +	pair = &((struct ally_button_mapping
> +			  *)(cfg->button_mappings))[cfg->gamepad_mode]
> +			.button_pairs[pair_idx - 1];
> +	btn_map = is_first ? &pair->first : &pair->second;
> +
> +	if (btn_attr->is_macro) {
> +		if (btn_map->macro->type == BTN_TYPE_NONE)

[Severity: High]
Could btn_map->macro be NULL here during device initialization?

When ally_create_button_attributes() registers the sysfs groups,
cfg->gamepad_mode is 0 by default. It is not set to 1 (Gamepad) until later
in the probe sequence.

If a user reads the remap or macro sysfs files before cfg->gamepad_mode is
updated, this will index cfg->button_mappings[0]. Since only indexes 1 and 2
are populated by ally_set_default_gamepad_mapping() and
ally_set_default_keyboard_mapping(), the pointers in index 0 remain
uninitialized. Dereferencing btn_map->macro->type would then cause a kernel
panic.

> +			return sysfs_emit(buf, "NONE\n");
> +		else
> +			return sysfs_emit(buf, "%s\n", btn_map->macro->name);
> +	} else {
> +		if (btn_map->remap->type == BTN_TYPE_NONE)
> +			return sysfs_emit(buf, "NONE\n");
> +		else
> +			return sysfs_emit(buf, "%s\n", btn_map->remap->name);
> +	}
> +}

[ ... ]

> +static ssize_t button_remap_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{

[ ... ]

> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Validate that the remap target is allowed for this button/mode */
> +	if (!ally_remap_code_valid(button_id, cfg->gamepad_mode, code))
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		/* Access the mapping for current gamepad mode */
> +		pair = &((struct ally_button_mapping
> +				  *)(cfg->button_mappings))[cfg->gamepad_mode]

[Severity: Medium]
Is there a Time-of-Check to Time-of-Use (TOCTOU) race condition here?

The validation function ally_remap_code_valid() reads cfg->gamepad_mode
locklessly. If a concurrent sysfs write to gamepad_mode_store() alters the
mode right before cfg->config_mutex is acquired, an invalid mapping could
bypass the checks and be written to the new mode's configuration.

> +				.button_pairs[pair_idx - 1];
> +		btn_map = is_first ? &pair->first : &pair->second;
> +
> +		if (btn_attr->is_macro)
> +			btn_map->macro = (struct btn_code_map *)code;
> +		else
> +			btn_map->remap = (struct btn_code_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 (cfg->is_ally_x) {
> +			ret = ally_set_button_mapping(hdev, ally, pair);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			mode_map = &((struct ally_button_mapping
> +					 *)(cfg->button_mappings))[cfg->gamepad_mode];
> +			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;
> +			}
> +		}
> +	}
> +
> +	return count;
> +}

[Severity: Medium]
For non-Ally X devices, if ally_set_button_mapping() fails, this loop breaks
but then unconditionally returns count (success) instead of ret.

Also, earlier in this block, the software cache is updated:

    if (btn_attr->is_macro)
        btn_map->macro = (struct btn_code_map *)code;
    else
        btn_map->remap = (struct btn_code_map *)code;

If the hardware update fails, does this mean the software cache is now
permanently desynchronized from the actual hardware state?

[ ... ]

> @@ -2528,29 +3311,78 @@ static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device
>  static int ally_create_button_attributes(struct hid_device *hdev, struct ally_config *cfg)
>  {
>  	struct ally_btn_sysfs_entry *entries;
> +	struct ally_button_mapping *mappings;
>  	int i, ret;
>  
> -	if (!cfg->turbo_support)
> -		return 0;
> -

[Severity: Medium]
Even though this check is removed here, does the caller (ally_config_create())
still restrict the creation of these attributes based on cfg->turbo_support?

If ally_create_button_attributes() is only called when cfg->turbo_support is
true, then devices without turbo support will not have access to the button
remapping sysfs attributes either.

>  	entries = devm_kcalloc(&hdev->dev, ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);

[ ... ]

> @@ -2598,8 +3439,16 @@ static void ally_remove_button_attributes(struct hid_device *hdev, struct ally_c
>  	for (i = 0; i < ALLY_BTN_MAX; i++) {
>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
>  		kfree(entries[i].turbo_attr);
> +		kfree(entries[i].remap_attr);
> +		kfree(entries[i].macro_attr);
> +	}
> +
> +	if (cfg->button_mappings) {
> +		devm_kfree(&hdev->dev, cfg->button_mappings);
> +		cfg->button_mappings = NULL;
>  	}

[Severity: High]
Can this cause a use-after-free if sysfs files are accessed concurrently?

The teardown path frees cfg->button_mappings manually using devm_kfree()
without holding cfg->config_mutex.

If a user writes to gamepad_mode_store() or button_remap_store() right as
this is being freed, it could lead to memory corruption or a kernel panic,
as those functions dereference cfg->button_mappings.

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