<!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> Hi, <div class="moz-cite-prefix">Am 30.09.24 um 19:06 schrieb Benjamin Tissoires: </div> <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap="">On Sep 30 2024, Werner Sembach wrote: </pre> <blockquote type="cite">[...] <pre class="moz-quote-pre" wrap="">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. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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.</pre> </blockquote> 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. <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap=""> But anyway, BPF allows to dynamically change the behaviour of the device, so that's IMO one bonus point of it. </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> 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. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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.</pre> </blockquote> 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? <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap=""> 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?</pre> </blockquote> Since this driver is for built in keyboards of x86 notebooks it isn't required or is it? <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> So the only point for me currently is: Is it ok to have key position/usage description tables in the kernel driver or not? </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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.</pre> </blockquote> 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? <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap=""> Sorry I don't have a clear yes/no answer.</pre> </blockquote> 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. <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap=""> Cheers, Benjamin</pre> </blockquote> 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 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. <blockquote type="cite" cite="mid:cflor5mz4flekn44ttlbanfigmwn5mmp3p54gkeeznzmzkyjqz@p2c6q7gulrdl"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> 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 </pre> </blockquote> </blockquote> </blockquote> 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? </body> </html>