[PATCH v5 0/3] Make input drivers y2038 safe

[PATCH v5 0/3] Make input drivers y2038 safe

[Deepa Dinamani]

The series is aimed at making input events y2038 safe.
It extends the lifetime of the realtime timestamps in the
events to year 2106.
The series is also a necessary update as glibc is set to provide
64 bit time_t support for 32 bit binaries. glibc plan is detailed
at https://sourceware.org/glibc/wiki/Y2038ProofnessDesign .

The series is a result of discussions with Arnd Bergmann and
Dmitry Torokhov at last Plumbers.

The plan is to deprecate realtime timestamps anyway as they
are not appropriate for these timestamps as noted in the patch
a80b83b7b8 by John Stultz.

The design also updates the format of the input events read/ written
to the device nodes. This breaks 32 bit interface to the input
events at compile time as preferred by the maintainer.

The userspace library changes to libevdev, libuinput and mtdev
will be posted to the respective mailing groups for review.

Changes from v4:
* Dropped serio hil patch
Changes from v3:
* Updated uinput to support monotonic time only
* Addressed review comments
Changes from v2:
* Updated the design to break 32 bit interfaces at compile time.
Changes from v1:
* Updated changes according to review comments.
* Posted userspace library changes that go along with the series.

Deepa Dinamani (3):
  uinput: Use monotonic times for uinput timestamps.
  input: evdev: Replace timeval with timespec64
  input: Deprecate real timestamps beyond year 2106

 drivers/input/evdev.c        | 43 +++++++++++++++++++++++++++----------------
 drivers/input/input-compat.c | 11 ++++++-----
 drivers/input/input-compat.h |  3 ++-
 drivers/input/misc/uinput.c  |  5 ++++-
 include/uapi/linux/input.h   | 12 +++++++++++-
 5 files changed, 50 insertions(+), 24 deletions(-)


base-commit: 0c1f9d81ac360d8ad31cbfd2bdcf44de8204188e
-- 
2.14.1

[PATCH v5 1/3] uinput: Use monotonic times for uinput timestamps.

[Deepa Dinamani]

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.

The patch switches the timestamps to use monotonic time
from realtime time. This is assuming no one is using
absolute times from these timestamps.

The structure to maintain input events will be changed
in a different patch.

Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 drivers/input/misc/uinput.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 91df0df15e68..9251765645d1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -84,11 +84,14 @@ 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);
+	ktime_get_ts64(&ts);
+	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);
-- 
2.14.1

[PATCH v5 2/3] input: evdev: Replace timeval with timespec64

[Deepa Dinamani]

struct timeval is not y2038 safe.

All references to timeval in the kernel will be replaced
by y2038 safe structures.
Replace all references to timeval with y2038 safe
struct timespec64 here.

struct input_event will be changed in a different patch.

Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 drivers/input/evdev.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 0193dd4f0452..3171c4882d80 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -156,15 +156,22 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 static void __evdev_queue_syn_dropped(struct evdev_client *client)
 {
 	struct input_event ev;
-	ktime_t time;
+	struct timespec64 ts;
 
-	time = client->clk_type == EV_CLK_REAL ?
-			ktime_get_real() :
-			client->clk_type == EV_CLK_MONO ?
-				ktime_get() :
-				ktime_get_boottime();
+	switch (client->clk_type) {
+	case EV_CLK_REAL:
+		ktime_get_real_ts64(&ts);
+		break;
+	case EV_CLK_MONO:
+		ktime_get_ts64(&ts);
+		break;
+	case EV_CLK_BOOT:
+		get_monotonic_boottime64(&ts);
+		break;
+	}
 
-	ev.time = ktime_to_timeval(time);
+	ev.time.tv_sec = ts.tv_sec;
+	ev.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 	ev.type = EV_SYN;
 	ev.code = SYN_DROPPED;
 	ev.value = 0;
@@ -257,17 +264,20 @@ static void __pass_event(struct evdev_client *client,
 
 static void evdev_pass_values(struct evdev_client *client,
 			const struct input_value *vals, unsigned int count,
-			ktime_t *ev_time)
+			struct timespec64 *ev_time)
 {
 	struct evdev *evdev = client->evdev;
 	const struct input_value *v;
 	struct input_event event;
+	struct timespec64 ts;
 	bool wakeup = false;
 
 	if (client->revoked)
 		return;
 
-	event.time = ktime_to_timeval(ev_time[client->clk_type]);
+	ts = ev_time[client->clk_type];
+	event.time.tv_sec = ts.tv_sec;
+	event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 
 	/* Interrupts are disabled, just acquire the lock. */
 	spin_lock(&client->buffer_lock);
@@ -304,12 +314,11 @@ static void evdev_events(struct input_handle *handle,
 {
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;
-	ktime_t ev_time[EV_CLK_MAX];
+	struct timespec64 ev_time[EV_CLK_MAX];
 
-	ev_time[EV_CLK_MONO] = ktime_get();
-	ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]);
-	ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO],
-						 TK_OFFS_BOOT);
+	ktime_get_ts64(&ev_time[EV_CLK_MONO]);
+	ktime_get_real_ts64(&ev_time[EV_CLK_REAL]);
+	get_monotonic_boottime64(&ev_time[EV_CLK_BOOT]);
 
 	rcu_read_lock();
 
