[PATCH v4 0/5] HID: asus: security fixes and more hardware support

[PATCH v4 0/5] HID: asus: security fixes and more hardware support

[Denis Benato]

Hi all,

I have added support for controlling the (way too bright) XG mobile
LEDs in hid-asus and added the i2c version of already supported
hardware that was probed only when it's a USB: these are two separate
features changes and are the only two that are not fixes for
pre-existing issue (see below).

Auto-review bot has spotted a bunch of pre-existing problems alongside
problems in my own code therefore at this point I am going to fix the
more problems I can and including those fixes and improvements in this
patchset.

For v4 I decided to follow Antheas' suggestion of reusing the existing
workqueue and by making it more generic I solved a good bunch of issues.

Link: https://lore.kernel.org/all/20260612124830.984592-1-denis.benato@linux.dev/
Link: https://lore.kernel.org/all/30258f28-4036-4e99-9494-031212544dc2@linux.dev/

Best regards,
Denis Benato

Link v2: https://lore.kernel.org/all/20260612142326.1704858-1-denis.benato@linux.dev/
Link v3: https://lore.kernel.org/all/20260613153029.2559774-1-denis.benato@linux.dev/

Changelog:
  - v1
    - Initial submission (as separate patches)
  - v2
    - HID: asus: add i2c entry for FA808UM and other TUFs
      - Joined to the patchset
    - HID: asus: add support for xgm led
      - Use the correct callback set_brightness_blocking
      - Stop hardcoding the cled name
    - HID: asus: prevent wrong pointer cast
      - Added
    - HID: asus: mitigate possible use-after-free
      - Added
  - v3
    - HID: asus: add support for xgm led
      - check devm_kasprintf result
    - HID: asus: prevent a late KEY_FN_ESC to trigger a use-after-free
      - Added
    - HID: asus: avoid sleeping calls in atomic context
      - Added
    - HID: asus: cleanup keyboard listener on failure: avoid use-after-free
      - Added
  - v4
    - HID: asus: refactor the two workqueues and init sequence
      - Added to replace previous patches in a clearer way
    - HID: asus: add support for xgm led
      - Simplify cled interface allocation and registration

Denis Benato (5):
  HID: asus: refactor the two workqueues and init sequence
  HID: asus: remove unnecessary OOM message
  HID: asus: fix a off-by-one issue making a check stricter that it
    needs to be
  HID: asus: add support for xgm led
  HID: asus: add i2c entry for FA808UM and other TUFs

 drivers/hid/hid-asus.c | 483 +++++++++++++++++++++++++++++++----------
 1 file changed, 373 insertions(+), 110 deletions(-)

-- 
2.47.3

[PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence

[Denis Benato]

Multiple issues have been found within the hid-asus driver:
- unchecked size in asus_raw_event()
- unclean teardown of asus_probe on failure
- possible use-after-free in asus_probe
- multiple workqueue used for jobs where one was enough
- sleeping calls in atomic context
- packets of incorrect size being sent to the keyboard controller

Join the two workqueues into one reusing the stopping mechanism
of the brightness workqueue, use the joined workqueue to also
move the asus_wmi_send_event() sleeping call away from atomic
context and add a size check in asus_raw_event().

Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
Reported-by: sahiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 393 +++++++++++++++++++++++++++++------------
 1 file changed, 282 insertions(+), 111 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index d34d74df3dc0..05ca6665e0a4 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 
 #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
 
-struct asus_kbd_leds {
-	struct asus_hid_listener listener;
+enum asus_work_action_type {
+	FN_LOCK_SYNC,
+	BRIGHTNESS_SET,
+	WMI_FAN,
+};
+
+struct hid_raw_event_data {
+	u8 report_data[FEATURE_KBD_REPORT_SIZE];
+	size_t report_size;
+};
+
+struct asus_work_action {
+	struct list_head node;
+	enum asus_work_action_type type;
+	union {
+		/* Data for BRIGHTNESS_SET */
+		unsigned int brightness;
+
+		/* Data for FN_LOCK_SYNC */
+		bool fn_lock;
+
+		/* Data for WMI_FAN */
+		struct hid_raw_event_data fan_hid_data;
+	} data;
+};
+
+struct asus_worker {
 	struct hid_device *hdev;
 	struct work_struct work;
-	unsigned int brightness;
+	struct list_head actions;
 	spinlock_t lock;
 	bool removed;
 };
@@ -133,7 +158,8 @@ struct asus_drvdata {
 	struct hid_device *hdev;
 	struct input_dev *input;
 	struct input_dev *tp_kbd_input;
-	struct asus_kbd_leds *kbd_backlight;
+	struct asus_worker *worker;
+	unsigned int kbd_backlight_brightness;
 	const struct asus_touchpad_info *tp;
 	struct power_supply *battery;
 	struct power_supply_desc battery_desc;
@@ -141,7 +167,7 @@ struct asus_drvdata {
 	int battery_stat;
 	bool battery_in_query;
 	unsigned long battery_next_query;
-	struct work_struct fn_lock_sync_work;
+	struct asus_hid_listener listener;
 	bool fn_lock;
 };
 
@@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
 	FEATURE_KBD_LED_REPORT_ID2
 };
 
+/*
+ * Send events to asus-wmi driver for handling special keys
+ */
+static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
+{
+	int err;
+	u32 retval;
+
+	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
+				       ASUS_WMI_METHODID_NOTIF, code, &retval);
+	if (err) {
+		pr_warn("Failed to notify asus-wmi: %d\n", err);
+		return err;
+	}
+
+	if (retval != 0) {
+		pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static void asus_report_contact_down(struct asus_drvdata *drvdat,
 		int toolType, u8 *data)
 {
@@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
 }
 
 /*
- * Send events to asus-wmi driver for handling special keys
+ * Used in atomic contexts to schedule work involving sleeps operations or
+ * asus-wmi interactions.
+ *
+ * Caller is responsible to store relevant data in the structure to carry out
+ * the required action.
+ *
+ * This function must be called while the spin lock protecting the workqueue
+ * is already being held.
  */
-static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
+static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
 {
-	int err;
-	u32 retval;
-
-	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
-				       ASUS_WMI_METHODID_NOTIF, code, &retval);
-	if (err) {
-		pr_warn("Failed to notify asus-wmi: %d\n", err);
-		return err;
+	if (worker->removed) {
+		kfree(action);
+		return;
 	}
 
-	if (retval != 0) {
-		pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
-		return -EIO;
+	list_add_tail(&action->node, &worker->actions);
+	schedule_work(&worker->work);
+}
+
+static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
+{
+	struct asus_work_action *action;
+	unsigned long flags;
+
+	action = kzalloc_obj(struct asus_work_action);
+	if (!action)
+		return -ENOMEM;
+
+	drvdata->fn_lock = enabled;
+	action->type = FN_LOCK_SYNC;
+	action->data.fn_lock = drvdata->fn_lock;
+	INIT_LIST_HEAD(&action->node);
+
+	spin_lock_irqsave(&drvdata->worker->lock, flags);
+	asus_worker_schedule(drvdata->worker, action);
+	spin_unlock_irqrestore(&drvdata->worker->lock, flags);
+
+	return 0;
+}
+
+static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
+				 size_t report_size)
+{
+	struct asus_work_action *action;
+	unsigned long flags;
+
+	if (report_size > FEATURE_KBD_REPORT_SIZE) {
+		hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
+		return -EINVAL;
 	}
 
+	action = kzalloc_obj(struct asus_work_action);
+	if (!action)
+		return -ENOMEM;
+
+	action->type = WMI_FAN;
+	action->data.fan_hid_data.report_size = report_size;
+	memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
+	INIT_LIST_HEAD(&action->node);
+
+	spin_lock_irqsave(&drvdata->worker->lock, flags);
+	asus_worker_schedule(drvdata->worker, action);
+	spin_unlock_irqrestore(&drvdata->worker->lock, flags);
+
 	return 0;
 }
 
@@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
 		      struct hid_usage *usage, __s32 value)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	int ret;
 
 	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
 	    (usage->hid & HID_USAGE) != 0x00 &&
@@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
 			return !asus_hid_event(ASUS_EV_BRTTOGGLE);
 		case KEY_FN_ESC:
 			if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
-				drvdata->fn_lock = !drvdata->fn_lock;
-				schedule_work(&drvdata->fn_lock_sync_work);
+				ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
+				if (ret) {
+					hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
+					return ret;
+				}
 			}
 			break;
 		}
@@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
 		struct hid_report *report, u8 *data, int size)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	int ret;
+
+	if (size < 2) {
+		hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
+		return 0;
+	}
 
 	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
 		return asus_report_battery(drvdata, data, size);
@@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
 			 * pass to userspace so it can implement its own fan control.
 			 */
 			if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
-				int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
+				ret = asus_kbd_wmi_fan_send(drvdata, data, size);
 
-				if (ret == 0) {
-					/* Successfully handled by asus-wmi, block event */
+				/* if execution deferred successfully block event */
+				if (ret == 0)
 					return -1;
-				}
 
