Evdev Extensions

This moves basic checks and setup from uinput_setup_device() into uinput_validate_absbits() to make it easier to use. This way, we can call it from other places without copying the boilerplate code.

Reviewed-by Peter Hutterer <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/misc/uinput.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

drivers/input/misc/uinput.c +21 -19

index 8569362..615324c 100644
@@ -311,7 +311,13 @@ static int uinput_open(struct inode *inode, struct file *file)
 static int uinput_validate_absbits(struct input_dev *dev)
 {
 	unsigned int cnt;
-	int retval = 0;
+	int nslot;
+
+	if (!test_bit(EV_ABS, dev->evbit))
+		return 0;
+
+	/* check if absmin/absmax/absfuzz/absflat are filled as
+	 * told in Documentation/input/input-programming.txt */
 
 	for (cnt = 0; cnt < ABS_CNT; cnt++) {
 		int min, max;

@@ -327,8 +333,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
 				UINPUT_NAME, cnt,
 				input_abs_get_min(dev, cnt),
 				input_abs_get_max(dev, cnt));
-			retval = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
 		if (input_abs_get_flat(dev, cnt) >

@@ -340,11 +345,18 @@ static int uinput_validate_absbits(struct input_dev *dev)
 				input_abs_get_flat(dev, cnt),
 				input_abs_get_min(dev, cnt),
 				input_abs_get_max(dev, cnt));
-			retval = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 	}
-	return retval;
+
+	if (test_bit(ABS_MT_SLOT, dev->absbit)) {
+		nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
+		input_mt_init_slots(dev, nslot, 0);
+	} else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
+		input_set_events_per_packet(dev, 60);
+	}
+
+	return 0;
 }
 
 static int uinput_allocate_device(struct uinput_device *udev)

@@ -410,19 +422,9 @@ static int uinput_setup_device(struct uinput_device *udev,
 		input_abs_set_flat(dev, i, user_dev->absflat[i]);
 	}
 
-	/* check if absmin/absmax/absfuzz/absflat are filled as
-	 * told in Documentation/input/input-programming.txt */
-	if (test_bit(EV_ABS, dev->evbit)) {
-		retval = uinput_validate_absbits(dev);
-		if (retval < 0)
-			goto exit;
-		if (test_bit(ABS_MT_SLOT, dev->absbit)) {
-			int nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
-			input_mt_init_slots(dev, nslot, 0);
-		} else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
-			input_set_events_per_packet(dev, 60);
-		}
-	}
+	retval = uinput_validate_absbits(dev);
+	if (retval < 0)
+		goto exit;
 
 	udev->state = UIST_SETUP_COMPLETE;
 	retval = count;

Signature · 1 line

2.0.2

This ioctl is the counterpart to EVIOCGVERSION and returns the uinput-version the kernel was compiled with.

Reviewed-by Peter Hutterer <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/misc/uinput.c | 6 ++++++
 include/uapi/linux/uinput.h | 9 +++++++++
 2 files changed, 15 insertions(+)

drivers/input/misc/uinput.c +6 -0

