[PATCH RESEND 0/5] Evdev Extensions

[PATCH RESEND 0/5] Evdev Extensions

[David Herrmann]

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.

Thanks
David

[1] fdo repo: http://cgit.freedesktop.org/~dvdhrm/linux/log/?h=input-next

David Herrmann (5):
  Input: evdev - add event-mask API
  Input: uinput - uinput_validate_absbits() cleanup
  Input: uinput - add UI_GET_VERSION ioctl
  Input: uinput - add new UINPUT_DEV_SETUP ioctl
  Input: evdev - add new EVIOCGABSRANGE ioctl

 drivers/input/evdev.c       | 336 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/input/misc/uinput.c | 115 ++++++++++++---
 include/uapi/linux/input.h  | 100 ++++++++++++-
 include/uapi/linux/uinput.h |  64 ++++++++-
 4 files changed, 583 insertions(+), 32 deletions(-)

-- 
2.0.2

[PATCH RESEND 1/5] Input: evdev - add event-mask API

[David Herrmann]

Hardware manufacturers group keys in the weirdest way possible. This may
cause a power-key to be grouped together with normal keyboard keys and
thus be reported on the same kernel interface.

However, user-space is often only interested in specific sets of events.
For instance, daemons dealing with system-reboot (like systemd-logind)
listen for KEY_POWER, but are not interested in any main keyboard keys.
Usually, power keys are reported via separate interfaces, however,
some i8042 boards report it in the AT matrix. To avoid waking up those
system daemons on each key-press, we had two ideas:
 - split off KEY_POWER into a separate interface unconditionally
 - allow filtering a specific set of events on evdev FDs

Splitting of KEY_POWER is a rather weird way to deal with this and may
break backwards-compatibility. It is also specific to KEY_POWER and might
be required for other stuff, too. Moreover, we might end up with a huge
set of input-devices just to have them properly split.

Hence, this patchset implements the second idea: An event-mask to specify
which events you're interested in. Two ioctls allow setting this mask for
each event-type. If not set, all events are reported. The type==0 entry is
used same as in EVIOCGBIT to set the actual EV_* mask of filtered events.
This way, you have a two-level filter.

