[PATCH libevdev] Document that the fd should be drained before libevdev_set_fd

[David Herrmann]

Hi

On Wed, Dec 16, 2015 at 10:57 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> On Wed, Dec 16, 2015 at 11:58:32AM +0100, David Herrmann wrote:
>> On Wed, Dec 16, 2015 at 1:39 AM, Peter Hutterer
>> <peter.hutterer at who-t.net> wrote:
>> > + * A caller should ensure that any events currently pending on the fd are
>> > + * drained before the file descriptor is passed to libevdev for
>> > + * initialization. Due to how the kernel's ioctl handling works, the initial
>> > + * device state will reflect the current device state *after* applying all
>> > + * events currently pending on the fd. Thus, if the fd is not drained, the
>> > + * state visible to the caller will be inconsistent with the events
>> > + * immediately available on the device. This does not affect state-less
>> > + * events like EV_REL.
>> > + *
>>
>> Care to elaborate on this? Even if the caller drains the FD, there
>> might be events coming in _before_ you can pass it to libevdev. I can
>> see that the draining works as a barrier, but I cannot see how your
>> comment addresses that. TBH, I'm not entirely sure what your comment
>> is trying to say.
>
> the specific use-case that triggered this was the X case where we open the
> fd once on server start, then keep it open until shutdown/VT switch. if a
> device is disabled and re-enabled the fd isn't opened again, it is kept
> open. the driver doesn't necessarily know about that though, it just gets
> an fd and initializes the libevdev device. So events that happen between
> disabling and enabling the device are available on fd immediately when they
> shouldn't be.
>
> libevdev doesn't know this is happening and there's an argument that it
> shouldn't know. hence the documentation update only, the xorg drivers (and
> libinput, in this case) need to be updated to drain anything still sitting
> there.
>
>> I can imagine a situation where an ABS event, followed by a
>> slot-change is pending. If you now open the FD and initialize
>> libevdev, then it will have the SLOT event already parsed, as such the
>> pending ABS event is wrongly attributed. Is this what this is
>> referring to? Because if it is, you cannot solve it by draining the
>> fd. The race is still there. The solution would rather be to force the
>> kernel to flush the input-queue if you query device state (which is
>> already partly done by the kernel).
>
> it's done partially, but some events get through. simple use-case is
>    xinput disable <mouse device>
>    <move the mouse a bit>
>    xinput enable <mouse device>
>
> you'll see the cursor jump based on the movement while the device was
> disabled. quick testing yesterday showed that this doesn't apply to key
> events (though MSC_SCAN still gets through).
>
> either way, it's a race condition, so we can't get rid of it completely.
> but there's a definitive line of "this event happened before we even got the
> fd" - those events are usually wrong and should be drained by the caller.

I see, this makes sense. I just wanted to make sure that you're not
explicitly referring to the race-condition here (maybe we should
revive the EVIOCRESYNC patches that flush the entire state?). Anyway,
this is:

Acked-by: David Herrmann <dh.herrmann at gmail.com>

Thanks a lot!
David