Re: [Y2038] [PATCH v2 3/4] input: Deprecate real timestamps beyond year 2106

[Peter Hutterer]

On Thu, Oct 27, 2016 at 03:24:55PM -0700, Deepa Dinamani wrote:
> On Wed, Oct 26, 2016 at 7:56 PM, Peter Hutterer
>  wrote:
> > On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote:
> >> struct timeval is not y2038 safe.
> >> All usage of timeval in the kernel will be replaced by
> >> y2038 safe structures.
> >>
> >> struct input_event maintains time for each input event.
> >> Real time timestamps are not ideal for input as this
> >> time can go backwards as noted in the patch a80b83b7b8
> >> by John Stultz. Hence, having the input_event.time fields
> >> only big enough for monotonic and boot times are
> >> sufficient.
> >>
> >> Leave the original input_event as is. This is to maintain
> >> backward compatibility with existing userspace interfaces
> >> that use input_event.
> >> Introduce a new replacement struct raw_input_event.
> >
> > general comment here - please don't name it "raw_input_event".
> > First, when you grep for input_event you want the new ones to show up too,
> > so a struct input_event_raw would be better here. That also has better
> > namespacing in general. Second though: the event isn't any more "raw" than
> > the previous we had.
> >
> > I can't think of anything better than struct input_event_v2 though.
> 
> The general idea was to leave the original struct input_event as a
> common interface for userspace (as it cannot be deleted).
> So reading raw data unformatted by the userspace will have the new
> struct raw_input_event format.
> This was the reason for the "raw" in the name.
> 
> struct input_event_v2 is fine too, if this is more preferred.
> 
> >> This replaces timeval with struct input_timeval. This structure
> >> maintains time in __kernel_ulong_t or compat_ulong_t to allow
> >> for architectures to override types as in the case of x32.
> >>
> >> The change requires any userspace utilities reading or writing
> >> from event nodes to update their reading format to match
> >> raw_input_event. The changes to the popular libraries will be
> >> posted along with the kernel changes.
> >> The driver version is also updated to reflect the change in
> >> event format.
> >
> > Doesn't this break *all* of userspace then? I don't see anything to
> > negotiate the type of input event the kernel gives me. And nothing right now
> > checks for EVDEV_VERSION, so they all just assume it's a struct
> > input_event. Best case, if the available events aren't a multiple of
> > sizeof(struct input_event) userspace will bomb out, but unless that happens,
> > everyone will just happily read old-style events.
> >
> > So we need some negotiation what is acceptable. Which also needs to address
> > the race conditions we're going to get when events start coming in before
> > the client has announced that it supports the new-style events.
> 
> No, this does not break any userspace right now.
> Both struct input_event and struct raw_input_event are exactly the same today.

oh, right, the ABI is the same. I see that now, thanks.

> This will be the case until a 2038-safe glibc is used with a 64 bit time_t 
> flag.
> 
> So these are the scenarios:
> 1. old kernel driver + new userspace
>   -- should still be ok until 2038. Version checks could help discover these
> 2. new kernel driver + old userspace (without recompiled with new 2038 gblic)
>   -- works because the format is really the same.
> 
> The patch I posted to libevdev checks this driver version.

btw, where did you post the libevdev patch? I haven't seen it anywhere I'm
subscribed to.

> And, hence any library that results in a call to libevdev_set_fd()
> will fail if it is not this updated driver.

without having seen the libevdev patch - that sounds like a bad idea . there
are plenty of usecases where libevdev_set_fd() is called but timestamps in
events just don't matter. So we may need need some more negotiation between
the library user, libevdev and the kernel.

Cheers,
   Peter

> We could just do a similar check in every library also.
> I think the latter would be better.
> 
> So, the kernel patches can go in as a no-op right now and then I can
> add version checks to respective user space libraries.

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2 1/4] uinput: Add ioctl for using monotonic/ boot times

[Peter Hutterer]

On Thu, Oct 27, 2016 at 01:39:30PM -0700, Deepa Dinamani wrote:
> > hmm, I'm a bit confused here. This is an in-kernel bit only (passing the
> > time through uinput events has no effect). So why do we need an ioctl here?
> > it's an in-kernel decision only anyway and the time in the events sent to
> > the evdev client should be dictated by what that client sets for the clock
> > type, right?
> 
> This is for input events queued by the uinput driver for the virtual
> input device.

oh, right. I thought this was in the path for uinput_write(). sorry about
that.

> This can be read through uinput_read() fops.
> I don't think anybody is doing a read on uinput nodes, so another
> option(Arnd and I considered this) could be not supporting reads on
> these nodes at all.
> 
> This is not related to evdev events in the kernel.
> Currently, this timestamp could be the same format as the evdev
> timestamps or not.

I can say I've never done the read from the uinput device, never even
occured to me. quick skim of the code looks like this only matters for
force_feedback stuff. can't really comment on that too much.

Cheers,
   Peter
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2 3/4] input: Deprecate real timestamps beyond year 2106

[Deepa Dinamani]

