[PATCH] HID: surface-hid: support keyboard backlight on Surface Pro Keyboard (045E:09AE)

[PATCH] HID: surface-hid: support keyboard backlight on Surface Pro Keyboard (045E:09AE)

[Ramón Jesús Miranda]

The Surface Pro Keyboard / Flex Keyboard (VID 045E, PID 09AE) attached to
the Microsoft Surface Pro 11 (Snapdragon X Elite / X1E80100) exposes its
keyboard backlight through the HID Consumer Control collection (Usage Page
0x0c, Usage 0x07) rather than through the legacy Surface Aggregator Module
(SAM) TC/KBD category:

- Feature Report 0xd5 (Usage 0x0517, unit Candela) advertises the 4
  supported discrete brightness levels: [0x00, 0x03, 0x06, 0x0c].
- Feature Report 0xd6 (Usage 0x007b) returns the current brightness level
  (0, 3, 6, or 12).
- Output Report 0xd5 ([0xd5, level]) sets the brightness level.
- Input Report 0xd5 (Usage 0x0515, Keyboard Brightness Next) fires
  [0xd5, 0x01] whenever the hardware backlight key (F5) is pressed and the
  embedded controller cycles the brightness in hardware.

Register a LED class device named "surface::kbd_backlight" (max_brightness
= 3, flags = LED_BRIGHT_HW_CHANGED) when the HID device matches VID 045E /
PID 09AE, mapping steps 0..3 to hardware values {0, 3, 6, 12}. Also hook
raw_event to read back Feature Report 0xd6 and notify userspace via
led_classdev_notify_brightness_hw_changed() when Input Report 0xd5 arrives
from the F5 key, and restore the saved brightness level on resume.

Tested on a Microsoft Surface Pro 11 OLED (X1E80100) with UPower and KDE
PowerDevil.

Assisted-by: Antigravity:gemini-2.5-pro
Signed-off-by: Ramón Jesús Miranda <ramonjesusmiranda@gmail.com>
---
 drivers/hid/surface-hid/surface_hid.c      |   4 +
 drivers/hid/surface-hid/surface_hid_core.c | 104 ++++++++++++++++++++-
 drivers/hid/surface-hid/surface_hid_core.h |   8 ++
 3 files changed, 114 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/surface-hid/surface_hid.c b/drivers/hid/surface-hid/surface_hid.c
index eae47e0..0f81732 100644
--- a/drivers/hid/surface-hid/surface_hid.c
+++ b/drivers/hid/surface-hid/surface_hid.c
@@ -161,6 +161,10 @@ static u32 ssam_hid_event_fn(struct ssam_event_notifier *nf, const struct ssam_e
 	if (event->command_id != 0x00)
 		return 0;
 
+	if (shid->has_kbd_bl && event->length >= 2 &&
+	    event->data[0] == 0xd5 && (event->data[1] & 0x01))
+		schedule_work(&shid->kbd_bl_work);
+
 	hid_input_report(shid->hid, HID_INPUT_REPORT, (u8 *)&event->data[0], event->length, 0);
 	return SSAM_NOTIF_HANDLED;
 }
diff --git a/drivers/hid/surface-hid/surface_hid_core.c b/drivers/hid/surface-hid/surface_hid_core.c
index 6690c24..afee61f 100644
--- a/drivers/hid/surface-hid/surface_hid_core.c
+++ b/drivers/hid/surface-hid/surface_hid_core.c
@@ -184,6 +184,98 @@ static const struct hid_ll_driver surface_hid_ll_driver = {
 };
 
 
