diff mbox series

[v4,2/8] media: v4l2: abstract timeval handling in v4l2_buffer

Message ID 20191111203835.2260382-3-arnd@arndb.de
State New
Headers show
Series [v4,1/8] media: documentation: fix video_event description | expand

Commit Message

Arnd Bergmann Nov. 11, 2019, 8:38 p.m. UTC 
As a preparation for adding 64-bit time_t support in the uapi,
change the drivers to no longer care about the format of the
timestamp field in struct v4l2_buffer.

The v4l2_timeval_to_ns() function is no longer needed in the
kernel after this, but there may be userspace code relying on
it because it is part of the uapi header.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/media/common/videobuf2/videobuf2-v4l2.c |  4 ++--
 drivers/media/pci/meye/meye.c                   |  4 ++--
 drivers/media/usb/cpia2/cpia2_v4l.c             |  4 ++--
 drivers/media/usb/stkwebcam/stk-webcam.c        |  2 +-
 drivers/media/usb/usbvision/usbvision-video.c   |  4 ++--
 drivers/media/v4l2-core/videobuf-core.c         |  4 ++--
 include/linux/videodev2.h                       | 17 ++++++++++++++++-
 include/trace/events/v4l2.h                     |  2 +-
 include/uapi/linux/videodev2.h                  |  2 ++
 9 files changed, 30 insertions(+), 13 deletions(-)

-- 
2.20.0

Comments

Arnd Bergmann Nov. 26, 2019, 11:34 a.m. UTC | #1 
On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>

> On 11/11/19 9:38 PM, Arnd Bergmann wrote:

> > As a preparation for adding 64-bit time_t support in the uapi,

> > change the drivers to no longer care about the format of the

> > timestamp field in struct v4l2_buffer.

> >

> > The v4l2_timeval_to_ns() function is no longer needed in the

> > kernel after this, but there may be userspace code relying on

> > it because it is part of the uapi header.

>

> There is indeed userspace code that relies on this.


Ok, good to know. I rephrased the changelog text as

The v4l2_timeval_to_ns() function is no longer needed in the
kernel after this, but there is userspace code relying on
it to be part of the uapi header.

> >

> > +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)

> > +{

> > +     return buf->timestamp.tv_sec * NSEC_PER_SEC +

> > +            (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;

>

> Why the (u32) cast?


Simple question, long answer:

on 32-bit architectures, the tv_usec member may be 32-bit wide plus
padding in user space when interpreted as a regular 'struct timeval',
but the kernel implementation now sees it as a 64-bit member,
with half of it being possibly uninitialized user space data.

The 32-bit cast avoids that uninitialized data and ensures user space
passing garbage in the upper half gets ignored, as it has to be on 32-bit
user space.

On 64-bit native user space, the tv_usec field is always 64 bit wide,
so this is a change in behavior for denormalized timeval data
with tv_usec > U32_MAX, but the current behavior does not appear
worth preserving either.

The correct way would probably be to return an error for
 tv_usec >USEC_PER_SEC, but as the code never did that, this
would risk a regression for user space that relies on passing
invalid timestamps without getting an error.

> > +static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,

> > +                                          u64 timestamp)

> > +{

> > +     struct timespec64 ts = ns_to_timespec64(timestamp);

> > +

> > +     buf->timestamp.tv_sec  = ts.tv_sec;

> > +     buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;

> > +}

> > +

>

> This does not belong in the public header. This is kernel specific,


Note: this is not the uapi header but the in-kernel one.

> so media/v4l2-common.h would be a good place.


Ok, sounds good. I wasn't sure where to put it, and ended up
with include/linux/videodev2.h as the best replacement for
include/uapi/linux/videodev2.h, changed it to
include/media/v4l2-common.h now.

       Arnd
Hans Verkuil Nov. 26, 2019, 11:43 a.m. UTC | #2 
On 11/26/19 12:34 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

>>

>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:

>>> As a preparation for adding 64-bit time_t support in the uapi,

>>> change the drivers to no longer care about the format of the

>>> timestamp field in struct v4l2_buffer.

>>>

>>> The v4l2_timeval_to_ns() function is no longer needed in the

>>> kernel after this, but there may be userspace code relying on

>>> it because it is part of the uapi header.

>>

>> There is indeed userspace code that relies on this.

> 

> Ok, good to know. I rephrased the changelog text as

> 

> The v4l2_timeval_to_ns() function is no longer needed in the

> kernel after this, but there is userspace code relying on

> it to be part of the uapi header.

> 

>>>

>>> +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)

