[PATCH 0/4] Input: ABS2 and friends

[PATCH 0/4] Input: ABS2 and friends

[David Herrmann]

Hi

This implements the recently discussed ABS2 API. It's working fine on my machine
with libevdev. Comments welcome!

* Patch #1 fixes some uinput shortcomings and prepares uinput for ABS2
* Patch #2 adds ABS2
* Patch #3 is just a small comment-fix for #4
* Patch #4 adds some new example ABS values in the new range

Note that I have patches pending which make use of the new ABS values, but I'd
like to get this reduced series in first.

Thanks
David

David Herrmann (4):
  Input: uinput: add full absinfo support
  Input: introduce ABS_MAX2/CNT2 and friends
  Input: remove ambigious gamepad comment
  Input: add motion-tracking ABS_* bits and docs

 Documentation/input/gamepad.txt          |   9 +-
 Documentation/input/motion-tracking.txt  | 149 +++++++++++++++++++++++++++++++
 drivers/hid/hid-debug.c                  |   2 +-
 drivers/hid/hid-input.c                  |   2 +-
 drivers/input/evdev.c                    |  95 +++++++++++++++++++-
 drivers/input/input.c                    |  14 +--
 drivers/input/keyboard/goldfish_events.c |   6 +-
 drivers/input/keyboard/hil_kbd.c         |   2 +-
 drivers/input/misc/uinput.c              | 146 ++++++++++++++++++++++--------
 include/linux/hid.h                      |   2 +-
 include/linux/input.h                    |   6 +-
 include/linux/mod_devicetable.h          |   2 +-
 include/uapi/linux/input.h               |  49 +++++++++-
 include/uapi/linux/uinput.h              |   9 ++
 14 files changed, 431 insertions(+), 62 deletions(-)
 create mode 100644 Documentation/input/motion-tracking.txt

-- 
1.8.5.1

[PATCH 1/4] Input: uinput: add full absinfo support

[David Herrmann]

We currently lack support for abs-resolution and abs-value parameters
during uinput ABS initialization. Furthermore, our parsers don't allow
growing ABS_CNT values. Therefore, introduce uinput_user_dev2.

User-space is free to write uinput_user_dev2 objects instead of
uinput_user_dev legacy objects now. If the kernel lacks support for it,
our comparison for "count != sizeof(struct uinput_user_dev)" will catch
this and return EINVAL. User-space shall retry with the legacy mode then.

Internally, we transform the legacy object into uinput_user_dev2 and then
handle both the same way.

