On 01/31/2012 08:00 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 INPUT_MT_REQUEST(64) req;
> req.code = ABS_MT_POSITION_X;
> if (ioctl(fd, EVIOCGMTSLOTS(sizeof(req)), &req) < 0)
> return -1;
> for (i = 0; i < 64; i++)
> printf("slot %d: %d\n", i, req.values[i]);
>
> Signed-off-by: Henrik Rydberg <[email protected]>
> ---
> Hi Dmitry,
>
> Here is v3 of the EVIOC patch for MT slots. The number of slots if
> gone, the struct is simplified, targeting userland, and calling the
> ioctl with a smaller struct will return the first set of slots.
>
> Cheers,
> Henrik
>
>
> drivers/input/evdev.c | 27 ++++++++++++++++++++++++++-
> include/linux/input.h | 23 +++++++++++++++++++++++
> 2 files changed, 49 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 76457d5..e4cad16 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"
> @@ -623,6 +623,28 @@ 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 max_slots;
> + int i;
> +
> + if (get_user(code, &ip[0]))
> + return -EFAULT;
> + if (!input_is_mt_value(code))
> + return -EINVAL;
> +
> + max_slots = (size - sizeof(__u32)) / sizeof(__s32);
> + for (i = 0; i < dev->mtsize && i < max_slots; i++)
> + if (put_user(input_mt_get_value(&mt[i], code), &ip[1 + i]))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> void __user *p, int compat_mode)
> {
> @@ -708,6 +730,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 3862e32..1e7e2e5 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -99,6 +99,28 @@ struct input_keymap_entry {
> __u8 scancode[32];
> };
>
> +/**
> + * struct INPUT_MT_REQUEST(num_slots) - used by EVIOCGMTSLOTS ioctl
> + * @code: ABS_MT code to read
> + * @values: value array, one value per slot
> + *
> + * The struct definition is used to create an appropriate MT slot message
> + * buffer in userland. 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.
> + *
> + * The call argument (len) is the size of the return buffer, which should
> + * satisfy len >= sizeof(struct INPUT_MT_REQUEST(num_slots)). If len is
> + * too small to fit all available slots, the first num_slots are returned.
> + *
> + * If the request code is not an ABS_MT value, -EINVAL is returned.
> + */
> +#define INPUT_MT_REQUEST(num_slots) \
> + { \
> + __u32 code; \
> + __s32 values[num_slots]; \
I think this assumes a userspace C compiler that can handle variable
length arrays. This would require only compiling in C source code at the
C99 standard or later. It looks like C++ doesn't even allow variable
length arrays, though gcc handles it. According to:
http://www.cplusplus.com/forum/beginner/1601/
it looks like Borland c++ compilers may not be able to compile this :(.
-- Chase
Hi Chase,
> > +#define INPUT_MT_REQUEST(num_slots) \
> > + { \
> > + __u32 code; \
> > + __s32 values[num_slots]; \
>
> I think this assumes a userspace C compiler that can handle variable
> length arrays. This would require only compiling in C source code at the
> C99 standard or later. It looks like C++ doesn't even allow variable
> length arrays, though gcc handles it. According to:
>
> http://www.cplusplus.com/forum/beginner/1601/
>
> it looks like Borland c++ compilers may not be able to compile this :(.
This is resolved on the preprocessor level, so C99 or not does not
enter the problem. Compile-time constant, as you can see in the code
example in the patch summary.
Thanks,
Henrik
On 02/03/2012 08:27 AM, Henrik Rydberg wrote:
> Hi Chase,
>
>>> +#define INPUT_MT_REQUEST(num_slots) \
>>> + { \
>>> + __u32 code; \
>>> + __s32 values[num_slots]; \
>>
>> I think this assumes a userspace C compiler that can handle variable
>> length arrays. This would require only compiling in C source code at the
>> C99 standard or later. It looks like C++ doesn't even allow variable
>> length arrays, though gcc handles it. According to:
>>
>> http://www.cplusplus.com/forum/beginner/1601/
>>
>> it looks like Borland c++ compilers may not be able to compile this :(.
>
> This is resolved on the preprocessor level, so C99 or not does not
> enter the problem. Compile-time constant, as you can see in the code
> example in the patch summary.
You're right, I didn't catch that. It will be compatible with all C
compilers if you use a static number of slots.
However, it will break if you use a non-C99 C compiler and the code
wants to do dynamic number of slots calculations. I imagine most callers
would do:
EVIOCGABS call on ABS_MT_SLOT;
int num_slots = ABS_MT_SLOT.max - ABS_MT_SLOT.min
struct INPUT_MT_REQUEST(num_slots) req;
This will break on non-C99 C compilers and other language compilers. It
also will lead to head-scratcher bugs when someone compiles it just fine
in their C99 project, copies the code to another project with a
different compiler, and is confronted with the issue.
I think this issue should be enough to rethink the interface.
-- Chase
> > This is resolved on the preprocessor level, so C99 or not does not
> > enter the problem. Compile-time constant, as you can see in the code
> > example in the patch summary.
>
> You're right, I didn't catch that. It will be compatible with all C
> compilers if you use a static number of slots.
Yes, but this statement is merely repeating something that has been
true since the sixties.
> However, it will break if you use a non-C99 C compiler and the code
> wants to do dynamic number of slots calculations.
Of course, which is why C99 cannot be used for portable code. And it
still has nothing to do with this patch.
> I imagine most callers would do:
>
> EVIOCGABS call on ABS_MT_SLOT;
> int num_slots = ABS_MT_SLOT.max - ABS_MT_SLOT.min
> struct INPUT_MT_REQUEST(num_slots) req;
Besides leaving a possible giant stack crash in your code, it assumes
memory is somehow magically allocated. Not good practise in low-level
programming. You wouldn't use a template this way, would you?
> This will break on non-C99 C compilers and other language compilers.
Of course, since you use the C99 dynamic stack construct, which,
again, is not portable.
> It also will lead to head-scratcher bugs when someone compiles it
> just fine in their C99 project, copies the code to another project
> with a different compiler, and is confronted with the issue.
No, since people how know C do not do things like that.
> I think this issue should be enough to rethink the interface.
No, since your issues with C99 has nothing to do with this patch.
Henrik
On 02/05/2012 08:59 AM, Henrik Rydberg wrote:
>>> This is resolved on the preprocessor level, so C99 or not does not
>>> enter the problem. Compile-time constant, as you can see in the code
>>> example in the patch summary.
>>
>> You're right, I didn't catch that. It will be compatible with all C
>> compilers if you use a static number of slots.
>
> Yes, but this statement is merely repeating something that has been
> true since the sixties.
>
>> However, it will break if you use a non-C99 C compiler and the code
>> wants to do dynamic number of slots calculations.
>
> Of course, which is why C99 cannot be used for portable code. And it
> still has nothing to do with this patch.
>
>> I imagine most callers would do:
>>
>> EVIOCGABS call on ABS_MT_SLOT;
>> int num_slots = ABS_MT_SLOT.max - ABS_MT_SLOT.min
>> struct INPUT_MT_REQUEST(num_slots) req;
>
> Besides leaving a possible giant stack crash in your code, it assumes
> memory is somehow magically allocated. Not good practise in low-level
> programming. You wouldn't use a template this way, would you?
No, which is why I think this interface is bad. I should be able to
dynamically set the size of the array, but it's not possible with this
interface.
>> This will break on non-C99 C compilers and other language compilers.
>
> Of course, since you use the C99 dynamic stack construct, which,
> again, is not portable.
>
>> It also will lead to head-scratcher bugs when someone compiles it
>> just fine in their C99 project, copies the code to another project
>> with a different compiler, and is confronted with the issue.
>
> No, since people how know C do not do things like that.
>
>> I think this issue should be enough to rethink the interface.
>
> No, since your issues with C99 has nothing to do with this patch.
With this macro, one is forced to pick a number at compile time. If that
number isn't big enough you have no recourse. If you pick too big of a
number you are wasting stack space.
I think the implementation is fine in terms of how the plumbing works. I
just don't think this macro should be included. Make the user create the
struct themselves:
In linux/input.h:
struct input_mt_request {
__u32 code;
__s32 values[];
};
In code:
int num_slots;
int size;
struct input_mt_request *req;
EVIOCGABS call on ABS_MT_SLOT;
num_slots = ABS_MT_SLOT.max - ABS_MT_SLOT.min
size = sizeof(struct input_mt_request) + num_slots * sizeof(__s32);
req = malloc(size);
req->code = ABS_MT_POSITION_X;
if (ioctl(fd, EVIOCGMTSLOTS(size), req) < 0) {
free(req);
return -1;
}
for (i = 0; i < num_slots; i++)
printf("slot %d: %d\n", i, req->values[i]);
free(req);
(I'm still not clear on how the values[] member of the request can work,
so this may not be quite right. I tried to copy the way the first
version of this patch worked. However, the dynamic request size is the
important part.)
It could be argued that we should leave the macro around for people who
want to statically define the size of the request, but I think that is
leading them down the wrong path. It's easier, but it will lead to
broken code if you pick the wrong size.
-- Chase
> > Besides leaving a possible giant stack crash in your code, it assumes
> > memory is somehow magically allocated. Not good practise in low-level
> > programming. You wouldn't use a template this way, would you?
>
> No, which is why I think this interface is bad. I should be able to
> dynamically set the size of the array, but it's not possible with this
> interface.
It is possible (using num_slots == 0 or creating your own struct), but
not ideal, granted. The patch serves the purpose of definining the
binary interface, the rest is up to userland.
> I think the implementation is fine in terms of how the plumbing works. I
> just don't think this macro should be included. Make the user create the
> struct themselves:
>
> In linux/input.h:
>
> struct input_mt_request {
> __u32 code;
> __s32 values[];
> };
The above (the first) version is not ideal either, since it cannot be
used as it is.
> It could be argued that we should leave the macro around for people who
> want to statically define the size of the request, but I think that is
> leading them down the wrong path. It's easier, but it will lead to
> broken code if you pick the wrong size.
Rather than creating both a suboptimal static and a suboptimal dynamic
version, removing the struct altogether is tempting. All that is
really needed is a clear definition of the binary interface. The rest
can easily be set up in userland, using whatever method is preferred.
Thanks.
Henrik
On 02/05/2012 08:40 PM, Henrik Rydberg wrote:
>>> Besides leaving a possible giant stack crash in your code, it assumes
>>> memory is somehow magically allocated. Not good practise in low-level
>>> programming. You wouldn't use a template this way, would you?
>>
>> No, which is why I think this interface is bad. I should be able to
>> dynamically set the size of the array, but it's not possible with this
>> interface.
>
> It is possible (using num_slots == 0 or creating your own struct), but
> not ideal, granted. The patch serves the purpose of definining the
> binary interface, the rest is up to userland.
>
>> I think the implementation is fine in terms of how the plumbing works. I
>> just don't think this macro should be included. Make the user create the
>> struct themselves:
>>
>> In linux/input.h:
>>
>> struct input_mt_request {
>> __u32 code;
>> __s32 values[];
>> };
>
> The above (the first) version is not ideal either, since it cannot be
> used as it is.
>
>> It could be argued that we should leave the macro around for people who
>> want to statically define the size of the request, but I think that is
>> leading them down the wrong path. It's easier, but it will lead to
>> broken code if you pick the wrong size.
>
> Rather than creating both a suboptimal static and a suboptimal dynamic
> version, removing the struct altogether is tempting. All that is
> really needed is a clear definition of the binary interface. The rest
> can easily be set up in userland, using whatever method is preferred.
I'm normally not a fan of static functions in header files, but maybe it
would be a good solution here:
struct input_mt_request {
__u32 code;
__s32 values[];
};
static struct input_mt_request *
linux_input_mt_request_alloc(int num_slots) {
return (struct input_mt_request *)malloc(
sizeof(__u32) + num_slots * sizeof(__s32));
}
#define EVIOCGMTSLOTS(num_slots) \
_IOC(_IOC_READ, 'E', 0x0a, \
sizeof(__u32) + (num_slots) * sizeof(__s32))
This would lead to userspace code:
struct input_mt_request *req;
int num_slots;
EVIOCGABS call on ABS_MT_SLOT;
num_slots = ABS_MT_SLOT.max - ABS_MT_SLOT.min + 1;
req = linux_input_mt_request_alloc(num_slots);
req->code = ABS_MT_POSITION_X;
if (ioctl(fd, EVIOCGMTSLOTS(num_slots), req) < 0) {
free(req);
return -1;
}
for (i = 0; i < 64; i++)
printf("slot %d: %d\n", i, req.values[i]);
free(req);
Normally, I would recommend adding a free() function too, but the
necessity of a free() function is only when libc's free() won't work or
the implementation may change. Here, however, the implementation would
be codified by the ioctl interface in a way that guarantees libc's
free() to be correct.
-- Chase
On Sun, Feb 05, 2012 at 11:55:09PM +0100, Chase Douglas wrote:
> On 02/05/2012 08:40 PM, Henrik Rydberg wrote:
> >>> Besides leaving a possible giant stack crash in your code, it assumes
> >>> memory is somehow magically allocated. Not good practise in low-level
> >>> programming. You wouldn't use a template this way, would you?
> >>
> >> No, which is why I think this interface is bad. I should be able to
> >> dynamically set the size of the array, but it's not possible with this
> >> interface.
> >
> > It is possible (using num_slots == 0 or creating your own struct), but
> > not ideal, granted. The patch serves the purpose of definining the
> > binary interface, the rest is up to userland.
> >
> >> I think the implementation is fine in terms of how the plumbing works. I
> >> just don't think this macro should be included. Make the user create the
> >> struct themselves:
> >>
> >> In linux/input.h:
> >>
> >> struct input_mt_request {
> >> __u32 code;
> >> __s32 values[];
> >> };
> >
> > The above (the first) version is not ideal either, since it cannot be
> > used as it is.
> >
> >> It could be argued that we should leave the macro around for people who
> >> want to statically define the size of the request, but I think that is
> >> leading them down the wrong path. It's easier, but it will lead to
> >> broken code if you pick the wrong size.
> >
> > Rather than creating both a suboptimal static and a suboptimal dynamic
> > version, removing the struct altogether is tempting. All that is
> > really needed is a clear definition of the binary interface. The rest
> > can easily be set up in userland, using whatever method is preferred.
>
> I'm normally not a fan of static functions in header files, but maybe it
> would be a good solution here:
>
> struct input_mt_request {
> __u32 code;
> __s32 values[];
> };
>
> static struct input_mt_request *
> linux_input_mt_request_alloc(int num_slots) {
> return (struct input_mt_request *)malloc(
> sizeof(__u32) + num_slots * sizeof(__s32));
> }
Gah, can we please leave it to userspace programs to define and allocate
the structure as it makes most sense for them. As far as kernel goes we
just want to document that we'll need a u32 and a place to put N s32s.
How they are allocated we do not care at all. It could be on stack
(which does not have the same limits as kernel stack) or in the heap.
Thanks.
--
Dmitry