index 615324c..a2a3895 100644
@@ -722,6 +722,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	}
 
 	switch (cmd) {
+		case UI_GET_VERSION:
+			if (put_user(UINPUT_VERSION,
+				     (unsigned int __user*)p))
+				retval = -EFAULT;
+			goto out;
+
 		case UI_DEV_CREATE:
 			retval = uinput_create_device(udev);
 			goto out;

include/uapi/linux/uinput.h +9 -0

index 0389b48..19339cf 100644
@@ -84,6 +84,15 @@ struct uinput_ff_erase {
  */
 #define UI_GET_SYSNAME(len)	_IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
 
+/**
+ * UI_GET_VERSION - Return uinput version of the kernel
+ *
+ * This writes the kernel's uinput version into the integer pointed to by the
+ * ioctl argument. The uinput-version is hard-coded in the kernel and
+ * independent of the uinput device.
+ */
+#define UI_GET_VERSION _IOR(UINPUT_IOCTL_BASE, 301, unsigned int)
+
 /*
  * To write a force-feedback-capable driver, the upload_effect
  * and erase_effect callbacks in input_dev must be implemented.

Signature · 1 line

2.0.2

This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup method (by write()'ing "struct uinput_user_dev" to the node). The old method is not easily extendable and requires huge payloads. Furthermore, overloading write() without properly versioned objects is error-prone.

Therefore, we introduce a new ioctl to replace the old method. The ioctl supports all features of the old method, plus a "resolution" field for absinfo. Furthermore, it's properly forward-compatible to new ABS codes and a growing "struct input_absinfo" structure.

The ioctl also allows user-space to skip unknown axes if not set. The payload-size can now be specified by the caller. There is no need to copy the whole array temporarily into the kernel, but instead we can iterate over it and copy each value manually.

Reviewed-by Peter Hutterer <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
 2 files changed, 118 insertions(+), 6 deletions(-)

drivers/input/misc/uinput.c +7 -2

index a2a3895..0f45595 100644
@@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
 	return 0;
 }
 
-static int uinput_setup_device(struct uinput_device *udev,
-			       const char __user *buffer, size_t count)
+static int uinput_dev_setup(struct uinput_device *udev,
+			    struct uinput_setup __user *arg)
+{
+	struct uinput_setup setup;
+	struct input_dev *dev;
+	int i, retval;
+

Hi David,

Quoted 39 lines · David Herrmann

This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup method (by write()'ing "struct uinput_user_dev" to the node). The old method is not easily extendable and requires huge payloads. Furthermore, overloading write() without properly versioned objects is error-prone.

Therefore, we introduce a new ioctl to replace the old method. The ioctl supports all features of the old method, plus a "resolution" field for absinfo. Furthermore, it's properly forward-compatible to new ABS codes and a growing "struct input_absinfo" structure.

The ioctl also allows user-space to skip unknown axes if not set. The payload-size can now be specified by the caller. There is no need to copy the whole array temporarily into the kernel, but instead we can iterate over it and copy each value manually.

Reviewed-by Peter Hutterer <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
 2 files changed, 118 insertions(+), 6 deletions(-)

drivers/input/misc/uinput.c +7 -2

index a2a3895..0f45595 100644
@@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
      return 0;
 }
 
-static int uinput_setup_device(struct uinput_device *udev,
-                            const char __user *buffer, size_t count)
+static int uinput_dev_setup(struct uinput_device *udev,
+                         struct uinput_setup __user *arg)
+{
+     struct uinput_setup setup;
+     struct input_dev *dev;
+     int i, retval;
+

drivers/input/misc/uinput.c static int uinput_allocate_device(struct uinput_device *udev) · line 390

Quoted 10 lines

drivers/input/misc/uinput.c +10 -0

+     if (udev->state == UIST_CREATED)
+             return -EINVAL;
+     if (copy_from_user(&setup, arg, sizeof(setup)))
+             return -EFAULT;
+     if (!setup.name[0])
+             return -EINVAL;
+     /* So far we only support the original "struct input_absinfo", but be
+      * forward compatible and allow larger payloads. */
+     if (setup.absinfo_size < sizeof(struct input_absinfo))
+             return -EINVAL;

No, we can not do this, as it breaks backward compatibility (the most important one!). If we were to increase size of in-kernel input_absinfo in let's say 3.20, userspace compiled against older kernel headers (but using the new ioctl available let's say since 3.16 - don't hold me to the numbers ;) ), would break since it wold start tripping on thi check.

The proper way to handle it is to convert "old" absinfo into new one, applying as much as we can.

Hi

Quoted 60 lines · Dmitry Torokhov

Hi David,

Quoted 39 lines · David Herrmann

This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup method (by write()'ing "struct uinput_user_dev" to the node). The old method is not easily extendable and requires huge payloads. Furthermore, overloading write() without properly versioned objects is error-prone.

Therefore, we introduce a new ioctl to replace the old method. The ioctl supports all features of the old method, plus a "resolution" field for absinfo. Furthermore, it's properly forward-compatible to new ABS codes and a growing "struct input_absinfo" structure.

The ioctl also allows user-space to skip unknown axes if not set. The payload-size can now be specified by the caller. There is no need to copy the whole array temporarily into the kernel, but instead we can iterate over it and copy each value manually.

Reviewed-by Peter Hutterer <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
 2 files changed, 118 insertions(+), 6 deletions(-)

drivers/input/misc/uinput.c +7 -2

index a2a3895..0f45595 100644
@@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
      return 0;
 }
 
-static int uinput_setup_device(struct uinput_device *udev,
-                            const char __user *buffer, size_t count)
+static int uinput_dev_setup(struct uinput_device *udev,
+                         struct uinput_setup __user *arg)
+{
+     struct uinput_setup setup;
+     struct input_dev *dev;
+     int i, retval;
+

drivers/input/misc/uinput.c static int uinput_allocate_device(struct uinput_device *udev) · line 390

Quoted 10 lines

drivers/input/misc/uinput.c +10 -0

+     if (udev->state == UIST_CREATED)
+             return -EINVAL;
+     if (copy_from_user(&setup, arg, sizeof(setup)))
+             return -EFAULT;
+     if (!setup.name[0])
+             return -EINVAL;
+     /* So far we only support the original "struct input_absinfo", but be
+      * forward compatible and allow larger payloads. */
+     if (setup.absinfo_size < sizeof(struct input_absinfo))
+             return -EINVAL;

No, we can not do this, as it breaks backward compatibility (the most important one!). If we were to increase size of in-kernel input_absinfo in let's say 3.20, userspace compiled against older kernel headers (but using the new ioctl available let's say since 3.16 - don't hold me to the numbers ;) ), would break since it wold start tripping on thi check.

Quoted 2 lines

The proper way to handle it is to convert "old" absinfo into new one, applying as much as we can.

I know, but there is no "old absinfo". Once we extend "struct absinfo" I expect this code to change to:

{
        /* initially supported absinfo had size 24 */
        if (setup.absinfo_size < 24)
               return -EINVAL;

        /* ...pseudo code... */
        memset(&absinfo, 0, sizeof(absinfo));
        memcpy(&absinfo, sth->absinfo, MIN(setup.absinfo_size,

sizeof(absinfo)));
}

This allows you to use this ioctl with old absinfo objects. I can change the current code to this already, if you want? I tried to avoid it, because a memset() is not neccessarily an appropriate way to initialize unset fields.
I cal also add support for "absinfo" without the "resolution" field, which I think is the only field that wasn't available in the initial structure.

Quoted 14 lines · David Herrmann

I know, but there is no "old absinfo". Once we extend "struct absinfo" I expect this code to change to:

{
        /* initially supported absinfo had size 24 */
        if (setup.absinfo_size < 24)
               return -EINVAL;

        /* ...pseudo code... */
        memset(&absinfo, 0, sizeof(absinfo));
        memcpy(&absinfo, sth->absinfo, MIN(setup.absinfo_size,

sizeof(absinfo)));
}

Quoted 7 lines

This allows you to use this ioctl with old absinfo objects. I can change the current code to this already, if you want? I tried to avoid it, because a memset() is not neccessarily an appropriate way to initialize unset fields.
I cal also add support for "absinfo" without the "resolution" field, which I think is the only field that wasn't available in the initial structure.

I think for now I would do:

        /*
         * Whoever is changing struct input_absinfo will have to take
         * care of backwards compatibility.
         */
        BUILD_BUG_ON(sizeof(struct input_absinfo)) != 24);
        if (setup.absinfo_size != sizeof(struct input_absinfo))
                return -EINVAL;

        ...

and later when we detect setup.absinfo_size < sizeof(struct input_absinfo) we'll have to take care about backwards compatibility. We do not need to take care of forward compatibility as we do not know if userspace will be satisfied with partial results or not and newer userspace can deal with proper handling of older kernels.

By the way, I realize I do not like the new IOCTL as it is - it's too big and would be hard to extend if we want to change items other than absinfo. Why don't we create UI_DEV_SETUP that only sets name, id, and number of effects, and add UI_SET_ABSAXIS that would take:

        struct uinput_abs_setup {
                __u16   code;   /* axis code */
                /* __u16 filler; */
                struct input_absinfo absinfo;
        }

By the way, while you are hacking on uinput can we also fix formatting style of switch/case and switch printk() to pr_debug() and friends? I'd do it myself but do not want to step on your toes.

When we introduced the slotted MT ABS extensions, we didn't take care to make SYN_DROPPED recoverable. Imagine a client recevies a SYN_DROPPED and syncs its current state via EVIOCGABS. It has to call this ioctl for each and every ABS code separately. Besides being horribly slow, this series of ioctl-calls is not atomic. The kernel might queue new ABS events while the client fetches the data.

Now for normal ABS codes this is negligible as ABS values provide absolute data. That is, there is usually no need to resync ABS codes as we don't need previous values to interpret the next ABS code. Furthermore, most ABS codes are also sent pretty frequently so a refresh is usually useless.

However, with the introduction of slotted ABS axes we added a relative
component: ABS_MT_SLOT. If a client syncs all ABS codes via EVIOCGABS while the kernel has ABS-events and an ABS_MT_SLOT event queued, the client will think any read ABS-event is for the retrieved SLOT, however, this is not true as all events until the next ABS_MT_SLOT event are for the previously active slot:

Kernel queue is: { ABS_DROPPED,

                       ABS_MT_POSITION_X(slot: 0),
                       ABS_MT_SLOT(slot: 1),
                       ABS_MT_POSITION_X(slot: 1) }
    Client reads ABS_DROPPED from queue.

Client syncs all ABS values:
As part of that, client syncs ABS_MT_SLOT, which is 1 in the current view of the kernel.
Client reads ABS_MT_POSITION_X and attributes it to slot 1 instead of slot 0, as the slot-value is not explicit.

This is just a simple example how the relative information provided by the ABS_MT_SLOT axis can be problematic to clients.

Now there are many ways to fix this:
* Make ABS_MT_SLOT a per-evdev-client attribute. On each EVIOCGABS(ABS_MT_SLOT) we can add fake ABS_MT_SLOT events to the queue. => Ugly and overkill
* Flush all ABS events when clients read ABS_MT_SLOT.
=> Ugly hack and client might loose important ABS_MT_* events
* Provide atomic EVIOCGABS API.
=> Sounds good!

This patch introduces EVIOCGABSRANGE. Unlike EVIOCGABS, this ioctl only fetches ABS values, rather than the whole "struct input_absinfo" set. However, the new ioctl can fetch a range of ABS axes atomically and will flush matching events from the client's receive queue. Moreover, you can fetch all axes for *all* slots with a single call.

This way, a client can simply run EVIOCGABSRANGE(0, ABS_CNT) and it will receive a consistent view of the whole ABS state, while the kernel flushes the receive-buffer for a consistent view.
While most clients probably only need EVIOCGABSRANGE(ABS_MT_SLOT, ABS_MT_TOOL_y - ABS_MT_SLOT + 1), the ioctl allows to receive an arbitrary range of axes.

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/evdev.c      | 180 ++++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/input.h |  44 ++++++++++-
 2 files changed, 219 insertions(+), 5 deletions(-)

drivers/input/evdev.c +176 -4

index 6386882..7a25a7a 100644
@@ -175,8 +175,9 @@ static bool __evdev_is_filtered(struct evdev_client *client,
 	return mask && !test_bit(code, mask);
 }
 
-/* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+/* flush queued, matching events, caller must hold client->buffer_lock */
+static void __evdev_flush_queue(struct evdev_client *client, unsigned int type,
+				unsigned int code_first, unsigned int code_last)
 {
 	unsigned int i, head, num;
 	unsigned int mask = client->bufsize - 1;

@@ -195,7 +196,9 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 		ev = &client->buffer[i];
 		is_report = ev->type == EV_SYN && ev->code == SYN_REPORT;
 
-		if (ev->type == type) {
+		if (ev->type == type &&
+		    ev->code >= code_first &&
+		    ev->code <= code_last) {
 			/* drop matched entry */
 			continue;
 		} else if (is_report && !num) {

@@ -786,6 +789,172 @@ static int handle_eviocgbit(struct input_dev *dev,
 	return bits_to_user(bits, len, size, p, compat_mode);
 }
 
+static inline void free_absrange(s32 **pages, size_t page_cnt)
+{
+	if (page_cnt > 1) {
+		while (page_cnt > 0) {
+			if (!pages[--page_cnt])
+				break;
+			__free_page(virt_to_page(pages[page_cnt]));
+		}
+		kfree(pages);
+	} else if (page_cnt == 1) {
+		kfree(pages);
+	}
+}
+
+static inline s32 *absrange_ptr(s32 **pages, size_t page_cnt, size_t slots,
+				size_t i_code, size_t j_slot)
+{
+	size_t idx, off;
+
+	idx = (i_code * slots + j_slot) / (PAGE_SIZE / sizeof(s32));
+	off = (i_code * slots + j_slot) % (PAGE_SIZE / sizeof(s32));
+
+	if (page_cnt == 1)
+		return &((s32*)pages)[off];
+	else
+		return &pages[idx][off];
+}
+
+static inline ssize_t fetch_absrange(struct evdev_client *client,
+				     struct input_dev *dev, size_t start,
+				     size_t count, size_t slots, s32 ***out)
+{
+	size_t size, page_cnt, i, j;
+	unsigned long flags;
+	s32 **pages;
+
+	/*
+	 * Fetch data atomically from the device and flush buffers. We need to
+	 * allocate a temporary buffer as copy_to_user() is not allowed while
+	 * holding spinlocks. However, to-be-copied data might be huge and
+	 * high-order allocations should be avoided. Therefore, do the
+	 * page-allocation manually.
+	 */
+
+	BUILD_BUG_ON(PAGE_SIZE % sizeof(s32) != 0);
+
+	size = sizeof(s32) * count * slots;
+	page_cnt = DIV_ROUND_UP(size, PAGE_SIZE);
+	if (page_cnt < 1) {
+		return 0;
+	} else if (page_cnt == 1) {
+		pages = kzalloc(size, GFP_TEMPORARY);
+		if (!pages)
+			return -ENOMEM;
+	} else {
+		pages = kzalloc(sizeof(*pages) * page_cnt, GFP_TEMPORARY);
+		if (!pages)
+			return -ENOMEM;
+
+		for (i = 0; i < page_cnt; ++i) {
+			pages[i] = (void*)get_zeroed_page(GFP_TEMPORARY);
+			if (!pages[i]) {
+				free_absrange(pages, page_cnt);
+				return -ENOMEM;
+			}
+		}
+	}
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	spin_lock(&client->buffer_lock);
+
+	for (i = 0; i < count; ++i) {
+		__u16 code;
+		bool is_mt;
+
+		code = start + i;
+		is_mt = input_is_mt_value(code);
+		if (is_mt && !dev->mt)
+			continue;
+
+		for (j = 0; j < slots; ++j) {
+			__s32 v;
+
+			if (is_mt)
+				v = input_mt_get_value(&dev->mt->slots[j],
+						       code);
+			else
+				v = dev->absinfo[code].value;
+
+			*absrange_ptr(pages, page_cnt, slots, i, j) = v;
+
+			if (!is_mt)
+				break;
+		}
+	}
+
+	spin_unlock(&client->buffer_lock);
+	__evdev_flush_queue(client, EV_ABS, start, start + count - 1);
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	*out = pages;
+	return page_cnt;
+}
+
+static int evdev_handle_get_absrange(struct evdev_client *client,
+				     struct input_dev *dev,
+				     struct input_absrange __user *p)
+{
+	size_t slots, code, count, i, j;
+	struct input_absrange absbuf;
+	s32 **vals = NULL;
+	ssize_t val_cnt;
+	s32 __user *b;
+	int retval;
+
+	if (!dev->absinfo)
+		return -EINVAL;
+	if (copy_from_user(&absbuf, p, sizeof(absbuf)))
+		return -EFAULT;
+
+	slots = min_t(size_t, dev->mt ? dev->mt->num_slots : 1, absbuf.slots);
+	code = min_t(size_t, absbuf.code, ABS_CNT);
+	count = min_t(size_t, absbuf.count, ABS_CNT);
+
+	/* first fetch data atomically from device */
+
+	if (code + count > ABS_CNT)
+		count = ABS_CNT - code;
+
+	if (!slots || !count) {
+		val_cnt = 0;
+	} else {
+		val_cnt = fetch_absrange(client, dev, code, count,
+					 slots, &vals);
+		if (val_cnt < 0)
+			return val_cnt;
+	}
+
+	/* now copy data to user-space */
+
+	b = (void __user*)(unsigned long)absbuf.buffer;
+	for (i = 0; i < absbuf.count; ++i) {
+		for (j = 0; j < absbuf.slots; ++j, ++b) {
+			s32 v;
+
+			if (i >= count || j >= slots)
+				v = 0;
+			else
+				v = *absrange_ptr(vals, val_cnt, slots, i, j);
+
+			if (put_user(v, b)) {
+				retval = -EFAULT;
+				goto out;
+			}
+		}
+	}
+
+	retval = 0;
+
+out:
+	free_absrange(vals, val_cnt);
+	if (retval < 0)
+		evdev_queue_syn_dropped(client);
+	return retval;
+}
+
 static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
 {
 	struct input_keymap_entry ke = {

@@ -889,7 +1058,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 
 	spin_unlock(&dev->event_lock);
 
-	__evdev_flush_queue(client, type);
+	__evdev_flush_queue(client, type, 0, UINT_MAX);
 
 	spin_unlock_irq(&client->buffer_lock);
 

@@ -1006,6 +1175,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_revoke(evdev, client, file);
 
+	case EVIOCGABSRANGE:
+		return evdev_handle_get_absrange(client, dev, p);
+
 	case EVIOCGMASK:
 		if (copy_from_user(&mask, p, sizeof(mask)))
 			return -EFAULT;

include/uapi/linux/input.h +11 -1

index f6ace0e..9f851d4 100644
@@ -32,7 +32,7 @@ struct input_event {
  * Protocol version.
  */
 
-#define EV_VERSION		0x010001
+#define EV_VERSION		0x010002
 
 /*
  * IOCTLs (0x00 - 0x7f)

@@ -210,6 +210,48 @@ struct input_mask {
  */
 #define EVIOCSMASK		_IOW('E', 0x93, struct input_mask)	/* Set event-masks */
 
+struct input_absrange {
+	__u16 slots;
+	__u16 code;
+	__u32 count;
+	__u64 buffer;
+};
+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *

include/uapi/linux/input.h +18 -0

+ * the client's receive buffer. But unlike EVIOCGABS, this ioctl only returns
+ * the current value of an axis, rather than the whole "struct input_absinfo"
+ * set. All fields of "struct input_absinfo" except for the value are constant,
+ * though.
+ *
+ * The kernel's current view of the ABS axes is copied into the provided buffer.
+ * If an ABS axis is not enabled on the device, its value will be zero. Also, if
+ * an axis is not a slotted MT-axis, values for all but the first slot will be
+ * 0. If @slots is greater than the actual number of slots provided by the
+ * device, values for all slots higher than that will be 0.
+ *
+ * This call may fail with -EINVAL if the kernel doesn't support this call or
+ * the arguments are invalid, with -ENODEV if access was revoked, -ENOMEM if the
+ * kernel couldn't allocate temporary buffers for data-copy or -EFAULT if the
+ * passed pointer was invalid.
+ */
+#define EVIOCGABSRANGE		_IOR('E', 0x94, struct input_absrange)
+
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
 /*

Signature · 1 line

2.0.2

sorry for the late comments, not sure how that slipped through but it hadn't shown up in my inbox unil Benjamin poked me.

Quoted 19 lines · David Herrmann

When we introduced the slotted MT ABS extensions, we didn't take care to make SYN_DROPPED recoverable. Imagine a client recevies a SYN_DROPPED and syncs its current state via EVIOCGABS. It has to call this ioctl for each and every ABS code separately. Besides being horribly slow, this series of ioctl-calls is not atomic. The kernel might queue new ABS events while the client fetches the data.

Now for normal ABS codes this is negligible as ABS values provide absolute data. That is, there is usually no need to resync ABS codes as we don't need previous values to interpret the next ABS code. Furthermore, most ABS codes are also sent pretty frequently so a refresh is usually useless.

However, with the introduction of slotted ABS axes we added a relative
component: ABS_MT_SLOT. If a client syncs all ABS codes via EVIOCGABS while the kernel has ABS-events and an ABS_MT_SLOT event queued, the client will think any read ABS-event is for the retrieved SLOT, however, this is not true as all events until the next ABS_MT_SLOT event are for the previously active slot:

Quoted 1 line

Kernel queue is: { ABS_DROPPED,

shouldn't this be SYN_DROPPED?

Quoted 4 lines

                       ABS_MT_POSITION_X(slot: 0),
                       ABS_MT_SLOT(slot: 1),
                       ABS_MT_POSITION_X(slot: 1) }
    Client reads ABS_DROPPED from queue.

here too

Quoted 283 lines

Client syncs all ABS values:
As part of that, client syncs ABS_MT_SLOT, which is 1 in the current view of the kernel.
Client reads ABS_MT_POSITION_X and attributes it to slot 1 instead of slot 0, as the slot-value is not explicit.

This is just a simple example how the relative information provided by the ABS_MT_SLOT axis can be problematic to clients.

Now there are many ways to fix this:
* Make ABS_MT_SLOT a per-evdev-client attribute. On each EVIOCGABS(ABS_MT_SLOT) we can add fake ABS_MT_SLOT events to the queue. => Ugly and overkill
* Flush all ABS events when clients read ABS_MT_SLOT.
=> Ugly hack and client might loose important ABS_MT_* events
* Provide atomic EVIOCGABS API.
=> Sounds good!

This patch introduces EVIOCGABSRANGE. Unlike EVIOCGABS, this ioctl only fetches ABS values, rather than the whole "struct input_absinfo" set. However, the new ioctl can fetch a range of ABS axes atomically and will flush matching events from the client's receive queue. Moreover, you can fetch all axes for *all* slots with a single call.

This way, a client can simply run EVIOCGABSRANGE(0, ABS_CNT) and it will receive a consistent view of the whole ABS state, while the kernel flushes the receive-buffer for a consistent view.
While most clients probably only need EVIOCGABSRANGE(ABS_MT_SLOT, ABS_MT_TOOL_y - ABS_MT_SLOT + 1), the ioctl allows to receive an arbitrary range of axes.

Signed-off-by David Herrmann <[email protected]>

---
 drivers/input/evdev.c      | 180 ++++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/input.h |  44 ++++++++++-
 2 files changed, 219 insertions(+), 5 deletions(-)

drivers/input/evdev.c +176 -4

index 6386882..7a25a7a 100644
@@ -175,8 +175,9 @@ static bool __evdev_is_filtered(struct evdev_client *client,
      return mask && !test_bit(code, mask);
 }
 
-/* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+/* flush queued, matching events, caller must hold client->buffer_lock */
+static void __evdev_flush_queue(struct evdev_client *client, unsigned int type,
+                             unsigned int code_first, unsigned int code_last)
 {
      unsigned int i, head, num;
      unsigned int mask = client->bufsize - 1;

@@ -195,7 +196,9 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
              ev = &client->buffer[i];
              is_report = ev->type == EV_SYN && ev->code == SYN_REPORT;
 
-             if (ev->type == type) {
+             if (ev->type == type &&
+                 ev->code >= code_first &&
+                 ev->code <= code_last) {
                      /* drop matched entry */
                      continue;
              } else if (is_report && !num) {

@@ -786,6 +789,172 @@ static int handle_eviocgbit(struct input_dev *dev,
      return bits_to_user(bits, len, size, p, compat_mode);
 }
 
+static inline void free_absrange(s32 **pages, size_t page_cnt)
+{
+     if (page_cnt > 1) {
+             while (page_cnt > 0) {
+                     if (!pages[--page_cnt])
+                             break;
+                     __free_page(virt_to_page(pages[page_cnt]));
+             }
+             kfree(pages);
+     } else if (page_cnt == 1) {
+             kfree(pages);
+     }
+}
+
+static inline s32 *absrange_ptr(s32 **pages, size_t page_cnt, size_t slots,
+                             size_t i_code, size_t j_slot)
+{
+     size_t idx, off;
+
+     idx = (i_code * slots + j_slot) / (PAGE_SIZE / sizeof(s32));
+     off = (i_code * slots + j_slot) % (PAGE_SIZE / sizeof(s32));
+
+     if (page_cnt == 1)
+             return &((s32*)pages)[off];
+     else
+             return &pages[idx][off];
+}
+
+static inline ssize_t fetch_absrange(struct evdev_client *client,
+                                  struct input_dev *dev, size_t start,
+                                  size_t count, size_t slots, s32 ***out)
+{
+     size_t size, page_cnt, i, j;
+     unsigned long flags;
+     s32 **pages;
+
+     /*
+      * Fetch data atomically from the device and flush buffers. We need to
+      * allocate a temporary buffer as copy_to_user() is not allowed while
+      * holding spinlocks. However, to-be-copied data might be huge and
+      * high-order allocations should be avoided. Therefore, do the
+      * page-allocation manually.
+      */
+
+     BUILD_BUG_ON(PAGE_SIZE % sizeof(s32) != 0);
+
+     size = sizeof(s32) * count * slots;
+     page_cnt = DIV_ROUND_UP(size, PAGE_SIZE);
+     if (page_cnt < 1) {
+             return 0;
+     } else if (page_cnt == 1) {
+             pages = kzalloc(size, GFP_TEMPORARY);
+             if (!pages)
+                     return -ENOMEM;
+     } else {
+             pages = kzalloc(sizeof(*pages) * page_cnt, GFP_TEMPORARY);
+             if (!pages)
+                     return -ENOMEM;
+
+             for (i = 0; i < page_cnt; ++i) {
+                     pages[i] = (void*)get_zeroed_page(GFP_TEMPORARY);
+                     if (!pages[i]) {
+                             free_absrange(pages, page_cnt);
+                             return -ENOMEM;
+                     }
+             }
+     }
+
+     spin_lock_irqsave(&dev->event_lock, flags);
+     spin_lock(&client->buffer_lock);
+
+     for (i = 0; i < count; ++i) {
+             __u16 code;
+             bool is_mt;
+
+             code = start + i;
+             is_mt = input_is_mt_value(code);
+             if (is_mt && !dev->mt)
+                     continue;
+
+             for (j = 0; j < slots; ++j) {
+                     __s32 v;
+
+                     if (is_mt)
+                             v = input_mt_get_value(&dev->mt->slots[j],
+                                                    code);
+                     else
+                             v = dev->absinfo[code].value;
+
+                     *absrange_ptr(pages, page_cnt, slots, i, j) = v;
+
+                     if (!is_mt)
+                             break;
+             }
+     }
+
+     spin_unlock(&client->buffer_lock);
+     __evdev_flush_queue(client, EV_ABS, start, start + count - 1);
+     spin_unlock_irqrestore(&dev->event_lock, flags);
+
+     *out = pages;
+     return page_cnt;
+}
+
+static int evdev_handle_get_absrange(struct evdev_client *client,
+                                  struct input_dev *dev,
+                                  struct input_absrange __user *p)
+{
+     size_t slots, code, count, i, j;
+     struct input_absrange absbuf;
+     s32 **vals = NULL;
+     ssize_t val_cnt;
+     s32 __user *b;
+     int retval;
+
+     if (!dev->absinfo)
+             return -EINVAL;
+     if (copy_from_user(&absbuf, p, sizeof(absbuf)))
+             return -EFAULT;
+
+     slots = min_t(size_t, dev->mt ? dev->mt->num_slots : 1, absbuf.slots);
+     code = min_t(size_t, absbuf.code, ABS_CNT);
+     count = min_t(size_t, absbuf.count, ABS_CNT);
+
+     /* first fetch data atomically from device */
+
+     if (code + count > ABS_CNT)
+             count = ABS_CNT - code;
+
+     if (!slots || !count) {
+             val_cnt = 0;
+     } else {
+             val_cnt = fetch_absrange(client, dev, code, count,
+                                      slots, &vals);
+             if (val_cnt < 0)
+                     return val_cnt;
+     }
+
+     /* now copy data to user-space */
+
+     b = (void __user*)(unsigned long)absbuf.buffer;
+     for (i = 0; i < absbuf.count; ++i) {
+             for (j = 0; j < absbuf.slots; ++j, ++b) {
+                     s32 v;
+
+                     if (i >= count || j >= slots)
+                             v = 0;
+                     else
+                             v = *absrange_ptr(vals, val_cnt, slots, i, j);
+
+                     if (put_user(v, b)) {
+                             retval = -EFAULT;
+                             goto out;
+                     }
+             }
+     }
+
+     retval = 0;
+
+out:
+     free_absrange(vals, val_cnt);
+     if (retval < 0)
+             evdev_queue_syn_dropped(client);
+     return retval;
+}
+
 static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
 {
      struct input_keymap_entry ke = {

@@ -889,7 +1058,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 
      spin_unlock(&dev->event_lock);
 
-     __evdev_flush_queue(client, type);
+     __evdev_flush_queue(client, type, 0, UINT_MAX);
 
      spin_unlock_irq(&client->buffer_lock);
 

@@ -1006,6 +1175,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
              else
                      return evdev_revoke(evdev, client, file);
 
+     case EVIOCGABSRANGE:
+             return evdev_handle_get_absrange(client, dev, p);
+
      case EVIOCGMASK:
              if (copy_from_user(&mask, p, sizeof(mask)))
                      return -EFAULT;

include/uapi/linux/input.h +11 -1

index f6ace0e..9f851d4 100644
@@ -32,7 +32,7 @@ struct input_event {
  * Protocol version.
  */
 
-#define EV_VERSION           0x010001
+#define EV_VERSION           0x010002
 
 /*
  * IOCTLs (0x00 - 0x7f)

@@ -210,6 +210,48 @@ struct input_mask {
  */
 #define EVIOCSMASK           _IOW('E', 0x93, struct input_mask)      /* Set event-masks */
 
+struct input_absrange {
+     __u16 slots;
+     __u16 code;
+     __u32 count;
+     __u64 buffer;
+};
+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *

include/uapi/linux/input.h struct input_mask { · line 232

Quoted 10 lines

include/uapi/linux/input.h +10 -0

+ * This fetches the current values of a range of ABS codes atomically. The range
+ * of codes to fetch and the buffer-types are passed as "struct input_absrange",
+ * which has the following fields:
+ *      slots: Number of MT slots to fetch data for.
+ *       code: First ABS axis to query.
+ *      count: Number of ABS axes to query starting at @code.
+ *     buffer: Pointer to a receive buffer where to store the fetched ABS
+ *             values. This buffer must be an array of __s32 with at least
+ *             (@slots * @code) elements. The buffer is interpreted as two
+ *             dimensional __s32 array, declared as: __s32[slots][codes]

tbh this seems more complicated than necessary. Have you thought about just dumping the events into the client buffer as if they came fresh in from the device? So to sync, the client calls the ioctl with a buffer and a buffer size, and the kernel simply writes a series of struct input_events into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?) followed by a SYN_DROPPED. So the buffer afterwards could look like this:
EV_ABS ABS_X 30
EV_ABS ABS_X 1202
EV_ABS ABS_MT_SLOT 0
EV_ABS ABS_MT_POSITION_X 30
EV_ABS ABS_MT_POSITION_Y 1202
EV_ABS ABS_MT_SLOT 1
EV_ABS ABS_MT_POSITION_X 80
EV_ABS ABS_MT_POSITION_Y 1800
EV_SYN SYN_REPORT 0

the client can then go through and just process the events on-by-one as it would otherwise with real events.

This approach could be even extended to include EV_KEY, etc. providing a single ioctl to sync the whole state of the device atomically.

comments?

Hi

Quoted 38 lines · Peter Hutterer

Quoted 4 lines

include/uapi/linux/input.h +4 -0

+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *

include/uapi/linux/input.h struct input_mask { · line 232

Quoted 10 lines

include/uapi/linux/input.h +10 -0

+ * This fetches the current values of a range of ABS codes atomically. The range
+ * of codes to fetch and the buffer-types are passed as "struct input_absrange",
+ * which has the following fields:
+ *      slots: Number of MT slots to fetch data for.
+ *       code: First ABS axis to query.
+ *      count: Number of ABS axes to query starting at @code.
+ *     buffer: Pointer to a receive buffer where to store the fetched ABS
+ *             values. This buffer must be an array of __s32 with at least
+ *             (@slots * @code) elements. The buffer is interpreted as two
+ *             dimensional __s32 array, declared as: __s32[slots][codes]

tbh this seems more complicated than necessary. Have you thought about just dumping the events into the client buffer as if they came fresh in from the device? So to sync, the client calls the ioctl with a buffer and a buffer size, and the kernel simply writes a series of struct input_events into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?) followed by a SYN_DROPPED. So the buffer afterwards could look like this:
EV_ABS ABS_X 30
EV_ABS ABS_X 1202
EV_ABS ABS_MT_SLOT 0
EV_ABS ABS_MT_POSITION_X 30
EV_ABS ABS_MT_POSITION_Y 1202
EV_ABS ABS_MT_SLOT 1
EV_ABS ABS_MT_POSITION_X 80
EV_ABS ABS_MT_POSITION_Y 1800
EV_SYN SYN_REPORT 0

the client can then go through and just process the events on-by-one as it would otherwise with real events.

This approach could be even extended to include EV_KEY, etc. providing a single ioctl to sync the whole state of the device atomically.

comments?

So you mean instead of passing a __32 array we pass a "struct input_event" array and write it there? So bypassing the receive-queue? That does sound quite nice, indeed. We could replace all the other "sync" ioctls and just provide a way to receive all the events directly.

Something like:

EVIOCQUERY(struct input_query)

struct input_query {
        __u16 type;
        __u16 start_code;
        __u16 end_code;
        __u16 slots;

        struct input_event buffer[];
};

Quoted 41 lines · David Herrmann

Hi

Quoted 38 lines · Peter Hutterer

Quoted 4 lines

include/uapi/linux/input.h +4 -0

+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *

include/uapi/linux/input.h struct input_mask { · line 232

Quoted 10 lines

include/uapi/linux/input.h +10 -0

+ * This fetches the current values of a range of ABS codes atomically. The range
+ * of codes to fetch and the buffer-types are passed as "struct input_absrange",
+ * which has the following fields:
+ *      slots: Number of MT slots to fetch data for.
+ *       code: First ABS axis to query.
+ *      count: Number of ABS axes to query starting at @code.
+ *     buffer: Pointer to a receive buffer where to store the fetched ABS
+ *             values. This buffer must be an array of __s32 with at least
+ *             (@slots * @code) elements. The buffer is interpreted as two
+ *             dimensional __s32 array, declared as: __s32[slots][codes]

tbh this seems more complicated than necessary. Have you thought about just dumping the events into the client buffer as if they came fresh in from the device? So to sync, the client calls the ioctl with a buffer and a buffer size, and the kernel simply writes a series of struct input_events into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?) followed by a SYN_DROPPED. So the buffer afterwards could look like this:
EV_ABS ABS_X 30
EV_ABS ABS_X 1202
EV_ABS ABS_MT_SLOT 0
EV_ABS ABS_MT_POSITION_X 30
EV_ABS ABS_MT_POSITION_Y 1202
EV_ABS ABS_MT_SLOT 1
EV_ABS ABS_MT_POSITION_X 80
EV_ABS ABS_MT_POSITION_Y 1800
EV_SYN SYN_REPORT 0

the client can then go through and just process the events on-by-one as it would otherwise with real events.

This approach could be even extended to include EV_KEY, etc. providing a single ioctl to sync the whole state of the device atomically.

comments?

I like it.

Quoted 17 lines

So you mean instead of passing a __32 array we pass a "struct input_event" array and write it there? So bypassing the receive-queue? That does sound quite nice, indeed. We could replace all the other "sync" ioctls and just provide a way to receive all the events directly.

Something like:

EVIOCQUERY(struct input_query)

struct input_query {
        __u16 type;
        __u16 start_code;
        __u16 end_code;
        __u16 slots;

Quoted 2 lines

        struct input_event buffer[];
};

No, it is more like EVIOCRESYNC(void) which makes input core to dump all existing state into the client's standard event queue so that here is no need to reconcile/reconstruct anything. We could give a new SYN marker to indicate end-of-state boundary.

Hi

Quoted 62 lines · Dmitry Torokhov

Quoted 41 lines · David Herrmann

Hi

Quoted 38 lines · Peter Hutterer

Quoted 4 lines

include/uapi/linux/input.h +4 -0

+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *

include/uapi/linux/input.h struct input_mask { · line 232

Quoted 10 lines

include/uapi/linux/input.h +10 -0

+ * This fetches the current values of a range of ABS codes atomically. The range
+ * of codes to fetch and the buffer-types are passed as "struct input_absrange",
+ * which has the following fields:
+ *      slots: Number of MT slots to fetch data for.
+ *       code: First ABS axis to query.
+ *      count: Number of ABS axes to query starting at @code.
+ *     buffer: Pointer to a receive buffer where to store the fetched ABS
+ *             values. This buffer must be an array of __s32 with at least
+ *             (@slots * @code) elements. The buffer is interpreted as two
+ *             dimensional __s32 array, declared as: __s32[slots][codes]

tbh this seems more complicated than necessary. Have you thought about just dumping the events into the client buffer as if they came fresh in from the device? So to sync, the client calls the ioctl with a buffer and a buffer size, and the kernel simply writes a series of struct input_events into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?) followed by a SYN_DROPPED. So the buffer afterwards could look like this:
EV_ABS ABS_X 30
EV_ABS ABS_X 1202
EV_ABS ABS_MT_SLOT 0
EV_ABS ABS_MT_POSITION_X 30
EV_ABS ABS_MT_POSITION_Y 1202
EV_ABS ABS_MT_SLOT 1
EV_ABS ABS_MT_POSITION_X 80
EV_ABS ABS_MT_POSITION_Y 1800
EV_SYN SYN_REPORT 0

the client can then go through and just process the events on-by-one as it would otherwise with real events.

This approach could be even extended to include EV_KEY, etc. providing a single ioctl to sync the whole state of the device atomically.

comments?

I like it.

Quoted 17 lines

So you mean instead of passing a __32 array we pass a "struct input_event" array and write it there? So bypassing the receive-queue? That does sound quite nice, indeed. We could replace all the other "sync" ioctls and just provide a way to receive all the events directly.

Something like:

EVIOCQUERY(struct input_query)

struct input_query {
        __u16 type;
        __u16 start_code;
        __u16 end_code;
        __u16 slots;

Quoted 7 lines

Quoted 2 lines

        struct input_event buffer[];
};

No, it is more like EVIOCRESYNC(void) which makes input core to dump all existing state into the client's standard event queue so that here is no need to reconcile/reconstruct anything. We could give a new SYN marker to indicate end-of-state boundary.

This doesn't make sense to me, for two reasons:
* Events received during re-sync are usually handled differently than normal events. For instance, you *must* never handle key events from re-syncs for shortcuts, because you don't know the order they were pressed in.

If we put sync events in the queue, users cannot know where an event from.
Inserting a SYNC marker is ridiculous, because following normal events may drop those (via SYN_DROPPED). We'd have to protect it. Furthermore, it's awful to handle this from user-space, because you have no idea how many events are sent as part of the sync.

* The receive-queue is usually too small to hold all those events. I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer slots (defined as 8!). You cannot even hold a whole keyboard state there. Yes, usually it should be enough, but re-syncing is about handling corner-cases. If we make SYN_DROPPED handling cause SYN_DROPPED, we can just ignore it.

I understand why EVIORESYNC(void) is tempting. It avoids all the complexity of my patch and makes all the other sync-ioctls we have so far obsolete. But the reason we want it, is to avoid shortcomings of the limited receive-queue. I don't think any solution involving the receive-queue is going to work out. Even if we always make sure the receive-queue is big enough, we might still get new events coming in before user-space can drain it.

How about this:

struct input_resync {
        __u64 count;
        struct input_event buffer[];
};

EVIOCSYNC(struct input_resync) copies the current state of all available event-types of the device into the buffer. It returns the number of events that it wants to write (so user-space can pass count==0 the first time to figure out the required buffer-size). The ioctl then flushed the _whole_ input queue.

Note that even if we used the client's receive buffer, we'd have to copy huge amounts of events while holding the event lock. So both ideas have to lock the buffers during the operation. The downside of my solution is that we have to allocate temporary buffers as we cannot copy to user-space while holding spin-locks. However, my code already makes sure we don't do high-order allocations, so I think this is fine.

This ioctl would also allow us to get rid of all the other QUERY ioctls. User-space can use it to initialize it's state and then update it according to incoming events. This way, we simplify the API considerably! I really like this idea. I know, we only need this to handle corner-cases. But as Peter already said, it's really easy to trigger and ignoring it causes awful bugs in user-space.

Quoted 74 lines · David Herrmann

Hi

Quoted 62 lines · Dmitry Torokhov

Quoted 41 lines · David Herrmann

Hi

Quoted 38 lines · Peter Hutterer

Quoted 4 lines

include/uapi/linux/input.h +4 -0

+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *

include/uapi/linux/input.h struct input_mask { · line 232

Quoted 10 lines

include/uapi/linux/input.h +10 -0

+ * This fetches the current values of a range of ABS codes atomically. The range
+ * of codes to fetch and the buffer-types are passed as "struct input_absrange",
+ * which has the following fields:
+ *      slots: Number of MT slots to fetch data for.
+ *       code: First ABS axis to query.
+ *      count: Number of ABS axes to query starting at @code.
+ *     buffer: Pointer to a receive buffer where to store the fetched ABS
+ *             values. This buffer must be an array of __s32 with at least
+ *             (@slots * @code) elements. The buffer is interpreted as two
+ *             dimensional __s32 array, declared as: __s32[slots][codes]

tbh this seems more complicated than necessary. Have you thought about just dumping the events into the client buffer as if they came fresh in from the device? So to sync, the client calls the ioctl with a buffer and a buffer size, and the kernel simply writes a series of struct input_events into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?) followed by a SYN_DROPPED. So the buffer afterwards could look like this:
EV_ABS ABS_X 30
EV_ABS ABS_X 1202
EV_ABS ABS_MT_SLOT 0
EV_ABS ABS_MT_POSITION_X 30
EV_ABS ABS_MT_POSITION_Y 1202
EV_ABS ABS_MT_SLOT 1
EV_ABS ABS_MT_POSITION_X 80
EV_ABS ABS_MT_POSITION_Y 1800
EV_SYN SYN_REPORT 0

the client can then go through and just process the events on-by-one as it would otherwise with real events.

This approach could be even extended to include EV_KEY, etc. providing a single ioctl to sync the whole state of the device atomically.

comments?

I like it.

Quoted 17 lines

So you mean instead of passing a __32 array we pass a "struct input_event" array and write it there? So bypassing the receive-queue? That does sound quite nice, indeed. We could replace all the other "sync" ioctls and just provide a way to receive all the events directly.

Something like:

EVIOCQUERY(struct input_query)

struct input_query {
        __u16 type;
        __u16 start_code;
        __u16 end_code;
        __u16 slots;

Quoted 7 lines

Quoted 2 lines

        struct input_event buffer[];
};

No, it is more like EVIOCRESYNC(void) which makes input core to dump all existing state into the client's standard event queue so that here is no need to reconcile/reconstruct anything. We could give a new SYN marker to indicate end-of-state boundary.

Quoted 5 lines

This doesn't make sense to me, for two reasons:
* Events received during re-sync are usually handled differently than normal events. For instance, you *must* never handle key events from re-syncs for shortcuts, because you don't know the order they were pressed in.

I think you're vastly overestimating what the majority of userspace does ;) both the xorg and the libinput-based stacks pretty much ignore this at the moment and by the time we handle shortcuts we're already one or more APIs away from the SYN_DROPPED handling.

also, I haven't seen SYN_DROPPED issues from keyboard events. They usually happen only on absolute touch devices and ironically enough that's the one case where the kernel currently doesn't allow for a race-less resync.

Quoted 7 lines

If we put sync events in the queue, users cannot know where an event from.
Inserting a SYNC marker is ridiculous, because following normal events may drop those (via SYN_DROPPED). We'd have to protect it. Furthermore, it's awful to handle this from user-space, because you have no idea how many events are sent as part of the sync.

I suspect Dmitry meant a new EV_SYN code. From a userspace POV, I'd know that after the ioctl() all events up until SYN_SYNC_DONE are the syncing events. That would work well and is coincidentally almost identical to the libevdev public API.

In libevdev we already have code to calculate the maximum number of events by a device for the current SYN_DROPPED handling. And you can reduce the number of events by only sending those in a non-zero state for EV_KEY, EV_LED, etc.

IMO the real issue preventing this approach is:

Quoted 4 lines

* The receive-queue is usually too small to hold all those events. I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer slots (defined as 8!). You cannot even hold a whole keyboard state there. Yes, usually it should be enough, but re-syncing

Quoted 3 lines

is about handling corner-cases. If we make SYN_DROPPED handling cause SYN_DROPPED, we can just ignore it.

yep, that too was my first thought. with a plain resync ioctl you're pretty much guaranteed to get SYN_DROPPED before the client manages to handle the resync. Even if you reduce the number of events as above because the most common occurance for SYN_DROPPED is in the ABS_MT range which we cannot meaningfully reduce.

Quoted 9 lines

Quoted 3 lines

is about handling corner-cases. If we make SYN_DROPPED handling cause SYN_DROPPED, we can just ignore it.

yep, that too was my first thought. with a plain resync ioctl you're pretty much guaranteed to get SYN_DROPPED before the client manages to handle the resync. Even if you reduce the number of events as above because the most common occurance for SYN_DROPPED is in the ABS_MT range which we cannot meaningfully reduce.

Hmm, that's a problem... But is it? We need to make sure that buffer is large enough for the MT device to transmit all it's contacts properly. We can not expect that we'll always be able to reduce number of events if a user actively uses 10 contacts. IOW we need to solve this issue regardless of this proposed sync ioctl.

Maybe we need to review drivers and see if they need to supply their own hints or update hinting logic in core?

On Sun, Aug 10, 2014 at 05:01:35PM -0700, Dmitry Torokhov wrote:
[...]

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

is about handling corner-cases. If we make SYN_DROPPED handling cause SYN_DROPPED, we can just ignore it.

yep, that too was my first thought. with a plain resync ioctl you're pretty much guaranteed to get SYN_DROPPED before the client manages to handle the resync. Even if you reduce the number of events as above because the most common occurance for SYN_DROPPED is in the ABS_MT range which we cannot meaningfully reduce.

Quoted 8 lines

Hmm, that's a problem... But is it? We need to make sure that buffer is large enough for the MT device to transmit all it's contacts properly. We can not expect that we'll always be able to reduce number of events if a user actively uses 10 contacts. IOW we need to solve this issue regardless of this proposed sync ioctl.

Maybe we need to review drivers and see if they need to supply their own hints or update hinting logic in core?

The buffer is already large enough for at least one full report from the device plus a few extra events [1]. for the devices we see SYN_REPORT most frequently dumping the state means filling up the buffer to the almost maximum. To give some room for movement, we need to increase the queue by at least a factor 2. That gives us with room for one whole sync report and at least one full extra event. Anything smaller we get the side-effect that a client that is too slow and gets a SYN_DROPPED is actually worse off because now the buffer is so full from the sync that a SYN_DROPPED is even more likely to occur than before.

We also need to define the behaviour for the queue filling up while the client is in the middle of a sync. That means the client must be able to handle SYN_DROPPED as well as SYN_SYNC_DONE during a sync or the kernel protects the events up to SYN_SYNC_DONE in the queue in the case of a SYN_DROPPED.

Either way it's IMO more complicated than having a separate buffer for the sync state.

Quoted 10 lines

Quoted 4 lines

No, it is more like EVIOCRESYNC(void) which makes input core to dump all existing state into the client's standard event queue so that here is no need to reconcile/reconstruct anything. We could give a new SYN marker to indicate end-of-state boundary.

This doesn't make sense to me, for two reasons:
* Events received during re-sync are usually handled differently than normal events. For instance, you *must* never handle key events from re-syncs for shortcuts, because you don't know the order they were pressed in.

I think you're vastly overestimating what the majority of userspace does ;) both the xorg and the libinput-based stacks pretty much ignore this at the moment and by the time we handle shortcuts we're already one or more APIs away from the SYN_DROPPED handling.

also, I haven't seen SYN_DROPPED issues from keyboard events. They usually happen only on absolute touch devices and ironically enough that's the one case where the kernel currently doesn't allow for a race-less resync.

Quoted 7 lines

If we put sync events in the queue, users cannot know where an event from.
Inserting a SYNC marker is ridiculous, because following normal events may drop those (via SYN_DROPPED). We'd have to protect it. Furthermore, it's awful to handle this from user-space, because you have no idea how many events are sent as part of the sync.

I suspect Dmitry meant a new EV_SYN code. From a userspace POV, I'd know that after the ioctl() all events up until SYN_SYNC_DONE are the syncing events. That would work well and is coincidentally almost identical to the libevdev public API.

In libevdev we already have code to calculate the maximum number of events by a device for the current SYN_DROPPED handling. And you can reduce the number of events by only sending those in a non-zero state for EV_KEY, EV_LED, etc.

IMO the real issue preventing this approach is:

Quoted 6 lines

* The receive-queue is usually too small to hold all those events. I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer slots (defined as 8!). You cannot even hold a whole keyboard state there. Yes, usually it should be enough, but re-syncing is about handling corner-cases. If we make SYN_DROPPED handling cause SYN_DROPPED, we can just ignore it.

yep, that too was my first thought. with a plain resync ioctl you're pretty much guaranteed to get SYN_DROPPED before the client manages to handle the resync. Even if you reduce the number of events as above because the most common occurance for SYN_DROPPED is in the ABS_MT range which we cannot meaningfully reduce.

Quoted 15 lines

I understand why EVIORESYNC(void) is tempting. It avoids all the complexity of my patch and makes all the other sync-ioctls we have so far obsolete. But the reason we want it, is to avoid shortcomings of the limited receive-queue. I don't think any solution involving the receive-queue is going to work out. Even if we always make sure the receive-queue is big enough, we might still get new events coming in before user-space can drain it.

How about this:

struct input_resync {
        __u64 count;
        struct input_event buffer[];
};

Quoted 5 lines

EVIOCSYNC(struct input_resync) copies the current state of all available event-types of the device into the buffer. It returns the number of events that it wants to write (so user-space can pass count==0 the first time to figure out the required buffer-size). The ioctl then flushed the _whole_ input queue.

if you go with the suggestion above of only putting events in with a non-zero state then a count of 0 is guaranteed to return the wrong number since you may get events between the two ioctl calls. I don't think the double ioctl() call is that a case we need. as I said above the case of all events must already be handled in userspace anyway.

what's the reason you don't use a EVIOCSYNC(len) approach btw? quick check shows at 13 or 14 bits for len which should be enough for pretty much any device for a long time.

Quoted 8 lines

Note that even if we used the client's receive buffer, we'd have to copy huge amounts of events while holding the event lock. So both ideas have to lock the buffers during the operation. The downside of my solution is that we have to allocate temporary buffers as we cannot copy to user-space while holding spin-locks. However, my code already makes sure we don't do high-order allocations, so I think this is fine.

Quoted 6 lines

This ioctl would also allow us to get rid of all the other QUERY ioctls. User-space can use it to initialize it's state and then update it according to incoming events. This way, we simplify the API considerably! I really like this idea. I know, we only need this to handle corner-cases. But as Peter already said, it's really easy to trigger and ignoring it causes awful bugs in user-space.

summary from my side: I'd prefer EVIOCSYNC() with a buffer parameter, only sending nonzero state for key/led/sw/etc. and the full state for abs. no need for a new EV_SYN code, and provided it empties the whole queue we should be race-free.

Hi Dmitry

I posted all these ~2 months ago, but haven't seen any comments from you. Seeing that you switched jobs (congratulations, btw!),

I guess you were quite busy the
last few weeks. Hence, here's a resend of all the evdev changes squashed into a single series. They're also available in my fdo repo [1] in case you wanna fetch them directly.

No big changes except for a single "udev->state == UIST_CREATED" fix spotted by Peter.

The last patch is still RFC, so please omit it in case you apply the patches!

All patches but the last are tested via libevdev and run fine here since several months.