-				/*
-				 * Warn if asus-wmi failed (but not if it's unavailable).
-				 * Let the event reach userspace in all failure cases.
-				 */
-				if (ret != -ENODEV)
-					hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
+				return ret;
 			}
 
 			/*
@@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
 	return 0;
 }
 
-static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
+static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
 {
-	u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
+	const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
+	int ret;
 
-	return asus_kbd_set_report(hdev, buf, sizeof(buf));
+	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
 }
 
-static void asus_sync_fn_lock(struct work_struct *work)
+static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
 {
-	struct asus_drvdata *drvdata =
-	container_of(work, struct asus_drvdata, fn_lock_sync_work);
+	const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
+		FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
+	};
+	int ret;
 
-	asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
+	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
 }
 
-static void asus_schedule_work(struct asus_kbd_leds *led)
+static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
 {
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	int ret;
+
+	ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
+
+	/*
+	 * Warn if asus-wmi failed (but not if it's unavailable).
+	 * Let the event reach userspace in all failure cases.
+	 */
+	switch (ret) {
+	case -ENODEV:
+		break;
+	case 0:
+		return;
+	default:
+		hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
+		break;
+	}
+
+	/* Fallback: pass the raw event to the HID core */
+	hid_report_raw_event(hdev, HID_INPUT_REPORT,
+			     data->report_data, data->report_size,
+			     data->report_size, 1);
+}
+
+static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
+{
+	struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
+	struct asus_worker *worker = drvdata->worker;
+	struct asus_work_action *action;
 	unsigned long flags;
 
-	spin_lock_irqsave(&led->lock, flags);
-	if (!led->removed)
-		schedule_work(&led->work);
-	spin_unlock_irqrestore(&led->lock, flags);
+	drvdata->kbd_backlight_brightness = brightness;
+
+	action = kzalloc_obj(struct asus_work_action);
+	if (!action) {
+		hid_warn(drvdata->hdev, "Failed to allocate memory for backlight action\n");
+		return;
+	}
+
+	action->type = BRIGHTNESS_SET;
+	action->data.brightness = brightness;
+	INIT_LIST_HEAD(&action->node);
+
+	spin_lock_irqsave(&worker->lock, flags);
+	asus_worker_schedule(worker, action);
+	spin_unlock_irqrestore(&worker->lock, flags);
 }
 
-static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
-				   int brightness)
+static void asus_work(struct work_struct *work)
 {
-	struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
-						 listener);
+	struct asus_worker *worker = container_of(work, struct asus_worker, work);
+	struct asus_work_action *action = NULL;
 	unsigned long flags;
 
-	spin_lock_irqsave(&led->lock, flags);
-	led->brightness = brightness;
-	spin_unlock_irqrestore(&led->lock, flags);
+	/* Save the action to be performed and clear the flag */
+	spin_lock_irqsave(&worker->lock, flags);
+	if (!list_empty(&worker->actions)) {
+		action = list_first_entry(&worker->actions,
+					  struct asus_work_action, node);
+		list_del(&action->node);
+	}
+	spin_unlock_irqrestore(&worker->lock, flags);
+
+	if (!action)
+		return;
+
+	switch (action->type) {
+	case BRIGHTNESS_SET:
+		asus_kbd_set_brightness(worker->hdev, action->data.brightness);
+		break;
+	case FN_LOCK_SYNC:
+		asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
+		break;
+	case WMI_FAN:
+		asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
+		break;
+	default:
+		hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
+		break;
+	}
+
+	kfree(action);
 
-	asus_schedule_work(led);
+	/* Re-schedule if there are more pending actions */
+	spin_lock_irqsave(&worker->lock, flags);
+	if (!list_empty(&worker->actions))
+		schedule_work(&worker->work);
+	spin_unlock_irqrestore(&worker->lock, flags);
 }
 
-static void asus_kbd_backlight_work(struct work_struct *work)
+static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
 {
-	struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
-	u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
-	int ret;
+	drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
+	if (!drvdata->worker)
+		return -ENOMEM;
+
+	drvdata->worker->removed = false;
+	drvdata->worker->hdev = hdev;
+	INIT_LIST_HEAD(&drvdata->worker->actions);
+
+	INIT_WORK(&drvdata->worker->work, asus_work);
+	spin_lock_init(&drvdata->worker->lock);
+
+	return 0;
+}
+
+static void asus_worker_stop(struct asus_worker *worker)
+{
+	struct asus_work_action *action, *tmp;
 	unsigned long flags;
 
-	spin_lock_irqsave(&led->lock, flags);
-	buf[4] = led->brightness;
-	spin_unlock_irqrestore(&led->lock, flags);
+	spin_lock_irqsave(&worker->lock, flags);
+	worker->removed = true;
+	list_for_each_entry_safe(action, tmp, &worker->actions, node) {
+		list_del(&action->node);
+		kfree(action);
+	}
+	spin_unlock_irqrestore(&worker->lock, flags);
 
-	ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
-	if (ret < 0)
-		hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
+	cancel_work_sync(&worker->work);
 }
 
 /*
@@ -760,23 +951,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 			le16_to_cpu(udev->descriptor.idProduct));
 	}
 
-	drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
-					      sizeof(struct asus_kbd_leds),
-					      GFP_KERNEL);
-	if (!drvdata->kbd_backlight)
-		return -ENOMEM;
-
-	drvdata->kbd_backlight->removed = false;
-	drvdata->kbd_backlight->brightness = 0;
-	drvdata->kbd_backlight->hdev = hdev;
-	drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
-	INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
-	spin_lock_init(&drvdata->kbd_backlight->lock);
-
-	ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
+	drvdata->listener.brightness_set = asus_kbd_backlight_set;
+	ret = asus_hid_register_listener(&drvdata->listener);
 	if (ret < 0) {
-		/* No need to have this still around */
-		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
+		hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
+		drvdata->listener.brightness_set = NULL;
 	}
 
 	return ret;
@@ -965,11 +1144,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 		}
 	}
 
-	if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
-		drvdata->fn_lock = true;
-		INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
-		asus_kbd_set_fn_lock(hdev, true);
-	}
+	if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
+	    (asus_kbd_fn_lock_set(drvdata, true)))
+		hid_warn(hdev, "Error while setting FN lock to ON\n");
 
 	if (drvdata->tp) {
 		int ret;
@@ -1004,11 +1181,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 
 	drvdata->input = input;
 
-	if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
-		drvdata->fn_lock = true;
-		INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
-		asus_kbd_set_fn_lock(hdev, true);
-	}
+	if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
+	    (asus_kbd_fn_lock_set(drvdata, true)))
+		hid_warn(hdev, "Error while setting FN lock to ON\n");
 
 	return 0;
 }
@@ -1171,20 +1346,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
 static int __maybe_unused asus_resume(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
-	int ret = 0;
 
-	if (drvdata->kbd_backlight) {
-		const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
-				drvdata->kbd_backlight->brightness };
-		ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
-		if (ret < 0) {
-			hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
-			goto asus_resume_err;
-		}
-	}
+	/*
+	 * If we have a backlight listener registered, restore the previous state,
+	 * in case of error do not fail: most models restore the backlight
+	 * automatically, and the error is non-fatal.
+	 */
+	if (drvdata->listener.brightness_set)
+		asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
 
-asus_resume_err:
-	return ret;
+	return 0;
 }
 
 static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
@@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			is_vendor = true;
 	}
 
+	ret = asus_worker_create(hdev, drvdata);
+	if (ret) {
+		hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
+		return ret;
+	}
+
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "Asus hw start failed: %d\n", ret);
@@ -1343,6 +1520,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	return 0;
 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;
 }
@@ -1350,21 +1531,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static void asus_remove(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
-	unsigned long flags;
-
-	if (drvdata->kbd_backlight) {
-		asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
-
-		spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
-		drvdata->kbd_backlight->removed = true;
-		spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
-
-		cancel_work_sync(&drvdata->kbd_backlight->work);
-	}
 
-	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
-		cancel_work_sync(&drvdata->fn_lock_sync_work);
+	if (drvdata->listener.brightness_set)
+		asus_hid_unregister_listener(&drvdata->listener);
 
+	asus_worker_stop(drvdata->worker);
 	hid_hw_stop(hdev);
 }
 
-- 
2.47.3

Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in atomic context due to implicit GFP_KERNEL allocations in HID event handlers.
- [High] Data race on shared `hid_report` object when calling `hid_report_raw_event()` asynchronously from the workqueue.
- [High] Use-After-Free due to un-canceled workqueue items if `hid_hw_start()` fails during probe.
--

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

    HID: asus: refactor the two workqueues and init sequence
    
    Join the two workqueues into one reusing the stopping mechanism
    of the brightness workqueue, use the joined workqueue to also
    move the asus_wmi_send_event() sleeping call away from atomic
    context and add a size check in asus_raw_event().

    Link: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/

