Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757526AbcKDU6S (ORCPT ); Fri, 4 Nov 2016 16:58:18 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35798 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757335AbcKDU6Q (ORCPT ); Fri, 4 Nov 2016 16:58:16 -0400 MIME-Version: 1.0 In-Reply-To: References: <1475608376-3077-1-git-send-email-a.mathur@samsung.com> From: Aniroop Mathur Date: Sat, 5 Nov 2016 01:27:37 +0530 Message-ID: Subject: Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event To: Dmitry Torokhov , "linux-input@vger.kernel.org" , "linux-kernel@vger.kernel.org" Cc: Aniroop Mathur Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7313 Lines: 178 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 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 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 >>> >>> 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 >>>