-- 
2.14.1

[PATCH v5 3/3] input: Deprecate real timestamps beyond year 2106

[Deepa Dinamani]

struct timeval is not y2038 safe.
All usage of timeval in the kernel will be replaced by
y2038 safe structures.
The change is also necessary as glibc is introducing support
for 32 bit applications to use 64 bit time_t. Without this
change, many applications would incorrectly interpret values
in the struct input_event.
More details about glibc at
https://sourceware.org/glibc/wiki/Y2038ProofnessDesign .

struct input_event maintains time for each input event.
Real time timestamps are not ideal for input as this
time can go backwards as noted in the patch a80b83b7b8
by John Stultz. Hence, having the input_event.time fields
only big enough for monotonic and boot times are
sufficient.

The change leaves the representation of struct input_event as is
on 64 bit architectures. But uses 2 unsigned long values on 32 bit
machines to support real timestamps until year 2106.
This intentionally breaks the ABI on 32 bit architectures and
compat handling on 64 bit architectures.
This is as per maintainer's preference to introduce compile time errors
rather than run into runtime incompatibilities.

The change requires any 32 bit userspace utilities reading or writing
from event nodes to update their reading format to match the new
input_event. The changes to the popular libraries will be posted once
we agree on the kernel change.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 drivers/input/evdev.c        | 14 ++++++++------
 drivers/input/input-compat.c | 11 ++++++-----
 drivers/input/input-compat.h |  3 ++-
 drivers/input/misc/uinput.c  |  4 ++--
 include/uapi/linux/input.h   | 12 +++++++++++-
 5 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 3171c4882d80..7a7fb0d0a227 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -135,7 +135,8 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 			continue;
 		} else if (head != i) {
 			/* move entry to fill the gap */
-			client->buffer[head].time = ev->time;
+			client->buffer[head].input_event_sec = ev->input_event_sec;
+			client->buffer[head].input_event_usec = ev->input_event_usec;
 			client->buffer[head].type = ev->type;
 			client->buffer[head].code = ev->code;
 			client->buffer[head].value = ev->value;
@@ -170,8 +171,8 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
 		break;
 	}
 
-	ev.time.tv_sec = ts.tv_sec;
-	ev.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+	ev.input_event_sec = ts.tv_sec;
+	ev.input_event_usec = ts.tv_nsec / NSEC_PER_USEC;
 	ev.type = EV_SYN;
 	ev.code = SYN_DROPPED;
 	ev.value = 0;
