Re: [PATCH] platform/x86: tpacpi: Mark charging LEDs as safe

[Rong Zhang <i@rong.moe>]

Hi Josh, Mark,

On Thu, 2026-11-05 at 11:24 -0800, Josh Triplett wrote:
> tpacpi can expose the charging LEDs as `tpacpi:orange:batt` (LED 1) and
> `tpacpi:green:batt` (LED 2). However, at least on my X1 Carbon Gen 11,
> these LEDs get hidden because `TPACPI_SAFE_LEDS` doesn't allow them.
> 
> Add these to `TPACPI_SAFE_LEDS`, so that tpacpi can expose them.

Please see the commit message of commit a4d5effcc737 ("thinkpad-acpi:
restrict access to some firmware LEDs").

The term "unsafe LED" exists because LEDs are write-only for software and
fire-and-forget for the EC. By taking the control of an unsafe LED, e.g.,
attaching an LED trigger, any warning signal from the EC can no longer be
displayed by the LED.

When it comes to battery LEDs, they are definitely unsafe to override.
After all, batteries are dangerous things when they malfuction. Thus, I
don't think it's suitable to enable it for all users.

> 
> This was previously proposed in
> https://lore.kernel.org/lkml/20250403040846.7834-1-martin@arp242.net/ .
> 
> I'm currently toggling these using write_support via ec_sys, and would
> prefer to handle these via the normal LED mechanism.

Since not providing them via the LED core does not really prevents users
to control them, I am also thinking about adding a debugfs attribute that
unlocks unsafe LEDs and taints the kernel.

I believe tainting the kernel when overriding unsafe LEDs would be a
better approach. This is also one of the reason why taint flags exist,
i.e., indicating that something unsupported has happened. Multiple
drivers have taken the approach (hint: grep TAINT_USER).

We can also make the overriding procedure tedious to hopefully block
users who don't know what they are doing:

- Drop CONFIG_THINKPAD_ACPI_UNSAFE_LEDS, so there is no longer a
permanent switch
- Unsafe LEDs are also registered, but with LED_SYSFS_DISABLE set to
disable write access
- Add a debugfs attribute unlock_unsafe_led to every unsafe LED, which
can be written to unlock the LED's write access
- When unlocking, taints the kernel with TAINT_USER

Users must write to the attribute to unlock an unsafe LED. They can't
unlock all unsafe LEDs at once. Instead, they must unlock whatever they
desire one by one.

From the said commit message, it seems that the reason why
CONFIG_THINKPAD_ACPI_UNSAFE_LEDS was introduced was that Lenovo didn't
want to support the use case of overriding unsafe LEDs. So...

Mark, what do you think of the approach?

I will send a patch implementing my proposal soon. You can reply there.

Thanks,
Rong

> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index 4f81855ad721..50a8cff5b374 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5608,7 +5608,7 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>  	"tpacpi::unknown_led3",
>  	"tpacpi::thinkvantage",
>  };
> -#define TPACPI_SAFE_LEDS	0x1481U
> +#define TPACPI_SAFE_LEDS	0x1487U
>  
>  static inline bool tpacpi_is_led_restricted(const unsigned int led)
>  {