Hello Mr. Torokhov, On 1/25/16, Aniroop Mathur <aniroop.mathur@xxxxxxxxx> wrote: > On Sun, Jan 24, 2016 at 1:35 AM, Aniroop Mathur > <aniroop.mathur@xxxxxxxxx> wrote: >> On Sun, Jan 24, 2016 at 12:09 AM, Dmitry Torokhov >> <dmitry.torokhov@xxxxxxxxx> wrote: >>> On Sat, Jan 23, 2016 at 11:29:29PM +0530, Aniroop Mathur wrote: >>>> Hi Mr. Torokhov, >>>> >>>> On Fri, Jan 22, 2016 at 12:47 AM, Dmitry Torokhov >>>> <dmitry.torokhov@xxxxxxxxx> wrote: >>>> > Hi Anoroop, >>>> > >>>> > On Thu, Jan 21, 2016 at 11:07:19PM +0530, Aniroop Mathur wrote: >>>> >> If last event dropped in the old queue was EVi_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 | 46 >>>> >> ++++++++++++++++++++++++++++++++++------------ >>>> >> 1 file changed, 34 insertions(+), 12 deletions(-) >>>> >> >>>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c >>>> >> index e9ae3d5..821b68a 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 have still the same comment. How do you know that event at last_ev >>>> > position is in fact valid and unconsumed yet event. Also, you need to >>>> > figure out not only if queue contains last SYN event, but also to >>>> > handle >>>> > the case where the queue is empty and client has consumed either full >>>> > or >>>> > partial packet at the time you are queueing the drop. >>>> > >>>> >>>> Could you please explain what you mean exactly so that I could answer >>>> it >>>> properly? >>>> >>>> From what I understood, it seems to me that there is no problem related >>>> to >>>> validity, unconsumption, empty queue, full/partial packet. >>>> I would like to explain for clock change request case so that you can >>>> know >>>> my understanding for your question. >>>> >>>> Clock change request case: >>>> >>>> 1.1 About last event being valid and unconsumed: >>>> We flush buffer and queue syn_dropped only when buffer is not empty. So >>>> there >>>> will be always be atleast one event in buffer that is not consumed and >>>> is >>>> ofcourse valid. >>>> >>>> 1.2 About queue is empty >>>> If not empty, we do not flush or add syn_dropped at all. >>> >>> Clock type change is not the only time we queue SYN_DROP, the other time >>> is when we fail to handle EVIOCG[type] (during which we remove some >>> events from the queue). Queue may be empty when these ioctls are issued. >>> >> >> yeah, ideally it should be changed to: >> ret = bits_to_user(mem, maxbit, maxlen, p, compat); >> if (ret < 0) >> + if (client->head != client->tail) >> - evdev_queue_syn_dropped(client); >> + evdev_queue_syn_dropped(client); >> >> Firstly, there is a need to follow protocol of SYN_REPORT as mentioned in >> next section. >> > > and yes, there is need to make many more changes to have correct > last_event for this case. But it seems really complex. > >> Unless syn_report does not denote end of a packet, >> it is okay without this change too because last event stored would be >> syn_report and after flushing, syn_report will still be at the end and >> with empty queue too if syn_dropped is queued then we have added >> syn_report >> to not ignore upcoming valid packet. >> >>>> >>>> 1.3 About consumption of full or partial packet >>>> If client has consumed full packet, then buffer will look like, >>>> ... X Y S(consumed) ... X Y S >>>> As we always store packets keeping buffer lock and not single events, so >>>> there >>>> will always be syn_report in the end. >>> >>> We try to pass full packets to clients, but there is no guarantee. We >>> only estimate number of events in device's packet, not guarantee that it >>> is correct size. >>> >> >> As per documentation, SYN_REPORT should be used to separate packets. >> * SYN_REPORT: >> - Used to synchronize and separate events into packets of input data >> changes >> occurring at the same moment in time. For example, motion of a mouse >> may set >> the REL_X and REL_Y values for one motion, then emit a SYN_REPORT. The >> next >> motion will emit more REL_X and REL_Y values and send another >> SYN_REPORT. >> >> So I think, there is a need of below change: >> file: input.c >> function: input_handle_event: >> } else if (dev->num_vals >= dev->max_vals - 2) { >> - dev->vals[dev->num_vals++] = input_value_sync; >> input_pass_values(dev, dev->vals, dev->num_vals); >> >> We have sufficient space in buffer to store more than 1 packet even when >> actual packet size is more than max_vals so there seems no need to add >> syn_report event here by self. So whenever driver sends syn_report, then >> only >> it will be considered as end of a packet and on exceding max_vals, we can >> simply pass to handlers to store those partial events in buffer. And >> anyways >> unless the packet is really completed then only client will send it to >> application. >> >> Without following this protocol, we would not be able to find the end of >> a >> packet because if max_vals comes out to 3 but actual packet size is 15, >> then >> in the buffer, there will be many syn_reports within a single packet. So >> with >> both current code and with patch code, there will be trouble in handling >> syn_dropped because after one syn_report comes, client will stop ignoring >> the >> events. >> > > How about above change and do you want me send separate patch for it? > Could you please update about above change as based on this change further changes can be decided. Thank you, Aniroop Mathur >>>> If syn_dropped is queued here, then queing syn_report is fine. >>>> If client has consumed partial packet, then buffer will look like, >>>> ... X(consumed) Y S ... S >>>> If syn_dropped is queued here, then it is fine to queue syn_report >>>> because >>>> now new X Y will be reported by driver, and so client will consume new X >>>> and Y >>>> and that old X will be replaced with new X and this new packet will be >>>> sent by >>>> client to application. Obviously, client never sends partial events to >>>> application and send data packet by packet. >>>> So I do not see any trouble here related to partial or full packet. >>>> >>>> > Also please enumerate what changes you done between version n and n+1 >>>> > so >>>> > I do not have to compare them line by line trying to figure it out >>>> > myself. >>>> > >>>> >>>> Difference from v5: >>>> Made a mistake about head index in v5. >>>> Corrected in v8. >>>> >>>> evdev_set_clk_type: >>>> - client->packet_head = client->head = client->tail; >>>> + client->packet_head = client->tail = client->head; >>> >>> Why does this matter? >>> >> >> We must not change the head index. So we need to make packet_head and >> tail >> index same as head index and not same as tail index. After this setting, >> we call evdev_queue_syn_dropped where we capture last event as the event >> present at (head - 1) index; >> >> Thanks, >> Aniroop Mathur >> >>> Thanks. >>> >>> -- >>> Dmitry > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html