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

[Chase Douglas <chase.douglas@canonical.com>]

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