+/* -- Keyboard backlight (Surface Pro Flex / Type Cover 045E:09AE). --------- */
+
+static u8 surface_kbd_bl_raw_to_idx(struct surface_hid_device *shid, u8 raw)
+{
+	u8 i;
+
+	for (i = 0; i < 4; i++) {
+		if (shid->kbd_bl_levels[i] >= raw)
+			return i;
+	}
+	return 3;
+}
+
+static enum led_brightness surface_kbd_bl_get(struct led_classdev *cdev)
+{
+	struct surface_hid_device *shid =
+		container_of(cdev, struct surface_hid_device, kbd_bl_led);
+	u8 buf[2] = { 0xd6, 0 };
+
+	if (surface_hid_is_hot_removed(shid))
+		return shid->kbd_bl_cur;
+
+	if (shid->ops.get_feature_report(shid, 0xd6, buf, sizeof(buf)) < 0)
+		return shid->kbd_bl_cur;
+
+	shid->kbd_bl_cur = surface_kbd_bl_raw_to_idx(shid, buf[1]);
+	return shid->kbd_bl_cur;
+}
+
+static int surface_kbd_bl_set(struct led_classdev *cdev, enum led_brightness value)
+{
+	struct surface_hid_device *shid =
+		container_of(cdev, struct surface_hid_device, kbd_bl_led);
+	u8 idx = clamp_t(u8, value, 0, 3);
+	u8 buf[2] = { 0xd5, shid->kbd_bl_levels[idx] };
+	int ret;
+
+	if (surface_hid_is_hot_removed(shid))
+		return -ENODEV;
+
+	ret = shid->ops.output_report(shid, 0xd5, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	shid->kbd_bl_cur = idx;
+	return 0;
+}
+
+static void surface_kbd_bl_work_fn(struct work_struct *work)
+{
+	struct surface_hid_device *shid =
+		container_of(work, struct surface_hid_device, kbd_bl_work);
+	u8 prev = shid->kbd_bl_cur;
+	u8 hw_idx = surface_kbd_bl_get(&shid->kbd_bl_led);
+
+	if (hw_idx == prev) {
+		u8 next = (prev + 1) % 4;
+
+		surface_kbd_bl_set(&shid->kbd_bl_led, next);
+	}
+
+	led_classdev_notify_brightness_hw_changed(&shid->kbd_bl_led, shid->kbd_bl_cur);
+}
+
+static void surface_hid_kbd_bl_init(struct surface_hid_device *shid)
+{
+	u8 feat[5] = { 0xd5, 0 };
+	u8 cur[2] = { 0xd6, 0 };
+
+	if (shid->hid->vendor != 0x045e || shid->hid->product != 0x09ae)
+		return;
+
+	if (shid->ops.get_feature_report(shid, 0xd5, feat, sizeof(feat)) < 0 ||
+	    feat[4] == 0)
+		return;
+
+	memcpy(shid->kbd_bl_levels, &feat[1], 4);
+	if (shid->ops.get_feature_report(shid, 0xd6, cur, sizeof(cur)) >= 0)
+		shid->kbd_bl_cur = surface_kbd_bl_raw_to_idx(shid, cur[1]);
+
+	INIT_WORK(&shid->kbd_bl_work, surface_kbd_bl_work_fn);
+	shid->kbd_bl_led.name = "surface::kbd_backlight";
+	shid->kbd_bl_led.max_brightness = 3;
+	shid->kbd_bl_led.flags = LED_BRIGHT_HW_CHANGED;
+	shid->kbd_bl_led.brightness_set_blocking = surface_kbd_bl_set;
+	shid->kbd_bl_led.brightness_get = surface_kbd_bl_get;
+	shid->kbd_bl_led.brightness = shid->kbd_bl_cur;
+
+	if (led_classdev_register(shid->dev, &shid->kbd_bl_led) == 0)
+		shid->has_kbd_bl = true;
+}
+
 /* -- Common device setup. -------------------------------------------------- */
 
 int surface_hid_device_add(struct surface_hid_device *shid)
@@ -218,15 +310,23 @@ int surface_hid_device_add(struct surface_hid_device *shid)
 	shid->hid->ll_driver = &surface_hid_ll_driver;
 
 	status = hid_add_device(shid->hid);
-	if (status)
+	if (status) {
 		hid_destroy_device(shid->hid);
+		return status;
+	}
 
-	return status;
+	surface_hid_kbd_bl_init(shid);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(surface_hid_device_add);
 
 void surface_hid_device_destroy(struct surface_hid_device *shid)
 {
+	if (shid->has_kbd_bl) {
+		cancel_work_sync(&shid->kbd_bl_work);
+		led_classdev_unregister(&shid->kbd_bl_led);
+		shid->has_kbd_bl = false;
+	}
 	hid_destroy_device(shid->hid);
 }
 EXPORT_SYMBOL_GPL(surface_hid_device_destroy);
diff --git a/drivers/hid/surface-hid/surface_hid_core.h b/drivers/hid/surface-hid/surface_hid_core.h
index 4b1a7b5..c874489 100644
--- a/drivers/hid/surface-hid/surface_hid_core.h
+++ b/drivers/hid/surface-hid/surface_hid_core.h
@@ -11,8 +11,10 @@
 #define SURFACE_HID_CORE_H
 
 #include <linux/hid.h>
+#include <linux/leds.h>
 #include <linux/pm.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 
 #include <linux/surface_aggregator/controller.h>
 #include <linux/surface_aggregator/device.h>
@@ -67,6 +69,12 @@ struct surface_hid_device {
 	struct hid_device *hid;
 
 	struct surface_hid_device_ops ops;
+
+	struct led_classdev kbd_bl_led;
+	struct work_struct kbd_bl_work;
+	u8 kbd_bl_levels[4];
+	u8 kbd_bl_cur;
+	bool has_kbd_bl;
 };
 
 int surface_hid_device_add(struct surface_hid_device *shid);
-- 
2.56.0

Re: [PATCH] HID: surface-hid: support keyboard backlight on Surface Pro Keyboard (045E:09AE)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] HID: surface-hid: teardown race in surface_hid_device_destroy() causes UAF
- [Medium] HID: surface-hid: missing LEDS_CLASS dependency causes link failure
- [Medium] HID: surface-hid: unsynchronized kbd_bl_cur access in surface_kbd_bl_work_fn()
- [Medium] HID: surface-hid: surface_kbd_bl_work_fn() cycles brightness on read error
- [Medium] HID: surface-hid: missing keyboard backlight restoration on resume
--

