linux-kernel - Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw

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]

Date:   Tue, 5 Jun 2018 09:35:04 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Darren Hart <dvhart@...radead.org>
Cc:     Daniel Drake <drake@...lessm.com>, Chris Chiu <chiu@...lessm.com>,
        Corentin Chary <corentin.chary@...il.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        acpi4asus-user <acpi4asus-user@...ts.sourceforge.net>,
        Linux Upstreaming Team <linux@...lessm.com>,
        Bastien Nocera <hadess@...ess.net>,
        Benjamin Berg <benjamin@...solutions.net>
Subject: Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API
 on kbd brightness change

Hi,

On 05-06-18 04:31, Darren Hart wrote:
> On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 04-06-18 15:51, Daniel Drake wrote:
>>> On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede <hdegoede@...hat.com> wrote:
>>>> Is this really a case of the hardware itself processing the
>>>> keypress and then changing the brightness *itself* ?
>>>>
>>>>   From the "[PATCH 2/2] platform/x86: asus-wmi: Add keyboard backlight
>>>> toggle support" patch I get the impression that the driver is
>>>> modifying the brightness from within the kernel rather then the
>>>> keyboard controller are ACPI embeddec-controller doing it itself.
>>>>
>>>> If that is the case then the right fix is for the driver to stop
>>>> mucking with the brighness itself, it should simply report the
>>>> right keyboard events and export a led interface and then userspace
>>>> will do the right thing (and be able to offer flexible policies
>>>> to the user).
>>>
>>> Before this modification, the driver reports the brightness keypresses
>>> to userspace and then userspace can respond by changing the brightness
>>> level, as you describe.
>>>
>>> You are right in that the hardware doesn't change the brightness
>>> directly itself, which is the normal usage of LED_BRIGHT_HW_CHANGED.
>>>
>>> However this approach was suggested by Benjamin Berg and Bastien
>>> Nocera in the thread: Re: [PATCH v2] platform/x86: asus-wmi: Add
>>> keyboard backlight toggle support
>>> https://marc.info/?l=linux-kernel&m=152639169210655&w=2
>>>
>>> The issue is that we need to support a new "keyboard backlight
>>> brightness cycle" key (in the patch that follows this one) which
>>> doesn't fit into any definitions of keys recognised by the kernel and
>>> likewise there's no userspace code to handle it.
>>>
>>> If preferred we could leave the standard brightness keys behaving as
>>> they are (input events) and make the new special key type directly
>>> handled by the kernel?
>>
>> I'm sorry that Benjamin and Bastien steered you in this direction,
>> IMHO none of it should be handled in the kernel.
>>
>> Anytime any sort of input is directly responded to by the kernel
>> it is a huge PITA to deal with from userspace. The kernel will have
>> a simplistic implementation which almost always is wrong.
>>
>> Benjamin, remember the pain we went through with rfkill hotkey
>> presses being handled in the kernel ?
>>
>> And then there is the whole acpi_video.brightness_switch_enabled
>> debacle, which is an option which defaults to true which causes
>> the kernel to handle LCD brightness key presses, which all distros
>> have been patching to default to off for ages.
>>
>> To give a concrete example, we may want to implement software
>> dimming / auto-off of the kbd backlight when the no keys are
>> touched for x seconds. This would seriously get in the way of that.
>>
>> So sorry, but NACK to this series.
> 
> So if instead of modifying the LED value, the kernel platform drivers
> converted the TOGGLE into a cycle even by converting to an UP event
> based on awareness of the plaform specific max value and the read
> current value, leaving userspace to act on the TOGGLE/UP events - would
> that be preferable?

I was thinking about doing that as a workaround myself, but I'm not sure
that is a good idea (because of e.g. races with userspace auto-correcting
the brightness based on ambient light and/or keyboard activity).

As I see it there are 2 proper solutions to this:

1) Make userspace treat KEY_KBDILLUMTOGGLE as a cycle key, this is
what the kbd-backlight on most laptops with a single hotkey (*) does
in cases where this is handled in firmware, rather then left to the
OS. The handled in firmware is the case which I created the
led_classdev_notify_brightness_hw_changed() API for. This would be
my preferred solution and I believe that Benjamin is discussing this
with Bastien ATM.

2) Add a new KEY_KBDILLUMCYCLE event and send that for the TOGGLE code
on these laptops.

Regards,

Hans



*) Rather then separate up / down hotkeys


> 
> Something like:
> 
> 	if (code == TOGGLE && ledval < ledmax)
> 		code = UP;
> 
> 	sparse_keymap_report_event(..., code, ...)
> 
> }
>