[PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

[PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

[Guillermo Rodríguez]

Hi all,

We are seeing an issue with gpio_keys where the first event after
a SYN_DROPPED has the same timestamp as the SYN_DROPPED event itself.
After some investigation it looks like this is an issue with evdev
and not specific to gpio_keys.

The issue was originally introduced in commit 3b51c44 ("Input: allow
drivers specify timestamp for input events").

This commit introduced input_set_timestamp and input_get_timestamp.
The latter (despite the name) actually generates and stores a timestamp
in dev->timestamp if the driver did not set one itself. This timestamp
needs to be reset when events are flushed; otherwise the next event
will use a stale timestamp. A partial fix was implemented in 4370b23
("Input: reset device timestamp on sync"), but this does not cover the
case of SYN_DROPPED.

If a SYN_DROPPED is generated (currently only done by evdev), the
following happens:

- evdev calls input_get_timestamp to generate a timestamp for the
  SYN_DROPPED event.
- input_get_timestamp generates a timestamp and stores it in
  dev->timestamp
- When the next real event is reported (in evdev_events), evdev
  calls input_get_timestamp, which uses the timestamp already
  stored in dev->timestamp, which corresponds to the SYN_DROPPED event
  
How to fix:

- When a SYN_DROPPED is generated, after calling input_get_timestamp,
  the timestamp stored in dev->timestamp should be reset (same as is
  currently done in input_handle_event). The attached patch does that.
  
(Perhaps the underlying problem is that it is not expected that a
function called input_get_timestamp will have side effects. The
commit history of input.c shows that this has actually caused a
few issues since 3b51c44.)


Guillermo Rodríguez (1):
  Input: evdev - fix wrong timestamp after SYN_DROPPED event

 drivers/input/evdev.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)


base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95
2.25.1

[PATCH 1/1] Input: evdev - fix wrong timestamp after SYN_DROPPED event

[Guillermo Rodríguez]

Reset the input device's timestamp after generating a SYN_DROPPED
event, otherwise the next real event will use the same timestamp as
the SYN_DROPPED event itself.

Fixes: 3b51c44bd693 ("Input: allow drivers specify timestamp for input events")

Signed-off-by: Guillermo Rodríguez <guille.rodriguez@gmail.com>
---
 drivers/input/evdev.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index b5cbb57ee5f6..6ab38a611321 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -142,7 +142,8 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 
 static void __evdev_queue_syn_dropped(struct evdev_client *client)
 {
-	ktime_t *ev_time = input_get_timestamp(client->evdev->handle.dev);
+	struct input_dev *input_dev = client->evdev->handle.dev;
+	ktime_t *ev_time = input_get_timestamp(input_dev);
 	struct timespec64 ts = ktime_to_timespec64(ev_time[client->clk_type]);
 	struct input_event ev;
 
@@ -160,6 +161,13 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
 		client->tail = (client->head - 1) & (client->bufsize - 1);
 		client->packet_head = client->tail;
 	}
+
+	/*
+	 * Reset the timestamp; otherwise the next event will
+	 * have the same timestamp as this SYN_DROPPED event.
+	 * See input_handle_event.
+	 */
+	input_dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
 }
 
 static void evdev_queue_syn_dropped(struct evdev_client *client)
-- 
2.25.1

Re: [PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

[Guillermo Rodriguez Garcia]

Hi all,

Any comments on the patch below?

Thank you,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

El lun, 2 dic 2024 a las 13:34, Guillermo Rodríguez
(<guille.rodriguez@gmail.com>) escribió:
>
> Hi all,
>
> We are seeing an issue with gpio_keys where the first event after
> a SYN_DROPPED has the same timestamp as the SYN_DROPPED event itself.
> After some investigation it looks like this is an issue with evdev
> and not specific to gpio_keys.
>
> The issue was originally introduced in commit 3b51c44 ("Input: allow
> drivers specify timestamp for input events").
>
> This commit introduced input_set_timestamp and input_get_timestamp.
> The latter (despite the name) actually generates and stores a timestamp
> in dev->timestamp if the driver did not set one itself. This timestamp
> needs to be reset when events are flushed; otherwise the next event
> will use a stale timestamp. A partial fix was implemented in 4370b23
> ("Input: reset device timestamp on sync"), but this does not cover the
> case of SYN_DROPPED.
>
> If a SYN_DROPPED is generated (currently only done by evdev), the
> following happens:
>
> - evdev calls input_get_timestamp to generate a timestamp for the
>   SYN_DROPPED event.
> - input_get_timestamp generates a timestamp and stores it in
>   dev->timestamp
> - When the next real event is reported (in evdev_events), evdev
>   calls input_get_timestamp, which uses the timestamp already
>   stored in dev->timestamp, which corresponds to the SYN_DROPPED event
>
> How to fix:
>
> - When a SYN_DROPPED is generated, after calling input_get_timestamp,
>   the timestamp stored in dev->timestamp should be reset (same as is
>   currently done in input_handle_event). The attached patch does that.
>
> (Perhaps the underlying problem is that it is not expected that a
> function called input_get_timestamp will have side effects. The
> commit history of input.c shows that this has actually caused a
> few issues since 3b51c44.)
>
>
> Guillermo Rodríguez (1):
>   Input: evdev - fix wrong timestamp after SYN_DROPPED event
>
>  drivers/input/evdev.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
>
> base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95
> 2.25.1
>

Re: [PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

[Dmitry Torokhov]

Hi Guillermo,

On Mon, Dec 02, 2024 at 01:33:50PM +0100, Guillermo Rodríguez wrote:
> Hi all,
> 
> We are seeing an issue with gpio_keys where the first event after
> a SYN_DROPPED has the same timestamp as the SYN_DROPPED event itself.
> After some investigation it looks like this is an issue with evdev
> and not specific to gpio_keys.
> 
> The issue was originally introduced in commit 3b51c44 ("Input: allow
> drivers specify timestamp for input events").
> 
> This commit introduced input_set_timestamp and input_get_timestamp.
> The latter (despite the name) actually generates and stores a timestamp
> in dev->timestamp if the driver did not set one itself. This timestamp
> needs to be reset when events are flushed; otherwise the next event
> will use a stale timestamp. A partial fix was implemented in 4370b23
> ("Input: reset device timestamp on sync"), but this does not cover the
> case of SYN_DROPPED.
> 
> If a SYN_DROPPED is generated (currently only done by evdev), the
> following happens:
> 
> - evdev calls input_get_timestamp to generate a timestamp for the
>   SYN_DROPPED event.
> - input_get_timestamp generates a timestamp and stores it in
>   dev->timestamp
> - When the next real event is reported (in evdev_events), evdev
>   calls input_get_timestamp, which uses the timestamp already
>   stored in dev->timestamp, which corresponds to the SYN_DROPPED event
>
> How to fix:
>
> - When a SYN_DROPPED is generated, after calling input_get_timestamp,
>   the timestamp stored in dev->timestamp should be reset (same as is
>   currently done in input_handle_event). The attached patch does that.

So this happens after you change clock type of a client, right?

I do dot think having one client affecting timestamp for everyone is a
good idea. Instead I think __evdev_queue_syn_dropped() should either:

- use "now" time for SYN_DROPPED generated when user requests clock
  change or reads device's current state, or

- check if input device has timestamp set and use it or use "now". This
  option is needed if we are concerned about timestamps potentially
  going backwards if clock change happens in a middle of delivering
  input packet.

>
> (Perhaps the underlying problem is that it is not expected that a
> function called input_get_timestamp will have side effects. The
> commit history of input.c shows that this has actually caused a
> few issues since 3b51c44.)

Yes, maybe something like below will work better. It does not address
the keeping timestamp for SYN_DROPPED though.

Thanks.

-- 
Dmitry


diff --git a/drivers/input/input.c b/drivers/input/input.c
index 54d35c1a2a24..954c5104a1c1 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -61,6 +61,66 @@ static const unsigned int input_max_code[EV_CNT] = {
 	[EV_FF] = FF_MAX,
 };
 
+/**
+ * input_set_timestamp - set timestamp for input events
+ * @dev: input device to set timestamp for
+ * @timestamp: the time at which the event has occurred
+ *   in CLOCK_MONOTONIC
+ *
+ * This function is intended to provide to the input system a more
+ * accurate time of when an event actually occurred. The driver should
+ * call this function as soon as a timestamp is acquired ensuring
+ * clock conversions in input_set_timestamp are done correctly.
+ *
+ * The system entering suspend state between timestamp acquisition and
+ * calling input_set_timestamp can result in inaccurate conversions.
+ */
+void input_set_timestamp(struct input_dev *dev, ktime_t timestamp)
+{
+	dev->timestamp[INPUT_CLK_MONO] = timestamp;
+	dev->timestamp[INPUT_CLK_REAL] = ktime_mono_to_real(timestamp);
+	dev->timestamp[INPUT_CLK_BOOT] = ktime_mono_to_any(timestamp,
+							   TK_OFFS_BOOT);
+}
+EXPORT_SYMBOL(input_set_timestamp);
+
+/**
+ * input_get_timestamp - get timestamp for input events
+ * @dev: input device to get timestamp from
+ *
+ * A valid timestamp is a timestamp of non-zero value.
+ */
+ktime_t *input_get_timestamp(struct input_dev *dev)
+{
+	bool have_timestamp;
+
+	/* TODO: remove setting of the timestamp in a few releases. */
+	have_timestamp = ktime_compare(dev->timestamp[INPUT_CLK_MONO],
+				       ktime_set(0, 0));
+	if (WARN_ON_ONCE(!have_timestamp))
+		input_set_timestamp(dev, ktime_get());
+
+	return dev->timestamp;
+}
+EXPORT_SYMBOL(input_get_timestamp);
+
+static bool input_is_timestamp_set(struct input_dev *dev)
+{
+	return ktime_compare(dev->timestamp[INPUT_CLK_MONO], ktime_set(0, 0));
+}
+
+/*
+ * Reset timestamp for an input device so that next input event will get
+ * a new one.
+ *
+ * Note we only need to reset the monolithic one as we use its presence when
+ * deciding whether to generate a synthetic timestamp.
+ */
+static void input_reset_timestamp(struct input_dev *dev)
+{
+	dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
+}
+
 static inline int is_event_supported(unsigned int code,
 				     unsigned long *bm, unsigned int max)
 {
@@ -342,11 +402,9 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
 		dev->num_vals = 0;
 		/*
 		 * Reset the timestamp on flush so we won't end up
-		 * with a stale one. Note we only need to reset the
-		 * monolithic one as we use its presence when deciding
-		 * whether to generate a synthetic timestamp.
+		 * with a stale one in the next event packet.
 		 */
-		dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
+		input_reset_timestamp(dev);
 	} else if (dev->num_vals >= dev->max_vals - 2) {
 		dev->vals[dev->num_vals++] = input_value_sync;
 		input_pass_values(dev, dev->vals, dev->num_vals);
@@ -366,6 +424,9 @@ void input_handle_event(struct input_dev *dev,
 		if (type != EV_SYN)
 			add_input_randomness(type, code, value);
 
+		if (!input_is_timestamp_set(dev))
+			input_set_timestamp(dev, ktime_get());
+
 		input_event_dispose(dev, disposition, type, code, value);
 	}
 }
@@ -2053,46 +2114,6 @@ void input_free_device(struct input_dev *dev)
 }
 EXPORT_SYMBOL(input_free_device);
 
-/**
- * input_set_timestamp - set timestamp for input events
- * @dev: input device to set timestamp for
- * @timestamp: the time at which the event has occurred
- *   in CLOCK_MONOTONIC
- *
- * This function is intended to provide to the input system a more
- * accurate time of when an event actually occurred. The driver should
- * call this function as soon as a timestamp is acquired ensuring
- * clock conversions in input_set_timestamp are done correctly.
- *
- * The system entering suspend state between timestamp acquisition and
- * calling input_set_timestamp can result in inaccurate conversions.
- */
-void input_set_timestamp(struct input_dev *dev, ktime_t timestamp)
-{
-	dev->timestamp[INPUT_CLK_MONO] = timestamp;
-	dev->timestamp[INPUT_CLK_REAL] = ktime_mono_to_real(timestamp);
-	dev->timestamp[INPUT_CLK_BOOT] = ktime_mono_to_any(timestamp,
-							   TK_OFFS_BOOT);
-}
-EXPORT_SYMBOL(input_set_timestamp);
-
-/**
- * input_get_timestamp - get timestamp for input events
- * @dev: input device to get timestamp from
- *
- * A valid timestamp is a timestamp of non-zero value.
- */
-ktime_t *input_get_timestamp(struct input_dev *dev)
-{
-	const ktime_t invalid_timestamp = ktime_set(0, 0);
-
-	if (!ktime_compare(dev->timestamp[INPUT_CLK_MONO], invalid_timestamp))
-		input_set_timestamp(dev, ktime_get());
-
-	return dev->timestamp;
-}
-EXPORT_SYMBOL(input_get_timestamp);
-
 /**
  * input_set_capability - mark device as capable of a certain event
  * @dev: device that is capable of emitting or accepting event

Re: [PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

[Guillermo Rodriguez Garcia]

Hi Dmitry,

El mar, 25 feb 2025 a las 1:53, Dmitry Torokhov
(<dmitry.torokhov@gmail.com>) escribió:
>
> Hi Guillermo,
>
> On Mon, Dec 02, 2024 at 01:33:50PM +0100, Guillermo Rodríguez wrote:
> > Hi all,
> >
> > We are seeing an issue with gpio_keys where the first event after
> > a SYN_DROPPED has the same timestamp as the SYN_DROPPED event itself.
> > After some investigation it looks like this is an issue with evdev
> > and not specific to gpio_keys.
> >
> > The issue was originally introduced in commit 3b51c44 ("Input: allow
> > drivers specify timestamp for input events").
> >
> > This commit introduced input_set_timestamp and input_get_timestamp.
> > The latter (despite the name) actually generates and stores a timestamp
> > in dev->timestamp if the driver did not set one itself. This timestamp
> > needs to be reset when events are flushed; otherwise the next event
> > will use a stale timestamp. A partial fix was implemented in 4370b23
> > ("Input: reset device timestamp on sync"), but this does not cover the
> > case of SYN_DROPPED.
> >
> > If a SYN_DROPPED is generated (currently only done by evdev), the
> > following happens:
> >
> > - evdev calls input_get_timestamp to generate a timestamp for the
> >   SYN_DROPPED event.
> > - input_get_timestamp generates a timestamp and stores it in
> >   dev->timestamp
> > - When the next real event is reported (in evdev_events), evdev
> >   calls input_get_timestamp, which uses the timestamp already
> >   stored in dev->timestamp, which corresponds to the SYN_DROPPED event
> >
> > How to fix:
> >
> > - When a SYN_DROPPED is generated, after calling input_get_timestamp,
> >   the timestamp stored in dev->timestamp should be reset (same as is
> >   currently done in input_handle_event). The attached patch does that.
>
> So this happens after you change clock type of a client, right?

Yes.

>
> I do dot think having one client affecting timestamp for everyone is a
> good idea.

Do you mean the timestamp of the SYN_DROPPED event itself?
I am not sure if this is an issue. The timestamp of the SYN_DROPPED
event is not particularly meaningful.
A client that sees a SYN_DROPPED only knows that some events were
dropped, but the timestamp of the SYN_DROPPED event itself does not
carry any useful information -- it is not, for example, the timestamp
of the event that was dropped (in fact you cannot even know how many
events were dropped).

> Instead I think __evdev_queue_syn_dropped() should either:
>
> - use "now" time for SYN_DROPPED generated when user requests clock
>   change or reads device's current state, or
>
> - check if input device has timestamp set and use it or use "now". This
>   option is needed if we are concerned about timestamps potentially
>   going backwards if clock change happens in a middle of delivering
>   input packet.

If you want to do it like this I would advise to just use "now".

CLOCK_MONOTONIC (and CLOCK_BOOTTIME) cannot go backwards by definition.

The wall clock (CLOCK_REALTIME) can go backwards but this is a
"feature" and should not be "fixed". if client code wants to see wall
clock timestamps then it should be prepared to see time going
backwards, for example when the time is updated, or in the middle of
DST changes.

>
> >
> > (Perhaps the underlying problem is that it is not expected that a
> > function called input_get_timestamp will have side effects. The
> > commit history of input.c shows that this has actually caused a
> > few issues since 3b51c44.)
>
> Yes, maybe something like below will work better. It does not address
> the keeping timestamp for SYN_DROPPED though.

Could be.
But I can't help wondering whether 3b51c44 was a good idea.
input_set_timestamp was supposed to "allow drivers to provide a more
accurate timestamp" but I wonder if there was a real need for that --
it did not seem to have in-tree users except for uinput and more
recently wacom_wac.

Anyway the original problem I reported is not related to the timestamp
of the SYN_DROPPED event itself, but to the fact that this timestamp
is then reused for the next "real" event after SYN_DROPPED. My patch
clears the timestamp after the SYN_DROPPED is handled, in the same way
it was already being done on flush, in input_handle_event (now moved
to input_dispose_event).

Thanks,

Guillermo

>
> Thanks.
>
> --
> Dmitry
>
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 54d35c1a2a24..954c5104a1c1 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -61,6 +61,66 @@ static const unsigned int input_max_code[EV_CNT] = {
>         [EV_FF] = FF_MAX,
>  };
>
> +/**
> + * input_set_timestamp - set timestamp for input events
> + * @dev: input device to set timestamp for
> + * @timestamp: the time at which the event has occurred
> + *   in CLOCK_MONOTONIC
> + *
> + * This function is intended to provide to the input system a more
> + * accurate time of when an event actually occurred. The driver should
> + * call this function as soon as a timestamp is acquired ensuring
> + * clock conversions in input_set_timestamp are done correctly.
> + *
> + * The system entering suspend state between timestamp acquisition and
> + * calling input_set_timestamp can result in inaccurate conversions.
> + */
> +void input_set_timestamp(struct input_dev *dev, ktime_t timestamp)
> +{
> +       dev->timestamp[INPUT_CLK_MONO] = timestamp;
> +       dev->timestamp[INPUT_CLK_REAL] = ktime_mono_to_real(timestamp);
> +       dev->timestamp[INPUT_CLK_BOOT] = ktime_mono_to_any(timestamp,
> +                                                          TK_OFFS_BOOT);
> +}
> +EXPORT_SYMBOL(input_set_timestamp);
> +
> +/**
> + * input_get_timestamp - get timestamp for input events
> + * @dev: input device to get timestamp from
> + *
> + * A valid timestamp is a timestamp of non-zero value.
> + */
> +ktime_t *input_get_timestamp(struct input_dev *dev)
> +{
> +       bool have_timestamp;
> +
> +       /* TODO: remove setting of the timestamp in a few releases. */
> +       have_timestamp = ktime_compare(dev->timestamp[INPUT_CLK_MONO],
> +                                      ktime_set(0, 0));
> +       if (WARN_ON_ONCE(!have_timestamp))
> +               input_set_timestamp(dev, ktime_get());
> +
> +       return dev->timestamp;
> +}
> +EXPORT_SYMBOL(input_get_timestamp);
> +
> +static bool input_is_timestamp_set(struct input_dev *dev)
> +{
> +       return ktime_compare(dev->timestamp[INPUT_CLK_MONO], ktime_set(0, 0));
> +}
> +
> +/*
> + * Reset timestamp for an input device so that next input event will get
> + * a new one.
> + *
> + * Note we only need to reset the monolithic one as we use its presence when
> + * deciding whether to generate a synthetic timestamp.
> + */
> +static void input_reset_timestamp(struct input_dev *dev)
> +{
> +       dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
> +}
> +
>  static inline int is_event_supported(unsigned int code,
>                                      unsigned long *bm, unsigned int max)
>  {
> @@ -342,11 +402,9 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
>                 dev->num_vals = 0;
>                 /*
>                  * Reset the timestamp on flush so we won't end up
> -                * with a stale one. Note we only need to reset the
> -                * monolithic one as we use its presence when deciding
> -                * whether to generate a synthetic timestamp.
> +                * with a stale one in the next event packet.
>                  */
> -               dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
> +               input_reset_timestamp(dev);
>         } else if (dev->num_vals >= dev->max_vals - 2) {
>                 dev->vals[dev->num_vals++] = input_value_sync;
>                 input_pass_values(dev, dev->vals, dev->num_vals);
> @@ -366,6 +424,9 @@ void input_handle_event(struct input_dev *dev,
>                 if (type != EV_SYN)
>                         add_input_randomness(type, code, value);
>
> +               if (!input_is_timestamp_set(dev))
> +                       input_set_timestamp(dev, ktime_get());
> +
>                 input_event_dispose(dev, disposition, type, code, value);
>         }
>  }
> @@ -2053,46 +2114,6 @@ void input_free_device(struct input_dev *dev)
>  }
>  EXPORT_SYMBOL(input_free_device);
>
> -/**
> - * input_set_timestamp - set timestamp for input events
> - * @dev: input device to set timestamp for
> - * @timestamp: the time at which the event has occurred
> - *   in CLOCK_MONOTONIC
> - *
> - * This function is intended to provide to the input system a more
> - * accurate time of when an event actually occurred. The driver should
> - * call this function as soon as a timestamp is acquired ensuring
> - * clock conversions in input_set_timestamp are done correctly.
> - *
> - * The system entering suspend state between timestamp acquisition and
> - * calling input_set_timestamp can result in inaccurate conversions.
> - */
> -void input_set_timestamp(struct input_dev *dev, ktime_t timestamp)
> -{
> -       dev->timestamp[INPUT_CLK_MONO] = timestamp;
> -       dev->timestamp[INPUT_CLK_REAL] = ktime_mono_to_real(timestamp);
> -       dev->timestamp[INPUT_CLK_BOOT] = ktime_mono_to_any(timestamp,
> -                                                          TK_OFFS_BOOT);
> -}
> -EXPORT_SYMBOL(input_set_timestamp);
> -
> -/**
> - * input_get_timestamp - get timestamp for input events
> - * @dev: input device to get timestamp from
> - *
> - * A valid timestamp is a timestamp of non-zero value.
> - */
> -ktime_t *input_get_timestamp(struct input_dev *dev)
> -{
> -       const ktime_t invalid_timestamp = ktime_set(0, 0);
> -
> -       if (!ktime_compare(dev->timestamp[INPUT_CLK_MONO], invalid_timestamp))
> -               input_set_timestamp(dev, ktime_get());
> -
> -       return dev->timestamp;
> -}
> -EXPORT_SYMBOL(input_get_timestamp);
> -
>  /**
>   * input_set_capability - mark device as capable of a certain event
>   * @dev: device that is capable of emitting or accepting event



-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: [PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

[Guillermo Rodriguez Garcia]

Hi all,

Any comments on the patch I proposed?

Thank you,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

El lun, 3 mar 2025 a las 10:56, Guillermo Rodriguez Garcia
(<guille.rodriguez@gmail.com>) escribió:
>
> Hi Dmitry,
>
> El mar, 25 feb 2025 a las 1:53, Dmitry Torokhov
> (<dmitry.torokhov@gmail.com>) escribió:
> >
> > Hi Guillermo,
> >
> > On Mon, Dec 02, 2024 at 01:33:50PM +0100, Guillermo Rodríguez wrote:
> > > Hi all,
> > >
> > > We are seeing an issue with gpio_keys where the first event after
> > > a SYN_DROPPED has the same timestamp as the SYN_DROPPED event itself.
> > > After some investigation it looks like this is an issue with evdev
> > > and not specific to gpio_keys.
> > >
> > > The issue was originally introduced in commit 3b51c44 ("Input: allow
> > > drivers specify timestamp for input events").
> > >
> > > This commit introduced input_set_timestamp and input_get_timestamp.
> > > The latter (despite the name) actually generates and stores a timestamp
> > > in dev->timestamp if the driver did not set one itself. This timestamp
> > > needs to be reset when events are flushed; otherwise the next event
> > > will use a stale timestamp. A partial fix was implemented in 4370b23
> > > ("Input: reset device timestamp on sync"), but this does not cover the
> > > case of SYN_DROPPED.
> > >
> > > If a SYN_DROPPED is generated (currently only done by evdev), the
> > > following happens:
> > >
> > > - evdev calls input_get_timestamp to generate a timestamp for the
> > >   SYN_DROPPED event.
> > > - input_get_timestamp generates a timestamp and stores it in
> > >   dev->timestamp
> > > - When the next real event is reported (in evdev_events), evdev
> > >   calls input_get_timestamp, which uses the timestamp already
> > >   stored in dev->timestamp, which corresponds to the SYN_DROPPED event
> > >
> > > How to fix:
> > >
> > > - When a SYN_DROPPED is generated, after calling input_get_timestamp,
> > >   the timestamp stored in dev->timestamp should be reset (same as is
> > >   currently done in input_handle_event). The attached patch does that.
> >
> > So this happens after you change clock type of a client, right?
>
> Yes.
>
> >
> > I do dot think having one client affecting timestamp for everyone is a
> > good idea.
>
> Do you mean the timestamp of the SYN_DROPPED event itself?
> I am not sure if this is an issue. The timestamp of the SYN_DROPPED
> event is not particularly meaningful.
> A client that sees a SYN_DROPPED only knows that some events were
> dropped, but the timestamp of the SYN_DROPPED event itself does not
> carry any useful information -- it is not, for example, the timestamp
> of the event that was dropped (in fact you cannot even know how many
> events were dropped).
>
> > Instead I think __evdev_queue_syn_dropped() should either:
> >
> > - use "now" time for SYN_DROPPED generated when user requests clock
> >   change or reads device's current state, or
> >
> > - check if input device has timestamp set and use it or use "now". This
> >   option is needed if we are concerned about timestamps potentially
> >   going backwards if clock change happens in a middle of delivering
> >   input packet.
>
> If you want to do it like this I would advise to just use "now".
>
> CLOCK_MONOTONIC (and CLOCK_BOOTTIME) cannot go backwards by definition.
>
> The wall clock (CLOCK_REALTIME) can go backwards but this is a
> "feature" and should not be "fixed". if client code wants to see wall
> clock timestamps then it should be prepared to see time going
> backwards, for example when the time is updated, or in the middle of
> DST changes.
>
> >
> > >
> > > (Perhaps the underlying problem is that it is not expected that a
> > > function called input_get_timestamp will have side effects. The
> > > commit history of input.c shows that this has actually caused a
> > > few issues since 3b51c44.)
> >
> > Yes, maybe something like below will work better. It does not address
> > the keeping timestamp for SYN_DROPPED though.
>
> Could be.
> But I can't help wondering whether 3b51c44 was a good idea.
> input_set_timestamp was supposed to "allow drivers to provide a more
> accurate timestamp" but I wonder if there was a real need for that --
> it did not seem to have in-tree users except for uinput and more
> recently wacom_wac.
>
> Anyway the original problem I reported is not related to the timestamp
> of the SYN_DROPPED event itself, but to the fact that this timestamp
> is then reused for the next "real" event after SYN_DROPPED. My patch
> clears the timestamp after the SYN_DROPPED is handled, in the same way
> it was already being done on flush, in input_handle_event (now moved
> to input_dispose_event).
>
> Thanks,
>
> Guillermo
>
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> >
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index 54d35c1a2a24..954c5104a1c1 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -61,6 +61,66 @@ static const unsigned int input_max_code[EV_CNT] = {
> >         [EV_FF] = FF_MAX,
> >  };
> >
> > +/**
> > + * input_set_timestamp - set timestamp for input events
> > + * @dev: input device to set timestamp for
> > + * @timestamp: the time at which the event has occurred
> > + *   in CLOCK_MONOTONIC
> > + *
> > + * This function is intended to provide to the input system a more
> > + * accurate time of when an event actually occurred. The driver should
> > + * call this function as soon as a timestamp is acquired ensuring
> > + * clock conversions in input_set_timestamp are done correctly.
> > + *
> > + * The system entering suspend state between timestamp acquisition and
> > + * calling input_set_timestamp can result in inaccurate conversions.
> > + */
> > +void input_set_timestamp(struct input_dev *dev, ktime_t timestamp)
> > +{
> > +       dev->timestamp[INPUT_CLK_MONO] = timestamp;
> > +       dev->timestamp[INPUT_CLK_REAL] = ktime_mono_to_real(timestamp);
> > +       dev->timestamp[INPUT_CLK_BOOT] = ktime_mono_to_any(timestamp,
> > +                                                          TK_OFFS_BOOT);
> > +}
> > +EXPORT_SYMBOL(input_set_timestamp);
> > +
> > +/**
> > + * input_get_timestamp - get timestamp for input events
> > + * @dev: input device to get timestamp from
> > + *
> > + * A valid timestamp is a timestamp of non-zero value.
> > + */
> > +ktime_t *input_get_timestamp(struct input_dev *dev)
> > +{
> > +       bool have_timestamp;
> > +
> > +       /* TODO: remove setting of the timestamp in a few releases. */
> > +       have_timestamp = ktime_compare(dev->timestamp[INPUT_CLK_MONO],
> > +                                      ktime_set(0, 0));
> > +       if (WARN_ON_ONCE(!have_timestamp))
> > +               input_set_timestamp(dev, ktime_get());
> > +
> > +       return dev->timestamp;
> > +}
> > +EXPORT_SYMBOL(input_get_timestamp);
> > +
> > +static bool input_is_timestamp_set(struct input_dev *dev)
> > +{
> > +       return ktime_compare(dev->timestamp[INPUT_CLK_MONO], ktime_set(0, 0));
> > +}
> > +
> > +/*
> > + * Reset timestamp for an input device so that next input event will get
> > + * a new one.
> > + *
> > + * Note we only need to reset the monolithic one as we use its presence when
> > + * deciding whether to generate a synthetic timestamp.
> > + */
> > +static void input_reset_timestamp(struct input_dev *dev)
> > +{
> > +       dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
> > +}
> > +
> >  static inline int is_event_supported(unsigned int code,
> >                                      unsigned long *bm, unsigned int max)
> >  {
> > @@ -342,11 +402,9 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
> >                 dev->num_vals = 0;
> >                 /*
> >                  * Reset the timestamp on flush so we won't end up
> > -                * with a stale one. Note we only need to reset the
> > -                * monolithic one as we use its presence when deciding
> > -                * whether to generate a synthetic timestamp.
> > +                * with a stale one in the next event packet.
> >                  */
> > -               dev->timestamp[INPUT_CLK_MONO] = ktime_set(0, 0);
> > +               input_reset_timestamp(dev);
> >         } else if (dev->num_vals >= dev->max_vals - 2) {
> >                 dev->vals[dev->num_vals++] = input_value_sync;
> >                 input_pass_values(dev, dev->vals, dev->num_vals);
> > @@ -366,6 +424,9 @@ void input_handle_event(struct input_dev *dev,
> >                 if (type != EV_SYN)
> >                         add_input_randomness(type, code, value);
> >
> > +               if (!input_is_timestamp_set(dev))
> > +                       input_set_timestamp(dev, ktime_get());
> > +
> >                 input_event_dispose(dev, disposition, type, code, value);
> >         }
> >  }
> > @@ -2053,46 +2114,6 @@ void input_free_device(struct input_dev *dev)
> >  }
> >  EXPORT_SYMBOL(input_free_device);
> >
> > -/**
> > - * input_set_timestamp - set timestamp for input events
> > - * @dev: input device to set timestamp for
> > - * @timestamp: the time at which the event has occurred
> > - *   in CLOCK_MONOTONIC
> > - *
> > - * This function is intended to provide to the input system a more
> > - * accurate time of when an event actually occurred. The driver should
> > - * call this function as soon as a timestamp is acquired ensuring
> > - * clock conversions in input_set_timestamp are done correctly.
> > - *
> > - * The system entering suspend state between timestamp acquisition and
> > - * calling input_set_timestamp can result in inaccurate conversions.
> > - */
> > -void input_set_timestamp(struct input_dev *dev, ktime_t timestamp)
> > -{
> > -       dev->timestamp[INPUT_CLK_MONO] = timestamp;
> > -       dev->timestamp[INPUT_CLK_REAL] = ktime_mono_to_real(timestamp);
> > -       dev->timestamp[INPUT_CLK_BOOT] = ktime_mono_to_any(timestamp,
> > -                                                          TK_OFFS_BOOT);
> > -}
> > -EXPORT_SYMBOL(input_set_timestamp);
> > -
> > -/**
> > - * input_get_timestamp - get timestamp for input events
> > - * @dev: input device to get timestamp from
> > - *
> > - * A valid timestamp is a timestamp of non-zero value.
> > - */
> > -ktime_t *input_get_timestamp(struct input_dev *dev)
> > -{
> > -       const ktime_t invalid_timestamp = ktime_set(0, 0);
> > -
> > -       if (!ktime_compare(dev->timestamp[INPUT_CLK_MONO], invalid_timestamp))
> > -               input_set_timestamp(dev, ktime_get());
> > -
> > -       return dev->timestamp;
> > -}
> > -EXPORT_SYMBOL(input_get_timestamp);
> > -
> >  /**
> >   * input_set_capability - mark device as capable of a certain event
> >   * @dev: device that is capable of emitting or accepting event
>
>
>
> --
> Guillermo Rodriguez Garcia
> guille.rodriguez@gmail.com



-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com