Message-ID: <c4e0beb6-acd1-45fa-ad47-f5cf9df89b11@gmx.de>
Date: Tue, 1 Oct 2024 18:45:07 +0200
From: Armin Wolf <W_Armin@....de>
To: Benjamin Tissoires <bentiss@...nel.org>,
 Werner Sembach <wse@...edocomputers.com>
Cc: Pavel Machek <pavel@....cz>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 dri-devel@...ts.freedesktop.org, jelle@...aa.nl, jikos@...nel.org,
 lee@...nel.org, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-leds@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
 ojeda@...nel.org, onitake@...il.com, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO
 NB04 devices

Am 01.10.24 um 15:41 schrieb Benjamin Tissoires:

> On Oct 01 2024, Werner Sembach wrote:
>> (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.
> FWIW, I'm never a big fan of sysfs. They become UAPI and we are screwed
> without possibility to change them...

Why not having a simple led driver for HID LampArray devices which exposes the
whole LampArray as a single LED?

If userspace wants to have direct control over the underlying LampArray device,
it just needs to unbind the default driver (maybe udev can be useful here?).

>> 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.
> FWIW, udev-hid-bpf can inject a udev property into a HID-BPF. So
> basically we can have a udev property set (or not) by openrgb which
> makes the BPF program decide whether to present the keyboard as
> LampArray or not.

I do not think that using HID-BPF makes sense here, since the underlying HID device
is already purely virtual.

Using HID-BPF on top of the already virtual HID device would be a bit strange.

>>> 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.
> I would say that the HID subsystem knows how to translate LampArray into
> a subset of LEDs. But I think that's what you are saying.
>
>> 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.
> yep, that's the plan. However, not sure how to fit LampArray into LED.

Maybe the LED driver can present the whole LampArry as a single RGB LED. This
might be enough for basic LED control (on/off, changing color, ...).

For advanced LED control (effects, large number of LEDs, ...) userspace can just
unbind the default LED driver and use hidraw to drive the LampArray themself.

>> 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?
> Yeah... I think?
>
>  From my point of view we are just bikeshedding on to where put that
> "firmware" extension, in WMI, in HID (kernel with a subdriver), or
> externally in BPF.
>
Just a insane idea: can Tuxedo change the ACPI code supplied by the BIOS?

>>> 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?
> Good point. Let's just hope you don't start shipping a LE laptop with
> the same keyboard hardware :)

I believe we should do those CPU conversions regardless, so future driver developers
have good examples to copy from.

>>>> 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?
> It is, in a way. I think it's more a question for Hans and the other
> platform maintainers, whether they would accept this.

Is there a possibility to query the physical keyboard layout using the WMI interface?

>>> 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.
> If the WMI part doesn't change, then maybe having BPF would be easier
> for you in the future. Adding a HID-BPF file would cost basically
> nothing, and it'll be out of band with the kernel, meaning you can ship
> it in already running kernels (assuming the same WMI driver doesn't need
> any updates).
>
>>> 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
>>
>> 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.
> Again, not a big fan of the sysfs, because it's UAPI and need root to
> trigger it (though the udev rule sort this one out).
> BPF allows already to re-enumerate the device with a different look and
> feel, so it seems more appropriate to me.
>
> Also, having a sysfs that depends on the report descriptor basically
> means that we will lose it whenever we re-enumerate it (kind of what the
> LED problem you mentioned above). So we would need to have a sysfs on
> *every* HID devices???
>
> Actually, what would work is (ignoring the BPF bikeshedding for Tuxedos
> HW):
> - a device presents a report descriptor with LampArray (wherever it
>    comes from)
> - hid-led.c takes over it (assuming we extend it for LampArray), and
>    creates a few LEDs based on the Input usage (one global rgb color for
>    regular keys, another one for the few other LEDs known to userspace)
> - when openRGB is present (special udev property), a BPF program is
>    inserted that only contains a report descriptor fixup that prevent the
>    use of hid-led.c

Can we just manually unbind the hid-led driver?

> - the device gets re-enumerated, cleaning the in-kernel leds, and only
>    present the LampArray through hidraw, waiting for OpenRGB to take
>    over.
> - at any time we can remove the BPF and restore the LEDs functionality
>    of hid-led.c
>
>>>>> 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?
> - broken HID device:
>    it depends on what you call "broken" HID device. If the report
>    descriptor is boggus, hid-core will reject the device and will not
>    present it to user space (by returning -EINVAL).
>    If the device is sensible in terms of HID protocol, it will present it
>    to userspace, but the fact that it creates an input node or LED or
>    whatever just depends on what is inside the report descriptor.
>
> - HID-BPF:
>    HID-BPF is inserted between the device itself and the rest of the
>    in-kernel HID stack.
>    Whenever you load and attach (or detach) a BPF program which has a
>    report descriptor fixup, HID-core will reconnect the device,
>    re-enumerate it (calling ->probe()), and will re-present it to
>    userspace as if it were a new device (you get all uevents).
>
> - double initialization:
>    nowadays hid-generic doesn't do anything on the device itself except
>    calling the powerup/powerdown, by calling ->start and ->stop on the
>    HID transport driver. It's not a problem on 99% of the devices AFAICT.
>    technically, if the report descriptor is bogus, start/stop won't be
>    called, but you'll get an error in the dmesg. So if you really want to
>    rely on that "broken" scenario we can always add a specific quirk in
>    HID to not spew that error.
>
> Cheers,
> Benjamin
>
> PPS: sorry for pushing that hard on HID-BPF, but I can see that it fits
> all of the requirements here:
> - need to be dynamic
> - still unsure of the userspace implementation, meaning that userspace
>    might do something wrong, which might require kernel changes
> - possibility to extend later the kernel API
> - lots of fun :)

Powered by blists - more mailing lists