Regression in dtor/input.git/next - flush pending events on clock type change

Regression in dtor/input.git/next - flush pending events on clock type change

[Benjamin Tissoires]

Hi Anshul,

The commit 0c3e99437a66e4c869c60c2398449e6d98f3a988 in dtor/input.git/next
tree introduce an interesting regression in libinput. The tests fail :)

Actually, evemu-record and libinput switch the clock to monotonic when
opening an input node, and the first thing that gets queued is a
SYN_DROPPED event.

However, in the libinput test suite, events are the bare minimum, and
most of the tests contain only one event set (one EV_SYN).
When seeing the SYN_DROPPED, the clients are supposed to drain the events
until the next EV_SYN, and so they are losing the events that came long
after the ioctl call.
And in the end, the test suite does not receive any events.

Removing the evdev_queue_syn_dropped() call in the ioctl handling fixes
the test suite, and Peter suggested that maybe we should queue a
SYN_DROPPED event iff there are events in the queue.

Cheers,
Benjamin

Re: Regression in dtor/input.git/next - flush pending events on clock type change

[Dmitry Torokhov]

Hi Benjamin,

On Thu, Feb 05, 2015 at 06:06:29PM -0500, Benjamin Tissoires wrote:
> Hi Anshul,
> 
> The commit 0c3e99437a66e4c869c60c2398449e6d98f3a988 in dtor/input.git/next
> tree introduce an interesting regression in libinput. The tests fail :)
> 
> Actually, evemu-record and libinput switch the clock to monotonic when
> opening an input node, and the first thing that gets queued is a
> SYN_DROPPED event.
> 
> However, in the libinput test suite, events are the bare minimum, and
> most of the tests contain only one event set (one EV_SYN).
> When seeing the SYN_DROPPED, the clients are supposed to drain the events
> until the next EV_SYN, and so they are losing the events that came long
> after the ioctl call.
> And in the end, the test suite does not receive any events.
> 
> Removing the evdev_queue_syn_dropped() call in the ioctl handling fixes
> the test suite, and Peter suggested that maybe we should queue a
> SYN_DROPPED event iff there are events in the queue.

Does the following patch fixe it? But I would like to see libinput
tests more robust.

Thanks.

-- 
Dmitry

Input: evdev - do not queue SYN_DROPPED if queue is empty

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/evdev.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e7cee38..f2a494a 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -108,10 +108,8 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 	client->head = head;
 }
 
-/* queue SYN_DROPPED event and flush queue if flush parameter is true */
-static void evdev_queue_syn_dropped(struct evdev_client *client, bool flush)
+static void __evdev_queue_syn_dropped(struct evdev_client *client)
 {
-	unsigned long flags;
 	struct input_event ev;
 	ktime_t time;
 
@@ -126,11 +124,6 @@ static void evdev_queue_syn_dropped(struct evdev_client *client, bool flush)
 	ev.code = SYN_DROPPED;
 	ev.value = 0;
 
-	spin_lock_irqsave(&client->buffer_lock, flags);
-
-	if (flush)
-		client->packet_head = client->head = client->tail;
-
 	client->buffer[client->head++] = ev;
 	client->head &= client->bufsize - 1;
 
@@ -139,12 +132,21 @@ static void evdev_queue_syn_dropped(struct evdev_client *client, bool flush)
 		client->tail = (client->head - 1) & (client->bufsize - 1);
 		client->packet_head = client->tail;
 	}
+}
+
+static void evdev_queue_syn_dropped(struct evdev_client *client)
+{
+	unsigned long flags;
 
+	spin_lock_irqsave(&client->buffer_lock, flags);
+	__evdev_queue_syn_dropped(client);
 	spin_unlock_irqrestore(&client->buffer_lock, flags);
 }
 
 static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {
+	unsigned long flags;
+
 	if (client->clk_type == clkid)
 		return 0;
 
@@ -163,8 +165,18 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 		return -EINVAL;
 	}
 
-	/* Flush pending events and queue SYN_DROPPED event.*/
-	evdev_queue_syn_dropped(client, true);
+	/*
+	 * Flush pending events and queue SYN_DROPPED event,
+	 * but only if queue is not empty.
+	 */
+	spin_lock_irqsave(&client->buffer_lock, flags);
+
+	if (client->head != client->tail) {
+		client->packet_head = client->head = client->tail;
+		__evdev_queue_syn_dropped(client);
+	}
+
+	spin_unlock_irqrestore(&client->buffer_lock, flags);
 
 	return 0;
 }
@@ -803,7 +815,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 
 	ret = bits_to_user(mem, maxbit, maxlen, p, compat);
 	if (ret < 0)
-		evdev_queue_syn_dropped(client, false);
+		evdev_queue_syn_dropped(client);
 
 	kfree(mem);
 

