[PATCH 2/2] Input: Add EVIOC mechanism for MT slots

[PATCH 2/2] Input: Add EVIOC mechanism for MT slots

[Henrik Rydberg]

This patch adds the ability to extract MT slot data via a new ioctl,
EVIOCGMTSLOTS. The function returns an array of slot values for the
specified ABS_MT event type.

Example of user space usage:

struct { struct input_mt_request req; int values[64]; } data;
data.req.code = ABS_MT_POSITION_X;
if (ioctl(fd, EVIOCGMTSLOTS(sizeof(data)), &data) < 0)
	return -1;
for (i = 0; i < data.req.num_values; i++)
	printf("slot %d: %d\n", i, data.values[i]);

Cc: Chase Douglas <chase.douglas@canonical.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
Hi Dmitry,

This is a new take on the MT slot ioctl patch. A new ioctl is added,
which takes a request structure and returns a (variable) array of slot
values. The patch seems to work, but testing has not been extensive (yet).

Cheers,
Henrik

 drivers/input/evdev.c |   30 +++++++++++++++++++++++++++++-
 include/linux/input.h |   25 +++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 4cf2534..8bf45d4 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,7 +20,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/init.h>
-#include <linux/input.h>
+#include <linux/input/mt.h>
 #include <linux/major.h>
 #include <linux/device.h>
 #include "input-compat.h"
@@ -621,6 +621,31 @@ static int evdev_handle_set_keycode_v2(struct input_dev *dev, void __user *p)
 	return input_set_keycode(dev, &ke);
 }
 
