| Message ID | 20171207181306.5623-2-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 7, 2017 at 7:13 PM, Deepa Dinamani <deepa.kernel@gmail.com> 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> Reviewed-by: Arnd Bergmann <arnd@arndb.de> -- 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 Thu, 2017-12-07 at 10:13 -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. Why is this change not opt-in, as for evdev? I assume there were compatibility reasons for not changing evdev's clock by default, so I would expect them to apply to uinput as well. (But I'm also prepared to believe that user-space is now generally compatible with and would prefer monotonic time from all input devices.) Ben. > 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 | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c > index 39ddd9a73feb..d521aecbc078 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);
On Thu, 2017-12-14 at 21:17 +0000, Ben Hutchings wrote: > On Thu, 2017-12-07 at 10:13 -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. > > Why is this change not opt-in, as for evdev? I assume there were > compatibility reasons for not changing evdev's clock by default, so I > would expect them to apply to uinput as well. (But I'm also prepared > to believe that user-space is now generally compatible with and would > prefer monotonic time from all input devices.) Never mind, I've gone back and seen Arnd's comments about compatibility on v3. It might be worth copying those into the commit message though. Ben. > Ben. > > > 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 | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/input/misc/uinput.c > > b/drivers/input/misc/uinput.c > > index 39ddd9a73feb..d521aecbc078 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);
On Thu, Dec 14, 2017 at 1:18 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > On Thu, 2017-12-14 at 21:17 +0000, Ben Hutchings wrote: >> On Thu, 2017-12-07 at 10:13 -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. >> >> Why is this change not opt-in, as for evdev? I assume there were >> compatibility reasons for not changing evdev's clock by default, so I >> would expect them to apply to uinput as well. (But I'm also prepared >> to believe that user-space is now generally compatible with and would >> prefer monotonic time from all input devices.) > > Never mind, I've gone back and seen Arnd's comments about compatibility > on v3. It might be worth copying those into the commit message though. Commit message already talks about this assumption?: The patch switches the timestamps to use monotonic time from realtime time. This is assuming no one is using absolute times from these timestamps. -Deepa -- 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 Thu, 2017-12-14 at 13:44 -0800, Deepa Dinamani wrote: > On Thu, Dec 14, 2017 at 1:18 PM, Ben Hutchings > > <ben.hutchings@codethink.co.uk> wrote: > > On Thu, 2017-12-14 at 21:17 +0000, Ben Hutchings wrote: > > > On Thu, 2017-12-07 at 10:13 -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. > > > > > > Why is this change not opt-in, as for evdev? I assume there were > > > compatibility reasons for not changing evdev's clock by default, so I > > > would expect them to apply to uinput as well. (But I'm also prepared > > > to believe that user-space is now generally compatible with and would > > > prefer monotonic time from all input devices.) > > > > Never mind, I've gone back and seen Arnd's comments about compatibility > > on v3. It might be worth copying those into the commit message though. > > Commit message already talks about this assumption?: > > The patch switches the timestamps to use monotonic time > from realtime time. This is assuming no one is using > absolute times from these timestamps. Yes, but Arnd did a bit of code research to check that assumption. A commit message that says "we checked and it appears that no user- space depends on this" looks better than "I assume that no user-space depends on this". Ben.
On Thu, Dec 14, 2017 at 1:53 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > On Thu, 2017-12-14 at 13:44 -0800, Deepa Dinamani wrote: >> On Thu, Dec 14, 2017 at 1:18 PM, Ben Hutchings >> > <ben.hutchings@codethink.co.uk> wrote: >> > On Thu, 2017-12-14 at 21:17 +0000, Ben Hutchings wrote: >> > > On Thu, 2017-12-07 at 10:13 -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. >> > > >> > > Why is this change not opt-in, as for evdev? I assume there were >> > > compatibility reasons for not changing evdev's clock by default, so I >> > > would expect them to apply to uinput as well. (But I'm also prepared >> > > to believe that user-space is now generally compatible with and would >> > > prefer monotonic time from all input devices.) >> > >> > Never mind, I've gone back and seen Arnd's comments about compatibility >> > on v3. It might be worth copying those into the commit message though. >> >> Commit message already talks about this assumption?: >> >> The patch switches the timestamps to use monotonic time >> from realtime time. This is assuming no one is using >> absolute times from these timestamps. > > Yes, but Arnd did a bit of code research to check that assumption. > A commit message that says "we checked and it appears that no user- > space depends on this" looks better than "I assume that no user-space > depends on this". The fact is we do not know all the places this is used. Arnd happened to find an instance. This is why Dmitry suggested that we will provide a ioctl or something if someone complains. It is not my assumption. It is the assumption that the patch is based on. This is to call people's attention to the fact that this is controversial. And, if they do not agree with the assumption, they should complain. So it does not matter we found an instance where the assumption is true. -Deepa -- 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
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index 39ddd9a73feb..d521aecbc078 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);
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> --- drivers/input/misc/uinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)