> 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
[ ... ]
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
[ ... ]
> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
> +{
> +	struct asus_work_action *action;
> +	unsigned long flags;
> +
> +	action = kzalloc_obj(struct asus_work_action);

[Severity: High]
Does this allocation sleep in an atomic context?

Because kzalloc_obj() implicitly uses default_gfp() which resolves to
GFP_KERNEL, this introduces a blocking allocation. Since
asus_kbd_fn_lock_set() is called directly from the raw HID event handler
asus_event() which runs in an atomic context, this might trigger kernel
panics or deadlocks under load.

> +	if (!action)
> +		return -ENOMEM;
[ ... ]
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> +				 size_t report_size)
> +{
> +	struct asus_work_action *action;
> +	unsigned long flags;
> +
> +	if (report_size > FEATURE_KBD_REPORT_SIZE) {
> +		hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> +		return -EINVAL;
>  	}
>  
> +	action = kzalloc_obj(struct asus_work_action);

[Severity: High]
Similarly, does this kzalloc_obj() introduce a blocking allocation in an
atomic context?

asus_kbd_wmi_fan_send() is invoked from the asus_raw_event() handler, which
also executes in an atomic context.

> +	if (!action)
> +		return -ENOMEM;
[ ... ]
> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
[ ... ]
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
> +
> +	ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
[ ... ]
> +	/* Fallback: pass the raw event to the HID core */
> +	hid_report_raw_event(hdev, HID_INPUT_REPORT,
> +			     data->report_data, data->report_size,
> +			     data->report_size, 1);

[Severity: High]
Can this data race with native HID IRQ delivery?

If notifying the WMI system of a fan key event fails, the deferred work
handler asus_kbd_wmi_fan() falls back to injecting the raw event into the
HID core using hid_report_raw_event().

However, hid_report_raw_event() parses the report and mutates the shared
report->field state arrays without taking any locks. Because the workqueue
bypasses the hid->driver_input_lock semaphore used by the native IRQ path,
a concurrent hardware interrupt targeting the same report ID could race with
this workqueue. This might lead to unsynchronized reads and writes to
field->value and field->new_value, potentially corrupting the core HID
report state.

> +}
[ ... ]
> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
[ ... ]
> +	ret = asus_worker_create(hdev, drvdata);
> +	if (ret) {
> +		hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret) {
>  		hid_err(hdev, "Asus hw start failed: %d\n", ret);
>  		return ret;
>  	}

[Severity: High]
Could this leak a pending work item and cause a use-after-free if
hid_hw_start() fails?

During driver probe, asus_worker_create() creates a devres-managed worker
struct. Next, hid_hw_start() connects the device, invoking
asus_input_configured(), which synchronously schedules a FN_LOCK_SYNC
work item on the global workqueue.

If hid_hw_start() subsequently fails (e.g., due to an input registration
error later in the sequence), asus_probe() captures the error and returns
it directly, bypassing the err_stop_hw label. Consequently, it fails to
call asus_worker_stop() or cancel_work_sync().

The devres framework will automatically free the worker memory, but the
scheduled work item remains active. When the system workqueue eventually
executes the pending work, it will dereference the freed memory.

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

Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence

[Antheas Kapenekakis]

On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> Multiple issues have been found within the hid-asus driver:
> - unchecked size in asus_raw_event()
> - unclean teardown of asus_probe on failure
> - possible use-after-free in asus_probe
> - multiple workqueue used for jobs where one was enough
> - sleeping calls in atomic context
> - packets of incorrect size being sent to the keyboard controller
>
> Join the two workqueues into one reusing the stopping mechanism
> of the brightness workqueue, use the joined workqueue to also
> move the asus_wmi_send_event() sleeping call away from atomic
> context and add a size check in asus_raw_event().
>
> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
> Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
> Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")

I'm not sure b34b5945a769 caused an issue, perhaps drop it from fixes.
Does f631011e36b8 do a WMI from a hid interrupt? If not, perhaps it
does not need to be changed

But moreso, this patch is too large and will take a bit to review. Can
you send it separately after the rest of the series goes through
unless users report warnings?

Antheas

