[PATCH] HID: asus: add support for xgm led

[PATCH] 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 | 53 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index d34d74df3dc0..4298a3fe98c7 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
@@ -118,6 +120,11 @@ struct asus_kbd_leds {
 	bool removed;
 };
 
+struct asus_xgm_led {
+	struct led_classdev cdev;
+	struct hid_device *hdev;
+};
+
 struct asus_touchpad_info {
 	int max_x;
 	int max_y;
@@ -143,6 +150,7 @@ struct asus_drvdata {
 	unsigned long battery_next_query;
 	struct work_struct fn_lock_sync_work;
 	bool fn_lock;
+	struct asus_xgm_led *xgm_led;
 };
 
 static int asus_report_battery(struct asus_drvdata *, u8 *, int);
@@ -940,6 +948,19 @@ static int asus_battery_probe(struct hid_device *hdev)
 	return ret;
 }
 
+static void 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 != ROG_XGM_REPORT_SIZE)
+		hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
+}
+
 static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 {
 	struct input_dev *input = hi->input;
@@ -1183,6 +1204,9 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
 		}
 	}
 
+	if (drvdata->xgm_led)
+		asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+
 asus_resume_err:
 	return ret;
 }
@@ -1309,6 +1333,32 @@ 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))) {
+		drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
+		if (drvdata->xgm_led == NULL) {
+			ret = -ENOMEM;
+			goto err_stop_hw;
+		}
+		drvdata->xgm_led->hdev = hdev;
+		drvdata->xgm_led->cdev.name = "asus:xgm:led";
+		drvdata->xgm_led->cdev.brightness = 1;
+		drvdata->xgm_led->cdev.max_brightness = 1;
+		drvdata->xgm_led->cdev.brightness_set = asus_xgm_led_set;
+		ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
+		if (ret) {
+			hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
+			goto err_stop_hw;
+		}
+
+		/*
+		 * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
+		 * what the default brightness resets when doing a cold boot.
+		 */
+		asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+	}
+
 	/* 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)) &&
@@ -1365,6 +1415,9 @@ static void asus_remove(struct hid_device *hdev)
 	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
 		cancel_work_sync(&drvdata->fn_lock_sync_work);
 
+	if (drvdata->xgm_led)
+		led_classdev_unregister(&drvdata->xgm_led->cdev);
+
 	hid_hw_stop(hdev);
 }
 
-- 
2.47.3

Re: [PATCH] HID: asus: add support for xgm led

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `asus_xgm_led_set` function is incorrectly assigned to the non-blocking `brightness_set` LED callback, but it performs sleeping operations.
- [Medium] The XGM LED class device name is hardcoded, which causes device probe failures due to sysfs naming collisions.
--

commit 55bc647f99f9d2c64c1e99f0ee43e81efa5a6298
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 3f5e96900b67a..8a5f364b7535f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1303,6 +1327,32 @@ 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))) {
> +		drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> +		if (drvdata->xgm_led == NULL) {
> +			ret = -ENOMEM;
> +			goto err_stop_hw;
> +		}
> +		drvdata->xgm_led->hdev = hdev;
> +		drvdata->xgm_led->cdev.name = "asus:xgm:led";

[Severity: Medium]
If multiple XG Mobile docks are connected, or if an ungraceful disconnect
leaves a stale sysfs entry behind, will this hardcoded name cause a sysfs
naming collision? A duplicate name error here would cause
led_classdev_register() to fail and abort the entire HID device probe.

Should this use a dynamic identifier, perhaps incorporating dev_name(),
to ensure uniqueness?

> +		drvdata->xgm_led->cdev.brightness = 1;
> +		drvdata->xgm_led->cdev.max_brightness = 1;
> +		drvdata->xgm_led->cdev.brightness_set = asus_xgm_led_set;

[Severity: High]
Can this cause a sleep-in-atomic bug if the LED is triggered by a software
blink (such as a timer or heartbeat sysfs trigger)?

When software blinking is used, the LED core invokes the brightness_set
callback directly from softirq context. However, asus_xgm_led_set() calls
asus_kbd_set_report(), which performs a GFP_KERNEL allocation and a blocking
USB transaction via hid_hw_raw_request().

Should this callback be assigned to brightness_set_blocking instead,
so the LED core knows to defer the sleeping work to a workqueue?

> +		ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> +		if (ret) {
> +			hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +			goto err_stop_hw;
> +		}
> +
> +		/*
> +		 * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
> +		 * what the default brightness resets when doing a cold boot.
> +		 */
> +		asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +	}
> +

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