[PATCH libevdev 2/2] Discard incomplete sequences in case of SYN_DROPPED
Peter Hutterer
peter.hutterer at who-t.net
Tue Feb 25 17:31:34 PST 2014
On Tue, Feb 25, 2014 at 11:01:33AM +0100, David Herrmann wrote:
> Hi
>
> On Tue, Feb 25, 2014 at 5:26 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > A event sequence emitted by the kernel in case of SYN_DROPPED may be:
> > ABS_X
> > SYN_REPORT
>
> This SYN_REPORT should be reported right away with the ABS_X value. I
> don't know why you drop it and instead report both _X and _Y together
> as a sync?
>
> > ABS_Y
> > SYN_DROPPED
> > ABS_Y
> > SYN_REPORT
> >
> > Both ABS_Y events are parts of an incomplete sequence and must be
> > dropped, so the event sequence we return to the caller is.
> > ABS_X
> > ABS_Y
> > SYN_REPORT
> > SYN_DROPPED
>
> What do you mean by reporting SYN_DROPPED? Our re-sync phase? In this
> case, why do you report ABS_Y at all? It's part of an invalid report
> that we should not return to the caller with any cached _X values. For
> ABS this is mood, but for boolean values that only make sense in
> pairs, you might report wrong information here.
>
> If the kernel sends:
> ABS_X
> SYN_REPORT
> ABS_Y
> SYN_DROPPED
> ABS_Y
> SYN_REPORT
>
> My interpretation is for libevdev to return:
> ABS_X
> SYN_REPORT
> SYN_DROPPED
>
> The ABS_X is returned as it's a complete report. The ABS_Y reports are
> all dropped as they were not completed. Therefore, the queue might
> have been flushed and it should be resynced at SYN_DROPPED receival.
huh, yes, you're right. this is a commit message error and the behaviour of
the patch, sorry. last minute rewrite of the commit msg messed that up. So
yes, ABS_Y is dropped completely here (though it would be part of the sync
event sequence).
Cheers,
Peter
> >
> > This is a lot easier for callers to handle.
> >
> > It requires us to keep track of incomplete event sequences, specifically
> > whether there is an EV_SYN event in the pipe. The kernel usually doesn't send
> > incomplete sequences but if our internal buffer is almost full, we may not
> > read off all the events of a sequence.
> >
> > If there is no EV_SYN terminating the current sequence, we must pretend there
> > are no events waiting since the events we already do have may get discarded in
> > case of SYN_DROPPED.
> >
> > Note that this patch only handles dropping events _before_ SYN_DROPPED, events
> > after SYN_DROPPED are already handled by sync_state (see the comment at the
> > top of the function).
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > libevdev/libevdev.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
> > libevdev/libevdev.h | 6 ++++
> > test/test-libevdev-events.c | 64 +++++++++++++++++++++++++++++++++++----
> > 3 files changed, 134 insertions(+), 9 deletions(-)
> >
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index ead0205..04913fa 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -36,6 +36,12 @@
> >
> > #define MAXEVENTS 64
> >
> > +enum read_result {
> > + READ_RESULT_NO_EVENTS = 1,
> > + READ_RESULT_EVENTS,
> > + READ_RESULT_NO_EV_SYN,
> > +};
> > +
> > static int sync_mt_state(struct libevdev *dev, int create_events);
> >
> > static int
> > @@ -750,6 +756,52 @@ update_state(struct libevdev *dev, const struct input_event *e)
> > }
> >
> > static int
> > +check_for_syn_dropped(struct libevdev *dev)
> > +{
> > + size_t i;
> > + int last_syn_report = -1,
> > + syn_dropped = -1;
> > +
> > + if (queue_num_elements(dev) == 0)
> > + return READ_RESULT_NO_EVENTS;
> > +
> > + /* SYN_DROPPED may come at any time, any event since the last EV_SYN
> > + and up to the next EV_SYN must be dropped. see
> > + https://www.kernel.org/doc/Documentation/input/event-codes.txt
> > + */
> > +
> > + for (i = 0; i < queue_num_elements(dev); i++) {
> > + struct input_event ev;
> > +
> > + queue_peek(dev, i, &ev);
> > + if (ev.type == EV_SYN) {
> > + if (ev.code == SYN_REPORT)
> > + last_syn_report = i;
> > + else if (ev.code == SYN_DROPPED) {
> > + syn_dropped = i;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (syn_dropped == -1) {
> > + /* We've read an incomplete event frame due to our own buffer
> > + constraints, let's pretend there are no events ATM. The
> > + next read will give us the SYN_REPORT of this frame */
> > + if (last_syn_report == -1)
> > + return READ_RESULT_NO_EV_SYN;
> > +
> > + return READ_RESULT_EVENTS;
> > + }
> > +
> > + /* slice events out, but leave SYN_DROPPED in place */
> > + if (syn_dropped > 0)
> > + queue_slice(dev, last_syn_report + 1, syn_dropped - last_syn_report - 1);
> > +
> > + return READ_RESULT_EVENTS;
> > +}
> > +
> > +static int
> > read_more_events(struct libevdev *dev)
> > {
> > int free_elem;
> > @@ -758,7 +810,7 @@ read_more_events(struct libevdev *dev)
> >
> > free_elem = queue_num_free_elements(dev);
> > if (free_elem <= 0)
> > - return 0;
> > + return READ_RESULT_NO_EVENTS;
> >
> > next = queue_next_element(dev);
> > len = read(dev->fd, next, free_elem * sizeof(struct input_event));
> > @@ -769,9 +821,11 @@ read_more_events(struct libevdev *dev)
> > else if (len > 0) {
> > int nev = len/sizeof(struct input_event);
> > queue_set_num_elements(dev, queue_num_elements(dev) + nev);
> > +
> > + return check_for_syn_dropped(dev);
> > }
> >
> > - return 0;
> > + return READ_RESULT_NO_EVENTS;
> > }
> >
> > static int
> > @@ -856,6 +910,10 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> > goto out;
> > }
> >
> > + if (rc == READ_RESULT_NO_EV_SYN) {
> > + rc = -EAGAIN;
> > + goto out;
> > + }
> >
> > if (queue_shift(dev, ev) != 0)
> > return -EAGAIN;
> > @@ -891,8 +949,15 @@ libevdev_has_event_pending(struct libevdev *dev)
> > } else if (dev->fd < 0)
> > return -EBADF;
> >
> > - if (queue_num_elements(dev) != 0)
> > - return 1;
> > + if (queue_num_elements(dev) != 0) {
> > + size_t i;
> > + for (i = 0; i < queue_num_elements(dev); i++) {
> > + struct input_event ev;
> > + queue_peek(dev, i, &ev);
> > + if (ev.type == EV_SYN)
> > + return 1;
> > + }
> > + }
> >
> > rc = poll(&fds, 1, 0);
> > return (rc >= 0) ? rc : -errno;
> > diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> > index 898e919..ad409dd 100644
> > --- a/libevdev/libevdev.h
> > +++ b/libevdev/libevdev.h
> > @@ -699,6 +699,12 @@ enum libevdev_read_status {
> > * dropped after libevdev updates its internal state and event processing
> > * continues as normal.
> > *
> > + * libevdev transparently drops events that are part of an incomplete event
> > + * sequence caused by a SYN_DROPPED event. The caller is guaranteed that the
> > + * last event before a SYN_DROPPED event is an EV_SYN, and the next event
> > + * after a SYN_DROPPED is the first event of the next valid event sequence
> > + * (or another SYN_DROPPED) event.
> > + *
> > * @param dev The evdev device, already initialized with libevdev_set_fd()
> > * @param flags Set of flags to determine behaviour. If @ref LIBEVDEV_READ_FLAG_NORMAL
> > * is set, the next event is read in normal mode. If @ref LIBEVDEV_READ_FLAG_SYNC is
> > diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> > index 666b98d..1b68469 100644
> > --- a/test/test-libevdev-events.c
> > +++ b/test/test-libevdev-events.c
> > @@ -66,6 +66,14 @@ START_TEST(test_syn_dropped_event)
> > struct libevdev *dev;
> > int rc;
> > struct input_event ev;
> > + struct input_event syn_dropped_sequence[] = {
> > + { .type = EV_REL, .code = REL_X, .value = 1 },
> > + { .type = EV_REL, .code = REL_Y, .value = 2 },
> > + { .type = EV_SYN, .code = SYN_DROPPED, .value = 0 },
> > + { .type = EV_REL, .code = REL_X, .value = 3 },
> > + { .type = EV_REL, .code = REL_Y, .value = 4 },
> > + { .type = EV_SYN, .code = SYN_REPORT, .value = 0 },
> > + };
> > int pipefd[2];
> >
> > rc = test_create_device(&uidev, &dev,
> > @@ -95,11 +103,9 @@ START_TEST(test_syn_dropped_event)
> > ck_assert_int_eq(rc, 0);
> >
> > libevdev_change_fd(dev, pipefd[0]);
> > - ev.type = EV_SYN;
> > - ev.code = SYN_DROPPED;
> > - ev.value = 0;
> > - rc = write(pipefd[1], &ev, sizeof(ev));
> > - ck_assert_int_eq(rc, sizeof(ev));
> > + rc = write(pipefd[1], syn_dropped_sequence, sizeof(syn_dropped_sequence));
> > + ck_assert_int_eq(rc, sizeof(syn_dropped_sequence));
> > +
> > rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> >
> > libevdev_change_fd(dev, uinput_device_get_fd(uidev));
> > @@ -112,6 +118,11 @@ START_TEST(test_syn_dropped_event)
> > ck_assert_int_eq(ev.type, EV_SYN);
> > ck_assert_int_eq(ev.code, SYN_DROPPED);
> >
> > + /* no delta, so expect EAGAIN since the REL_X/REL_Y we wrote after
> > + * SYN_DROPPED are supposed to be dropped */
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> > + ck_assert_int_eq(rc, -EAGAIN);
> > +
> > /* only check for the rc, nothing actually changed on the device */
> >
> > libevdev_free(dev);
> > @@ -345,6 +356,48 @@ START_TEST(test_has_event_pending)
> >
> > }
> > END_TEST
> > +
> > +START_TEST(test_next_event_incomplete)
> > +{
> > + struct uinput_device* uidev;
> > + struct libevdev *dev;
> > + int rc;
> > + struct input_event ev;
> > + int pipefd[2];
> > + struct input_event incomplete_sequence[] = {
> > + { .type = EV_REL, .code = REL_X, .value = 1 },
> > + { .type = EV_REL, .code = REL_Y, .value = 2 },
> > + };
> > +
> > + rc = test_create_device(&uidev, &dev,
> > + EV_REL, REL_X,
> > + EV_REL, REL_Y,
> > + EV_KEY, BTN_LEFT,
> > + -1);
> > + ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> > +
> > + /* we can't write incomplete events through uinput, so let's use a
> > + * pipe instead */
> > + rc = pipe2(pipefd, O_NONBLOCK);
> > + ck_assert_int_eq(rc, 0);
> > +
> > + libevdev_change_fd(dev, pipefd[0]);
> > +
> > + rc = write(pipefd[1], incomplete_sequence, sizeof(incomplete_sequence));
> > + ck_assert_int_eq(rc, sizeof(incomplete_sequence));
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > + ck_assert_int_eq(rc, -EAGAIN);
> > +
> > + ck_assert_int_eq(libevdev_has_event_pending(dev), 0);
> > +
> > + libevdev_free(dev);
> > + uinput_device_free(uidev);
> > + close(pipefd[0]);
> > + close(pipefd[1]);
> > +}
> > +END_TEST
> > +
> > START_TEST(test_syn_delta_button)
> > {
> > struct uinput_device* uidev;
> > @@ -1317,6 +1370,7 @@ libevdev_events(void)
> > tcase_add_test(tc, test_event_type_filtered);
> > tcase_add_test(tc, test_event_code_filtered);
> > tcase_add_test(tc, test_has_event_pending);
> > + tcase_add_test(tc, test_next_event_incomplete);
> > suite_add_tcase(s, tc);
> >
> > tc = tcase_create("SYN_DROPPED deltas");
> > --
> > 1.8.4.2
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list