Re: [PATCH v2 1/4] uinput: Add ioctl for using monotonic/ boot times

[Peter Hutterer <peter.hutterer@who-t.net>]

On Mon, Oct 17, 2016 at 08:27:30PM -0700, Deepa Dinamani wrote:
> struct timeval which is part of struct input_event to
> maintain the event times is not y2038 safe.
> 
> Real time timestamps are also not ideal for input_event
> as this time can go backwards as noted in the patch
> a80b83b7b8 by John Stultz.
> 
> Arnd Bergmann suggested deprecating real time and using
> monotonic or other timers for all input_event times as a
> solution to both the above problems.
> 
> Add a new ioctl to let the user dictate the kind of time
> to be used for input events. This is similar to the evdev
> implementation of the feature. Realtime is still the
> default time. This is to maintain backward compatibility.
> 
> The structure to maintain input events will be changed
> in a different patch.
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> ---
>  drivers/input/misc/uinput.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/uinput.h      |  1 +
>  include/uapi/linux/uinput.h |  3 +++
>  3 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 92595b9..3d75c5a 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -46,11 +46,26 @@ static int uinput_dev_event(struct input_dev *dev,
>  			    unsigned int type, unsigned int code, int value)
>  {
>  	struct uinput_device	*udev = input_get_drvdata(dev);
> +	struct timespec64	ts;
>  
>  	udev->buff[udev->head].type = type;
>  	udev->buff[udev->head].code = code;
>  	udev->buff[udev->head].value = value;
> -	do_gettimeofday(&udev->buff[udev->head].time);
> +
> +	switch (udev->clk_type) {
> +	case CLOCK_REALTIME:
> +		ktime_get_real_ts64(&ts);
> +		break;
> +	case CLOCK_MONOTONIC:
> +		ktime_get_ts64(&ts);
> +		break;
> +	case CLOCK_BOOTTIME:
> +		get_monotonic_boottime64(&ts);
> +		break;
> +	}

hmm, I'm a bit confused here. This is an in-kernel bit only (passing the
time through uinput events has no effect). So why do we need an ioctl here?
it's an in-kernel decision only anyway and the time in the events sent to
the evdev client should be dictated by what that client sets for the clock
type, right?

Cheers,
   Peter

> +
> +	udev->buff[udev->head].time.tv_sec = ts.tv_sec;
> +	udev->buff[udev->head].time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>  	udev->head = (udev->head + 1) % UINPUT_BUFFER_SIZE;
>  
>  	wake_up_interruptible(&udev->waitq);
> @@ -295,6 +310,7 @@ static int uinput_create_device(struct uinput_device *udev)
>  	if (error)
>  		goto fail2;
>  
> +	udev->clk_type = CLOCK_REALTIME;
>  	udev->state = UIST_CREATED;
>  
>  	return 0;
> @@ -304,6 +320,38 @@ static int uinput_create_device(struct uinput_device *udev)
>  	return error;
>  }
>  
> +static int uinput_set_clk_type(struct uinput_device *udev, unsigned int clkid)
> +{
> +	unsigned int clk_type;
> +
> +	if (udev->state != UIST_CREATED)
> +		return -EINVAL;
> +
> +	switch (clkid) {
> +	/* Realtime clock is only valid until year 2038.*/
> +	case CLOCK_REALTIME:
> +		clk_type = CLOCK_REALTIME;
> +		break;
> +	case CLOCK_MONOTONIC:
> +		clk_type = CLOCK_MONOTONIC;
> +		break;
> +	case CLOCK_BOOTTIME:
> +		clk_type = CLOCK_BOOTTIME;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (udev->clk_type != clk_type) {
> +		udev->clk_type = clk_type;
> +
> +		/* Flush pending events */
> +		uinput_flush_requests(udev);
> +	}
> +
> +	return 0;
> +}
> +
>  static int uinput_open(struct inode *inode, struct file *file)
>  {
>  	struct uinput_device *newdev;
> @@ -787,6 +835,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  	char			*phys;
>  	const char		*name;
>  	unsigned int		size;
> +	int			clock_id;
>  
>  	retval = mutex_lock_interruptible(&udev->mutex);
>  	if (retval)
> @@ -817,6 +866,11 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  			retval = uinput_dev_setup(udev, p);
>  			goto out;
>  
> +		case UI_SET_CLOCKID:
> +			if (copy_from_user(&clock_id, p, sizeof(unsigned int)))
> +				return -EFAULT;
> +			return uinput_set_clk_type(udev, clock_id);
> +
>  		/* UI_ABS_SETUP is handled in the variable size ioctls */
>  
>  		case UI_SET_EVBIT:
> diff --git a/include/linux/uinput.h b/include/linux/uinput.h
> index 75de43d..6527fb7 100644
> --- a/include/linux/uinput.h
> +++ b/include/linux/uinput.h
> @@ -72,6 +72,7 @@ struct uinput_device {
>  	unsigned char		head;
>  	unsigned char		tail;
>  	struct input_event	buff[UINPUT_BUFFER_SIZE];
> +	int			clk_type;
>  	unsigned int		ff_effects_max;
>  
>  	struct uinput_request	*requests[UINPUT_NUM_REQUESTS];
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index dc652e2..d9494ae 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -133,6 +133,9 @@ struct uinput_abs_setup {
>   */
>  #define UI_ABS_SETUP _IOW(UINPUT_IOCTL_BASE, 4, struct uinput_abs_setup)
>  
> +/* Set clockid to be used for timestamps */
> +#define UI_SET_CLOCKID _IOW(UINPUT_IOCTL_BASE, 5, int)
> +
>  #define UI_SET_EVBIT		_IOW(UINPUT_IOCTL_BASE, 100, int)
>  #define UI_SET_KEYBIT		_IOW(UINPUT_IOCTL_BASE, 101, int)
>  #define UI_SET_RELBIT		_IOW(UINPUT_IOCTL_BASE, 102, int)
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>