Re: [PATCH v2 3/4] input: evdev: Indicate buffer overrun withSYN_DROPPED.

[Dmitry Torokhov]

Hi Jeff,

On Fri, Apr 01, 2011 at 11:54:18PM -0700, Jeff Brown wrote:
>  	/*
>  	 * Interrupts are disabled, just acquire the lock.
> -	 * Make sure we don't leave with the client buffer
> -	 * "empty" by having client->head == client->tail.
> +	 * When the client buffer is full, drain the buffer and enqueue a
> +	 * SYN_DROPPED event to let the client know that events were dropped
> +	 * and the last packet was incomplete.  We then consume all remaining
> +	 * events from the dropped packet until the next packet begins.

I do not think we (kernel) should be doing this. Clients should take
care and not allow overruns but if they happen the pain should be on the
client to restore the context. It has to query the device to get its
current state and it can drop the events till next sync as well.

Therefore I intend to apply the patch in the form below.

Thanks!

-- 
Dmitry


Input: evdev - indicate buffer overrun with SYN_DROPPED

From: Jeff Brown <jeffbrown@xxxxxxxxxx>

Add a new EV_SYN code, SYN_DROPPED, to inform the client when input
events have been dropped from the evdev input buffer due to a
buffer overrun.  The client should use this event as a hint to
reset its state or ignore all following events until the next
packet begins.

Signed-off-by: Jeff Brown <jeffbrown@xxxxxxxxxxx>
[dtor@xxxxxxx: Implement Henrik's suggestion and drop old events in
 case of overflow.]
Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---

 Documentation/input/event-codes.txt |    6 ++++++
 drivers/input/evdev.c               |   33 +++++++++++++++++++++------------
 include/linux/input.h               |    1 +
 3 files changed, 28 insertions(+), 12 deletions(-)


diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
index d6732a4..e0326a0 100644
--- a/Documentation/input/event-codes.txt
+++ b/Documentation/input/event-codes.txt
@@ -79,6 +79,12 @@ defined only by when they are sent in the evdev event stream.
   - Used to synchronize and separate touch events. See the
     multi-touch-protocol.txt document for more information.
 
+* SYN_DROPPED:
+  - Used to indicate buffer overrun in the evdev client's event queue.
+    Client should ignore all events up to and including next SYN_REPORT
+    event and query the device (using EVIOCG* ioctls) to obtain its
+    current state.
+
 EV_KEY:
 ----------
 EV_KEY events take the form KEY_<name> or BTN_<name>. For example, KEY_A is used
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 7f42d3a..f1c0910 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -39,13 +39,13 @@ struct evdev {
 };
 
 struct evdev_client {
-	int head;
-	int tail;
+	unsigned int head;
+	unsigned int tail;
 	spinlock_t buffer_lock; /* protects access to buffer, head and tail */
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
-	int bufsize;
+	unsigned int bufsize;
 	struct input_event buffer[];
 };
 
@@ -55,16 +55,25 @@ static DEFINE_MUTEX(evdev_table_mutex);
 static void evdev_pass_event(struct evdev_client *client,
 			     struct input_event *event)
 {
-	/*
-	 * Interrupts are disabled, just acquire the lock.
-	 * Make sure we don't leave with the client buffer
-	 * "empty" by having client->head == client->tail.
-	 */
+	/* Interrupts are disabled, just acquire the lock. */
 	spin_lock(&client->buffer_lock);
-	do {
-		client->buffer[client->head++] = *event;
-		client->head &= client->bufsize - 1;
-	} while (client->head == client->tail);
+
+	client->buffer[client->head++] = *event;
+	client->head &= client->bufsize - 1;
+
+	if (unlikely(client->head == client->tail)) {
+		/*
+		 * This effectively "drops" all unconsumed events, leaving
+		 * EV_SYN/SYN_DROPPED plus the 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;
+	}
+
 	spin_unlock(&client->buffer_lock);
 
 	if (event->type == EV_SYN)
diff --git a/include/linux/input.h b/include/linux/input.h
index f3a7794..71d3651 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -167,6 +167,7 @@ struct input_keymap_entry {
 #define SYN_REPORT		0
 #define SYN_CONFIG		1
 #define SYN_MT_REPORT		2
+#define SYN_DROPPED		3
 
 /*
  * Keys and buttons
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/