Page MenuHomeFreeBSD

input event interface implementation
ClosedPublic

Authored by wulf_cicgroup.ru on Jun 27 2016, 9:17 PM.

Details

Summary

Adds input event interface AKA evdev found on most modern Linuxes.
Mostly feature-complete, missing parts are:

  • power and force-feedback events support
  • "euclidean bipartite matching" problem solver
  • man pages

This patch is kernel API only, evdev support for some individual drivers can be found here: https://github.com/wulf7/freebsd/tree/evdev
Heavily based on GSOC2014 project by Jakub Klama

Test Plan

Passes all meaningfull libevdev unit tests (3 still fails)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bapt added a reviewer: trasz.Jun 27 2016, 10:37 PM
bapt added a subscriber: bapt.Jun 27 2016, 10:39 PM

sorry right now I do not have any time to spend on the review, but added people I know interested into that area and eager to get that in

hselasky edited edge metadata.Jun 28 2016, 7:10 AM

Hi,

Did you test that your implementation does not interfere with /usr/ports/multimedia/webcamd and the V4L linux compat files?

--HPS

Hi,
Did you test that your implementation does not interfere with /usr/ports/multimedia/webcamd

It tries to choose next device unit number if make_dev returned EEXISTS but real coexistence has not been tested
Is webcamd able to create dummy event device? Is webcamd supports a case when /dev/input/ already populated?

and the V4L linux compat files?

Both (reviewed and v4linux) headers can not be #included at one time, but they are compatible.
Really, I use patched V4L linux compat headers for userland evdev stack parts to ease of porting not headers from this review:
http://195.170.219.74/evdev-ports-20160628.diff

Hi,

Hi,
Did you test that your implementation does not interfere with /usr/ports/multimedia/webcamd

It tries to choose next device unit number if make_dev returned EEXISTS but real coexistence has not been tested
Is webcamd able to create dummy event device? Is webcamd supports a case when /dev/input/ already populated?

Yes, webcamd currently uses /dev/input/xxx so we need to find a solution there. Webcamd does not jump to the next device name when a EEXISTS happens. So it will need a patch - at least to do that. Can you test that?

and the V4L linux compat files?

Both (reviewed and v4linux) headers can not be #included at one time, but they are compatible.
Really, I use patched V4L linux compat headers for userland evdev stack parts to ease of porting not headers from this review:
http://195.170.219.74/evdev-ports-20160628.diff

Are you aware about the following PR and attached patches:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=196678

--HPS

hselasky requested changes to this revision.Jun 28 2016, 8:42 AM
hselasky edited edge metadata.

Hi,

Can you propose a solution how webcamd and kernel's evdev can co-exist in /dev/input? For example

Use different node names? eventXX for userspace bsdeventYY for the kernel?

This is a blocking issue.

The cuse module already has a unit management, so that all event nodes created from userspace are unique.

Also what to you think about renaming the event header files so that they do not interfere with v4l-compat's port? Or can we use both at the same time?

--HPS

This revision now requires changes to proceed.Jun 28 2016, 8:42 AM
wulf_cicgroup.ru added a comment.EditedJun 28 2016, 10:59 AM

Can you propose a solution how webcamd and kernel's evdev can co-exist in /dev/input? For example
Use different node names? eventXX for userspace bsdeventYY for the kernel?

Best solution is to use uinput interface instead of cuse

Are you aware about the following PR and attached patches:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=196678

Yes. I am aware of it. I borrowed some code from it for udev-devd wrapper needed by libinput.
I will try it in libinput-less setup and then post results here or in PR itself

Can you propose a patch for webcamd to use the kernel's /dev/input/eventX layer?

Many people are using it on their desktops, so this must work smoothly.

--HPS

emaste edited edge metadata.Jun 28 2016, 2:26 PM

Next time please upload with full context -- see https://wiki.freebsd.org/CodeReview. For git, it's just git diff -U9999, for svn svn diff --diff-cmd=diff -x -U999999. No need to re-upload now just to include the context but please do so if/when you have other changes to upload in this review.