> Reported-by: sahiko-bot@kernel.org
> Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 393 +++++++++++++++++++++++++++++------------
>  1 file changed, 282 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d34d74df3dc0..05ca6665e0a4 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>
>  #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
>
> -struct asus_kbd_leds {
> -       struct asus_hid_listener listener;
> +enum asus_work_action_type {
> +       FN_LOCK_SYNC,
> +       BRIGHTNESS_SET,
> +       WMI_FAN,
> +};
> +
> +struct hid_raw_event_data {
> +       u8 report_data[FEATURE_KBD_REPORT_SIZE];
> +       size_t report_size;
> +};
> +
> +struct asus_work_action {
> +       struct list_head node;
> +       enum asus_work_action_type type;
> +       union {
> +               /* Data for BRIGHTNESS_SET */
> +               unsigned int brightness;
> +
> +               /* Data for FN_LOCK_SYNC */
> +               bool fn_lock;
> +
> +               /* Data for WMI_FAN */
> +               struct hid_raw_event_data fan_hid_data;
> +       } data;
> +};
> +
> +struct asus_worker {
>         struct hid_device *hdev;
>         struct work_struct work;
> -       unsigned int brightness;
> +       struct list_head actions;
>         spinlock_t lock;
>         bool removed;
>  };
> @@ -133,7 +158,8 @@ struct asus_drvdata {
>         struct hid_device *hdev;
>         struct input_dev *input;
>         struct input_dev *tp_kbd_input;
> -       struct asus_kbd_leds *kbd_backlight;
> +       struct asus_worker *worker;
> +       unsigned int kbd_backlight_brightness;
>         const struct asus_touchpad_info *tp;
>         struct power_supply *battery;
>         struct power_supply_desc battery_desc;
> @@ -141,7 +167,7 @@ struct asus_drvdata {
>         int battery_stat;
>         bool battery_in_query;
>         unsigned long battery_next_query;
> -       struct work_struct fn_lock_sync_work;
> +       struct asus_hid_listener listener;
>         bool fn_lock;
>  };
>
> @@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
>         FEATURE_KBD_LED_REPORT_ID2
>  };
>
> +/*
> + * Send events to asus-wmi driver for handling special keys
> + */
> +static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
> +{
> +       int err;
> +       u32 retval;
> +
> +       err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> +                                      ASUS_WMI_METHODID_NOTIF, code, &retval);
> +       if (err) {
> +               pr_warn("Failed to notify asus-wmi: %d\n", err);
> +               return err;
> +       }
> +
> +       if (retval != 0) {
> +               pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  static void asus_report_contact_down(struct asus_drvdata *drvdat,
>                 int toolType, u8 *data)
>  {
> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
>  }
>
>  /*
> - * Send events to asus-wmi driver for handling special keys
> + * Used in atomic contexts to schedule work involving sleeps operations or
> + * asus-wmi interactions.
> + *
> + * Caller is responsible to store relevant data in the structure to carry out
> + * the required action.
> + *
> + * This function must be called while the spin lock protecting the workqueue
> + * is already being held.
>   */
> -static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
> +static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
>  {
> -       int err;
> -       u32 retval;
> -
> -       err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> -                                      ASUS_WMI_METHODID_NOTIF, code, &retval);
> -       if (err) {
> -               pr_warn("Failed to notify asus-wmi: %d\n", err);
> -               return err;
> +       if (worker->removed) {
> +               kfree(action);
> +               return;
>         }
>
> -       if (retval != 0) {
> -               pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
> -               return -EIO;
> +       list_add_tail(&action->node, &worker->actions);
> +       schedule_work(&worker->work);
> +}
> +
> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
> +{
> +       struct asus_work_action *action;
> +       unsigned long flags;
> +
> +       action = kzalloc_obj(struct asus_work_action);
> +       if (!action)
> +               return -ENOMEM;
> +
> +       drvdata->fn_lock = enabled;
> +       action->type = FN_LOCK_SYNC;
> +       action->data.fn_lock = drvdata->fn_lock;
> +       INIT_LIST_HEAD(&action->node);
> +
> +       spin_lock_irqsave(&drvdata->worker->lock, flags);
> +       asus_worker_schedule(drvdata->worker, action);
> +       spin_unlock_irqrestore(&drvdata->worker->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
> +                                size_t report_size)
> +{
> +       struct asus_work_action *action;
> +       unsigned long flags;
> +
> +       if (report_size > FEATURE_KBD_REPORT_SIZE) {
> +               hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
> +               return -EINVAL;
>         }
>
> +       action = kzalloc_obj(struct asus_work_action);
> +       if (!action)
> +               return -ENOMEM;
> +
> +       action->type = WMI_FAN;
> +       action->data.fan_hid_data.report_size = report_size;
> +       memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
> +       INIT_LIST_HEAD(&action->node);
> +
> +       spin_lock_irqsave(&drvdata->worker->lock, flags);
> +       asus_worker_schedule(drvdata->worker, action);
> +       spin_unlock_irqrestore(&drvdata->worker->lock, flags);
> +
>         return 0;
>  }
>
> @@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>                       struct hid_usage *usage, __s32 value)
>  {
>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +       int ret;
>
>         if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>             (usage->hid & HID_USAGE) != 0x00 &&
> @@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>                         return !asus_hid_event(ASUS_EV_BRTTOGGLE);
>                 case KEY_FN_ESC:
>                         if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> -                               drvdata->fn_lock = !drvdata->fn_lock;
> -                               schedule_work(&drvdata->fn_lock_sync_work);
> +                               ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
> +                               if (ret) {
> +                                       hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
> +                                       return ret;
> +                               }
>                         }
>                         break;
>                 }
> @@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
>                 struct hid_report *report, u8 *data, int size)
>  {
>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +       int ret;
> +
> +       if (size < 2) {
> +               hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
> +               return 0;
> +       }
>
>         if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>                 return asus_report_battery(drvdata, data, size);
> @@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
>                          * pass to userspace so it can implement its own fan control.
>                          */
>                         if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
> -                               int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
> +                               ret = asus_kbd_wmi_fan_send(drvdata, data, size);
>
> -                               if (ret == 0) {
> -                                       /* Successfully handled by asus-wmi, block event */
> +                               /* if execution deferred successfully block event */
> +                               if (ret == 0)
>                                         return -1;
> -                               }
>
> -                               /*
> -                                * Warn if asus-wmi failed (but not if it's unavailable).
> -                                * Let the event reach userspace in all failure cases.
> -                                */
> -                               if (ret != -ENODEV)
> -                                       hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
> +                               return ret;
>                         }
>
>                         /*
> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
>         return 0;
>  }
>
> -static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
> +static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>  {
> -       u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
> +       const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
> +       int ret;
>
> -       return asus_kbd_set_report(hdev, buf, sizeof(buf));
> +       ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
>  }
>
> -static void asus_sync_fn_lock(struct work_struct *work)
> +static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
>  {
> -       struct asus_drvdata *drvdata =
> -       container_of(work, struct asus_drvdata, fn_lock_sync_work);
> +       const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
> +               FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
> +       };
> +       int ret;
>
> -       asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
> +       ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>  }
>
> -static void asus_schedule_work(struct asus_kbd_leds *led)
> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
>  {
> +       struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +       int ret;
> +
> +       ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
> +
> +       /*
> +        * Warn if asus-wmi failed (but not if it's unavailable).
> +        * Let the event reach userspace in all failure cases.
> +        */
> +       switch (ret) {
> +       case -ENODEV:
> +               break;
> +       case 0:
> +               return;
> +       default:
> +               hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
> +               break;
> +       }
> +
> +       /* Fallback: pass the raw event to the HID core */
> +       hid_report_raw_event(hdev, HID_INPUT_REPORT,
> +                            data->report_data, data->report_size,
> +                            data->report_size, 1);
> +}
> +
> +static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
> +{
> +       struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
> +       struct asus_worker *worker = drvdata->worker;
> +       struct asus_work_action *action;
>         unsigned long flags;
>
> -       spin_lock_irqsave(&led->lock, flags);
> -       if (!led->removed)
> -               schedule_work(&led->work);
> -       spin_unlock_irqrestore(&led->lock, flags);
> +       drvdata->kbd_backlight_brightness = brightness;
> +
> +       action = kzalloc_obj(struct asus_work_action);
> +       if (!action) {
> +               hid_warn(drvdata->hdev, "Failed to allocate memory for backlight action\n");
> +               return;
> +       }
> +
> +       action->type = BRIGHTNESS_SET;
> +       action->data.brightness = brightness;
> +       INIT_LIST_HEAD(&action->node);
> +
> +       spin_lock_irqsave(&worker->lock, flags);
> +       asus_worker_schedule(worker, action);
> +       spin_unlock_irqrestore(&worker->lock, flags);
>  }
>
> -static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
> -                                  int brightness)
> +static void asus_work(struct work_struct *work)
>  {
> -       struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
> -                                                listener);
> +       struct asus_worker *worker = container_of(work, struct asus_worker, work);
> +       struct asus_work_action *action = NULL;
>         unsigned long flags;
>
> -       spin_lock_irqsave(&led->lock, flags);
> -       led->brightness = brightness;
> -       spin_unlock_irqrestore(&led->lock, flags);
> +       /* Save the action to be performed and clear the flag */
> +       spin_lock_irqsave(&worker->lock, flags);
> +       if (!list_empty(&worker->actions)) {
> +               action = list_first_entry(&worker->actions,
> +                                         struct asus_work_action, node);
> +               list_del(&action->node);
> +       }
> +       spin_unlock_irqrestore(&worker->lock, flags);
> +
> +       if (!action)
> +               return;
> +
> +       switch (action->type) {
> +       case BRIGHTNESS_SET:
> +               asus_kbd_set_brightness(worker->hdev, action->data.brightness);
> +               break;
> +       case FN_LOCK_SYNC:
> +               asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
> +               break;
> +       case WMI_FAN:
> +               asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
> +               break;
> +       default:
> +               hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
> +               break;
> +       }
> +
> +       kfree(action);
>
> -       asus_schedule_work(led);
> +       /* Re-schedule if there are more pending actions */
> +       spin_lock_irqsave(&worker->lock, flags);
> +       if (!list_empty(&worker->actions))
> +               schedule_work(&worker->work);
> +       spin_unlock_irqrestore(&worker->lock, flags);
>  }
>
> -static void asus_kbd_backlight_work(struct work_struct *work)
> +static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
>  {
> -       struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
> -       u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
> -       int ret;
> +       drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
> +       if (!drvdata->worker)
> +               return -ENOMEM;
> +
> +       drvdata->worker->removed = false;
> +       drvdata->worker->hdev = hdev;
> +       INIT_LIST_HEAD(&drvdata->worker->actions);
> +
> +       INIT_WORK(&drvdata->worker->work, asus_work);
> +       spin_lock_init(&drvdata->worker->lock);
> +
> +       return 0;
> +}
> +
> +static void asus_worker_stop(struct asus_worker *worker)
> +{
> +       struct asus_work_action *action, *tmp;
>         unsigned long flags;
>
> -       spin_lock_irqsave(&led->lock, flags);
> -       buf[4] = led->brightness;
> -       spin_unlock_irqrestore(&led->lock, flags);
> +       spin_lock_irqsave(&worker->lock, flags);
> +       worker->removed = true;
> +       list_for_each_entry_safe(action, tmp, &worker->actions, node) {
> +               list_del(&action->node);
> +               kfree(action);
> +       }
> +       spin_unlock_irqrestore(&worker->lock, flags);
>
> -       ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
> -       if (ret < 0)
> -               hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> +       cancel_work_sync(&worker->work);
>  }
>
>  /*
> @@ -760,23 +951,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>                         le16_to_cpu(udev->descriptor.idProduct));
>         }
>
> -       drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> -                                             sizeof(struct asus_kbd_leds),
> -                                             GFP_KERNEL);
> -       if (!drvdata->kbd_backlight)
> -               return -ENOMEM;
> -
> -       drvdata->kbd_backlight->removed = false;
> -       drvdata->kbd_backlight->brightness = 0;
> -       drvdata->kbd_backlight->hdev = hdev;
> -       drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
> -       INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
> -       spin_lock_init(&drvdata->kbd_backlight->lock);
> -
> -       ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
> +       drvdata->listener.brightness_set = asus_kbd_backlight_set;
> +       ret = asus_hid_register_listener(&drvdata->listener);
>         if (ret < 0) {
> -               /* No need to have this still around */
> -               devm_kfree(&hdev->dev, drvdata->kbd_backlight);
> +               hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
> +               drvdata->listener.brightness_set = NULL;
>         }
>
>         return ret;
> @@ -965,11 +1144,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>                 }
>         }
>
> -       if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> -               drvdata->fn_lock = true;
> -               INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
> -               asus_kbd_set_fn_lock(hdev, true);
> -       }
> +       if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
> +           (asus_kbd_fn_lock_set(drvdata, true)))
> +               hid_warn(hdev, "Error while setting FN lock to ON\n");
>
>         if (drvdata->tp) {
>                 int ret;
> @@ -1004,11 +1181,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>
>         drvdata->input = input;
>
> -       if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> -               drvdata->fn_lock = true;
> -               INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
> -               asus_kbd_set_fn_lock(hdev, true);
> -       }
> +       if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
> +           (asus_kbd_fn_lock_set(drvdata, true)))
> +               hid_warn(hdev, "Error while setting FN lock to ON\n");
>
>         return 0;
>  }
> @@ -1171,20 +1346,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>  {
>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> -       int ret = 0;
>
> -       if (drvdata->kbd_backlight) {
> -               const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
> -                               drvdata->kbd_backlight->brightness };
> -               ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> -               if (ret < 0) {
> -                       hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> -                       goto asus_resume_err;
> -               }
> -       }
> +       /*
> +        * If we have a backlight listener registered, restore the previous state,
> +        * in case of error do not fail: most models restore the backlight
> +        * automatically, and the error is non-fatal.
> +        */
> +       if (drvdata->listener.brightness_set)
> +               asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>
> -asus_resume_err:
> -       return ret;
> +       return 0;
>  }
>
>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                         is_vendor = true;
>         }
>
> +       ret = asus_worker_create(hdev, drvdata);
> +       if (ret) {
> +               hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
> +               return ret;
> +       }
> +
>         ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>         if (ret) {
>                 hid_err(hdev, "Asus hw start failed: %d\n", ret);
> @@ -1343,6 +1520,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
>         return 0;
>  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;
>  }
> @@ -1350,21 +1531,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  static void asus_remove(struct hid_device *hdev)
>  {
>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> -       unsigned long flags;
> -
> -       if (drvdata->kbd_backlight) {
> -               asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
> -
> -               spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
> -               drvdata->kbd_backlight->removed = true;
> -               spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
> -
> -               cancel_work_sync(&drvdata->kbd_backlight->work);
> -       }
>
> -       if (drvdata->quirks & QUIRK_HID_FN_LOCK)
> -               cancel_work_sync(&drvdata->fn_lock_sync_work);
> +       if (drvdata->listener.brightness_set)
> +               asus_hid_unregister_listener(&drvdata->listener);
>
> +       asus_worker_stop(drvdata->worker);
>         hid_hw_stop(hdev);
>  }
>
> --
> 2.47.3
>
>

Re: [PATCH v4 1/5] HID: asus: refactor the two workqueues and init sequence

[Denis Benato]