The new uinput_user_dev2 object has multiple new features:
 - abs payload now has "value" and "resolution" parameters as part of the
   switch to "struct input_absinfo". We simply copy these over.
 - Our parser allows growing ABS_CNT. We automatically detect the payload
   size of the caller, thus, calculating the ABS_CNT the program was
   compiled with.
 - A "version" field to denote the uinput-version used. This is required
   to properly support changing "struct input_user_dev2" changes in the
   future. Due to the dynamic ABS_CNT support, we cannot simply add new
   fields, as we cannot deduce the structure size from the user-space
   given size. Thus, we need the "version" field to allow changing the
   object and properly detecting it in our write() handler.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/misc/uinput.c | 142 ++++++++++++++++++++++++++++++++------------
 include/uapi/linux/uinput.h |   9 +++
 2 files changed, 114 insertions(+), 37 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 7728359..927ad9a 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -358,14 +358,16 @@ static int uinput_allocate_device(struct uinput_device *udev)
 }
 
 static int uinput_setup_device(struct uinput_device *udev,
-			       const char __user *buffer, size_t count)
+			       struct uinput_user_dev2 *user_dev2,
+			       size_t abscnt)
 {
-	struct uinput_user_dev	*user_dev;
 	struct input_dev	*dev;
 	int			i;
 	int			retval;
+	struct input_absinfo	*abs;
 
-	if (count != sizeof(struct uinput_user_dev))
+	/* Ensure name is filled in */
+	if (!user_dev2->name[0])
 		return -EINVAL;
 
 	if (!udev->dev) {
@@ -375,37 +377,27 @@ static int uinput_setup_device(struct uinput_device *udev,
 	}
 
 	dev = udev->dev;
-
-	user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
-	if (IS_ERR(user_dev))
-		return PTR_ERR(user_dev);
-
-	udev->ff_effects_max = user_dev->ff_effects_max;
-
-	/* Ensure name is filled in */
-	if (!user_dev->name[0]) {
-		retval = -EINVAL;
-		goto exit;
-	}
+	udev->ff_effects_max = user_dev2->ff_effects_max;
 
 	kfree(dev->name);
-	dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE,
+	dev->name = kstrndup(user_dev2->name, UINPUT_MAX_NAME_SIZE,
 			     GFP_KERNEL);
-	if (!dev->name) {
-		retval = -ENOMEM;
-		goto exit;
-	}
-
-	dev->id.bustype	= user_dev->id.bustype;
-	dev->id.vendor	= user_dev->id.vendor;
-	dev->id.product	= user_dev->id.product;
-	dev->id.version	= user_dev->id.version;
+	if (!dev->name)
+		return -ENOMEM;
 
-	for (i = 0; i < ABS_CNT; i++) {
-		input_abs_set_max(dev, i, user_dev->absmax[i]);
-		input_abs_set_min(dev, i, user_dev->absmin[i]);
-		input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
-		input_abs_set_flat(dev, i, user_dev->absflat[i]);
+	dev->id.bustype = user_dev2->id.bustype;
+	dev->id.vendor = user_dev2->id.vendor;
+	dev->id.product = user_dev2->id.product;
+	dev->id.version = user_dev2->id.version;
+
+	for (i = 0; i < abscnt; i++) {
+		abs = &user_dev2->abs[i];
+		input_abs_set_val(dev, i, abs->value);
+		input_abs_set_max(dev, i, abs->maximum);
+		input_abs_set_min(dev, i, abs->minimum);
+		input_abs_set_fuzz(dev, i, abs->fuzz);
+		input_abs_set_flat(dev, i, abs->flat);
+		input_abs_set_res(dev, i, abs->resolution);
 	}
 
 	/* check if absmin/absmax/absfuzz/absflat are filled as
@@ -413,7 +405,7 @@ static int uinput_setup_device(struct uinput_device *udev,
 	if (test_bit(EV_ABS, dev->evbit)) {
 		retval = uinput_validate_absbits(dev);
 		if (retval < 0)
-			goto exit;
+			return retval;
 		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);
@@ -423,11 +415,84 @@ static int uinput_setup_device(struct uinput_device *udev,
 	}
 
 	udev->state = UIST_SETUP_COMPLETE;
-	retval = count;
+	return 0;
+}
+
+static int uinput_setup_device1(struct uinput_device *udev,
+				const char __user *buffer, size_t count)
+{
+	struct uinput_user_dev	*user_dev;
+	struct uinput_user_dev2	*user_dev2;
+	int			i;
+	int			retval;
+
+	if (count != sizeof(struct uinput_user_dev))
+		return -EINVAL;
+
+	user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
+	if (IS_ERR(user_dev))
+		return PTR_ERR(user_dev);
+
+	user_dev2 = kmalloc(sizeof(*user_dev2), GFP_KERNEL);
+	if (!user_dev2) {
+		kfree(user_dev);
+		return -ENOMEM;
+	}
+
+	user_dev2->version = UINPUT_VERSION;
+	memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE);
+	memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id));
+	user_dev2->ff_effects_max = user_dev->ff_effects_max;
+
+	for (i = 0; i < ABS_CNT; ++i) {
+		user_dev2->abs[i].value = 0;
+		user_dev2->abs[i].maximum = user_dev->absmax[i];
+		user_dev2->abs[i].minimum = user_dev->absmin[i];
+		user_dev2->abs[i].fuzz = user_dev->absfuzz[i];
+		user_dev2->abs[i].flat = user_dev->absflat[i];
+		user_dev2->abs[i].resolution = 0;
+	}
+
+	retval = uinput_setup_device(udev, user_dev2, ABS_CNT);
 
- exit:
 	kfree(user_dev);
-	return retval;
+	kfree(user_dev2);
+
+	return retval ? retval : count;
+}
+
+static int uinput_setup_device2(struct uinput_device *udev,
+			       const char __user *buffer, size_t count)
+{
+	struct uinput_user_dev2	*user_dev2;
+	int			retval;
+	size_t			off, abscnt, max;
+
+	/* The first revision of "uinput_user_dev2" is bigger than
+	 * "uinput_user_dev" and growing. Disallow any smaller payloads. */
+	if (count <= sizeof(struct uinput_user_dev))
+		return -EINVAL;
+
+	/* rough check to avoid huge kernel space allocations */
+	max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
+	if (count > max)
+		return -EINVAL;
+
+	user_dev2 = memdup_user(buffer, count);
+	if (IS_ERR(user_dev2))
+		return PTR_ERR(user_dev2);
+
+	if (user_dev2->version > UINPUT_VERSION) {
+		retval = -EINVAL;
+	} else {
+		off = offsetof(struct uinput_user_dev2, abs);
+		abscnt = (count - off) / sizeof(*user_dev2->abs);
+		retval = uinput_setup_device(udev, user_dev2, abscnt);
+	}
+
+	kfree(user_dev2);
+
+	return retval ? retval : count;
 }
 
 static ssize_t uinput_inject_events(struct uinput_device *udev,
@@ -469,9 +534,12 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
 	if (retval)
 		return retval;
 
-	retval = udev->state == UIST_CREATED ?
-			uinput_inject_events(udev, buffer, count) :
-			uinput_setup_device(udev, buffer, count);
+	if (udev->state == UIST_CREATED)
+		retval = uinput_inject_events(udev, buffer, count);
+	else if (count <= sizeof(struct uinput_user_dev))
+		retval = uinput_setup_device1(udev, buffer, count);
+	else
+		retval = uinput_setup_device2(udev, buffer, count);
 
 	mutex_unlock(&udev->mutex);
 
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index fe46431..c2e8710 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -134,4 +134,13 @@ struct uinput_user_dev {
 	__s32 absfuzz[ABS_CNT];
 	__s32 absflat[ABS_CNT];
 };
+
+struct uinput_user_dev2 {
+	__u8 version;
+	char name[UINPUT_MAX_NAME_SIZE];
+	struct input_id id;
+	__u32 ff_effects_max;
+	struct input_absinfo abs[ABS_CNT];
+};
+
 #endif /* _UAPI__UINPUT_H_ */
-- 
1.8.5.1

[PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[David Herrmann]

As we painfully noticed during the 3.12 merge-window our
EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
hacks to work around it but if we ever decide to increase ABS_MAX, the
EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
misinterpretations in the kernel that we cannot catch.

Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
ioctls to get/set abs-params. They no longer encode the ABS code in the
ioctl number and thus allow up to 4 billion ABS codes.

The new API also allows to query multiple ABS values with one call. To
allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
writes to ABS_MT_SLOT. Furthermore, for better compatibility with
newer user-space, we ignore writes to unknown codes. Hence, if we ever
increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
fine even on old kernels.

Note that we also need to increase EV_VERSION so user-space can reliably
know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
reliably without EVIOCGVERSION.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/hid-debug.c                  |  2 +-
 drivers/hid/hid-input.c                  |  2 +-
 drivers/input/evdev.c                    | 95 +++++++++++++++++++++++++++++++-
 drivers/input/input.c                    | 14 ++---
 drivers/input/keyboard/goldfish_events.c |  6 +-
 drivers/input/keyboard/hil_kbd.c         |  2 +-
 drivers/input/misc/uinput.c              |  6 +-
 include/linux/hid.h                      |  2 +-
 include/linux/input.h                    |  6 +-
 include/uapi/linux/input.h               | 42 +++++++++++++-
 include/uapi/linux/uinput.h              |  2 +-
 11 files changed, 155 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 8453214..d32fa30 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -862,7 +862,7 @@ static const char *relatives[REL_MAX + 1] = {
 	[REL_WHEEL] = "Wheel",		[REL_MISC] = "Misc",
 };
 
-static const char *absolutes[ABS_CNT] = {
+static const char *absolutes[ABS_CNT2] = {
 	[ABS_X] = "X",			[ABS_Y] = "Y",
 	[ABS_Z] = "Z",			[ABS_RX] = "Rx",
 	[ABS_RY] = "Ry",		[ABS_RZ] = "Rz",
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d97f232..a02721c 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1300,7 +1300,7 @@ static bool hidinput_has_been_populated(struct hid_input *hidinput)
 	for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
 		r |= hidinput->input->relbit[i];
 
-	for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
+	for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
 		r |= hidinput->input->absbit[i];
 
 	for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index a06e125..32b74e5 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -643,7 +643,7 @@ static int handle_eviocgbit(struct input_dev *dev,
 	case      0: bits = dev->evbit;  len = EV_MAX;  break;
 	case EV_KEY: bits = dev->keybit; len = KEY_MAX; break;
 	case EV_REL: bits = dev->relbit; len = REL_MAX; break;
-	case EV_ABS: bits = dev->absbit; len = ABS_MAX; break;
+	case EV_ABS: bits = dev->absbit; len = ABS_MAX2; break;
 	case EV_MSC: bits = dev->mscbit; len = MSC_MAX; break;
 	case EV_LED: bits = dev->ledbit; len = LED_MAX; break;
 	case EV_SND: bits = dev->sndbit; len = SND_MAX; break;
@@ -671,6 +671,93 @@ static int handle_eviocgbit(struct input_dev *dev,
 }
 #undef OLD_KEY_MAX
 
+static int evdev_handle_get_abs2(struct input_dev *dev, void __user *p)
+{
+	u32 code, cnt, valid_cnt, i;
+	struct input_absinfo2 __user *pinfo = p;
+	struct input_absinfo abs;
+
+	if (copy_from_user(&code, &pinfo->code, sizeof(code)))
+		return -EFAULT;
+	if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
+		return -EFAULT;
+	if (!cnt)
+		return 0;
+
+	if (!dev->absinfo)
+		valid_cnt = 0;
+	else if (code > ABS_MAX2)
+		valid_cnt = 0;
+	else if (code + cnt <= code || code + cnt > ABS_MAX2)
+		valid_cnt = ABS_MAX2 - code + 1;
+	else
+		valid_cnt = cnt;
+
+	for (i = 0; i < valid_cnt; ++i) {
+		/*
+		 * Take event lock to ensure that we are not
+		 * copying data while EVIOCSABS2 changes it.
+		 * Might be inconsistent, otherwise.
+		 */
+		spin_lock_irq(&dev->event_lock);
+		abs = dev->absinfo[code + i];
+		spin_unlock_irq(&dev->event_lock);
+
+		if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
+			return -EFAULT;
+	}
+
+	memset(&abs, 0, sizeof(abs));
+	for (i = valid_cnt; i < cnt; ++i)
+		if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
+			return -EFAULT;
+
+	return 0;
+}
+
+static int evdev_handle_set_abs2(struct input_dev *dev, void __user *p)
+{
+	struct input_absinfo2 __user *pinfo = p;
+	struct input_absinfo *abs;
+	u32 code, cnt, i;
+	size_t size;
+
+	if (!dev->absinfo)
+		return 0;
+	if (copy_from_user(&code, &pinfo->code, sizeof(code)))
+		return -EFAULT;
+	if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
+		return -EFAULT;
+	if (!cnt || code > ABS_MAX2)
+		return 0;
+
+	if (code + cnt <= code || code + cnt > ABS_MAX2)
+		cnt = ABS_MAX2 - code + 1;
+
+	size = cnt * sizeof(*abs);
+	abs = memdup_user(pinfo->info, size);
+	if (IS_ERR(abs))
+		return PTR_ERR(abs);
+
+	/*
+	 * Take event lock to ensure that we are not
+	 * changing device parameters in the middle
+	 * of event.
+	 */
+	spin_lock_irq(&dev->event_lock);
+	for (i = 0; i < cnt; ++i) {
+		/* silently drop ABS_MT_SLOT */
+		if (code + i == ABS_MT_SLOT)
+			continue;
+
+		dev->absinfo[code + i] = abs[i];
+	}
+	spin_unlock_irq(&dev->event_lock);
+
+	kfree(abs);
+	return 0;
+}
+
 static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
 {
 	struct input_keymap_entry ke = {
@@ -898,6 +985,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		client->clkid = i;
 		return 0;
 
+	case EVIOCGABS2:
+		return evdev_handle_get_abs2(dev, p);
+
+	case EVIOCSABS2:
+		return evdev_handle_set_abs2(dev, p);
+
 	case EVIOCGKEYCODE:
 		return evdev_handle_get_keycode(dev, p);
 
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 846ccdd..042157c 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -305,7 +305,7 @@ static int input_get_disposition(struct input_dev *dev,
 		break;
 
 	case EV_ABS:
-		if (is_event_supported(code, dev->absbit, ABS_MAX))
+		if (is_event_supported(code, dev->absbit, ABS_MAX2))
 			disposition = input_handle_abs_event(dev, code, &value);
 
 		break;
@@ -474,7 +474,7 @@ EXPORT_SYMBOL(input_inject_event);
 void input_alloc_absinfo(struct input_dev *dev)
 {
 	if (!dev->absinfo)
-		dev->absinfo = kcalloc(ABS_CNT, sizeof(struct input_absinfo),
+		dev->absinfo = kcalloc(ABS_CNT2, sizeof(struct input_absinfo),
 					GFP_KERNEL);
 
 	WARN(!dev->absinfo, "%s(): kcalloc() failed?\n", __func__);
@@ -954,7 +954,7 @@ static const struct input_device_id *input_match_device(struct input_handler *ha
 		if (!bitmap_subset(id->relbit, dev->relbit, REL_MAX))
 			continue;
 
-		if (!bitmap_subset(id->absbit, dev->absbit, ABS_MAX))
+		if (!bitmap_subset(id->absbit, dev->absbit, ABS_MAX2))
 			continue;
 
 		if (!bitmap_subset(id->mscbit, dev->mscbit, MSC_MAX))
@@ -1147,7 +1147,7 @@ static int input_devices_seq_show(struct seq_file *seq, void *v)
 	if (test_bit(EV_REL, dev->evbit))
 		input_seq_print_bitmap(seq, "REL", dev->relbit, REL_MAX);
 	if (test_bit(EV_ABS, dev->evbit))
-		input_seq_print_bitmap(seq, "ABS", dev->absbit, ABS_MAX);
+		input_seq_print_bitmap(seq, "ABS", dev->absbit, ABS_MAX2);
 	if (test_bit(EV_MSC, dev->evbit))
 		input_seq_print_bitmap(seq, "MSC", dev->mscbit, MSC_MAX);
 	if (test_bit(EV_LED, dev->evbit))
@@ -1333,7 +1333,7 @@ static int input_print_modalias(char *buf, int size, struct input_dev *id,
 	len += input_print_modalias_bits(buf + len, size - len,
 				'r', id->relbit, 0, REL_MAX);
 	len += input_print_modalias_bits(buf + len, size - len,
-				'a', id->absbit, 0, ABS_MAX);
+				'a', id->absbit, 0, ABS_MAX2);
 	len += input_print_modalias_bits(buf + len, size - len,
 				'm', id->mscbit, 0, MSC_MAX);
 	len += input_print_modalias_bits(buf + len, size - len,
@@ -1592,7 +1592,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 	if (test_bit(EV_REL, dev->evbit))
 		INPUT_ADD_HOTPLUG_BM_VAR("REL=", dev->relbit, REL_MAX);
 	if (test_bit(EV_ABS, dev->evbit))
-		INPUT_ADD_HOTPLUG_BM_VAR("ABS=", dev->absbit, ABS_MAX);
+		INPUT_ADD_HOTPLUG_BM_VAR("ABS=", dev->absbit, ABS_MAX2);
 	if (test_bit(EV_MSC, dev->evbit))
 		INPUT_ADD_HOTPLUG_BM_VAR("MSC=", dev->mscbit, MSC_MAX);
 	if (test_bit(EV_LED, dev->evbit))
@@ -1929,7 +1929,7 @@ static unsigned int input_estimate_events_per_packet(struct input_dev *dev)
 
 	events = mt_slots + 1; /* count SYN_MT_REPORT and SYN_REPORT */
 
-	for (i = 0; i < ABS_CNT; i++) {
+	for (i = 0; i < ABS_CNT2; i++) {
 		if (test_bit(i, dev->absbit)) {
 			if (input_is_mt_axis(i))
 				events += mt_slots;
diff --git a/drivers/input/keyboard/goldfish_events.c b/drivers/input/keyboard/goldfish_events.c
index 9f60a2e..9999cea 100644
--- a/drivers/input/keyboard/goldfish_events.c
+++ b/drivers/input/keyboard/goldfish_events.c
@@ -90,8 +90,8 @@ static void events_import_abs_params(struct event_dev *edev)
 	__raw_writel(PAGE_ABSDATA, addr + REG_SET_PAGE);
 
 	count = __raw_readl(addr + REG_LEN) / sizeof(val);
-	if (count > ABS_MAX)
-		count = ABS_MAX;
+	if (count > ABS_MAX2)
+		count = ABS_MAX2;
 
 	for (i = 0; i < count; i++) {
 		if (!test_bit(i, input_dev->absbit))
@@ -158,7 +158,7 @@ static int events_probe(struct platform_device *pdev)
 	events_import_bits(edev, input_dev->evbit, EV_SYN, EV_MAX);
 	events_import_bits(edev, input_dev->keybit, EV_KEY, KEY_MAX);
 	events_import_bits(edev, input_dev->relbit, EV_REL, REL_MAX);
-	events_import_bits(edev, input_dev->absbit, EV_ABS, ABS_MAX);
+	events_import_bits(edev, input_dev->absbit, EV_ABS, ABS_MAX2);
 	events_import_bits(edev, input_dev->mscbit, EV_MSC, MSC_MAX);
 	events_import_bits(edev, input_dev->ledbit, EV_LED, LED_MAX);
 	events_import_bits(edev, input_dev->sndbit, EV_SND, SND_MAX);
diff --git a/drivers/input/keyboard/hil_kbd.c b/drivers/input/keyboard/hil_kbd.c
index 589e3c2..4e4e010 100644
--- a/drivers/input/keyboard/hil_kbd.c
+++ b/drivers/input/keyboard/hil_kbd.c
@@ -387,7 +387,7 @@ static void hil_dev_pointer_setup(struct hil_dev *ptr)
 					0, HIL_IDD_AXIS_MAX(idd, i - 3), 0, 0);
 
 #ifdef TABLET_AUTOADJUST
-		for (i = 0; i < ABS_MAX; i++) {
+		for (i = 0; i < ABS_MAX2; i++) {
 			int diff = input_abs_get_max(input_dev, ABS_X + i) / 10;
 			input_abs_set_min(input_dev, ABS_X + i,
 				input_abs_get_min(input_dev, ABS_X + i) + diff);
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 927ad9a..4660ed1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -311,7 +311,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
 	unsigned int cnt;
 	int retval = 0;
 
-	for (cnt = 0; cnt < ABS_CNT; cnt++) {
+	for (cnt = 0; cnt < ABS_CNT2; cnt++) {
 		int min, max;
 		if (!test_bit(cnt, dev->absbit))
 			continue;
@@ -474,7 +474,7 @@ static int uinput_setup_device2(struct uinput_device *udev,
 		return -EINVAL;
 
 	/* rough check to avoid huge kernel space allocations */
-	max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
+	max = ABS_CNT2 * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
 	if (count > max)
 		return -EINVAL;
 
@@ -780,7 +780,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			break;
 
 		case UI_SET_ABSBIT:
-			retval = uinput_set_bit(arg, absbit, ABS_MAX);
+			retval = uinput_set_bit(arg, absbit, ABS_MAX2);
 			break;
 
 		case UI_SET_MSCBIT:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 31b9d29..c21d8bb 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -828,7 +828,7 @@ static inline void hid_map_usage(struct hid_input *hidinput,
 	switch (type) {
 	case EV_ABS:
 		*bit = input->absbit;
-		*max = ABS_MAX;
+		*max = ABS_MAX2;
 		break;
 	case EV_REL:
 		*bit = input->relbit;
diff --git a/include/linux/input.h b/include/linux/input.h
index 82ce323..c6add6f 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -129,7 +129,7 @@ struct input_dev {
 	unsigned long evbit[BITS_TO_LONGS(EV_CNT)];
 	unsigned long keybit[BITS_TO_LONGS(KEY_CNT)];
 	unsigned long relbit[BITS_TO_LONGS(REL_CNT)];
-	unsigned long absbit[BITS_TO_LONGS(ABS_CNT)];
+	unsigned long absbit[BITS_TO_LONGS(ABS_CNT2)];
 	unsigned long mscbit[BITS_TO_LONGS(MSC_CNT)];
 	unsigned long ledbit[BITS_TO_LONGS(LED_CNT)];
 	unsigned long sndbit[BITS_TO_LONGS(SND_CNT)];
@@ -210,8 +210,8 @@ struct input_dev {
 #error "REL_MAX and INPUT_DEVICE_ID_REL_MAX do not match"
 #endif
 
-#if ABS_MAX != INPUT_DEVICE_ID_ABS_MAX
-#error "ABS_MAX and INPUT_DEVICE_ID_ABS_MAX do not match"
+#if ABS_MAX2 != INPUT_DEVICE_ID_ABS_MAX
+#error "ABS_MAX2 and INPUT_DEVICE_ID_ABS_MAX do not match"
 #endif
 
 #if MSC_MAX != INPUT_DEVICE_ID_MSC_MAX
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index bd24470..1856461 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)
@@ -74,6 +74,30 @@ struct input_absinfo {
 };
 
 /**
+ * struct input_absinfo2 - used by EVIOC[G/S]ABS2 ioctls
+ * @code: First ABS code to query
+ * @cnt: Number of ABS codes to query starting at @code
+ * @info: #@cnt absinfo structures to get/set abs parameters for all codes
+ *
+ * This structure is used by the new EVIOC[G/S]ABS2 ioctls which
+ * do the same as the old EVIOC[G/S]ABS ioctls but avoid encoding
+ * the ABS code in the ioctl number. This allows a much wider
+ * range of ABS codes. Furthermore, it allows to query multiple codes with a
+ * single call.
+ *
+ * Note that this silently drops any requests to set ABS_MT_SLOT. Hence, it is
+ * allowed to call this with code=0 cnt=ABS_CNT2. Furthermore, retrieving
+ * invalid codes returns all 0, setting them does nothing. So you must check
+ * with EVIOCGBIT first if you want reliable results. This behavior is needed
+ * to allow forward compatibility to new ABS codes.
+ */
+struct input_absinfo2 {
+	__u32 code;
+	__u32 cnt;
+	struct input_absinfo info[1];
+};
+
+/**
  * struct input_keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
  * @scancode: scancode represented in machine-endian form.
  * @len: length of the scancode that resides in @scancode buffer.
@@ -153,6 +177,8 @@ struct input_keymap_entry {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 #define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
+#define EVIOCGABS2		_IOR('E', 0x92, struct input_absinfo2)	/* get abs value/limits */
+#define EVIOCSABS2		_IOW('E', 0x93, struct input_absinfo2)	/* set abs value/limits */
 
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
@@ -835,11 +861,23 @@ struct input_keymap_entry {
 #define ABS_MT_TOOL_X		0x3c	/* Center X tool position */
 #define ABS_MT_TOOL_Y		0x3d	/* Center Y tool position */
 
-
+/*
+ * ABS_MAX/CNT is limited to a maximum of 0x3f due to the design of EVIOCGABS
+ * and EVIOCSABS ioctls. Other kernel APIs like uinput also hardcoded it. Do
+ * not modify this value and instead use the extended ABS_MAX2/CNT2 API.
+ */
 #define ABS_MAX			0x3f
 #define ABS_CNT			(ABS_MAX+1)
 
 /*
+ * Due to API restrictions the legacy evdev API only supports ABS values up to
+ * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
+ * between ABS_MAX and ABS_MAX2.
+ */
+#define ABS_MAX2		0x3f
+#define ABS_CNT2		(ABS_MAX2+1)
+
+/*
  * Switch events
  */
 
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index c2e8710..27ee521 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -140,7 +140,7 @@ struct uinput_user_dev2 {
 	char name[UINPUT_MAX_NAME_SIZE];
 	struct input_id id;
 	__u32 ff_effects_max;
-	struct input_absinfo abs[ABS_CNT];
+	struct input_absinfo abs[ABS_CNT2];
 };
 
 #endif /* _UAPI__UINPUT_H_ */
-- 
1.8.5.1

[PATCH 3/4] Input: remove ambigious gamepad comment

[David Herrmann]

If only a single trigger combination is present on a device, either can be
reported. If the side of the trigger (left/right) cannot be decided (eg.,
if it's centered), then choose any side. User-space automatically notices
that only a single trigger is reported and thus doesn't care how it is
reported.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 Documentation/input/gamepad.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/input/gamepad.txt b/Documentation/input/gamepad.txt
index 31bb6a4..196dc42 100644
--- a/Documentation/input/gamepad.txt
+++ b/Documentation/input/gamepad.txt
@@ -138,8 +138,6 @@ Triggers:
   Upper trigger buttons are reported as BTN_TR or ABS_HAT1X (right) and BTN_TL
   or ABS_HAT1Y (left). Lower trigger buttons are reported as BTN_TR2 or
   ABS_HAT2X (right/ZR) and BTN_TL2 or ABS_HAT2Y (left/ZL).
-  If only one trigger-button combination is present (upper+lower), they are
-  reported as "right" triggers (BTN_TR/ABS_HAT1X).
     (ABS trigger values start at 0, pressure is reported as positive values)
 
 Menu-Pad:
-- 
1.8.5.1

[PATCH 4/4] Input: add motion-tracking ABS_* bits and docs

[David Herrmann]

Motion sensors are getting quite common in mobile devices. To avoid
returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
mouse-driver, this adds separate ABS_* bits for that.

This is needed if gaming devices want to report their normal data plus
accelerometer/gyro data. Usually, ABS_X/Y are already used by analog
sticks, so need separate definitions, anyway.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 Documentation/input/gamepad.txt         |   7 ++
 Documentation/input/motion-tracking.txt | 149 ++++++++++++++++++++++++++++++++
 include/linux/mod_devicetable.h         |   2 +-
 include/uapi/linux/input.h              |   9 +-
 4 files changed, 165 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/input/motion-tracking.txt

diff --git a/Documentation/input/gamepad.txt b/Documentation/input/gamepad.txt
index 196dc42..eeda685 100644
--- a/Documentation/input/gamepad.txt
+++ b/Documentation/input/gamepad.txt
@@ -57,6 +57,9 @@ Most gamepads have the following features:
   - Rumble
     Many devices provide force-feedback features. But are mostly just
     simple rumble motors.
+  - Motion-tracking
+    Gamepads may include motion-tracking sensors like accelerometers and
+    gyroscopes.
 
 3. Detection
 ~~~~~~~~~~~~
@@ -153,5 +156,9 @@ Menu-Pad:
 Rumble:
   Rumble is adverticed as FF_RUMBLE.
 
+Motion-tracking:
+  Motion-tracking is defined in ./Documentation/input/motion-tracking.txt and
+  gamepads shall comply to the rules defined there.
+
 ----------------------------------------------------------------------------
   Written 2013 by David Herrmann <dh.herrmann@gmail.com>
diff --git a/Documentation/input/motion-tracking.txt b/Documentation/input/motion-tracking.txt
new file mode 100644
index 0000000..0885c9a
--- /dev/null
+++ b/Documentation/input/motion-tracking.txt
@@ -0,0 +1,149 @@
+                           Motion Tracking API
+----------------------------------------------------------------------------
+
+1. Intro
+~~~~~~~~
+Motion tracking devices produce device motion events generated from an
+accelerometer, gyroscope or compass. This data can be returned to user-space
+via input events. This document defines how this data is reported.
+
+2. Devices
+~~~~~~~~~~
+In this document, a "device" is one of:
+ - accelerometer
+ - gyroscope
+ - compass
+
+These devices returned their information via different APIs in the past. To
+unify them and define a common API, a set of input evdev codes was created. Old
+drivers might continue using their API, but developers are encouraged to use
+the input evdev API for new drivers.
+
+2.1 Axes
+~~~~~~~~
+Movement data is usually returned as absolute data for the 3 axes of a device.
+In this context, the three axes are defined as:
+ - X: Axis goes from the left to the right side of the device
+ - Y: Axis goes from the bottom to the top of the device
+ - Z: Axis goes from the back to the front of the device
+
+The front of a device is the side faced to the user. For a mobile-phone it
+would be the screen. For devices without a screen, the front is usually the
+side with the most buttons on it.
+
+                           Example: Mobile-Phone
+  +-------------------------------------------------------------------------+
+  |                      TOP                                                |
+  |                                                                         |
+  |                                                                         |
+  |          +---------------------------+                                  |
+  |          |\  ________________________ \      .__                        |
+  |          \ \ \                       \ \     |\                         |
+  |           \ \ \              __       \ \      \                   RIGHT|
+  |            \ \ \              /|       \ \      \__                     |
+  |             \ \ \          __/          \ \     |\                      |
+  |              \ \ \          /|           \ \      \ (Y Axis)            |
+  |               \ \ \      __/  (Z axis)    \ \      \__                  |
+  |                \ \ \      /|               \ \     |\                   |
+  | LEFT            \ \ \    /                  \ \      \                  |
+  |                  \ \ \         FRONT         \ \      \                 |
+  |                   \ \ \                       \ \                       |
+  |                    \ \ \_______________________\ \                      |
+  |                     \ \             ___           \                     |
+  |                     /\ \            \__\           \                    |
+  |                  __/  \ +---------------------------+                   |
+  |                   /|   \|___________________________|                   |
+  |                  / BACK                                                 |
+  |                                      (X axis)                           |
+  |                        ------->------->------->------->                 |
+  |                                                                         |
+  |                                                                         |
+  |                                         BOTTOM                          |
+  +-------------------------------------------------------------------------+
+
+Rotation-data is reported as clock-wise rotation on an axis. For a given axes,
+the reported rotation would be:
+                                          ___
+                                            /|
+                                           / | (axis)
+                                          /
+                                    .-**-.
+                                   /    / \
+                                  |    / \ | /
+                                   \  /   \|/  (clock-wise rotation)
+                                     /
+                                    /
+                                   /
+
+2.2 Calibration
+~~~~~~~~~~~~~~~
+Motion sensors are often highly sensitive and need precise calibration. Users
+are adviced to perform neutral-point calibration themselves or to implement a
+state-machine to normalize input data automatically.
+
+Kernel devices may perform their own calibration and/or normalization. However,
+this is usually sparse and, if implemented, transparent to the user.
+
+There is currently no way to feed calibration data into the kernel in a generic
+way. Proposals welcome!
+
+2.3 Units
+~~~~~~~~~
+(NOTE: This section describes an experimental API. Currently, no device complies
+to these rules so this might change in the future.)
+
+Reported data shall be returned as:
+ - Acceleration: 1/1000 m per s^2
+ - Rotation: 1/1000 degree per second
+
+However, for most devices the reported units are unknown (more precisely: no
+one has the time to measure them and figure them out). Therefore, user-space
+shall use abs-minimum and abs-maximum to calculate relative data and use that
+instead. Devices which return wrong units may be fixed in the future to comply
+to these rules.
+
+3.1 Accelerometer
+~~~~~~~~~~~~~~~~~
+Accelerometers measure movement acceleration of devices. Any combination of the
+three available axes can be used. Usually, all three are supported.
+
+Data is provided as absolute acceleration. A positive integer defines the
+acceleration in the direction of an axis. A negative integer defines
+acceleration in the opposite direction.
+
+The evdev ABS codes used are:
+ - ABS_ACCEL_X: X axis
+ - ABS_ACCEL_Y: Y axis
+ - ABS_ACCEL_Z: Z axis
+
+3.2 Gyroscope
+~~~~~~~~~~~~~
+A gyroscope measures rotational speed (*not* acceleration!). Any combination of
+the three available axes can be used. Usually, all three are supported.
+
+Data is provided as absolute speed. A positive integer defines the rotational
+speed in clock-wise order around a given axis. A negative integer defines it in
+counter-clock-wise order.
+
+The evdev ABS codes used are:
+ - ABS_GYRO_X: X axis (also: Pitch)
+ - ABS_GYRO_Y: Y axis (also: Roll)
+ - ABS_GYRO_Z: Z axis (also: Azimuth/Yaw)
+
+3.3 Compass
+~~~~~~~~~~~
+(NOTE: No compass device currently uses the evdev input subsystem. Thus, this
+API is only a proposal, it hasn't been implemented, yet.)
+
+A compass measures the ambient magnetic field of the three defined axes. This
+makes the data self-contained and independent of the current device position.
+Any combination of the three axes can be used. Usually all three are supported,
+otherwise, it's not really useful as a compass.
+
+Proposed evdev ABS codes are:
+ - ABS_COMPASS_X: X axis
+ - ABS_COMPASS_Y: Y axis
+ - ABS_COMPASS_Z: Z axis
+
+----------------------------------------------------------------------------
+  Written 2013 by David Herrmann <dh.herrmann@gmail.com>
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 45e9214..329aa30 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -277,7 +277,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_KEY_MIN_INTERESTING	0x71
 #define INPUT_DEVICE_ID_KEY_MAX		0x2ff
 #define INPUT_DEVICE_ID_REL_MAX		0x0f
-#define INPUT_DEVICE_ID_ABS_MAX		0x3f
+#define INPUT_DEVICE_ID_ABS_MAX		0x4f
 #define INPUT_DEVICE_ID_MSC_MAX		0x07
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 1856461..e4c3596 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -869,12 +869,19 @@ struct input_keymap_entry {
 #define ABS_MAX			0x3f
 #define ABS_CNT			(ABS_MAX+1)
 
+#define ABS_ACCEL_X		0x40	/* Accelerometer X axis */
+#define ABS_ACCEL_Y		0x41	/* Accelerometer Y axis */
+#define ABS_ACCEL_Z		0x42	/* Accelerometer Z axis */
+#define ABS_GYRO_X		0x43	/* Gyroscope X axis */
+#define ABS_GYRO_Y		0x44	/* Gyroscope Y axis */
+#define ABS_GYRO_Z		0x45	/* Gyroscope Z axis */
+
 /*
  * Due to API restrictions the legacy evdev API only supports ABS values up to
  * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
  * between ABS_MAX and ABS_MAX2.
  */
-#define ABS_MAX2		0x3f
+#define ABS_MAX2		0x4f
 #define ABS_CNT2		(ABS_MAX2+1)
 
 /*
-- 
1.8.5.1

Re: [PATCH 0/4] Input: ABS2 and friends

[David Herrmann]

Hi

On Tue, Dec 17, 2013 at 4:48 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> This implements the recently discussed ABS2 API. It's working fine on my machine
> with libevdev. Comments welcome!
>
> * Patch #1 fixes some uinput shortcomings and prepares uinput for ABS2
> * Patch #2 adds ABS2
> * Patch #3 is just a small comment-fix for #4
> * Patch #4 adds some new example ABS values in the new range
>
> Note that I have patches pending which make use of the new ABS values, but I'd
> like to get this reduced series in first.

If someone is interested in the libevdev patches, see here:
  http://cgit.freedesktop.org/~dvdhrm/libevdev/log/?h=abs2

I tested the libevdev test-suite in all combinations linux+libevdev,
linux-abs2 + libevdev, linux + libevdev-abs2, linux-abs2 +
libevdev-abs2 and all worked fine.

Thanks
David

Re: [PATCH 0/4] Input: ABS2 and friends

[simon]

> Hi
>
> This implements the recently discussed ABS2 API. It's working fine on my
> machine with libevdev. Comments welcome!

Just looking at the documentation file, I have a couple of suggestions.

1). Make a note on the direction of gravity wrt the example image. Ie what
which axis/value will be positive with the device laid flat/stationary on
a desk. Might just prevent some confusion later.

2). Calibration, Accels and Gyros are noisy in different ways. Gyros tend
to be wrong in the long term, giving some consistant bias which should be
calibrated out. Accels are noisy in the short term, but generally right
over a long period.

Am I correct that the event framework will provide time-stamping of data,
thus making computation of relative movements possible.

Simon

Re: [PATCH 0/4] Input: ABS2 and friends

[David Herrmann]

Hi

On Tue, Dec 17, 2013 at 10:28 PM,  <simon@mungewell.org> wrote:
>> Hi
>>
>> This implements the recently discussed ABS2 API. It's working fine on my
>> machine with libevdev. Comments welcome!
>
> Just looking at the documentation file, I have a couple of suggestions.
>
> 1). Make a note on the direction of gravity wrt the example image. Ie what
> which axis/value will be positive with the device laid flat/stationary on
> a desk. Might just prevent some confusion later.

Yepp, can add that to the Accelerometer section.

> 2). Calibration, Accels and Gyros are noisy in different ways. Gyros tend
> to be wrong in the long term, giving some consistant bias which should be
> calibrated out. Accels are noisy in the short term, but generally right
> over a long period.

I'm not entirely sure where to put calibration/normalization. Letting
user-space deal with that should simplify things a lot. We could even
add it to libevdev. Comments welcome..

> Am I correct that the event framework will provide time-stamping of data,
> thus making computation of relative movements possible.

Yes, part of the evdev API.

Thanks
David

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Antonio Ospite]

Hi David,

thanks for the update.

On Tue, 17 Dec 2013 16:48:52 +0100
David Herrmann <dh.herrmann@gmail.com> wrote:

> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
> 
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
>

Just a question: did you consider the possibility of not exposing _MAX2
and _CNT2 in a header file at all but let those be queried? Or maybe
this is not necessary if we assume that userspace programs are supposed
to know what is the MAX _they_ support?

BTW I was thinking for instance to a program compiled with ABS_MAX2 =
0x4f but working with future kernels with an increased value, it still
won't be able to support values greater than 0x4f.

> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
>

To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled
as a special case. Maybe because I don't even know what
code=0,cnt=ABS_CNT2 is used for.

> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
>

I'll check how to use that in evtest.

Just one more comment below.

> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/hid/hid-debug.c                  |  2 +-
>  drivers/hid/hid-input.c                  |  2 +-
>  drivers/input/evdev.c                    | 95 +++++++++++++++++++++++++++++++-
>  drivers/input/input.c                    | 14 ++---
>  drivers/input/keyboard/goldfish_events.c |  6 +-
>  drivers/input/keyboard/hil_kbd.c         |  2 +-
>  drivers/input/misc/uinput.c              |  6 +-
>  include/linux/hid.h                      |  2 +-
>  include/linux/input.h                    |  6 +-
>  include/uapi/linux/input.h               | 42 +++++++++++++-
>  include/uapi/linux/uinput.h              |  2 +-
>  11 files changed, 155 insertions(+), 24 deletions(-)
> 

[...]
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index bd24470..1856461 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
[...]
>  
>  /*
> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> + * between ABS_MAX and ABS_MAX2.
> + */
> +#define ABS_MAX2		0x3f
> +#define ABS_CNT2		(ABS_MAX2+1)
> +

Maybe it's just my English, but when you say "between ABS_MAX and
ABS_MAX2" it sounds like the old protocol still _must_ be used for
values <= ABS_MAX.

IIUC this is true "only" for compatibility with older kernels right?
If a program decides to support only newer kernels it can check the
protocol version and use only the new ioctls, right?

Maybe you can be more explicit about that in the comment?

Thanks,
   Antonio
-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 4/4] Input: add motion-tracking ABS_* bits and docs

[Antonio Ospite]

On Tue, 17 Dec 2013 16:48:54 +0100
David Herrmann <dh.herrmann@gmail.com> wrote:

> Motion sensors are getting quite common in mobile devices. To avoid
> returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
> mouse-driver, this adds separate ABS_* bits for that.
> 
> This is needed if gaming devices want to report their normal data plus
> accelerometer/gyro data. Usually, ABS_X/Y are already used by analog
> sticks, so need separate definitions, anyway.

What about:
	...so motion sensors need separate definitions anyway.
              ^^^^^^^^^^^^^^
It was not immediately clear to me what the "need" referred to.

> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  Documentation/input/gamepad.txt         |   7 ++
>  Documentation/input/motion-tracking.txt | 149 ++++++++++++++++++++++++++++++++
>  include/linux/mod_devicetable.h         |   2 +-
>  include/uapi/linux/input.h              |   9 +-
>  4 files changed, 165 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/input/motion-tracking.txt
> 
> diff --git a/Documentation/input/gamepad.txt b/Documentation/input/gamepad.txt
> index 196dc42..eeda685 100644
> --- a/Documentation/input/gamepad.txt
> +++ b/Documentation/input/gamepad.txt
> @@ -57,6 +57,9 @@ Most gamepads have the following features:
>    - Rumble
>      Many devices provide force-feedback features. But are mostly just
>      simple rumble motors.
> +  - Motion-tracking
> +    Gamepads may include motion-tracking sensors like accelerometers and
> +    gyroscopes.
>  
>  3. Detection
>  ~~~~~~~~~~~~
> @@ -153,5 +156,9 @@ Menu-Pad:
>  Rumble:
>    Rumble is adverticed as FF_RUMBLE.
>  
> +Motion-tracking:
> +  Motion-tracking is defined in ./Documentation/input/motion-tracking.txt and
> +  gamepads shall comply to the rules defined there.
> +

./Documentation/... looks like it's relative to this document, drop
the leading ./

>  ----------------------------------------------------------------------------
>    Written 2013 by David Herrmann <dh.herrmann@gmail.com>
> diff --git a/Documentation/input/motion-tracking.txt b/Documentation/input/motion-tracking.txt
> new file mode 100644
> index 0000000..0885c9a
> --- /dev/null
> +++ b/Documentation/input/motion-tracking.txt
> @@ -0,0 +1,149 @@
> +                           Motion Tracking API
> +----------------------------------------------------------------------------
> +
> +1. Intro
> +~~~~~~~~
> +Motion tracking devices produce device motion events generated from an
> +accelerometer, gyroscope or compass. This data can be returned to user-space
> +via input events. This document defines how this data is reported.
> +
> +2. Devices
> +~~~~~~~~~~
> +In this document, a "device" is one of:
> + - accelerometer
> + - gyroscope
> + - compass
> +
> +These devices returned their information via different APIs in the past. To
> +unify them and define a common API, a set of input evdev codes was created. Old
> +drivers might continue using their API, but developers are encouraged to use
> +the input evdev API for new drivers.
> +
> +2.1 Axes
> +~~~~~~~~
> +Movement data is usually returned as absolute data for the 3 axes of a device.
> +In this context, the three axes are defined as:
> + - X: Axis goes from the left to the right side of the device
> + - Y: Axis goes from the bottom to the top of the device
> + - Z: Axis goes from the back to the front of the device
> +
> +The front of a device is the side faced to the user. For a mobile-phone it
> +would be the screen. For devices without a screen, the front is usually the
> +side with the most buttons on it.
> +
> +                           Example: Mobile-Phone
> +  +-------------------------------------------------------------------------+
> +  |                      TOP                                                |
> +  |                                                                         |
> +  |                                                                         |
> +  |          +---------------------------+                                  |
> +  |          |\  ________________________ \      .__                        |
> +  |          \ \ \                       \ \     |\                         |
> +  |           \ \ \              __       \ \      \                   RIGHT|
> +  |            \ \ \              /|       \ \      \__                     |
> +  |             \ \ \          __/          \ \     |\                      |
> +  |              \ \ \          /|           \ \      \ (Y Axis)            |
> +  |               \ \ \      __/  (Z axis)    \ \      \__                  |
> +  |                \ \ \      /|               \ \     |\                   |
> +  | LEFT            \ \ \    /                  \ \      \                  |
> +  |                  \ \ \         FRONT         \ \      \                 |
> +  |                   \ \ \                       \ \                       |
> +  |                    \ \ \_______________________\ \                      |
> +  |                     \ \             ___           \                     |
> +  |                     /\ \            \__\           \                    |
> +  |                  __/  \ +---------------------------+                   |
> +  |                   /|   \|___________________________|                   |
> +  |                  / BACK                                                 |
> +  |                                      (X axis)                           |
> +  |                        ------->------->------->------->                 |
> +  |                                                                         |
> +  |                                                                         |
> +  |                                         BOTTOM                          |
> +  +-------------------------------------------------------------------------+
> +
> +Rotation-data is reported as clock-wise rotation on an axis. For a given axes,
> +the reported rotation would be:
> +                                          ___
> +                                            /|
> +                                           / | (axis)
> +                                          /
> +                                    .-**-.
> +                                   /    / \
> +                                  |    / \ | /
> +                                   \  /   \|/  (clock-wise rotation)
> +                                     /
> +                                    /
> +                                   /
> +
> +2.2 Calibration
> +~~~~~~~~~~~~~~~
> +Motion sensors are often highly sensitive and need precise calibration. Users
> +are adviced to perform neutral-point calibration themselves or to implement a
       ^^^^^^^
       advised

> +state-machine to normalize input data automatically.
> +
> +Kernel devices may perform their own calibration and/or normalization. However,
> +this is usually sparse and, if implemented, transparent to the user.
> +
> +There is currently no way to feed calibration data into the kernel in a generic
> +way. Proposals welcome!
> +
> +2.3 Units
> +~~~~~~~~~
> +(NOTE: This section describes an experimental API. Currently, no device complies
> +to these rules so this might change in the future.)
> +
> +Reported data shall be returned as:
> + - Acceleration: 1/1000 m per s^2

Do you think it is worth mentioning that most hardware tend to report
the values as relative to some multiple of G (9.8 m/s^2)?

Or maybe we can get back to that when some actual driver (Wiimote, PS3
controller) start using that part of the API.

> + - Rotation: 1/1000 degree per second
> +
> +However, for most devices the reported units are unknown (more precisely: no
> +one has the time to measure them and figure them out). Therefore, user-space
> +shall use abs-minimum and abs-maximum to calculate relative data and use that
> +instead. Devices which return wrong units may be fixed in the future to comply
> +to these rules.
> +
> +3.1 Accelerometer
> +~~~~~~~~~~~~~~~~~
> +Accelerometers measure movement acceleration of devices. Any combination of the
> +three available axes can be used. Usually, all three are supported.
> +
> +Data is provided as absolute acceleration. A positive integer defines the
> +acceleration in the direction of an axis. A negative integer defines
> +acceleration in the opposite direction.
> +
> +The evdev ABS codes used are:
> + - ABS_ACCEL_X: X axis
> + - ABS_ACCEL_Y: Y axis
> + - ABS_ACCEL_Z: Z axis
> +
> +3.2 Gyroscope
> +~~~~~~~~~~~~~
> +A gyroscope measures rotational speed (*not* acceleration!). Any combination of
> +the three available axes can be used. Usually, all three are supported.
> +
> +Data is provided as absolute speed. A positive integer defines the rotational
> +speed in clock-wise order around a given axis. A negative integer defines it in
> +counter-clock-wise order.
> +
> +The evdev ABS codes used are:
> + - ABS_GYRO_X: X axis (also: Pitch)
> + - ABS_GYRO_Y: Y axis (also: Roll)
> + - ABS_GYRO_Z: Z axis (also: Azimuth/Yaw)
> +
> +3.3 Compass
> +~~~~~~~~~~~
> +(NOTE: No compass device currently uses the evdev input subsystem. Thus, this
> +API is only a proposal, it hasn't been implemented, yet.)
> +
> +A compass measures the ambient magnetic field of the three defined axes. This
> +makes the data self-contained and independent of the current device position.
> +Any combination of the three axes can be used. Usually all three are supported,
> +otherwise, it's not really useful as a compass.
> +
> +Proposed evdev ABS codes are:
> + - ABS_COMPASS_X: X axis
> + - ABS_COMPASS_Y: Y axis
> + - ABS_COMPASS_Z: Z axis
> +
> +----------------------------------------------------------------------------
> +  Written 2013 by David Herrmann <dh.herrmann@gmail.com>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 45e9214..329aa30 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -277,7 +277,7 @@ struct pcmcia_device_id {
>  #define INPUT_DEVICE_ID_KEY_MIN_INTERESTING	0x71
>  #define INPUT_DEVICE_ID_KEY_MAX		0x2ff
>  #define INPUT_DEVICE_ID_REL_MAX		0x0f
> -#define INPUT_DEVICE_ID_ABS_MAX		0x3f
> +#define INPUT_DEVICE_ID_ABS_MAX		0x4f
>  #define INPUT_DEVICE_ID_MSC_MAX		0x07
>  #define INPUT_DEVICE_ID_LED_MAX		0x0f
>  #define INPUT_DEVICE_ID_SND_MAX		0x07
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 1856461..e4c3596 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -869,12 +869,19 @@ struct input_keymap_entry {
>  #define ABS_MAX			0x3f
>  #define ABS_CNT			(ABS_MAX+1)
>  
> +#define ABS_ACCEL_X		0x40	/* Accelerometer X axis */
> +#define ABS_ACCEL_Y		0x41	/* Accelerometer Y axis */
> +#define ABS_ACCEL_Z		0x42	/* Accelerometer Z axis */
> +#define ABS_GYRO_X		0x43	/* Gyroscope X axis */
> +#define ABS_GYRO_Y		0x44	/* Gyroscope Y axis */
> +#define ABS_GYRO_Z		0x45	/* Gyroscope Z axis */
> +
>  /*
>   * Due to API restrictions the legacy evdev API only supports ABS values up to
>   * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
>   * between ABS_MAX and ABS_MAX2.
>   */
> -#define ABS_MAX2		0x3f
> +#define ABS_MAX2		0x4f
>  #define ABS_CNT2		(ABS_MAX2+1)
>  
>  /*
> -- 
> 1.8.5.1
> 

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[David Herrmann]

Hi

On Wed, Dec 18, 2013 at 3:27 PM, Antonio Ospite
<ospite@studenti.unina.it> wrote:
> Hi David,
>
> thanks for the update.
>
> On Tue, 17 Dec 2013 16:48:52 +0100
> David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> As we painfully noticed during the 3.12 merge-window our
>> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
>> hacks to work around it but if we ever decide to increase ABS_MAX, the
>> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
>> misinterpretations in the kernel that we cannot catch.
>>
>> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
>> ioctls to get/set abs-params. They no longer encode the ABS code in the
>> ioctl number and thus allow up to 4 billion ABS codes.
>>
>
> Just a question: did you consider the possibility of not exposing _MAX2
> and _CNT2 in a header file at all but let those be queried? Or maybe
> this is not necessary if we assume that userspace programs are supposed
> to know what is the MAX _they_ support?
>
> BTW I was thinking for instance to a program compiled with ABS_MAX2 =
> 0x4f but working with future kernels with an increased value, it still
> won't be able to support values greater than 0x4f.

Why would a program that was compiled with ABS_MAX2=0x4f want to use
ABS_* values greater than 0x4f? It doesn't know of any new values so
even if it could query ABS_MAX2, it could never know anything about
the new constants. The only place where it is useful is for generic
programs that just foward ABS_* codes. But these can just ignore
ABS_MAX2 entirely and use the whole int32_t range for codes.

If there is a specific use-case that'd require a dynamic way to
retrieve ABS_MAX2, I can consider changing the API. But unless there's
such a use-case, I'd like to keep it the same as for KEY_*, SW_*, ...

>> The new API also allows to query multiple ABS values with one call. To
>> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
>> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
>> newer user-space, we ignore writes to unknown codes. Hence, if we ever
>> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
>> fine even on old kernels.
>>
>
> To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled
> as a special case. Maybe because I don't even know what
> code=0,cnt=ABS_CNT2 is used for.

ABS_MT_SLOT describes the current MT-slot that is reported via
ABS_MT_* bits. It is read-only so we cannot let user-space write to
it.

>> Note that we also need to increase EV_VERSION so user-space can reliably
>> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
>> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
>> reliably without EVIOCGVERSION.
>>
>
> I'll check how to use that in evtest.
>
> Just one more comment below.
>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  drivers/hid/hid-debug.c                  |  2 +-
>>  drivers/hid/hid-input.c                  |  2 +-
>>  drivers/input/evdev.c                    | 95 +++++++++++++++++++++++++++++++-
>>  drivers/input/input.c                    | 14 ++---
>>  drivers/input/keyboard/goldfish_events.c |  6 +-
>>  drivers/input/keyboard/hil_kbd.c         |  2 +-
>>  drivers/input/misc/uinput.c              |  6 +-
>>  include/linux/hid.h                      |  2 +-
>>  include/linux/input.h                    |  6 +-
>>  include/uapi/linux/input.h               | 42 +++++++++++++-
>>  include/uapi/linux/uinput.h              |  2 +-
>>  11 files changed, 155 insertions(+), 24 deletions(-)
>>
>
> [...]
>> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>> index bd24470..1856461 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
> [...]
>>
>>  /*
>> + * Due to API restrictions the legacy evdev API only supports ABS values up to
>> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
>> + * between ABS_MAX and ABS_MAX2.
>> + */
>> +#define ABS_MAX2             0x3f
>> +#define ABS_CNT2             (ABS_MAX2+1)
>> +
>
> Maybe it's just my English, but when you say "between ABS_MAX and
> ABS_MAX2" it sounds like the old protocol still _must_ be used for
> values <= ABS_MAX.
>
> IIUC this is true "only" for compatibility with older kernels right?
> If a program decides to support only newer kernels it can check the
> protocol version and use only the new ioctls, right?
>
> Maybe you can be more explicit about that in the comment?

See the comment on "struct input_absinfo2". It describes the API, this
comment just describes the ABS_* values. But if anyone has a better
phrase to use here, I will gladly adjust it.

Thanks
David

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Dmitry Torokhov]

On Tue, Dec 17, 2013 at 04:48:52PM +0100, David Herrmann wrote:
> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
> 
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
> 
> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
> 
> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/hid/hid-debug.c                  |  2 +-
>  drivers/hid/hid-input.c                  |  2 +-
>  drivers/input/evdev.c                    | 95 +++++++++++++++++++++++++++++++-
>  drivers/input/input.c                    | 14 ++---
>  drivers/input/keyboard/goldfish_events.c |  6 +-
>  drivers/input/keyboard/hil_kbd.c         |  2 +-
>  drivers/input/misc/uinput.c              |  6 +-
>  include/linux/hid.h                      |  2 +-
>  include/linux/input.h                    |  6 +-
>  include/uapi/linux/input.h               | 42 +++++++++++++-
>  include/uapi/linux/uinput.h              |  2 +-
>  11 files changed, 155 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index 8453214..d32fa30 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -862,7 +862,7 @@ static const char *relatives[REL_MAX + 1] = {
>  	[REL_WHEEL] = "Wheel",		[REL_MISC] = "Misc",
>  };
>  
> -static const char *absolutes[ABS_CNT] = {
> +static const char *absolutes[ABS_CNT2] = {
>  	[ABS_X] = "X",			[ABS_Y] = "Y",
>  	[ABS_Z] = "Z",			[ABS_RX] = "Rx",
>  	[ABS_RY] = "Ry",		[ABS_RZ] = "Rz",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d97f232..a02721c 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1300,7 +1300,7 @@ static bool hidinput_has_been_populated(struct hid_input *hidinput)
>  	for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
>  		r |= hidinput->input->relbit[i];
>  
> -	for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
> +	for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
>  		r |= hidinput->input->absbit[i];

I wonder if kittens will die if we do:

in include/uapi/linux/input:

#define ABS_MAX1	0x3f
#define ABS_MAX2	0xYY

/*
 * Keep old value of ABS_MAX exported to userspace. Users ready to
 * handle bigger range will have to use ABS_MAX2 explicitly.
 */
#define ABS_MAX		ABS_MAX1

in include/linux/input.h

...
/* Kernel should use the new ABS range by default */
#undef ABS_MAX
#define ABS_MAX		ABS_MAX2

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 03:44:48PM +0100, David Herrmann wrote:
> >>
> >>  /*
> >> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> >> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> >> + * between ABS_MAX and ABS_MAX2.
> >> + */
> >> +#define ABS_MAX2             0x3f
> >> +#define ABS_CNT2             (ABS_MAX2+1)
> >> +
> >
> > Maybe it's just my English, but when you say "between ABS_MAX and
> > ABS_MAX2" it sounds like the old protocol still _must_ be used for
> > values <= ABS_MAX.
> >
> > IIUC this is true "only" for compatibility with older kernels right?
> > If a program decides to support only newer kernels it can check the
> > protocol version and use only the new ioctls, right?
> >
> > Maybe you can be more explicit about that in the comment?
> 
> See the comment on "struct input_absinfo2". It describes the API, this
> comment just describes the ABS_* values. But if anyone has a better
> phrase to use here, I will gladly adjust it.

"Use the extended *ABS2 ioctls to access the full range of ABS values
supported by the kernel."

?

-- 
Dmitry

Re: [PATCH 1/4] Input: uinput: add full absinfo support

[Peter Hutterer]

On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> We currently lack support for abs-resolution and abs-value parameters
> during uinput ABS initialization. Furthermore, our parsers don't allow
> growing ABS_CNT values. Therefore, introduce uinput_user_dev2.
> 
> User-space is free to write uinput_user_dev2 objects instead of
> uinput_user_dev legacy objects now. If the kernel lacks support for it,
> our comparison for "count != sizeof(struct uinput_user_dev)" will catch
> this and return EINVAL. User-space shall retry with the legacy mode then.
> 
> Internally, we transform the legacy object into uinput_user_dev2 and then
> handle both the same way.
> 
> The new uinput_user_dev2 object has multiple new features:
>  - abs payload now has "value" and "resolution" parameters as part of the
>    switch to "struct input_absinfo". We simply copy these over.
>  - Our parser allows growing ABS_CNT. We automatically detect the payload
>    size of the caller, thus, calculating the ABS_CNT the program was
>    compiled with.
>  - A "version" field to denote the uinput-version used. This is required
>    to properly support changing "struct input_user_dev2" changes in the
>    future. Due to the dynamic ABS_CNT support, we cannot simply add new
>    fields, as we cannot deduce the structure size from the user-space
>    given size. Thus, we need the "version" field to allow changing the
>    object and properly detecting it in our write() handler.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/input/misc/uinput.c | 142 ++++++++++++++++++++++++++++++++------------
>  include/uapi/linux/uinput.h |   9 +++
>  2 files changed, 114 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 7728359..927ad9a 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -358,14 +358,16 @@ static int uinput_allocate_device(struct uinput_device *udev)
>  }
>  
>  static int uinput_setup_device(struct uinput_device *udev,
> -			       const char __user *buffer, size_t count)
> +			       struct uinput_user_dev2 *user_dev2,
> +			       size_t abscnt)
>  {
> -	struct uinput_user_dev	*user_dev;
>  	struct input_dev	*dev;
>  	int			i;
>  	int			retval;
> +	struct input_absinfo	*abs;
>  
> -	if (count != sizeof(struct uinput_user_dev))
> +	/* Ensure name is filled in */
> +	if (!user_dev2->name[0])
>  		return -EINVAL;
>  
>  	if (!udev->dev) {
> @@ -375,37 +377,27 @@ static int uinput_setup_device(struct uinput_device *udev,
>  	}
>  
>  	dev = udev->dev;
> -
> -	user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
> -	if (IS_ERR(user_dev))
> -		return PTR_ERR(user_dev);
> -
> -	udev->ff_effects_max = user_dev->ff_effects_max;
> -
> -	/* Ensure name is filled in */
> -	if (!user_dev->name[0]) {
> -		retval = -EINVAL;
> -		goto exit;
> -	}
> +	udev->ff_effects_max = user_dev2->ff_effects_max;
>  
>  	kfree(dev->name);
> -	dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE,
> +	dev->name = kstrndup(user_dev2->name, UINPUT_MAX_NAME_SIZE,
>  			     GFP_KERNEL);
> -	if (!dev->name) {
> -		retval = -ENOMEM;
> -		goto exit;
> -	}
> -
> -	dev->id.bustype	= user_dev->id.bustype;
> -	dev->id.vendor	= user_dev->id.vendor;
> -	dev->id.product	= user_dev->id.product;
> -	dev->id.version	= user_dev->id.version;
> +	if (!dev->name)
> +		return -ENOMEM;
>  
> -	for (i = 0; i < ABS_CNT; i++) {
> -		input_abs_set_max(dev, i, user_dev->absmax[i]);
> -		input_abs_set_min(dev, i, user_dev->absmin[i]);
> -		input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
> -		input_abs_set_flat(dev, i, user_dev->absflat[i]);
> +	dev->id.bustype = user_dev2->id.bustype;
> +	dev->id.vendor = user_dev2->id.vendor;
> +	dev->id.product = user_dev2->id.product;
> +	dev->id.version = user_dev2->id.version;
> +
> +	for (i = 0; i < abscnt; i++) {
> +		abs = &user_dev2->abs[i];

minor nit: I'd just declare abs here.

> +		input_abs_set_val(dev, i, abs->value);
> +		input_abs_set_max(dev, i, abs->maximum);
> +		input_abs_set_min(dev, i, abs->minimum);
> +		input_abs_set_fuzz(dev, i, abs->fuzz);
> +		input_abs_set_flat(dev, i, abs->flat);
> +		input_abs_set_res(dev, i, abs->resolution);
>  	}
>  
>  	/* check if absmin/absmax/absfuzz/absflat are filled as
> @@ -413,7 +405,7 @@ static int uinput_setup_device(struct uinput_device *udev,
>  	if (test_bit(EV_ABS, dev->evbit)) {
>  		retval = uinput_validate_absbits(dev);
>  		if (retval < 0)
> -			goto exit;
> +			return retval;
>  		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);
> @@ -423,11 +415,84 @@ static int uinput_setup_device(struct uinput_device *udev,
>  	}
>  
>  	udev->state = UIST_SETUP_COMPLETE;
> -	retval = count;
> +	return 0;
> +}
> +
> +static int uinput_setup_device1(struct uinput_device *udev,
> +				const char __user *buffer, size_t count)
> +{
> +	struct uinput_user_dev	*user_dev;
> +	struct uinput_user_dev2	*user_dev2;
> +	int			i;
> +	int			retval;
> +
> +	if (count != sizeof(struct uinput_user_dev))
> +		return -EINVAL;
> +
> +	user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
> +	if (IS_ERR(user_dev))
> +		return PTR_ERR(user_dev);
> +
> +	user_dev2 = kmalloc(sizeof(*user_dev2), GFP_KERNEL);
> +	if (!user_dev2) {
> +		kfree(user_dev);
> +		return -ENOMEM;
> +	}
> +
> +	user_dev2->version = UINPUT_VERSION;
> +	memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE);
> +	memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id));

you copy the id bits one-by-one into the input_dev but you memcpy it here.
is this intentional?

> +	user_dev2->ff_effects_max = user_dev->ff_effects_max;
> +
> +	for (i = 0; i < ABS_CNT; ++i) {
> +		user_dev2->abs[i].value = 0;
> +		user_dev2->abs[i].maximum = user_dev->absmax[i];
> +		user_dev2->abs[i].minimum = user_dev->absmin[i];
> +		user_dev2->abs[i].fuzz = user_dev->absfuzz[i];
> +		user_dev2->abs[i].flat = user_dev->absflat[i];
> +		user_dev2->abs[i].resolution = 0;
> +	}
> +
> +	retval = uinput_setup_device(udev, user_dev2, ABS_CNT);
>  
> - exit:
>  	kfree(user_dev);
> -	return retval;
> +	kfree(user_dev2);
> +
> +	return retval ? retval : count;
> +}
> +
> +static int uinput_setup_device2(struct uinput_device *udev,
> +			       const char __user *buffer, size_t count)
> +{
> +	struct uinput_user_dev2	*user_dev2;
> +	int			retval;
> +	size_t			off, abscnt, max;
> +
> +	/* The first revision of "uinput_user_dev2" is bigger than
> +	 * "uinput_user_dev" and growing. Disallow any smaller payloads. */
> +	if (count <= sizeof(struct uinput_user_dev))
> +		return -EINVAL;
> +
> +	/* rough check to avoid huge kernel space allocations */
> +	max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> +	if (count > max)
> +		return -EINVAL;
> +
> +	user_dev2 = memdup_user(buffer, count);
> +	if (IS_ERR(user_dev2))
> +		return PTR_ERR(user_dev2);
> +
> +	if (user_dev2->version > UINPUT_VERSION) {
> +		retval = -EINVAL;
> +	} else {
> +		off = offsetof(struct uinput_user_dev2, abs);
> +		abscnt = (count - off) / sizeof(*user_dev2->abs);
> +		retval = uinput_setup_device(udev, user_dev2, abscnt);
> +	}
> +

I really wish uinput would be a bit easier to debug than just returning
-EINVAL when it's not happy. having said that, the only errno that would
remotely make sense is -ERANGE for count > max and even that is a bit meh.

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
   Peter




> +	kfree(user_dev2);
> +
> +	return retval ? retval : count;
>  }
>  
>  static ssize_t uinput_inject_events(struct uinput_device *udev,
> @@ -469,9 +534,12 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
>  	if (retval)
>  		return retval;
>  
> -	retval = udev->state == UIST_CREATED ?
> -			uinput_inject_events(udev, buffer, count) :
> -			uinput_setup_device(udev, buffer, count);
> +	if (udev->state == UIST_CREATED)
> +		retval = uinput_inject_events(udev, buffer, count);
> +	else if (count <= sizeof(struct uinput_user_dev))
> +		retval = uinput_setup_device1(udev, buffer, count);
> +	else
> +		retval = uinput_setup_device2(udev, buffer, count);
>  
>  	mutex_unlock(&udev->mutex);
>  
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index fe46431..c2e8710 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -134,4 +134,13 @@ struct uinput_user_dev {
>  	__s32 absfuzz[ABS_CNT];
>  	__s32 absflat[ABS_CNT];
>  };
> +
> +struct uinput_user_dev2 {
> +	__u8 version;
> +	char name[UINPUT_MAX_NAME_SIZE];
> +	struct input_id id;
> +	__u32 ff_effects_max;
> +	struct input_absinfo abs[ABS_CNT];
> +};
> +
>  #endif /* _UAPI__UINPUT_H_ */
> -- 
> 1.8.5.1
> 

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Antonio Ospite]

On Wed, 18 Dec 2013 15:44:48 +0100
David Herrmann <dh.herrmann@gmail.com> wrote:

> Hi
> 
> On Wed, Dec 18, 2013 at 3:27 PM, Antonio Ospite
> <ospite@studenti.unina.it> wrote:
> > Hi David,
> >
> > thanks for the update.
> >
> > On Tue, 17 Dec 2013 16:48:52 +0100
> > David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> As we painfully noticed during the 3.12 merge-window our
> >> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> >> hacks to work around it but if we ever decide to increase ABS_MAX, the
> >> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> >> misinterpretations in the kernel that we cannot catch.
> >>
> >> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> >> ioctls to get/set abs-params. They no longer encode the ABS code in the
> >> ioctl number and thus allow up to 4 billion ABS codes.
> >>
> >
> > Just a question: did you consider the possibility of not exposing _MAX2
> > and _CNT2 in a header file at all but let those be queried? Or maybe
> > this is not necessary if we assume that userspace programs are supposed
> > to know what is the MAX _they_ support?
> >
> > BTW I was thinking for instance to a program compiled with ABS_MAX2 =
> > 0x4f but working with future kernels with an increased value, it still
> > won't be able to support values greater than 0x4f.
> 
> Why would a program that was compiled with ABS_MAX2=0x4f want to use
> ABS_* values greater than 0x4f? It doesn't know of any new values so
> even if it could query ABS_MAX2, it could never know anything about
> the new constants. The only place where it is useful is for generic
> programs that just foward ABS_* codes. But these can just ignore
> ABS_MAX2 entirely and use the whole int32_t range for codes.
>

OK, that makes sense, thanks.

> If there is a specific use-case that'd require a dynamic way to
> retrieve ABS_MAX2, I can consider changing the API. But unless there's
> such a use-case, I'd like to keep it the same as for KEY_*, SW_*, ...
> 
> >> The new API also allows to query multiple ABS values with one call. To
> >> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> >> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> >> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> >> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> >> fine even on old kernels.
> >>
> >
> > To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled
> > as a special case. Maybe because I don't even know what
> > code=0,cnt=ABS_CNT2 is used for.
> 
> ABS_MT_SLOT describes the current MT-slot that is reported via
> ABS_MT_* bits. It is read-only so we cannot let user-space write to
> it.
>

I see.

> >> Note that we also need to increase EV_VERSION so user-space can reliably
> >> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> >> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> >> reliably without EVIOCGVERSION.
> >>
> >
> > I'll check how to use that in evtest.
> >
> > Just one more comment below.
> >
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >>  drivers/hid/hid-debug.c                  |  2 +-
> >>  drivers/hid/hid-input.c                  |  2 +-
> >>  drivers/input/evdev.c                    | 95 +++++++++++++++++++++++++++++++-
> >>  drivers/input/input.c                    | 14 ++---
> >>  drivers/input/keyboard/goldfish_events.c |  6 +-
> >>  drivers/input/keyboard/hil_kbd.c         |  2 +-
> >>  drivers/input/misc/uinput.c              |  6 +-
> >>  include/linux/hid.h                      |  2 +-
> >>  include/linux/input.h                    |  6 +-
> >>  include/uapi/linux/input.h               | 42 +++++++++++++-
> >>  include/uapi/linux/uinput.h              |  2 +-
> >>  11 files changed, 155 insertions(+), 24 deletions(-)
> >>
> >
> > [...]
> >> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> >> index bd24470..1856461 100644
> >> --- a/include/uapi/linux/input.h
> >> +++ b/include/uapi/linux/input.h
> > [...]
> >>
> >>  /*
> >> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> >> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> >> + * between ABS_MAX and ABS_MAX2.
> >> + */
> >> +#define ABS_MAX2             0x3f
> >> +#define ABS_CNT2             (ABS_MAX2+1)
> >> +
> >
> > Maybe it's just my English, but when you say "between ABS_MAX and
> > ABS_MAX2" it sounds like the old protocol still _must_ be used for
> > values <= ABS_MAX.
> >
> > IIUC this is true "only" for compatibility with older kernels right?
> > If a program decides to support only newer kernels it can check the
> > protocol version and use only the new ioctls, right?
> >
> > Maybe you can be more explicit about that in the comment?
> 
> See the comment on "struct input_absinfo2". It describes the API, this
> comment just describes the ABS_* values. But if anyone has a better
> phrase to use here, I will gladly adjust it.
> 

That comment says:

	EVIOC[G/S]ABS2 ioctls [...] do the same as the old EVIOC
	[G/S]ABS ioctls but...

My bad, I was confused by the fact that the new ioctls are presented as
a replacement for the old ones, but then I interpreted the later comment
as suggesting to use them only for values between ABS_MAX and ABS_MAX2.

Looking at how you use both mechanisms combined in libevdev it is clear
they can be used together for backward compatibility.

I like keywords, so I'd stress in the commit message that the new
protocol can either _replace_ or _extend_ the old one.

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Peter Hutterer]

On Tue, Dec 17, 2013 at 04:48:52PM +0100, David Herrmann wrote:
> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
> 
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
> 
> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
> 
> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/hid/hid-debug.c                  |  2 +-
>  drivers/hid/hid-input.c                  |  2 +-
>  drivers/input/evdev.c                    | 95 +++++++++++++++++++++++++++++++-
>  drivers/input/input.c                    | 14 ++---
>  drivers/input/keyboard/goldfish_events.c |  6 +-
>  drivers/input/keyboard/hil_kbd.c         |  2 +-
>  drivers/input/misc/uinput.c              |  6 +-
>  include/linux/hid.h                      |  2 +-
>  include/linux/input.h                    |  6 +-
>  include/uapi/linux/input.h               | 42 +++++++++++++-
>  include/uapi/linux/uinput.h              |  2 +-
>  11 files changed, 155 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index 8453214..d32fa30 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -862,7 +862,7 @@ static const char *relatives[REL_MAX + 1] = {
>  	[REL_WHEEL] = "Wheel",		[REL_MISC] = "Misc",
>  };
>  
> -static const char *absolutes[ABS_CNT] = {
> +static const char *absolutes[ABS_CNT2] = {
>  	[ABS_X] = "X",			[ABS_Y] = "Y",
>  	[ABS_Z] = "Z",			[ABS_RX] = "Rx",
>  	[ABS_RY] = "Ry",		[ABS_RZ] = "Rz",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d97f232..a02721c 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1300,7 +1300,7 @@ static bool hidinput_has_been_populated(struct hid_input *hidinput)
>  	for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
>  		r |= hidinput->input->relbit[i];
>  
> -	for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
> +	for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
>  		r |= hidinput->input->absbit[i];
>  
>  	for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index a06e125..32b74e5 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -643,7 +643,7 @@ static int handle_eviocgbit(struct input_dev *dev,
>  	case      0: bits = dev->evbit;  len = EV_MAX;  break;
>  	case EV_KEY: bits = dev->keybit; len = KEY_MAX; break;
>  	case EV_REL: bits = dev->relbit; len = REL_MAX; break;
> -	case EV_ABS: bits = dev->absbit; len = ABS_MAX; break;
> +	case EV_ABS: bits = dev->absbit; len = ABS_MAX2; break;
>  	case EV_MSC: bits = dev->mscbit; len = MSC_MAX; break;
>  	case EV_LED: bits = dev->ledbit; len = LED_MAX; break;
>  	case EV_SND: bits = dev->sndbit; len = SND_MAX; break;
> @@ -671,6 +671,93 @@ static int handle_eviocgbit(struct input_dev *dev,
>  }
>  #undef OLD_KEY_MAX
>  
> +static int evdev_handle_get_abs2(struct input_dev *dev, void __user *p)
> +{
> +	u32 code, cnt, valid_cnt, i;
> +	struct input_absinfo2 __user *pinfo = p;
> +	struct input_absinfo abs;
> +
> +	if (copy_from_user(&code, &pinfo->code, sizeof(code)))
> +		return -EFAULT;
> +	if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
> +		return -EFAULT;
> +	if (!cnt)
> +		return 0;
> +
> +	if (!dev->absinfo)
> +		valid_cnt = 0;
> +	else if (code > ABS_MAX2)
> +		valid_cnt = 0;
> +	else if (code + cnt <= code || code + cnt > ABS_MAX2)
> +		valid_cnt = ABS_MAX2 - code + 1;
> +	else
> +		valid_cnt = cnt;
> +
> +	for (i = 0; i < valid_cnt; ++i) {
> +		/*
> +		 * Take event lock to ensure that we are not
> +		 * copying data while EVIOCSABS2 changes it.
> +		 * Might be inconsistent, otherwise.
> +		 */
> +		spin_lock_irq(&dev->event_lock);
> +		abs = dev->absinfo[code + i];
> +		spin_unlock_irq(&dev->event_lock);
> +
> +		if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> +			return -EFAULT;
> +	}
> +
> +	memset(&abs, 0, sizeof(abs));
> +	for (i = valid_cnt; i < cnt; ++i)
> +		if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> +			return -EFAULT;
> +
> +	return 0;

why don't you return the number of valid copied axes to the user?
that seems better even than forcing the remainder to 0.

> +}
> +
> +static int evdev_handle_set_abs2(struct input_dev *dev, void __user *p)
> +{
> +	struct input_absinfo2 __user *pinfo = p;
> +	struct input_absinfo *abs;
> +	u32 code, cnt, i;
> +	size_t size;
> +
> +	if (!dev->absinfo)
> +		return 0;
> +	if (copy_from_user(&code, &pinfo->code, sizeof(code)))
> +		return -EFAULT;
> +	if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
> +		return -EFAULT;
> +	if (!cnt || code > ABS_MAX2)
> +		return 0;
> +
> +	if (code + cnt <= code || code + cnt > ABS_MAX2)
> +		cnt = ABS_MAX2 - code + 1;
> +
> +	size = cnt * sizeof(*abs);
> +	abs = memdup_user(pinfo->info, size);
> +	if (IS_ERR(abs))
> +		return PTR_ERR(abs);
> +
> +	/*
> +	 * Take event lock to ensure that we are not
> +	 * changing device parameters in the middle
> +	 * of event.
> +	 */
> +	spin_lock_irq(&dev->event_lock);
> +	for (i = 0; i < cnt; ++i) {
> +		/* silently drop ABS_MT_SLOT */
> +		if (code + i == ABS_MT_SLOT)
> +			continue;
> +
> +		dev->absinfo[code + i] = abs[i];
> +	}
> +	spin_unlock_irq(&dev->event_lock);
> +
> +	kfree(abs);
> +	return 0;
> +}
> +
>  static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
>  {
>  	struct input_keymap_entry ke = {
> @@ -898,6 +985,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  		client->clkid = i;
>  		return 0;
>  
> +	case EVIOCGABS2:
> +		return evdev_handle_get_abs2(dev, p);
> +
> +	case EVIOCSABS2:
> +		return evdev_handle_set_abs2(dev, p);
> +
>  	case EVIOCGKEYCODE:
>  		return evdev_handle_get_keycode(dev, p);
>  

[...]

> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 927ad9a..4660ed1 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -311,7 +311,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
>  	unsigned int cnt;
>  	int retval = 0;
>  
> -	for (cnt = 0; cnt < ABS_CNT; cnt++) {
> +	for (cnt = 0; cnt < ABS_CNT2; cnt++) {
>  		int min, max;
>  		if (!test_bit(cnt, dev->absbit))
>  			continue;
> @@ -474,7 +474,7 @@ static int uinput_setup_device2(struct uinput_device *udev,
>  		return -EINVAL;
>  
>  	/* rough check to avoid huge kernel space allocations */
> -	max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> +	max = ABS_CNT2 * sizeof(*user_dev2->abs) + sizeof(*user_dev2);

hmm, if you ever up the value of ABS_CNT2 and you don't have it query-able,
userspace won't be able to create a uinput device on a kernel with a smaller
ABS_CNT2.  worst of all, the only error you get is EINVAL, which is also
used for invalid axis ranges, etc.

>  	if (count > max)
>  		return -EINVAL;
>  
> @@ -780,7 +780,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  			break;
>  
>  		case UI_SET_ABSBIT:
> -			retval = uinput_set_bit(arg, absbit, ABS_MAX);
> +			retval = uinput_set_bit(arg, absbit, ABS_MAX2);
>  			break;
>  
>  		case UI_SET_MSCBIT:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 31b9d29..c21d8bb 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -828,7 +828,7 @@ static inline void hid_map_usage(struct hid_input *hidinput,
>  	switch (type) {
>  	case EV_ABS:
>  		*bit = input->absbit;
> -		*max = ABS_MAX;
> +		*max = ABS_MAX2;
>  		break;
>  	case EV_REL:
>  		*bit = input->relbit;
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 82ce323..c6add6f 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -129,7 +129,7 @@ struct input_dev {
>  	unsigned long evbit[BITS_TO_LONGS(EV_CNT)];
>  	unsigned long keybit[BITS_TO_LONGS(KEY_CNT)];
>  	unsigned long relbit[BITS_TO_LONGS(REL_CNT)];
> -	unsigned long absbit[BITS_TO_LONGS(ABS_CNT)];
> +	unsigned long absbit[BITS_TO_LONGS(ABS_CNT2)];
>  	unsigned long mscbit[BITS_TO_LONGS(MSC_CNT)];
>  	unsigned long ledbit[BITS_TO_LONGS(LED_CNT)];
>  	unsigned long sndbit[BITS_TO_LONGS(SND_CNT)];
> @@ -210,8 +210,8 @@ struct input_dev {
>  #error "REL_MAX and INPUT_DEVICE_ID_REL_MAX do not match"
>  #endif
>  
> -#if ABS_MAX != INPUT_DEVICE_ID_ABS_MAX
> -#error "ABS_MAX and INPUT_DEVICE_ID_ABS_MAX do not match"
> +#if ABS_MAX2 != INPUT_DEVICE_ID_ABS_MAX
> +#error "ABS_MAX2 and INPUT_DEVICE_ID_ABS_MAX do not match"
>  #endif
>  
>  #if MSC_MAX != INPUT_DEVICE_ID_MSC_MAX
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index bd24470..1856461 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

A history in the comments would be nice. something like

/**
 * 0x010000: original version
 * 0x010001: support for long scancodes
 * 0x010002: added ABS_CNT2/ABS_MAX2, EVIOCSABS2, EVIOCGABS2
 */


>  /*
>   * IOCTLs (0x00 - 0x7f)
> @@ -74,6 +74,30 @@ struct input_absinfo {
>  };
>  
>  /**
> + * struct input_absinfo2 - used by EVIOC[G/S]ABS2 ioctls

please spell the two out (at least once in this paragraph), it makes it grep-able.

> + * @code: First ABS code to query
> + * @cnt: Number of ABS codes to query starting at @code
> + * @info: #@cnt absinfo structures to get/set abs parameters for all codes
> + *
> + * This structure is used by the new EVIOC[G/S]ABS2 ioctls which
> + * do the same as the old EVIOC[G/S]ABS ioctls but avoid encoding
> + * the ABS code in the ioctl number. This allows a much wider
> + * range of ABS codes. Furthermore, it allows to query multiple codes with a
> + * single call.

"new" and "old" have a tendency to become "old" and "before old" quickly,
just skip both. A simple "use of EVIOCGABS/EVIOCSABS is discouraged except on kernels
without EVIOCGABS2/EVIOCSABS2 support" is enough to signal which one is preferred.

> + *
> + * Note that this silently drops any requests to set ABS_MT_SLOT. Hence, it is
> + * allowed to call this with code=0 cnt=ABS_CNT2. Furthermore, retrieving
> + * invalid codes returns all 0, setting them does nothing. So you must check
> + * with EVIOCGBIT first if you want reliable results. This behavior is needed
> + * to allow forward compatibility to new ABS codes.

I think this needs rewording, I was quite confused reading this. How about:

"For axes not present on the device and for axes exceeding the kernel's
built-in ABS_CNT2 maximum, EVIOCGABS2 sets all values in the struct absinfo
to 0. EVIOCGSABS2 silently ignores write requests to these axes.
ABS_MT_SlOT is an immutable axis and cannot be modified by EVIOCSABS2, 
the respective value is silently ignored."

also, please document the return value of the ioctl.

Cheers,
   Peter

> + */
> +struct input_absinfo2 {
> +	__u32 code;
> +	__u32 cnt;
> +	struct input_absinfo info[1];
> +};
> +
> +/**
>   * struct input_keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
>   * @scancode: scancode represented in machine-endian form.
>   * @len: length of the scancode that resides in @scancode buffer.
> @@ -153,6 +177,8 @@ struct input_keymap_entry {
>  
>  #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
>  #define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
> +#define EVIOCGABS2		_IOR('E', 0x92, struct input_absinfo2)	/* get abs value/limits */
> +#define EVIOCSABS2		_IOW('E', 0x93, struct input_absinfo2)	/* set abs value/limits */
>  
>  #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
>  
> @@ -835,11 +861,23 @@ struct input_keymap_entry {
>  #define ABS_MT_TOOL_X		0x3c	/* Center X tool position */
>  #define ABS_MT_TOOL_Y		0x3d	/* Center Y tool position */
>  
> -
> +/*
> + * ABS_MAX/CNT is limited to a maximum of 0x3f due to the design of EVIOCGABS
> + * and EVIOCSABS ioctls. Other kernel APIs like uinput also hardcoded it. Do
> + * not modify this value and instead use the extended ABS_MAX2/CNT2 API.
> + */
>  #define ABS_MAX			0x3f
>  #define ABS_CNT			(ABS_MAX+1)
>  
>  /*
> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> + * between ABS_MAX and ABS_MAX2.
> + */
> +#define ABS_MAX2		0x3f
> +#define ABS_CNT2		(ABS_MAX2+1)
> +
> +/*
>   * Switch events
>   */
>  
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c2e8710..27ee521 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -140,7 +140,7 @@ struct uinput_user_dev2 {
>  	char name[UINPUT_MAX_NAME_SIZE];
>  	struct input_id id;
>  	__u32 ff_effects_max;
> -	struct input_absinfo abs[ABS_CNT];
> +	struct input_absinfo abs[ABS_CNT2];
>  };
>  
>  #endif /* _UAPI__UINPUT_H_ */
> -- 
> 1.8.5.1
> 

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Dmitry Torokhov]

On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > +     memset(&abs, 0, sizeof(abs));
> > +     for (i = valid_cnt; i < cnt; ++i)
> > +             if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > +                     return -EFAULT;
> > +
> > +     return 0;
> 
> why don't you return the number of valid copied axes to the user?
> that seems better even than forcing the remainder to 0.

Well, if your program messed up buffers that it faulted we do not know
for sure if data that did not cause fault ended up where it should have
or if it smashed something else. This condition I think should be
signaled early.

-- 
Dmitry

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Peter Hutterer]

On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > +     memset(&abs, 0, sizeof(abs));
> > > +     for (i = valid_cnt; i < cnt; ++i)
> > > +             if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > +                     return -EFAULT;
> > > +
> > > +     return 0;
> > 
> > why don't you return the number of valid copied axes to the user?
> > that seems better even than forcing the remainder to 0.
> 
> Well, if your program messed up buffers that it faulted we do not know
> for sure if data that did not cause fault ended up where it should have
> or if it smashed something else. This condition I think should be
> signaled early.

not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
was proposing to replace "return 0" with "return valid_cnt".

Cheers,
   Peter

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Dmitry Torokhov]

On Thu, Dec 19, 2013 at 09:55:04AM +1000, Peter Hutterer wrote:
> On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> > On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > > +     memset(&abs, 0, sizeof(abs));
> > > > +     for (i = valid_cnt; i < cnt; ++i)
> > > > +             if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > > +                     return -EFAULT;
> > > > +
> > > > +     return 0;
> > > 
> > > why don't you return the number of valid copied axes to the user?
> > > that seems better even than forcing the remainder to 0.
> > 
> > Well, if your program messed up buffers that it faulted we do not know
> > for sure if data that did not cause fault ended up where it should have
> > or if it smashed something else. This condition I think should be
> > signaled early.
> 
> not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
> was proposing to replace "return 0" with "return valid_cnt".

I understand what you were saying. Now consider: your program supplied
buffer that is actually smaller than what it said to the kernel.
Depending on the exact placement we may or may not fault when we get
pass the buffer boundary, most likely not. We are likely to fault when
we go way past the buffer boundary and wracked process' memory. If we
return -EFAULT the program will at least notice that something wrong. If
we return count it will try to resubmit the remainder of operation and
not even know that there was something very bad happening.

IOW we should not treat fault condition as other partial read/write
conditions.

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Peter Hutterer]

On Wed, Dec 18, 2013 at 04:05:37PM -0800, Dmitry Torokhov wrote:
> On Thu, Dec 19, 2013 at 09:55:04AM +1000, Peter Hutterer wrote:
> > On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> > > On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > > > +     memset(&abs, 0, sizeof(abs));
> > > > > +     for (i = valid_cnt; i < cnt; ++i)
> > > > > +             if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > > > +                     return -EFAULT;
> > > > > +
> > > > > +     return 0;
> > > > 
> > > > why don't you return the number of valid copied axes to the user?
> > > > that seems better even than forcing the remainder to 0.
> > > 
> > > Well, if your program messed up buffers that it faulted we do not know
> > > for sure if data that did not cause fault ended up where it should have
> > > or if it smashed something else. This condition I think should be
> > > signaled early.
> > 
> > not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
> > was proposing to replace "return 0" with "return valid_cnt".
> 
> I understand what you were saying. Now consider: your program supplied
> buffer that is actually smaller than what it said to the kernel.
> Depending on the exact placement we may or may not fault when we get
> pass the buffer boundary, most likely not. We are likely to fault when
> we go way past the buffer boundary and wracked process' memory. If we
> return -EFAULT the program will at least notice that something wrong. If
> we return count it will try to resubmit the remainder of operation and
> not even know that there was something very bad happening.
> 
> IOW we should not treat fault condition as other partial read/write
> conditions.

I'm still not sure we're talking about the same thing :)
let me rephrase: why can't we use the behaviour bits_to_user() provides?
it limits the output to maxlen and returns that value (or -EFAULT), it's
only a small step from that to limit the output to min(maxbit, ABS_CNT2).

Cheers,
   Peter

 

Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

[Dmitry Torokhov]

On Thu, Dec 19, 2013 at 10:25:42AM +1000, Peter Hutterer wrote:
> On Wed, Dec 18, 2013 at 04:05:37PM -0800, Dmitry Torokhov wrote:
> > On Thu, Dec 19, 2013 at 09:55:04AM +1000, Peter Hutterer wrote:
> > > On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> > > > On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > > > > +     memset(&abs, 0, sizeof(abs));
> > > > > > +     for (i = valid_cnt; i < cnt; ++i)
> > > > > > +             if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > > > > +                     return -EFAULT;
> > > > > > +
> > > > > > +     return 0;
> > > > > 
> > > > > why don't you return the number of valid copied axes to the user?
> > > > > that seems better even than forcing the remainder to 0.
> > > > 
> > > > Well, if your program messed up buffers that it faulted we do not know
> > > > for sure if data that did not cause fault ended up where it should have
> > > > or if it smashed something else. This condition I think should be
> > > > signaled early.
> > > 
> > > not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
> > > was proposing to replace "return 0" with "return valid_cnt".
> > 
> > I understand what you were saying. Now consider: your program supplied
> > buffer that is actually smaller than what it said to the kernel.
> > Depending on the exact placement we may or may not fault when we get
> > pass the buffer boundary, most likely not. We are likely to fault when
> > we go way past the buffer boundary and wracked process' memory. If we
> > return -EFAULT the program will at least notice that something wrong. If
> > we return count it will try to resubmit the remainder of operation and
> > not even know that there was something very bad happening.
> > 
> > IOW we should not treat fault condition as other partial read/write
> > conditions.
> 
> I'm still not sure we're talking about the same thing :)

Hmm, it appears you are right ;)

> let me rephrase: why can't we use the behaviour bits_to_user() provides?
> it limits the output to maxlen and returns that value (or -EFAULT), it's
> only a small step from that to limit the output to min(maxbit, ABS_CNT2).

OK, makes sense.

-- 
Dmitry

Re: [PATCH 1/4] Input: uinput: add full absinfo support

[Dmitry Torokhov]

On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> +
> +struct uinput_user_dev2 {
> +	__u8 version;

It does not make sense to have version u8 since we going to have padding
(1 byte I believe) padding between name and id. 

> +	char name[UINPUT_MAX_NAME_SIZE];
> +	struct input_id id;
> +	__u32 ff_effects_max;
> +	struct input_absinfo abs[ABS_CNT];
> +};
> +
>  #endif /* _UAPI__UINPUT_H_ */
> -- 
> 1.8.5.1
> 

-- 
Dmitry

Re: [PATCH 1/4] Input: uinput: add full absinfo support

[Dmitry Torokhov]

On Thu, Dec 19, 2013 at 08:27:32AM +1000, Peter Hutterer wrote:
> On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> > +
> > +	user_dev2->version = UINPUT_VERSION;
> > +	memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE);
> > +	memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id));
> 
> you copy the id bits one-by-one into the input_dev but you memcpy it here.
> is this intentional?

That should simply be:

	user_dev2->id = user_dev->id;

and in othe rplace as well I think.

> 
> > +	user_dev2->ff_effects_max = user_dev->ff_effects_max;
> > +
> > +	for (i = 0; i < ABS_CNT; ++i) {
> > +		user_dev2->abs[i].value = 0;
> > +		user_dev2->abs[i].maximum = user_dev->absmax[i];
> > +		user_dev2->abs[i].minimum = user_dev->absmin[i];
> > +		user_dev2->abs[i].fuzz = user_dev->absfuzz[i];
> > +		user_dev2->abs[i].flat = user_dev->absflat[i];
> > +		user_dev2->abs[i].resolution = 0;
> > +	}
> > +
> > +	retval = uinput_setup_device(udev, user_dev2, ABS_CNT);
> >  
> > - exit:
> >  	kfree(user_dev);
> > -	return retval;
> > +	kfree(user_dev2);
> > +
> > +	return retval ? retval : count;
> > +}
> > +
> > +static int uinput_setup_device2(struct uinput_device *udev,
> > +			       const char __user *buffer, size_t count)
> > +{
> > +	struct uinput_user_dev2	*user_dev2;
> > +	int			retval;
> > +	size_t			off, abscnt, max;
> > +
> > +	/* The first revision of "uinput_user_dev2" is bigger than
> > +	 * "uinput_user_dev" and growing. Disallow any smaller payloads. */
> > +	if (count <= sizeof(struct uinput_user_dev))
> > +		return -EINVAL;
> > +
> > +	/* rough check to avoid huge kernel space allocations */
> > +	max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> > +	if (count > max)
> > +		return -EINVAL;
> > +
> > +	user_dev2 = memdup_user(buffer, count);
> > +	if (IS_ERR(user_dev2))
> > +		return PTR_ERR(user_dev2);
> > +
> > +	if (user_dev2->version > UINPUT_VERSION) {
> > +		retval = -EINVAL;
> > +	} else {
> > +		off = offsetof(struct uinput_user_dev2, abs);
> > +		abscnt = (count - off) / sizeof(*user_dev2->abs);
> > +		retval = uinput_setup_device(udev, user_dev2, abscnt);
> > +	}
> > +
> 
> I really wish uinput would be a bit easier to debug than just returning
> -EINVAL when it's not happy. having said that, the only errno that would
> remotely make sense is -ERANGE for count > max and even that is a bit meh.

Maybe we should add a few dev_dbg() and rely on having dynamic debug to
activate logging on demand?

Thanks.

-- 
Dmitry