We are heavily forward-compatible to new event-types and event-codes. So
new user-space will be able to run on an old kernel which doesn't know the
given event-codes or event-types.

Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/evdev.c      | 156 ++++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/input.h |  56 ++++++++++++++++
 2 files changed, 210 insertions(+), 2 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index fd325ec..6386882 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -51,10 +51,130 @@ struct evdev_client {
 	struct list_head node;
 	int clkid;
 	bool revoked;
+	unsigned long *evmasks[EV_CNT];
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
 
+static size_t evdev_get_mask_cnt(unsigned int type)
+{
+	static size_t counts[EV_CNT] = {
+		/* EV_SYN==0 is EV_CNT, _not_ SYN_CNT, see EVIOCGBIT */
+		[EV_SYN] = EV_CNT,
+		[EV_KEY] = KEY_CNT,
+		[EV_REL] = REL_CNT,
+		[EV_ABS] = ABS_CNT,
+		[EV_MSC] = MSC_CNT,
+		[EV_SW] = SW_CNT,
+		[EV_LED] = LED_CNT,
+		[EV_SND] = SND_CNT,
+		[EV_FF] = FF_CNT,
+	};
+
+	return (type < EV_CNT) ? counts[type] : 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_set_mask(struct evdev_client *client,
+			  unsigned int type,
+			  const void __user *codes,
+			  u32 codes_size)
+{
+	unsigned long flags, *mask, *oldmask;
+	size_t cnt, size;
+
+	/* unknown masks are simply ignored for forward-compat */
+	cnt = evdev_get_mask_cnt(type);
+	if (!cnt)
+		return 0;
+
+	/* we allow 'codes_size > size' for forward-compat */
+	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+
+	mask = kzalloc(size, GFP_KERNEL);
+	if (!mask)
+		return -ENOMEM;
+
+	if (copy_from_user(mask, codes, min_t(size_t, codes_size, size))) {
+		kfree(mask);
+		return -EFAULT;
+	}
+
+	spin_lock_irqsave(&client->buffer_lock, flags);
+	oldmask = client->evmasks[type];
+	client->evmasks[type] = mask;
+	spin_unlock_irqrestore(&client->buffer_lock, flags);
+
+	kfree(oldmask);
+
+	return 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_get_mask(struct evdev_client *client,
+			  unsigned int type,
+			  void __user *codes,
+			  u32 codes_size)
+{
+	unsigned long *mask;
+	size_t cnt, size, min, i;
+	u8 __user *out;
+
+	/* we allow unknown types and 'codes_size > size' for forward-compat */
+	cnt = evdev_get_mask_cnt(type);
+	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+	min = min_t(size_t, codes_size, size);
+
+	if (cnt > 0) {
+		mask = client->evmasks[type];
+		if (mask) {
+			if (copy_to_user(codes, mask, min))
+				return -EFAULT;
+		} else {
+			/* fake mask with all bits set */
+			out = (u8 __user*)codes;
+			for (i = 0; i < min; ++i) {
+				if (put_user((u8)0xff,  out + i))
+					return -EFAULT;
+			}
+		}
+	}
+
+	codes = (u8*)codes + min;
+	codes_size -= min;
+
+	if (codes_size > 0 && clear_user(codes, codes_size))
+		return -EFAULT;
+
+	return 0;
+}
+
+/* requires the buffer lock to be held */
+static bool __evdev_is_filtered(struct evdev_client *client,
+				unsigned int type,
+				unsigned int code)
+{
+	unsigned long *mask;
+	size_t cnt;
+
+	/* EV_SYN and unknown codes are never filtered */
+	if (type == EV_SYN || type >= EV_CNT)
+		return false;
+
+	/* first test whether the type is filtered */
+	mask = client->evmasks[0];
+	if (mask && !test_bit(type, mask))
+		return true;
+
+	/* unknown values are never filtered */
+	cnt = evdev_get_mask_cnt(type);
+	if (!cnt || code >= cnt)
+		return false;
+
+	mask = client->evmasks[type];
+	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)
 {
@@ -177,12 +297,21 @@ static void evdev_pass_values(struct evdev_client *client,
 	spin_lock(&client->buffer_lock);
 
 	for (v = vals; v != vals + count; v++) {
+		if (__evdev_is_filtered(client, v->type, v->code))
+			continue;
+
+		if (v->type == EV_SYN && v->code == SYN_REPORT) {
+			/* drop empty SYN_REPORT */
+			if (client->packet_head == client->head)
+				continue;
+
+			wakeup = true;
+		}
+
 		event.type = v->type;
 		event.code = v->code;
 		event.value = v->value;
 		__pass_event(client, &event);
-		if (v->type == EV_SYN && v->code == SYN_REPORT)
-			wakeup = true;
 	}
 
 	spin_unlock(&client->buffer_lock);
@@ -365,6 +494,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 {
 	struct evdev_client *client = file->private_data;
 	struct evdev *evdev = client->evdev;
+	unsigned int i;
 
 	mutex_lock(&evdev->mutex);
 	evdev_ungrab(evdev, client);
@@ -372,6 +502,9 @@ static int evdev_release(struct inode *inode, struct file *file)
 
 	evdev_detach_client(evdev, client);
 
+	for (i = 0; i < EV_CNT; ++i)
+		kfree(client->evmasks[i]);
+
 	if (is_vmalloc_addr(client))
 		vfree(client);
 	else
@@ -811,6 +944,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	struct evdev *evdev = client->evdev;
 	struct input_dev *dev = evdev->handle.dev;
 	struct input_absinfo abs;
+	struct input_mask mask;
 	struct ff_effect effect;
 	int __user *ip = (int __user *)p;
 	unsigned int i, t, u, v;
@@ -872,6 +1006,24 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_revoke(evdev, client, file);
 
+	case EVIOCGMASK:
+		if (copy_from_user(&mask, p, sizeof(mask)))
+			return -EFAULT;
+
+		return evdev_get_mask(client,
+				      mask.type,
+				      (void*)(long)mask.codes_ptr,
+				      mask.codes_size);
+
+	case EVIOCSMASK:
+		if (copy_from_user(&mask, p, sizeof(mask)))
+			return -EFAULT;
+
+		return evdev_set_mask(client,
+				      mask.type,
+				      (const void*)(long)mask.codes_ptr,
+				      mask.codes_size);
+
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 19df18c..f6ace0e 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -97,6 +97,12 @@ struct input_keymap_entry {
 	__u8  scancode[32];
 };
 
+struct input_mask {
+	__u32 type;
+	__u32 codes_size;
+	__u64 codes_ptr;
+};
+
 #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
 #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
 #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
@@ -154,6 +160,56 @@ struct input_keymap_entry {
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 #define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
 
+/**
+ * EVIOCGMASK - Retrieve current event-mask
+ *
+ * This retrieves the current event-mask for a specific event-type. The
+ * argument must be of type "struct input_mask" and specifies the event-type to
+ * query, the receive buffer and the size of the receive buffer.
+ *
+ * The event-mask is a per-client mask that specifies which events are forwarded
+ * to the client. Each event-code is represented by a single bit in the
+ * event-mask. If the bit is set, the event is passed to the client normally.
+ * Otherwise, the event is filtered and and will never be queued on the
+ * client's receive buffer.
+ * Event-masks do not affect global state of an input-device. They only affect
+ * the open-file they're applied on. Each open-file (i.e, file-description) can
+ * have a different event-mask.
+ *
+ * The default event-mask for a client has all bits set, i.e. all events are
+ * forwarded to the client. If a kernel is queried for an unknown event-type
+ * or if the receive buffer is larger than the number of event-codes known to
+ * the kernel, the kernel returns all zeroes for those codes.
+ *
+ * At maximum, codes_size bytes are copied.
+ *
+ * This ioctl may fail with ENODEV in case the file is revoked, EFAULT
+ * if the receive-buffer points to invalid memory, or EINVAL if the kernel
+ * does not implement the ioctl.
+ */
+#define EVIOCGMASK		_IOR('E', 0x92, struct input_mask)	/* Get event-masks */
+
+/**
+ * EVIOCSMASK - Set event-mask
+ *
+ * This is the counterpart to EVIOCGMASK. Instead of receiving the current
+ * event-mask, this changes the client's event-mask for a specific type. See
+ * EVIOCGMASK for a description of event-masks and the argument-type.
+ *
+ * This ioctl provides full forward-compatibility. If the passed event-type is
+ * unknown to the kernel, or if the number of codes is bigger than known to the
+ * kernel, the ioctl is still accepted and applied. However, any unknown codes
+ * are left untouched and stay cleared. That means, the kernel always filters
+ * unknown codes regardless of what the client requests.
+ * If the new mask doesn't cover all known event-codes, all remaining codes are
+ * automatically cleared and thus filtered.
+ *
+ * This ioctl may fail with ENODEV in case the file is revoked. EFAULT is
+ * returned if the receive-buffer points to invalid memory. EINVAL is returned
+ * if the kernel does not implement the ioctl.
+ */
+#define EVIOCSMASK		_IOW('E', 0x93, struct input_mask)	/* Set event-masks */
+
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
 /*
-- 
2.0.2

[PATCH RESEND 2/5] Input: uinput - uinput_validate_absbits() cleanup

[David Herrmann]

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 <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/misc/uinput.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 8569362..615324c 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -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;
-- 
2.0.2

Re: [PATCH RESEND 2/5] Input: uinput - uinput_validate_absbits() cleanup

[Dmitry Torokhov]

On Sat, Jul 19, 2014 at 03:10:42PM +0200, David Herrmann wrote:
> 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 <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Applied, thank you.

> ---
>  drivers/input/misc/uinput.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 8569362..615324c 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -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;
> -- 
> 2.0.2
> 

-- 
Dmitry

[PATCH RESEND 3/5] Input: uinput - add UI_GET_VERSION ioctl

[David Herrmann]

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

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/misc/uinput.c | 6 ++++++
 include/uapi/linux/uinput.h | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 615324c..a2a3895 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -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;
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index 0389b48..19339cf 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -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.
-- 
2.0.2

Re: [PATCH RESEND 3/5] Input: uinput - add UI_GET_VERSION ioctl

[Dmitry Torokhov]

On Sat, Jul 19, 2014 at 03:10:43PM +0200, David Herrmann wrote:
> This ioctl is the counterpart to EVIOCGVERSION and returns the
> uinput-version the kernel was compiled with.
> 
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Applied, thank you.

> ---
>  drivers/input/misc/uinput.c | 6 ++++++
>  include/uapi/linux/uinput.h | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 615324c..a2a3895 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -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;
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index 0389b48..19339cf 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -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.
> -- 
> 2.0.2
> 

-- 
Dmitry

[PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl

[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 <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
 2 files changed, 118 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index a2a3895..0f45595 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -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;
+
+	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;
+
+	dev = udev->dev;
+	dev->id = setup.id;
+	udev->ff_effects_max = setup.ff_effects_max;
+
+	kfree(dev->name);
+	dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL);
+	if (!dev->name)
+		return -ENOMEM;
+
+	if (setup.abs_cnt > ABS_CNT)
+		setup.abs_cnt = ABS_CNT;
+
+	if (setup.abs_cnt > 0) {
+		u8 __user *p = (u8 __user*)arg->abs;
+
+		input_alloc_absinfo(dev);
+		if (!dev->absinfo)
+			return -ENOMEM;
+
+		for (i = 0; i < setup.abs_cnt; ++i, p += setup.absinfo_size) {
+			struct input_absinfo absinfo;
+
+			if (!test_bit(i, dev->absbit))
+				continue;
+
+			if (copy_from_user(&absinfo, p, sizeof(absinfo)))
+				return -EFAULT;
+
+			dev->absinfo[i] = absinfo;
+		}
+	}
+
+	retval = uinput_validate_absbits(dev);
+	if (retval < 0)
+		return retval;
+
+	udev->state = UIST_SETUP_COMPLETE;
+	return 0;
+}
+
+/* legacy setup via write() */
+static int uinput_setup_device_legacy(struct uinput_device *udev,
+				      const char __user *buffer, size_t count)
 {
 	struct uinput_user_dev	*user_dev;
 	struct input_dev	*dev;
@@ -475,7 +534,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
 
 	retval = udev->state == UIST_CREATED ?
 			uinput_inject_events(udev, buffer, count) :
-			uinput_setup_device(udev, buffer, count);
+			uinput_setup_device_legacy(udev, buffer, count);
 
 	mutex_unlock(&udev->mutex);
 
@@ -736,6 +795,10 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			uinput_destroy_device(udev);
 			goto out;
 
+		case UI_DEV_SETUP:
+			retval = uinput_dev_setup(udev, p);
+			goto out;
+
 		case UI_SET_EVBIT:
 			retval = uinput_set_bit(arg, evbit, EV_MAX);
 			goto out;
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index 19339cf..04a3876 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -20,6 +20,8 @@
  * Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
  *
  * Changes/Revisions:
+ *	0.5	06/20/2014 (David Herrmann <dh.herrmann@gmail.com>
+ *		- add UI_DEV_SETUP ioctl
  *	0.4	01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
  *		- add UI_GET_SYSNAME ioctl
  *	0.3	24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
@@ -37,8 +39,8 @@
 #include <linux/types.h>
 #include <linux/input.h>
 
-#define UINPUT_VERSION		4
-
+#define UINPUT_VERSION		5
+#define UINPUT_MAX_NAME_SIZE	80
 
 struct uinput_ff_upload {
 	__u32			request_id;
@@ -93,6 +95,54 @@ struct uinput_ff_erase {
  */
 #define UI_GET_VERSION _IOR(UINPUT_IOCTL_BASE, 301, unsigned int)
 
+struct uinput_setup {
+	__u64 absinfo_size;
+	struct input_id id;
+	char name[UINPUT_MAX_NAME_SIZE];
+	__u32 ff_effects_max;
+	__u32 abs_cnt;
+	struct input_absinfo abs[];
+};
+
+/**
+ * UI_DEV_SETUP - Set device parameters for setup
+ *
+ * This ioctl sets parameters for the input-device to be created. It must be
+ * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes
+ * the old "struct uinput_user_dev" method, which wrote this data via write().
+ *
+ * This ioctl takes a "struct uinput_setup" object as argument. The fields of
+ * this object are as follows:
+ *    absinfo_size: This field *must* be initialized to
+ *                  "sizeof(struct input_absinfo)". It allows to extend the API
+ *                  to support more absinfo fields. Older kernels ignore unknown
+ *                  extensions to "struct input_absinfo".
+ *              id: See the description of "struct input_id". This field is
+ *                  copied unchanged into the new device.
+ *            name: This is used unchanged as name for the new device.
+ *  ff_effects_max: This limits the maximum numbers of force-feedback effects.
+ *                  See below for a description of FF with uinput.
+ *         abs_cnt: This field defines the amount of elements in the following
+ *                  "abs" array. Axes beyond the kernel's ABS_CNT are ignored.
+ *             abs: This is an appended array that contains parameters for ABS
+ *                  axes. See "struct input_absinfo" for a description of these
+ *                  fields. This array is copied unchanged into the kernel for
+ *                  all specified axes. Axes not enabled via UI_SET_ABSBIT are
+ *                  ignored.
+ *
+ * This ioctl can be called multiple times and will overwrite previous values.
+ * If this ioctl fails with -EINVAL, you're recommended to use the old
+ * "uinput_user_dev" method via write() as fallback, in case you run on an old
+ * kernel that does not support this ioctl.
+ *
+ * This ioctl may fail with -EINVAL if it is not supported or if you passed
+ * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
+ * passed uinput_setup object cannot be read/written.
+ * If this call fails, partial data may have already been applied to the
+ * internal device.
+ */
+#define UI_DEV_SETUP _IOWR(UINPUT_IOCTL_BASE, 302, struct uinput_setup)
+
 /*
  * To write a force-feedback-capable driver, the upload_effect
  * and erase_effect callbacks in input_dev must be implemented.
@@ -144,7 +194,6 @@ struct uinput_ff_erase {
 #define UI_FF_UPLOAD		1
 #define UI_FF_ERASE		2
 
-#define UINPUT_MAX_NAME_SIZE	80
 struct uinput_user_dev {
 	char name[UINPUT_MAX_NAME_SIZE];
 	struct input_id id;
-- 
2.0.2

Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl

[Dmitry Torokhov]

Hi David,

On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
> 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 <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
>  include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 118 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index a2a3895..0f45595 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -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;
> +
> +	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.

Thanks.

-- 
Dmitry

Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl

[David Herrmann]

Hi

On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
> On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
>> 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 <peter.hutterer@who-t.net>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
>>  include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
>>  2 files changed, 118 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index a2a3895..0f45595 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -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;
>> +
>> +     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.

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.

Let me know which way you prefer.

Thanks
David

Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl

[Dmitry Torokhov]

On Mon, Jul 21, 2014 at 08:22:09AM +0200, David Herrmann wrote:
> Hi
> 
> On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi David,
> >
> > On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
> >> 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 <peter.hutterer@who-t.net>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >>  drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
> >>  include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
> >>  2 files changed, 118 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> index a2a3895..0f45595 100644
> >> --- a/drivers/input/misc/uinput.c
> >> +++ b/drivers/input/misc/uinput.c
> >> @@ -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;
> >> +
> >> +     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.
> 
> 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.

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.

Thanks!

-- 
Dmitry

Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl

[David Herrmann]

Hi

On Mon, Jul 21, 2014 at 10:11 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jul 21, 2014 at 08:22:09AM +0200, David Herrmann wrote:
>> 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.
>
> 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.

I'm not sure I agree. I'm a fan of forward-compatibility, but that's
probably a matter of taste. I will change my code accordingly.

> 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;
>         }

Hm, that is actually a good idea. I will give it a try.

> 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.

Yepp, I will include those in the next revision.

Thanks
David

[RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[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:

    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 <dh.herrmann@gmail.com>
---
 drivers/input/evdev.c      | 180 ++++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/input.h |  44 ++++++++++-
 2 files changed, 219 insertions(+), 5 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 6386882..7a25a7a 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -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;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index f6ace0e..9f851d4 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -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
+ *
+ * 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]
+ *
+ * Compared to EVIOCGABS this ioctl allows to retrieve a range of ABS codes
+ * atomically regarding any concurrent buffer modifications. Furthermore, any
+ * pending events for codes that were retrived via this call are flushed from
+ * 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 */
 
 /*
-- 
2.0.2

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[Peter Hutterer]

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

On Sat, Jul 19, 2014 at 03:10:45PM +0200, David Herrmann wrote:
> 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,

shouldn't this be SYN_DROPPED?

>                        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

>     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 <dh.herrmann@gmail.com>
> ---
>  drivers/input/evdev.c      | 180 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/input.h |  44 ++++++++++-
>  2 files changed, 219 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 6386882..7a25a7a 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -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;
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index f6ace0e..9f851d4 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -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
> + *
> + * 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?

> + *
> + * Compared to EVIOCGABS this ioctl allows to retrieve a range of ABS codes
> + * atomically regarding any concurrent buffer modifications. Furthermore, any
> + * pending events for codes that were retrived via this call are flushed from

typo: retrieved

Cheers,
   Peter

> + * 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 */
>  
>  /*
> -- 
> 2.0.2
> 

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[David Herrmann]

Hi

On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> sorry for the late comments, not sure how that slipped through but it hadn't
> shown up in my inbox unil Benjamin poked me.
>
> On Sat, Jul 19, 2014 at 03:10:45PM +0200, David Herrmann wrote:
>> 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,
>
> shouldn't this be SYN_DROPPED?

Whoops, indeed.

>>                        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

Yep!

>>     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 <dh.herrmann@gmail.com>
>> ---
>>  drivers/input/evdev.c      | 180 ++++++++++++++++++++++++++++++++++++++++++++-
>>  include/uapi/linux/input.h |  44 ++++++++++-
>>  2 files changed, 219 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index 6386882..7a25a7a 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -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;
>> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>> index f6ace0e..9f851d4 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
>> @@ -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
>> + *
>> + * 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[];
};

This way, you specify the event type as "type", the start/end code and
the kernel copies the queried events into "buffer". For ABS we need
the extra "slots" variable, so you can query all slots atomically.
I think I will give it a try. I like the generic touch it has.

Btw., I wonder whether it is cheaper to use get_user_pages() on the
receive buffer instead of allocating temporary pages, and then mapping
them temporarily for direct access. Hm... stupid huge buffers..

Thanks
David

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[Dmitry Torokhov]

On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
> Hi
> 
> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >> +
> >> +/**
> >> + * EVIOCGABSRANGE - Fetch range of ABS values
> >> + *
> >> + * 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.

> 
> 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[];
> };

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.

Thanks.

-- 
Dmitry

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[David Herrmann]

Hi

On Fri, Aug 8, 2014 at 7:47 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
>> Hi
>>
>> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
>> >> +
>> >> +/**
>> >> + * EVIOCGABSRANGE - Fetch range of ABS values
>> >> + *
>> >> + * 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.
>
>>
>> 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[];
>> };
>
> 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 came 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.

Comments?
David

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[Peter Hutterer]

On Sun, Aug 10, 2014 at 05:21:47PM +0200, David Herrmann wrote:
> Hi
> 
> On Fri, Aug 8, 2014 at 7:47 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
> >> Hi
> >>
> >> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >> >> +
> >> >> +/**
> >> >> + * EVIOCGABSRANGE - Fetch range of ABS values
> >> >> + *
> >> >> + * 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.
> >
> >>
> >> 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[];
> >> };
> >
> > 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.

>    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:

>  * 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.

> 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.

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.

> 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.

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.

Cheers,
   Peter

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[Dmitry Torokhov]

On Mon, Aug 11, 2014 at 09:17:59AM +1000, Peter Hutterer wrote:
> On Sun, Aug 10, 2014 at 05:21:47PM +0200, David Herrmann wrote:
> > Hi
> > 
> > On Fri, Aug 8, 2014 at 7:47 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
> > >> Hi
> > >>
> > >> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> > >> >> +
> > >> >> +/**
> > >> >> + * EVIOCGABSRANGE - Fetch range of ABS values
> > >> >> + *
> > >> >> + * 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.
> > >
> > >>
> > >> 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[];
> > >> };
> > >
> > > 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, client would not out of sudden get a bunch of events, it would have
requested them to be resent so it would know how to handle them properly.

> 
> 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.
> 
> >    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.

Yes, that's what I ment.


> 
> 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:
> 
> >  * 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

Note that EVDEV_BUF_PACKETS is not raw number of events in the queue, but
number for full "packets". For plain keyboards we end up with buffer enough to
hold 8 * 8 = 64 events. Since we only need to transmit active keys that should
be enough for any keyboard and if there are keyboards reporting more then we'd
have to change their hint anyway.

> >
> >    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?

Thanks.

-- 
Dmitry

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[Peter Hutterer]

On Sun, Aug 10, 2014 at 05:01:35PM -0700, Dmitry Torokhov wrote:
[...]
> > >
> > >    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?
 
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.

Cheers,
   Peter

[1] almost, it doesn't account for EV_SW for example

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[David Herrmann]

Hi

On Mon, Aug 11, 2014 at 4:13 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
> On Sun, Aug 10, 2014 at 05:01:35PM -0700, Dmitry Torokhov wrote:
> [...]
>> > >
>> > >    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?
>
> 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.

Yepp, fully agree on both points you make (I summarized them way worse
than you did!).

Thanks
David

Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl

[David Herrmann]

Hi

>> > 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.

I know what Xorg and weston do and, honestly, I don't care. If major
user-space components decide to ignore those side-effects, I'm fine.
They may have legitimate reason to do so. But I don't think we should
use this to argue against proper API design in the kernel.

> 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.

Yes, obviously ABS and REL events are the culprits as they can occur
in vast amounts. However, any other events in between (like button
presses) may be dropped due to excessive ABS events. So I disagree
that this is an exclusive issue of ABS/REL, they just trigger the
SYN_DROPPED.

>>    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.

Yes, I understand what Dmitry meant and I see the similarities to
libevdev. However, there are several issues:

1) *If* you flush all events into the queue, followed by a
SYN_SYNC_DONE marker, any following *real* events might cause the
queue to overflow and thus drop the SYN_SYNC_DONE marker. So if
user-space requested a sync and then reads a SYN_DROPPED *before*
reading a SYN_SYNC_DONE, it knows the sync didn't complete and has do
redo the sync all over. This is doable, but makes we wonder whether
it's the right design.

2) The evdev queue is not designed to hold this many events. Even if
it is, a SYN_DROPPED happens during excessive event reports. So if we
now also fill the queue with all sync-events, we increase the change
of overflowing the queue again, instead of clearing it and syncing
separately.

3) Flushing sync-events in the queue with special semantics (like only
sending non-zero state) sounds to me like a dirty hack, not like
proper API design. I mean, the queue was designed to send realtime
events, not to provide initial/SYN_DROPPED syncing. I don't see why
this is preferable to a separate user-provided buffer.

> IMO the real issue preventing this approach is:
>
>>  * 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.
>
>> 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.
>
> 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.

I think it's a bad idea to only send events with non-zero state. I can
live with it, but I don't understand why we do such optimizations for
an edge-case like SYN_DROPPED handling. It just complicates things and
no-one cares whether we copy 10 or 100 events, right?

> 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.

I think we can just make user-space read the event-bitmasks and
calculate the length themselves. This way, we avoid double-ioctl()
calls and it's easy enough to do from user-space.

>> 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.
>
> 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.

Except for "only send non-zero state" I fully agree. But I would also
accept it with that limitation/optimization.

Thanks
David

Re: [PATCH RESEND 0/5] Evdev Extensions

[Dmitry Torokhov]

Hi David,

On Sat, Jul 19, 2014 at 03:10:40PM +0200, David Herrmann wrote:
> Hi Dmitry
> 
> I posted all these ~2 months ago, but haven't seen any comments from you. Seeing
> that you switched jobs (congratulations, btw!),

Thanks :)

>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.

OK, I am looking at/applying them.

BTW, please continue using my @gmail.com address for upstream input
stuff and @google if it is something corporate related.

Thanks!

-- 
Dmitry