@@ -248,7 +249,8 @@ static void __pass_event(struct evdev_client *client,
 		 */
 		client->tail = (client->head - 2) & (client->bufsize - 1);
 
-		client->buffer[client->tail].time = event->time;
+		client->buffer[client->tail].input_event_sec = event->input_event_sec;
+		client->buffer[client->tail].input_event_usec = event->input_event_usec;
 		client->buffer[client->tail].type = EV_SYN;
 		client->buffer[client->tail].code = SYN_DROPPED;
 		client->buffer[client->tail].value = 0;
@@ -276,8 +278,8 @@ static void evdev_pass_values(struct evdev_client *client,
 		return;
 
 	ts = ev_time[client->clk_type];
-	event.time.tv_sec = ts.tv_sec;
-	event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+	event.input_event_sec = ts.tv_sec;
+	event.input_event_usec = ts.tv_nsec / NSEC_PER_USEC;
 
 	/* Interrupts are disabled, just acquire the lock. */
 	spin_lock(&client->buffer_lock);
diff --git a/drivers/input/input-compat.c b/drivers/input/input-compat.c
index 2186f71c9fe5..419e40b68486 100644
--- a/drivers/input/input-compat.c
+++ b/drivers/input/input-compat.c
@@ -24,14 +24,15 @@ int input_event_from_user(const char __user *buffer,
 				   sizeof(struct input_event_compat)))
 			return -EFAULT;
 
-		event->time.tv_sec = compat_event.time.tv_sec;
-		event->time.tv_usec = compat_event.time.tv_usec;
+		event->input_event_sec = compat_event.sec;
+		event->input_event_usec = compat_event.usec;
 		event->type = compat_event.type;
 		event->code = compat_event.code;
 		event->value = compat_event.value;
 
 	} else {
-		if (copy_from_user(event, buffer, sizeof(struct input_event)))
+		if (copy_from_user(event, buffer,
+				   sizeof(struct input_event)))
 			return -EFAULT;
 	}
 
