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@xxxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>>  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