On 6/15/26 23:49, Antheas Kapenekakis wrote:
> On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>> Multiple issues have been found within the hid-asus driver:
>> - unchecked size in asus_raw_event()
>> - unclean teardown of asus_probe on failure
>> - possible use-after-free in asus_probe
>> - multiple workqueue used for jobs where one was enough
>> - sleeping calls in atomic context
>> - packets of incorrect size being sent to the keyboard controller
>>
>> Join the two workqueues into one reusing the stopping mechanism
>> of the brightness workqueue, use the joined workqueue to also
>> move the asus_wmi_send_event() sleeping call away from atomic
>> context and add a size check in asus_raw_event().
>>
>> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
>> Fixes: 1489a34e97ef ("HID: asus: Implement Fn+F5 fan control key handler")
>> Fixes: b34b5945a769 ("HID: asus: listen to the asus-wmi brightness device instead of creating one")
> I'm not sure b34b5945a769 caused an issue, perhaps drop it from fixes.
> Does f631011e36b8 do a WMI from a hid interrupt? If not, perhaps it
> does not need to be changed
Maybe I confused some tags.
> But moreso, this patch is too large and will take a bit to review. Can
> you send it separately after the rest of the series goes through
> unless users report warnings?
I agree on it being large. I tried doing it by pieces, but got a bunch of compile
errors.

I ended up deciding trying it when after a few hours I had to modify code
I did not want to modify (just yet) just for the thing to compile.

Probably result of frustration more than technical limitation...

