| Message ID | 20230427000152.1407471-1-biswarupp@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Call input_set_timestamp for events injected using uinput | expand |
Thanks Biswarup! Just to add a bit of context: we were concerned with breaking the existing uinput usages where the caller might be setting an incorrect value (since previously, that had no effect). So the 10 second guard was added to fall back to the default behaviour for those devices. Reviewed-by: Siarhei Vishniakou <svv@google.com> On Wed, Apr 26, 2023 at 5:03 PM Biswarup Pal <biswarupp@google.com> wrote: > > Currently, uinput doesn't use the input_set_timestamp API, so any > event injected using uinput is not accurately timestamped in terms of > measuring when the actual event happened. Hence, call the > input_set_timestamp API from uinput in order to provide a more > accurate sense of time for the event. Propagate only the timestamps > which are a) positive, b) within a pre-defined offset (10 secs) from > the current time, and c) not in the future. > > Signed-off-by: Biswarup Pal <biswarupp@google.com> > --- > drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c > index f2593133e524..d98212d55108 100644 > --- a/drivers/input/misc/uinput.c > +++ b/drivers/input/misc/uinput.c > @@ -33,6 +33,7 @@ > #define UINPUT_NAME "uinput" > #define UINPUT_BUFFER_SIZE 16 > #define UINPUT_NUM_REQUESTS 16 > +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10 > > enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED }; > > @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev, > return retval; > } > > +/* > + * Returns true if the given timestamp is valid (i.e., if all the following > + * conditions are satisfied), false otherwise. > + * 1) given timestamp is positive > + * 2) it's within the allowed offset before the current time > + * 3) it's not in the future > + */ > +static bool is_valid_timestamp(const ktime_t timestamp) > +{ > + ktime_t zero_time; > + ktime_t current_time; > + ktime_t min_time; > + ktime_t offset; > + > + zero_time = ktime_set(0, 0); > + if (ktime_compare(zero_time, timestamp) >= 0) > + return false; > + > + current_time = ktime_get(); > + offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0); > + min_time = ktime_sub(current_time, offset); > + > + if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time)) > + return false; > + > + return true; > +} > + > static ssize_t uinput_inject_events(struct uinput_device *udev, > const char __user *buffer, size_t count) > { > struct input_event ev; > size_t bytes = 0; > + ktime_t timestamp; > > if (count != 0 && count < input_event_size()) > return -EINVAL; > @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev, > if (input_event_from_user(buffer + bytes, &ev)) > return -EFAULT; > > + timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC); > + if (is_valid_timestamp(timestamp)) > + input_set_timestamp(udev->dev, timestamp); > + > input_event(udev->dev, ev.type, ev.code, ev.value); > bytes += input_event_size(); > cond_resched(); > -- > 2.40.1.495.gc816e09b53d-goog >
Hi, On Fri, Apr 28, 2023 at 11:38:36AM -0700, Siarhei Vishniakou wrote: > Thanks Biswarup! Please avoid top posting. > > Just to add a bit of context: we were concerned with breaking the > existing uinput usages where the caller might be setting an incorrect > value (since previously, that had no effect). > So the 10 second guard was added to fall back to the default behaviour > for those devices. What is the benefit of sending this in uinput? It is not much close to the exact time the hardware generated the event than the timestamp generated in the input core, so I do not see why it is needed in uinput. Thanks.
On Fri, Apr 28, 2023 at 12:52:43PM -0700, Dmitry Torokhov wrote: > Hi, > > On Fri, Apr 28, 2023 at 11:38:36AM -0700, Siarhei Vishniakou wrote: > > Thanks Biswarup! > > Please avoid top posting. > > > > > Just to add a bit of context: we were concerned with breaking the > > existing uinput usages where the caller might be setting an incorrect > > value (since previously, that had no effect). > > So the 10 second guard was added to fall back to the default behaviour > > for those devices. > > What is the benefit of sending this in uinput? It is not much close > to the exact time the hardware generated the event than the timestamp > generated in the input core, so I do not see why it is needed in uinput. Ah, sorry, my apologies, I mis-parsed the code.
On Thu, Apr 27, 2023 at 12:01:51AM +0000, Biswarup Pal wrote: > Currently, uinput doesn't use the input_set_timestamp API, so any > event injected using uinput is not accurately timestamped in terms of > measuring when the actual event happened. Hence, call the > input_set_timestamp API from uinput in order to provide a more > accurate sense of time for the event. Propagate only the timestamps > which are a) positive, b) within a pre-defined offset (10 secs) from > the current time, and c) not in the future. This makes sense to me. Peter, do you see any issues? Thanks! > > Signed-off-by: Biswarup Pal <biswarupp@google.com> > --- > drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c > index f2593133e524..d98212d55108 100644 > --- a/drivers/input/misc/uinput.c > +++ b/drivers/input/misc/uinput.c > @@ -33,6 +33,7 @@ > #define UINPUT_NAME "uinput" > #define UINPUT_BUFFER_SIZE 16 > #define UINPUT_NUM_REQUESTS 16 > +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10 > > enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED }; > > @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev, > return retval; > } > > +/* > + * Returns true if the given timestamp is valid (i.e., if all the following > + * conditions are satisfied), false otherwise. > + * 1) given timestamp is positive > + * 2) it's within the allowed offset before the current time > + * 3) it's not in the future > + */ > +static bool is_valid_timestamp(const ktime_t timestamp) > +{ > + ktime_t zero_time; > + ktime_t current_time; > + ktime_t min_time; > + ktime_t offset; > + > + zero_time = ktime_set(0, 0); > + if (ktime_compare(zero_time, timestamp) >= 0) > + return false; > + > + current_time = ktime_get(); > + offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0); > + min_time = ktime_sub(current_time, offset); > + > + if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time)) > + return false; > + > + return true; > +} > + > static ssize_t uinput_inject_events(struct uinput_device *udev, > const char __user *buffer, size_t count) > { > struct input_event ev; > size_t bytes = 0; > + ktime_t timestamp; > > if (count != 0 && count < input_event_size()) > return -EINVAL; > @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev, > if (input_event_from_user(buffer + bytes, &ev)) > return -EFAULT; > > + timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC); > + if (is_valid_timestamp(timestamp)) > + input_set_timestamp(udev->dev, timestamp); > + > input_event(udev->dev, ev.type, ev.code, ev.value); > bytes += input_event_size(); > cond_resched(); > -- > 2.40.1.495.gc816e09b53d-goog >
On Tue, May 02, 2023 at 12:13:05PM +1000, Peter Hutterer wrote: > Thanks for the CC, I would've missed that. > > On Mon, May 01, 2023 at 06:05:59PM -0700, Dmitry Torokhov wrote: > > On Thu, Apr 27, 2023 at 12:01:51AM +0000, Biswarup Pal wrote: > > > Currently, uinput doesn't use the input_set_timestamp API, so any > > > event injected using uinput is not accurately timestamped in terms of > > > measuring when the actual event happened. Hence, call the > > > input_set_timestamp API from uinput in order to provide a more > > > accurate sense of time for the event. Propagate only the timestamps > > > which are a) positive, b) within a pre-defined offset (10 secs) from > > > the current time, and c) not in the future. > > > > This makes sense to me. Peter, do you see any issues? > > nope, this looks good and has my > Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net> Great, applied. > > afaict any code that has compiler warnings enabled should have this on > zero anyway. It'd be really nice to pass a timestamp down to uinput but > that's obviously a lot more involved. > > Cheers, > Peter > > > > > Thanks! > > > > > > > > Signed-off-by: Biswarup Pal <biswarupp@google.com> > > > --- > > > drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c > > > index f2593133e524..d98212d55108 100644 > > > --- a/drivers/input/misc/uinput.c > > > +++ b/drivers/input/misc/uinput.c > > > @@ -33,6 +33,7 @@ > > > #define UINPUT_NAME "uinput" > > > #define UINPUT_BUFFER_SIZE 16 > > > #define UINPUT_NUM_REQUESTS 16 > > > +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10 > > > > > > enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED }; > > > > > > @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev, > > > return retval; > > > } > > > > > > +/* > > > + * Returns true if the given timestamp is valid (i.e., if all the following > > > + * conditions are satisfied), false otherwise. > > > + * 1) given timestamp is positive > > > + * 2) it's within the allowed offset before the current time > > > + * 3) it's not in the future > > > + */ > > > +static bool is_valid_timestamp(const ktime_t timestamp) > > > +{ > > > + ktime_t zero_time; > > > + ktime_t current_time; > > > + ktime_t min_time; > > > + ktime_t offset; > > > + > > > + zero_time = ktime_set(0, 0); > > > + if (ktime_compare(zero_time, timestamp) >= 0) > > > + return false; > > > + > > > + current_time = ktime_get(); > > > + offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0); > > > + min_time = ktime_sub(current_time, offset); > > > + > > > + if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time)) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > static ssize_t uinput_inject_events(struct uinput_device *udev, > > > const char __user *buffer, size_t count) > > > { > > > struct input_event ev; > > > size_t bytes = 0; > > > + ktime_t timestamp; > > > > > > if (count != 0 && count < input_event_size()) > > > return -EINVAL; > > > @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev, > > > if (input_event_from_user(buffer + bytes, &ev)) > > > return -EFAULT; > > > > > > + timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC); > > > + if (is_valid_timestamp(timestamp)) > > > + input_set_timestamp(udev->dev, timestamp); > > > + > > > input_event(udev->dev, ev.type, ev.code, ev.value); > > > bytes += input_event_size(); > > > cond_resched(); > > > -- > > > 2.40.1.495.gc816e09b53d-goog > > > > > > > -- > > Dmitry
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index f2593133e524..d98212d55108 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -33,6 +33,7 @@ #define UINPUT_NAME "uinput" #define UINPUT_BUFFER_SIZE 16 #define UINPUT_NUM_REQUESTS 16 +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10 enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED }; @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev, return retval; } +/* + * Returns true if the given timestamp is valid (i.e., if all the following + * conditions are satisfied), false otherwise. + * 1) given timestamp is positive + * 2) it's within the allowed offset before the current time + * 3) it's not in the future + */ +static bool is_valid_timestamp(const ktime_t timestamp) +{ + ktime_t zero_time; + ktime_t current_time; + ktime_t min_time; + ktime_t offset; + + zero_time = ktime_set(0, 0); + if (ktime_compare(zero_time, timestamp) >= 0) + return false; + + current_time = ktime_get(); + offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0); + min_time = ktime_sub(current_time, offset); + + if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time)) + return false; + + return true; +} + static ssize_t uinput_inject_events(struct uinput_device *udev, const char __user *buffer, size_t count) { struct input_event ev; size_t bytes = 0; + ktime_t timestamp; if (count != 0 && count < input_event_size()) return -EINVAL; @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev, if (input_event_from_user(buffer + bytes, &ev)) return -EFAULT; + timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC); + if (is_valid_timestamp(timestamp)) + input_set_timestamp(udev->dev, timestamp); + input_event(udev->dev, ev.type, ev.code, ev.value); bytes += input_event_size(); cond_resched();
Currently, uinput doesn't use the input_set_timestamp API, so any event injected using uinput is not accurately timestamped in terms of measuring when the actual event happened. Hence, call the input_set_timestamp API from uinput in order to provide a more accurate sense of time for the event. Propagate only the timestamps which are a) positive, b) within a pre-defined offset (10 secs) from the current time, and c) not in the future. Signed-off-by: Biswarup Pal <biswarupp@google.com> --- drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)