Message-ID: <CABDcavb7AF+4hAeUD0ccW8ScHMFFPQ46XKat3xv7qP6y+4mVwA@mail.gmail.com>
Date: Sat, 12 Apr 2025 12:01:25 +0200
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 all,

Any comments on the patch I proposed?

Thank you,

Guillermo Rodriguez Garcia
guille.rodriguez@...il.com

El lun, 3 mar 2025 a las 10:56, Guillermo Rodriguez Garcia
(<guille.rodriguez@...il.com>) escribió:
>
> 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



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

Powered by blists - more mailing lists