Reposting the series as I did not hear comments from the maintainers.
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 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 series is a result of many discussions with Arnd Bergmann.
The design updates the format of the input events read/ written to the device nodes. This structure and the uapi/input.h header file are copied across many userspace libraries. These libraries not only use this struct input_event locally, but also expose interfaces that contain input_event. To maintain backward compatibility with all these interfaces, kernel updates the format of the input events at the dev node to struct raw_input_event. Kernel also maintains the old struct input_event and provides apis to convert between the old and new input event types.
The userspace library changes to libevdev, libuinput and mtdev will be posted to the respective mailing groups for review. Once there is a consensus on the design, all the changes dependent on the kernel change will be merged at the same time.
Changes from v1: * Updated changes according to review comments. * Posted userspace library changes that go along with the series.
Deepa Dinamani (4): uinput: Add ioctl for using monotonic/ boot times input: evdev: Replace timeval with timespec64 input: Deprecate real timestamps beyond year 2106 input: serio: Replace timeval by timespec64
drivers/input/evdev.c | 57 ++++++++++++++++++++---------------- drivers/input/input-compat.c | 29 ++++++++++--------- drivers/input/input-compat.h | 19 +++++++----- drivers/input/misc/uinput.c | 62 +++++++++++++++++++++++++++++++++++++--- drivers/input/serio/hil_mlc.c | 37 ++++++++++++------------ drivers/input/serio/hp_sdc.c | 17 +++++------ drivers/input/serio/hp_sdc_mlc.c | 10 +++---- include/linux/hil_mlc.h | 6 ++-- include/linux/hp_sdc.h | 6 ++-- include/linux/uinput.h | 3 +- include/uapi/linux/input.h | 47 ++++++++++++++++++++++++++++++ include/uapi/linux/uinput.h | 3 ++ 12 files changed, 208 insertions(+), 88 deletions(-)
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; + } + + 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)
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 --- 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 e9ae3d5..b4e3171 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();
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event. This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
Suggested-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com Reviewed-by: Arnd Bergmann arnd@arndb.de --- drivers/input/evdev.c | 20 +++++++++---------- drivers/input/input-compat.c | 29 ++++++++++++++------------- drivers/input/input-compat.h | 19 +++++++++++------- drivers/input/misc/uinput.c | 6 +++--- include/linux/uinput.h | 2 +- include/uapi/linux/input.h | 47 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 88 insertions(+), 35 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index b4e3171..459e3ba 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -60,7 +60,7 @@ struct evdev_client { bool revoked; unsigned long *evmasks[EV_CNT]; unsigned int bufsize; - struct input_event buffer[]; + struct raw_input_event buffer[]; };
static size_t evdev_get_mask_cnt(unsigned int type) @@ -113,7 +113,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type) unsigned int i, head, num; unsigned int mask = client->bufsize - 1; bool is_report; - struct input_event *ev; + struct raw_input_event *ev;
BUG_ON(type == EV_SYN);
@@ -155,7 +155,7 @@ 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; + struct raw_input_event ev; struct timespec64 ts;
switch (client->clk_type) { @@ -236,7 +236,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid) }
static void __pass_event(struct evdev_client *client, - const struct input_event *event) + const struct raw_input_event *event) { client->buffer[client->head++] = *event; client->head &= client->bufsize - 1; @@ -268,7 +268,7 @@ static void evdev_pass_values(struct evdev_client *client, { struct evdev *evdev = client->evdev; const struct input_value *v; - struct input_event event; + struct raw_input_event event; struct timespec64 ts; bool wakeup = false;
@@ -507,7 +507,7 @@ static int evdev_open(struct inode *inode, struct file *file) struct evdev *evdev = container_of(inode->i_cdev, struct evdev, cdev); unsigned int bufsize = evdev_compute_buffer_size(evdev->handle.dev); unsigned int size = sizeof(struct evdev_client) + - bufsize * sizeof(struct input_event); + bufsize * sizeof(struct raw_input_event); struct evdev_client *client; int error;
@@ -542,7 +542,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer, { struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev; - struct input_event event; + struct raw_input_event event; int retval = 0;
if (count != 0 && count < input_event_size()) @@ -575,7 +575,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer, }
static int evdev_fetch_next_event(struct evdev_client *client, - struct input_event *event) + struct raw_input_event *event) { int have_event;
@@ -597,7 +597,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer, { struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev; - struct input_event event; + struct raw_input_event event; size_t read = 0; int error;
@@ -1083,7 +1083,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, switch (cmd) {
case EVIOCGVERSION: - return put_user(EV_VERSION, ip); + return put_user(EV_VERSION_1_2, ip);
case EVIOCGID: if (copy_to_user(p, &dev->id, sizeof(struct input_id))) diff --git a/drivers/input/input-compat.c b/drivers/input/input-compat.c index d84d20b..b58d35c 100644 --- a/drivers/input/input-compat.c +++ b/drivers/input/input-compat.c @@ -15,13 +15,13 @@ #ifdef CONFIG_COMPAT
int input_event_from_user(const char __user *buffer, - struct input_event *event) + struct raw_input_event *event) { - if (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) { - struct input_event_compat compat_event; + if (in_compat_syscall()) { + struct raw_input_event_compat compat_event;
if (copy_from_user(&compat_event, buffer, - sizeof(struct input_event_compat))) + sizeof(struct raw_input_event_compat))) return -EFAULT;
event->time.tv_sec = compat_event.time.tv_sec; @@ -31,7 +31,8 @@ int input_event_from_user(const char __user *buffer, event->value = compat_event.value;
} else { - if (copy_from_user(event, buffer, sizeof(struct input_event))) + if (copy_from_user(event, buffer, + sizeof(struct raw_input_event))) return -EFAULT; }
@@ -39,10 +40,10 @@ int input_event_from_user(const char __user *buffer, }
int input_event_to_user(char __user *buffer, - const struct input_event *event) + const struct raw_input_event *event) { - if (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) { - struct input_event_compat compat_event; + if (in_compat_syscall()) { + struct raw_input_event_compat compat_event;
compat_event.time.tv_sec = event->time.tv_sec; compat_event.time.tv_usec = event->time.tv_usec; @@ -51,11 +52,11 @@ int input_event_to_user(char __user *buffer, compat_event.value = event->value;
if (copy_to_user(buffer, &compat_event, - sizeof(struct input_event_compat))) + sizeof(struct raw_input_event_compat))) return -EFAULT;
} else { - if (copy_to_user(buffer, event, sizeof(struct input_event))) + if (copy_to_user(buffer, event, sizeof(struct raw_input_event))) return -EFAULT; }
@@ -100,18 +101,18 @@ int input_ff_effect_from_user(const char __user *buffer, size_t size, #else
int input_event_from_user(const char __user *buffer, - struct input_event *event) + struct raw_input_event *event) { - if (copy_from_user(event, buffer, sizeof(struct input_event))) + if (copy_from_user(event, buffer, sizeof(struct raw_input_event))) return -EFAULT;
return 0; }
int input_event_to_user(char __user *buffer, - const struct input_event *event) + const struct raw_input_event *event) { - if (copy_to_user(buffer, event, sizeof(struct input_event))) + if (copy_to_user(buffer, event, sizeof(struct raw_input_event))) return -EFAULT;
return 0; diff --git a/drivers/input/input-compat.h b/drivers/input/input-compat.h index 1563160..c18132d 100644 --- a/drivers/input/input-compat.h +++ b/drivers/input/input-compat.h @@ -17,8 +17,13 @@
#ifdef CONFIG_COMPAT
-struct input_event_compat { - struct compat_timeval time; +struct input_timeval_compat { + compat_ulong_t tv_sec; + compat_ulong_t tv_usec; +}; + +struct raw_input_event_compat { + struct input_timeval_compat time; __u16 type; __u16 code; __s32 value; @@ -55,24 +60,24 @@ struct ff_effect_compat {
static inline size_t input_event_size(void) { - return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ? - sizeof(struct input_event_compat) : sizeof(struct input_event); + return in_compat_syscall() ? sizeof(struct raw_input_event_compat) : + sizeof(struct raw_input_event); }
#else
static inline size_t input_event_size(void) { - return sizeof(struct input_event); + return sizeof(struct raw_input_event); }
#endif /* CONFIG_COMPAT */
int input_event_from_user(const char __user *buffer, - struct input_event *event); + struct raw_input_event *event);
int input_event_to_user(char __user *buffer, - const struct input_event *event); + const struct raw_input_event *event);
int input_ff_effect_from_user(const char __user *buffer, size_t size, struct ff_effect *effect); diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index 3d75c5a..113a3ae 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -562,7 +562,7 @@ static int uinput_setup_device_legacy(struct uinput_device *udev, static ssize_t uinput_inject_events(struct uinput_device *udev, const char __user *buffer, size_t count) { - struct input_event ev; + struct raw_input_event ev; size_t bytes = 0;
if (count != 0 && count < input_event_size()) @@ -608,7 +608,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, }
static bool uinput_fetch_next_event(struct uinput_device *udev, - struct input_event *event) + struct raw_input_event *event) { bool have_event;
@@ -628,7 +628,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev, static ssize_t uinput_events_to_user(struct uinput_device *udev, char __user *buffer, size_t count) { - struct input_event event; + struct raw_input_event event; size_t read = 0;
while (read + input_event_size() <= count && diff --git a/include/linux/uinput.h b/include/linux/uinput.h index 6527fb7..d1accb3 100644 --- a/include/linux/uinput.h +++ b/include/linux/uinput.h @@ -71,7 +71,7 @@ struct uinput_device { unsigned char ready; unsigned char head; unsigned char tail; - struct input_event buff[UINPUT_BUFFER_SIZE]; + struct raw_input_event buff[UINPUT_BUFFER_SIZE]; int clk_type; unsigned int ff_effects_max;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h index e794f7b..6691d83 100644 --- a/include/uapi/linux/input.h +++ b/include/uapi/linux/input.h @@ -22,6 +22,29 @@ * The event structure itself */
+/* The time structure for y2038 safe raw_input_event. + * The fields use unsigned types to extend times until + * year 2106 rather than 2038. + */ +struct input_timeval { + __kernel_ulong_t tv_sec; + __kernel_ulong_t tv_usec; +}; + +struct raw_input_event { + struct input_timeval time; + __u16 type; + __u16 code; + __s32 value; +}; + +#ifndef __KERNEL__ + +/* Userspace structure. + * Definition maintained here for userspace that is not yet updated to use + * struct raw_input_event. + * Not to be used anywhere within the kernel. + */ struct input_event { struct timeval time; __u16 type; @@ -29,11 +52,35 @@ struct input_event { __s32 value; };
+static inline void +raw_input_to_input_event(const struct raw_input_event *raw, + struct input_event *ev) +{ + ev->time.tv_sec = raw->time.tv_sec; + ev->time.tv_usec = raw->time.tv_usec; + ev->type = raw->type; + ev->code = raw->code; + ev->value = raw->value; +} + +static inline void +input_to_raw_event(const struct input_event *ev, struct raw_input_event *raw) +{ + raw->time.tv_sec = ev->time.tv_sec; + raw->time.tv_usec = ev->time.tv_usec; + raw->type = ev->type; + raw->code = ev->code; + raw->value = ev->value; +} + +#endif + /* * Protocol version. */
#define EV_VERSION 0x010001 +#define EV_VERSION_1_2 0x010002
/* * IOCTLs (0x00 - 0x7f)
struct timeval is not y2038 safe. All references to timeval will be deleted from the kernel to make it y2038 safe. Replace its uses by y2038 safe struct timespec64.
The timestamps changed here only keep track of delta times. These timestamps are also internal to kernel. Hence, monotonic times are sufficient here. The unit of the delta times is also changed in certain cases to nanoseconds rather than microseconds. This is in line with timespec64 which keeps time in nanoseconds.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com Reviewed-by: Arnd Bergmann arnd@arndb.de --- drivers/input/serio/hil_mlc.c | 37 ++++++++++++++++++------------------- drivers/input/serio/hp_sdc.c | 17 +++++++++-------- drivers/input/serio/hp_sdc_mlc.c | 10 +++++----- include/linux/hil_mlc.h | 6 +++--- include/linux/hp_sdc.h | 6 +++--- 5 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c index 65605e4..7c59a79 100644 --- a/drivers/input/serio/hil_mlc.c +++ b/drivers/input/serio/hil_mlc.c @@ -274,14 +274,14 @@ static int hilse_match(hil_mlc *mlc, int unused) /* An LCV used to prevent runaway loops, forces 5 second sleep when reset. */ static int hilse_init_lcv(hil_mlc *mlc, int unused) { - struct timeval tv; + time64_t time;
- do_gettimeofday(&tv); + time = ktime_get_seconds();
- if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5) + if (mlc->lcv && (time - mlc->lcv_tv.tv_sec) < 5) return -1;
- mlc->lcv_tv = tv; + mlc->lcv_tv.tv_sec = time; mlc->lcv = 0;
return 0; @@ -466,7 +466,7 @@ static const struct hilse_node hil_mlc_se[HILSEN_END] = { FUNC(hilse_init_lcv, 0, HILSEN_NEXT, HILSEN_SLEEP, 0)
/* 1 HILSEN_RESTART */ - FUNC(hilse_inc_lcv, 10, HILSEN_NEXT, HILSEN_START, 0) + FUNC(hilse_inc_lcv, 10000, HILSEN_NEXT, HILSEN_START, 0) OUT(HIL_CTRL_ONLY) /* Disable APE */ CTS
@@ -485,7 +485,7 @@ static const struct hilse_node hil_mlc_se[HILSEN_END] = { FUNC(hilse_init_lcv, 0, HILSEN_NEXT, HILSEN_SLEEP, 0)
/* 10 HILSEN_DHR2 */ - FUNC(hilse_inc_lcv, 10, HILSEN_NEXT, HILSEN_START, 0) + FUNC(hilse_inc_lcv, 10000, HILSEN_NEXT, HILSEN_START, 0) FUNC(hilse_set_ddi, -1, HILSEN_NEXT, 0, 0) OUT(HIL_PKT_CMD | HIL_CMD_DHR) IN(300000, HILSEN_DHR2, HILSEN_DHR2, HILSEN_NEXT) @@ -515,7 +515,7 @@ static const struct hilse_node hil_mlc_se[HILSEN_END] = { FUNC(hilse_init_lcv, 0, HILSEN_NEXT, HILSEN_DOZE, 0)
/* 22 HILSEN_ACF2 */ - FUNC(hilse_inc_lcv, 10, HILSEN_NEXT, HILSEN_START, 0) + FUNC(hilse_inc_lcv, 10000, HILSEN_NEXT, HILSEN_START, 0) OUT(HIL_PKT_CMD | HIL_CMD_ACF | 1) IN(20000, HILSEN_NEXT, HILSEN_DSR, HILSEN_NEXT)
@@ -572,7 +572,7 @@ static const struct hilse_node hil_mlc_se[HILSEN_END] = { OUT(HIL_PKT_CMD | HIL_CMD_RPL) EXPECT(HIL_PKT_CMD | HIL_CMD_RPL | HIL_ERR_INT, 20000, HILSEN_NEXT, HILSEN_DSR, HILSEN_NEXT) - FUNC(hilse_operate, 1, HILSEN_OPERATE, HILSEN_IFC, HILSEN_PROBE) + FUNC(hilse_operate, 1000, HILSEN_OPERATE, HILSEN_IFC, HILSEN_PROBE)
/* 58 HILSEN_IFCACF */ OUT(HIL_PKT_CMD | HIL_CMD_IFC) @@ -584,7 +584,6 @@ static const struct hilse_node hil_mlc_se[HILSEN_END] = {
static inline void hilse_setup_input(hil_mlc *mlc, const struct hilse_node *node) { - switch (node->act) { case HILSE_EXPECT_DISC: mlc->imatch = node->object.packet; @@ -605,7 +604,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const struct hilse_node *node } mlc->istarted = 1; mlc->intimeout = node->arg; - do_gettimeofday(&(mlc->instart)); + ktime_get_ts64(&(mlc->instart)); mlc->icount = 15; memset(mlc->ipacket, 0, 16 * sizeof(hil_packet)); BUG_ON(down_trylock(&mlc->isem)); @@ -710,7 +709,7 @@ static int hilse_donode(hil_mlc *mlc) break; } mlc->ostarted = 0; - do_gettimeofday(&(mlc->instart)); + ktime_get_ts64(&(mlc->instart)); write_unlock_irqrestore(&mlc->lock, flags); nextidx = HILSEN_NEXT; break; @@ -731,18 +730,18 @@ static int hilse_donode(hil_mlc *mlc) #endif
while (nextidx & HILSEN_SCHED) { - struct timeval tv; + struct timespec64 ts;
if (!sched_long) goto sched;
- do_gettimeofday(&tv); - tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec); - tv.tv_usec -= mlc->instart.tv_usec; - if (tv.tv_usec >= mlc->intimeout) goto sched; - tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC; - if (!tv.tv_usec) goto sched; - mod_timer(&hil_mlcs_kicker, jiffies + tv.tv_usec); + ktime_get_ts64(&ts); + ts.tv_nsec += NSEC_PER_SEC * (ts.tv_sec - mlc->instart.tv_sec); + ts.tv_nsec -= mlc->instart.tv_nsec; + if (ts.tv_nsec >= mlc->intimeout) goto sched; + ts.tv_nsec = (mlc->intimeout - ts.tv_nsec) * HZ / NSEC_PER_SEC; + if (!ts.tv_nsec) goto sched; + mod_timer(&hil_mlcs_kicker, jiffies + ts.tv_nsec); break; sched: tasklet_schedule(&hil_mlcs_tasklet); diff --git a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c index 852858e..ce8f21b 100644 --- a/drivers/input/serio/hp_sdc.c +++ b/drivers/input/serio/hp_sdc.c @@ -193,7 +193,7 @@ static void hp_sdc_take(int irq, void *dev_id, uint8_t status, uint8_t data) curr->seq[curr->idx++] = status; curr->seq[curr->idx++] = data; hp_sdc.rqty -= 2; - do_gettimeofday(&hp_sdc.rtv); + ktime_get_ts64(&hp_sdc.rtv);
if (hp_sdc.rqty <= 0) { /* All data has been gathered. */ @@ -306,13 +306,13 @@ static void hp_sdc_tasklet(unsigned long foo) write_lock_irq(&hp_sdc.rtq_lock);
if (hp_sdc.rcurr >= 0) { - struct timeval tv; + struct timespec64 ts;
- do_gettimeofday(&tv); - if (tv.tv_sec > hp_sdc.rtv.tv_sec) - tv.tv_usec += USEC_PER_SEC; + ktime_get_ts64(&ts); + if (ts.tv_sec > hp_sdc.rtv.tv_sec) + ts.tv_nsec += NSEC_PER_SEC;
- if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) { + if (ts.tv_nsec - hp_sdc.rtv.tv_nsec > HP_SDC_MAX_REG_DELAY) { hp_sdc_transaction *curr; uint8_t tmp;
@@ -322,7 +322,8 @@ static void hp_sdc_tasklet(unsigned long foo) * it back to the application. and be less verbose. */ printk(KERN_WARNING PREFIX "read timeout (%ius)!\n", - (int)(tv.tv_usec - hp_sdc.rtv.tv_usec)); + (int)(ts.tv_nsec - hp_sdc.rtv.tv_nsec) / + (int)NSEC_PER_USEC); curr->idx += hp_sdc.rqty; hp_sdc.rqty = 0; tmp = curr->seq[curr->actidx]; @@ -551,7 +552,7 @@ unsigned long hp_sdc_put(void)
/* Start a new read */ hp_sdc.rqty = curr->seq[curr->idx]; - do_gettimeofday(&hp_sdc.rtv); + ktime_get_ts64(&hp_sdc.rtv); curr->idx++; /* Still need to lock here in case of spurious irq. */ write_lock_irq(&hp_sdc.rtq_lock); diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c index d50f067..66020cd 100644 --- a/drivers/input/serio/hp_sdc_mlc.c +++ b/drivers/input/serio/hp_sdc_mlc.c @@ -149,7 +149,7 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
/* Try to down the semaphore */ if (down_trylock(&mlc->isem)) { - struct timeval tv; + struct timespec64 ts; if (priv->emtestmode) { mlc->ipacket[0] = HIL_ERR_INT | (mlc->opacket & @@ -160,11 +160,11 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout) /* printk(KERN_DEBUG PREFIX ">[%x]\n", mlc->ipacket[0]); */ goto wasup; } - do_gettimeofday(&tv); - tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec); - if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) { + ktime_get_ts64(&ts); + ts.tv_nsec += NSEC_PER_SEC * (ts.tv_sec - mlc->instart.tv_sec); + if (ts.tv_nsec - mlc->instart.tv_nsec > mlc->intimeout) { /* printk("!%i %i", - tv.tv_usec - mlc->instart.tv_usec, + tv.tv_nsec - mlc->instart.tv_nsec, mlc->intimeout); */ rc = 1; diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h index 394a840..f846730 100644 --- a/include/linux/hil_mlc.h +++ b/include/linux/hil_mlc.h @@ -32,7 +32,7 @@ */
#include <linux/hil.h> -#include <linux/time.h> +#include <linux/time64.h> #include <linux/interrupt.h> #include <linux/semaphore.h> #include <linux/serio.h> @@ -144,12 +144,12 @@ struct hil_mlc { hil_packet ipacket[16]; hil_packet imatch; int icount; - struct timeval instart; + struct timespec64 instart; suseconds_t intimeout;
int ddi; /* Last operational device id */ int lcv; /* LCV to throttle loops */ - struct timeval lcv_tv; /* Time loop was started */ + struct timespec64 lcv_tv; /* Time loop was started */
int di_map[7]; /* Maps below items to live devs */ struct hil_mlc_devinfo di[HIL_MLC_DEVMEM]; diff --git a/include/linux/hp_sdc.h b/include/linux/hp_sdc.h index d392975..d863944 100644 --- a/include/linux/hp_sdc.h +++ b/include/linux/hp_sdc.h @@ -47,9 +47,9 @@ #endif
-/* No 4X status reads take longer than this (in usec). +/* No 4X status reads take longer than this (in nsec). */ -#define HP_SDC_MAX_REG_DELAY 20000 +#define HP_SDC_MAX_REG_DELAY 20000000
typedef void (hp_sdc_irqhook) (int irq, void *dev_id, uint8_t status, uint8_t data); @@ -281,7 +281,7 @@ typedef struct { hp_sdc_transaction *tq[HP_SDC_QUEUE_LEN]; /* All pending read/writes */
int rcurr, rqty; /* Current read transact in process */ - struct timeval rtv; /* Time when current read started */ + struct timespec64 rtv; /* Monotonic time when current read started */ int wcurr; /* Current write transact in process */
int dev_err; /* carries status from registration */
On Mon, Oct 17, 2016 at 08:27:31PM -0700, 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
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 e9ae3d5..b4e3171 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;
you have ktime_get_* helpers below but you don't have one for timespec64 to struct timeval? That seems like a bug waitig to happen.
Cheers, Peter
/* 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.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
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
Hi Deepa,
On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote:
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event. This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
If users are forced to update to adapt to the new event format, should we consider more radical changes? For example, does it make sense to send timestamp on every event? Maybe we should only send it once per event packet (between EV_SYN/SYN_REPORT)? What granularity do we need? Is there anything else in current protocol that we'd like to change?
Thanks.
On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote:
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event.
general comment here - please don't name it "raw_input_event". First, when you grep for input_event you want the new ones to show up too, so a struct input_event_raw would be better here. That also has better namespacing in general. Second though: the event isn't any more "raw" than the previous we had.
I can't think of anything better than struct input_event_v2 though.
This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
Doesn't this break *all* of userspace then? I don't see anything to negotiate the type of input event the kernel gives me. And nothing right now checks for EVDEV_VERSION, so they all just assume it's a struct input_event. Best case, if the available events aren't a multiple of sizeof(struct input_event) userspace will bomb out, but unless that happens, everyone will just happily read old-style events.
So we need some negotiation what is acceptable. Which also needs to address the race conditions we're going to get when events start coming in before the client has announced that it supports the new-style events.
Cheers, Peter
Suggested-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com Reviewed-by: Arnd Bergmann arnd@arndb.de
drivers/input/evdev.c | 20 +++++++++---------- drivers/input/input-compat.c | 29 ++++++++++++++------------- drivers/input/input-compat.h | 19 +++++++++++------- drivers/input/misc/uinput.c | 6 +++--- include/linux/uinput.h | 2 +- include/uapi/linux/input.h | 47 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 88 insertions(+), 35 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index b4e3171..459e3ba 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -60,7 +60,7 @@ struct evdev_client { bool revoked; unsigned long *evmasks[EV_CNT]; unsigned int bufsize;
- struct input_event buffer[];
- struct raw_input_event buffer[];
}; static size_t evdev_get_mask_cnt(unsigned int type) @@ -113,7 +113,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type) unsigned int i, head, num; unsigned int mask = client->bufsize - 1; bool is_report;
- struct input_event *ev;
- struct raw_input_event *ev;
BUG_ON(type == EV_SYN); @@ -155,7 +155,7 @@ 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;
- struct raw_input_event ev; struct timespec64 ts;
switch (client->clk_type) { @@ -236,7 +236,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid) } static void __pass_event(struct evdev_client *client,
const struct input_event *event)
const struct raw_input_event *event){ client->buffer[client->head++] = *event; client->head &= client->bufsize - 1; @@ -268,7 +268,7 @@ static void evdev_pass_values(struct evdev_client *client, { struct evdev *evdev = client->evdev; const struct input_value *v;
- struct input_event event;
- struct raw_input_event event; struct timespec64 ts; bool wakeup = false;
@@ -507,7 +507,7 @@ static int evdev_open(struct inode *inode, struct file *file) struct evdev *evdev = container_of(inode->i_cdev, struct evdev, cdev); unsigned int bufsize = evdev_compute_buffer_size(evdev->handle.dev); unsigned int size = sizeof(struct evdev_client) +
bufsize * sizeof(struct input_event);
bufsize * sizeof(struct raw_input_event);@@ -542,7 +542,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer, { struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev;
- struct input_event event;
- struct raw_input_event event; int retval = 0;
if (count != 0 && count < input_event_size()) @@ -575,7 +575,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer, } static int evdev_fetch_next_event(struct evdev_client *client,
struct input_event *event)
struct raw_input_event *event){ int have_event; @@ -597,7 +597,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer, { struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev;
- struct input_event event;
- struct raw_input_event event; size_t read = 0; int error;
@@ -1083,7 +1083,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, switch (cmd) { case EVIOCGVERSION:
return put_user(EV_VERSION, ip);
return put_user(EV_VERSION_1_2, ip);case EVIOCGID: if (copy_to_user(p, &dev->id, sizeof(struct input_id))) diff --git a/drivers/input/input-compat.c b/drivers/input/input-compat.c index d84d20b..b58d35c 100644 --- a/drivers/input/input-compat.c +++ b/drivers/input/input-compat.c @@ -15,13 +15,13 @@ #ifdef CONFIG_COMPAT int input_event_from_user(const char __user *buffer,
struct input_event *event)
struct raw_input_event *event){
- if (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) {
struct input_event_compat compat_event;
- if (in_compat_syscall()) {
struct raw_input_event_compat compat_event;if (copy_from_user(&compat_event, buffer,
sizeof(struct input_event_compat)))
sizeof(struct raw_input_event_compat))) return -EFAULT;event->time.tv_sec = compat_event.time.tv_sec; @@ -31,7 +31,8 @@ int input_event_from_user(const char __user *buffer, event->value = compat_event.value; } else {
if (copy_from_user(event, buffer, sizeof(struct input_event)))
if (copy_from_user(event, buffer,sizeof(struct raw_input_event))) return -EFAULT;@@ -39,10 +40,10 @@ int input_event_from_user(const char __user *buffer, } int input_event_to_user(char __user *buffer,
const struct input_event *event)
const struct raw_input_event *event){
- if (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) {
struct input_event_compat compat_event;
- if (in_compat_syscall()) {
struct raw_input_event_compat compat_event;compat_event.time.tv_sec = event->time.tv_sec; compat_event.time.tv_usec = event->time.tv_usec; @@ -51,11 +52,11 @@ int input_event_to_user(char __user *buffer, compat_event.value = event->value; if (copy_to_user(buffer, &compat_event,
sizeof(struct input_event_compat)))
sizeof(struct raw_input_event_compat))) return -EFAULT;} else {
if (copy_to_user(buffer, event, sizeof(struct input_event)))
if (copy_to_user(buffer, event, sizeof(struct raw_input_event))) return -EFAULT;@@ -100,18 +101,18 @@ int input_ff_effect_from_user(const char __user *buffer, size_t size, #else int input_event_from_user(const char __user *buffer,
struct input_event *event)
struct raw_input_event *event){
- if (copy_from_user(event, buffer, sizeof(struct input_event)))
- if (copy_from_user(event, buffer, sizeof(struct raw_input_event))) return -EFAULT;
return 0; } int input_event_to_user(char __user *buffer,
const struct input_event *event)
const struct raw_input_event *event){
- if (copy_to_user(buffer, event, sizeof(struct input_event)))
- if (copy_to_user(buffer, event, sizeof(struct raw_input_event))) return -EFAULT;
return 0; diff --git a/drivers/input/input-compat.h b/drivers/input/input-compat.h index 1563160..c18132d 100644 --- a/drivers/input/input-compat.h +++ b/drivers/input/input-compat.h @@ -17,8 +17,13 @@ #ifdef CONFIG_COMPAT -struct input_event_compat {
- struct compat_timeval time;
+struct input_timeval_compat {
- compat_ulong_t tv_sec;
- compat_ulong_t tv_usec;
+};
+struct raw_input_event_compat {
- struct input_timeval_compat time; __u16 type; __u16 code; __s32 value;
@@ -55,24 +60,24 @@ struct ff_effect_compat { static inline size_t input_event_size(void) {
- return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?
sizeof(struct input_event_compat) : sizeof(struct input_event);
- return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :
sizeof(struct raw_input_event);} #else static inline size_t input_event_size(void) {
- return sizeof(struct input_event);
- return sizeof(struct raw_input_event);
} #endif /* CONFIG_COMPAT */ int input_event_from_user(const char __user *buffer,
struct input_event *event);
struct raw_input_event *event);int input_event_to_user(char __user *buffer,
const struct input_event *event);
const struct raw_input_event *event);int input_ff_effect_from_user(const char __user *buffer, size_t size, struct ff_effect *effect); diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index 3d75c5a..113a3ae 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -562,7 +562,7 @@ static int uinput_setup_device_legacy(struct uinput_device *udev, static ssize_t uinput_inject_events(struct uinput_device *udev, const char __user *buffer, size_t count) {
- struct input_event ev;
- struct raw_input_event ev; size_t bytes = 0;
if (count != 0 && count < input_event_size()) @@ -608,7 +608,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, } static bool uinput_fetch_next_event(struct uinput_device *udev,
struct input_event *event)
struct raw_input_event *event){ bool have_event; @@ -628,7 +628,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev, static ssize_t uinput_events_to_user(struct uinput_device *udev, char __user *buffer, size_t count) {
- struct input_event event;
- struct raw_input_event event; size_t read = 0;
while (read + input_event_size() <= count && diff --git a/include/linux/uinput.h b/include/linux/uinput.h index 6527fb7..d1accb3 100644 --- a/include/linux/uinput.h +++ b/include/linux/uinput.h @@ -71,7 +71,7 @@ struct uinput_device { unsigned char ready; unsigned char head; unsigned char tail;
- struct input_event buff[UINPUT_BUFFER_SIZE];
- struct raw_input_event buff[UINPUT_BUFFER_SIZE]; int clk_type; unsigned int ff_effects_max;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h index e794f7b..6691d83 100644 --- a/include/uapi/linux/input.h +++ b/include/uapi/linux/input.h @@ -22,6 +22,29 @@
- The event structure itself
*/ +/* The time structure for y2038 safe raw_input_event.
- The fields use unsigned types to extend times until
- year 2106 rather than 2038.
- */
+struct input_timeval {
- __kernel_ulong_t tv_sec;
- __kernel_ulong_t tv_usec;
+};
+struct raw_input_event {
- struct input_timeval time;
- __u16 type;
- __u16 code;
- __s32 value;
+};
+#ifndef __KERNEL__
+/* Userspace structure.
- Definition maintained here for userspace that is not yet updated to use
- struct raw_input_event.
- Not to be used anywhere within the kernel.
- */
struct input_event { struct timeval time; __u16 type; @@ -29,11 +52,35 @@ struct input_event { __s32 value; }; +static inline void +raw_input_to_input_event(const struct raw_input_event *raw,
struct input_event *ev)+{
- ev->time.tv_sec = raw->time.tv_sec;
- ev->time.tv_usec = raw->time.tv_usec;
- ev->type = raw->type;
- ev->code = raw->code;
- ev->value = raw->value;
+}
+static inline void +input_to_raw_event(const struct input_event *ev, struct raw_input_event *raw) +{
- raw->time.tv_sec = ev->time.tv_sec;
- raw->time.tv_usec = ev->time.tv_usec;
- raw->type = ev->type;
- raw->code = ev->code;
- raw->value = ev->value;
+}
+#endif
/*
- Protocol version.
*/ #define EV_VERSION 0x010001 +#define EV_VERSION_1_2 0x010002 /*
- IOCTLs (0x00 - 0x7f)
-- 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
On Thursday, October 27, 2016 11:34:33 AM CEST Peter Hutterer wrote:
@@ -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;you have ktime_get_* helpers below but you don't have one for timespec64 to struct timeval? That seems like a bug waitig to happen.
This is intentional to a certain degree: we don't have a timeval64 because any conversion to a new interface should prefer timespec64 or 64-bit nanoseconds, and we try to remove timeval (along with timespec) from everywhere in the kernel because basically all uses are problematic for y2038.
Note that after patch 3, event->time is no longer a 'timeval' either, so even if we had a conversion function, we could no longer use it here.
Arnd
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?
This is for input events queued by the uinput driver for the virtual input device. This can be read through uinput_read() fops. I don't think anybody is doing a read on uinput nodes, so another option(Arnd and I considered this) could be not supporting reads on these nodes at all.
This is not related to evdev events in the kernel. Currently, this timestamp could be the same format as the evdev timestamps or not.
-Deepa
On Wed, Oct 26, 2016 at 7:56 PM, Peter Hutterer peter.hutterer@who-t.net wrote:
On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote:
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event.
general comment here - please don't name it "raw_input_event". First, when you grep for input_event you want the new ones to show up too, so a struct input_event_raw would be better here. That also has better namespacing in general. Second though: the event isn't any more "raw" than the previous we had.
I can't think of anything better than struct input_event_v2 though.
The general idea was to leave the original struct input_event as a common interface for userspace (as it cannot be deleted). So reading raw data unformatted by the userspace will have the new struct raw_input_event format. This was the reason for the "raw" in the name.
struct input_event_v2 is fine too, if this is more preferred.
This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
Doesn't this break *all* of userspace then? I don't see anything to negotiate the type of input event the kernel gives me. And nothing right now checks for EVDEV_VERSION, so they all just assume it's a struct input_event. Best case, if the available events aren't a multiple of sizeof(struct input_event) userspace will bomb out, but unless that happens, everyone will just happily read old-style events.
So we need some negotiation what is acceptable. Which also needs to address the race conditions we're going to get when events start coming in before the client has announced that it supports the new-style events.
No, this does not break any userspace right now. Both struct input_event and struct raw_input_event are exactly the same today. This will be the case until a 2038-safe glibc is used with a 64 bit time_t flag.
So these are the scenarios: 1. old kernel driver + new userspace -- should still be ok until 2038. Version checks could help discover these 2. new kernel driver + old userspace (without recompiled with new 2038 gblic) -- works because the format is really the same.
The patch I posted to libevdev checks this driver version. And, hence any library that results in a call to libevdev_set_fd() will fail if it is not this updated driver. We could just do a similar check in every library also. I think the latter would be better.
So, the kernel patches can go in as a no-op right now and then I can add version checks to respective user space libraries.
-Deepa
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event. This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
If users are forced to update to adapt to the new event format, should we consider more radical changes? For example, does it make sense to send timestamp on every event? Maybe we should only send it once per event packet (between EV_SYN/SYN_REPORT)? What granularity do we need? Is there anything else in current protocol that we'd like to change?
I did see the thread with Pingbo's patches where you had a similar comment.
I see my series as decoupling the kernel input event format from the userspace format. The formats also are really the same still. Could this be considered the first step towards changing the protocol?
The protocol changes might need new interfaces to be defined between libraries. And, could end up being a substantial change. Would a step by step approach make sense?
-Deepa
On Thu, Oct 27, 2016 at 03:25:43PM -0700, Deepa Dinamani wrote:
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event. This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
If users are forced to update to adapt to the new event format, should we consider more radical changes? For example, does it make sense to send timestamp on every event? Maybe we should only send it once per event packet (between EV_SYN/SYN_REPORT)? What granularity do we need? Is there anything else in current protocol that we'd like to change?
I did see the thread with Pingbo's patches where you had a similar comment.
I see my series as decoupling the kernel input event format from the userspace format. The formats also are really the same still. Could this be considered the first step towards changing the protocol?
I really do not see the point. I think we agree that the current protocol is not working past 2038 and it does not seem we can fix it transparently for the user. So we need to define new protocol and let kernel and clients negotiate which one is used.
I am not concerned about in-kernel representation much as it does not get stored anywhere so we can adjust it as needed without too much effort.
The protocol changes might need new interfaces to be defined between libraries. And, could end up being a substantial change. Would a step by step approach make sense?
It would depend largely on the scope.
Thanks.
On Thu, Oct 27, 2016 at 01:39:30PM -0700, Deepa Dinamani wrote:
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?
This is for input events queued by the uinput driver for the virtual input device.
oh, right. I thought this was in the path for uinput_write(). sorry about that.
This can be read through uinput_read() fops. I don't think anybody is doing a read on uinput nodes, so another option(Arnd and I considered this) could be not supporting reads on these nodes at all.
This is not related to evdev events in the kernel. Currently, this timestamp could be the same format as the evdev timestamps or not.
I can say I've never done the read from the uinput device, never even occured to me. quick skim of the code looks like this only matters for force_feedback stuff. can't really comment on that too much.
Cheers, Peter
On Thu, Oct 27, 2016 at 03:24:55PM -0700, Deepa Dinamani wrote:
On Wed, Oct 26, 2016 at 7:56 PM, Peter Hutterer peter.hutterer@who-t.net wrote:
On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote:
struct timeval is not y2038 safe. All usage of timeval in the kernel will be replaced by y2038 safe structures.
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.
Leave the original input_event as is. This is to maintain backward compatibility with existing userspace interfaces that use input_event. Introduce a new replacement struct raw_input_event.
general comment here - please don't name it "raw_input_event". First, when you grep for input_event you want the new ones to show up too, so a struct input_event_raw would be better here. That also has better namespacing in general. Second though: the event isn't any more "raw" than the previous we had.
I can't think of anything better than struct input_event_v2 though.
The general idea was to leave the original struct input_event as a common interface for userspace (as it cannot be deleted). So reading raw data unformatted by the userspace will have the new struct raw_input_event format. This was the reason for the "raw" in the name.
struct input_event_v2 is fine too, if this is more preferred.
This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
Doesn't this break *all* of userspace then? I don't see anything to negotiate the type of input event the kernel gives me. And nothing right now checks for EVDEV_VERSION, so they all just assume it's a struct input_event. Best case, if the available events aren't a multiple of sizeof(struct input_event) userspace will bomb out, but unless that happens, everyone will just happily read old-style events.
So we need some negotiation what is acceptable. Which also needs to address the race conditions we're going to get when events start coming in before the client has announced that it supports the new-style events.
No, this does not break any userspace right now. Both struct input_event and struct raw_input_event are exactly the same today.
oh, right, the ABI is the same. I see that now, thanks.
This will be the case until a 2038-safe glibc is used with a 64 bit time_t flag.
So these are the scenarios:
- old kernel driver + new userspace
-- should still be ok until 2038. Version checks could help discover these 2. new kernel driver + old userspace (without recompiled with new 2038 gblic) -- works because the format is really the same.
The patch I posted to libevdev checks this driver version.
btw, where did you post the libevdev patch? I haven't seen it anywhere I'm subscribed to.
And, hence any library that results in a call to libevdev_set_fd() will fail if it is not this updated driver.
without having seen the libevdev patch - that sounds like a bad idea . there are plenty of usecases where libevdev_set_fd() is called but timestamps in events just don't matter. So we may need need some more negotiation between the library user, libevdev and the kernel.
Cheers, Peter
We could just do a similar check in every library also. I think the latter would be better.
So, the kernel patches can go in as a no-op right now and then I can add version checks to respective user space libraries.
On Thursday, October 27, 2016 4:12:54 PM CEST Dmitry Torokhov wrote:
On Thu, Oct 27, 2016 at 03:25:43PM -0700, Deepa Dinamani wrote:
If users are forced to update to adapt to the new event format, should we consider more radical changes? For example, does it make sense to send timestamp on every event? Maybe we should only send it once per event packet (between EV_SYN/SYN_REPORT)? What granularity do we need? Is there anything else in current protocol that we'd like to change?
I did see the thread with Pingbo's patches where you had a similar comment.
I see my series as decoupling the kernel input event format from the userspace format. The formats also are really the same still. Could this be considered the first step towards changing the protocol?
I really do not see the point. I think we agree that the current protocol is not working past 2038 and it does not seem we can fix it transparently for the user. So we need to define new protocol and let kernel and clients negotiate which one is used.
This work is not primarily about fixing the protocol to work beyond 2038 (although as a side-effect it will work until 2106). The main intention here is to not break existing applications when they get recompiled against a C library that defines time_t as 64-bit.
I am not concerned about in-kernel representation much as it does not get stored anywhere so we can adjust it as needed without too much effort.
The protocol changes might need new interfaces to be defined between libraries. And, could end up being a substantial change. Would a step by step approach make sense?
It would depend largely on the scope.
I think we should do those two things completely independently. We need to do something now to preserve the current interfaces for the glibc changes that are coming soon [1], and Deepa's patches do that (though I now realize the changelog doesn't mention the requirement).
An overhaul of the input_event handling with a new modern but incompatible format may or may not be a good idea, but this should be decided independently.
Arnd
On Monday, October 17, 2016 8:27:32 PM CEST Deepa Dinamani wrote:
@@ -55,24 +60,24 @@ struct ff_effect_compat { static inline size_t input_event_size(void) {
return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?sizeof(struct input_event_compat) : sizeof(struct input_event);
return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :sizeof(struct raw_input_event);}
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
Arnd
On Friday, October 28, 2016 2:46:42 PM CEST Peter Hutterer wrote:
On Thu, Oct 27, 2016 at 03:24:55PM -0700, Deepa Dinamani wrote:
On Wed, Oct 26, 2016 at 7:56 PM, Peter Hutterer peter.hutterer@who-t.net wrote:
On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote: general comment here - please don't name it "raw_input_event". First, when you grep for input_event you want the new ones to show up too, so a struct input_event_raw would be better here. That also has better namespacing in general. Second though: the event isn't any more "raw" than the previous we had.
I can't think of anything better than struct input_event_v2 though.
The general idea was to leave the original struct input_event as a common interface for userspace (as it cannot be deleted). So reading raw data unformatted by the userspace will have the new struct raw_input_event format. This was the reason for the "raw" in the name.
struct input_event_v2 is fine too, if this is more preferred.
I think input_event_v2 would be more confusing. An alternative to raw_input_event might be __kernel_input_event, which parallels things like __kernel_off_t that is also independent from the user space off_t in the same way that the user space input_event structure will get redefined in a way that is incompatible with the kernel ABI.
This replaces timeval with struct input_timeval. This structure maintains time in __kernel_ulong_t or compat_ulong_t to allow for architectures to override types as in the case of x32.
The change requires any userspace utilities reading or writing from event nodes to update their reading format to match raw_input_event. The changes to the popular libraries will be posted along with the kernel changes. The driver version is also updated to reflect the change in event format.
Doesn't this break *all* of userspace then? I don't see anything to negotiate the type of input event the kernel gives me. And nothing right now checks for EVDEV_VERSION, so they all just assume it's a struct input_event. Best case, if the available events aren't a multiple of sizeof(struct input_event) userspace will bomb out, but unless that happens, everyone will just happily read old-style events.
So we need some negotiation what is acceptable. Which also needs to address the race conditions we're going to get when events start coming in before the client has announced that it supports the new-style events.
No, this does not break any userspace right now. Both struct input_event and struct raw_input_event are exactly the same today.
oh, right, the ABI is the same. I see that now, thanks.
One minor difference is that the seconds in raw_input_event are 'unsigned', so even with the 'real' time domain, we can represent times from 1970 to 2106, whereas 'timeval' represents times between 1902 and 2038.
Once user space has a 64-bit time_t and the conversion function in libinput that Deepa suggested, the raw_input_event is only used on the kernel ABI and the normal timestamps will work fine.
And, hence any library that results in a call to libevdev_set_fd() will fail if it is not this updated driver.
without having seen the libevdev patch - that sounds like a bad idea . there are plenty of usecases where libevdev_set_fd() is called but timestamps in events just don't matter. So we may need need some more negotiation between the library user, libevdev and the kernel.
I also don't see any strict dependency at all: the binary data format has not changed, and I agree we absolutely should not break running a newly built library on old kernels.
We can also safely assume that any user space that is actually built for 64-bit time_t is also running on a recent enough kernel, as today's kernels do not support 64-bit time_t yet.
Arnd
On Fri, Oct 28, 2016 at 5:43 AM, Arnd Bergmann arnd@arndb.de wrote:
On Monday, October 17, 2016 8:27:32 PM CEST Deepa Dinamani wrote:
@@ -55,24 +60,24 @@ struct ff_effect_compat {
static inline size_t input_event_size(void) {
return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?sizeof(struct input_event_compat) : sizeof(struct input_event);
return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :sizeof(struct raw_input_event);}
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
There is no time_t anymore in the raw_input_event structure. The struct uses __kernel_ulong_t type. This should take care of x32 support.
From this cover letter:
https://www.spinics.net/lists/linux-arch/msg16356.html
I see that that the __kernel types were introduced to address the ABI issues for x32.
-Deepa
On Friday, October 28, 2016 8:19:46 AM CEST Deepa Dinamani wrote:
On Fri, Oct 28, 2016 at 5:43 AM, Arnd Bergmann arnd@arndb.de wrote:
On Monday, October 17, 2016 8:27:32 PM CEST Deepa Dinamani wrote:
@@ -55,24 +60,24 @@ struct ff_effect_compat {
static inline size_t input_event_size(void) {
return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?sizeof(struct input_event_compat) : sizeof(struct input_event);
return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :sizeof(struct raw_input_event);}
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
There is no time_t anymore in the raw_input_event structure. The struct uses __kernel_ulong_t type. This should take care of x32 support.
I don't think it does.
From this cover letter: https://www.spinics.net/lists/linux-arch/msg16356.html
I see that that the __kernel types were introduced to address the ABI issues for x32.
This is a variation of the problem we are trying to solve for the other architectures in your patch set:
On x32, the kernel uses produces a structure with the 64-bit layout, using __u64 tv_sec, to match the current user space that has 64-bit __kernel_ulong_t and 64-bit time_t, but in_compat_syscall() also returns 'true' here, as this is mostly a 32-bit ABI (time_t being one of the exceptions).
ARnd
@@ -55,24 +60,24 @@ struct ff_effect_compat {
static inline size_t input_event_size(void) {
return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?sizeof(struct input_event_compat) : sizeof(struct input_event);
return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :sizeof(struct raw_input_event);}
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
There is no time_t anymore in the raw_input_event structure. The struct uses __kernel_ulong_t type. This should take care of x32 support.
I don't think it does.
From this cover letter: https://www.spinics.net/lists/linux-arch/msg16356.html
I see that that the __kernel types were introduced to address the ABI issues for x32.
This is a variation of the problem we are trying to solve for the other architectures in your patch set:
On x32, the kernel uses produces a structure with the 64-bit layout, using __u64 tv_sec, to match the current user space that has 64-bit __kernel_ulong_t and 64-bit time_t, but in_compat_syscall() also returns 'true' here, as this is mostly a 32-bit ABI (time_t being one of the exceptions).
Yes, I missed this.
in_compat_syscall() is true for x32, this would mean we end up here even if it is a x32 syscall. But, wouldn't it be better to use in_x32_syscall() here since there is no timeval any more?
Thanks, Deepa
On Friday, October 28, 2016 2:39:35 PM CEST Deepa Dinamani wrote:
@@ -55,24 +60,24 @@ struct ff_effect_compat {
static inline size_t input_event_size(void) {
return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?sizeof(struct input_event_compat) : sizeof(struct input_event);
return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :sizeof(struct raw_input_event);}
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
There is no time_t anymore in the raw_input_event structure. The struct uses __kernel_ulong_t type. This should take care of x32 support.
I don't think it does.
From this cover letter: https://www.spinics.net/lists/linux-arch/msg16356.html
I see that that the __kernel types were introduced to address the ABI issues for x32.
This is a variation of the problem we are trying to solve for the other architectures in your patch set:
On x32, the kernel uses produces a structure with the 64-bit layout, using __u64 tv_sec, to match the current user space that has 64-bit __kernel_ulong_t and 64-bit time_t, but in_compat_syscall() also returns 'true' here, as this is mostly a 32-bit ABI (time_t being one of the exceptions).
Yes, I missed this.
in_compat_syscall() is true for x32, this would mean we end up here even if it is a x32 syscall. But, wouldn't it be better to use in_x32_syscall() here since there is no timeval any more?
We have to distinguish four cases on x86:
- native 32-bit, input_event with 32-bit time_t - compat 32-bit, input_event_compat with 32-bit time_t - native 64-bit, input_event with 64-bit time_t - compat x32, input_event with 64-bit time_t
The first three can happen on other architectures too, the last one is x86 specific. There are probably other ways to express the condition above, but I can't think of one that is better than the one we have today.
Arnd
On Fri, Oct 28, 2016 at 2:47 PM, Arnd Bergmann arnd@arndb.de wrote:
On Friday, October 28, 2016 2:39:35 PM CEST Deepa Dinamani wrote:
@@ -55,24 +60,24 @@ struct ff_effect_compat {
static inline size_t input_event_size(void) {
return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ?sizeof(struct input_event_compat) : sizeof(struct input_event);
return in_compat_syscall() ? sizeof(struct raw_input_event_compat) :sizeof(struct raw_input_event);}
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
There is no time_t anymore in the raw_input_event structure. The struct uses __kernel_ulong_t type. This should take care of x32 support.
I don't think it does.
From this cover letter: https://www.spinics.net/lists/linux-arch/msg16356.html
I see that that the __kernel types were introduced to address the ABI issues for x32.
This is a variation of the problem we are trying to solve for the other architectures in your patch set:
On x32, the kernel uses produces a structure with the 64-bit layout, using __u64 tv_sec, to match the current user space that has 64-bit __kernel_ulong_t and 64-bit time_t, but in_compat_syscall() also returns 'true' here, as this is mostly a 32-bit ABI (time_t being one of the exceptions).
Yes, I missed this.
in_compat_syscall() is true for x32, this would mean we end up here even if it is a x32 syscall. But, wouldn't it be better to use in_x32_syscall() here since there is no timeval any more?
We have to distinguish four cases on x86:
- native 32-bit, input_event with 32-bit time_t
- compat 32-bit, input_event_compat with 32-bit time_t
- native 64-bit, input_event with 64-bit time_t
- compat x32, input_event with 64-bit time_t
The first three can happen on other architectures too, the last one is x86 specific. There are probably other ways to express the condition above, but I can't think of one that is better than the one we have today.
Can we detect if given task is compat x32, like we do for compat 64/32? Or entire userspace has to be x32?
Thanks.
On Friday, October 28, 2016 2:56:10 PM CEST Dmitry Torokhov wrote:
On Fri, Oct 28, 2016 at 2:47 PM, Arnd Bergmann arnd@arndb.de wrote:
On Friday, October 28, 2016 2:39:35 PM CEST Deepa Dinamani wrote:
> @@ -55,24 +60,24 @@ struct ff_effect_compat { > > static inline size_t input_event_size(void) > { > - return (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) ? > - sizeof(struct input_event_compat) : sizeof(struct input_event); > + return in_compat_syscall() ? sizeof(struct raw_input_event_compat) : > + sizeof(struct raw_input_event); > }
I think the COMPAT_USE_64BIT_TIME check has to stay here, it's needed for x32 mode on x86-64.
We have to distinguish four cases on x86:
- native 32-bit, input_event with 32-bit time_t
- compat 32-bit, input_event_compat with 32-bit time_t
- native 64-bit, input_event with 64-bit time_t
- compat x32, input_event with 64-bit time_t
The first three can happen on other architectures too, the last one is x86 specific. There are probably other ways to express the condition above, but I can't think of one that is better than the one we have today.
Can we detect if given task is compat x32, like we do for compat 64/32? Or entire userspace has to be x32?
Yes, this works fine per task, with the definition of COMPAT_USE_64BIT_TIME that is hardcoded to zero everywhere except on x86 where it is
#define COMPAT_USE_64BIT_TIME \ (!!(task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT))
This is unrelated to the patch in question, the existing code is correct as long as we don't change the logic and just replace input_event with raw_input_event (or __kernel_input_event or whichever you prefer).
Arnd
btw, where did you post the libevdev patch? I haven't seen it anywhere I'm subscribed to.
The libevdev patch was posted to input-tools@lists.freedesktop.org : https://www.mail-archive.com/y2038@lists.linaro.org/msg01824.html
-Deepa
I think we should do those two things completely independently. We need to do something now to preserve the current interfaces for the glibc changes that are coming soon [1], and Deepa's patches do that (though I now realize the changelog doesn't mention the requirement).
I'll update the changelog in the next version of the series along with the task check for x32 mode.
An overhaul of the input_event handling with a new modern but incompatible format may or may not be a good idea, but this should be decided independently.
Just to clarify, we are going with the plan that Arnd suggested: leaving the overhaul of the input event protocol alone and proceeding with the series according to suggested changes. Are we in agreement over this?
-Deepa
On Sat, Oct 29, 2016 at 09:19:05PM -0700, Deepa Dinamani wrote:
btw, where did you post the libevdev patch? I haven't seen it anywhere I'm subscribed to.
The libevdev patch was posted to input-tools@lists.freedesktop.org : https://www.mail-archive.com/y2038@lists.linaro.org/msg01824.html
ah, right. that would do it. input-tools@ discarded all posts by nonmembers (guess that explains why we didn't get much spam :)
I changed that over now, please re-send to the list, thanks.
Cheers, Peter
-
Arnd Bergmann -
Deepa Dinamani -
Dmitry Torokhov -
Peter Hutterer