Re: Userspace API for per key backlight for non HID (no hidraw) keyboards

[Werner Sembach <wse@tuxedocomputers.com>]

Hi Hans and Armin,

Am 17.01.24 um 20:03 schrieb Armin Wolf:
> Am 17.01.24 um 17:50 schrieb Hans de Goede:
>
>> Hi All,
>>
>> On 11/27/23 11:59, Werner Sembach wrote:
>>
>> <snip>
>>
>>> I also stumbled across a new Problem:
>>>
>>> We have an upcoming device that has a per-key keyboard backlight, but does 
>>> the control completely via a wmi/acpi interface. So no usable hidraw here 
>>> for a potential userspace driver implementation ...
>>>
>>> So a quick summary for the ideas floating in this thread so far:
>>>
>>> 1. Expand leds interface allowing arbitrary modes with semi arbitrary 
>>> optional attributes:
>>>
>>>      - Pro:
>>>
>>>          - Still offers all default attributes for use with UPower
>>>
>>>          - Fairly simple to implement from the preexisting codebase
>>>
>>>          - Could be implemented for all (to me) known internal keyboard 
>>> backlights
>>>
>>>      - Con:
>>>
>>>          - Violates the simplicity paradigm of the leds interface (e.g. with 
>>> this one leds entry controls possible multiple leds)
>> So what you are suggesting here is having some way (a-z + other sysfs attr?)
>> to use a single LED class device and then extend that to allow setting all
>> keys ?
>>
>> This does not seem like a good idea to me and this will also cause issues
>> when doing animations in software, since this API will likely be slow.
>>
>> And if the API is not slow, then it will likely involve some sort
>> of binary sysfs file for setting multiple keys rather then 1
>> file per key which would break the normal 1 file per setting sysfs
>> paradigm.
>>
>>> 2. Implement per-key keyboards as auxdisplay
>>>
>>>      - Pro:
>>>
>>>          - Already has a concept for led positions
>> With a "concept" you mean simple x,y positioning or is
>> there something more advanced here that I'm aware of ?
>>
>>>          - Is conceptually closer to "multiple leds forming a singular entity"
>>>
>>>      - Con:
>>>
>>>          - No preexisting UPower support
>>>
>>>          - No concept for special hardware lightning modes
>>>
>>>          - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>> Hmm, so there is very little documentation on this and what
>> docs there is: Documentation/admin-guide/auxdisplay/cfag12864b.rst
>> as well as the example program how to uses this suggests that
>> this is using the old /dev/fb# interface which we are sorta
>> trying to retire.
>>
>>
>>> 3. Implement in input subsystem
>>>
>>>      - Pro:
>>>
>>>          - Preexisting concept for keys and key purpose
>>>
>>>      - Con:
>>>
>>>          - Not in scope for subsystem
>>>
>>>          - No other preexisting light infrastructure
>> Dmitry actually recently nacked the addition of
>> a LED_MIC_MUTE define to include/uapi/linux/input-event-codes.h
>> which was intended to be able to allow the input LED support
>> with standard HID mic-mute leds (spk-mute is already supported
>> this way).
>>
>> Dmitry was very clear that no new LEDs must be added and
>> that any new LED support should be done through the LED
>> subsytem, so I do not think that something like this
>> is going to fly.
>>
>>
>>> 4. Implement a simple leds driver only supporting a small subset of the 
>>> capabilities and make it disable-able for a userspace driver to take over
>>>
>>>      - Pro:
>>>
>>>          - Most simple to implement basic support
>>>
>>>          - In scope for led subsystem simplicity paradigm
>>>
>>>      - Con:
>>>
>>>          - Not all built in keyboard backlights can be implemented in a 
>>> userspace only driver
>> Right, so this is basically what we have been discussing in the other
>> part of the thread with the:
>>
>> /sys/bus/hid/devices/0003:xxxx:xxxx.xxxx/disable_kbd_backlight_support
>>
>> proposal to unregister the kernel's LED class device and then
>> allow userspace to do whatever it wants through /dev/hidraw
>> without the kernel also trying to access the backlight
>> functionality at the same time.
>>
>> AFAIK there already is a bunch of userspace support for
>> per key addressable kbd RGB backlights using hidraw support,
>> so this way we can use the momentum / code of these existing
>> projects, at least for existing hidraw keyboards and adding
>> support for:
>>
>> /sys/bus/hid/devices/0003:xxxx:xxxx.xxxx/disable_kbd_backlight_support
>>
>> to these existing projects should be simple.
>>
>> Yet this will not work for your mentioned "control completely
>> via a wmi/acpi interface". Still I think we should go the same
>> route for those adding a misc-char device or something like
>> that to allow making WMI calls from userspace (like Windows
>> can do). Maybe with an allow list per GUID to only allow
>> specific calls, so that we can avoid possible dangerous calls.
>>
>> Armin Wolf recently became the WMI bus maintainer.
>>
>> Armin, we are discussing how to deal with (laptop) keyboards
>> which have a separate RGB LED per key and how to control
>> those LEDs.
>>
>> So far per key addressable keyboard backlighting has always
>> been HID based, so any existing support is just userspace
>> based using /dev/hidraw. In my experience the problem with
>> supporting gaming peripherals is that there is interest in it,
>> but not really enough interest to keep a sustained momentum
>> behind projects, especially not when it comes to taking code
>> from a fun weekend hack to upstreaming them into bigger
>> projects like the kernel.
>>
>> So I would like to offer some sort of easy accessible
>> API to userspace for accessing this, basically allowing
>> userspace drivers for the LED part of the keyboard which
>> in some cases will involve making WMI calls from userspace.
>>
>> So, Armin, what do you think about a way of allowing
>> (filtered) WMI calls from userspace through say
>> a misc-char device + ioctls or something like that?
>>
>> Werner atm I personally do think that option 4. from
>> your list is the way to go. Mainly because designing
>> a generic kernel API for all bells and whistles of gaming
>> hw is very tricky and would require a large ongoing
>> effort which I just don't see happening (based on
>> past experience).
>>
>> Regards,
>>
>> Hans
>>
> Hi,
>
> i can understand your concerns, but i strongly advise against a generic WMI 
> userspace API.
> The reasons for this are:
>
> 1. We are still unable to parse (and use) the binary MOF buffers describing 
> the WMI devices,
> so all of that would require the driver parsing a raw byte buffer. In this 
> case a separate
> misc device managed by the driver would basically do the same.
>
> 2. Many WMI implementations are like RWEverything implemented inside the ACPI 
> firmware, so
> most devices will require the driver to do excessive filtering. Many 
> implementations also do
> no proper input validation either so the driver has to know all possible use 
> cases since he
> has to protect the buggy ACPI firmware from userspace attacks.

