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

[Werner Sembach <wse@tuxedocomputers.com>]

Hi again,

Am 01.10.24 um 14:23 schrieb Werner Sembach:
> (sorry resend because thunderbird made it a html mail)
>
> Hi,
>
> Am 30.09.24 um 19:06 schrieb Benjamin Tissoires:
>> On Sep 30 2024, Werner Sembach wrote:
>>> [...]
>>> Thinking about it, maybe it's not to bad that it only changes once udev is
>>> ready, like this udev could decide if leds should be used or if it should
>>> directly be passed to OpenRGB for example, giving at least some consistency
>>> only changing once: i.e. firmware -> OpenRGB setting and not firmware->leds
>>> setting->OpenRGB setting.
>> That would work if OpenRGB gets to ship the LampArray bpf object (not
>> saying that it should). Because if OpenRGB is not installed, you'll get
>> a led class device, and if/when OpenRGB is installed, full LampArray
>> would be presented.
>
> The idea in my head is still that there is some kind of sysfs switch to 
> enable/disable leds.
>
> My idea is then that a udev rule shipped with OpenRGB sets this switch to 
> disable before loading the BPF driver so leds never get initialized for the 
> final LampArray device.
>
>> But anyway, BPF allows to dynamically change the behaviour of the
>> device, so that's IMO one bonus point of it.
>>
>>>> FWIW, the use of BPF only allows you to not corner yourself. If you
>>>> failed at your LampArray implementation, you'll have to deal with it
>>>> forever-ish. So it's perfectly sensible to use BPF as an intermediate step
>>>> where you develop both userspace and kernel space and then convert back
>>>> the BPF into a proper HID driver.
>>> I don't really see this point: The LampArray API is defined by the HID Usage
>>> Table and the report descriptor, so there is not API to mess up and
>>> everything else has to be parsed dynamically by userspace anyway, so it can
>>> easily be changed and userspace just adopts automatically.
>>>
>>> And for this case the proper HID driver is already ready.
>> Yeah, except we don't have the fallback LED class. If you are confident
>> enough with your implementation, then maybe yes we can include it as a
>> driver from day one, but that looks like looking for troubles from my
>> point of view.
>
> To be on the safe side that we don't talk about different things: My current 
> plan is that the leds subsystem builds on top of the LampArray implementation.
>
> Like this the leds part has to be only implemented once for all LampArray 
> devices be it emulated via a driver or native via firmware in the device itself.
>
> And I feel confident that the UAPI should be that the userspace gets a hidraw 
> device with a LampArray HID descriptor, and every thing else is, by the HID 
> spec, dynamic anyway so I can still change my mind in implementation specifics 
> there, can't I?
>
>> After a second look at the LampArray code here... Aren't you forgetting
>> the to/from CPU conversions in case you are on a little endian system?
> Since this driver is for built in keyboards of x86 notebooks it isn't required 
> or is it?
>>> So the only point for me currently is: Is it ok to have key position/usage
>>> description tables in the kernel driver or not?
>> good question :)
>>
>> I would say, probably not in the WMI driver itself. I would rather have
>> a hid-tuxedo.c HID driver that does that. But even there, we already had
>> Linus complaining once regarding the report descriptors we sometimes
>> insert in drivers, which are looking like opaque blobs. So it might not be
>> the best either.
> Isn't tuxedo_nb04_wmi_ab_virtual_lamp_array.c not something like hid-tuxedo.c? 
> or should it be a separate file with just the arrays?
>> Sorry I don't have a clear yes/no answer.
>
> Hm... Well if it's no problem I would keep the current implementation with 
> minor adjustments because, like i described above, I don't see a benefit now 
> that this already works to rewrite it in BPF again.
>
> If it is a problem then i don't see another way then to rewrite it in BPF.
>
> Note: For future devices there might be more keyboard layouts added, basically 
> every time the chassis form factor changes.
>
>> Cheers,
>> Benjamin
> To sum up the architechture (not mutally exclusive technically)
>
> /- leds
> WMI <- WMI to LampArray Kernel driver <-switch-|
>                                                \- OpenRGB
>
> /- leds
> WMI <- WMI to Custom HID Kernel driver <- Custom HID to LampArray BPF 
> driver<-switch-|
> \- OpenRGB

ups my ascii art formatting got botched, the switch decides between "leds" and 
"OpenRGB" was what I wanted to visualize

Regards,

Werner

>
> With the "switch" and "leds" implemented in hid core, automatically 
> initialized every time a LampArray device pops up (regardless if it is from 
> native firmware, a bpf driver or a kernel driver)
>
> Writing this down I think it was never decided how the switch should look like:
>
> It should not be a sysfs attribute of the leds device as the leds device 
> should disappear when the switch is set away from it, but should it be a sysfs 
> variable of the hid device? This would mean that hid core needs to add that 
> switch variable to every hid device having a LampArray section in the descriptor.
>
>>>> Being able to develop a kernel driver without having to reboot and
>>>> being sure you won't crash your kernel is a game changer ;)
>>>>
>>>> Cheers,
>>>> Benjamin
>
> Best regards and sorry for the many questions,
>
> Werner Sembach
>
> PS: on a side node: How does hid core handle HID devices with a broken HID 
> implementation fixed by bpf, if bpf is loaded after hid-core? Does the hid 
> device get reinitialized by hid core once the bpf driver got loaded? If yes, 
> is there a way to avoid side effects by this double initialization or is there 
> a way to avoid this double initialization, like marking the device id as 
> broken so that hid core- does not initialize it unless it's fixed by bpf?
>