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

Hi,

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

--HPS

In D6998#146685, @hselasky wrote:

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,

In D6998#146714, @wulf_cicgroup.ru wrote:

In D6998#146685, @hselasky wrote:

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

In D6998#146721, @hselasky wrote:

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

In D6998#146718, @hselasky wrote:

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

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.

In D6998#146760, @hselasky wrote:

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

In D6998#146797, @emaste wrote:

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?

In D6998#146955, @hselasky wrote:

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?

In D6998#147241, @wulf_cicgroup.ru wrote:

In D6998#146955, @hselasky wrote:

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.

In D6998#147328, @hselasky wrote:

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

In D6998#146760, @hselasky wrote:

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.

In D6998#147567, @hselasky wrote:

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

In D6998#147849, @hselasky wrote:

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

In D6998#147849, @hselasky wrote:

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

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 ?

In D6998#154470, @adrian wrote:

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

@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

In D6998#154695, @hselasky wrote:

@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

In D6998#154695, @hselasky wrote:

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

In D6998#155472, @hselasky wrote:

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)

In D6998#155472, @hselasky wrote:

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

In D6998#155480, @wulf_cicgroup.ru wrote:

In D6998#155472, @hselasky wrote:

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

In D6998#155478, @wulf_cicgroup.ru wrote:

In D6998#155472, @hselasky wrote:

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

In D6998#155649, @hselasky wrote:

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

In D6998#155829, @hselasky wrote:

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

In D6998#159709, @hselasky wrote:

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

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

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#162251, @dumbbell wrote:

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

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

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.

In D6998#163131, @emaste wrote:

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.

ukbd(4): D7957