How to properly extend uinput API

How to properly extend uinput API

[Manuel Reimer]

Hello,

I'm currently trying to somehow get uinput connected with ff-memless.

My first try (which is some kind of hack which prevents API changes and 
doesn't work at all) failed. Now my idea is to add some more simple (and 
probably working) API for this. All, I need in the "memless case", is 
some way to get two motor speeds sent to usermode, which doesn't require 
all this callback stuff.

There is a new API which, as far as I understand, is meant to be easier 
extendable:
https://github.com/torvalds/linux/commit/052876f8e5

What I need is some way to pass an additional boolean to tell the kernel 
module that usermode wants to use ff-memless. Device setup is done with 
an struct "uinput_setup". As far as I know it isn't easily possible to 
extend this without breaking existing stuff.

It would be possible to write some hack which passes this information 
via magic-value through ff_effects_max but I think there has to be some 
nicer way.

So how to pass one more piece of information from usermode to kernel 
module while setup without breaking existing (probably closed-source) 
stuff? Keep a definition of the old struct version and decide which one 
to use based on size?

Thank you very much in advance for any help.

Manuel

Re: How to properly extend uinput API

[Dmitry Torokhov]

Hi Manuel,

On Sat, Apr 30, 2016 at 11:36:24AM +0200, Manuel Reimer wrote:
> Hello,
> 
> I'm currently trying to somehow get uinput connected with ff-memless.

I am not sure that exposing ff-memless to userspace is a good idea.
Right now what we have is a supporting library for several kernel
modules and there were considerations to change it to work differently.

I'd rather userspace took care of handling dumb devices if t decided to
handle device communication instead of leaving it to the kernel.

Thanks.

