[no subject]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> > 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 :)

> > > 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.

> > 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
- 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 :)



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux