Re: [PATCH v3 0/7] platform/x86: thinkpad_acpi: Add support for auto keyboard backlight

[Rong Zhang <i@rong.moe>]

Hi Qiling,

Thanks for working on this.

On Sun, 2026-11-15 at 01:48 +0800, Qiling Zhu wrote:
> Hi all,
> 
> Thanks, Rong, for the two rounds of detailed review, Vishnu and Mark
> for the firmware-side answers, and Andrea for the bug report that
> became the first patch. This v3 addresses the feedback in full; the
> series grew from two to seven patches to pick up the suggested
> structure and to make the fixes independently backportable.

The next time when you send a series (i.e., PATCH x/n, n > 1), please do
not reply to the previous thread to keep it tidy.

> 
> The series in short:
> 
>   1/7  Report auto mode as brightness 0 instead of 3          (stable fix)
>   2/7  Extract kbdlight_notify_change() helpers               (refactor)
>   3/7  Sync LED core state on level writes          (write-path sync)
>   4/7  Detect auto-mode capability via MLCG bit 20            (groundwork)
>   5/7  Skip LED suspend/resume on auto-mode machines
>   6/7  Add support for auto keyboard backlight                 (the feature)
>   7/7  Reconcile EC level on resume
> 
> Changes since v2, by comment:
> 
> * The notification path is now split as suggested: a helper that accepts
>   a level and a wrapper that queries the EC (2/7), and the proc write
>   path goes through it (3/7), like commit 07f48f668fac did for the
>   ideapad FnLock LED. The manual check-then-act trigger removal is gone
>   entirely; leaving auto mode through either interface now drops the
>   trigger through the LED core.
> 
> * LED suspend/resume is skipped entirely on machines that support auto
>   mode, keyed on the capability bit rather than the current level (5/7),
>   as suggested. My testing confirms the firmware restores the previous
>   level on its own in all cases except auto mode across hibernation;
>   less state remains in the driver, and the class of races around
>   LED_SUSPENDED disappears.
> 
> * activate() is now a one-liner with no read-back verification (6/7);
>   it only writes the level, using the plain set helper. I did not use
>   the updating wrapper there on purpose: the trigger core is in the
>   middle of its own activation flow at that point, and the 0x1012 event
>   that follows the level change is expected to bring the state back in
>   sync through the regular notify path.
> 
> * Trigger registration is gated on MLCG bit 20 as per Vishnu's
>   confirmation (4/7 + 6/7).
> 
> * Andrea's report is fixed by 1/7, which reports auto mode as 0 to the
>   LED classdev and is marked for stable. It is deliberately minimal -
>   a single check, no further splitting - to keep backporting easy.
> 
> * On the proc attribute deprecation: I support the idea, deferring to
>   the thinkpad_acpi maintainers.
> 
> One addition that was not in v2: testing showed that the firmware does
> not restore auto mode across hibernation
> 

How about s2idle? And what if hw brightness = [0,2] before hibernation?

Meanwhile, could you test s2idle, hibernation and cold boots with
thinkpad_acpi blacklisted to see if hw brightness ([0,2] and 3) is
preserved? We need the raw behavior to determine our strategy.

>  - the EC comes back with the
> backlight off, without an event. Rather than re-attaching the trigger
> from the pre-suspend state, 
> 

"re-attaching"

I am not familiar with hibernation, so I am not sure if the LED core
would detach the trigger before entering hibernation. If not, why did you
say that?

Thanks,
Rong

> 7/7 simply re-reads the EC level on resume
> and routes it through the regular notification path. If the firmware
> exits auto mode across hibernation, the driver simply reflects the
> resulting EC state rather than restoring the previous mode: it stays
> out of the firmware's way and only synchronizes, which keeps the code
> simple. Happy to hear opinions.
> 
> A few notes on the result:
> 
> * Writing a level through either interface now also emits
>   brightness_hw_changed: both write paths share the same notify helper,
>   which keeps the LED core and userspace listeners consistent with the
>   new level no matter which interface was used. Rewriting the current
>   level emits nothing, matching the firmware's own
>   no-event-on-same-level semantics. Extending the notification to
>   userspace-initiated changes seemed reasonable to me - the OSD then
>   also reflects echo-based changes - but I'm happy to restrict it to
>   hardware-initiated transitions if that is considered the better
>   semantics.
> 
> * All state transitions now run under kbdlight_mutex in single
>   critical sections (query + reconcile, write + reconcile), with the
>   set and notify helpers expecting the caller to hold it. On the
>   ordering with the LED core's trigger_lock, my reasoning is that the
>   trigger callbacks (activate, hw_offloaded) are reached with
>   trigger_lock held and then take kbdlight_mutex, while the
>   notification helper is lock-free (it only queues the worker), so I
>   don't see a reverse nesting. Extra eyes on this are welcome.
> 
> * The cached level is now the raw EC level including auto mode, so the
>   kbdlight_brightness name has drifted a bit from what it holds.
>   Renaming it would touch quite a few places; I left it as is for now
>   and can do the rename as a follow-up if preferred.
> 
> One open question, mainly for Mark and Vishnu: the skip of LED
> suspend/resume assumes the firmware turns the backlight off on suspend
> and restores it on resume on all auto-mode capable machines, which is
> what I observed here for manual levels (suspend and hibernation), auto
> across suspend, and auto across hibernation coming back off. My belief
> is that most machines handle this fine, but more testing on other
> models or a look at the EC documentation from the Lenovo side would be
> needed to be sure; if an exception turns up, a DMI quirk could restore
> the old path for that machine.
> 
> Tested on my ThinkPad X1 Carbon Gen 14 (linux-next next-20261030).
> 
> I'm still learning the ways of upstream; thank you all for your time
> and reviews, and any further comments are most welcome.
> 
> This series was developed with LLM assistance; I reviewed every design
> decision against the framework sources and validated the behavior on
> real hardware myself.
> 
> Regards,
> Qiling Zhu
> 
> Qiling Zhu (7):
>   platform/x86: thinkpad_acpi: Report auto keyboard backlight mode as off
>   platform/x86: thinkpad_acpi: Extract kbdlight_notify_change() helpers
>   platform/x86: thinkpad_acpi: Sync LED core state on level writes
>   platform/x86: thinkpad_acpi: Detect auto-mode capability via MLCG
>   platform/x86: thinkpad_acpi: Skip LED suspend/resume on auto-mode machines
>   platform/x86: thinkpad_acpi: Add support for auto keyboard backlight
>   platform/x86: thinkpad_acpi: Reconcile EC level on resume