+static int evdev_handle_mt_request(struct input_dev *dev,
+				   unsigned int size,
+				   int __user *ip)
+{
+	const struct input_mt_slot *mt = dev->mt;
+	unsigned int code;
+	int i;
+
+	if (get_user(code, &ip[0]))
+		return -EFAULT;
+	if (!input_is_mt_value(code))
+		return -EINVAL;
+
+	if (size < sizeof(struct input_mt_request) + dev->mtsize * sizeof(int))
+		return -EINVAL;
+
+	if (put_user(dev->mtsize, &ip[1]))
+		return -EFAULT;
+	for (i = 0; i < dev->mtsize; i++)
+		if (put_user(input_mt_get_value(&mt[i], code), &ip[2 + i]))
+			return -EFAULT;
+
+	return 0;
+}
+
 static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			   void __user *p, int compat_mode)
 {
@@ -706,6 +731,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		return bits_to_user(dev->propbit, INPUT_PROP_MAX,
 				    size, p, compat_mode);
 
+	case EVIOCGMTSLOTS(0):
+		return evdev_handle_mt_request(dev, size, ip);
+
 	case EVIOCGKEY(0):
 		return bits_to_user(dev->key, KEY_MAX, size, p, compat_mode);
 
diff --git a/include/linux/input.h b/include/linux/input.h
index a637e78..9a73f18 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -99,6 +99,30 @@ struct input_keymap_entry {
 	__u8  scancode[32];
 };
 
+/**
+ * struct input_mt_request - used by EVIOCGMTSLOTS ioctl
+ * @code: ABS_MT code to read
+ * @num_values: number of values written to the value array
+ * @values: value array, one value per slot
+ *
+ * The structure is used to retrieve MT slot data. Before the call,
+ * code is set to the wanted ABS_MT event type. On return, the value
+ * array is filled with the slot values for the specified ABS_MT code,
+ * and num_values is set to the actual number of slots.
+ *
+ * The call argument (len) is the size of the return buffer, satisfying
+ *
+ * len >= sizeof(input_mt_request) + sizeof(__s32) * number_of_slots
+ *
+ * If the request code is not an ABS_MT value, or if len is too small,
+ * -EINVAL is returned.
+ */
+struct input_mt_request {
+	__u32 code;
+	__u32 num_values;
+	__s32 values[];
+};
+
 #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
 #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
 #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
@@ -113,6 +137,7 @@ struct input_keymap_entry {
 #define EVIOCGPHYS(len)		_IOC(_IOC_READ, 'E', 0x07, len)		/* get physical location */
 #define EVIOCGUNIQ(len)		_IOC(_IOC_READ, 'E', 0x08, len)		/* get unique identifier */
 #define EVIOCGPROP(len)		_IOC(_IOC_READ, 'E', 0x09, len)		/* get device properties */
+#define EVIOCGMTSLOTS(len)	_IOC(_IOC_READ, 'E', 0x0a, len)		/* get MT slot values */
 
 #define EVIOCGKEY(len)		_IOC(_IOC_READ, 'E', 0x18, len)		/* get global key state */
 #define EVIOCGLED(len)		_IOC(_IOC_READ, 'E', 0x19, len)		/* get all LEDs */
-- 
1.7.8.1

Re: [PATCH 2/2] Input: Add EVIOC mechanism for MT slots

[Chase Douglas]

On 01/12/2012 08:24 PM, Henrik Rydberg wrote:
> This patch adds the ability to extract MT slot data via a new ioctl,
> EVIOCGMTSLOTS. The function returns an array of slot values for the
> specified ABS_MT event type.
> 
> Example of user space usage:
> 
> struct { struct input_mt_request req; int values[64]; } data;
> data.req.code = ABS_MT_POSITION_X;
> if (ioctl(fd, EVIOCGMTSLOTS(sizeof(data)), &data) < 0)
> 	return -1;
> for (i = 0; i < data.req.num_values; i++)
> 	printf("slot %d: %d\n", i, data.values[i]);

If input_mt_request is:

struct input_mt_request {
	__u32 code;
	__u32 num_values;
	__s32 values[];
};

then how passing in the data struct above work? The passed in data would be:

__u32 code;
__u32 num_values; /* BTW, this isn't set in the example above */
__s32 *values; /* This is left uninitialized in the example above */
int values[0]
int values[1]
int values[2]
...
int values[63]

I'm not too familiar with ioctl interfaces, but I don't see how this
example could work.

> 
> Cc: Chase Douglas <chase.douglas@canonical.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
> Hi Dmitry,
> 
> This is a new take on the MT slot ioctl patch. A new ioctl is added,
> which takes a request structure and returns a (variable) array of slot
> values. The patch seems to work, but testing has not been extensive (yet).
> 
> Cheers,
> Henrik
> 
>  drivers/input/evdev.c |   30 +++++++++++++++++++++++++++++-
>  include/linux/input.h |   25 +++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 4cf2534..8bf45d4 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -20,7 +20,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> -#include <linux/input.h>
> +#include <linux/input/mt.h>
>  #include <linux/major.h>
>  #include <linux/device.h>
>  #include "input-compat.h"
> @@ -621,6 +621,31 @@ static int evdev_handle_set_keycode_v2(struct input_dev *dev, void __user *p)
>  	return input_set_keycode(dev, &ke);
>  }
>  
> +static int evdev_handle_mt_request(struct input_dev *dev,
> +				   unsigned int size,
> +				   int __user *ip)
> +{
> +	const struct input_mt_slot *mt = dev->mt;
> +	unsigned int code;
> +	int i;
> +
> +	if (get_user(code, &ip[0]))
> +		return -EFAULT;
> +	if (!input_is_mt_value(code))
> +		return -EINVAL;
> +
> +	if (size < sizeof(struct input_mt_request) + dev->mtsize * sizeof(int))
> +		return -EINVAL;

Why not fill in up to the size of the array provided?

> +	if (put_user(dev->mtsize, &ip[1]))
> +		return -EFAULT;
> +	for (i = 0; i < dev->mtsize; i++)
> +		if (put_user(input_mt_get_value(&mt[i], code), &ip[2 + i]))
> +			return -EFAULT;

This would be easier to understand if you did:

struct input_mt_request *req = (struct input_mt_request *)ip;

Then modify the stuff in a straightforward way.

> +	return 0;
> +}
> +
>  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  			   void __user *p, int compat_mode)
>  {
> @@ -706,6 +731,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  		return bits_to_user(dev->propbit, INPUT_PROP_MAX,
>  				    size, p, compat_mode);
>  
> +	case EVIOCGMTSLOTS(0):
> +		return evdev_handle_mt_request(dev, size, ip);
> +
>  	case EVIOCGKEY(0):
>  		return bits_to_user(dev->key, KEY_MAX, size, p, compat_mode);
>  
> diff --git a/include/linux/input.h b/include/linux/input.h
> index a637e78..9a73f18 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -99,6 +99,30 @@ struct input_keymap_entry {
>  	__u8  scancode[32];
>  };
>  
> +/**
> + * struct input_mt_request - used by EVIOCGMTSLOTS ioctl
> + * @code: ABS_MT code to read
> + * @num_values: number of values written to the value array
> + * @values: value array, one value per slot
> + *
> + * The structure is used to retrieve MT slot data. Before the call,
> + * code is set to the wanted ABS_MT event type. On return, the value
> + * array is filled with the slot values for the specified ABS_MT code,
> + * and num_values is set to the actual number of slots.
> + *
> + * The call argument (len) is the size of the return buffer, satisfying
> + *
> + * len >= sizeof(input_mt_request) + sizeof(__s32) * number_of_slots
> + *
> + * If the request code is not an ABS_MT value, or if len is too small,
> + * -EINVAL is returned.
> + */
> +struct input_mt_request {
> +	__u32 code;
> +	__u32 num_values;
> +	__s32 values[];
> +};
> +
>  #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
>  #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
>  #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
> @@ -113,6 +137,7 @@ struct input_keymap_entry {
>  #define EVIOCGPHYS(len)		_IOC(_IOC_READ, 'E', 0x07, len)		/* get physical location */
>  #define EVIOCGUNIQ(len)		_IOC(_IOC_READ, 'E', 0x08, len)		/* get unique identifier */
>  #define EVIOCGPROP(len)		_IOC(_IOC_READ, 'E', 0x09, len)		/* get device properties */
> +#define EVIOCGMTSLOTS(len)	_IOC(_IOC_READ, 'E', 0x0a, len)		/* get MT slot values */
>  
>  #define EVIOCGKEY(len)		_IOC(_IOC_READ, 'E', 0x18, len)		/* get global key state */
>  #define EVIOCGLED(len)		_IOC(_IOC_READ, 'E', 0x19, len)		/* get all LEDs */

Re: [PATCH 2/2] Input: Add EVIOC mechanism for MT slots

[Dmitry Torokhov]

On Fri, Jan 13, 2012 at 09:34:12AM +0100, Chase Douglas wrote:
> On 01/12/2012 08:24 PM, Henrik Rydberg wrote:
> > This patch adds the ability to extract MT slot data via a new ioctl,
> > EVIOCGMTSLOTS. The function returns an array of slot values for the
> > specified ABS_MT event type.
> > 
> > Example of user space usage:
> > 
> > struct { struct input_mt_request req; int values[64]; } data;
> > data.req.code = ABS_MT_POSITION_X;
> > if (ioctl(fd, EVIOCGMTSLOTS(sizeof(data)), &data) < 0)
> > 	return -1;
> > for (i = 0; i < data.req.num_values; i++)
> > 	printf("slot %d: %d\n", i, data.values[i]);
> 
> If input_mt_request is:
> 
> struct input_mt_request {
> 	__u32 code;
> 	__u32 num_values;
> 	__s32 values[];
> };
> 
> then how passing in the data struct above work? The passed in data would be:
> 
> __u32 code;
> __u32 num_values; /* BTW, this isn't set in the example above */
> __s32 *values; /* This is left uninitialized in the example above */
> int values[0]
> int values[1]
> int values[2]
> ...
> int values[63]
> 
> I'm not too familiar with ioctl interfaces, but I don't see how this
> example could work.

There isn't a separate __s32 *values pointer, the actual memory layout
from kernel's POV would be:

__u32 code;
__u32 num_values;
__s32 values[0];
__s32 values[1];
...
__s32 values[63];

> 
> > 
> > Cc: Chase Douglas <chase.douglas@canonical.com>
> > Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> > ---
> > Hi Dmitry,
> > 
> > This is a new take on the MT slot ioctl patch. A new ioctl is added,
> > which takes a request structure and returns a (variable) array of slot
> > values. The patch seems to work, but testing has not been extensive (yet).
> > 
> > Cheers,
> > Henrik
> > 
> >  drivers/input/evdev.c |   30 +++++++++++++++++++++++++++++-
> >  include/linux/input.h |   25 +++++++++++++++++++++++++
> >  2 files changed, 54 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > index 4cf2534..8bf45d4 100644
> > --- a/drivers/input/evdev.c
> > +++ b/drivers/input/evdev.c
> > @@ -20,7 +20,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> > -#include <linux/input.h>
> > +#include <linux/input/mt.h>
> >  #include <linux/major.h>
> >  #include <linux/device.h>
> >  #include "input-compat.h"
> > @@ -621,6 +621,31 @@ static int evdev_handle_set_keycode_v2(struct input_dev *dev, void __user *p)
> >  	return input_set_keycode(dev, &ke);
> >  }
> >  
> > +static int evdev_handle_mt_request(struct input_dev *dev,
> > +				   unsigned int size,
> > +				   int __user *ip)
> > +{
> > +	const struct input_mt_slot *mt = dev->mt;
> > +	unsigned int code;
> > +	int i;
> > +
> > +	if (get_user(code, &ip[0]))
> > +		return -EFAULT;
> > +	if (!input_is_mt_value(code))
> > +		return -EINVAL;
> > +
> > +	if (size < sizeof(struct input_mt_request) + dev->mtsize * sizeof(int))
> > +		return -EINVAL;
> 
> Why not fill in up to the size of the array provided?

Yes, I think we do the same with other ioctls as well.

> 
> > +	if (put_user(dev->mtsize, &ip[1]))
> > +		return -EFAULT;
> > +	for (i = 0; i < dev->mtsize; i++)
> > +		if (put_user(input_mt_get_value(&mt[i], code), &ip[2 + i]))
> > +			return -EFAULT;
> 
> This would be easier to understand if you did:
> 
> struct input_mt_request *req = (struct input_mt_request *)ip;
> 
> Then modify the stuff in a straightforward way.

This is a user pointer, you can't really do it in straightforward way...

> 
> > +	return 0;
> > +}
> > +
> >  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> >  			   void __user *p, int compat_mode)
> >  {
> > @@ -706,6 +731,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> >  		return bits_to_user(dev->propbit, INPUT_PROP_MAX,
> >  				    size, p, compat_mode);
> >  
> > +	case EVIOCGMTSLOTS(0):
> > +		return evdev_handle_mt_request(dev, size, ip);
> > +
> >  	case EVIOCGKEY(0):
> >  		return bits_to_user(dev->key, KEY_MAX, size, p, compat_mode);
> >  
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index a637e78..9a73f18 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -99,6 +99,30 @@ struct input_keymap_entry {
> >  	__u8  scancode[32];
> >  };
> >  
> > +/**
> > + * struct input_mt_request - used by EVIOCGMTSLOTS ioctl
> > + * @code: ABS_MT code to read
> > + * @num_values: number of values written to the value array
> > + * @values: value array, one value per slot
> > + *
> > + * The structure is used to retrieve MT slot data. Before the call,
> > + * code is set to the wanted ABS_MT event type. On return, the value
> > + * array is filled with the slot values for the specified ABS_MT code,
> > + * and num_values is set to the actual number of slots.
> > + *
> > + * The call argument (len) is the size of the return buffer, satisfying
> > + *
> > + * len >= sizeof(input_mt_request) + sizeof(__s32) * number_of_slots
> > + *
> > + * If the request code is not an ABS_MT value, or if len is too small,
> > + * -EINVAL is returned.
> > + */
> > +struct input_mt_request {
> > +	__u32 code;
> > +	__u32 num_values;

Hmm, might avoid this one... Userspace can do EVIOCGABS to get number of
slots and then it will know how much data EVIOCGMTSLOTS will return...

> > +	__s32 values[];
> > +};
> > +
> >  #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
> >  #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
> >  #define EVIOCGREP		_IOR('E', 0x03, unsigned int[2])	/* get repeat settings */
> > @@ -113,6 +137,7 @@ struct input_keymap_entry {
> >  #define EVIOCGPHYS(len)		_IOC(_IOC_READ, 'E', 0x07, len)		/* get physical location */
> >  #define EVIOCGUNIQ(len)		_IOC(_IOC_READ, 'E', 0x08, len)		/* get unique identifier */
> >  #define EVIOCGPROP(len)		_IOC(_IOC_READ, 'E', 0x09, len)		/* get device properties */
> > +#define EVIOCGMTSLOTS(len)	_IOC(_IOC_READ, 'E', 0x0a, len)		/* get MT slot values */
> >  
> >  #define EVIOCGKEY(len)		_IOC(_IOC_READ, 'E', 0x18, len)		/* get global key state */
> >  #define EVIOCGLED(len)		_IOC(_IOC_READ, 'E', 0x19, len)		/* get all LEDs */
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: Add EVIOC mechanism for MT slots

[Chase Douglas]

On 01/13/2012 10:15 AM, Dmitry Torokhov wrote:
> On Fri, Jan 13, 2012 at 09:34:12AM +0100, Chase Douglas wrote:
>> On 01/12/2012 08:24 PM, Henrik Rydberg wrote:
>>> +	if (put_user(dev->mtsize, &ip[1]))
>>> +		return -EFAULT;
>>> +	for (i = 0; i < dev->mtsize; i++)
>>> +		if (put_user(input_mt_get_value(&mt[i], code), &ip[2 + i]))
>>> +			return -EFAULT;
>>
>> This would be easier to understand if you did:
>>
>> struct input_mt_request *req = (struct input_mt_request *)ip;
>>
>> Then modify the stuff in a straightforward way.
> 
> This is a user pointer, you can't really do it in straightforward way...

I don't know why that is, but as a thought could you do:

struct input_mt_request __user *req =
	(struct input_mt_request __user *)ip;

?

>>
>>> +	return 0;
>>> +}
>>> +
>>>  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>>>  			   void __user *p, int compat_mode)
>>>  {
>>> @@ -706,6 +731,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>>>  		return bits_to_user(dev->propbit, INPUT_PROP_MAX,
>>>  				    size, p, compat_mode);
>>>  
>>> +	case EVIOCGMTSLOTS(0):
>>> +		return evdev_handle_mt_request(dev, size, ip);
>>> +
>>>  	case EVIOCGKEY(0):
>>>  		return bits_to_user(dev->key, KEY_MAX, size, p, compat_mode);
>>>  
>>> diff --git a/include/linux/input.h b/include/linux/input.h
>>> index a637e78..9a73f18 100644
>>> --- a/include/linux/input.h
>>> +++ b/include/linux/input.h
>>> @@ -99,6 +99,30 @@ struct input_keymap_entry {
>>>  	__u8  scancode[32];
>>>  };
>>>  
>>> +/**
>>> + * struct input_mt_request - used by EVIOCGMTSLOTS ioctl
>>> + * @code: ABS_MT code to read
>>> + * @num_values: number of values written to the value array
>>> + * @values: value array, one value per slot
>>> + *
>>> + * The structure is used to retrieve MT slot data. Before the call,
>>> + * code is set to the wanted ABS_MT event type. On return, the value
>>> + * array is filled with the slot values for the specified ABS_MT code,
>>> + * and num_values is set to the actual number of slots.
>>> + *
>>> + * The call argument (len) is the size of the return buffer, satisfying
>>> + *
>>> + * len >= sizeof(input_mt_request) + sizeof(__s32) * number_of_slots
>>> + *
>>> + * If the request code is not an ABS_MT value, or if len is too small,
>>> + * -EINVAL is returned.
>>> + */
>>> +struct input_mt_request {
>>> +	__u32 code;
>>> +	__u32 num_values;
> 
> Hmm, might avoid this one... Userspace can do EVIOCGABS to get number of
> slots and then it will know how much data EVIOCGMTSLOTS will return...

Is it ever possible that a driver could change the number of slots after
initialization? I can't imagine why that would occur, but if we ever
come across a need for it, this would be broken.

-- Chase

Re: [PATCH 2/2] Input: Add EVIOC mechanism for MT slots

[Henrik Rydberg]

Hi Dmitry,

> > > +static int evdev_handle_mt_request(struct input_dev *dev,
> > > +				   unsigned int size,
> > > +				   int __user *ip)
> > > +{
> > > +	const struct input_mt_slot *mt = dev->mt;
> > > +	unsigned int code;
> > > +	int i;
> > > +
> > > +	if (get_user(code, &ip[0]))
> > > +		return -EFAULT;
> > > +	if (!input_is_mt_value(code))
> > > +		return -EINVAL;
> > > +
> > > +	if (size < sizeof(struct input_mt_request) + dev->mtsize * sizeof(int))
> > > +		return -EINVAL;
> > 
> > Why not fill in up to the size of the array provided?
> 
> Yes, I think we do the same with other ioctls as well.

Agreed, will do it that way.

> > > +/**
> > > + * struct input_mt_request - used by EVIOCGMTSLOTS ioctl
> > > + * @code: ABS_MT code to read
> > > + * @num_values: number of values written to the value array
> > > + * @values: value array, one value per slot
> > > + *
> > > + * The structure is used to retrieve MT slot data. Before the call,
> > > + * code is set to the wanted ABS_MT event type. On return, the value
> > > + * array is filled with the slot values for the specified ABS_MT code,
> > > + * and num_values is set to the actual number of slots.
> > > + *
> > > + * The call argument (len) is the size of the return buffer, satisfying
> > > + *
> > > + * len >= sizeof(input_mt_request) + sizeof(__s32) * number_of_slots
> > > + *
> > > + * If the request code is not an ABS_MT value, or if len is too small,
> > > + * -EINVAL is returned.
> > > + */
> > > +struct input_mt_request {
> > > +	__u32 code;
> > > +	__u32 num_values;
> 
> Hmm, might avoid this one... Userspace can do EVIOCGABS to get number of
> slots and then it will know how much data EVIOCGMTSLOTS will return...

I think it might be good to keep the length, to avoid possible future
synchronization problems. Perhaps we could get rid of the structure
though, for instance by specifying the write as a 32-bit code and the
read buffer as length, value, value...

Thanks,
Henrik