Can you propose a patch for webcamd to use the kernel's /dev/input/eventX layer?
Many people are using it on their desktops, so this must work smoothly.

Could you try following patch? http://195.170.219.74/webcamd-20160628.patch
It is compile tested only due to lack of webcam-supported hardware so it can do something other than intended. It should create both cuse-cdev as /dev/input/webcamdXX and its uinput alter-ego as /dev/input/eventXX

Hi,

Thank you for your patch. I'll see if I can give it a spin. Does this also handle when a device registers multiple input event devices or only one? For example wacom tablets register 3 separate input devices.

You can attach webcamd to any USB mouse/keyboard if you start it by hand. See:

webcamd -l

--HPS

Next time please upload with full context -- see https://wiki.freebsd.org/CodeReview. For git, it's just git diff -U9999, for svn svn diff --diff-cmd=diff -x -U999999. No need to re-upload now just to include the context but please do so if/when you have other changes to upload in this review.

Thanx for clarification, Ed! Could this link be placed on the start page somewhere before or after bugzilla link?

Thank you for your patch. I'll see if I can give it a spin.

Thank you too. I tried it and it found it working (after some fixes).

Does this also handle when a device registers multiple input event devices or only one? For example wacom tablets register 3 separate input devices.

I hope, yes, as it fires at every input event device registration.

You can attach webcamd to any USB mouse/keyboard if you start it by hand. See:
webcamd -l

Unfortunately I was not able to attach any of my USB keyboards/mices to webcamd. Should a special compile or runtime options be used?
Finally, I found supported USB touch device and successfully ran it using uinput interface with slightly modified version of proposed patch.

Next step I faced with another problem: I have to send some events like LEDs in opposite direction (from user(uinput) to hardware(webcamd)) and simplest way to achieve that is to open() cuse device and than write() to it from dedicated broker thread. And than the question is: Is it possible to know which character device (path or opened filehandle) corresponds to current webcamd process/thread?

Thank you for your patch. I'll see if I can give it a spin.

Thank you too. I tried it and it found it working (after some fixes).

Does this also handle when a device registers multiple input event devices or only one? For example wacom tablets register 3 separate input devices.

I hope, yes, as it fires at every input event device registration.

You can attach webcamd to any USB mouse/keyboard if you start it by hand. See:
webcamd -l

Unfortunately I was not able to attach any of my USB keyboards/mices to webcamd. Should a special compile or runtime options be used?

No, it should work out of the box. What does it print when it doesn't work? You may need to run webcamd like super-user. Or perhaps CONFIG_XXX is not enabled in webcamd towards the Linux code for mouse/keyboard ....

Finally, I found supported USB touch device and successfully ran it using uinput interface with slightly modified version of proposed patch.
Next step I faced with another problem: I have to send some events like LEDs in opposite direction (from user(uinput) to hardware(webcamd)) and simplest way to achieve that is to open() cuse device and than write() to it from dedicated broker thread. And than the question is: Is it possible to know which character device (path or opened filehandle) corresponds to current webcamd process/thread?

Yes, I think this information is available by "fstat -p xxx". I'm a bit worried that your approach will cause a devfs deadlock when webcamd is exiting, when calling forth and back. The reason is that the destroy dev function will sleep until all references are gone. So all file handles opened must be closed before webcamd can detach from the USB device.

No, it should work out of the box. What does it print when it doesn't work? You may need to run webcamd like super-user. Or perhaps CONFIG_XXX is not enabled in webcamd towards the Linux code for mouse/keyboard ....

I sent output in private mail to not spam the thread

wulf_cicgroup.ru edited edge metadata.
wulf_cicgroup.ru removed rS FreeBSD src repository as the repository for this revision.

Implements passing events from user to userspace drivers like webcamd via uinput interface

wulf_cicgroup.ru set the repository for this revision to rS FreeBSD src repository.

Can you propose a patch for webcamd to use the kernel's /dev/input/eventX layer?

Done

