On Mon, Dec 03, 2018 at 11:26:46AM +0100, Linus Walleij wrote: > n Thu, Nov 29, 2018 at 10:08 AM Benjamin Tissoires > <benjamin.tissoires@xxxxxxxxxx> wrote: > > On Wed, Nov 28, 2018 at 11:27 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > > I must say, I am *very* reluctant at adding an other gaming specific > > driver in the kernel. We have a nice external project named libratbag > > that has the same goal, which is much easier to update and use from > > the users. > > This project announces itself as targeted for gaming mice. > I guess it is also for keyboard? Razer keyboards have mice > interfaces, but these interfaces are mainly used for control > messages, not actual mouse events. gaming mice was the original target, yes. we thought about keyboards but never got to it, mostly because the main question of "what do we actually need to do here?" was never narrowed down. And no-one picked it up. > > Also, adding yet an other gaming device in the kernel means > > you'll have to support yet an other kernel ABI, and this is just wrong > > because you can't know what the future will be. > > Well the ABI used is mainly the LED ABI, with 1 (one) additional > sysfs file for controlling the enumerated LED effects. > It's not especially sophisticated. > > If you insist, I can drop that sysfs file and then we only use > the existing LED ABI and I do not add anything. > > As far as I understand one does not exclude the other here, > if I add the majority of this driver for key translation and > LEDs that do not add any userspace ABIs at all, then > hidraw can still be used for the remaining portions of the > device, right? > > > Anyway, you'll need a > > userspace tool (or at least a shell script) to enable the various > > settings, so using hidraw directly is usually the best choice. > > So hidraw is the kernel ABI that libratbag is using IIUC? for the devices we have currently in the tree - yes. at some point I had an openrazer backend that would talk to the openrazer daemon via DBus but it didn't get merge because of technical issues in that daemon. the architecture doesn't care about the actual protocol though. > I am not a fan of this design. It is essentially a microkernel > architecture where device drivers are relayed to userspace > daemons. And it's not just my personal opinion or gut feeling, > it is my experience after spending years in the past writing and > maintaining libmtp which is another microkernelish userspace > that handle MTP devices (like Android phones). > https://elinux.org/images/8/85/Media_Transfer_Protocol.pdf > > I might be grumpy. But the sum total of my experience is > that sooner or later you *will* run into a situation where > the kernel is way better suited to handle the device and > you are essentially writing substandard device drivers in > userspace. I have absolutely no hard proof for this. > > The second concern is community-related and concerns the > fact that such projects are usually dependent on one person > and when this person lose interest, development stalls > and updates stop coming out. But I take it RedHat et al > are officially backing libratbag so it is a minor concern. we find some time to work on libratbag, but it's few and far in between and it almost always takes a backseat to other issues. > The third concern is that there will be distributions that > do not package this ratbag library in their default install > and that is going to be pretty annoying to users. *shrug* there will always be packages that aren't part of the default install. something that affects a tiny number of users is less likely to be part of the default set. > So in total, all such external userspace comes with a > maintenance price elsewhere and the price of a kernel > module is IMO way lower. Especially for these keyboards. IMO the maintenance argument doesn't hold up that well. Sure, you have more developers in the kernel but you have no guarantee any of this works after being modified by someone who doesn't have access to all devices. Our biggest issue is having to reverse-engineer the devices (sometimes with a bit of legal limbo depending which jurisdiction you're in). Half the time we don't know what a byte means and it may be that device Foo has a different byte than Bar even though they're of the same generation or whatever. I don't see how this will magically get better by moving things into the kernel where it's harder to test with CI. And potentially duplicating everything. IMO all you're guaranteeing in the kernel is that it keeps on building. Beyond that it's just guesswork. (we have some grand plans for better firmware-like testing but they're just that at the moment, plans) > But maybe the best is a compromise, a reasonable kernel > driver and all extra icing relayed to userspace? > > > That being said, if the device does some fancy features like switching > > to a proprietary protocol, then yes, having a kernel driver is the > > only option. > > This is the case ... the kernel driver switches the keyboard > around into a "driver" mode, which is what the OS is expected > to do, the keyboard comes up in some compatibility mode > which is for interacting with BIOS and boot loader, but it is > not supposed to be used during main operation. > > > But as far as setting the LEDs effects, I would > > definitively prefer having this done in userspace. > > It could be a both/and rather than an either/or approach > I think. > > > > The extra functions have been supported for years by an > > > out-of-tree project, OpenRazer. However out-of-tree code > > > annoys me because special keys and features don't "just > > > work" out of the box with the mainline kernel. > > > > IIRC, I opened a yet-to-be-finished PR on this very same project to > > get rid off the dkms and only rely on userspace bits: > > https://github.com/openrazer/openrazer/pull/382 > > This made the whole openRazer project much easier to install from a > > user and to upgrade for developers. > > So this looks stalled to me. It's been sitting there for > over a year. > > It surely illustrates that the OpenRazer project is in maintenance > mode not development mode. > > Also illustrates the point that odd userspace projects behave > like this when initial development has slowed down: sporadic > maintenance and lack of interest and thus manpower except > for the odd creative burst. > > This just motivates me to work even more on this kernel > module TBH. But please share your view of it. > > > Sorry, but you'll have to convince me to review this patch. A 2700 > > lines of code patch is not something to review lightly > > This patch is long but it is because: > > 1. It contains a length device database in C99 notation i.e. > > { > .foo = bar, > .baz = fnord, > }, > > 2. It uses the new kerneldoc documentation style: > > struct foo { > /** > * @bar: This is a member > */ > bool bar; > (...) > > These two things add to the linecount considerably. The first can be > amended by switching to either a separate file+patch for the device > database or using macros (which is subject to maintainer preference). > > The second can be amended by using the old compact kerneldoc > on top of the struct. This more verbose style is preferred by eg > DRM and I thought it's neat but if you don't like it I can ditch it. > > > and I am > > already convinced we should strip off most of it to only handle what > > is not reasonable to ask user space to do (key translation and other > > basic functionalities). > > Does this mean that users who don't have the ratbag library > (daemon?) and the right udev scripts and whatnot in place are > at loss? > > I guess that is OK if the library+daemon has widespread > acceptance and is part of Fedora and Ubuntu default installs. > > Is it? it's not. and fwiw, we merged ratbagd and the library over a year ago but kept the name. so now you just have the dbus daemon because there was no good use-case for having this as separate library. The packaging situation is messy. Fedora ships the most recent release, Arch updates regularly from git iirc. Debian/Ubuntu haven't updated for 18 months so they still have a separate ratbagd/libratbag package and none of the things we fixed since. I'm not even sure they ship a working version of Piper (or at all, launchpad.net is unclear). We're directing users to PPAs. We've changed away from udev rules to .device files since too, the udev rules (well, the hwdb) didn't scale. That came after the version ubuntu ships though. That's possibly the biggest advantage you'd get from a kernel driver - you implicitly force Ubuntu to update the package... Cheers, Peter