I am fine with waiting and will be unavailable for a few days,
in the meanwhile I hope in some input from hid maintainers
too.
> Antheas
>
>> Reported-by: sahiko-bot@kernel.org
>> Closes: https://lore.kernel.org/all/20260613154732.60A4B1F000E9@smtp.kernel.org/
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> ---
>>  drivers/hid/hid-asus.c | 393 +++++++++++++++++++++++++++++------------
>>  1 file changed, 282 insertions(+), 111 deletions(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index d34d74df3dc0..05ca6665e0a4 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -109,11 +109,36 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>
>>  #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
>>
>> -struct asus_kbd_leds {
>> -       struct asus_hid_listener listener;
>> +enum asus_work_action_type {
>> +       FN_LOCK_SYNC,
>> +       BRIGHTNESS_SET,
>> +       WMI_FAN,
>> +};
>> +
>> +struct hid_raw_event_data {
>> +       u8 report_data[FEATURE_KBD_REPORT_SIZE];
>> +       size_t report_size;
>> +};
>> +
>> +struct asus_work_action {
>> +       struct list_head node;
>> +       enum asus_work_action_type type;
>> +       union {
>> +               /* Data for BRIGHTNESS_SET */
>> +               unsigned int brightness;
>> +
>> +               /* Data for FN_LOCK_SYNC */
>> +               bool fn_lock;
>> +
>> +               /* Data for WMI_FAN */
>> +               struct hid_raw_event_data fan_hid_data;
>> +       } data;
>> +};
>> +
>> +struct asus_worker {
>>         struct hid_device *hdev;
>>         struct work_struct work;
>> -       unsigned int brightness;
>> +       struct list_head actions;
>>         spinlock_t lock;
>>         bool removed;
>>  };
>> @@ -133,7 +158,8 @@ struct asus_drvdata {
>>         struct hid_device *hdev;
>>         struct input_dev *input;
>>         struct input_dev *tp_kbd_input;
>> -       struct asus_kbd_leds *kbd_backlight;
>> +       struct asus_worker *worker;
>> +       unsigned int kbd_backlight_brightness;
>>         const struct asus_touchpad_info *tp;
>>         struct power_supply *battery;
>>         struct power_supply_desc battery_desc;
>> @@ -141,7 +167,7 @@ struct asus_drvdata {
>>         int battery_stat;
>>         bool battery_in_query;
>>         unsigned long battery_next_query;
>> -       struct work_struct fn_lock_sync_work;
>> +       struct asus_hid_listener listener;
>>         bool fn_lock;
>>  };
>>
>> @@ -211,6 +237,29 @@ static const u8 asus_report_id_init[] = {
>>         FEATURE_KBD_LED_REPORT_ID2
>>  };
>>
>> +/*
>> + * Send events to asus-wmi driver for handling special keys
>> + */
>> +static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
>> +{
>> +       int err;
>> +       u32 retval;
>> +
>> +       err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>> +                                      ASUS_WMI_METHODID_NOTIF, code, &retval);
>> +       if (err) {
>> +               pr_warn("Failed to notify asus-wmi: %d\n", err);
>> +               return err;
>> +       }
>> +
>> +       if (retval != 0) {
>> +               pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>> +               return -EIO;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static void asus_report_contact_down(struct asus_drvdata *drvdat,
>>                 int toolType, u8 *data)
>>  {
>> @@ -331,25 +380,71 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
>>  }
>>
>>  /*
>> - * Send events to asus-wmi driver for handling special keys
>> + * Used in atomic contexts to schedule work involving sleeps operations or
>> + * asus-wmi interactions.
>> + *
>> + * Caller is responsible to store relevant data in the structure to carry out
>> + * the required action.
>> + *
>> + * This function must be called while the spin lock protecting the workqueue
>> + * is already being held.
>>   */
>> -static int asus_wmi_send_event(struct asus_drvdata *drvdata, u8 code)
>> +static void asus_worker_schedule(struct asus_worker *worker, struct asus_work_action *action)
>>  {
>> -       int err;
>> -       u32 retval;
>> -
>> -       err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>> -                                      ASUS_WMI_METHODID_NOTIF, code, &retval);
>> -       if (err) {
>> -               pr_warn("Failed to notify asus-wmi: %d\n", err);
>> -               return err;
>> +       if (worker->removed) {
>> +               kfree(action);
>> +               return;
>>         }
>>
>> -       if (retval != 0) {
>> -               pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>> -               return -EIO;
>> +       list_add_tail(&action->node, &worker->actions);
>> +       schedule_work(&worker->work);
>> +}
>> +
>> +static int asus_kbd_fn_lock_set(struct asus_drvdata *drvdata, bool enabled)
>> +{
>> +       struct asus_work_action *action;
>> +       unsigned long flags;
>> +
>> +       action = kzalloc_obj(struct asus_work_action);
>> +       if (!action)
>> +               return -ENOMEM;
>> +
>> +       drvdata->fn_lock = enabled;
>> +       action->type = FN_LOCK_SYNC;
>> +       action->data.fn_lock = drvdata->fn_lock;
>> +       INIT_LIST_HEAD(&action->node);
>> +
>> +       spin_lock_irqsave(&drvdata->worker->lock, flags);
>> +       asus_worker_schedule(drvdata->worker, action);
>> +       spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>> +
>> +       return 0;
>> +}
>> +
>> +static int asus_kbd_wmi_fan_send(struct asus_drvdata *drvdata, u8 *report_data,
>> +                                size_t report_size)
>> +{
>> +       struct asus_work_action *action;
>> +       unsigned long flags;
>> +
>> +       if (report_size > FEATURE_KBD_REPORT_SIZE) {
>> +               hid_err(drvdata->hdev, "Invalid report size for fan event: %zu\n", report_size);
>> +               return -EINVAL;
>>         }
>>
>> +       action = kzalloc_obj(struct asus_work_action);
>> +       if (!action)
>> +               return -ENOMEM;
>> +
>> +       action->type = WMI_FAN;
>> +       action->data.fan_hid_data.report_size = report_size;
>> +       memcpy(action->data.fan_hid_data.report_data, report_data, report_size);
>> +       INIT_LIST_HEAD(&action->node);
>> +
>> +       spin_lock_irqsave(&drvdata->worker->lock, flags);
>> +       asus_worker_schedule(drvdata->worker, action);
>> +       spin_unlock_irqrestore(&drvdata->worker->lock, flags);
>> +
>>         return 0;
>>  }
>>
>> @@ -357,6 +452,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>>                       struct hid_usage *usage, __s32 value)
>>  {
>>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +       int ret;
>>
>>         if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>>             (usage->hid & HID_USAGE) != 0x00 &&
>> @@ -375,8 +471,11 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>>                         return !asus_hid_event(ASUS_EV_BRTTOGGLE);
>>                 case KEY_FN_ESC:
>>                         if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> -                               drvdata->fn_lock = !drvdata->fn_lock;
>> -                               schedule_work(&drvdata->fn_lock_sync_work);
>> +                               ret = asus_kbd_fn_lock_set(drvdata, !drvdata->fn_lock);
>> +                               if (ret) {
>> +                                       hid_err(hdev, "Error while toggling FN lock: %d\n", ret);
>> +                                       return ret;
>> +                               }
>>                         }
>>                         break;
>>                 }
>> @@ -389,6 +488,12 @@ static int asus_raw_event(struct hid_device *hdev,
>>                 struct hid_report *report, u8 *data, int size)
>>  {
>>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +       int ret;
>> +
>> +       if (size < 2) {
>> +               hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
>> +               return 0;
>> +       }
>>
>>         if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>>                 return asus_report_battery(drvdata, data, size);
>> @@ -414,19 +519,13 @@ static int asus_raw_event(struct hid_device *hdev,
>>                          * pass to userspace so it can implement its own fan control.
>>                          */
>>                         if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
>> -                               int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
>> +                               ret = asus_kbd_wmi_fan_send(drvdata, data, size);
>>
>> -                               if (ret == 0) {
>> -                                       /* Successfully handled by asus-wmi, block event */
>> +                               /* if execution deferred successfully block event */
>> +                               if (ret == 0)
>>                                         return -1;
>> -                               }
>>
>> -                               /*
>> -                                * Warn if asus-wmi failed (but not if it's unavailable).
>> -                                * Let the event reach userspace in all failure cases.
>> -                                */
>> -                               if (ret != -ENODEV)
>> -                                       hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
>> +                               return ret;
>>                         }
>>
>>                         /*
>> @@ -569,59 +668,151 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
>>         return 0;
>>  }
>>
>> -static int asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>> +static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
>>  {
>> -       u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
>> +       const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
>> +       int ret;
>>
>> -       return asus_kbd_set_report(hdev, buf, sizeof(buf));
>> +       ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>> +       if (ret < 0)
>> +               hid_err(hdev, "Asus failed to set fn lock: %d\n", ret);
>>  }
>>
>> -static void asus_sync_fn_lock(struct work_struct *work)
>> +static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
>>  {
>> -       struct asus_drvdata *drvdata =
>> -       container_of(work, struct asus_drvdata, fn_lock_sync_work);
>> +       const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
>> +               FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
>> +       };
>> +       int ret;
>>
>> -       asus_kbd_set_fn_lock(drvdata->hdev, drvdata->fn_lock);
>> +       ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>> +       if (ret < 0)
>> +               hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>  }
>>
>> -static void asus_schedule_work(struct asus_kbd_leds *led)
>> +static void asus_kbd_wmi_fan(struct hid_device *hdev, struct hid_raw_event_data *data)
>>  {
>> +       struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +       int ret;
>> +
>> +       ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
>> +
>> +       /*
>> +        * Warn if asus-wmi failed (but not if it's unavailable).
>> +        * Let the event reach userspace in all failure cases.
>> +        */
>> +       switch (ret) {
>> +       case -ENODEV:
>> +               break;
>> +       case 0:
>> +               return;
>> +       default:
>> +               hid_warn(hdev, "Failed to notify asus-wmi: %d\n", ret);
>> +               break;
>> +       }
>> +
>> +       /* Fallback: pass the raw event to the HID core */
>> +       hid_report_raw_event(hdev, HID_INPUT_REPORT,
>> +                            data->report_data, data->report_size,
>> +                            data->report_size, 1);
>> +}
>> +
>> +static void asus_kbd_backlight_set(struct asus_hid_listener *listener, int brightness)
>> +{
>> +       struct asus_drvdata *drvdata = container_of(listener, struct asus_drvdata, listener);
>> +       struct asus_worker *worker = drvdata->worker;
>> +       struct asus_work_action *action;
>>         unsigned long flags;
>>
>> -       spin_lock_irqsave(&led->lock, flags);
>> -       if (!led->removed)
>> -               schedule_work(&led->work);
>> -       spin_unlock_irqrestore(&led->lock, flags);
>> +       drvdata->kbd_backlight_brightness = brightness;
>> +
>> +       action = kzalloc_obj(struct asus_work_action);
>> +       if (!action) {
>> +               hid_warn(drvdata->hdev, "Failed to allocate memory for backlight action\n");
>> +               return;
>> +       }
>> +
>> +       action->type = BRIGHTNESS_SET;
>> +       action->data.brightness = brightness;
>> +       INIT_LIST_HEAD(&action->node);
>> +
>> +       spin_lock_irqsave(&worker->lock, flags);
>> +       asus_worker_schedule(worker, action);
>> +       spin_unlock_irqrestore(&worker->lock, flags);
>>  }
>>
>> -static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
>> -                                  int brightness)
>> +static void asus_work(struct work_struct *work)
>>  {
>> -       struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
>> -                                                listener);
>> +       struct asus_worker *worker = container_of(work, struct asus_worker, work);
>> +       struct asus_work_action *action = NULL;
>>         unsigned long flags;
>>
>> -       spin_lock_irqsave(&led->lock, flags);
>> -       led->brightness = brightness;
>> -       spin_unlock_irqrestore(&led->lock, flags);
>> +       /* Save the action to be performed and clear the flag */
>> +       spin_lock_irqsave(&worker->lock, flags);
>> +       if (!list_empty(&worker->actions)) {
>> +               action = list_first_entry(&worker->actions,
>> +                                         struct asus_work_action, node);
>> +               list_del(&action->node);
>> +       }
>> +       spin_unlock_irqrestore(&worker->lock, flags);
>> +
>> +       if (!action)
>> +               return;
>> +
>> +       switch (action->type) {
>> +       case BRIGHTNESS_SET:
>> +               asus_kbd_set_brightness(worker->hdev, action->data.brightness);
>> +               break;
>> +       case FN_LOCK_SYNC:
>> +               asus_kbd_set_fn_lock(worker->hdev, action->data.fn_lock);
>> +               break;
>> +       case WMI_FAN:
>> +               asus_kbd_wmi_fan(worker->hdev, &action->data.fan_hid_data);
>> +               break;
>> +       default:
>> +               hid_err(worker->hdev, "Invalid action type: %d\n", action->type);
>> +               break;
>> +       }
>> +
>> +       kfree(action);
>>
>> -       asus_schedule_work(led);
>> +       /* Re-schedule if there are more pending actions */
>> +       spin_lock_irqsave(&worker->lock, flags);
>> +       if (!list_empty(&worker->actions))
>> +               schedule_work(&worker->work);
>> +       spin_unlock_irqrestore(&worker->lock, flags);
>>  }
>>
>> -static void asus_kbd_backlight_work(struct work_struct *work)
>> +static int asus_worker_create(struct hid_device *hdev, struct asus_drvdata *drvdata)
>>  {
>> -       struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
>> -       u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>> -       int ret;
>> +       drvdata->worker = devm_kzalloc(&hdev->dev, sizeof(struct asus_worker), GFP_KERNEL);
>> +       if (!drvdata->worker)
>> +               return -ENOMEM;
>> +
>> +       drvdata->worker->removed = false;
>> +       drvdata->worker->hdev = hdev;
>> +       INIT_LIST_HEAD(&drvdata->worker->actions);
>> +
>> +       INIT_WORK(&drvdata->worker->work, asus_work);
>> +       spin_lock_init(&drvdata->worker->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static void asus_worker_stop(struct asus_worker *worker)
>> +{
>> +       struct asus_work_action *action, *tmp;
>>         unsigned long flags;
>>
>> -       spin_lock_irqsave(&led->lock, flags);
>> -       buf[4] = led->brightness;
>> -       spin_unlock_irqrestore(&led->lock, flags);
>> +       spin_lock_irqsave(&worker->lock, flags);
>> +       worker->removed = true;
>> +       list_for_each_entry_safe(action, tmp, &worker->actions, node) {
>> +               list_del(&action->node);
>> +               kfree(action);
>> +       }
>> +       spin_unlock_irqrestore(&worker->lock, flags);
>>
>> -       ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
>> -       if (ret < 0)
>> -               hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>> +       cancel_work_sync(&worker->work);
>>  }
>>
>>  /*
>> @@ -760,23 +951,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>                         le16_to_cpu(udev->descriptor.idProduct));
>>         }
>>
>> -       drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>> -                                             sizeof(struct asus_kbd_leds),
>> -                                             GFP_KERNEL);
>> -       if (!drvdata->kbd_backlight)
>> -               return -ENOMEM;
>> -
>> -       drvdata->kbd_backlight->removed = false;
>> -       drvdata->kbd_backlight->brightness = 0;
>> -       drvdata->kbd_backlight->hdev = hdev;
>> -       drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
>> -       INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
>> -       spin_lock_init(&drvdata->kbd_backlight->lock);
>> -
>> -       ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
>> +       drvdata->listener.brightness_set = asus_kbd_backlight_set;
>> +       ret = asus_hid_register_listener(&drvdata->listener);
>>         if (ret < 0) {
>> -               /* No need to have this still around */
>> -               devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>> +               hid_err(hdev, "Unable to register kbd brightness listener: %d\n", ret);
>> +               drvdata->listener.brightness_set = NULL;
>>         }
>>
>>         return ret;
>> @@ -965,11 +1144,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>                 }
>>         }
>>
>> -       if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> -               drvdata->fn_lock = true;
>> -               INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>> -               asus_kbd_set_fn_lock(hdev, true);
>> -       }
>> +       if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
>> +           (asus_kbd_fn_lock_set(drvdata, true)))
>> +               hid_warn(hdev, "Error while setting FN lock to ON\n");
>>
>>         if (drvdata->tp) {
>>                 int ret;
>> @@ -1004,11 +1181,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>
>>         drvdata->input = input;
>>
>> -       if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> -               drvdata->fn_lock = true;
>> -               INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>> -               asus_kbd_set_fn_lock(hdev, true);
>> -       }
>> +       if ((drvdata->quirks & QUIRK_HID_FN_LOCK) &&
>> +           (asus_kbd_fn_lock_set(drvdata, true)))
>> +               hid_warn(hdev, "Error while setting FN lock to ON\n");
>>
>>         return 0;
>>  }
>> @@ -1171,20 +1346,16 @@ static int asus_start_multitouch(struct hid_device *hdev)
>>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>>  {
>>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> -       int ret = 0;
>>
>> -       if (drvdata->kbd_backlight) {
>> -               const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
>> -                               drvdata->kbd_backlight->brightness };
>> -               ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>> -               if (ret < 0) {
>> -                       hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>> -                       goto asus_resume_err;
>> -               }
>> -       }
>> +       /*
>> +        * If we have a backlight listener registered, restore the previous state,
>> +        * in case of error do not fail: most models restore the backlight
>> +        * automatically, and the error is non-fatal.
>> +        */
>> +       if (drvdata->listener.brightness_set)
>> +               asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>>
>> -asus_resume_err:
>> -       return ret;
>> +       return 0;
>>  }
>>
>>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>> @@ -1294,6 +1465,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                         is_vendor = true;
>>         }
>>
>> +       ret = asus_worker_create(hdev, drvdata);
>> +       if (ret) {
>> +               hid_warn(hdev, "Failed to initialize worker: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>>         ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>         if (ret) {
>>                 hid_err(hdev, "Asus hw start failed: %d\n", ret);
>> @@ -1343,6 +1520,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>
>>         return 0;
>>  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;
>>  }
>> @@ -1350,21 +1531,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  static void asus_remove(struct hid_device *hdev)
>>  {
>>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> -       unsigned long flags;
>> -
>> -       if (drvdata->kbd_backlight) {
>> -               asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
>> -
>> -               spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
>> -               drvdata->kbd_backlight->removed = true;
>> -               spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
>> -
>> -               cancel_work_sync(&drvdata->kbd_backlight->work);
>> -       }
>>
>> -       if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>> -               cancel_work_sync(&drvdata->fn_lock_sync_work);
>> +       if (drvdata->listener.brightness_set)
>> +               asus_hid_unregister_listener(&drvdata->listener);
>>
>> +       asus_worker_stop(drvdata->worker);
>>         hid_hw_stop(hdev);
>>  }
>>
>> --
>> 2.47.3
>>
>>

[PATCH v4 2/5] HID: asus: remove unnecessary OOM message

[Denis Benato]

If devm_kzalloc fails an allocation error is already being reported:
there is no need for the driver to repeat it, moreover if this were
new code checkpatch.pl would throw a warning.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 05ca6665e0a4..bbf964b12c16 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1377,10 +1377,8 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	int ret;
 
 	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
-	if (drvdata == NULL) {
-		hid_err(hdev, "Can't alloc Asus descriptor\n");
+	if (drvdata == NULL)
 		return -ENOMEM;
-	}
 
 	hid_set_drvdata(hdev, drvdata);
 
-- 
2.47.3

Re: [PATCH v4 2/5] HID: asus: remove unnecessary OOM message

[Antheas Kapenekakis]

On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> If devm_kzalloc fails an allocation error is already being reported:
> there is no need for the driver to repeat it, moreover if this were
> new code checkpatch.pl would throw a warning.

Perhaps something more of:

HID: asus: remove extraneous allocation warning

If devm_kzalloc fails an allocation error is already being reported.
Therefore, there is no need for the driver to repeat it. For new code,
this behavior is disincentivized and checkpatch.pl reports a warning.

Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>

> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 05ca6665e0a4..bbf964b12c16 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1377,10 +1377,8 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         int ret;
>
>         drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> -       if (drvdata == NULL) {
> -               hid_err(hdev, "Can't alloc Asus descriptor\n");
> +       if (drvdata == NULL)
>                 return -ENOMEM;
> -       }
>
>         hid_set_drvdata(hdev, drvdata);
>
> --
> 2.47.3
>
>

[PATCH v4 3/5] HID: asus: fix a off-by-one issue making a check stricter that it needs to be

[Denis Benato]

In mcu_parse_version_string() a size validation for response is stricter
that it needs to be: relax the check by one byte.

Fixes: ("hid-asus: check ROG Ally MCU version and warn")
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bbf964b12c16..6896730efafc 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -835,7 +835,7 @@ static int mcu_parse_version_string(const u8 *response, size_t response_size)
 			dots++;
 	}
 
-	if (dots != 2 || p >= end || (p + 3) >= end)
+	if (dots != 2 || end - p < 3)
 		return -EINVAL;
 
 	memcpy(buf, p, 3);
-- 
2.47.3

Re: [PATCH v4 3/5] HID: asus: fix a off-by-one issue making a check stricter that it needs to be

[Antheas Kapenekakis]

On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> In mcu_parse_version_string() a size validation for response is stricter
> that it needs to be: relax the check by one byte.

"making a check stricter that it needs to be"

what check, you might need to reword the title

> Fixes: ("hid-asus: check ROG Ally MCU version and warn")
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index bbf964b12c16..6896730efafc 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -835,7 +835,7 @@ static int mcu_parse_version_string(const u8 *response, size_t response_size)
>                         dots++;
>         }
>
> -       if (dots != 2 || p >= end || (p + 3) >= end)
> +       if (dots != 2 || end - p < 3)
>                 return -EINVAL;
>
>         memcpy(buf, p, 3);
> --
> 2.47.3
>
>

[PATCH v4 4/5] HID: asus: add support for xgm led

[Denis Benato]

XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 6896730efafc..e68a6d93369d 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define FEATURE_KBD_LED_REPORT_ID1 0x5d
 #define FEATURE_KBD_LED_REPORT_ID2 0x5e
 
+#define ROG_XGM_REPORT_SIZE 300
+
 #define ROG_ALLY_REPORT_SIZE 64
 #define ROG_ALLY_X_MIN_MCU 313
 #define ROG_ALLY_MIN_MCU 319
@@ -143,6 +145,11 @@ struct asus_worker {
 	bool removed;
 };
 
+struct asus_xgm_led {
+	struct led_classdev cdev;
+	struct hid_device *hdev;
+};
+
 struct asus_touchpad_info {
 	int max_x;
 	int max_y;
@@ -169,6 +176,7 @@ struct asus_drvdata {
 	unsigned long battery_next_query;
 	struct asus_hid_listener listener;
 	bool fn_lock;
+	struct asus_xgm_led *xgm_led;
 };
 
 static int asus_report_battery(struct asus_drvdata *, u8 *, int);
@@ -1119,6 +1127,26 @@ static int asus_battery_probe(struct hid_device *hdev)
 	return ret;
 }
 
