| Message ID | 20191126161824.337724-7-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/8] media: documentation: fix video_event description | expand |
On 12/13/19 4:08 PM, Arnd Bergmann wrote: > On Thu, Dec 12, 2019 at 4:43 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 11/26/19 5:18 PM, Arnd Bergmann wrote: >>> >>> switch (cmd) { >>> +#ifdef COMPAT_32BIT_TIME >> >> COMPAT_32BIT_TIME -> CONFIG_COMPAT_32BIT_TIME > > Fixed. > >>> + *vb = (struct v4l2_buffer) { >>> + .index = vb32.index, >>> + .type = vb32.type, >>> + .bytesused = vb32.bytesused, >>> + .flags = vb32.flags, >>> + .field = vb32.field, >>> + .timestamp.tv_sec = vb32.timestamp.tv_sec, >>> + .timestamp.tv_usec = vb32.timestamp.tv_usec, >>> + .timecode = vb32.timecode, >> >> You forgot to copy sequence. >> >>> + .memory = vb32.memory, >>> + .m.userptr = vb32.m.usercopy, >> >> usercopy -> userptr > > Fixed. > >>> + .length = vb32.length, >>> + .request_fd = vb32.request_fd, >>> + }; >>> + >>> + if (cmd == VIDIOC_QUERYBUF_TIME32) >>> + memset(&vb->length, 0, sizeof(*vb) - >>> + offsetof(struct v4l2_buffer, length)); >> >> It's from the field AFTER vb->length that this needs to be zeroed. It's best to >> use the CLEAR_AFTER_FIELD macro here. > > I'm a bit lost about this one: the fields that are not explicitly > uninitialized here are already set to zero by the assignment > above. Should this simply be a > > if (cmd == VIDIOC_QUERYBUF_TIME32) > vb->request_fd = 0; Yes, you are correct. That's much simpler. > > then? I don't remember where that memset() originally came > from or why request_fd has to be cleared here. > >>> @@ -3100,6 +3141,30 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd) >>> return -EFAULT; >>> break; >>> } >>> + case VIDIOC_QUERYBUF_TIME32: >>> + case VIDIOC_QBUF_TIME32: >>> + case VIDIOC_DQBUF_TIME32: >>> + case VIDIOC_PREPARE_BUF_TIME32: { >>> + struct v4l2_buffer *vb = parg; >>> + struct v4l2_buffer_time32 vb32 = { >>> + .index = vb->index, >>> + .type = vb->type, >>> + .bytesused = vb->bytesused, >>> + .flags = vb->flags, >>> + .field = vb->field, >>> + .timestamp.tv_sec = vb->timestamp.tv_sec, >>> + .timestamp.tv_usec = vb->timestamp.tv_usec, >>> + .timecode = vb->timecode, >> >> You forgot to copy sequence. > > Fixed. > >> With these changes this patch series passed both the 64 and 32 bit compliance >> tests (in fact, all the issues mentioned above were found with these compliance >> tests). > > Yay compliance tests! > >> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs, >> and there doesn't appear to be an easy way to compile a C++ program with musl. >> >> If you happen to have a test environment where you can compile C++ with musl, >> then let me know and I can give instructions on how to run the compliance tests. >> >> If you can't test that, then I can merge this regardless, and hope for the best >> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested. > > I've heard good things about the prebuilt toolchains from http://musl.cc/. > These seems to come with a libstdc++, but I have not tried that myself. I'll see if I can give those a spin, but if I can't get it to work quickly, then I don't plan on spending much time on it. Regards, Hans > > I've folded the change below into this patch in my y2038-v4l2-v6 branch > but have not been able to update the copy on git.kernel.org yet because of > server-side issues today. > > Arnd > > 8<----- > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c > b/drivers/media/v4l2-core/v4l2-ioctl.c > index c416870a3166..667225712343 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -3055,7 +3055,7 @@ static int video_get_user(void __user *arg, void > *parg, unsigned int cmd, > } > > switch (cmd) { > -#ifdef COMPAT_32BIT_TIME > +#ifdef CONFIG_COMPAT_32BIT_TIME > case VIDIOC_QUERYBUF_TIME32: > case VIDIOC_QBUF_TIME32: > case VIDIOC_DQBUF_TIME32: > @@ -3075,15 +3075,15 @@ static int video_get_user(void __user *arg, > void *parg, unsigned int cmd, > .timestamp.tv_sec = vb32.timestamp.tv_sec, > .timestamp.tv_usec = vb32.timestamp.tv_usec, > .timecode = vb32.timecode, > + .sequence = vb32.sequence, > .memory = vb32.memory, > - .m.userptr = vb32.m.usercopy, > + .m.userptr = vb32.m.userptr, > .length = vb32.length, > .request_fd = vb32.request_fd, > }; > > if (cmd == VIDIOC_QUERYBUF_TIME32) > - memset(&vb->length, 0, sizeof(*vb) - > - offsetof(struct v4l2_buffer, length)); > + vb->request_fd = 0; > > break; > } > @@ -3155,6 +3155,7 @@ static int video_put_user(void __user *arg, void > *parg, unsigned int cmd) > .timestamp.tv_sec = vb->timestamp.tv_sec, > .timestamp.tv_usec = vb->timestamp.tv_usec, > .timecode = vb->timecode, > + .sequence = vb->sequence, > .memory = vb->memory, > .m.userptr = vb->m.userptr, > .length = vb->length, >
On Sat, Dec 14, 2019 at 12:27 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 12/13/19 4:32 PM, Hans Verkuil wrote: > >>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs, > >>> and there doesn't appear to be an easy way to compile a C++ program with musl. > >>> > >>> If you happen to have a test environment where you can compile C++ with musl, > >>> then let me know and I can give instructions on how to run the compliance tests. > >>> > >>> If you can't test that, then I can merge this regardless, and hope for the best > >>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested. > >> > >> I've heard good things about the prebuilt toolchains from http://musl.cc/. > >> These seems to come with a libstdc++, but I have not tried that myself. > > > > I'll see if I can give those a spin, but if I can't get it to work quickly, > > then I don't plan on spending much time on it. > > I managed to build v4l2-ctl/compliance with those toolchains, but they seem to be > still using a 32-bit time_t. > > Do I need to get a specific version or do something special? My mistake: only musl-1.2.0 and up have 64-bit time_t, but this isn't released yet. According to https://wiki.musl-libc.org/roadmap.html, the release was planned for last month, no idea how long it will take. It appears that a snapshot build at http://more.musl.cc/7.5.0/x86_64-linux-musl/i686-linux-musl-native.tgz is new enough to have 64-bit time_t (according to include/bits/alltypes.h), but this is a month old as well, so it may have known bugs. Adding Zach to Cc here, maybe he already has plans for another build with the latest version. Arnd
On 12/14/19 10:44 PM, Arnd Bergmann wrote: > On Sat, Dec 14, 2019 at 12:27 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 12/13/19 4:32 PM, Hans Verkuil wrote: >>>>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs, >>>>> and there doesn't appear to be an easy way to compile a C++ program with musl. >>>>> >>>>> If you happen to have a test environment where you can compile C++ with musl, >>>>> then let me know and I can give instructions on how to run the compliance tests. >>>>> >>>>> If you can't test that, then I can merge this regardless, and hope for the best >>>>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested. >>>> >>>> I've heard good things about the prebuilt toolchains from http://musl.cc/. >>>> These seems to come with a libstdc++, but I have not tried that myself. >>> >>> I'll see if I can give those a spin, but if I can't get it to work quickly, >>> then I don't plan on spending much time on it. >> >> I managed to build v4l2-ctl/compliance with those toolchains, but they seem to be >> still using a 32-bit time_t. >> >> Do I need to get a specific version or do something special? > > My mistake: only musl-1.2.0 and up have 64-bit time_t, but this isn't released > yet. According to https://wiki.musl-libc.org/roadmap.html, the release > was planned > for last month, no idea how long it will take. > > It appears that a snapshot build at > http://more.musl.cc/7.5.0/x86_64-linux-musl/i686-linux-musl-native.tgz > is new enough to have 64-bit time_t (according to include/bits/alltypes.h), > but this is a month old as well, so it may have known bugs. Ah, great, that worked, after applying the patch below. Both struct v4l2_buffer32 and v4l2_event32 need to be packed, otherwise you would get an additional 4 bytes since the 64 bit compiler wants to align the 8 byte tv_secs to an 8 byte boundary. But that's not what the i686 compiler does. If I remember correctly, packed is only needed for CONFIG_X86_64. With these changes (plus a bunch of fixes for v4l-utils) I was able to do full compliance tests for 64-bit, 32-bit time32 under x86_64, 32-bit time64 under x86_64, time32 under i686 and time64 under i686. Arnd, if you can post a v6 with the previous fixes and this fix included, then I'll make a pull request for this for kernel 5.6. Regards, Hans Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 3bbf47d950e0..c01492cf6160 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -492,7 +492,11 @@ struct v4l2_buffer32 { __u32 length; __u32 reserved2; __s32 request_fd; +#ifdef CONFIG_X86_64 +} __attribute__ ((packed)); +#else }; +#endif struct v4l2_buffer32_time32 { __u32 index; @@ -1280,7 +1284,7 @@ struct v4l2_event32 { struct __kernel_timespec timestamp; __u32 id; __u32 reserved[8]; -}; +} __attribute__ ((packed)); struct v4l2_event32_time32 { __u32 type;
On Sun, Dec 15, 2019 at 6:26 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Ah, great, that worked, after applying the patch below. > > Both struct v4l2_buffer32 and v4l2_event32 need to be packed, otherwise you would > get an additional 4 bytes since the 64 bit compiler wants to align the 8 byte tv_secs > to an 8 byte boundary. But that's not what the i686 compiler does. Thanks so much for the testing and finding this issue. It would be much more embarrassing to find it later, given that I explained how it's supposed to work in the comment above v4l2_event32 and in the documentation I just submitted but got it wrong anyway ;-) > If I remember correctly, packed is only needed for CONFIG_X86_64. Correct. > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 3bbf47d950e0..c01492cf6160 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -492,7 +492,11 @@ struct v4l2_buffer32 { > __u32 length; > __u32 reserved2; > __s32 request_fd; > +#ifdef CONFIG_X86_64 > +} __attribute__ ((packed)); > +#else > }; > +#endif I would prefer to write it like this instead to avoid the #ifdef, the effect should be the same: --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -475,8 +475,8 @@ struct v4l2_buffer32 { __u32 flags; __u32 field; /* enum v4l2_field */ struct { - long long tv_sec; - long long tv_usec; + compat_s64 tv_sec; + compat_s64 tv_usec; } timestamp; struct v4l2_timecode timecode; __u32 sequence; --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -1277,7 +1277,10 @@ struct v4l2_event32 { } u; __u32 pending; __u32 sequence; - struct __kernel_timespec timestamp; + struct { + compat_s64 tv_sec; + compat_s64 tv_usec; + } timestamp; __u32 id; __u32 reserved[8]; }; If you agree, I'll push out a modified branch with that version and send out that series to the list again. There is one more complication that I just noticed: The "struct v4l2_buffer32" definition has always been defined in a way that works for i386 user space but is broken for x32 user space. The version I used accidentally fixed x32 while breaking i386. With the change above, it's back to missing x32 support (so nothing changed). There is no way to fix the uapi definition of v4l2_buffer to have x32 and i386 use the same format, because applications may be using old headers, but I suppose I could add yet another version of the struct to correctly deal with x32, or just add a comment like --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -468,6 +468,10 @@ struct v4l2_plane32 { __u32 reserved[11]; }; +/* + * This is correct for all architectures including i386, but not x32, + * which has different alignment requirements for timestamp + */ struct v4l2_buffer32 { __u32 index; __u32 type; /* enum v4l2_buf_type */ Arnd
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 96aafb659783..4d611a847462 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -474,10 +474,10 @@ static void v4l_print_buffer(const void *arg, bool write_only) const struct v4l2_plane *plane; int i; - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s", - p->timestamp.tv_sec / 3600, - (int)(p->timestamp.tv_sec / 60) % 60, - (int)(p->timestamp.tv_sec % 60), + pr_cont("%02d:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s", + (int)p->timestamp.tv_sec / 3600, + ((int)p->timestamp.tv_sec / 60) % 60, + ((int)p->timestamp.tv_sec % 60), (long)p->timestamp.tv_usec, p->index, prt_names(p->type, v4l2_type_names), p->request_fd, @@ -3029,6 +3029,14 @@ static unsigned int video_translate_cmd(unsigned int cmd) #ifdef CONFIG_COMPAT_32BIT_TIME case VIDIOC_DQEVENT_TIME32: return VIDIOC_DQEVENT; + case VIDIOC_QUERYBUF_TIME32: + return VIDIOC_QUERYBUF; + case VIDIOC_QBUF_TIME32: + return VIDIOC_QBUF; + case VIDIOC_DQBUF_TIME32: + return VIDIOC_DQBUF; + case VIDIOC_PREPARE_BUF_TIME32: + return VIDIOC_PREPARE_BUF; #endif } @@ -3047,6 +3055,39 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd, } switch (cmd) { +#ifdef COMPAT_32BIT_TIME + case VIDIOC_QUERYBUF_TIME32: + case VIDIOC_QBUF_TIME32: + case VIDIOC_DQBUF_TIME32: + case VIDIOC_PREPARE_BUF_TIME32: { + struct v4l2_buffer_time32 vb32; + struct v4l2_buffer *vb = parg; + + if (copy_from_user(&vb32, arg, sizeof(vb32))) + return -EFAULT; + + *vb = (struct v4l2_buffer) { + .index = vb32.index, + .type = vb32.type, + .bytesused = vb32.bytesused, + .flags = vb32.flags, + .field = vb32.field, + .timestamp.tv_sec = vb32.timestamp.tv_sec, + .timestamp.tv_usec = vb32.timestamp.tv_usec, + .timecode = vb32.timecode, + .memory = vb32.memory, + .m.userptr = vb32.m.usercopy, + .length = vb32.length, + .request_fd = vb32.request_fd, + }; + + if (cmd == VIDIOC_QUERYBUF_TIME32) + memset(&vb->length, 0, sizeof(*vb) - + offsetof(struct v4l2_buffer, length)); + + break; + } +#endif default: /* * In some cases, only a few fields are used as input, @@ -3100,6 +3141,30 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd) return -EFAULT; break; } + case VIDIOC_QUERYBUF_TIME32: + case VIDIOC_QBUF_TIME32: + case VIDIOC_DQBUF_TIME32: + case VIDIOC_PREPARE_BUF_TIME32: { + struct v4l2_buffer *vb = parg; + struct v4l2_buffer_time32 vb32 = { + .index = vb->index, + .type = vb->type, + .bytesused = vb->bytesused, + .flags = vb->flags, + .field = vb->field, + .timestamp.tv_sec = vb->timestamp.tv_sec, + .timestamp.tv_usec = vb->timestamp.tv_usec, + .timecode = vb->timecode, + .memory = vb->memory, + .m.userptr = vb->m.userptr, + .length = vb->length, + .request_fd = vb->request_fd, + }; + + if (copy_to_user(arg, &vb32, sizeof(vb32))) + return -EFAULT; + break; + } #endif default: /* Copy results into user buffer */ diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index 05c1ec93a911..86878fba332b 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -749,4 +749,34 @@ struct v4l2_event_time32 { #define VIDIOC_DQEVENT_TIME32 _IOR('V', 89, struct v4l2_event_time32) +struct v4l2_buffer_time32 { + __u32 index; + __u32 type; + __u32 bytesused; + __u32 flags; + __u32 field; + struct old_timeval32 timestamp; + struct v4l2_timecode timecode; + __u32 sequence; + + /* memory location */ + __u32 memory; + union { + __u32 offset; + unsigned long userptr; + struct v4l2_plane *planes; + __s32 fd; + } m; + __u32 length; + __u32 reserved2; + union { + __s32 request_fd; + __u32 reserved; + }; +}; +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32) +#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32) +#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32) +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32) + #endif /* _V4L2_IOCTL_H */ diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index caf156d45842..5f9357dcb060 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -912,6 +912,25 @@ struct v4l2_jpegcompression { /* * M E M O R Y - M A P P I N G B U F F E R S */ + +#ifdef __KERNEL__ +/* + * This corresponds to the user space version of timeval + * for 64-bit time_t. sparc64 is different from everyone + * else, using the microseconds in the wrong half of the + * second 64-bit word. + */ +struct __kernel_v4l2_timeval { + long long tv_sec; +#if defined(__sparc__) && defined(__arch64__) + int tv_usec; + int __pad; +#else + long long tv_usec; +#endif +}; +#endif + struct v4l2_requestbuffers { __u32 count; __u32 type; /* enum v4l2_buf_type */ @@ -997,7 +1016,11 @@ struct v4l2_buffer { __u32 bytesused; __u32 flags; __u32 field; +#ifdef __KERNEL__ + struct __kernel_v4l2_timeval timestamp; +#else struct timeval timestamp; +#endif struct v4l2_timecode timecode; __u32 sequence;
The v4l2_buffer structure contains a 'struct timeval' member that is defined by the user space C library, creating an ABI incompatibility when that gets updated to a 64-bit time_t. As in v4l2_event, handle this with a special case in video_put_user() and video_get_user() to replace the memcpy there. Since the structure also contains a pointer, there are now two native versions (on 32-bit systems) as well as two compat versions (on 64-bit systems), which unfortunately complicates the compat handler quite a bit. Duplicating the existing handlers for the new types is a safe conversion for now, but unfortunately this may turn into a maintenance burden later. A larger-scale rework of the compat code might be a better alternative, but is out of scope of the y2038 work. Sparc64 needs a special case because of their special suseconds_t definition. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/media/v4l2-core/v4l2-ioctl.c | 73 ++++++++++++++++++++++++++-- include/media/v4l2-ioctl.h | 30 ++++++++++++ include/uapi/linux/videodev2.h | 23 +++++++++ 3 files changed, 122 insertions(+), 4 deletions(-) -- 2.20.0