>>> +{

>>> +     return buf->timestamp.tv_sec * NSEC_PER_SEC +

>>> +            (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;

>>

>> Why the (u32) cast?

> 

> Simple question, long answer:

> 

> on 32-bit architectures, the tv_usec member may be 32-bit wide plus

> padding in user space when interpreted as a regular 'struct timeval',

> but the kernel implementation now sees it as a 64-bit member,

> with half of it being possibly uninitialized user space data.

> 

> The 32-bit cast avoids that uninitialized data and ensures user space

> passing garbage in the upper half gets ignored, as it has to be on 32-bit

> user space.


But that's only valid for little endian 32 bit systems, right?
Is this only an issue for x86 platforms?

> 

> On 64-bit native user space, the tv_usec field is always 64 bit wide,

> so this is a change in behavior for denormalized timeval data

> with tv_usec > U32_MAX, but the current behavior does not appear

> worth preserving either.

> 

> The correct way would probably be to return an error for

>  tv_usec >USEC_PER_SEC, but as the code never did that, this

> would risk a regression for user space that relies on passing

> invalid timestamps without getting an error.


This long answer needs to be added to a comment to that function.
Because otherwise someone will come along later and remove that
seemingly unnecessary cast.

It's OK if it is a long comment, it's a non-trivial reason.

> 

>>> +static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,

>>> +                                          u64 timestamp)

>>> +{

>>> +     struct timespec64 ts = ns_to_timespec64(timestamp);

>>> +

>>> +     buf->timestamp.tv_sec  = ts.tv_sec;

>>> +     buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;

>>> +}

>>> +

>>

>> This does not belong in the public header. This is kernel specific,

> 

> Note: this is not the uapi header but the in-kernel one.


Ah, I missed that.

> 

>> so media/v4l2-common.h would be a good place.

> 

> Ok, sounds good. I wasn't sure where to put it, and ended up

> with include/linux/videodev2.h as the best replacement for

> include/uapi/linux/videodev2.h, changed it to

> include/media/v4l2-common.h now.


Never use include/linux/videodev2.h. It's just a wrapper around
the uapi header and should not contain any 'real' code.

It's also why I missed that you modified that header since we never
touch it.

Regards,

	Hans

> 

>        Arnd

>
Arnd Bergmann Nov. 26, 2019, 12:42 p.m. UTC | #3 
On Tue, Nov 26, 2019 at 12:43 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 11/26/19 12:34 PM, Arnd Bergmann wrote:

> > On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> >>>

> >>> +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)

