linux-kernel - Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170616075220.GV5248@pali>
Date:   Fri, 16 Jun 2017 09:52:20 +0200
From:   Pali Rohár <pali.rohar@...il.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc:     mjg59@...f.ucam.org, dvhart@...radead.org,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight
 sysfs interface

Hi!

On Friday 16 June 2017 15:35:39 Kai-Heng Feng wrote:
> Dell Latitude 3160 does not have keyboard backlight, but there is a
> sysfs interface for it, which does nothing at all.
> 
> KBD_LED_OFF_TOKEN is the only token can be found. Since it doesn't have
> KBD_LED_ON_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
> does not support keyboard backlight.
> 
> Reports keyboard backlight is supported only when tokens other than
> KBD_LED_OFF_TOKEN can be found.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index ec202094bd50..743d7ce8c0c8 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1510,7 +1510,11 @@ static void kbd_init(void)
>  	ret = kbd_init_info();
>  	kbd_init_tokens();
>  
> -	if (kbd_token_bits != 0 || ret == 0)
> +	/*
> +	 * If KBD_LED_OFF_TOKEN is the only token,
> +	 * consider there is no keyboard backlight.
> +	 */
> +	if ((kbd_token_bits & ~BIT(5)) != 0 || ret == 0)

Should not this check to be rather:

(kbd_token_bits != 0 && (kbd_token_bits & BIT(KBD_LED_OFF_TOKEN)) != BIT(KBD_LED_OFF_TOKEN))

To express that we have at least one token at it is different from
KBD_LED_OFF_TOKEN token?

>  		kbd_led_present = true;
>  }
>  

And more important, there are three ways how to control keyboard
backlight level:

1) Via SMBIOS token
2) Via SMBIOS call 4/11/0x2 (arg2, byte0)
3) Via SMBIOS call 4/11/0x2 (arg3, byte2)

You are adding special case when only one SMBIOS toekn OFF is present
which belongs to 1).

Therefore there should be same check for 2) and 3) that there are more
the one option to set...

-- 
Pali Rohár
pali.rohar@...il.com