@@ -44,8 +45,8 @@ int input_event_to_user(char __user *buffer,
 	if (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) {
 		struct input_event_compat compat_event;
 
-		compat_event.time.tv_sec = event->time.tv_sec;
-		compat_event.time.tv_usec = event->time.tv_usec;
+		compat_event.sec = event->input_event_sec;
+		compat_event.usec = event->input_event_usec;
 		compat_event.type = event->type;
 		compat_event.code = event->code;
 		compat_event.value = event->value;
diff --git a/drivers/input/input-compat.h b/drivers/input/input-compat.h
index 1563160a7af3..08cd755e73fd 100644
--- a/drivers/input/input-compat.h
+++ b/drivers/input/input-compat.h
@@ -18,7 +18,8 @@
 #ifdef CONFIG_COMPAT
 
 struct input_event_compat {
-	struct compat_timeval time;
+	compat_ulong_t sec;
+	compat_ulong_t usec;
 	__u16 type;
 	__u16 code;
 	__s32 value;
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 9251765645d1..03d22fc90a45 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -90,8 +90,8 @@ static int uinput_dev_event(struct input_dev *dev,
 	udev->buff[udev->head].code = code;
 	udev->buff[udev->head].value = value;
 	ktime_get_ts64(&ts);
-	udev->buff[udev->head].time.tv_sec = ts.tv_sec;
-	udev->buff[udev->head].time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+	udev->buff[udev->head].input_event_sec = ts.tv_sec;
+	udev->buff[udev->head].input_event_usec = ts.tv_nsec / NSEC_PER_USEC;
 	udev->head = (udev->head + 1) % UINPUT_BUFFER_SIZE;
 
 	wake_up_interruptible(&udev->waitq);
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 8c5a0bf6ee35..9c5105ff5cc6 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -21,10 +21,20 @@
 
 /*
  * The event structure itself
+ * Note that __USE_TIME_BITS64 is defined by libc based on
+ * application's request to use 64 bit time_t.
  */
-
 struct input_event {
+#if	(__BITS_PER_LONG != 32 || !defined(__USE_TIME_BITS64)) && !defined(__KERNEL)
 	struct timeval time;
+#define input_event_sec time.tv_sec
+#define input_event_usec time.tv_usec
+#else
+	__kernel_ulong_t __sec;
+	__kernel_ulong_t __usec;
+#define input_event_sec  __sec
+#define input_event_usec __usec
+#endif
 	__u16 type;
 	__u16 code;
 	__s32 value;
-- 
2.14.1

Re: [PATCH v5 2/3] input: evdev: Replace timeval with timespec64

[Dmitry Torokhov]

Hi Deepa,

On Sun, Dec 17, 2017 at 09:18:43PM -0800, Deepa Dinamani wrote:
> struct timeval is not y2038 safe.
> 
> All references to timeval in the kernel will be replaced
> by y2038 safe structures.
> Replace all references to timeval with y2038 safe
> struct timespec64 here.
> 
> struct input_event will be changed in a different patch.
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
>  drivers/input/evdev.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 0193dd4f0452..3171c4882d80 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -156,15 +156,22 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>  {
>  	struct input_event ev;
> -	ktime_t time;
> +	struct timespec64 ts;
>  
> -	time = client->clk_type == EV_CLK_REAL ?
> -			ktime_get_real() :
> -			client->clk_type == EV_CLK_MONO ?
> -				ktime_get() :
> -				ktime_get_boottime();
> +	switch (client->clk_type) {
> +	case EV_CLK_REAL:
> +		ktime_get_real_ts64(&ts);
> +		break;
> +	case EV_CLK_MONO:
> +		ktime_get_ts64(&ts);
> +		break;
> +	case EV_CLK_BOOT:
> +		get_monotonic_boottime64(&ts);
> +		break;
> +	}
>  
> -	ev.time = ktime_to_timeval(time);
> +	ev.time.tv_sec = ts.tv_sec;
> +	ev.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>  	ev.type = EV_SYN;
>  	ev.code = SYN_DROPPED;
>  	ev.value = 0;
> @@ -257,17 +264,20 @@ static void __pass_event(struct evdev_client *client,
>  
>  static void evdev_pass_values(struct evdev_client *client,
>  			const struct input_value *vals, unsigned int count,
> -			ktime_t *ev_time)
> +			struct timespec64 *ev_time)
>  {
>  	struct evdev *evdev = client->evdev;
>  	const struct input_value *v;
>  	struct input_event event;
> +	struct timespec64 ts;
>  	bool wakeup = false;
>  
>  	if (client->revoked)
>  		return;
>  
> -	event.time = ktime_to_timeval(ev_time[client->clk_type]);
> +	ts = ev_time[client->clk_type];
> +	event.time.tv_sec = ts.tv_sec;
> +	event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>  
>  	/* Interrupts are disabled, just acquire the lock. */
>  	spin_lock(&client->buffer_lock);
> @@ -304,12 +314,11 @@ static void evdev_events(struct input_handle *handle,
>  {
>  	struct evdev *evdev = handle->private;
>  	struct evdev_client *client;
> -	ktime_t ev_time[EV_CLK_MAX];
> +	struct timespec64 ev_time[EV_CLK_MAX];
>  
> -	ev_time[EV_CLK_MONO] = ktime_get();
> -	ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]);
> -	ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO],
> -						 TK_OFFS_BOOT);
> +	ktime_get_ts64(&ev_time[EV_CLK_MONO]);
> +	ktime_get_real_ts64(&ev_time[EV_CLK_REAL]);
> +	get_monotonic_boottime64(&ev_time[EV_CLK_BOOT]);

This may result in different ev_time[] members holding different times,
whereas the original code would take one time sample and convert it to
different clocks.

Also, why can't we keep using ktime_t internally? It is y2038 safe,
right? I think you should drop this patch and adjust the 3rd one to
massage the input event timestamp patch to do ktime->timespec64->input
timestamp conversion.

Thanks.

-- 
Dmitry

Re: [PATCH v5 1/3] uinput: Use monotonic times for uinput timestamps.

[Dmitry Torokhov]

On Sun, Dec 17, 2017 at 09:18:42PM -0800, 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.
> 
> The patch switches the timestamps to use monotonic time
> from realtime time. This is assuming no one is using
> absolute times from these timestamps.
> 
> The structure to maintain input events will be changed
> in a different patch.
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

Applied, thank you.

> ---
>  drivers/input/misc/uinput.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 91df0df15e68..9251765645d1 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -84,11 +84,14 @@ 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);
> +	ktime_get_ts64(&ts);
> +	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);
> -- 
> 2.14.1
> 

-- 
Dmitry

Re: [PATCH v5 2/3] input: evdev: Replace timeval with timespec64

[Arnd Bergmann]

On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sun, Dec 17, 2017 at 09:18:43PM -0800, Deepa Dinamani wrote:
>> @@ -304,12 +314,11 @@ static void evdev_events(struct input_handle *handle,
>>  {
>>       struct evdev *evdev = handle->private;
>>       struct evdev_client *client;
>> -     ktime_t ev_time[EV_CLK_MAX];
>> +     struct timespec64 ev_time[EV_CLK_MAX];
>>
>> -     ev_time[EV_CLK_MONO] = ktime_get();
>> -     ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]);
>> -     ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO],
>> -                                              TK_OFFS_BOOT);
>> +     ktime_get_ts64(&ev_time[EV_CLK_MONO]);
>> +     ktime_get_real_ts64(&ev_time[EV_CLK_REAL]);
>> +     get_monotonic_boottime64(&ev_time[EV_CLK_BOOT]);
>
> This may result in different ev_time[] members holding different times,
> whereas the original code would take one time sample and convert it to
> different clocks.

Is this important? On each client we only return one of the two
times, and I would guess that you cannot rely on a correlation
between timestamps on different devices, since the boot and real
offsets can change over time.

> Also, why can't we keep using ktime_t internally? It is y2038 safe,
> right?

Correct, but there may also be a performance difference if we get
a lot of events, not sure if that matters.

> I think you should drop this patch and adjust the 3rd one to
> massage the input event timestamp patch to do ktime->timespec64->input
> timestamp conversion.

The change in __evdev_queue_syn_dropped still seems useful to me
as ktime_get_*ts64() is a bit more efficient than ktime_get*() followed by
a slow ktime_to_timespec64() or ktime_to_timeval().

For evdev_events(), doing a single ktime_get() followed by a
ktime_to_timespec64/ktime_to_timeval can be faster than three
ktime_get_*ts64 (depending on the hardware clock source), or
it can be slower depending on the CPU and the clocksource
hardware. Again, no idea if this matters at the usual rate of
input events.

I guess dropping the evdev_events() change and replacing it with a
ktime_to_timespec64 change in evdev_pass_values()
would be fine here, it should keep the current performance
behavior and get rid of the timeval.

       Arnd

Re: [PATCH v5 2/3] input: evdev: Replace timeval with timespec64

[Deepa Dinamani]

On Tue, Jan 2, 2018 at 7:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Sun, Dec 17, 2017 at 09:18:43PM -0800, Deepa Dinamani wrote:
>>> @@ -304,12 +314,11 @@ static void evdev_events(struct input_handle *handle,
>>>  {
>>>       struct evdev *evdev = handle->private;
>>>       struct evdev_client *client;
>>> -     ktime_t ev_time[EV_CLK_MAX];
>>> +     struct timespec64 ev_time[EV_CLK_MAX];
>>>
>>> -     ev_time[EV_CLK_MONO] = ktime_get();
>>> -     ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]);
>>> -     ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO],
>>> -                                              TK_OFFS_BOOT);
>>> +     ktime_get_ts64(&ev_time[EV_CLK_MONO]);
>>> +     ktime_get_real_ts64(&ev_time[EV_CLK_REAL]);
>>> +     get_monotonic_boottime64(&ev_time[EV_CLK_BOOT]);
>>
>> This may result in different ev_time[] members holding different times,
>> whereas the original code would take one time sample and convert it to
>> different clocks.
>
> Is this important? On each client we only return one of the two
> times, and I would guess that you cannot rely on a correlation
> between timestamps on different devices, since the boot and real
> offsets can change over time.