I split it on two parts
First, http://195.170.219.74/webcamd-uinput.patch adds extra command line option -u for using uinput interface instead of cuse for evdev devices
Second, http://195.170.219.74/webcamd-devcoexist.patch tries to add coexistence logic to webcamd unit allocation. It reveals cuse bug also, character devices created on prepopulated /dev/input/ directory are not garbage collected after webcamd has exited

cuse bug: Can you see if any devices have opened /dev/input handles to evdevs using fstat? There should be no bug there.

cuse bug: Can you see if any devices have opened /dev/input handles to evdevs using fstat? There should be no bug there.

Yes! You are right! That is not cuse bug, sorry! Xorg was referencing this device nodes, so most probably, the problem is an absence of "Device is gone" notifications from webcamd process to Xorg device driver at least in my setup.
But that seems to be a separate problem not related to D6998

When your /dev/uinput device closes, does it wakeup the children devices and make them return an error? That might be the problem. Or is the xorg-event driver not checking read() / poll() for errors. Detach needs to work. Can you investigate?

--HPS

When your /dev/uinput device closes, does it wakeup the children devices and make them return an error? That might be the problem. Or is the xorg-event driver not checking read() / poll() for errors. Detach needs to work. Can you investigate?

I found the reason. Cuse returns EINVAL on read() if device is gone but xf86-input-evdev expects ENODEV in that case. Ports version of xf86-input-evdev has patch to handle this but I removed it for testing purposes.
Reverting of my change fixed this issue. Sorry for noise

When your /dev/uinput device closes, does it wakeup the children devices and make them return an error?

Yes it does wakeup and than returns ENODEV on every file operation like Linux does
Case I false-reported happened with cuse-backed devices not with uinput ones

kmacy added a subscriber: kmacy.Jul 28 2016, 10:37 PM
adrian added a subscriber: adrian.Aug 6 2016, 3:14 AM

So far it looks good to me. Configurable debugging would be nice. I'm glad there are lots of lock assertions everywhere, thank you for that!

What's it been tested on/with ?

