| Message ID | 1442332148-488079-8-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/15/2015 05:49 PM, Arnd Bergmann wrote: > The v4l2 event queue uses a 'struct timespec' to pass monotonic > timestamps. This is not a problem by itself, but it breaks when user > space redefines timespec to use 'long long' on 32-bit systems. > > To avoid that problem, we define our own replacement for timespec here, > using 'long' tv_sec and tv_nsec members. This means no change to the > existing API, but lets us keep using it with a y2038 compatible libc. > > As a side-effect, this changes the API for x32 programs to be the same > as native 32-bit, using a 32-bit tv_sec/tv_nsec value, but both old and > new kernels already support both formats for on the binary level for > compat and x32. > > Alternatively, we could leave the header file to define the interface > based on 'struct timespec', and implement both ioctls for native > processes. I picked the approach in this case because it matches what > we do for v4l2_timeval in the respective patch, but both would work > equally well here. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/v4l2-core/v4l2-event.c | 20 +++++++++++++------- > include/uapi/linux/videodev2.h | 8 +++++++- > 2 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c > index 8d3171c6bee8..2a26ad591f59 100644 > --- a/drivers/media/v4l2-core/v4l2-event.c > +++ b/drivers/media/v4l2-core/v4l2-event.c > @@ -108,7 +108,7 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed( > } > > static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev, > - const struct timespec *ts) > + const struct v4l2_timespec *ts) > { > struct v4l2_subscribed_event *sev; > struct v4l2_kevent *kev; > @@ -170,17 +170,20 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev) > { > struct v4l2_fh *fh; > unsigned long flags; > - struct timespec timestamp; > + struct timespec64 timestamp; > + struct v4l2_timespec vts; > > if (vdev == NULL) > return; > > - ktime_get_ts(×tamp); > + ktime_get_ts64(×tamp); > + vts.tv_sec = timestamp.tv_sec; > + vts.tv_nsec = timestamp.tv_nsec; I prefer to take this opportunity to create a v4l2_get_timespec helper function, just like v4l2_get_timeval. > > spin_lock_irqsave(&vdev->fh_lock, flags); > > list_for_each_entry(fh, &vdev->fh_list, list) > - __v4l2_event_queue_fh(fh, ev, ×tamp); > + __v4l2_event_queue_fh(fh, ev, &vts); > > spin_unlock_irqrestore(&vdev->fh_lock, flags); > } > @@ -189,12 +192,15 @@ EXPORT_SYMBOL_GPL(v4l2_event_queue); > void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev) > { > unsigned long flags; > - struct timespec timestamp; > + struct timespec64 timestamp; > + struct v4l2_timespec vts; > > - ktime_get_ts(×tamp); > + ktime_get_ts64(×tamp); > + vts.tv_sec = timestamp.tv_sec; > + vts.tv_nsec = timestamp.tv_nsec; > > spin_lock_irqsave(&fh->vdev->fh_lock, flags); > - __v4l2_event_queue_fh(fh, ev, ×tamp); > + __v4l2_event_queue_fh(fh, ev, &vts); > spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); > } > EXPORT_SYMBOL_GPL(v4l2_event_queue_fh); > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index b02cf054fbb8..bc659be87260 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -809,6 +809,12 @@ struct v4l2_timeval { > long tv_usec; > }; > > +/* used for monotonic times, therefore y2038 safe */ > +struct v4l2_timespec { > + long tv_sec; > + long tv_nsec; > +}; > + > /** > * struct v4l2_buffer - video buffer info > * @index: id number of the buffer > @@ -2088,7 +2094,7 @@ struct v4l2_event { > } u; > __u32 pending; > __u32 sequence; > - struct timespec timestamp; > + struct v4l2_timespec timestamp; > __u32 id; > __u32 reserved[8]; > }; > I think I am OK with this. This timestamp is used much more rarely and I do not expect this ABI change to cause any problems in userspace. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 15 September 2015 18:32:36 Hans Verkuil wrote: > > > > - ktime_get_ts(×tamp); > > + ktime_get_ts64(×tamp); > > + vts.tv_sec = timestamp.tv_sec; > > + vts.tv_nsec = timestamp.tv_nsec; > > I prefer to take this opportunity to create a v4l2_get_timespec helper > function, just like v4l2_get_timeval. Ok, good idea. I'll do that once we have agreed on the ABI. > > @@ -2088,7 +2094,7 @@ struct v4l2_event { > > } u; > > __u32 pending; > > __u32 sequence; > > - struct timespec timestamp; > > + struct v4l2_timespec timestamp; > > __u32 id; > > __u32 reserved[8]; > > }; > > > > I think I am OK with this. This timestamp is used much more rarely and I do > not expect this ABI change to cause any problems in userspace. I'd still wait the outcome of the v4l2_timeval discussion though. It may be useful for consistency to pick the same approach for both structures. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c index 8d3171c6bee8..2a26ad591f59 100644 --- a/drivers/media/v4l2-core/v4l2-event.c +++ b/drivers/media/v4l2-core/v4l2-event.c @@ -108,7 +108,7 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed( } static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev, - const struct timespec *ts) + const struct v4l2_timespec *ts) { struct v4l2_subscribed_event *sev; struct v4l2_kevent *kev; @@ -170,17 +170,20 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev) { struct v4l2_fh *fh; unsigned long flags; - struct timespec timestamp; + struct timespec64 timestamp; + struct v4l2_timespec vts; if (vdev == NULL) return; - ktime_get_ts(×tamp); + ktime_get_ts64(×tamp); + vts.tv_sec = timestamp.tv_sec; + vts.tv_nsec = timestamp.tv_nsec; spin_lock_irqsave(&vdev->fh_lock, flags); list_for_each_entry(fh, &vdev->fh_list, list) - __v4l2_event_queue_fh(fh, ev, ×tamp); + __v4l2_event_queue_fh(fh, ev, &vts); spin_unlock_irqrestore(&vdev->fh_lock, flags); } @@ -189,12 +192,15 @@ EXPORT_SYMBOL_GPL(v4l2_event_queue); void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev) { unsigned long flags; - struct timespec timestamp; + struct timespec64 timestamp; + struct v4l2_timespec vts; - ktime_get_ts(×tamp); + ktime_get_ts64(×tamp); + vts.tv_sec = timestamp.tv_sec; + vts.tv_nsec = timestamp.tv_nsec; spin_lock_irqsave(&fh->vdev->fh_lock, flags); - __v4l2_event_queue_fh(fh, ev, ×tamp); + __v4l2_event_queue_fh(fh, ev, &vts); spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); } EXPORT_SYMBOL_GPL(v4l2_event_queue_fh); diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index b02cf054fbb8..bc659be87260 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -809,6 +809,12 @@ struct v4l2_timeval { long tv_usec; }; +/* used for monotonic times, therefore y2038 safe */ +struct v4l2_timespec { + long tv_sec; + long tv_nsec; +}; + /** * struct v4l2_buffer - video buffer info * @index: id number of the buffer @@ -2088,7 +2094,7 @@ struct v4l2_event { } u; __u32 pending; __u32 sequence; - struct timespec timestamp; + struct v4l2_timespec timestamp; __u32 id; __u32 reserved[8]; };
The v4l2 event queue uses a 'struct timespec' to pass monotonic timestamps. This is not a problem by itself, but it breaks when user space redefines timespec to use 'long long' on 32-bit systems. To avoid that problem, we define our own replacement for timespec here, using 'long' tv_sec and tv_nsec members. This means no change to the existing API, but lets us keep using it with a y2038 compatible libc. As a side-effect, this changes the API for x32 programs to be the same as native 32-bit, using a 32-bit tv_sec/tv_nsec value, but both old and new kernels already support both formats for on the binary level for compat and x32. Alternatively, we could leave the header file to define the interface based on 'struct timespec', and implement both ioctls for native processes. I picked the approach in this case because it matches what we do for v4l2_timeval in the respective patch, but both would work equally well here. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/media/v4l2-core/v4l2-event.c | 20 +++++++++++++------- include/uapi/linux/videodev2.h | 8 +++++++- 2 files changed, 20 insertions(+), 8 deletions(-)