| Message ID | 1452686262-6684-1-git-send-email-a.mathur@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {
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(-)