RE: Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event

From: Aniroop Mathur
Date: Mon Nov 21 2016 - 11:17:12 EST

Next message: Rob Herring: "Re: [PATCH V2 1/2] regulator: pwm: DT: Add ramp delay for exponential voltage transition"
Previous message: Jerome Brunet: "Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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Â<dmitry.torokhov@xxxxxxxxx>
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Â<a.mathur@xxxxxxxxxxx>
>Â
>Â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
Â
Â

Next message: Rob Herring: "Re: [PATCH V2 1/2] regulator: pwm: DT: Add ramp delay for exponential voltage transition"
Previous message: Jerome Brunet: "Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]