Re: [RFC PATCH v2 0/2] Add support for auto keyboard backlight

[Rong Zhang]

Hi Qiling,

On Mon, 2026-11-02 at 23:10 +0800, Qiling Zhu wrote:
> Hi Mark, Rong, Ilpo,
> 
> Thanks for the review comments on v1. This v2 addresses them and is
> now split into a pure refactor (1/2) and the feature addition (2/2),
> as requested.

You don't need to refresh the series in a hurry, unless you've got enough
reviews or may miss the cycle. Asking an LLM agent to refresh the series
is easy, but a human being needs time to review your series.

Please leave some time for others to respond.

> 
> Summary of the changes:
> 
> * Kconfig: the "depends on LEDS_TRIGGERS_HW_CHANGED" is gone. While
>   at it I also added a "select LEDS_TRIGGERS": without it the trigger
>   helpers become no-op stubs and the whole feature would silently
>   disappear in configurations that don't opt into LEDS_TRIGGERS,
>   which seemed worse than a small select. HW_CHANGED support stays
>   optional; without it the trigger still works, only the
>   hardware-initiated transitions are not reported. The select goes
>   beyond what was suggested, so please let me know if you'd rather
>   drop it.
> 
> * The auto mode is never reported to the LED core as level 3: a
>   single helper maps it to a brightness of 0, used by both
>   brightness_get and the brightness_hw_changed notification, so
>   level 3 has no path into the LED core at all. Internal state
>   tracking compares raw EC levels.
> 
> * Writing a regular level through /proc/acpi/ibm/kbdlight now drops
>   the trigger when leaving auto mode. Instead of raising the
>   hardware-control notification and letting the worker handle it, I
>   went through the same path brightness_store() uses: drop the
>   trigger and hand the new level to the LED core, which applies it
>   after the LED_OFF queued by dropping the trigger. That way proc
>   and sysfs behave alike.
> 
> * suspend/resume no longer touch the LED while auto mode is active
>   (v1 would have restored brightness 2 there and left the trigger
>   state inconsistent, as Rong pointed out — thank you). The LED core
>   is now kept out of suspend entirely in auto mode; resume just
>   re-reads the EC level and reconciles. My reasoning is that in auto
>   mode the EC owns the backlight, so the driver should not make
>   decisions on its behalf — the firmware already turns the backlight
>   off for suspend and restores it on resume by itself. This is based
>   on observing my own machine (s2idle), so I may be wrong about
>   other models. The non-auto paths are unchanged: suspend still
>   turns the backlight off and resume still restores the previous
>   level.

If the firmware properly handles it when hw brightness != 3, you can skip
led_classdev_{suspend,resume}() completely as long as the device supports
auto mode. They are only needed when the firmware doesn't take care of
the keyboard backlight. Skipping them should make our life mush easier.

Of course, we still need an approach to determine the support. That's why
we should leave some time for Vishnu and Mark to answer the question.

> 
> One thing Sashiko (the Linux Foundation's AI review system)
> brought up in its review of v1, and which I then verified in the
> code, seems worth raising here: led_trigger_hw_control_changed_worker()
> reads the transition flag before acquiring the LED's trigger_lock,
> and the read and the transition are not atomic. If, while a worker
> carrying "deactivate" is still on its way, userspace re-selects the
> private trigger, the worker then drops the trigger that userspace
> just set, and no further event arrives to reconcile the state.
> I haven't reliably reproduced this, but the window looks real, and
> the ideapad wiring seems equally exposed. My feeling is that this
> would be better addressed in the trigger core than worked around in
> each driver, but I may be missing something — Rong, I'd appreciate
> your thoughts.

It is an unfortunate side-effect of the workqueue deferral mechanism. The
mechanism was introduced in order that:

- the LED driver can notify hw control change in an atomic context
- the LED driver can properly serialize notification handling, trigger
[de]activation, brightness_set[_blocking]() and hw_offloaded() callback
without worrying about lock reentrance

As for ideapad-laptop, it needs the second guarantee, as all said
functions or callbacks need to acquire the same mutex to prevent TOCTOU
issues.

IMHO the race condition is not a big deal. The interface was designed
with an assumption: it's the user who decides to press the shortcut
(Fn+Space) and/or select the trigger via sysfs. It can only be barely
reproduced in fuzz tests with scripts and robotic fingers, which is as
boring as fuzzing the bind/unbind interface to stress naive drivers...

That being said, I am thinking of working around this by querying
hw_offloaded() in the worker. I will find some time to prepare a patch to
see if the approach works and submit it. It won't change the interface,
so patches can be applied in an arbitrary order. Please ignore the issue
for the time being (OTOH, you don't need to rebase on top of the said
patch when it's ready).

Thanks,
Rong

> 
> A few observations that may be useful for the discussion:
> 
> * I disassembled the DSDT and instrumented the driver to see what
>   MLCS returns: it is the EC status word, and bit 31 looks like an
>   error flag — the DSDT skips the hotkey event when it is set. On my
>   machine it reads 0 for all levels, with no level echo, and the
>   error path is not reachable. That is why activation verifies with a
>   read-back rather than the return value.
> 
> * The 0x1012 event is raised by the DSDT itself: MLCS issues it when
>   the write succeeded, the level actually changed, and the hotkey is
>   not masked. This explains why driver-initiated writes also see the
>   event and why rewriting the same level doesn't.
> 
> * The Fn+Space cycle order on this machine is 0 -> 3 -> 1 -> 2 -> 0,
>   confirmed by testing; sharing in case it is useful for comparison
>   across models. Nothing in the code or tests relies on it.
> 
> An open question from v1 that I'm still unsure about: on models
> without an ALS the auto mode presumably doesn't exist — how should
> the driver handle that? Mark, do you know what the EC does when
> asked for level 3 on such machines, and whether there is a reliable
> way to tell at probe time whether auto mode is supported? For now
> the trigger is registered unconditionally on keyboard-backlight-
> capable machines.
> 
> Vishnu, Rong mentioned in the v1 discussion that you are also working
> on the same support using the hw control trigger framework — how is
> that going? If you have answers to the questions above, or if there
> is a chance to combine the two efforts, I'd be happy to coordinate.
> 
> Tested on the same ThinkPad X1 Carbon Gen 14 (linux-next
> next-20261030) with targeted tests and on-hardware verification of
> the affected paths, including suspend/resume; checkpatch --strict
> clean.
> 
> Regards,
> Qiling Zhu
> 
> This patch was developed with LLM assistance; I reviewed every design
> decision against the framework sources and validated the behavior on
> real hardware myself.
> 
> Qiling Zhu (2):
>   platform/x86: thinkpad_acpi: Extract kbdlight_notify_change() helper
>   platform/x86: thinkpad_acpi: Add support for auto keyboard backlight
> 
>  drivers/platform/x86/lenovo/Kconfig         |   1 +
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 179 +++++++++++++++++---
>  2 files changed, 159 insertions(+), 21 deletions(-)