sys/dev/evdev/uinput.c
47 ↗(On Diff #18071)

hi,

I'll comment this one, but I think this should done everywhere.

Don't use DEBUG for this. Define something new, say EVDEV_DEBUG, and condition things on that. Stick it in opt_evdev.h and make sure everything includes that.

That way it's a simple enough compile time option people can flip on.

315 ↗(On Diff #18071)

Another comment; I'd also suggest doing a pass with the debug calls to ensure the softc is passed in; that way later on you can enable some pretty simple debugging flags and such.

What's it been tested on/with ?

I use evdev-only laptop for about a year and run following hw/sw
Event senders
hardware: atkbd, psm(both relative and absolute modes), ums, ukbd, win7/8 compatible touchscreen
inkernel virtual: vt sysmouse, kbdmux
userspace virtual via uinput: bthidd mouses and keyboards, (patched)webcamd usb keyboards and win7/8 compatible touchscreen

Event receivers
xf86-input-evdev, xf86-input-libinput, xf86-input-synaptics

Regression
libevdev unit tests

Oleksandr Tymoshenko (gonzo@) did some testing on ARM platform also

  • split evdev.h header on public and private parts
  • make debug messages configurable on config stage as Adrian suggested
  • uinput locking reworked (userspace calls are fully serialized now)
  • some small fixes like EV_EOF/POLLHUP on revoked/detached device

Ooops. Forgot to attach evdev_private.h

adrian accepted this revision.Aug 8 2016, 2:33 AM
adrian added a reviewer: adrian.

Sounds good then! What's left before it can be committed?

@adrian

I need to patch webcamd and figure out how I can dynamically detect if this EVDEV driver is loaded or not.

Committing this AS-IS will break /usr/ports/multimedia/webcamd which many people use.

--HPS

wulf_cicgroup.ru marked 2 inline comments as done.Aug 8 2016, 10:54 AM

@adrian
I need to patch webcamd and figure out how I can dynamically detect if this EVDEV driver is loaded or not.
Committing this AS-IS will break /usr/ports/multimedia/webcamd which many people use.

I use following patches to allow webcamd/evdev interoperability in both cuse and uinput ways:
http://195.170.219.74/webcamd-devcoevixt.20160808.patch http://195.170.219.74/webcamd-iowint.20160808.patch http://195.170.219.74/webcamd-uinput.20160808.patch

I need figure out how I can dynamically detect if this EVDEV driver is loaded or not.

You can use feature_present("evdev") to check inkernel evdev presence

or stat("/dev/uinput", statbuf) to check uinput presence

to find if particular driver has inkernel evdev support its necessary to do ioctl(EVIOCGPHYS) on every /dev/input/eventXXX file

Thank you!

I will try to integrate the webcamd patches by end of next week and make a new version of webcamd. Hopefully no show stoppers will pop up.

--HPS

Hi,

What is the motivation behind using IOWINT() for FreeBSD IOCTLs and IOW(xxx, int) for Linux ones. Does this mean we are not binary compatible with Linux? Can this be changed so we avoid patching webcamd?

--HPS

What is the motivation behind using IOWINT() for FreeBSD IOCTLs and IOW(xxx, int) for Linux ones. Does this mean we are not binary compatible with Linux? Can this be changed so we avoid patching webcamd?

We are not binary compatible with Linux here as we do automatic copyin()/copyout() on each ioctl but they don`t. As a consequence they can provide arbitrary int data as IOW parameter via casting int to void* but we can`t as we would trigger EFAULT on copyin() of random memory location. We can use any ioctl with VOID direction like IO or IOWINT but not IOW. I prefer IOWINT

Following is what libevdev does for grabbing and ungrabbing event device:

$ grep EVIOCGRAB libevdev.c
libevdev.c:		rc = ioctl(dev->fd, EVIOCGRAB, (void *)1);
libevdev.c:		rc = ioctl(dev->fd, EVIOCGRAB, (void *)0);

On Linux EVIOCGRAB is defined in following way:
#define EVIOCGRAB _IOW('E', 0x90, int)

Can this be changed so we avoid patching webcamd?

Just now webcamd should return EFAULT on these ioctls
After switching outer headers to use IOWINT they should return EINVAL or something similar
And after both patching webcamd and switching to IOWINT they should start to work

So with avoiding of patching you would not make things worse

Can this be changed so we avoid patching webcamd?

Just now webcamd should return EFAULT on these ioctls
After switching outer headers to use IOWINT they should return EINVAL or something similar
And after both patching webcamd and switching to IOWINT they should start to work
So with avoiding of patching you would not make things worse

It should be mentioned that only meaningfull IOWINT ioctl is EVIOCGRAB and it is not used widely as you should add extra option to xorg.conf to trigger it.

Others are EVIOCRMFF and EVIOCREVOKE. I think no one tried them both on FreeBSD

kmacy added a comment.Aug 12 2016, 8:02 PM

What is the motivation behind using IOWINT() for FreeBSD IOCTLs and IOW(xxx, int) for Linux ones. Does this mean we are not binary compatible with Linux? Can this be changed so we avoid patching webcamd?

We are not binary compatible with Linux here as we do automatic copyin()/copyout() on each ioctl but they don`t. As a consequence they can provide arbitrary int data as IOW parameter via casting int to void* but we can`t as we would trigger EFAULT on copyin() of random memory location. We can use any ioctl with VOID direction like IO or IOWINT but not IOW. I prefer IOWINT
Following is what libevdev does for grabbing and ungrabbing event device:

$ grep EVIOCGRAB libevdev.c
libevdev.c:		rc = ioctl(dev->fd, EVIOCGRAB, (void *)1);
libevdev.c:		rc = ioctl(dev->fd, EVIOCGRAB, (void *)0);

On Linux EVIOCGRAB is defined in following way:
#define EVIOCGRAB _IOW('E', 0x90, int)

I don't understand this restriction. cxgb(9) and other drivers I've worked on are perfectly capable of handling nested copyins.

Would it be better to integrate this driver by way of the linuxkpi where this isn't considered a problem?

In D6998#155552, @kmacy wrote:

I don't understand this restriction. cxgb(9) and other drivers I've worked on are perfectly capable of handling nested copyins.
Would it be better to integrate this driver by way of the linuxkpi where this isn't considered a problem?

I think we are talking about other issue. Briefly:

Following is call graph of webcamd:

User program (compiled with FreeBSD input.h ioctls) -> FreeBSD ioctl syscall -> cuse -> webcamd -> Linux kernel parts inside webcamd (compiled with Linux input.h ioctls)

The problem is that we can not just borrow exact ioctl defines from Linux to FreeBSD as they cause EFAULT in FreeBSD ioctl layer. We have to adjust them to be FreeBSD compatible to pass ioctl syscall and after than convert to Linux-compatible values in webcamd layer so they can be used further in userland Linux kernel implementation

@wulf_cicgroup.ru

I understand what you mean. Could you change the Linux IOCTL header file in your webcamd patch instead of redirecting the IOCTLs in the IOCTL handler? I think then you only need to add code for the generic VOID IOCTL copy-in of the argument and you can keep the IOCTL of type VOID?

--HPS

I understand what you mean. Could you change the Linux IOCTL header file in your webcamd patch instead of redirecting the IOCTLs in the IOCTL handler? I think then you only need to add code for the generic VOID IOCTL copy-in of the argument and you can keep the IOCTL of type VOID?

Here you are: http://195.170.219.74/webcamd-iowint.20160813.patch
While here convert EVIOCGMTSLOTS to be bidirectional

Hi,

I've now updated the SVN version of webcamd with the input.h and ioctl.h patches you've provided.

Regarding using /dev/uinput I was wondering of you could add a new IOCTL API, that allows webcamd to only reserve a /dev/input/event X unit number, which is freed when the /dev/uinput file handle is closed. That would be much simpler to implement and maintain. Is there any purpose that all the webcamd events are piped through /dev/uinput ?

--HPS

hselasky added inline comments.Aug 15 2016, 11:16 AM
sys/dev/evdev/uinput.c
666 ↗(On Diff #19103)

You should check the error code of this function?

hselasky added inline comments.Aug 15 2016, 11:17 AM
sys/dev/evdev/cdev.c
685 ↗(On Diff #19103)

I guess looping in webcamd is OK, and saves having an open handle towards /dev/uinput .

wulf_cicgroup.ru marked an inline comment as done.Aug 15 2016, 1:29 PM
wulf_cicgroup.ru added inline comments.
sys/dev/evdev/uinput.c
666 ↗(On Diff #19103)

Not yet. At least until someone make uinput kld-able. AFAIK make_dev() can not fail if MAKEDEV_NOWAIT or MAKEDEV_CHECKNAME flags are not set. It panics instead of returning an error

wulf_cicgroup.ru marked an inline comment as done.EditedAug 15 2016, 6:22 PM

I've now updated the SVN version of webcamd with the input.h and ioctl.h patches you've provided.

Thank you!

Regarding using /dev/uinput I was wondering of you could add a new IOCTL API, that allows webcamd to only reserve a /dev/input/event X unit number, which is freed when the /dev/uinput file handle is closed.

Unfortunately, unit number assignment is handled by devfs not by evdev. That is why the only way to reserve evdev cdev is to create it.
I see other way: We should move unit number pooling code from cuse to devfs so evdev and cuse could allocate unit numbers from shared pool and after than assigning unique unit number would be as easy as:
in device_attach():

char *pool = "input/event%d";
err = devfs_alloc_unit(pool, &softc->unit);
if (err)
  return (err);
make_dev(..., &softc->cdev, pool, softc->unit);

in device_detach:

destroy_dev(softc->cdev);
devfs_free_unit(pool, softc->unit);

After that it would be possible to reserve eventX node even with existing IOCTL API

Is there any purpose that all the webcamd events are piped through /dev/uinput ?

That is not strictly necessary for ordinary users but can be useful for evdev driver porters/developers

Evdev ports support is submitted for review here: https://reviews.freebsd.org/D7588

Hi,

If kernel drivers are supposed to supply events, then it is a problem to use an SX lock, because many drivers hold a mutex when they generate events. And locking SX after MTX is an issue. If this layer is only intended for loopback of events via user-space it is fine.

--HPS

If kernel drivers are supposed to supply events, then it is a problem to use an SX lock, because many drivers hold a mutex when they generate events. And locking SX after MTX is an issue. If this layer is only intended for loopback of events via user-space it is fine.

Hi,

uinput is intended for loopback of events via user-space only. It should not be used from kernel drivers and have no exposed API/KPI except one struct cdevsw provides

hselasky accepted this revision.Aug 29 2016, 10:33 AM
hselasky edited edge metadata.
This revision is now accepted and ready to land.Aug 29 2016, 10:33 AM
gonzo edited edge metadata.Sep 7 2016, 8:52 PM

I guess it's time to commit this change. Jean-Sébastien would you commit it as a maintainer of all things input? Or if you don't have spare cycles I can do it myself.

This revision fails to compile with INVARIANTS disabled. I fixed that in my repo and I`m going to update diff in this review in hour or two with couple others minor fixes

https://github.com/wulf7/freebsd/commit/64fba04a38c6968a1580f6f5c30e0350898a0d3a

wulf_cicgroup.ru edited edge metadata.

Fix compiling with INVARIANTS disabled
Fix EVIOCGKEYCODE* ioctls direction
Split input.h on input.h(ioctls) and input-event-codes.h(event codes) like Linux did
Clear uinput device state after UI_DEV_DESTROY ioctl
Declare uinput as kernel module. Not kldload-able yet.
Reduce KPI(evdev.h) on API (input.h) dependence through removing struct input_absinfo from evdev_support_abs() arguments

This revision now requires review to proceed.Sep 7 2016, 9:54 PM
dumbbell edited edge metadata.Sep 7 2016, 10:02 PM
In D6998#162223, @gonzo wrote:

I guess it's time to commit this change. Jean-Sébastien would you commit it as a maintainer of all things input? Or if you don't have spare cycles I can do it myself.

I will give this a try again, I used it lightly lately. But I'm certainly not a "maintainer of all things input" :-) I barely improved Synaptics support years ago. But I have a lot of interest in this topic. After I tried it again, I will tell you if I feel like I can commit.

If someone else is ready to commit, then go ahead! It's a wonderful improvement and there is no need to wait for me.

Diff updated. It would be nice to commit https://reviews.freebsd.org/D7588 as well to allow multimedia/webcamd coexistence. 11-servers/xorg-server change can be postponed if someone thinks it too radical

In D6998#162223, @gonzo wrote:

I guess it's time to commit this change. Jean-Sébastien would you commit it as a maintainer of all things input? Or if you don't have spare cycles I can do it myself.

I barely improved Synaptics support years ago. But I have a lot of interest in this topic.

It would be nice if someone does review or commit of patch from https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=205690 as it introduces multitouch support in psm.c and is prerequisite to evdev support in that driver.

I'm about to commit this change, running final tinderbox build

I made three changes to the latest patch:

  • one instance of spaces to tab
  • sorted files alphabetically in sys/conf/files
  • locking-related change in evdev_open to make WITNESS happy
This revision was automatically updated to reflect the committed changes.

Vladimir, could you submit devices' evdev patches to differential? I suggest starting with USB devs, one device per review. I talked to Hans, he's OK with reviewing them

wulf_cicgroup.ru added a comment.EditedSep 12 2016, 11:41 PM
In D6998#163107, @gonzo wrote:

Vladimir, could you submit devices' evdev patches to differential? I suggest starting with USB devs, one device per review. I talked to Hans, he's OK with reviewing them

ums(4): D7863

Other will be placed here later after I get some spare time

A little note, you can use the review number by itself (e.g. just D7863) and Phabricator will automatically add the link, and also reference this review from the one mentioned.

A little note, you can use the review number by itself (e.g. just D7863) and Phabricator will automatically add the link, and also reference this review from the one mentioned.

Thanks for pointing that out.