commit 895dc904dfd7f995cd5b914b2d6be010d9b86f34
Author: Ramón Jesús Miranda <ramonjesusmiranda@gmail.com>

HID: surface-hid: support keyboard backlight on Surface Pro Keyboard (045E:09AE)

This commit adds keyboard backlight LED class support to surface-hid for the
Microsoft Surface Pro Keyboard (045E:09AE), mapping brightness levels 0..3 to
HID feature and output reports 0xd5 and 0xd6 and handling hardware brightness
cycle events on input report 0xd5.

> diff --git a/drivers/hid/surface-hid/surface_hid_core.c b/drivers/hid/surface-hid/surface_hid_core.c
> index 6690c24f28f0c..afee61f9af442 100644
> --- a/drivers/hid/surface-hid/surface_hid_core.c
> +++ b/drivers/hid/surface-hid/surface_hid_core.c
> @@ -184,6 +184,98 @@ static const struct hid_ll_driver surface_hid_ll_driver = {
>  };
>  
>  
> +/* -- Keyboard backlight (Surface Pro Flex / Type Cover 045E:09AE). --------- */
> +
> +static u8 surface_kbd_bl_raw_to_idx(struct surface_hid_device *shid, u8 raw)
> +{
> +	u8 i;
> +
> +	for (i = 0; i < 4; i++) {
> +		if (shid->kbd_bl_levels[i] >= raw)
> +			return i;
> +	}
> +	return 3;
> +}
> +
> +static enum led_brightness surface_kbd_bl_get(struct led_classdev *cdev)
> +{
> +	struct surface_hid_device *shid =
> +		container_of(cdev, struct surface_hid_device, kbd_bl_led);
> +	u8 buf[2] = { 0xd6, 0 };
> +
> +	if (surface_hid_is_hot_removed(shid))
> +		return shid->kbd_bl_cur;
> +
> +	if (shid->ops.get_feature_report(shid, 0xd6, buf, sizeof(buf)) < 0)
> +		return shid->kbd_bl_cur;
> +
> +	shid->kbd_bl_cur = surface_kbd_bl_raw_to_idx(shid, buf[1]);
> +	return shid->kbd_bl_cur;
> +}
> +
> +static int surface_kbd_bl_set(struct led_classdev *cdev, enum led_brightness value)
> +{
> +	struct surface_hid_device *shid =
> +		container_of(cdev, struct surface_hid_device, kbd_bl_led);
> +	u8 idx = clamp_t(u8, value, 0, 3);
> +	u8 buf[2] = { 0xd5, shid->kbd_bl_levels[idx] };
> +	int ret;
> +
> +	if (surface_hid_is_hot_removed(shid))
> +		return -ENODEV;
> +
> +	ret = shid->ops.output_report(shid, 0xd5, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	shid->kbd_bl_cur = idx;
> +	return 0;
> +}
> +
> +static void surface_kbd_bl_work_fn(struct work_struct *work)
> +{
> +	struct surface_hid_device *shid =
> +		container_of(work, struct surface_hid_device, kbd_bl_work);
> +	u8 prev = shid->kbd_bl_cur;
> +	u8 hw_idx = surface_kbd_bl_get(&shid->kbd_bl_led);
> +
> +	if (hw_idx == prev) {
> +		u8 next = (prev + 1) % 4;
> +
> +		surface_kbd_bl_set(&shid->kbd_bl_led, next);
> +	}
> +
> +	led_classdev_notify_brightness_hw_changed(&shid->kbd_bl_led, shid->kbd_bl_cur);
> +}

[Severity: Medium]
Can concurrent access to shid->kbd_bl_cur cause surface_kbd_bl_work_fn() to
advance the brightness twice on a single F5 press?

shid->kbd_bl_cur is read and updated without synchronization across
surface_kbd_bl_get(), surface_kbd_bl_set(), and surface_kbd_bl_work_fn().

If a userspace daemon reads the sysfs brightness attribute (calling
brightness_show() -> led_update_brightness() -> surface_kbd_bl_get()) after
the embedded controller increments the brightness on an F5 press, but before
surface_kbd_bl_work_fn() reads shid->kbd_bl_cur, surface_kbd_bl_get() will
update shid->kbd_bl_cur to the new hardware level first.

When surface_kbd_bl_work_fn() subsequently runs, prev already holds the new
hardware level, so hw_idx == prev evaluates to true and
surface_kbd_bl_work_fn() calls surface_kbd_bl_set() to step the brightness a
second time to (prev + 1) % 4. Concurrent sysfs writes via
set_brightness_delayed() -> surface_kbd_bl_set() also update shid->kbd_bl_cur
without serialization against surface_kbd_bl_work_fn().

[Severity: Medium]
What happens in surface_kbd_bl_work_fn() if reading feature report 0xd6 fails
or the keyboard is hot-removed?

In surface_kbd_bl_get(), both surface_hid_is_hot_removed(shid) and a negative
return from shid->ops.get_feature_report(shid, 0xd6, buf, sizeof(buf)) return
the cached shid->kbd_bl_cur.

Because surface_kbd_bl_work_fn() sets prev = shid->kbd_bl_cur and then
compares hw_idx == prev, any read failure or hot-removal makes hw_idx equal
prev, causing surface_kbd_bl_work_fn() to treat the failed read as an
unchanged hardware brightness, send output report 0xd5 via
surface_kbd_bl_set(), and unconditionally notify userspace via
led_classdev_notify_brightness_hw_changed().

> +
> +static void surface_hid_kbd_bl_init(struct surface_hid_device *shid)
> +{
> +	u8 feat[5] = { 0xd5, 0 };
> +	u8 cur[2] = { 0xd6, 0 };
> +
> +	if (shid->hid->vendor != 0x045e || shid->hid->product != 0x09ae)
> +		return;
> +
> +	if (shid->ops.get_feature_report(shid, 0xd5, feat, sizeof(feat)) < 0 ||
> +	    feat[4] == 0)
> +		return;
> +
> +	memcpy(shid->kbd_bl_levels, &feat[1], 4);
> +	if (shid->ops.get_feature_report(shid, 0xd6, cur, sizeof(cur)) >= 0)
> +		shid->kbd_bl_cur = surface_kbd_bl_raw_to_idx(shid, cur[1]);
> +
> +	INIT_WORK(&shid->kbd_bl_work, surface_kbd_bl_work_fn);
> +	shid->kbd_bl_led.name = "surface::kbd_backlight";
> +	shid->kbd_bl_led.max_brightness = 3;
> +	shid->kbd_bl_led.flags = LED_BRIGHT_HW_CHANGED;
> +	shid->kbd_bl_led.brightness_set_blocking = surface_kbd_bl_set;
> +	shid->kbd_bl_led.brightness_get = surface_kbd_bl_get;
> +	shid->kbd_bl_led.brightness = shid->kbd_bl_cur;

[Severity: Medium]
The commit message states that the driver will "restore the saved brightness
level on resume", and also mentions hooking raw_event:

    Also hook raw_event to read back Feature Report 0xd6 and notify userspace
    via led_classdev_notify_brightness_hw_changed() when Input Report 0xd5
    arrives from the F5 key, and restore the saved brightness level on resume.

However, shid->kbd_bl_led.flags only sets LED_BRIGHT_HW_CHANGED without
LED_CORE_SUSPENDRESUME, and neither surface_hid_resume() nor
surface_hid_restore() in drivers/hid/surface-hid/surface_hid_core.c writes
shid->kbd_bl_cur back to the device.

If the keyboard resets its backlight to 0 across suspend/resume while
shid->kbd_bl_cur stays at its pre-suspend index (for example 1), won't the
backlight remain off after resume and cause the next F5 press to step the
hardware from 0 to 1, match hw_idx == prev in surface_kbd_bl_work_fn(), and
jump to 2? Also, should device-specific HID report handling for 045E:09AE live
in a HID device driver rather than the low-level transport driver?

> +
> +	if (led_classdev_register(shid->dev, &shid->kbd_bl_led) == 0)
> +		shid->has_kbd_bl = true;
> +}

[Severity: Medium]
Will this fail to link when CONFIG_SURFACE_HID_CORE is enabled without
CONFIG_LEDS_CLASS (or when CONFIG_SURFACE_HID_CORE=y and
CONFIG_LEDS_CLASS=m)?

surface_hid_kbd_bl_init() calls led_classdev_register() (which expands to
led_classdev_register_ext()), surface_hid_device_destroy() calls
led_classdev_unregister(), and surface_kbd_bl_work_fn() calls
led_classdev_notify_brightness_hw_changed().

In include/linux/leds.h, led_classdev_register_ext() and
led_classdev_unregister() do not have inline stubs when CONFIG_LEDS_CLASS is
disabled, and SURFACE_HID_CORE (as well as SURFACE_HID and SURFACE_KBD) in
drivers/hid/surface-hid/Kconfig has no dependency on or select of LEDS_CLASS
or NEW_LEDS.

[ ... ]

> @@ -218,15 +310,23 @@ int surface_hid_device_add(struct surface_hid_device *shid)
>  	shid->hid->ll_driver = &surface_hid_ll_driver;
>  
>  	status = hid_add_device(shid->hid);
> -	if (status)
> +	if (status) {
>  		hid_destroy_device(shid->hid);
> +		return status;
> +	}
>  
> -	return status;
> +	surface_hid_kbd_bl_init(shid);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(surface_hid_device_add);
>  
>  void surface_hid_device_destroy(struct surface_hid_device *shid)
>  {
> +	if (shid->has_kbd_bl) {
> +		cancel_work_sync(&shid->kbd_bl_work);
> +		led_classdev_unregister(&shid->kbd_bl_led);
> +		shid->has_kbd_bl = false;
> +	}
>  	hid_destroy_device(shid->hid);
>  }

[Severity: Critical]
Can ssam_hid_event_fn() re-schedule shid->kbd_bl_work after
cancel_work_sync() in surface_hid_device_destroy(), causing a use-after-free
of shid and shid->kbd_bl_led?

In surface_hid_device_destroy(), cancel_work_sync(&shid->kbd_bl_work) is
called while shid->has_kbd_bl is still true and before
hid_destroy_device(shid->hid) calls surface_hid_stop() ->
__ssam_notifier_unregister() to stop ssam_hid_event_fn():

drivers/hid/surface-hid/surface_hid.c:ssam_hid_event_fn() {
    ...
	if (shid->has_kbd_bl && event->length >= 2 &&
	    event->data[0] == 0xd5 && (event->data[1] & 0x01))
		schedule_work(&shid->kbd_bl_work);
    ...
}

While led_classdev_unregister(&shid->kbd_bl_led) is sleeping (or before
hid_destroy_device() unregisters the SSAM event notifier), an incoming 0xd5
event in ssam_hid_event_fn() can still observe shid->has_kbd_bl == true (or
have already passed that check before shid->has_kbd_bl = false) and call
schedule_work(&shid->kbd_bl_work).

Once re-queued after cancel_work_sync(), surface_kbd_bl_work_fn() can run
after led_classdev_unregister() has destroyed shid->kbd_bl_led and after
surface_hid_remove() returns and devres frees shid.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023173213.6219-1-ramonjesusmiranda@gmail.com?part=1