>> struct timeval is not y2038 safe.
>> All usage of timeval in the kernel will be replaced by
>> y2038 safe structures.
>>
>> struct input_event maintains time for each input event.
>> Real time timestamps are not ideal for input as this
>> time can go backwards as noted in the patch a80b83b7b8
>> by John Stultz. Hence, having the input_event.time fields
>> only big enough for monotonic and boot times are
>> sufficient.
>>
>> Leave the original input_event as is. This is to maintain
>> backward compatibility with existing userspace interfaces
>> that use input_event.
>> Introduce a new replacement struct raw_input_event.
>> This replaces timeval with struct input_timeval. This structure
>> maintains time in __kernel_ulong_t or compat_ulong_t to allow
>> for architectures to override types as in the case of x32.
>>
>> The change requires any userspace utilities reading or writing
>> from event nodes to update their reading format to match
>> raw_input_event. The changes to the popular libraries will be
>> posted along with the kernel changes.
>> The driver version is also updated to reflect the change in
>> event format.
>
> If users are forced to update to adapt to the new event format, should
> we consider more radical changes? For example, does it make sense to
> send timestamp on every event? Maybe we should only send it once per
> event packet (between EV_SYN/SYN_REPORT)? What granularity do we need?
> Is there anything else in current protocol that we'd like to change?

I did see the thread with Pingbo's patches where you had a similar comment.

I see my series as decoupling the kernel input event format from the
userspace format.
The formats also are really the same still.
Could this be considered the first step towards changing the protocol?

The protocol changes might need new interfaces to be defined between libraries.
And, could end up being a substantial change.
Would a step by step approach make sense?

-Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2 3/4] input: Deprecate real timestamps beyond year 2106

[Deepa Dinamani]

On Wed, Oct 26, 2016 at 7:56 PM, Peter Hutterer
 wrote:
> On Mon, Oct 17, 2016 at 08:27:32PM -0700, Deepa Dinamani wrote:
>> struct timeval is not y2038 safe.
>> All usage of timeval in the kernel will be replaced by
>> y2038 safe structures.
>>
>> struct input_event maintains time for each input event.
>> Real time timestamps are not ideal for input as this
>> time can go backwards as noted in the patch a80b83b7b8
>> by John Stultz. Hence, having the input_event.time fields
>> only big enough for monotonic and boot times are
>> sufficient.
>>
>> Leave the original input_event as is. This is to maintain
>> backward compatibility with existing userspace interfaces
>> that use input_event.
>> Introduce a new replacement struct raw_input_event.
>
> general comment here - please don't name it "raw_input_event".
> First, when you grep for input_event you want the new ones to show up too,
> so a struct input_event_raw would be better here. That also has better
> namespacing in general. Second though: the event isn't any more "raw" than
> the previous we had.
>
> I can't think of anything better than struct input_event_v2 though.

The general idea was to leave the original struct input_event as a
common interface for userspace (as it cannot be deleted).
So reading raw data unformatted by the userspace will have the new
struct raw_input_event format.
This was the reason for the "raw" in the name.

struct input_event_v2 is fine too, if this is more preferred.

>> This replaces timeval with struct input_timeval. This structure
>> maintains time in __kernel_ulong_t or compat_ulong_t to allow
>> for architectures to override types as in the case of x32.
>>
>> The change requires any userspace utilities reading or writing
>> from event nodes to update their reading format to match
>> raw_input_event. The changes to the popular libraries will be
>> posted along with the kernel changes.
>> The driver version is also updated to reflect the change in
>> event format.
>
> Doesn't this break *all* of userspace then? I don't see anything to
> negotiate the type of input event the kernel gives me. And nothing right now
> checks for EVDEV_VERSION, so they all just assume it's a struct
> input_event. Best case, if the available events aren't a multiple of
> sizeof(struct input_event) userspace will bomb out, but unless that happens,
> everyone will just happily read old-style events.
>
> So we need some negotiation what is acceptable. Which also needs to address
> the race conditions we're going to get when events start coming in before
> the client has announced that it supports the new-style events.

No, this does not break any userspace right now.
Both struct input_event and struct raw_input_event are exactly the same today.
This will be the case until a 2038-safe glibc is used with a 64 bit time_t flag.

So these are the scenarios:
1. old kernel driver + new userspace
  -- should still be ok until 2038. Version checks could help discover these
2. new kernel driver + old userspace (without recompiled with new 2038 gblic)
  -- works because the format is really the same.

The patch I posted to libevdev checks this driver version.
And, hence any library that results in a call to libevdev_set_fd()
will fail if it is not this updated driver.
We could just do a similar check in every library also.
I think the latter would be better.

So, the kernel patches can go in as a no-op right now and then I can
add version checks to respective user space libraries.

-Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2 2/4] input: evdev: Replace timeval with timespec64

[Arnd Bergmann]

On Thursday, October 27, 2016 11:34:33 AM CEST Peter Hutterer wrote:
> > @@ -257,17 +264,20 @@ static void __pass_event(struct evdev_client *client,
> >  
> >  static void evdev_pass_values(struct evdev_client *client,
> >   const struct input_value *vals, unsigned int count,
> > - ktime_t *ev_time)
> > + struct timespec64 *ev_time)
> >  {
> >   struct evdev *evdev = client->evdev;
> >   const struct input_value *v;
> >   struct input_event event;
> > + struct timespec64 ts;
> >   bool wakeup = false;
> >  
> >   if (client->revoked)
> >   return;
> >  
> > - event.time = ktime_to_timeval(ev_time[client->clk_type]);
> > + ts = ev_time[client->clk_type];
> > + event.time.tv_sec = ts.tv_sec;
> > + event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> 
> you have ktime_get_* helpers below but you don't have one for timespec64 to
> struct timeval? That seems like a bug waitig to happen.

This is intentional to a certain degree: we don't have a timeval64
because any conversion to a new interface should prefer timespec64
or 64-bit nanoseconds, and we try to remove timeval (along with
timespec) from everywhere in the kernel because basically all uses
are problematic for y2038.

Note that after patch 3, event->time is no longer a 'timeval'
either, so even if we had a conversion function, we could
no longer use it here.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038