> >>> +{

> >>> +     return buf->timestamp.tv_sec * NSEC_PER_SEC +

> >>> +            (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;

> >>

> >> Why the (u32) cast?

> >

> > Simple question, long answer:

> >

> > on 32-bit architectures, the tv_usec member may be 32-bit wide plus

> > padding in user space when interpreted as a regular 'struct timeval',

> > but the kernel implementation now sees it as a 64-bit member,

> > with half of it being possibly uninitialized user space data.

> >

> > The 32-bit cast avoids that uninitialized data and ensures user space

> > passing garbage in the upper half gets ignored, as it has to be on 32-bit

> > user space.

>

> But that's only valid for little endian 32 bit systems, right?

> Is this only an issue for x86 platforms?


Uninitialized data is an issue on all 32-bit architectures. The layout
of the new timeval is such that the low 32 bits of tv_sec are in the
same place on both 32-bit and 64-bit architectures of the same
endianess, but if an application initializes the fields individually
without a memset before it, it may still pass invalid data.

> > On 64-bit native user space, the tv_usec field is always 64 bit wide,

> > so this is a change in behavior for denormalized timeval data

> > with tv_usec > U32_MAX, but the current behavior does not appear

> > worth preserving either.

> >

> > The correct way would probably be to return an error for

> >  tv_usec >USEC_PER_SEC, but as the code never did that, this

> > would risk a regression for user space that relies on passing

> > invalid timestamps without getting an error.

>

> This long answer needs to be added to a comment to that function.

> Because otherwise someone will come along later and remove that

> seemingly unnecessary cast.

>

> It's OK if it is a long comment, it's a non-trivial reason.


Added this comment now:

        /*
         * When the timestamp comes from 32-bit user space, there may be
         * uninitialized data in tv_usec, so cast it to u32.
         * Otherwise allow invalid input for backwards compatibility.
         */

Let me know if you prefer a more elaborate version.

> >> so media/v4l2-common.h would be a good place.

> >

> > Ok, sounds good. I wasn't sure where to put it, and ended up

> > with include/linux/videodev2.h as the best replacement for

> > include/uapi/linux/videodev2.h, changed it to

> > include/media/v4l2-common.h now.

>

> Never use include/linux/videodev2.h. It's just a wrapper around

> the uapi header and should not contain any 'real' code.

>

> It's also why I missed that you modified that header since we never

> touch it.


Ok, got it. I now tried to remove this file completely, hoping that the
include <linux/time.h> is no longer needed after my series, but
it seems we still need it.

       Arnd
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 5a9ba3846f0a..9ec710878db6 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -143,7 +143,7 @@  static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
 		 * and the timecode field and flag if needed.
 		 */
 		if (q->copy_timestamp)
-			vb->timestamp = v4l2_timeval_to_ns(&b->timestamp);
+			vb->timestamp = v4l2_buffer_get_timestamp(b);
 		vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
 		if (b->flags & V4L2_BUF_FLAG_TIMECODE)
 			vbuf->timecode = b->timecode;
@@ -476,7 +476,7 @@  static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 
 	b->flags = vbuf->flags;
 	b->field = vbuf->field;
-	b->timestamp = ns_to_timeval(vb->timestamp);
+	v4l2_buffer_set_timestamp(b, vb->timestamp);
 	b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
 	b->reserved2 = 0;
diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
index 0e61c81356ef..3a4c29bc0ba5 100644
--- a/drivers/media/pci/meye/meye.c
+++ b/drivers/media/pci/meye/meye.c
@@ -1266,7 +1266,7 @@  static int vidioc_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		buf->flags |= V4L2_BUF_FLAG_DONE;
 
 	buf->field = V4L2_FIELD_NONE;
-	buf->timestamp = ns_to_timeval(meye.grab_buffer[index].ts);
+	v4l2_buffer_set_timestamp(buf, meye.grab_buffer[index].ts);
 	buf->sequence = meye.grab_buffer[index].sequence;
 	buf->memory = V4L2_MEMORY_MMAP;
 	buf->m.offset = index * gbufsize;
@@ -1332,7 +1332,7 @@  static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->bytesused = meye.grab_buffer[reqnr].size;
 	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->field = V4L2_FIELD_NONE;
-	buf->timestamp = ns_to_timeval(meye.grab_buffer[reqnr].ts);
+	v4l2_buffer_set_timestamp(buf, meye.grab_buffer[reqnr].ts);
 	buf->sequence = meye.grab_buffer[reqnr].sequence;
 	buf->memory = V4L2_MEMORY_MMAP;
 	buf->m.offset = reqnr * gbufsize;
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 626264a56517..9d3d05125d7b 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -800,7 +800,7 @@  static int cpia2_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		break;
 	case FRAME_READY:
 		buf->bytesused = cam->buffers[buf->index].length;
-		buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts);
+		v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts);
 		buf->sequence = cam->buffers[buf->index].seq;
 		buf->flags = V4L2_BUF_FLAG_DONE;
 		break;
@@ -907,7 +907,7 @@  static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE
 		| V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->field = V4L2_FIELD_NONE;
-	buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts);
+	v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts);
 	buf->sequence = cam->buffers[buf->index].seq;
 	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
 	buf->length = cam->frame_size;
diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index 21f90a887485..b22501f76b78 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -1125,7 +1125,7 @@  static int stk_vidioc_dqbuf(struct file *filp,
 	sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;
 	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_DONE;
 	sbuf->v4lbuf.sequence = ++dev->sequence;
-	sbuf->v4lbuf.timestamp = ns_to_timeval(ktime_get_ns());
+	v4l2_buffer_set_timestamp(&sbuf->v4lbuf, ktime_get_ns());
 
 	*buf = sbuf->v4lbuf;
 	return 0;
diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c
index cdc66adda755..15a423c5deb7 100644
--- a/drivers/media/usb/usbvision/usbvision-video.c
+++ b/drivers/media/usb/usbvision/usbvision-video.c
@@ -687,7 +687,7 @@  static int vidioc_querybuf(struct file *file,
 	vb->length = usbvision->curwidth *
 		usbvision->curheight *
 		usbvision->palette.bytes_per_pixel;
-	vb->timestamp = ns_to_timeval(usbvision->frame[vb->index].ts);
+	v4l2_buffer_set_timestamp(vb, usbvision->frame[vb->index].ts);
 	vb->sequence = usbvision->frame[vb->index].sequence;
 	return 0;
 }
@@ -756,7 +756,7 @@  static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *vb)
 		V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	vb->index = f->index;
 	vb->sequence = f->sequence;
-	vb->timestamp = ns_to_timeval(f->ts);
+	v4l2_buffer_set_timestamp(vb, f->ts);
 	vb->field = V4L2_FIELD_NONE;
 	vb->bytesused = f->scanlength;
 
diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
index 939fc11cf080..ab650371c151 100644
--- a/drivers/media/v4l2-core/videobuf-core.c
+++ b/drivers/media/v4l2-core/videobuf-core.c
@@ -364,7 +364,7 @@  static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
 	}
 
 	b->field     = vb->field;
-	b->timestamp = ns_to_timeval(vb->ts);
+	v4l2_buffer_set_timestamp(b, vb->ts);
 	b->bytesused = vb->size;
 	b->sequence  = vb->field_count >> 1;
 }
@@ -578,7 +578,7 @@  int videobuf_qbuf(struct videobuf_queue *q, struct v4l2_buffer *b)
 		    || q->type == V4L2_BUF_TYPE_SDR_OUTPUT) {
 			buf->size = b->bytesused;
 			buf->field = b->field;
-			buf->ts = v4l2_timeval_to_ns(&b->timestamp);
+			buf->ts = v4l2_buffer_get_timestamp(b);
 		}
 		break;
 	case V4L2_MEMORY_USERPTR:
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 16c0ed6c50a7..4086036e37d5 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -56,7 +56,22 @@ 
 #ifndef __LINUX_VIDEODEV2_H
 #define __LINUX_VIDEODEV2_H
 
-#include <linux/time.h>     /* need struct timeval */
+#include <linux/time.h>
 #include <uapi/linux/videodev2.h>
 
+static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
+{
+	return buf->timestamp.tv_sec * NSEC_PER_SEC +
+	       (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;
+}
+
+static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
+					     u64 timestamp)
+{
+	struct timespec64 ts = ns_to_timespec64(timestamp);
+
+	buf->timestamp.tv_sec  = ts.tv_sec;
+	buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+}
+
 #endif /* __LINUX_VIDEODEV2_H */
diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
index 83860de120e3..248bc09bfc99 100644
--- a/include/trace/events/v4l2.h
+++ b/include/trace/events/v4l2.h
@@ -130,7 +130,7 @@  DECLARE_EVENT_CLASS(v4l2_event_class,
 		__entry->bytesused = buf->bytesused;
 		__entry->flags = buf->flags;
 		__entry->field = buf->field;
-		__entry->timestamp = timeval_to_ns(&buf->timestamp);
+		__entry->timestamp = v4l2_buffer_get_timestamp(buf);
 		__entry->timecode_type = buf->timecode.type;
 		__entry->timecode_flags = buf->timecode.flags;
 		__entry->timecode_frames = buf->timecode.frames;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 530638dffd93..74d3d522f3db 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1010,6 +1010,7 @@  struct v4l2_buffer {
 	};
 };
 
+#ifndef __KERNEL__
 /**
  * v4l2_timeval_to_ns - Convert timeval to nanoseconds
  * @ts:		pointer to the timeval variable to be converted
@@ -1021,6 +1022,7 @@  static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
 {
 	return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000;
 }
+#endif
 
 /*  Flags for 'flags' field */
 /* Buffer is mapped (flag) */