| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABxCQKug=Z2U+tT5YA1yDUW6FQVMHHHM-efmaTak-U+0pLxpNw@mail.gmail.com>
Date: Mon, 9 Feb 2026 13:17:08 +0900
From: Vishnu Sankar <vishnuocv@...il.com>
To: Rong Zhang <i@...g.moe>
Cc: Hans de Goede <hansg@...nel.org>, mpearson-lenovo@...ebb.ca, hmh@....eng.br,
derekjohn.clark@...il.com, ilpo.jarvinen@...ux.intel.com,
ibm-acpi-devel@...ts.sourceforge.net, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org, vsankar@...ovo.com
Subject: Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness
Hi Hans and Rong,
Thank you so much for the comments.
On Mon, Feb 9, 2026 at 6:03 AM Rong Zhang <i@...g.moe> wrote:
>
> Hi Hans, Vishnu and Mark,
>
> On Sun, 2026-02-08 at 11:54 +0100, Hans de Goede wrote:
> > Hi Vishnu,
> >
> > On 4-Feb-26 00:22, Vishnu Sankar wrote:
> > > Dynamically detect keyboard backlight capabilities and set
> > > max_brightness correctly (2 for old models, 3 for new models
> > > with Auto mode).
> >
> > Thank you for your patch.
> >
> > If I understand this correctly, writing 3 as level does not
> > make the backlight more bright then writing 2, but instead
> > it puts the backlight in some auto mode ?
> >
This is correct.
> > If I've that correct then userspace should keep seeing
> > a range of 0 - 2 and the special auto mode value should
> > be reported / be made settable through a separate als_enabled
> > sysfs attribute under the LED class device. See:
> >
> > Documentation/ABI/testing/sysfs-platform-dell-laptop
> >
> > You can add extra attributes there by setting the groups
> > member of the struct led_classdev, see kbd_led_groups[]
> > in drivers/platform/x86/dell/dell-laptop.c, except that
> > you should use a .is_visible callback to only show this
> > on hw which supports it and you only need 1 group with
> > 1 attribute.
Got it.
I believe, once Rong's changes come up, We may need to handle it a bit
different.
>
> When I implemented "als_enabled" for ideapad-laptop, Mark Pearson
> suggested it'd better to introduce "something similar to
> LED_BRIGHT_HW_CHANGED" rather than using custom attributes, as "this is
> going to be a common feature across multiple vendors it might need
> doing at a common layer". Also, auto mode can be activated by HW as a
> result of user input, so we need an approach to notify userspace just
> like what LED_BRIGHT_HW_CHANGED does. More importantly, the read value
> of the brightness attribute becomes nonsense when auto mode is on. This
> matches the semantic of hw control trigger.
>
> I agreed with Mark and had a proposal of allowing HW to initiate a
> transition from "none" to hw control trigger and vice versa. See the
> thread in
> https://lore.kernel.org/all/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com/
Thank you Rong.
We had a discussion a few months back about this internally.
I will discuss with Mark on this approach again.
>
> I hadn't push it further due to other things taking the priority,
> though I already had a PoC back to then. I quickly rebased the PoC with
> some cleanups and put it here for preview:
>
> https://github.com/Rongronggg9/linux/tree/leds-trigger-hw-changed
>
> I will find some time to refine it and send an RFC series.
Thank you.
>
> Thanks,
> Rong
>
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >
> > >
> > > Suggested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
> > > Signed-off-by: Vishnu Sankar <vishnuocv@...il.com>
> > > ---
> > > drivers/platform/x86/lenovo/thinkpad_acpi.c | 33 ++++++++++++++++++---
> > > 1 file changed, 29 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > index cc19fe520ea9..f670cdd1791e 100644
> > > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > @@ -5043,6 +5043,9 @@ static struct ibm_struct video_driver_data = {
> > > static enum led_brightness kbdlight_brightness;
> > > static DEFINE_MUTEX(kbdlight_mutex);
> > >
> > > +/* Maximum level supported by hardware, will be updated in init */
> > > +static int kbdlight_max_level = 2;
> > > +
> > > static int kbdlight_set_level(int level)
> > > {
> > > int ret = 0;
> > > @@ -5050,6 +5053,10 @@ static int kbdlight_set_level(int level)
> > > if (!hkey_handle)
> > > return -ENXIO;
> > >
> > > + /* Validate against detected max level */
> > > + if (level < 0 || level > kbdlight_max_level)
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&kbdlight_mutex);
> > >
> > > if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
> > > @@ -5075,6 +5082,7 @@ static int kbdlight_get_level(void)
> > > if (status < 0)
> > > return status;
> > >
> > > + /* Status can be 0, 1, 2, or 3 (Auto) */
> > > return status & 0x3;
> > > }
> > >
> > > @@ -5143,7 +5151,7 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
> > > static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
> > > .led_classdev = {
> > > .name = "tpacpi::kbd_backlight",
> > > - .max_brightness = 2,
> > > + .max_brightness = 2, /*Initial value, will be updated in init*/
> > > .flags = LED_BRIGHT_HW_CHANGED,
> > > .brightness_set_blocking = &kbdlight_sysfs_set,
> > > .brightness_get = &kbdlight_sysfs_get,
> > > @@ -5167,6 +5175,17 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
> > > kbdlight_brightness = kbdlight_sysfs_get(NULL);
> > > tp_features.kbdlight = 1;
> > >
> > > + /* Detect hardware capabilities and set max_brightness */
> > > + if (acpi_evalf(hkey_handle, NULL, "MLCS", "dd", 3)) {
> > > + /* MLCS accepts level 3 - new ThinkPad with Auto mode */
> > > + kbdlight_max_level = 3;
> > > + tpacpi_led_kbdlight.led_classdev.max_brightness = 3;
> > > + } else {
> > > + /* MLCS rejects level 3 - old ThinkPad */
> > > + kbdlight_max_level = 2;
> > > + tpacpi_led_kbdlight.led_classdev.max_brightness = 2;
> > > + }
> > > +
> > > rc = led_classdev_register(&tpacpi_pdev->dev,
> > > &tpacpi_led_kbdlight.led_classdev);
> > > if (rc < 0) {
> > > @@ -5201,6 +5220,7 @@ static int kbdlight_set_level_and_update(int level)
> > > static int kbdlight_read(struct seq_file *m)
> > > {
> > > int level;
> > > + int i;
> > >
> > > if (!tp_features.kbdlight) {
> > > seq_printf(m, "status:\t\tnot supported\n");
> > > @@ -5210,9 +5230,13 @@ static int kbdlight_read(struct seq_file *m)
> > > seq_printf(m, "status:\t\terror %d\n", level);
> > > else
> > > seq_printf(m, "status:\t\t%d\n", level);
> > > - seq_printf(m, "commands:\t0, 1, 2\n");
> > > - }
> > >
> > > + /* Show available commands based on hardware */
> > > + seq_puts(m, "commands:\t0");
> > > + for (i = 1; i <= tpacpi_led_kbdlight.led_classdev.max_brightness; i++)
> > > + seq_printf(m, ", %d", i);
> > > + seq_puts(m, "\n");
> > > + }
> > > return 0;
> > > }
> > >
> > > @@ -5230,7 +5254,8 @@ static int kbdlight_write(char *buf)
> > > return res;
> > > }
> > >
> > > - if (level >= 3 || level < 0)
> > > + /* Validate against max level */
> > > + if (level < 0 || level > tpacpi_led_kbdlight.led_classdev.max_brightness)
> > > return -EINVAL;
> > >
> > > return kbdlight_set_level_and_update(level);
--
Regards,
Vishnu Sankar
+817015150407 (Japan)
Powered by blists - more mailing lists