+static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	const u8 buf[ROG_XGM_REPORT_SIZE] = {
+		FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
+	};
+	struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
+	int ret;
+
+	ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
+	if (ret < 0) {
+		hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
+		return ret;
+	} else if (ret != ROG_XGM_REPORT_SIZE) {
+		hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 {
 	struct input_dev *input = hi->input;
@@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
 	return 0;
 }
 
+static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
+{
+	const char *name;
+	int ret;
+
+	drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
+	if (drvdata->xgm_led == NULL)
+		return -ENOMEM;
+
+	name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "asus:xgm-%s:led",
+			      strlen(hdev->uniq) ? hdev->uniq : dev_name(&hdev->dev));
+
+	if (name == NULL) {
+		ret = -ENOMEM;
+		goto asus_xgm_init_err;
+	}
+
+	drvdata->xgm_led->hdev = hdev;
+	drvdata->xgm_led->cdev.name = name;
+	drvdata->xgm_led->cdev.brightness = 1;
+	drvdata->xgm_led->cdev.max_brightness = 1;
+	drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
+
+	/* LED state is arbitrary on boot, set a default */
+	ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+	if (ret) {
+		hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
+		goto asus_xgm_init_err;
+	}
+
+	ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
+	if (ret) {
+		hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
+		goto asus_xgm_init_err;
+	}
+
+	return 0;
+asus_xgm_init_err:
+	drvdata->xgm_led = NULL;
+	return ret;
+}
+
 static int __maybe_unused asus_resume(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+	int ret;
 
 	/*
 	 * If we have a backlight listener registered, restore the previous state,
@@ -1355,7 +1426,17 @@ 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 (drvdata->xgm_led) {
+		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+		if (ret) {
+			hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
+			goto asus_resume_err;
+		}
+	}
+
 	return 0;
+asus_resume_err:
+	return ret;
 }
 
 static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
@@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
+	    ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
+	     (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
+		ret = asus_xgm_init(hdev, drvdata);
+		if (ret) {
+			hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
+			goto err_stop_hw;
+		}
+	}
+
 	/* Laptops keyboard backlight is always at 0x5a */
 	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
 	    (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
-- 
2.47.3

Re: [PATCH v4 4/5] HID: asus: add support for xgm led

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Propagating cosmetic LED restore errors during system resume causes the entire device resume to fail.
- [High] Failing to initialize the auxiliary XG Mobile LED aborts the entire HID device probe.
- [High] Manual call to `hid_hw_stop()` while using `devm_led_classdev_register()` leads to a use-after-teardown bug during device removal and probe error paths.
--

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

HID: asus: add support for xgm led

XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6896730efafca..e68a6d93369d8 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
> +{
[ ... ]
> +	drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> +	/* LED state is arbitrary on boot, set a default */
> +	ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +	if (ret) {
> +		hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> +		goto asus_xgm_init_err;
> +	}
> +
> +	ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);

[Severity: High]
Because this LED is registered using devm_led_classdev_register(), its
unregistration happens during the devres cleanup phase. This phase runs after
the driver's remove callback (or probe error path) finishes.

If the probe error path or asus_remove() manually calls hid_hw_stop(hdev),
the HID transport is shut down. During the subsequent devres cleanup,
led_classdev_unregister() is called and sets the LED to LED_OFF. This invokes
asus_xgm_led_set(), which calls hid_hw_raw_request() on a stopped interface.

Could this lead to a use-after-teardown bug?

> +	if (ret) {
> +		hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +		goto asus_xgm_init_err;
> +	}
> +
> +	return 0;
> +asus_xgm_init_err:
> +	drvdata->xgm_led = NULL;
> +	return ret;
> +}
> +
>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
>  
>  	/*
>  	 * If we have a backlight listener registered, restore the previous state,
> @@ -1355,7 +1426,17 @@ 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 (drvdata->xgm_led) {
> +		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +		if (ret) {
> +			hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> +			goto asus_resume_err;

[Severity: High]
If asus_xgm_led_set() fails to restore the LED state (e.g., due to an I/O
error or timeout), this returns the error code directly to the PM core.
A failed resume callback causes the PM subsystem to treat the device resume as
failed, potentially leaving primary features like the keyboard and touchpad
unresponsive.

Would it be better for the failure of a cosmetic LED restore to log a warning
instead of aborting the entire device resume?

> +		}
> +	}
> +
>  	return 0;
> +asus_resume_err:
> +	return ret;
>  }
>  
>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
[ ... ]
> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		}
>  	}
>  
> +	if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> +	    ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> +	     (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> +		ret = asus_xgm_init(hdev, drvdata);
> +		if (ret) {
> +			hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> +			goto err_stop_hw;

[Severity: High]
If asus_xgm_init() fails, the code jumps to err_stop_hw, failing the entire
device probe.

Does this make a non-critical aesthetic feature a hard dependency for the
entire device? Might it be safer to log the failure rather than preventing
the keyboard and touchpad from working?

> +		}
> +	}
> +
>  	/* Laptops keyboard backlight is always at 0x5a */
>  	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>  	    (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&

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

Re: [PATCH v4 4/5] HID: asus: add support for xgm led

[Denis Benato]

On 6/15/26 19:00, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Propagating cosmetic LED restore errors during system resume causes the entire device resume to fail.
> - [High] Failing to initialize the auxiliary XG Mobile LED aborts the entire HID device probe.
> - [High] Manual call to `hid_hw_stop()` while using `devm_led_classdev_register()` leads to a use-after-teardown bug during device removal and probe error paths.
> --
>
> commit 26f82fa14c4bebc817bb5d79a1cdcdc34bb503d3
> Author: Denis Benato <denis.benato@linux.dev>
>
> HID: asus: add support for xgm led
>
> XG mobile stations have very bright leds behind the fan that can be
> turned either ON or OFF: add a cled interface to allow controlling the
> brightness of those red leds.
>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 6896730efafca..e68a6d93369d8 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
> [ ... ]
>> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
>>  	return 0;
>>  }
>>  
>> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
>> +{
> [ ... ]
>> +	drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
>> +
>> +	/* LED state is arbitrary on boot, set a default */
>> +	ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> +	if (ret) {
>> +		hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
>> +		goto asus_xgm_init_err;
>> +	}
>> +
>> +	ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> [Severity: High]
> Because this LED is registered using devm_led_classdev_register(), its
> unregistration happens during the devres cleanup phase. This phase runs after
> the driver's remove callback (or probe error path) finishes.
>
> If the probe error path or asus_remove() manually calls hid_hw_stop(hdev),
> the HID transport is shut down. During the subsequent devres cleanup,
> led_classdev_unregister() is called and sets the LED to LED_OFF. This invokes
> asus_xgm_led_set(), which calls hid_hw_raw_request() on a stopped interface.
>
> Could this lead to a use-after-teardown bug?
>
>> +	if (ret) {
>> +		hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
>> +		goto asus_xgm_init_err;
>> +	}
>> +
>> +	return 0;
>> +asus_xgm_init_err:
>> +	drvdata->xgm_led = NULL;
>> +	return ret;
>> +}
>> +
>>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>>  {
>>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +	int ret;
>>  
>>  	/*
>>  	 * If we have a backlight listener registered, restore the previous state,
>> @@ -1355,7 +1426,17 @@ 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 (drvdata->xgm_led) {
>> +		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> +		if (ret) {
>> +			hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
>> +			goto asus_resume_err;
> [Severity: High]
> If asus_xgm_led_set() fails to restore the LED state (e.g., due to an I/O
> error or timeout), this returns the error code directly to the PM core.
> A failed resume callback causes the PM subsystem to treat the device resume as
> failed, potentially leaving primary features like the keyboard and touchpad
> unresponsive.
>
> Would it be better for the failure of a cosmetic LED restore to log a warning
> instead of aborting the entire device resume?
No: xg mobile device controllers are just that: not an input device: if it fails
there it's right to tear down the whole thing.
>> +		}
>> +	}
>> +
>>  	return 0;
>> +asus_resume_err:
>> +	return ret;
>>  }
>>  
>>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> [ ... ]
>> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  		}
>>  	}
>>  
>> +	if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
>> +	    ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
>> +	     (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
>> +		ret = asus_xgm_init(hdev, drvdata);
>> +		if (ret) {
>> +			hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
>> +			goto err_stop_hw;
> [Severity: High]
> If asus_xgm_init() fails, the code jumps to err_stop_hw, failing the entire
> device probe.
>
> Does this make a non-critical aesthetic feature a hard dependency for the
> entire device? Might it be safer to log the failure rather than preventing
> the keyboard and touchpad from working?
Ditto. Won't resolve these as it would only make the code more confusing.
>> +		}
>> +	}
>> +
>>  	/* Laptops keyboard backlight is always at 0x5a */
>>  	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>>  	    (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&

Re: [PATCH v4 4/5] HID: asus: add support for xgm led

[Antheas Kapenekakis]

On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> wrote:
>
> XG mobile stations have very bright leds behind the fan that can be
> turned either ON or OFF: add a cled interface to allow controlling the
> brightness of those red leds.
>
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 91 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6896730efafc..e68a6d93369d 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define FEATURE_KBD_LED_REPORT_ID1 0x5d
>  #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> +#define ROG_XGM_REPORT_SIZE 300
> +
>  #define ROG_ALLY_REPORT_SIZE 64
>  #define ROG_ALLY_X_MIN_MCU 313
>  #define ROG_ALLY_MIN_MCU 319
> @@ -143,6 +145,11 @@ struct asus_worker {
>         bool removed;
>  };
>
> +struct asus_xgm_led {
> +       struct led_classdev cdev;
> +       struct hid_device *hdev;
> +};
> +
>  struct asus_touchpad_info {
>         int max_x;
>         int max_y;
> @@ -169,6 +176,7 @@ struct asus_drvdata {
>         unsigned long battery_next_query;
>         struct asus_hid_listener listener;
>         bool fn_lock;
> +       struct asus_xgm_led *xgm_led;
>  };
>
>  static int asus_report_battery(struct asus_drvdata *, u8 *, int);
> @@ -1119,6 +1127,26 @@ static int asus_battery_probe(struct hid_device *hdev)
>         return ret;
>  }
>
> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +       const u8 buf[ROG_XGM_REPORT_SIZE] = {
> +               FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
> +       };
> +       struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
> +       int ret;
> +
> +       ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
> +       if (ret < 0) {
> +               hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
> +               return ret;
> +       } else if (ret != ROG_XGM_REPORT_SIZE) {
> +               hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  {
>         struct input_dev *input = hi->input;
> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
>         return 0;
>  }
>
> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
> +{
> +       const char *name;
> +       int ret;
> +
> +       drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> +       if (drvdata->xgm_led == NULL)
> +               return -ENOMEM;
> +
> +       name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "asus:xgm-%s:led",
> +                             strlen(hdev->uniq) ? hdev->uniq : dev_name(&hdev->dev));
> +
> +       if (name == NULL) {
> +               ret = -ENOMEM;
> +               goto asus_xgm_init_err;
> +       }
> +
> +       drvdata->xgm_led->hdev = hdev;
> +       drvdata->xgm_led->cdev.name = name;
> +       drvdata->xgm_led->cdev.brightness = 1;
> +       drvdata->xgm_led->cdev.max_brightness = 1;
> +       drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> +       /* LED state is arbitrary on boot, set a default */
> +       ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +       if (ret) {
> +               hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> +               goto asus_xgm_init_err;
> +       }
> +
> +       ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> +       if (ret) {
> +               hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +               goto asus_xgm_init_err;
> +       }
> +
> +       return 0;
> +asus_xgm_init_err:
> +       drvdata->xgm_led = NULL;
> +       return ret;
> +}
> +
>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>  {
>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +       int ret;
>
>         /*
>          * If we have a backlight listener registered, restore the previous state,
> @@ -1355,7 +1426,17 @@ 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 (drvdata->xgm_led) {
> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> +                       goto asus_resume_err;
> +               }
> +       }
> +
>         return 0;
> +asus_resume_err:
> +       return ret;
>  }
>
>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> @@ -1484,6 +1565,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 }
>         }
>
> +       if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> +           ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> +            (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> +               ret = asus_xgm_init(hdev, drvdata);
> +               if (ret) {
> +                       hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> +                       goto err_stop_hw;
> +               }
> +       }
> +

reset_resume has a special meaning and perhaps should not have been
used for this driver at all. Check if resume works and if it does use
that instead.

Other than that and the refactor patch I'd rather see separately, the
other 4 patches look fine to me.

Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>

Best,
Antheas

>         /* Laptops keyboard backlight is always at 0x5a */
>         if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>             (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
> --
> 2.47.3
>
>

[PATCH v4 5/5] HID: asus: add i2c entry for FA808UM and other TUFs

[Denis Benato]

On newer TUF laptops the keyboard HID device uses the same PID/VID of a
USB device that was found in ROG laptops: add it to hid-asus as i2c too.

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

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index e68a6d93369d..cdfc8ec7d47e 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1760,6 +1760,9 @@ static const struct hid_device_id asus_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
+	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
+	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
-- 
2.47.3