| 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: <1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe>
Date: Mon, 09 Feb 2026 04:58:28 +0800
From: Rong Zhang <i@...g.moe>
To: Hans de Goede <hansg@...nel.org>, Vishnu Sankar <vishnuocv@...il.com>,
mpearson-lenovo@...ebb.ca
Cc: 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, 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 ?
>
> 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.
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/
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.
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);
Powered by blists - more mailing lists