Message-ID: <Z70UagY4hxDwUDHv@google.com>
Date: Mon, 24 Feb 2025 16:52:58 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Guillermo Rodríguez <guille.rodriguez@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH 0/1] Input: evdev - fix wrong timestamp after SYN_DROPPED

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

Powered by blists - more mailing lists