Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices

[Hans de Goede <hdegoede@redhat.com>]

Hi Armin,

On 21-Oct-24 10:26 PM, Armin Wolf wrote:
> Am 11.10.24 um 17:26 schrieb Pavel Machek:
> 
>> Hi!
>>
>>>> 1.
>>>> https://lore.kernel.org/all/6b32fb73-0544-4a68-95ba-e82406a4b188@gmx.de/
>>>> -> Should be no problem? Because this is not generally exposing wmi
>>>> calls, just mapping two explicitly with sanitized input (whitelisting
>>>> basically).
>>> It would be OK to expose a selected set of WMI calls to userspace and sanitizing the input of protect potentially buggy firmware from userspace.
>>>
>> I don't believe this is good idea. Passthrough interfaces where
>> userland talks directly to hardware are very tricky.
>>
>>> Regarding the basic idea of having a virtual HID interface: i would prefer to create a illumination subsystem instead, but i have to agree that we should be doing this
>>> only after enough drivers are inside the kernel, so we can design a
>>> suitable interface for them. For now, creating a virtual HID
>>> interface seems to be good enough.
>> I have an RGB keyboard, and would like to get it supported. I already
>> have kernel driver for LEDs (which breaks input functionality). I'd
>> like to cooperate on "illumination" subsystem.
>>
>> Best regards,
>>                                 Pavel
> 
> Sorry for taking a bit long to respond.
> 
> This "illumination" subsystem would (from my perspective) act like some sort of LED subsystem
> for devices with a high count of LEDs, like some RGB keyboards.
> 
> This would allow us too:
> - provide an abstract interface for userspace applications like OpenRGB
> - provide an generic LED subsystem emulation on top of the illumination device (optional)
> - support future RGB controllers in a generic way
> 
> Advanced features like RGB effects, etc can be added later should the need arise.
> 
> I would suggest that we model it after the HID LampArray interface:
> 
> - interface for querying:
>  - number of LEDs
>  - supported colors, etc of those LEDs
>  - position of those LEDs if available
>  - kind (keyboard, ...)
>  - latency, etc
> - interface for setting multiple LEDs at once
> - interface for setting a range of LEDs at once
> - interface for getting the current LED colors
> 
> Since sysfs has a "one value per file" rule, i suggest that we use a chardev interface
> for querying per-LED data and for setting/getting LED colors.
> 
> I do not know if mixing sysfs (for controller attributes like number of LEDs, etc) and IOCTL
> (for setting/getting LED colors) is a good idea, any thoughts?

I wonder what the advantage of this approach is over simply using HID LampArray
(emulation), openRGB is already going to support HID LampArray and since Microsoft
is pushing this we will likely see it getting used more and more.

Using HID LampArray also has the advantage that work has landed and is landing
to allow safely handing over raw HID access to userspace programs or even
individual graphical apps with the option to revoke that access when it is
no longer desired for the app to have access.

HID LampArray gives us a well designed API + a safe way to give direct access
to e.g. games to control the lighting. I really don't see the advantage of
inventing our own API here only to then also have to design + code some way to
safely give access to sandboxed apps.

Note that giving access to sandboxed apps is a lot of work, it is not just
kernel API it also requires designing a portal interface + implementing
that portal for at least GNOME, KDE and wlroots.

Personally I really like the idea to just emulate a HID LampArray device
for this instead or rolling our own API.  I believe there need to be
strong arguments to go with some alternative NIH API and I have not
heard such arguments yet.

Regards,

Hans