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

[Werner Sembach]

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?