> 
> My first try (which is some kind of hack which prevents API changes
> and doesn't work at all) failed. Now my idea is to add some more
> simple (and probably working) API for this. All, I need in the
> "memless case", is some way to get two motor speeds sent to
> usermode, which doesn't require all this callback stuff.
> 
> There is a new API which, as far as I understand, is meant to be
> easier extendable:
> https://github.com/torvalds/linux/commit/052876f8e5
> 
> What I need is some way to pass an additional boolean to tell the
> kernel module that usermode wants to use ff-memless. Device setup is
> done with an struct "uinput_setup". As far as I know it isn't easily
> possible to extend this without breaking existing stuff.
> 
> It would be possible to write some hack which passes this
> information via magic-value through ff_effects_max but I think there
> has to be some nicer way.
> 
> So how to pass one more piece of information from usermode to kernel
> module while setup without breaking existing (probably
> closed-source) stuff? Keep a definition of the old struct version
> and decide which one to use based on size?
> 
> Thank you very much in advance for any help.
> 
> Manuel

-- 
Dmitry

Re: How to properly extend uinput API

[Manuel Reimer]

On 05/02/2016 08:35 PM, Dmitry Torokhov wrote:
> I am not sure that exposing ff-memless to userspace is a good idea.

It would prevent code-duplication, as something, that already exists, 
would be usable for user mode drivers.

> Right now what we have is a supporting library for several kernel
> modules and there were considerations to change it to work differently.

But even then there would still be a need for support for input devices 
with just two rumble motors. I guess this is the most common case.

While developing my own uinput driver, I had a look at some different 
projects and all of them at least try to somehow limit the complexity of 
the existing API. Many of them doing it wrong as simulating all the 
events, ff-memless can do, on two motors isn't an easy task.

> I'd rather userspace took care of handling dumb devices if t decided to
> handle device communication instead of leaving it to the kernel.

I considered that and I don't have any clue on how to port the 
ff-memless code to work in usermode. It uses too much kernel specific 
features.


The easiest way to extend the uinput API could be to introduce a new 
ioctl call to initialize a "memless" device. This way the uinput-module 
knows it has to use ff-memless for this device.

And to get the events back to usermode, the simplest way is to just pass 
the two motor values (both 16 bits) in the value field of an input event 
(32 bits). This doesn't need all the callback stuff and probably even is 
more robust (current API tends to create non-killable processes if the 
uinput daemon exits before the last application, using the device, exits).

I already have some test code running. So far I didn't try with real 
hardware, but my current results look promising.


Does your response mean, that there is no chance to get a patch for this 
accepted? If so, my only choice would be to ship this as patch with my 
own code, requiring the user to compile his own kernel.

Manuel

Re: How to properly extend uinput API

[Dmitry Torokhov]

On Mon, May 02, 2016 at 11:57:21PM +0200, Manuel Reimer wrote:
> On 05/02/2016 08:35 PM, Dmitry Torokhov wrote:
> >I am not sure that exposing ff-memless to userspace is a good idea.
> 
> It would prevent code-duplication, as something, that already
> exists, would be usable for user mode drivers.
> 
> >Right now what we have is a supporting library for several kernel
> >modules and there were considerations to change it to work differently.
> 
> But even then there would still be a need for support for input
> devices with just two rumble motors. I guess this is the most common
> case.
> 
> While developing my own uinput driver, I had a look at some
> different projects and all of them at least try to somehow limit the
> complexity of the existing API. Many of them doing it wrong as
> simulating all the events, ff-memless can do, on two motors isn't an
> easy task.

ff-memless does not really care about number of motors, it just
schedules the driver to be called at given times and tries to combine
several effects into resulting one. How to handle combined event is left
up to the hardware driver.

> 
> >I'd rather userspace took care of handling dumb devices if t decided to
> >handle device communication instead of leaving it to the kernel.
> 
> I considered that and I don't have any clue on how to port the
> ff-memless code to work in usermode. It uses too much kernel
> specific features.

That's because userspace has different rules and the resulting userspace
code will probably be different from kernel side. In userspace you can,
for example, use floating point easily. Or you can create a separate
thread for handling force feedback tasks for the device. Or do something
else.

> 
> The easiest way to extend the uinput API could be to introduce a new
> ioctl call to initialize a "memless" device. This way the
> uinput-module knows it has to use ff-memless for this device.
> 
> And to get the events back to usermode, the simplest way is to just
> pass the two motor values (both 16 bits) in the value field of an
> input event (32 bits).

Nowhere in the memoryless code we have "2 motor values". You are
basically designing an API for single device and that is not a good
idea.

> This doesn't need all the callback stuff and
> probably even is more robust (current API tends to create
> non-killable processes if the uinput daemon exits before the last
> application, using the device, exits).

Hmm, it should not, can you post some traces to show where it gets
stuck so we can fix it?

> 
> I already have some test code running. So far I didn't try with real
> hardware, but my current results look promising.
> 
> 
> Does your response mean, that there is no chance to get a patch for
> this accepted? If so, my only choice would be to ship this as patch
> with my own code, requiring the user to compile his own kernel.

So far I am convinced that relying on kernel implementation is a bad
idea. You should be able to take cues from the kernel code and create
userspace library providing similar features, but in the way that is
flexible and make sense in userspace. Like I said, we might change how
ff-memless works in the kernel and we do not need to tie it up with
userspace ABI.

Thanks.

-- 
Dmitry

Re: How to properly extend uinput API

[Clément VUCHENER]

2016-05-03 0:51 GMT+02:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Mon, May 02, 2016 at 11:57:21PM +0200, Manuel Reimer wrote:
>> This doesn't need all the callback stuff and
>> probably even is more robust (current API tends to create
>> non-killable processes if the uinput daemon exits before the last
>> application, using the device, exits).
>
> Hmm, it should not, can you post some traces to show where it gets
> stuck so we can fix it?

I have been using ff uinput recently, the bugs were easy to reproduce.
I encountered two different bugs.

Having ff_effects_max set to 0 but with EV_FF bit set (and maybe some
FF_* bits too) crashes uinput when starting fftest (easy to fix but
annoying during development).

Destroying a uinput device when there are still uploaded effects
crashes uinput (e.g. destroying the uinput device while fftest is
running). This one is more problematic since you may need to destroy
the device at any time (unplugged device or empty battery).

Here are my logs for the second bug (on a 4.4 fedora kernel, I can try
the latest mainline if you prefer):

NMI watchdog: Watchdog detected hard LOCKUP on cpu 3
Modules linked in:
 rfcomm cmac fuse xt_CHECKSUM ccm ipt_MASQUERADE
nf_nat_masquerade_ipv4 tun nf_conntrack_netbios_ns
nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT nf_reject_ipv6
xt_conntrack ip_set nfnetlink ebtable_filter ebtable_nat
ebtable_broute bridge ebtables ip6table_raw ip6table_security
ip6table_mangle ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6
nf_nat_ipv6 ip6table_filter ip6_tables iptable_raw iptable_security
iptable_mangle iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4
nf_nat_ipv4 nf_nat nf_conntrack bnep bbswitch(OE) intel_rapl iosf_mbi
x86_pkg_temp_thermal coretemp kvm_intel kvm irqbypass arc4
crct10dif_pclmul crc32_pclmul iTCO_wdt iwldvm crc32c_intel
iTCO_vendor_support mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek
snd_hda_codec_generic iwlwifi snd_hda_intel snd_hda_codec uvcvideo
 snd_hda_core btusb videobuf2_vmalloc snd_hwdep cfg80211 btrtl
videobuf2_memops videobuf2_v4l2 btbcm snd_seq joydev btintel
videobuf2_core vfat v4l2_common fat videodev bluetooth i2c_i801
snd_seq_device media snd_pcm snd_timer rtsx_pci_ms snd memstick
lpc_ich rfkill mei_me soundcore mei shpchp tpm_infineon tpm_tis tpm
nfsd binfmt_misc auth_rpcgss nfs_acl lockd grace sunrpc uinput i915
rtsx_pci_sdmmc mmc_core mxm_wmi serio_raw i2c_algo_bit drm_kms_helper
8021q garp stp llc mrp drm r8169 rtsx_pci mii wmi video fjes
CPU: 3 PID: 11654 Comm: input-scripts Tainted: P           OE
4.4.6-300.fc23.x86_64 #1
Hardware name: CLEVO CO.                        W350ET/W350ET, BIOS
1.02.21PM v3 07/01/2013
 0000000000000086 00000000f29b3249 ffff88022f385b90 ffffffff813b542e
 0000000000000000 0000000000000000 ffff88022f385ba8 ffffffff8115ac1d
 ffff8802236ac800 ffff88022f385be0 ffffffff811a308c 0000000000000001
Call Trace:
 <NMI>  [<ffffffff813b542e>] dump_stack+0x63/0x85
 [<ffffffff8115ac1d>] watchdog_overflow_callback+0xbd/0xd0
 [<ffffffff811a308c>] __perf_event_overflow+0x8c/0x1d0
 [<ffffffff811a3c64>] perf_event_overflow+0x14/0x20
 [<ffffffff81035fb1>] intel_pmu_handle_irq+0x1e1/0x460
 [<ffffffff8102c738>] perf_event_nmi_handler+0x28/0x50
 [<ffffffff8101b0a9>] nmi_handle+0x69/0x120
 [<ffffffff8101b61d>] default_do_nmi+0xad/0xf0
 [<ffffffff8101b742>] do_nmi+0xe2/0x130
 [<ffffffff817a2a31>] end_repeat_nmi+0x1a/0x1e
 [<ffffffff810ed07a>] ? queued_spin_lock_slowpath+0x10a/0x170
 [<ffffffff810ed07a>] ? queued_spin_lock_slowpath+0x10a/0x170
 [<ffffffff810ed07a>] ? queued_spin_lock_slowpath+0x10a/0x170
 <<EOE>>  [<ffffffff817a0307>] _raw_spin_lock_irqsave+0x37/0x40
 [<ffffffff810e633d>] complete+0x1d/0x50
 [<ffffffffa00ba08c>] uinput_request_done+0x3c/0x40 [uinput]
 [<ffffffffa00ba587>] uinput_request_submit.part.7+0x47/0xb0 [uinput]
 [<ffffffffa00bb62b>] uinput_dev_erase_effect+0x5b/0x76 [uinput]
 [<ffffffff815d91ad>] erase_effect+0xad/0xf0
 [<ffffffff815d929d>] flush_effects+0x4d/0x90
 [<ffffffff815d4cc0>] input_flush_device+0x40/0x60
 [<ffffffff815daf1c>] evdev_cleanup+0xac/0xc0
 [<ffffffff815daf5b>] evdev_disconnect+0x2b/0x60
 [<ffffffff815d74ac>] __input_unregister_device+0xac/0x150
 [<ffffffff815d75f7>] input_unregister_device+0x47/0x70
 [<ffffffffa00bac45>] uinput_destroy_device+0xb5/0xc0 [uinput]
 [<ffffffffa00bb2de>] uinput_ioctl_handler.isra.9+0x65e/0x740 [uinput]
 [<ffffffff811231ab>] ? do_futex+0x12b/0xad0
 [<ffffffffa00bb3f8>] uinput_ioctl+0x18/0x20 [uinput]
 [<ffffffff81241248>] do_vfs_ioctl+0x298/0x480
 [<ffffffff81337553>] ? security_file_ioctl+0x43/0x60
 [<ffffffff812414a9>] SyS_ioctl+0x79/0x90
 [<ffffffff817a04ee>] entry_SYSCALL_64_fastpath+0x12/0x71
INFO: rcu_sched detected stalls on CPUs/tasks:
        3-...: (1 GPs behind) idle=7cb/140000000000000/0
softirq=236332/236332 fqs=15113
        (detected by 1, t=60003 jiffies, g=264264, c=264263, q=0)
Task dump for CPU 3:
input-scripts   R  running task        0 11654  11278 0x0000000c
 ffff880122a57b50 ffffffff810ec510 ffff880122a58000 ffff880224417428
 ffff8800aa198000 0000000000000000 ffff880224417408 ffff880224417400
 0000000124417408 0000000000000000 ffff880224417660 0000000000000286
Call Trace:
 [<ffffffff810ec510>] ? mutex_optimistic_spin+0x170/0x1a0
 [<ffffffff817a0307>] ? _raw_spin_lock_irqsave+0x37/0x40
 [<ffffffff810e633d>] ? complete+0x1d/0x50
 [<ffffffffa00ba08c>] ? uinput_request_done+0x3c/0x40 [uinput]
 [<ffffffffa00ba587>] ? uinput_request_submit.part.7+0x47/0xb0 [uinput]
 [<ffffffffa00bb62b>] ? uinput_dev_erase_effect+0x5b/0x76 [uinput]
 [<ffffffff815d91ad>] ? erase_effect+0xad/0xf0
 [<ffffffff815d929d>] ? flush_effects+0x4d/0x90
 [<ffffffff815d4cc0>] ? input_flush_device+0x40/0x60
 [<ffffffff815daf1c>] ? evdev_cleanup+0xac/0xc0
 [<ffffffff815daf5b>] ? evdev_disconnect+0x2b/0x60
 [<ffffffff815d74ac>] ? __input_unregister_device+0xac/0x150
 [<ffffffff815d75f7>] ? input_unregister_device+0x47/0x70
 [<ffffffffa00bac45>] ? uinput_destroy_device+0xb5/0xc0 [uinput]
 [<ffffffffa00bb2de>] ? uinput_ioctl_handler.isra.9+0x65e/0x740 [uinput]
 [<ffffffff811231ab>] ? do_futex+0x12b/0xad0
 [<ffffffffa00bb3f8>] ? uinput_ioctl+0x18/0x20 [uinput]
 [<ffffffff81241248>] ? do_vfs_ioctl+0x298/0x480
 [<ffffffff81337553>] ? security_file_ioctl+0x43/0x60
 [<ffffffff812414a9>] ? SyS_ioctl+0x79/0x90
 [<ffffffff817a04ee>] ? entry_SYSCALL_64_fastpath+0x12/0x71

Re: How to properly extend uinput API

[Manuel Reimer]

On 05/03/2016 12:51 AM, Dmitry Torokhov wrote:
> Nowhere in the memoryless code we have "2 motor values".

But it simulates a few supported events with FF_RUMBLE events.

> You are
> basically designing an API for single device and that is not a good
> idea.

Not for a single device, but for a big (if not the biggest) group of 
game controllers.

> Hmm, it should not, can you post some traces to show where it gets
> stuck so we can fix it?

Some time ago, Elias Vanderstuyft sent me a link to this thread:

http://comments.gmane.org/gmane.linux.kernel.input/44528

Maybe this helps to track down this problem?

> You should be able to take cues from the kernel code and create
> userspace library providing similar features, but in the way that is
> flexible and make sense in userspace.

Of course this should be possible. It just is far more difficult than 
just using what is already there.

While searching for examples to learn from, I've collected a few 
existing uinput projects using force feedback. 100% of them somehow try 
to drive two motors and would profit from some API making this common 
case more easy.

But if the recommended way is to do this in userspace, I will try to 
understand how ff-memless works to port it 1:1 to userspace. 
Unfortunately the kernel documentation 
(https://www.kernel.org/doc/Documentation/input/ff.txt) says:

| Note: In most cases you should use FF_PERIODIC instead of FF_RUMBLE.
|       *All* devices that support FF_RUMBLE support FF_PERIODIC
|       (square, triangle, sine) and the other way around.

So I guess game developers will not just send FF_RUMBLE and so I have to 
do some kind of simulation to get my rumble working.

If someone already has simple code which can be used from plain C, I 
would be happy to get a link to the repository.

Manuel