If last event dropped in the old queue 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 <[email protected]>
Difference from v8:
Added check for handling EVIOCG[type] ioctl for queue empty case
---
drivers/input/evdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..69407ff 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 *last_ev;
ktime_t time;
+ unsigned int mask = client->bufsize - 1;
+
+ /* capture last event stored in the buffer */
+ last_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 (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
+ last_ev->time = ev.time;
+ client->buffer[client->head++] = *last_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)
@@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
spin_lock_irqsave(&client->buffer_lock, flags);
if (client->head != client->tail) {
- client->packet_head = client->head = client->tail;
+ client->packet_head = client->tail = client->head;
__evdev_queue_syn_dropped(client);
}
@@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
static void __pass_event(struct evdev_client *client,
const struct input_event *event)
{
+ unsigned int mask = client->bufsize - 1;
+
client->buffer[client->head++] = *event;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/*
* This effectively "drops" all unconsumed events, leaving
- * EV_SYN/SYN_DROPPED plus the newest event in the queue.
+ * EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
+ * newest event in the queue.
*/
- 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->head = (client->head - 1) & mask;
+ client->packet_head = client->tail = client->head;
+ __evdev_queue_syn_dropped(client);
- client->packet_head = client->tail;
+ client->buffer[client->head++] = *event;
+ client->head &= mask;
+ /* No need to check for buffer overflow as it just occurred */
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
@@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
int ret;
unsigned long *mem;
size_t len;
+ bool is_queue_empty;
len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
mem = kmalloc(len, GFP_KERNEL);
@@ -933,12 +956,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
spin_unlock(&dev->event_lock);
+ if (client->head == client->tail)
+ is_queue_empty = true;
+
__evdev_flush_queue(client, type);
spin_unlock_irq(&client->buffer_lock);
ret = bits_to_user(mem, maxbit, maxlen, p, compat);
- if (ret < 0)
+ if (ret < 0 && !is_queue_empty)
evdev_queue_syn_dropped(client);
kfree(mem);
--
2.6.2
Hello Mr. Torokhov,
Hope you are doing great!
Could you please help to update about below version of the patch?
--
Copying text about last two problems in v8:
Problem 1: Handle EVIOCG[type] for queue empty case
--> Done
Queue empty condition needs to be added before calling evdev_queue_syn_dropped.
Since there is a rare chance that new packet gets inserted between buffer_lock
mutex unlocking and copying events to user space, so I will check whether queue
is empty or not before __evdev_flush_queue and then use it before invoking
evdev_queue_syn_dropped.
And if queue is not empty then there is no problem as after flushing some
events we always have atleast one SYN_REPORT event left in queue.
Problem 2: We try to pass full packets to clients, but there is no guarantee.
--> Done
>From my earlier patch discussion regarding dropping syn_report in between a
single packet it was deduced that since no driver is currently using that code
so there is no impact. Hence for this problem too, we are good to go.~
(Although from code reading/learning perspective, I would still suggest to not
have code of forceful sun_report event addition even if no driver using that
code :-) )
Have a good day!
Best Regards,
Aniroop Mathur
> On Wed, Oct 5, 2016 at 12:42 AM, Aniroop Mathur <[email protected]> wrote:
>> If last event dropped in the old queue 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 <[email protected]>
>>
>> Difference from v8:
>> Added check for handling EVIOCG[type] ioctl for queue empty case
>> ---
>> drivers/input/evdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..69407ff 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 *last_ev;
>> ktime_t time;
>> + unsigned int mask = client->bufsize - 1;
>> +
>> + /* capture last event stored in the buffer */
>> + last_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 (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
>> + last_ev->time = ev.time;
>> + client->buffer[client->head++] = *last_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)
>> @@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>> spin_lock_irqsave(&client->buffer_lock, flags);
>>
>> if (client->head != client->tail) {
>> - client->packet_head = client->head = client->tail;
>> + client->packet_head = client->tail = client->head;
>> __evdev_queue_syn_dropped(client);
>> }
>>
>> @@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>> static void __pass_event(struct evdev_client *client,
>> const struct input_event *event)
>> {
>> + unsigned int mask = client->bufsize - 1;
>> +
>> client->buffer[client->head++] = *event;
>> - client->head &= client->bufsize - 1;
>> + client->head &= mask;
>>
>> if (unlikely(client->head == client->tail)) {
>> /*
>> * This effectively "drops" all unconsumed events, leaving
>> - * EV_SYN/SYN_DROPPED plus the newest event in the queue.
>> + * EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
>> + * newest event in the queue.
>> */
>> - 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->head = (client->head - 1) & mask;
>> + client->packet_head = client->tail = client->head;
>> + __evdev_queue_syn_dropped(client);
>>
>> - client->packet_head = client->tail;
>> + client->buffer[client->head++] = *event;
>> + client->head &= mask;
>> + /* No need to check for buffer overflow as it just occurred */
>> }
>>
>> if (event->type == EV_SYN && event->code == SYN_REPORT) {
>> @@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>> int ret;
>> unsigned long *mem;
>> size_t len;
>> + bool is_queue_empty;
>>
>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>> mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +956,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>
>> spin_unlock(&dev->event_lock);
>>
>> + if (client->head == client->tail)
>> + is_queue_empty = true;
>> +
>> __evdev_flush_queue(client, type);
>>
>> spin_unlock_irq(&client->buffer_lock);
>>
>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> - if (ret < 0)
>> + if (ret < 0 && !is_queue_empty)
>> evdev_queue_syn_dropped(client);
>>
>> kfree(mem);
>> --
>> 2.6.2
>>
Hello Mr. Torokhov,
As you can see that this patch is pending for a long time,
I request you to please review it at the earliest possible.
Thanks,
Aniroop Mathur
On Thu, Oct 13, 2016 at 12:05 AM, Aniroop Mathur
<[email protected]> wrote:
> Hello Mr. Torokhov,
>
> Hope you are doing great!
>
> Could you please help to update about below version of the patch?
>
> --
> Copying text about last two problems in v8:
>
> Problem 1: Handle EVIOCG[type] for queue empty case
> --> Done
> Queue empty condition needs to be added before calling evdev_queue_syn_dropped.
> Since there is a rare chance that new packet gets inserted between buffer_lock
> mutex unlocking and copying events to user space, so I will check whether queue
> is empty or not before __evdev_flush_queue and then use it before invoking
> evdev_queue_syn_dropped.
> And if queue is not empty then there is no problem as after flushing some
> events we always have atleast one SYN_REPORT event left in queue.
>
> Problem 2: We try to pass full packets to clients, but there is no guarantee.
> --> Done
> From my earlier patch discussion regarding dropping syn_report in between a
> single packet it was deduced that since no driver is currently using that code
> so there is no impact. Hence for this problem too, we are good to go.~
> (Although from code reading/learning perspective, I would still suggest to not
> have code of forceful sun_report event addition even if no driver using that
> code :-) )
>
> Have a good day!
>
> Best Regards,
> Aniroop Mathur
>
>
>> On Wed, Oct 5, 2016 at 12:42 AM, Aniroop Mathur <[email protected]> wrote:
>>> If last event dropped in the old queue 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 <[email protected]>
>>>
>>> Difference from v8:
>>> Added check for handling EVIOCG[type] ioctl for queue empty case
>>> ---
>>> drivers/input/evdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
>>> 1 file changed, 39 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>>> index e9ae3d5..69407ff 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 *last_ev;
>>> ktime_t time;
>>> + unsigned int mask = client->bufsize - 1;
>>> +
>>> + /* capture last event stored in the buffer */
>>> + last_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 (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
>>> + last_ev->time = ev.time;
>>> + client->buffer[client->head++] = *last_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)
>>> @@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>>> spin_lock_irqsave(&client->buffer_lock, flags);
>>>
>>> if (client->head != client->tail) {
>>> - client->packet_head = client->head = client->tail;
>>> + client->packet_head = client->tail = client->head;
>>> __evdev_queue_syn_dropped(client);
>>> }
>>>
>>> @@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>>> static void __pass_event(struct evdev_client *client,
>>> const struct input_event *event)
>>> {
>>> + unsigned int mask = client->bufsize - 1;
>>> +
>>> client->buffer[client->head++] = *event;
>>> - client->head &= client->bufsize - 1;
>>> + client->head &= mask;
>>>
>>> if (unlikely(client->head == client->tail)) {
>>> /*
>>> * This effectively "drops" all unconsumed events, leaving
>>> - * EV_SYN/SYN_DROPPED plus the newest event in the queue.
>>> + * EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
>>> + * newest event in the queue.
>>> */
>>> - 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->head = (client->head - 1) & mask;
>>> + client->packet_head = client->tail = client->head;
>>> + __evdev_queue_syn_dropped(client);
>>>
>>> - client->packet_head = client->tail;
>>> + client->buffer[client->head++] = *event;
>>> + client->head &= mask;
>>> + /* No need to check for buffer overflow as it just occurred */
>>> }
>>>
>>> if (event->type == EV_SYN && event->code == SYN_REPORT) {
>>> @@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>> int ret;
>>> unsigned long *mem;
>>> size_t len;
>>> + bool is_queue_empty;
>>>
>>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>> mem = kmalloc(len, GFP_KERNEL);
>>> @@ -933,12 +956,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
>>>
>>> spin_unlock(&dev->event_lock);
>>>
>>> + if (client->head == client->tail)
>>> + is_queue_empty = true;
>>> +
>>> __evdev_flush_queue(client, type);
>>>
>>> spin_unlock_irq(&client->buffer_lock);
>>>
>>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>>> - if (ret < 0)
>>> + if (ret < 0 && !is_queue_empty)
>>> evdev_queue_syn_dropped(client);
>>>
>>> kfree(mem);
>>> --
>>> 2.6.2
>>>
Hi Anoroop,
On Wed, Oct 05, 2016 at 12:42:56AM +0530, Aniroop Mathur wrote:
> If last event dropped in the old queue 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 <[email protected]>
>
> Difference from v8:
> Added check for handling EVIOCG[type] ioctl for queue empty case
> ---
> drivers/input/evdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..69407ff 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 *last_ev;
> ktime_t time;
> + unsigned int mask = client->bufsize - 1;
> +
> + /* capture last event stored in the buffer */
> + last_ev = &client->buffer[(client->head - 1) & mask];
I am uneasy with this "looking back" into the queue. How can we be sure
that we are looking at the right event? As far as I can tell we do not
know if that event has been consumed or not.
>
> 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 (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
> + last_ev->time = ev.time;
> + client->buffer[client->head++] = *last_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)
> @@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
> spin_lock_irqsave(&client->buffer_lock, flags);
>
> if (client->head != client->tail) {
> - client->packet_head = client->head = client->tail;
> + client->packet_head = client->tail = client->head;
> __evdev_queue_syn_dropped(client);
> }
>
> @@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
> static void __pass_event(struct evdev_client *client,
> const struct input_event *event)
> {
> + unsigned int mask = client->bufsize - 1;
> +
> client->buffer[client->head++] = *event;
> - client->head &= client->bufsize - 1;
> + client->head &= mask;
>
> if (unlikely(client->head == client->tail)) {
> /*
> * This effectively "drops" all unconsumed events, leaving
> - * EV_SYN/SYN_DROPPED plus the newest event in the queue.
> + * EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
> + * newest event in the queue.
> */
> - 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->head = (client->head - 1) & mask;
> + client->packet_head = client->tail = client->head;
> + __evdev_queue_syn_dropped(client);
>
> - client->packet_head = client->tail;
> + client->buffer[client->head++] = *event;
> + client->head &= mask;
> + /* No need to check for buffer overflow as it just occurred */
> }
>
> if (event->type == EV_SYN && event->code == SYN_REPORT) {
> @@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
> int ret;
> unsigned long *mem;
> size_t len;
> + bool is_queue_empty;
>
> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> mem = kmalloc(len, GFP_KERNEL);
> @@ -933,12 +956,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
>
> spin_unlock(&dev->event_lock);
>
> + if (client->head == client->tail)
> + is_queue_empty = true;
> +
> __evdev_flush_queue(client, type);
>
> spin_unlock_irq(&client->buffer_lock);
>
> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> - if (ret < 0)
> + if (ret < 0 && !is_queue_empty)
> evdev_queue_syn_dropped(client);
>
> kfree(mem);
> --
> 2.6.2
>
Thanks.
--
Dmitry
Hello Mr. Torokhov,
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> {
>> struct input_event ev;
>> + struct input_event *last_ev;
>> ktime_t time;
>> + unsigned int mask = client->bufsize - 1;
>> +
>> + /* capture last event stored in the buffer */
>> + last_ev = &client->buffer[(client->head - 1) & mask];
> I am uneasy with this "looking back" into the queue. How can we be sure
> that we are looking at the right event? As far as I can tell we do not
> know if that event has been consumed or not.
Well, I think that for an exceptional case where even if last event is
consumed then also it is the right event to proceed with.
Before mentioning then exceptional case, there is a need to fix a bug in
calling evdev_queue_syn_dropped in evdev_handle_get_val function.
The bug is:
Currently we are calling evdev_queue_syn_dropped without taking care
whether some events have actually been flushed out or not by calling
__evdev_flush_queue function. But ideally we should call
evdev_queue_syn_dropped only when some events are being flushed out.
For example: If client requests for EV_KEY events and queue only has
EV_LED type of events, then it is possible that bits_to_user fails
but no events get flushed out from queue and hence we should not be
inserting SYN_DROPPED event for this case.
So to fix this,
I think we need to return the number of events dropped from function
__evdev_flush_queue, say n is that value. So only if n > 0, then only
evdev_queue_syn_dropped should be called.
- __evdev_flush_queue(client, type);
+ n = __evdev_flush_queue(client, type);
- if (ret < 0)
+ if (ret < 0 && n > 0)
evdev_queue_syn_dropped(client);
Along with this bug fix, it will also handle the case when queue is
empty at time of invoking EVIOCG[type] ioctl call so after including
this fix, we can remove explicit handling of queue empty case from this patch.
After having this change, that exceptional case where even if the last event
is consumed, it is still the right event is:
Suppose client request from EV_KEY type of events using EVIOCG[type] ioctl call
and queue does contain EV_KEY type of events and bits_to_user fails too.
Now, after this when we invoke evdev_queue_syn_dropped function, there is a
chance that queue get fully consumed i.e. head == tail. So last event is
consumed as well. But since we need to send SYN_DROPPED event for this case
to indicate to user space that some events have been dropped, so we also have
to check whether we need to insert a SYN_REPORT event or not right after
SYN_DROPPED event using that last consumed event. And I think that it is for
sure that this last event is actually SYN_REPORT event since input core
always send events in packets so SYN_REPORT is always the last event forwarded
by input core to evdev.
Does the above mentioned points seem okay to you?
--
Best Regards,
Aniroop Mathur
--------- Original Message ---------
Sender : Dmitry Torokhov <[email protected]>
Date : 2016-11-20 00:30 (GMT+5:30)
Title : Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Hi Anoroop,
On Wed, Oct 05, 2016 at 12:42:56AM +0530, Aniroop Mathur wrote:
> If last event dropped in the old queue 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 <[email protected]>
>
> Difference from v8:
> Added check for handling EVIOCG[type] ioctl for queue empty case
> ---
> drivers/input/evdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..69407ff 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 *last_ev;
> ktime_t time;
> + unsigned int mask = client->bufsize - 1;
> +
> + /* capture last event stored in the buffer */
> + last_ev = &client->buffer[(client->head - 1) & mask];
I am uneasy with this "looking back" into the queue. How can we be sure
that we are looking at the right event? As far as I can tell we do not
know if that event has been consumed or not.
>
> 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 (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
> + last_ev->time = ev.time;
> + client->buffer[client->head++] = *last_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)
> @@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
> spin_lock_irqsave(&client->buffer_lock, flags);
>
> if (client->head != client->tail) {
> - client->packet_head = client->head = client->tail;
> + client->packet_head = client->tail = client->head;
> __evdev_queue_syn_dropped(client);
> }
>
> @@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
> static void __pass_event(struct evdev_client *client,
> const struct input_event *event)
> {
> + unsigned int mask = client->bufsize - 1;
> +
> client->buffer[client->head++] = *event;
> - client->head &= client->bufsize - 1;
> + client->head &= mask;
>
> if (unlikely(client->head == client->tail)) {
> /*
> * This effectively "drops" all unconsumed events, leaving
> - * EV_SYN/SYN_DROPPED plus the newest event in the queue.
> + * EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
> + * newest event in the queue.
> */
> - 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->head = (client->head - 1) & mask;
> + client->packet_head = client->tail = client->head;
> + __evdev_queue_syn_dropped(client);
>
> - client->packet_head = client->tail;
> + client->buffer[client->head++] = *event;
> + client->head &= mask;
> + /* No need to check for buffer overflow as it just occurred */
> }
>
> if (event->type == EV_SYN && event->code == SYN_REPORT) {
> @@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
> int ret;
> unsigned long *mem;
> size_t len;
> + bool is_queue_empty;
>
> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> mem = kmalloc(len, GFP_KERNEL);
> @@ -933,12 +956,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
>
> spin_unlock(&dev->event_lock);
>
> + if (client->head == client->tail)
> + is_queue_empty = true;
> +
> __evdev_flush_queue(client, type);
>
> spin_unlock_irq(&client->buffer_lock);
>
> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> - if (ret < 0)
> + if (ret < 0 && !is_queue_empty)
> evdev_queue_syn_dropped(client);
>
> kfree(mem);
> --
> 2.6.2
>
Thanks.
--
Dmitry