Message-ID: <CABDcavYXBrZMMj1gn7CzNbA4f-L4c+q555goU+U0KUEs-6rW+w@mail.gmail.com>
Date: Mon, 3 Mar 2025 10:56:35 +0100
From: Guillermo Rodriguez Garcia <guille.rodriguez@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...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 Dmitry,

El mar, 25 feb 2025 a las 1:53, Dmitry Torokhov
(<dmitry.torokhov@...il.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@...il.com

Powered by blists - more mailing lists