Re: Regression in dtor/input.git/next - flush pending events on clock type change

[Dmitry Torokhov]

On Thu, Feb 05, 2015 at 08:28:28PM -0500, Benjamin Tissoires wrote:
> On Feb 5, 2015 7:04 PM, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Benjamin,
> >
> > On Thu, Feb 05, 2015 at 06:06:29PM -0500, Benjamin Tissoires wrote:
> > > Hi Anshul,
> > >
> > > The commit 0c3e99437a66e4c869c60c2398449e6d98f3a988 in
> dtor/input.git/next
> > > tree introduce an interesting regression in libinput. The tests fail :)
> > >
> > > Actually, evemu-record and libinput switch the clock to monotonic when
> > > opening an input node, and the first thing that gets queued is a
> > > SYN_DROPPED event.
> > >
> > > However, in the libinput test suite, events are the bare minimum, and
> > > most of the tests contain only one event set (one EV_SYN).
> > > When seeing the SYN_DROPPED, the clients are supposed to drain the
> events
> > > until the next EV_SYN, and so they are losing the events that came long
> > > after the ioctl call.
> > > And in the end, the test suite does not receive any events.
> > >
> > > Removing the evdev_queue_syn_dropped() call in the ioctl handling fixes
> > > the test suite, and Peter suggested that maybe we should queue a
> > > SYN_DROPPED event iff there are events in the queue.
> >
> > Does the following patch fixe it? But I would like to see libinput
> > tests more robust.
> 
> It does. Thanks for the quick fix.
> 
> Regarding libinput tests, I am not sure we could make them more robust in
> this situation. The tests rely on uinput to create predetermined kernel
> devices, with a known set of events. Usually, we test one feature/previous
> bug we already seen in the past per device per test. The mentioned commit
> changed the kernel behavior and I think there is no automatic way to detect
> that the problem lies in the kernel rather than in the libinput event
> processing.
> 
> For example, the simplest test creates one mouse, waits for libinput to
> open it, sends REL_X, EV_SYN, and ensures that libinput gets the REL_X
> event. Without this fix, the event is not seen, so the test fails. Which is
> right, because that means that any libinput client will see the first
> events dropped. This is not something we want for our users, especially for
> keyboards, when the first thing you do is typing your password for example.

OK, fair enough. I'll queue the patch with your tested-by then.

Thanks.

-- 
Dmitry

Re: Regression in dtor/input.git/next - flush pending events on clock type change

[Benjamin Tissoires]

On Thu, Feb 5, 2015 at 8:28 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
>
> On Feb 5, 2015 7:04 PM, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
>>
>> Hi Benjamin,
>>
>> On Thu, Feb 05, 2015 at 06:06:29PM -0500, Benjamin Tissoires wrote:
>> > Hi Anshul,
>> >
>> > The commit 0c3e99437a66e4c869c60c2398449e6d98f3a988 in
>> > dtor/input.git/next
>> > tree introduce an interesting regression in libinput. The tests fail :)
>> >
>> > Actually, evemu-record and libinput switch the clock to monotonic when
>> > opening an input node, and the first thing that gets queued is a
>> > SYN_DROPPED event.
>> >
>> > However, in the libinput test suite, events are the bare minimum, and
>> > most of the tests contain only one event set (one EV_SYN).
>> > When seeing the SYN_DROPPED, the clients are supposed to drain the
>> > events
>> > until the next EV_SYN, and so they are losing the events that came long
>> > after the ioctl call.
>> > And in the end, the test suite does not receive any events.
>> >
>> > Removing the evdev_queue_syn_dropped() call in the ioctl handling fixes
>> > the test suite, and Peter suggested that maybe we should queue a
>> > SYN_DROPPED event iff there are events in the queue.
>>
>> Does the following patch fixe it? But I would like to see libinput
>> tests more robust.
>
> It does. Thanks for the quick fix.
>
> Regarding libinput tests, I am not sure we could make them more robust in
> this situation. The tests rely on uinput to create predetermined kernel
> devices, with a known set of events. Usually, we test one feature/previous
> bug we already seen in the past per device per test. The mentioned commit
> changed the kernel behavior and I think there is no automatic way to detect
> that the problem lies in the kernel rather than in the libinput event
> processing.
>
> For example, the simplest test creates one mouse, waits for libinput to open
> it, sends REL_X, EV_SYN, and ensures that libinput gets the REL_X event.
> Without this fix, the event is not seen, so the test fails. Which is right,
> because that means that any libinput client will see the first events
> dropped. This is not something we want for our users, especially for
> keyboards, when the first thing you do is typing your password for example.
>
> Cheers,
> Benjamin
>

Grmbl, sorry for the dup. Re-sending the mail not from the tablet
which can not send the mail in plain text... :(

Cheers,
Benajmin