[PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume

[PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume

[Andre Eikmeyer]

The T2 virtual USB host controller re-enumerates the internal keyboard
after system resume. The butterfly keyboard backlight currently uses
LED_CORE_SUSPENDRESUME, so the LED core sends a blocking request to the
old HID device while it is disappearing. That request fails with -ENODEV
and the newly probed device starts with its backlight off.

To fix this, we cache the requested brightness when the old HID device is
removed and restore it when the replacement is probed. We let
re-enumeration handle restoration instead of issuing a request through the
stale device.

Fixes: 1f95a6cd5ad7 ("HID: apple: ensure the keyboard backlight is off if suspending")
Cc: stable@vger.kernel.org
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/hid/hid-apple.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index bf7dd0fbf249..a5f232cc2b66 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -91,6 +91,9 @@ struct apple_sc_backlight {
 	struct hid_device *hdev;
 };
 
+/* T2 VHCI re-enumerates the internal keyboard across system resume. */
+static int apple_backlight_resume_brightness = -1;
+
 struct apple_backlight_config_report {
 	u8 report_id;
 	u8 version;
@@ -825,6 +828,7 @@ static int apple_backlight_led_set(struct led_classdev *led_cdev,
 static int apple_backlight_init(struct hid_device *hdev)
 {
 	int ret;
+	int brightness;
 	struct apple_sc *asc = hid_get_drvdata(hdev);
 	struct apple_backlight_config_report *rep;
 
@@ -860,13 +864,20 @@ static int apple_backlight_init(struct hid_device *hdev)
 	asc->backlight->cdev.name = "apple::kbd_backlight";
 	asc->backlight->cdev.max_brightness = rep->backlight_on_max;
 	asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
-	asc->backlight->cdev.flags = LED_CORE_SUSPENDRESUME;
+	/* VHCI re-enumeration restores the cached brightness in the next probe. */
+
+	brightness = READ_ONCE(apple_backlight_resume_brightness);
+	if (brightness < 0)
+		brightness = LED_OFF;
+	else
+		brightness = min_t(int, brightness, rep->backlight_on_max);
 
-	ret = apple_backlight_set(hdev, 0, 0);
+	ret = apple_backlight_set(hdev, brightness, 0);
 	if (ret < 0) {
 		hid_err(hdev, "backlight set request failed: %d\n", ret);
 		goto cleanup_and_exit;
 	}
+	asc->backlight->cdev.brightness = brightness;
 
 	ret = devm_led_classdev_register(&hdev->dev, &asc->backlight->cdev);
 
@@ -999,6 +1010,9 @@ static void apple_remove(struct hid_device *hdev)
 
 	if (asc->quirks & APPLE_RDESC_BATTERY)
 		timer_delete_sync(&asc->battery_timer);
+	if (asc->backlight)
+		WRITE_ONCE(apple_backlight_resume_brightness,
+			   asc->backlight->cdev.brightness);
 
 	hid_hw_stop(hdev);
 }
-- 
2.55.0

[PATCH 2/2] HID: apple: use the standard keyboard backlight LED name

[Andre Eikmeyer]

The T2-attached butterfly keyboard backlight is exposed as
apple::kbd_backlight. This leaves the color field empty and gives
userspace a model-specific name for the same white keyboard-backlight
function exposed by Magic Keyboards.

Magic Keyboard backlight support was added later and already follows the
current LED naming convention. As a result, userspace has to handle two
different names for the same function.

We should use :white:kbd_backlight for both implementations. This follows
the LED color and function naming convention and lets userspace discover
either keyboard generation without a special case for the butterfly
models.

Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/hid/hid-apple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index a5f232cc2b66..0f30cf4670ce 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -861,7 +861,7 @@ static int apple_backlight_init(struct hid_device *hdev)
 	}
 
 	asc->backlight->hdev = hdev;
-	asc->backlight->cdev.name = "apple::kbd_backlight";
+	asc->backlight->cdev.name = ":white:" LED_FUNCTION_KBD_BACKLIGHT;
 	asc->backlight->cdev.max_brightness = rep->backlight_on_max;
 	asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
 	/* VHCI re-enumeration restores the cached brightness in the next probe. */
-- 
2.55.0

Re: [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the `LED_CORE_SUSPENDRESUME` flag reverts a previous power-management fix, causing the keyboard backlight to remain powered on during system suspend.
--

--- Patch [1]: [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume ---
commit 2758811c67b214fbd3316fec3f21127f8166f490
Author: Andre Eikmeyer <dev@deq.rocks>

HID: apple: preserve keyboard backlight across T2 resume

This commit fixes an issue where the T2 virtual USB host controller
re-enumerates the internal keyboard on resume, resulting in the backlight
starting off. It caches the brightness during removal and restores it
during the subsequent probe.

Fixes: 1f95a6cd5ad7 ("HID: apple: ensure the keyboard backlight is off if suspending")
Cc: stable@vger.kernel.org

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

[ ... ]

> @@ -860,13 +864,20 @@ static int apple_backlight_init(struct hid_device *hdev)
>  	asc->backlight->cdev.name = "apple::kbd_backlight";
>  	asc->backlight->cdev.max_brightness = rep->backlight_on_max;
>  	asc->backlight->cdev.brightness_set_blocking = apple_backlight_led_set;
> -	asc->backlight->cdev.flags = LED_CORE_SUSPENDRESUME;
> +	/* VHCI re-enumeration restores the cached brightness in the next probe. */

[Severity: High]
Does removing the LED_CORE_SUSPENDRESUME flag cause the backlight to remain
on during system suspend?

The T2 butterfly keyboard hardware does not automatically power off its
backlight when suspending. When this flag is removed, the LED subsystem
bypasses led_classdev_suspend(), leaving the hardware illuminated and
draining the battery.

The new caching mechanism in apple_remove() successfully tracks the user
brightness preference, as led_classdev_suspend() only alters the physical
hardware state without modifying the logical led_cdev->brightness.

Could the device initialization issue have been caused by the hardcoded
apple_backlight_set(hdev, 0, 0) during probe, rather than the presence of
this suspend flag?

>  
> -	ret = apple_backlight_set(hdev, 0, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718121527.15924-1-dev@deq.rocks?part=1

Re: [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing endianness conversion for USB HID report u16 fields.
--

--- Patch [2]: [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name ---
commit fe5b7459cd5f35bce672b51255250e2d12f88cf4
Author: Andre Eikmeyer <dev@deq.rocks>

HID: apple: use the standard keyboard backlight LED name

This commit changes the exposed LED name for the T2-attached butterfly
keyboard backlight from apple::kbd_backlight to the standard
:white:kbd_backlight naming convention. This allows userspace to discover
and handle both the butterfly models and the Magic Keyboards using the same
name without special casing.

> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
> @@ -860,8 +860,8 @@ static int apple_backlight_init(struct hid_device *hdev)
>  	}
>  
>  	asc->backlight->hdev = hdev;
> -	asc->backlight->cdev.name = "apple::kbd_backlight";
> +	asc->backlight->cdev.name = ":white:" LED_FUNCTION_KBD_BACKLIGHT;
>  	asc->backlight->cdev.max_brightness = rep->backlight_on_max;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but does
this code lack endianness conversion for the USB HID report fields?

According to the USB HID standard, multi-byte fields are little-endian.
Since rep->backlight_on_max is a u16 read directly from the raw device
report in apple_backlight_init(), could this result in byte-swapped values
on big-endian architectures and cause an out-of-bounds max_brightness?

A similar issue seems to be present in apple_backlight_set() where native
u16 values for rep->backlight and rep->rate are written into the report
struct without CPU to little-endian conversion before transmission.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718121527.15924-1-dev@deq.rocks?part=2

Re: [PATCH 1/2] HID: apple: preserve keyboard backlight across T2 resume

[Andre Eikmeyer]

Thanks for pointing this out.

LED_CORE_SUSPENDRESUME was needed with the older apple-bce suspend
implementation for which the original fix was tested. I maintain the
apple-bce source consumed by T2Linux as well as its split replacement,
t2bce.

Both current implementations now suspend the VHCI deterministically and
re-enumerate its devices after resume. The transport takes the keyboard
device offline during suspend, so its backlight does not remain powered
when LED_CORE_SUSPENDRESUME is removed. On resume, however, the flag makes
the LED core issue a synchronous request through the stale HID device
before re-enumeration completes, resulting in -ENODEV.

The new behavior was tested on a MacBookPro15,1 with a butterfly keyboard
and a MacBookAir9,1 with a Magic Keyboard. In both cases the backlight is
off during suspend and restored correctly after resume.

Re: [PATCH 2/2] HID: apple: use the standard keyboard backlight LED name

[Andre Eikmeyer]

Thanks. This is a valid pre-existing issue.

The multi-byte fields in these raw HID reports are little-endian. The
backlight configuration values should be converted with le16_to_cpu(), and
the brightness and transition rate should be written with cpu_to_le16().

This predates the LED naming change, so I will address it in a separate
patch rather than mix it into this one.