Or the WMI has a straight forward arbitrary read/write function into EC ram 
(e.g. all Uniwill/TongFang devices).

The filtering would need to be explicit whitelisting of wmi-calls+arguments. 
Don't know if this would reduce complexity for the kernel.

>
> Regarding point number 2, i just had to contact Asus so that they remove a 
> broken WMI interface
> from my motherboard or else a simple application could crash the Windows 
> kernel. This firmware
> is (sadly) being designed as an internal API and thus unstable beyond all 
> imagination.
>
> For HID devices, a userspace driver might be OK since they are somewhat 
> isolated from the remaining
> hardware, but WMI is basically a kernel bypass for ACPI firmware calls, so 
> userspace could easily
> attack the kernel is way.
>
> Personally, i would prefer extending the LED subsystem to support zone-like 
> devices with many LEDs,
> as this would prevent userspace from having to tinker with the hardware behind 
> the kernels back.
> Other highly device-specific features could be implemented with a 
> driver-specific misc device.

Something like my earlier suggestion "[...] adds a new entry zones_count. 
multi_intensity has now colors count * zones_count entries. aka a RGB keyboard 
with 126 leds would take 378 values for multi_intensity [...]"?

Setting all with one file access to multi_intensity could make it somewhat 
performant as Hans already mentioned, but also would violate the one file one 
led paradigm.

Or formulated differently: How should the sysfs folder look:

leds/
     rgb:kbd_backlight_a/
         brightness
         multi_intensity
     rgb:kbd_backlight_b/
         brightness
         multi_intensity
     ...

or

leds/
     rgb:kbd_backlight/
         brightness
         multi_intensity_a
         multi_intensity_b
         ...

or

leds/
     rgb:kbd_backlight/
         brightness
         zones_count
         multi_intensity

Personally I don't really like the idea of having the color set in 
/sys/class/leds/*:rgb:kbd_backlight/multi_intensity and e.g. the breathing mode 
in /sys/class/misc/<some_random_name>/<some_random_attribute>. Or at least there 
should be a hint in /sys/class/leds/*:rgb:kbd_backlight/ for the userspace to 
know where to look for associated additional attributes.

>
> Regarding the speed, it depends on the underlying WMI interface design if 
> smooth animations are
> even possible, since many WMI interfaces are quite slow. Can you share the 
> Binary MOF buffers
> describing the WMI interfaces in question?
Taking a colleague in the loop who currently has the device at hand. 
@Christoffer can you extract it? Is it one wmi call per key or is there a "set 
all" wmi call (because performance)?
>
> Thanks,
> Armin Wolf
>
Kind regards,

Werner