Right. I didn't think this was an issue either.

>> Also, why can't we keep using ktime_t internally? It is y2038 safe,
>> right?
>
> Correct, but there may also be a performance difference if we get
> a lot of events, not sure if that matters.
>
>> I think you should drop this patch and adjust the 3rd one to
>> massage the input event timestamp patch to do ktime->timespec64->input
>> timestamp conversion.
>
> The change in __evdev_queue_syn_dropped still seems useful to me
> as ktime_get_*ts64() is a bit more efficient than ktime_get*() followed by
> a slow ktime_to_timespec64() or ktime_to_timeval().
>
> For evdev_events(), doing a single ktime_get() followed by a
> ktime_to_timespec64/ktime_to_timeval can be faster than three
> ktime_get_*ts64 (depending on the hardware clock source), or
> it can be slower depending on the CPU and the clocksource
> hardware. Again, no idea if this matters at the usual rate of
> input events.
>
> I guess dropping the evdev_events() change and replacing it with a
> ktime_to_timespec64 change in evdev_pass_values()
> would be fine here, it should keep the current performance
> behavior and get rid of the timeval.

I was trying to use timespec64 everywhere so that we would not have
conversions back and forth at the input layer.
I dropped the ktime_t conversions for now and merged this patch with
the next one as requested.

