[PATCH] [v5]Input: evdev: fix bug of dropping full valid packet after syn_dropped

[PATCH] [v5]Input: evdev: fix bug of dropping full valid packet after syn_dropped

[Aniroop Mathur]

If last event in old queue that was dropped was EV_SYN/SYN_REPORT, then
lets generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
so that clients would not ignore next valid full packet events.

Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
 drivers/input/evdev.c |   45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..0bc7b98 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 static void __evdev_queue_syn_dropped(struct evdev_client *client)
 {
 	struct input_event ev;
+	struct input_event *prev_ev;
 	ktime_t time;
+	unsigned int mask = client->bufsize - 1;
+
+	/* store previous event */
+	prev_ev = &client->buffer[(client->head - 1) & mask];
 
 	time = client->clk_type == EV_CLK_REAL ?
 			ktime_get_real() :
@@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
 	ev.value = 0;
 
 	client->buffer[client->head++] = ev;
-	client->head &= client->bufsize - 1;
+	client->head &= mask;
 
 	if (unlikely(client->head == client->tail)) {
 		/* drop queue but keep our SYN_DROPPED event */
-		client->tail = (client->head - 1) & (client->bufsize - 1);
+		client->tail = (client->head - 1) & mask;
 		client->packet_head = client->tail;
 	}
+
+	/*
+	 * If last packet was completely stored, then queue SYN_REPORT
+	 * so that clients would not ignore next valid full packet
+	 */
+	if (prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT) {
+		prev_ev->time = ev.time;
+		client->buffer[client->head++] = *prev_ev;
+		client->head &= mask;
+		client->packet_head = client->head;
+
+		/* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
+		if (unlikely(client->head == client->tail))
+			client->tail = (client->head - 2) & mask;
+	}
 }
 
 static void evdev_queue_syn_dropped(struct evdev_client *client)
@@ -235,18 +255,19 @@ static void __pass_event(struct evdev_client *client,
 	client->head &= client->bufsize - 1;
 
 	if (unlikely(client->head == client->tail)) {
+		client->packet_head = client->tail;
+		__evdev_queue_syn_dropped(client);
+
 		/*
-		 * This effectively "drops" all unconsumed events, leaving
-		 * EV_SYN/SYN_DROPPED plus the newest event in the queue.
+		 * Store newest event in the queue,
+		 * but if it is SYN_REPORT then it is already stored by
+		 * __evdev_queue_syn_dropped, so should not be stored again.
+		 * As buffer overrun just occurred so no need to check here.
 		 */
-		client->tail = (client->head - 2) & (client->bufsize - 1);
-
-		client->buffer[client->tail].time = event->time;
-		client->buffer[client->tail].type = EV_SYN;
-		client->buffer[client->tail].code = SYN_DROPPED;
-		client->buffer[client->tail].value = 0;
-
-		client->packet_head = client->tail;
+		if (event->type != EV_SYN && event->code != SYN_REPORT) {
+			client->buffer[client->head++] = *event;
+			client->head &= client->bufsize - 1;
+		}
 	}
 
 	if (event->type == EV_SYN && event->code == SYN_REPORT) {
-- 
1.7.9.5

Re: [PATCH] [v5]Input: evdev: fix bug of dropping full valid packet after syn_dropped

[Dmitry Torokhov]

On Wed, Jan 13, 2016 at 05:27:41PM +0530, Aniroop Mathur wrote:
> If last event in old queue that was dropped was EV_SYN/SYN_REPORT, then
> lets generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
> so that clients would not ignore next valid full packet events.
> 
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
>  drivers/input/evdev.c |   45 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..0bc7b98 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>  {
>  	struct input_event ev;
> +	struct input_event *prev_ev;
>  	ktime_t time;
> +	unsigned int mask = client->bufsize - 1;
> +
> +	/* store previous event */
> +	prev_ev = &client->buffer[(client->head - 1) & mask];

How do you know that previous event is valid/exists? In fact, when we
are dropping events due to the full queue, you will be referencing the
newest event being processed, not the previous event.

I also wonder if this code is safe with regard to __evdev_flush_queue()
that is dropping bunch of events and possible empty SYN_REPORT groups.

Thanks.

-- 
Dmitry

Re: [PATCH] [v5]Input: evdev: fix bug of dropping full valid packet after syn_dropped

[Aniroop Mathur]

On Thu, Jan 14, 2016 at 12:22 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jan 13, 2016 at 05:27:41PM +0530, Aniroop Mathur wrote:
>> If last event in old queue that was dropped was EV_SYN/SYN_REPORT, then
>> lets generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>> so that clients would not ignore next valid full packet events.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>>  drivers/input/evdev.c |   45 +++++++++++++++++++++++++++++++++------------
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..0bc7b98 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>  {
>>       struct input_event ev;
>> +     struct input_event *prev_ev;
>>       ktime_t time;
>> +     unsigned int mask = client->bufsize - 1;
>> +
>> +     /* store previous event */
>> +     prev_ev = &client->buffer[(client->head - 1) & mask];
>
> How do you know that previous event is valid/exists? In fact, when we
> are dropping events due to the full queue, you will be referencing the
> newest event being processed, not the previous event.
>

yes right, prev_ev variable name is fine for clk_change & flush_queue but not
for buffer overrun. It is better to give some other name, may be last_ev or
temp_ev. (the event chosen is right though)

In case of buffer overrun,
old events does exist due to which overrun occurred, so last/newest event does
exist to compare with. (referring by the name prev_ev currently in patch)

In case of clock change request,
we flush queue only if it is not empty, so last event does exist to compare
with.
Most likely, last event will be syn_report here as input core passes events
to handler when syn_report occurs.

In case __evdev_flush_queue,
tbh, I have not dealt with this function much and thats why in my v1 & v2 patch,
I added the code separately for clock change and buffer run. But on
more thought,
I could not thought of any problem so moved the code to common function
__evdev_queue_syn_dropped.
AFAICT, in this case, even if all events are flushed, one syn_report
will be there
in queue at least, so last event will exist to compare with.

BR,
Aniroop Mathur

> I also wonder if this code is safe with regard to __evdev_flush_queue()
> that is dropping bunch of events and possible empty SYN_REPORT groups.
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH] [v5]Input: evdev: fix bug of dropping full valid packet after syn_dropped

[Aniroop Mathur]

On Thu, Jan 14, 2016 at 12:22 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jan 13, 2016 at 05:27:41PM +0530, Aniroop Mathur wrote:
>> If last event in old queue that was dropped was EV_SYN/SYN_REPORT, then
>> lets generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>> so that clients would not ignore next valid full packet events.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>>  drivers/input/evdev.c |   45 +++++++++++++++++++++++++++++++++------------
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..0bc7b98 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>  {
>>       struct input_event ev;
>> +     struct input_event *prev_ev;
>>       ktime_t time;
>> +     unsigned int mask = client->bufsize - 1;
>> +
>> +     /* store previous event */
>> +     prev_ev = &client->buffer[(client->head - 1) & mask];
>
> How do you know that previous event is valid/exists? In fact, when we
> are dropping events due to the full queue, you will be referencing the
> newest event being processed, not the previous event.
>
> I also wonder if this code is safe with regard to __evdev_flush_queue()
> that is dropping bunch of events and possible empty SYN_REPORT groups.
>

Yes, right.
Sorry, I could not understand what you meant on the first read.
You were asking to validate head for last event dropped in queue,
but I understood something else.
Thanks for making me find the problem.
Now, I have corrected it and sent the new patch v6 and v7.

v6:
Corrected head index to store last event properly
About __evdev_flush_queue():
As input core passes packets (not single events) to evdev handler so
after flushing queue for particular events, there will always be syn_report
in the buffer at the end. And if flush request is for ev_syn, then
syn_report will could not be queued after syn_dropped, anyways.
Only problematic case will be if last event stored in the queue was not
syn_report and other events are filtered out such that last event now becomes
syn_report. But I cannot think of a case when last event stored in the buffer
was not syn_report during flushing the queue.

Still, if such case exist, we can take care of it:
How about we store the last event occurred before flushing the queue and
decides of the basis of this event?

v7:
Included fix during pass_events and during clock change request.
It does not consider ev_dev_flush_queue() case.

So, if you find no problem for the case __evdev_flush_queue, we
can use v6, otherwise v7.

BR,
Aniroop Mathur

> Thanks.
>
> --
> Dmitry