Let me know if you would like to keep the changes Arnd preferred above
for __evdev_queue_syn_dropped(). I can submit a separate patch if this
is preferred.

-Deepa

Re: [PATCH v5 2/3] input: evdev: Replace timeval with timespec64

[Dmitry Torokhov]

On Sat, Jan 06, 2018 at 01:43:34PM -0800, Deepa Dinamani wrote:
> On Tue, Jan 2, 2018 at 7:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >> On Sun, Dec 17, 2017 at 09:18:43PM -0800, Deepa Dinamani wrote:
> >>> @@ -304,12 +314,11 @@ static void evdev_events(struct input_handle *handle,
> >>>  {
> >>>       struct evdev *evdev = handle->private;
> >>>       struct evdev_client *client;
> >>> -     ktime_t ev_time[EV_CLK_MAX];
> >>> +     struct timespec64 ev_time[EV_CLK_MAX];
> >>>
> >>> -     ev_time[EV_CLK_MONO] = ktime_get();
> >>> -     ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]);
> >>> -     ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO],
> >>> -                                              TK_OFFS_BOOT);
> >>> +     ktime_get_ts64(&ev_time[EV_CLK_MONO]);
> >>> +     ktime_get_real_ts64(&ev_time[EV_CLK_REAL]);
> >>> +     get_monotonic_boottime64(&ev_time[EV_CLK_BOOT]);
> >>
> >> This may result in different ev_time[] members holding different times,
> >> whereas the original code would take one time sample and convert it to
> >> different clocks.
> >
> > Is this important? On each client we only return one of the two
> > times, and I would guess that you cannot rely on a correlation
> > between timestamps on different devices, since the boot and real
> > offsets can change over time.
> 
> Right. I didn't think this was an issue either.
> 
> >> Also, why can't we keep using ktime_t internally? It is y2038 safe,
> >> right?
> >
> > Correct, but there may also be a performance difference if we get
> > a lot of events, not sure if that matters.
> >
> >> I think you should drop this patch and adjust the 3rd one to
> >> massage the input event timestamp patch to do ktime->timespec64->input
> >> timestamp conversion.
> >
> > The change in __evdev_queue_syn_dropped still seems useful to me
> > as ktime_get_*ts64() is a bit more efficient than ktime_get*() followed by
> > a slow ktime_to_timespec64() or ktime_to_timeval().
> >
> > For evdev_events(), doing a single ktime_get() followed by a
> > ktime_to_timespec64/ktime_to_timeval can be faster than three
> > ktime_get_*ts64 (depending on the hardware clock source), or
> > it can be slower depending on the CPU and the clocksource
> > hardware. Again, no idea if this matters at the usual rate of
> > input events.
> >
> > I guess dropping the evdev_events() change and replacing it with a
> > ktime_to_timespec64 change in evdev_pass_values()
> > would be fine here, it should keep the current performance
> > behavior and get rid of the timeval.
> 
> I was trying to use timespec64 everywhere so that we would not have
> conversions back and forth at the input layer.
> I dropped the ktime_t conversions for now and merged this patch with
> the next one as requested.
> 
> Let me know if you would like to keep the changes Arnd preferred above
> for __evdev_queue_syn_dropped(). I can submit a separate patch if this
> is preferred.

__evdev_queue_syn_dropped() is extremely cold path (hopefully, if it is
not we have much bigger problems) so I'd leave it as is.

Thanks!

-- 
Dmitry