Re: [Y2038] [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 12/16/19 10:29 AM, Arnd Bergmann wrote:
> On Sun, Dec 15, 2019 at 6:26 PM Hans Verkuil 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_timecodetimecode;
> __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_timespectimestamp;
> + 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.
That's fine. I did a quick test with this and it looks fine.
>
> 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
>
Go with a comment. We've never tested with x32 to be honest. There were
discussions
about a year ago of dropping x32 altogether, but that hasn't happened yet.
Regards,
Hans
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 12/14/19 10:44 PM, Arnd Bergmann wrote:
> On Sat, Dec 14, 2019 at 12:27 PM Hans Verkuil 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
---
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_timespectimestamp;
__u32 id;
__u32 reserved[8];
-};
+} __attribute__ ((packed));
struct v4l2_event32_time32 {
__u32 type;
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
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? Regards, Hans ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 12/13/19 4:08 PM, Arnd Bergmann wrote:
> On Thu, Dec 12, 2019 at 4:43 PM Hans Verkuil 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
Re: [Y2038] [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 11/26/19 5:18 PM, Arnd Bergmann wrote:
> 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
> ---
> 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(-)
>
> 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
COMPAT_32BIT_TIME -> CONFIG_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,
You forgot to copy sequence.
> + .memory = vb32.memory,
> + .m.userptr = vb32.m.usercopy,
usercopy -> 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));
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.
> +
> + 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_TIME
Re: [Y2038] [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 11/26/19 4:17 PM, Arnd Bergmann wrote:
> On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil wrote:
>>
>> Then use that in the struct v4l2_buffer definition:
>>
>> struct v4l2_buffer {
>> ...
>> #ifdef __KERNEL__
>> struct __kernel_v4l2_timeval timestamp;
>> #else
>> struct timeval timestamp;
>> #endif
>>
>> That keeps struct v4l2_buffer fairly clean. And it also makes it
>> possible to have a bit more extensive documentation for the
>> struct __kernel_v4l2_timeval without polluting the actual struct
>> v4l2_buffer definition.
>
> Yes, good idea. I've added this version now:
>
> #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
>
> I briefly considered using #else #define __kernel_v4l2_timeval timeval
> to avoid the second #ifdef, but went back to your version again
> for clarify.
>
>> The videodev2.h header is something users of the API look at a
>> lot and having this really ugly kernel timestamp in there is
>> not acceptably IMHO. But splitting it off should work.
>
> Do you also mean moving it into a separate header file, or
> just outside of struct v4l2_buffer? Since it's hidden in #ifdef
> __KERNEL__, it could be moved to media/ioctl.h or elsewhere.
I've thought about that, but that risks having to change drivers
since they would now have to include another header to get the
right timeval definition. In the end I don't think it is worth the
effort.
I think it is best to define __kernel_v4l2_timeval just before
the struct v4l2_requestbuffers definition rather than before the
struct v4l2_buffer. That way it doesn't interfere with the
userspace structs for the buffer API.
Regards,
Hans
>
> Arnd
>
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
On 11/26/19 3:43 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 3:40 PM Hans Verkuil wrote:
>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
>
>>> switch (cmd) {
>>> +#ifdef CONFIG_COMPAT_32BIT_TIME
>>> + case VIDIOC_DQEVENT_TIME32: {
>>> + struct v4l2_event_time32 ev32;
>>> + struct v4l2_event *ev = parg;
>>> +
>>> + memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));
>>> + ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
>>> + ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
>>> + memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct
>>> v4l2_event, id));
>>
>> This looks dangerous: due to 64-bit alignment requirements the
>> v4l2_event struct may end with a 4-byte hole at the end of the struct,
>> which you do not want to copy to ev32.
>>
>> I think it is safer to just copy id and reserved separately:
>>
>> ev32.id = ev->id;
>> memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));
>
> Actually I think it's that's also bad: The padding in *ev must already be
> cleared here (otherwise there is a leak of stack data in the kernel
> already), so *not* copying the padding requires at least adding a memset
> upfront.
>
> I would do the per-member copy like I did for v4l2_buffer in my
> other reply:
>
> struct v4l2_event *ev = parg;
> struct v4l2_event_time32 ev32 = {
> .type = ev->type,
> .pending= ev->pending,
> .sequence = ev->sequence,
> .timestamp.tv_sec = ev->timestamp.tv_sec,
> .timestamp.tv_nsec = ev->timestamp.tv_nsec,
> .id = ev->id,
> };
>
> memcpy(ev32.u, ev->u, sizeof(ev->u));
> memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));
>
> if (copy_to_user(arg, &ev32, sizeof(ev32)))
> return -EFAULT;
>
> Unfortunately this is a little uglier because it still requires the two
> memcpy() for the arrays, but I think it's good enough.
I agree.
Hmm, can't you do .u = ev->u ? Or is that not allowed by this syntax?
>
> Any other ideas? Let me know if I should do a memset()
> plus individual member copy instead.
I think we have to, unless you have a better solution. This is leaking
information from the holes in the struct.
>>> + if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
>>> + return -ENOIOCTLCMD;
>>> +
>>> + rval = v4l2_event_dequeue(vfh, &ev, file->f_flags &
>>> O_NONBLOCK);
>>> +
>>> + memcpy(ev32, &ev, offsetof(struct v4l2_event, timestamp));
>>> + ev32->timestamp.tv_sec = ev.timestamp.tv_sec;
>>> + ev32->timestamp.tv_nsec = ev.timestamp.tv_nsec;
>>> + memcpy(&ev32->id, &ev.id,
>>> +sizeof(ev) - offsetof(struct v4l2_event, id));
>>
>> Ditto.
>
> Using the corresponding code here as well.
>
>>> +
>>> #define V4L2_EVENT_SUB_FL_SEND_INITIAL (1 << 0)
>>> #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK (1 << 1)
>>>
>>> @@ -2486,6 +2515,7 @@ struct v4l2_create_buffers {
>>> #define VIDIOC_S_DV_TIMINGS _IOWR('V', 87, struct v4l2_dv_timings)
>>> #define VIDIOC_G_DV_TIMINGS _IOWR('V', 88, struct v4l2_dv_timings)
>>> #define VIDIOC_DQEVENT _IOR('V', 89, struct v4l2_event)
>>> +#define VIDIOC_DQEVENT_TIME32_IOR('V', 89, struct
>>> v4l2_event_time32)
>>
>> Shouldn't this be under #ifdef __KERNEL__?
>>
>> And should this be in the public header at all? media/v4l2-ioctl.h might be
>> a better
>> place.
>
> Done.
>
>Arnd
>
Regards,
Hans
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 11/26/19 2:50 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil wrote:
>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
>
>>> 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;
>>> +
>>> + memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
>>> + vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
>>> + vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
>>> + memcpy(&vb->timecode, &vb32.timecode,
>>> +sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>>
>> I have similar concerns as with dqevent about whether this memcpy is the
>> right approach.
>> Unless you can prove with a utility like pahole that this memcpy is safe.
>
> This is the video_get_user() function, so the input data comes from user
> space and gets copied into the kernel, which has to check each field for
> validity already, so I think this is safe regardless of the padding (which
> exists before the 64-bit timestamp on 32-bit architectures). The fields
> match because the definition of all members other than the timeval is
> the same.
>
> On the other hand, I agree it's not obvious from the code why this
> is correct. I've changed my copy to this version below now, do you like
> that better?
>
> 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.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));
>
> This way, all padding is zeroed out, and it's obvious to human
> readers that each field gets set in the correct location.
>
>>> + memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
>>> + vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
>>> + vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
>>> + memcpy(&vb32.timecode, &vb->timecode,
>>> +sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>>
>> Ditto.
>
> This is my new version:
>
> 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->usercopy,
> .length = vb->length,
> .request_fd = vb->request_fd,
> };
That looks clean.
>
> if (copy_to_user(arg, &vb32, sizeof(vb32)))
> return -EFAULT;
>
>>> __u32 field;
>>> +#ifdef __KERNEL__
>>> + /* match glibc timeva
Re: [Y2038] [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
On 11/26/19 12:34 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil 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
>
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v4 0/8] y2038 safety in v4l2
Hi Arnd, On 11/11/19 9:38 PM, Arnd Bergmann wrote: > I'm in the process of finishing up the last bits on y2038-unsafe > code in the kernel, this series is for v4l2, which has no problem > with overflow, but has multiple ioctls that break with user space > built against a new 32-bit libc. Thank you for working on this. Much appreciated! I've reviewed this v4 series and found a few issues (mostly things that ended up in videodev2.h that shouldn't be there). Once a v5 is posted I'll try to test this more. Is there an easy(-ish) way to test this with a time64 ABI? Do you have such an environment set up for testing? > > I posted similar patches as part of a series back in 2015, the > new version was rewritten from scratch and I double-checked with > the old version to make sure I did not miss anything I had already > taken care of before. > > Hans Verkuil worked on a different patch set in 2017, but this > also did not get to the point of being merged. > > My new version contains compat-ioctl support, which the old one > did not and should be complete, but given its size likely contains > bugs. I did randconfig build tests, but no runtime test, so > careful review as well as testing would be much appreciated. > > With this version, the newly added code takes care of the existing > ABI, while the existing code got moved to the 64-bit time_t > interface and is used internally. This means that testing with > existing binaries should exercise most of the modifications > and if that works and doesn't get shot down in review, we can > probably live without testing the new ABI explicitly. > > I'm not entirely happy with the compat-ioctl implementation that > adds quite a bit of code duplication, but I hope this is > acceptable anyway, as a better implementation would likely > require a larger refactoring of the compat-ioctl file, while > my approach simply adds support for the additional data structure > variants. A cleanup is indeed much more work. This y2038 code is awful, but that's really because the original structs are aligned in the worst possible way. Regards, Hans > > This is a minor update compared to version 3 of this series, > with bugfixes for small mistakes that I found or that were > reported by automated build bots. I updated the tree at [2] > to this version now. > > Arnd > > [1] https://lwn.net/Articles/657754/ > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-v4l2 > > Arnd Bergmann (8): > media: documentation: fix video_event description > media: v4l2: abstract timeval handling in v4l2_buffer > media: v4l2-core: compat: ignore native command codes > media: v4l2-core: split out data copy from video_usercopy > media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI > media: v4l2-core: fix v4l2_buffer handling for time64 ABI > media: v4l2-core: fix compat VIDIOC_DQEVENT for time64 ABI > media: v4l2-core: fix compat v4l2_buffer handling for time64 ABI > > .../media/uapi/dvb/video-get-event.rst| 2 +- > Documentation/media/uapi/dvb/video_types.rst | 2 +- > .../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/v4l2-compat-ioctl32.c | 470 +++--- > drivers/media/v4l2-core/v4l2-event.c | 5 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 188 +-- > drivers/media/v4l2-core/v4l2-subdev.c | 20 +- > drivers/media/v4l2-core/videobuf-core.c | 4 +- > include/linux/videodev2.h | 17 +- > include/trace/events/v4l2.h | 2 +- > include/uapi/linux/videodev2.h| 77 +++ > 15 files changed, 669 insertions(+), 136 deletions(-) > ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
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. > > Signed-off-by: Arnd Bergmann > --- > 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(-) > > 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/st
Re: [Y2038] [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> 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
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 57 ++--
> include/uapi/linux/videodev2.h | 45 ++
> 2 files changed, 98 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 1de939d11628..4ae1bcaec3fa 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,
> @@ -3014,6 +3014,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
> }
>
> @@ -3032,6 +3040,30 @@ 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;
> +
> + memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
> + vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
> + vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
> + memcpy(&vb->timecode, &vb32.timecode,
> +sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
I have similar concerns as with dqevent about whether this memcpy is the right
approach.
Unless you can prove with a utility like pahole that this memcpy is safe.
> +
> + 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,
> @@ -3080,6 +3112,23 @@ 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_time32 vb32;
> + struct v4l2_buffer *vb = parg;
> +
> + memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
> + vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
> + vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
> + memcpy(&vb32.timecode, &vb->timecode,
> +sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
Ditto.
> +
> + if (copy_to_user(arg, &vb32,
Re: [Y2038] [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> The v4l2_event structure contains a 'struct timespec' member that is
> defined by the user space C library, creating an ABI incompatibility
> when that gets updated to a 64-bit time_t.
>
> While passing a 32-bit time_t here would be sufficient for CLOCK_MONOTONIC
> timestamps, simply redefining the structure to use the kernel's
> __kernel_old_timespec would not work for any library that uses a copy
> of the linux/videodev2.h header file rather than including the copy from
> the latest kernel headers.
>
> This means the kernel has to be changed to handle both versions of the
> structure layout on a 32-bit architecture. The easiest way to do this
> is during the copy from/to user space.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/media/v4l2-core/v4l2-event.c | 5 -
> drivers/media/v4l2-core/v4l2-ioctl.c | 24 -
> drivers/media/v4l2-core/v4l2-subdev.c | 20 +-
> include/uapi/linux/videodev2.h| 30 +++
> 4 files changed, 76 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index 9d673d113d7a..290c6b213179 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -27,6 +27,7 @@ static unsigned sev_pos(const struct v4l2_subscribed_event
> *sev, unsigned idx)
> static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> {
> struct v4l2_kevent *kev;
> + struct timespec64 ts;
> unsigned long flags;
>
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -44,7 +45,9 @@ static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct
> v4l2_event *event)
>
> kev->event.pending = fh->navailable;
> *event = kev->event;
> - event->timestamp = ns_to_timespec(kev->ts);
> + ts = ns_to_timespec64(kev->ts);
> + event->timestamp.tv_sec = ts.tv_sec;
> + event->timestamp.tv_nsec = ts.tv_nsec;
> kev->sev->first = sev_pos(kev->sev, 1);
> kev->sev->in_use--;
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 693f9eb8e01b..1de939d11628 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -821,7 +821,7 @@ static void v4l_print_event(const void *arg, bool
> write_only)
> const struct v4l2_event *p = arg;
> const struct v4l2_event_ctrl *c;
>
> - pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u,
> timestamp=%lu.%9.9lu\n",
> + pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u,
> timestamp=%llu.%9.9llu\n",
> p->type, p->pending, p->sequence, p->id,
> p->timestamp.tv_sec, p->timestamp.tv_nsec);
> switch (p->type) {
> @@ -3010,6 +3010,13 @@ static int check_array_args(unsigned int cmd, void
> *parg, size_t *array_size,
>
> static unsigned int video_translate_cmd(unsigned int cmd)
> {
> + switch (cmd) {
> +#ifdef CONFIG_COMPAT_32BIT_TIME
> + case VIDIOC_DQEVENT_TIME32:
> + return VIDIOC_DQEVENT;
> +#endif
> + }
> +
> return cmd;
> }
>
> @@ -3059,6 +3066,21 @@ static int video_put_user(void __user *arg, void
> *parg, unsigned int cmd)
> return 0;
>
> switch (cmd) {
> +#ifdef CONFIG_COMPAT_32BIT_TIME
> + case VIDIOC_DQEVENT_TIME32: {
> + struct v4l2_event_time32 ev32;
> + struct v4l2_event *ev = parg;
> +
> + memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));
> + ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
> + ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
> + memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct
> v4l2_event, id));
This looks dangerous: due to 64-bit alignment requirements the
v4l2_event struct may end with a 4-byte hole at the end of the struct,
which you do not want to copy to ev32.
I think it is safer to just copy id and reserved separately:
ev32.id = ev->id;
memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));
> +
> + if (copy_to_user(arg, &ev32, sizeof(ev32)))
> + return -EFAULT;
> + break;
> + }
> +#endif
> default:
> /* Copy results into user buffer */
> if (copy_to_user(arg, parg, _IOC_SIZE(cmd)))
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> b/drivers/media/v4l2-core/v4l2-subdev.c
> index f725cd9b66b9..45454a628e45 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -331,8 +331,8 @@ static long subdev_do_ioctl(struct file *file, unsigned
> int cmd, void *arg)
> struct v4l2_fh *vfh = file->private_data;
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
> - int rval;
> #endif
> + int rval;
>
>
Re: [Y2038] [PATCH 3/5] media: cec: move compat_ioctl handling to cec-api.c
On 08/27/2018 09:56 PM, Arnd Bergmann wrote:
> All the CEC ioctls are compatible, and they are only implemented
> in one driver, so we can simply let this driver handle them
> natively.
>
> Signed-off-by: Arnd Bergmann
Reviewed-by: Hans Verkuil
Mauro, feel free to pick this up when you process the others in this
patch series.
Regards,
Hans
> ---
> drivers/media/cec/cec-api.c | 1 +
> fs/compat_ioctl.c | 12
> 2 files changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/media/cec/cec-api.c b/drivers/media/cec/cec-api.c
> index b6536bbad530..9067728feb60 100644
> --- a/drivers/media/cec/cec-api.c
> +++ b/drivers/media/cec/cec-api.c
> @@ -665,6 +665,7 @@ const struct file_operations cec_devnode_fops = {
> .owner = THIS_MODULE,
> .open = cec_open,
> .unlocked_ioctl = cec_ioctl,
> + .compat_ioctl = cec_ioctl,
> .release = cec_release,
> .poll = cec_poll,
> .llseek = no_llseek,
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index e38e6c785459..33f48933a865 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -1195,18 +1195,6 @@ COMPATIBLE_IOCTL(VIDEO_CLEAR_BUFFER)
> COMPATIBLE_IOCTL(VIDEO_SET_STREAMTYPE)
> COMPATIBLE_IOCTL(VIDEO_SET_FORMAT)
> COMPATIBLE_IOCTL(VIDEO_GET_SIZE)
> -/* cec */
> -COMPATIBLE_IOCTL(CEC_ADAP_G_CAPS)
> -COMPATIBLE_IOCTL(CEC_ADAP_G_LOG_ADDRS)
> -COMPATIBLE_IOCTL(CEC_ADAP_S_LOG_ADDRS)
> -COMPATIBLE_IOCTL(CEC_ADAP_G_PHYS_ADDR)
> -COMPATIBLE_IOCTL(CEC_ADAP_S_PHYS_ADDR)
> -COMPATIBLE_IOCTL(CEC_G_MODE)
> -COMPATIBLE_IOCTL(CEC_S_MODE)
> -COMPATIBLE_IOCTL(CEC_TRANSMIT)
> -COMPATIBLE_IOCTL(CEC_RECEIVE)
> -COMPATIBLE_IOCTL(CEC_DQEVENT)
> -
> /* joystick */
> COMPATIBLE_IOCTL(JSIOCGVERSION)
> COMPATIBLE_IOCTL(JSIOCGAXES)
>
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation
Hi Arnd,
On 11/27/2017 02:19 PM, Arnd Bergmann wrote:
> timespec is generally deprecated because of the y2038 overflow.
> In vivid, the usage is fine, since we are dealing with monotonic
> timestamps, but we can also simplify the code by going to ktime_t.
>
> Using ktime_divns() should be roughly as efficient as the old code,
> since the constant 64-bit division gets turned into a multiplication
> on modern platforms, and we save multiple 32-bit divisions that can be
> expensive e.g. on ARMv7.
>
> Signed-off-by: Arnd Bergmann
> ---
> I hope I understood the VIVID_RDS_GEN_BLOCKS calculation right,
> please review carefully.
> ---
> drivers/media/platform/vivid/vivid-core.c | 2 +-
> drivers/media/platform/vivid/vivid-core.h | 2 +-
> drivers/media/platform/vivid/vivid-radio-rx.c | 11 +--
> drivers/media/platform/vivid/vivid-radio-tx.c | 8 +++-
> drivers/media/platform/vivid/vivid-rds-gen.h | 1 +
> 5 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-core.c
> b/drivers/media/platform/vivid/vivid-core.c
> index 5f316a5e38db..a091cfd93164 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -995,7 +995,7 @@ static int vivid_create_instance(struct platform_device
> *pdev, int inst)
>
> dev->edid_max_blocks = dev->edid_blocks = 2;
> memcpy(dev->edid, vivid_hdmi_edid, sizeof(vivid_hdmi_edid));
> - ktime_get_ts(&dev->radio_rds_init_ts);
> + dev->radio_rds_init_time = ktime_get();
>
> /* create all controls */
> ret = vivid_create_controls(dev, ccs_cap == -1, ccs_out == -1,
> no_error_inj,
> diff --git a/drivers/media/platform/vivid/vivid-core.h
> b/drivers/media/platform/vivid/vivid-core.h
> index 5cdf95bdc4d1..d8bff4dcefb7 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -510,7 +510,7 @@ struct vivid_dev {
>
> /* Shared between radio receiver and transmitter */
> boolradio_rds_loop;
> - struct timespec radio_rds_init_ts;
> + ktime_t radio_rds_init_time;
>
> /* CEC */
> struct cec_adapter *cec_rx_adap;
> diff --git a/drivers/media/platform/vivid/vivid-radio-rx.c
> b/drivers/media/platform/vivid/vivid-radio-rx.c
> index 47c36c26096b..1b96cbd7f2ea 100644
> --- a/drivers/media/platform/vivid/vivid-radio-rx.c
> +++ b/drivers/media/platform/vivid/vivid-radio-rx.c
> @@ -38,9 +38,9 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user
> *buf,
>size_t size, loff_t *offset)
> {
> struct vivid_dev *dev = video_drvdata(file);
> - struct timespec ts;
> struct v4l2_rds_data *data = dev->rds_gen.data;
> bool use_alternates;
> + ktime_t timestamp;
> unsigned blk;
> int perc;
> int i;
> @@ -64,17 +64,16 @@ ssize_t vivid_radio_rx_read(struct file *file, char
> __user *buf,
> }
>
> retry:
> - ktime_get_ts(&ts);
> - use_alternates = ts.tv_sec % 10 >= 5;
> + timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
> + blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
> + use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
> +
Almost right. This last line should be:
use_alternates = (blk / VIVID_RDS_GEN_BLOCKS) & 1;
With that in place it works and you can add my:
Tested-by: Hans Verkuil
to this patch.
Regards,
Hans
> if (dev->radio_rx_rds_last_block == 0 ||
> dev->radio_rx_rds_use_alternates != use_alternates) {
> dev->radio_rx_rds_use_alternates = use_alternates;
> /* Re-init the RDS generator */
> vivid_radio_rds_init(dev);
> }
> - ts = timespec_sub(ts, dev->radio_rds_init_ts);
> - blk = ts.tv_sec * 100 + ts.tv_nsec / 1000;
> - blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
> if (blk >= dev->radio_rx_rds_last_block + VIVID_RDS_GEN_BLOCKS)
> dev->radio_rx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
>
> diff --git a/drivers/media/platform/vivid/vivid-radio-tx.c
> b/drivers/media/platform/vivid/vivid-radio-tx.c
> index 0e8025b7b4dd..897b56195ca7 100644
> --- a/drivers/media/platform/vivid/vivid-radio-tx.c
> +++ b/drivers/media/platform/vivid/vivid-radio-tx.c
> @@ -37,7 +37,7 @@ ssize_t vivid_radio_tx_write(struct file *file, const char
> __user *buf,
> {
> struct vivid_dev *dev = video_drvdata(file);
> struct v4l2_rds_data *data = dev->rds_gen.data;
> - struct tim
Re: [Y2038] Which type to use for timestamps: u64 or s64?
On 11/05/15 09:36, Arnd Bergmann wrote: > On Thursday 05 November 2015 08:41:11 Hans Verkuil wrote: >> Hi Arnd, >> >> We're redesigning the timestamp handling in the video4linux subsystem moving >> away >> from struct timeval to a single timestamp in ns (what ktime_get_ns() gives >> us). >> But I was wondering: ktime_get_ns() gives a s64, so should we use s64 as >> well as >> the timestamp type we'll eventually be returning to the user, or should we >> use u64? >> >> The current patch series we made uses a u64, but I am now beginning to doubt >> that >> decision. > > I would lean towards u64, but I don't think it really matters either way, > especially since all the drivers should be using monotonic timestamps now. One thing that might be easier if it is a s64 is when adding/subtracting offsets from the timestamp. And the other reason is that a u64 gives a false view of the underlying type. With a s64 it is clear that a timestamp will wrap around after 292 years instead of double that. Admittedly, not our problem, but if we ever send a space probe to Alpha Centauri, then it might be nice to know as application developer that you need to take special measures if the mission takes longer than 292 years :-) Regards, Hans ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
[Y2038] Which type to use for timestamps: u64 or s64?
Hi Arnd, We're redesigning the timestamp handling in the video4linux subsystem moving away from struct timeval to a single timestamp in ns (what ktime_get_ns() gives us). But I was wondering: ktime_get_ns() gives a s64, so should we use s64 as well as the timestamp type we'll eventually be returning to the user, or should we use u64? The current patch series we made uses a u64, but I am now beginning to doubt that decision. Regards, Hans ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 7/9] [media] v4l2: introduce v4l2_timeval
On 09/18/15 12:08, Arnd Bergmann wrote: > On Friday 18 September 2015 11:52:28 Hans Verkuil wrote: >> On 09/18/15 11:43, Arnd Bergmann wrote: >>> On Friday 18 September 2015 11:27:40 Hans Verkuil wrote: >>>> Ah, OK. Got it. >>>> >>>> I think this is dependent on the upcoming media workshop next month. If we >>>> decide to redesign v4l2_buffer anyway, then we can avoid timeval >>>> completely. >>>> And the only place where we would need to convert it in the compat code >>>> hidden in the v4l2 core (likely v4l2-ioctl.c). >>> >>> Ah, I think I understood the idea now, I missed that earlier when you >>> mention >>> the idea. >>> >>> So what you are saying here is that you could come up with a new unambiguous >>> (using only __u32 and __u64 types and no pointers) format that gets exposed >>> to a new set of ioctls, and then change the handling of the existing three >>> formats (native 64-bit, traditional 32-bit, and 32-bit with 64-bit time_t) >>> so they get converted into the new format by the common ioctl handling code? >> >> Right. Drivers only see the new struct, and only v4l2-ioctl.c (and possible >> v4l2-compat-ioctl32.c) see the old ones. >> >> BTW, I will probably pick up patches 4 and 6 for 4.4. That should help a bit. > > Ok, thanks! > > I guess it's up to Mauro to pick up the first three patches? Yes. > As I don't see anything more to do for me here until you've had the > discussion about the new format, I'll move on to another subsystem now. > I have around 70 patches waiting to be submitted, plus the system call > series. Agreed. Thanks again for working on this! Regards, Hans ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 7/9] [media] v4l2: introduce v4l2_timeval
On 09/18/15 11:43, Arnd Bergmann wrote: > On Friday 18 September 2015 11:27:40 Hans Verkuil wrote: >> Ah, OK. Got it. >> >> I think this is dependent on the upcoming media workshop next month. If we >> decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. >> And the only place where we would need to convert it in the compat code >> hidden in the v4l2 core (likely v4l2-ioctl.c). > > Ah, I think I understood the idea now, I missed that earlier when you mention > the idea. > > So what you are saying here is that you could come up with a new unambiguous > (using only __u32 and __u64 types and no pointers) format that gets exposed > to a new set of ioctls, and then change the handling of the existing three > formats (native 64-bit, traditional 32-bit, and 32-bit with 64-bit time_t) > so they get converted into the new format by the common ioctl handling code? Right. Drivers only see the new struct, and only v4l2-ioctl.c (and possible v4l2-compat-ioctl32.c) see the old ones. BTW, I will probably pick up patches 4 and 6 for 4.4. That should help a bit. Regards, Hans >> I am not really keen on having v4l2_timeval in all these drivers. I would >> have to check them anyway since I suspect that in several drivers the local >> timeval variable can be avoided by rewriting that part of the driver. > > I've tried to do that for all the drivers where I could find an easy solution > in patch 6/9, but I'm sure you can do it for a couple more. > > We could also do a lightweight redesign and use 'timespec64' internally > in all the drivers and then convert that to 'timeval' or the 64-bit > format of that in the ioctl handler. This is also something I tried at > some point but then found it a bit more intuitive to leave the normal ioctl > path alone and have an explicit type. > >> Personally I am in favor of a redesigned v4l2_buffer: it's awkward to use >> with multiplanar formats, there is cruft in there that can be removed >> (timecode), >> and there is little space for additions (HW-specific timecodes, more buffer >> meta data, etc). >> >> We'll see. > > Ok. > > Arnd > ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 8/9] [media] handle 64-bit time_t in v4l2_buffer
On 09/18/15 11:26, Arnd Bergmann wrote:
> On Friday 18 September 2015 09:18:45 Hans Verkuil wrote:
>> Hi Arnd,
>>
>> Thanks once again for working on this! Unfortunately, this approach won't
>> work, see my comments below.
>>
>> BTW, I would expect to see compile errors when compiling for 32 bit. Did
>> you try that?
>
> I only tested on 32-bit, both ARM and x86, but had a longer set of patches
> applied below.
>
>>> +static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to,
>>> + const struct v4l2_buffer *from)
>>> +{
>>> + to->index = from->index;
>>> + to->type= from->type;
>>> + to->bytesused = from->bytesused;
>>> + to->flags = from->flags;
>>> + to->field = from->field;
>>> + to->timestamp.tv_sec= from->timestamp.tv_sec;
>>> + to->timestamp.tv_usec = from->timestamp.tv_usec;
>>> + to->timecode= from->timecode;
>>> + to->sequence= from->sequence;
>>> + to->memory = from->memory;
>>> + to->m.offset= from->m.offset;
>>> + to->length = from->length;
>>> + to->reserved2 = from->reserved2;
>>> + to->reserved= from->reserved;
>>> +}
>>
>> Is there a reason why you didn't use memcpy like you did for VIDIOC_DQEVENT
>> (path 5/9)?
>> I would prefer that over these explicit assignments.
>
> No strong reason. I went back and forth a bit on the m.offset field that
> is part of a union: In a previous version, I planned to move all the
> compat handling here, and doing the conversion one field at a time would
> make it easier to share the code between 32-bit and 64-bit kernels
> handling old 32-bit user space. This version doesn't do that, so I can
> use the memcpy approach instead.
>
>>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>>> struct file *file, void *fh, void *arg)
>>> {
>>> @@ -2408,12 +2524,21 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>> IOCTL_INFO_FNC(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO),
>>> IOCTL_INFO_FNC(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers,
>>> INFO_FL_PRIO | INFO_FL_QUEUE),
>>> IOCTL_INFO_FNC(VIDIOC_QUERYBUF, v4l_querybuf, v4l_print_buffer,
>>> INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)),
>>> +#ifndef CONFIG_64BIT
>>> + IOCTL_INFO_FNC(VIDIOC_QUERYBUF_TIME32, v4l_querybuf_time32,
>>> v4l_print_buffer_time32, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer,
>>> length)),
>>> +#endif
>>
>> This doesn't work. These IOCTL macros use the ioctl nr as the index in the
>> array. Since
>> VIDIOC_QUERYBUF and VIDIOC_QUERYBUF_TIME32 have the same index, this will
>> fail.
>
> Ah, I see. No idea why that did not cause a compile-time error. I got some
> errors for duplicate 'case' values when the structures are the same size
> (that's why we need the '#ifdef CONFIG_64BIT' in some places) but not here.
>
>> I think (not 100% certain, there may be better suggestions) that the
>> conversion is best
>> done in video_usercopy(): just before func() is called have a switch for the
>> TIME32
>> variants, convert to the regular variant, call func() and convert back.
>
> Does the handler have access to the _IOC_SIZE() value that was passed? If
> it does, we could add a conditional inside of v4l_querybuf().
> I did not see an easy way to do that though.
No, the v4l_ handlers don't have access to that. But I think it is much easier
to
do the conversion at the first opportunity (video_usercopy), so no other code
needs to be modified. Easier to review and maintain that way.
>> My only concern here is that an additional v4l2_buffer struct (68 bytes) is
>> needed on the
>> stack. I don't see any way around that, though.
>
> Agreed.
>
>>> +struct v4l2_buffer_time32 {
>>> + __u32 index;
>>> + __u32 type;
>>> + __u32 bytesused;
>>> + __u32 flags;
>>> + __u32 field;
>>> + struct {
>>> + __s32 tv_sec;
>>> + __s32 tv_usec;
>>> + } timestamp;
>>> + struct v4l2_timecodetimecode;
>>
Re: [Y2038] [PATCH v2 7/9] [media] v4l2: introduce v4l2_timeval
On 09/18/15 11:09, Arnd Bergmann wrote: > On Friday 18 September 2015 10:05:06 Hans Verkuil wrote: >> On 09/17/15 23:19, Arnd Bergmann wrote: >>> The v4l2 API uses a 'struct timeval' to communicate time stamps to user >>> space. This is broken on 32-bit architectures as soon as we have a C library >>> that defines time_t as 64 bit, which then changes the structure layout of >>> struct v4l2_buffer. >>> >>> Since existing user space source code relies on the type to be 'struct >>> timeva' and we want to preserve compile-time compatibility when moving >> >> s/timeva/timeval/ > > Fixed > >>> to a new libc, we cannot make user-visible changes to the header file. >>> >>> In this patch, we change the type of the timestamp to 'struct v4l2_timeval' >> >> Don't we need a kernel-wide timeval64? Rather than adding a v4l2-specific >> struct? > > I still hope to avoid doing that. All in-kernel users should be changed to > use timespec64 or ktime_t, which are always more efficient and accurate. > > For the system call interface, all timeval APIs are deprecated and have > replacements using timespec64 (e.g. clock_gettime() replaces gettimeofday). > > Only a handful of ioctls pass timeval, and so far my impression is that > we are better off handling each one separately. The total amount of code > we need to add this way should be less than if we have to duplicate all > common code functions that today operate on timeval and can eventually > get removed. Ah, OK. Got it. I think this is dependent on the upcoming media workshop next month. If we decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. And the only place where we would need to convert it in the compat code hidden in the v4l2 core (likely v4l2-ioctl.c). I am not really keen on having v4l2_timeval in all these drivers. I would have to check them anyway since I suspect that in several drivers the local timeval variable can be avoided by rewriting that part of the driver. Personally I am in favor of a redesigned v4l2_buffer: it's awkward to use with multiplanar formats, there is cruft in there that can be removed (timecode), and there is little space for additions (HW-specific timecodes, more buffer meta data, etc). We'll see. Regards, Hans > >>> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c >>> index 295fde5fdb75..df5daac6d099 100644 >>> --- a/drivers/media/platform/vim2m.c >>> +++ b/drivers/media/platform/vim2m.c >>> @@ -235,7 +235,7 @@ static int device_process(struct vim2m_ctx *ctx, >>> in_vb->v4l2_buf.sequence = q_data->sequence++; >>> memcpy(&out_vb->v4l2_buf.timestamp, >>> &in_vb->v4l2_buf.timestamp, >>> - sizeof(struct timeval)); >>> + sizeof(struct v4l2_timeval)); >>> if (in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TIMECODE) >>> memcpy(&out_vb->v4l2_buf.timecode, &in_vb->v4l2_buf.timecode, >>> sizeof(struct v4l2_timecode)); >> >> See https://patchwork.linuxtv.org/patch/31405/ >> >> I'll merge that one for 4.4 very soon. > > Ok. > > Arnd > ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 7/9] [media] v4l2: introduce v4l2_timeval
On 09/17/15 23:19, Arnd Bergmann wrote:
> The v4l2 API uses a 'struct timeval' to communicate time stamps to user
> space. This is broken on 32-bit architectures as soon as we have a C library
> that defines time_t as 64 bit, which then changes the structure layout of
> struct v4l2_buffer.
>
> Since existing user space source code relies on the type to be 'struct
> timeva' and we want to preserve compile-time compatibility when moving
s/timeva/timeval/
> to a new libc, we cannot make user-visible changes to the header file.
>
> In this patch, we change the type of the timestamp to 'struct v4l2_timeval'
Don't we need a kernel-wide timeval64? Rather than adding a v4l2-specific
struct?
> as a preparation for a follow-up that adds API compatiblity for both
> 32-bit and 64-bit time_t. This patch should have no impact on generated
> code in either user space or kernel.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/media/pci/bt8xx/bttv-driver.c | 2 +-
> drivers/media/pci/meye/meye.h | 2 +-
> drivers/media/pci/zoran/zoran.h| 2 +-
> drivers/media/platform/coda/coda.h | 2 +-
> drivers/media/platform/omap/omap_vout.c| 4 ++--
> drivers/media/platform/omap3isp/ispstat.c | 3 ++-
> drivers/media/platform/omap3isp/ispstat.h | 2 +-
> drivers/media/platform/vim2m.c | 2 +-
> drivers/media/platform/vivid/vivid-ctrls.c | 2 +-
> drivers/media/usb/cpia2/cpia2.h| 2 +-
> drivers/media/usb/cpia2/cpia2_v4l.c| 2 +-
> drivers/media/usb/gspca/gspca.c| 2 +-
> drivers/media/usb/usbvision/usbvision.h| 2 +-
> drivers/media/v4l2-core/v4l2-common.c | 6 +++---
> include/media/v4l2-common.h| 2 +-
> include/media/videobuf-core.h | 2 +-
> include/trace/events/v4l2.h| 12 ++--
> include/uapi/linux/videodev2.h | 17 +
> 18 files changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c
> b/drivers/media/pci/bt8xx/bttv-driver.c
> index 15a4ebc2844d..b0ed8d799c14 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -3585,7 +3585,7 @@ static void
> bttv_irq_wakeup_video(struct bttv *btv, struct bttv_buffer_set *wakeup,
> struct bttv_buffer_set *curr, unsigned int state)
> {
> - struct timeval ts;
> + struct v4l2_timeval ts;
>
> v4l2_get_timestamp(&ts);
>
> diff --git a/drivers/media/pci/meye/meye.h b/drivers/media/pci/meye/meye.h
> index 751be5e533c7..a06aa5ba9abc 100644
> --- a/drivers/media/pci/meye/meye.h
> +++ b/drivers/media/pci/meye/meye.h
> @@ -281,7 +281,7 @@
> struct meye_grab_buffer {
> int state; /* state of buffer */
> unsigned long size; /* size of jpg frame */
> - struct timeval timestamp; /* timestamp */
> + struct v4l2_timeval timestamp; /* timestamp */
> unsigned long sequence; /* sequence number */
> };
>
> diff --git a/drivers/media/pci/zoran/zoran.h b/drivers/media/pci/zoran/zoran.h
> index 4e7db8939c2b..9a9f782cede9 100644
> --- a/drivers/media/pci/zoran/zoran.h
> +++ b/drivers/media/pci/zoran/zoran.h
> @@ -39,7 +39,7 @@ struct zoran_sync {
> unsigned long frame;/* number of buffer that has been free'd */
> unsigned long length; /* number of code bytes in buffer (capture
> only) */
> unsigned long seq; /* frame sequence number */
> - struct timeval timestamp; /* timestamp */
> + struct v4l2_timeval timestamp; /* timestamp */
> };
>
>
> diff --git a/drivers/media/platform/coda/coda.h
> b/drivers/media/platform/coda/coda.h
> index 59b2af9c7749..fa1e15a757cd 100644
> --- a/drivers/media/platform/coda/coda.h
> +++ b/drivers/media/platform/coda/coda.h
> @@ -138,7 +138,7 @@ struct coda_buffer_meta {
> struct list_headlist;
> u32 sequence;
> struct v4l2_timecodetimecode;
> - struct timeval timestamp;
> + struct v4l2_timeval timestamp;
> u32 start;
> u32 end;
> };
> diff --git a/drivers/media/platform/omap/omap_vout.c
> b/drivers/media/platform/omap/omap_vout.c
> index 70c28d19ea04..974a238a86fe 100644
> --- a/drivers/media/platform/omap/omap_vout.c
> +++ b/drivers/media/platform/omap/omap_vout.c
> @@ -513,7 +513,7 @@ static int omapvid_apply_changes(struct omap_vout_device
> *vout)
> }
>
> static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
> - unsigned int irqstatus, struct timeval timevalue)
> + unsigned int irqstatus, struct v4l2_timeval timevalue)
> {
> u32 fid;
>
> @@ -557,7 +557,7 @@ static void omap_vout_isr(void *arg, unsigned int
> irqstatus)
> int ret, fid, mgr_id;
> u32 addr, irq;
> struct omap_overlay *ovl;
> - struct timeval timevalue;
> +
Re: [Y2038] [PATCH v2 8/9] [media] handle 64-bit time_t in v4l2_buffer
Hi Arnd,
Thanks once again for working on this! Unfortunately, this approach won't
work, see my comments below.
BTW, I would expect to see compile errors when compiling for 32 bit. Did
you try that?
On 09/17/2015 11:19 PM, Arnd Bergmann wrote:
> This is the final change to enable user space with 64-bit time_t
> in the core v4l2 code.
>
> Four ioctls are affected here: VIDIOC_QUERYBUF, VIDIOC_QBUF,
> VIDIOC_DQBUF, and VIDIOC_PREPARE_BUF. All of them pass a
> struct v4l2_buffer, which can either contain a 32-bit time_t
> or a 64-bit time on 32-bit architectures.
>
> In this patch, we redefine the in-kernel version of this structure
> to use the 64-bit __s64 type through the use of v4l2_timeval.
> This means the normal ioctl handling now assumes 64-bit time_t
> and so we have to add support for 32-bit time_t in new handlers.
>
> This is somewhat similar to the 32-bit compat handling on 64-bit
> architectures, but those also have to modify other fields of
> the structure. For now, I leave that compat code alone, as it
> handles the normal case (32-bit compat_time_t) correctly, this
> has to be done as a follow-up.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 174
> +--
> include/uapi/linux/videodev2.h | 34 ++-
> 2 files changed, 199 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 7aab18dd2ca5..137475c28e8f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -438,15 +438,14 @@ static void v4l_print_buffer(const void *arg, bool
> write_only)
> const struct v4l2_timecode *tc = &p->timecode;
> const struct v4l2_plane *plane;
> int i;
> + /* y2038 safe because of monotonic time */
> + unsigned int seconds = p->timestamp.tv_sec;
>
> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, "
> + pr_cont("%02d:%02d:%02d.%08ld index=%d, type=%s, "
> "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),
> - (long)p->timestamp.tv_usec,
> - p->index,
> - prt_names(p->type, v4l2_type_names),
> + seconds / 3600, (seconds / 60) % 60,
> + seconds % 60, (long)p->timestamp.tv_usec,
> + p->index, prt_names(p->type, v4l2_type_names),
> p->flags, prt_names(p->field, v4l2_field_names),
> p->sequence, prt_names(p->memory, v4l2_memory_names));
>
> @@ -1846,6 +1845,123 @@ static int v4l_prepare_buf(const struct
> v4l2_ioctl_ops *ops,
> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
> }
>
> +#ifndef CONFIG_64BIT
> +static void v4l_get_buffer_time32(struct v4l2_buffer *to,
> + const struct v4l2_buffer_time32 *from)
> +{
> + to->index = from->index;
> + to->type= from->type;
> + to->bytesused = from->bytesused;
> + to->flags = from->flags;
> + to->field = from->field;
> + to->timestamp.tv_sec= from->timestamp.tv_sec;
> + to->timestamp.tv_usec = from->timestamp.tv_usec;
> + to->timecode= from->timecode;
> + to->sequence= from->sequence;
> + to->memory = from->memory;
> + to->m.offset= from->m.offset;
> + to->length = from->length;
> + to->reserved2 = from->reserved2;
> + to->reserved= from->reserved;
> +}
> +
> +static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to,
> + const struct v4l2_buffer *from)
> +{
> + to->index = from->index;
> + to->type= from->type;
> + to->bytesused = from->bytesused;
> + to->flags = from->flags;
> + to->field = from->field;
> + to->timestamp.tv_sec= from->timestamp.tv_sec;
> + to->timestamp.tv_usec = from->timestamp.tv_usec;
> + to->timecode= from->timecode;
> + to->sequence= from->sequence;
> + to->memory = from->memory;
> + to->m.offset= from->m.offset;
> + to->length = from->length;
> + to->reserved2 = from->reserved2;
> + to->reserved= from->reserved;
> +}
Is there a reason why you didn't use memcpy like you did for VIDIOC_DQEVENT
(path 5/9)?
I would prefer that over these explicit assignments.
> +
> +static int v4l_querybuf_time32(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct v4l2_buffer buffer;
> + int ret;
> +
> +
Re: [Y2038] [PATCH 6/7] [RFC] [media]: v4l2: introduce v4l2_timeval
On 09/16/2015 09:56 AM, Arnd Bergmann wrote:
> On Wednesday 16 September 2015 08:51:14 Hans Verkuil wrote:
>
>>> a) Similar to my first attempt, define a new struct v4l2_timeval, but
>>>only use it when building with a y2038-aware libc, so we don't break
>>>existing environments:
>>>
>>> /* some compile-time conditional that we first need to agree on with
>>> libc */
>>> #if __BITS_PER_TIME_T > __BITS_PER_LONG
>>> struct v4l2_timeval { long tv_sec; long tv_usec; }
>>> #else
>>> #define v4l2_timeval timeval
>>> #endif
>>>
>>>This means that any user space that currently assumes the timestamp
>>>member to be a 'struct timeval' has to be changed to access the members
>>>individually, or get a build error.
>>>The __BITS_PER_TIME_T trick has to be used in a couple of other
>>> subsystems
>>>too, as some of them have no other way to identify an interface
>>
>> I don't like this as this means some applications will compile on 64 bit or
>> with a non-y2038-aware libc, but fail on a 32-bit with y2038-aware libc. This
>> will be confusing and it may take a long time before the application
>> developer
>> discovers this.
>
> Right.
>
>>> b) Keep the header file unchanged, but deal with both formats of v4l2_buffer
>>>in the kernel. Fortunately, all ioctls that pass a v4l2_buffer have
>>>properly defined command codes, and it does not get passed using a
>>>read/write style interface. This means we move the v4l2_buffer32
>>>handling from v4l2-compat-ioctl32.c to v4l2-ioctl.c and add an in-kernel
>>>v4l2_buffer64 that matches the 64-bit variant of v4l2_buffer.
>>>This way, user space can use either definition of time_t, and the
>>>kernel will just handle them natively.
>>>This is going to be the most common way to handle y2038 compatibility
>>>in device drivers, and it has the additional advantage of simplifying
>>>the compat path.
>>
>> This would work.
>
> Ok. So the only downside I can think of for this is that it uses a slightly
> less efficient format with additional padding in it. The kernel side will
> be a little ugly as I'm trying to avoid defining a generic timeval64
> structure (the generic syscalls should not need one), but I'll try to
> implement it first to see how it ends up.
>
>>> c) As you describe above, introduce a new v4l2_buffer replacement with
>>>a different layout that does not reference timeval. For this case, I
>>>would recommend using a single 64-bit nanosecond timestamp that can
>>>be generated using ktime_get_ns().
>>>However, to avoid ambiguity with the user space definition of struct
>>>timeval, we still have to hide the existing 'struct v4l2_buffer' from
>>>y2038-aware user space by enclosing it in '#if __BITS_PER_TIME_T >
>>>__BITS_PER_LONG' or similar.
>>
>> Right, and if we do that we still have the problem I describe under a). So we
>> would need to implement b) regardless.
>>
>> In other words, choosing c) doesn't depend on y2038 and it should be decided
>> on its own merits.
>>
>> I've proposed this as a topic to the media workshop we'll have during the
>> Linux
>> Kernel Summit.
>
> Thanks, good idea. I'll be at the kernel summit, but don't plan to attend
> the media workshop otherwise. If you let me know about the schedule, I can
> come to this session (or ping me on IRC or hangout when it starts).
Are you also attending the ELCE in Dublin? We could have a quick talk there.
I think the discussion whether to switch to a new v4l2_buffer struct isn't
really
dependent on anything y2038.
Regards,
Hans
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 6/7] [RFC] [media]: v4l2: introduce v4l2_timeval
On 09/15/2015 10:26 PM, Arnd Bergmann wrote:
> On Tuesday 15 September 2015 18:27:19 Hans Verkuil wrote:
>> On 09/15/2015 05:49 PM, Arnd Bergmann wrote:
>>> The v4l2 API uses a 'struct timeval' to communicate time stamps to user
>>> space. This is broken on 32-bit architectures as soon as we have a C library
>>> that defines time_t as 64 bit, which then changes the structure layout of
>>> struct v4l2_buffer.
>>>
>>> Fortunately, almost all v4l2 drivers use monotonic timestamps and call
>>> v4l2_get_timestamp(), which means they don't also have a y2038 problem.
>>> This means we can keep using the existing binary layout of the structure
>>> and do not need to worry about defining a new kernel interface for
>>> userland with 64-bit time_t.
>>>
>>> A possible downside of this approach is that it breaks any user space
>>> that tries to assign the timeval structure returned from the kernel
>>> to another timeval, or to pass a pointer to it into a function that
>>> expects a timeval. Those will cause a build-time warning or error
>>> that can be fixed up in a backwards compatible way.
>>>
>>> The alternative to this patch is to leave the structure using
>>> 'struct timeval', but then we have to rework the kernel to let
>>> it handle both 32-bit and 64-bit time_t for 32-bit user space
>>> processes.
>>
>> Cool. Only this morning I was thinking about what would be needed in v4l2
>> to be y2038 safe, and here it is!
>
> Nice!
>
> fwiw, I also have a list of drivers at
> https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_0YiQwis/edit?usp=sharing
> which lists all known files that still need changing, in case you are
> wondering what else needs to be done, though it currently only covers
> things that nobody so far has started working on, and I have a couple
> patches on my disk that need polishing (I pushed out the v4l2 portion
> of that as a start)
I just *thought* about it, I never said I would do it! :-)
>
>>> @@ -839,7 +845,7 @@ struct v4l2_buffer {
>>> __u32 bytesused;
>>> __u32 flags;
>>> __u32 field;
>>> - struct timeval timestamp;
>>> + struct v4l2_timeval timestamp;
>>> struct v4l2_timecodetimecode;
>>> __u32 sequence;
>>>
>>>
>>
>> I suspect that quite a few apps use assign the timestamp to another timeval
>> struct. A quick grep in v4l-utils (which we maintain) shows at least two of
>> those assignments. Ditto for xawtv3.
>
> Ok, that is very helpful information, thanks for finding that!
>
>> So I don't think v4l2_timeval is an option as it would break userspace too
>> badly.
>
> Agreed, we definitely don't want to break building user space with
> existing environments, i.e. 64-bit architectures, or 32-bit architectures
> with 32-bit time_t.
>
>> An alternative to supporting a 64-bit timeval for 32-bit userspace is to
>> make a
>> new y2038-aware struct and a new set of ioctls and use this opportunity to
>> clean
>> up and extend the v4l2_buffer struct.
>>
>> So any 32-bit app that needs to be y2038 compliant would just use the new
>> struct and ioctls.
>>
>> But this is something to discuss among the v4l2 developers.
>
> Ok. We generally to require as few source level changes to user space
> as possible for the conversion, and we want to make sure that when
> using a 32-bit libc with 64-bit time_t, we don't accidentally get
> broken interfaces (i.e. we should get a compile error whenever we
> can't get it right automatically).
>
> One aspect that makes v4l2_buffer special is that the binary format
> is already clean for y2038 (once patch 4/7 "exynos4-is: use monotonic
> timestamps as advertized" gets merged), and we only need to worry about
> what happens when user space disagrees about the size of timeval.
>
> Let me describe the options that I can think of here:
>
> a) Similar to my first attempt, define a new struct v4l2_timeval, but
>only use it when building with a y2038-aware libc, so we don't break
>existing environments:
>
> /* some compile-time conditional that we first need to agree on with
> libc */
> #if __BITS_PER_TIME_T > __BITS_PER_LONG
> struct v4l2_timeval { long tv_sec; long tv_usec; }
> #else
> #define v4l2_timeval timeval
> #endif
>
>This means that any user space that currently ass
Re: [Y2038] [PATCH 7/7] [RFC] [media] introduce v4l2_timespec type for timestamps
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
> ---
> 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_timespectimestamp;
> __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
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 6/7] [RFC] [media]: v4l2: introduce v4l2_timeval
On 09/15/2015 05:49 PM, Arnd Bergmann wrote:
> The v4l2 API uses a 'struct timeval' to communicate time stamps to user
> space. This is broken on 32-bit architectures as soon as we have a C library
> that defines time_t as 64 bit, which then changes the structure layout of
> struct v4l2_buffer.
>
> Fortunately, almost all v4l2 drivers use monotonic timestamps and call
> v4l2_get_timestamp(), which means they don't also have a y2038 problem.
> This means we can keep using the existing binary layout of the structure
> and do not need to worry about defining a new kernel interface for
> userland with 64-bit time_t.
>
> A possible downside of this approach is that it breaks any user space
> that tries to assign the timeval structure returned from the kernel
> to another timeval, or to pass a pointer to it into a function that
> expects a timeval. Those will cause a build-time warning or error
> that can be fixed up in a backwards compatible way.
>
> The alternative to this patch is to leave the structure using
> 'struct timeval', but then we have to rework the kernel to let
> it handle both 32-bit and 64-bit time_t for 32-bit user space
> processes.
Cool. Only this morning I was thinking about what would be needed in v4l2
to be y2038 safe, and here it is!
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 3228fbebcd63..b02cf054fbb8 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -803,6 +803,12 @@ struct v4l2_plane {
> __u32 reserved[11];
> };
>
> +/* used for monotonic times, therefore y2038 safe */
> +struct v4l2_timeval {
> + long tv_sec;
> + long tv_usec;
> +};
> +
> /**
> * struct v4l2_buffer - video buffer info
> * @index: id number of the buffer
> @@ -839,7 +845,7 @@ struct v4l2_buffer {
> __u32 bytesused;
> __u32 flags;
> __u32 field;
> - struct timeval timestamp;
> + struct v4l2_timeval timestamp;
> struct v4l2_timecodetimecode;
> __u32 sequence;
>
>
I suspect that quite a few apps use assign the timestamp to another timeval
struct. A quick grep in v4l-utils (which we maintain) shows at least two of
those assignments. Ditto for xawtv3.
So I don't think v4l2_timeval is an option as it would break userspace too
badly.
An alternative to supporting a 64-bit timeval for 32-bit userspace is to make a
new y2038-aware struct and a new set of ioctls and use this opportunity to clean
up and extend the v4l2_buffer struct.
So any 32-bit app that needs to be y2038 compliant would just use the new
struct and ioctls.
But this is something to discuss among the v4l2 developers.
Regards,
Hans
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH] [media] dvb-frontend: Replace timeval with ktime_t
On 05/11/2015 04:37 AM, Tina Ruchandani wrote:
> struct timeval uses a 32-bit seconds representation which will
> overflow in the year 2038 and beyond. This patch replaces
> the usage of struct timeval with ktime_t which is a 64-bit
> timestamp and is year 2038 safe.
> This patch is part of a larger attempt to remove all instances
> of 32-bit timekeeping variables (timeval, timespec, time_t)
> which are not year 2038 safe, from the kernel.
> The patch is a work-in-progress - correctness of the following
> changes is unclear:
> (a) Usage of timeval_usec_diff - The function seems to subtract
> usec values without caring about the difference of the seconds field.
> There may be an implicit assumption in the original code that the
> time delta is always of the order of microseconds.
> The patch replaces the usage of timeval_usec_diff with
> ktime_to_us(ktime_sub()) which computes the real timestamp difference,
> not just the difference in the usec field.
> (b) printk diffing the tv[i] and tv[i-1] values. The original
> printk statement seems to get the order wrong. This patch preserves
> that order.
>
> Signed-off-by: Tina Ruchandani
Hi Tina!
Please CC patches for anything under drivers/media and/or include/media to the
linux-media mailinglist. That's where the experts on these drivers are and
they will be able to tell you if it is OK or not.
I suspect that in both cases it may well be a bug.
Regards,
Hans
> ---
> drivers/media/dvb-core/dvb_frontend.c | 46
> ---
> drivers/media/dvb-core/dvb_frontend.h | 3 +--
> drivers/media/dvb-frontends/stv0299.c | 15 ++--
> 3 files changed, 24 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c
> b/drivers/media/dvb-core/dvb_frontend.c
> index 882ca41..48c3daf 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -40,6 +40,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "dvb_frontend.h"
> @@ -889,42 +890,25 @@ static void dvb_frontend_stop(struct dvb_frontend *fe)
> fepriv->thread);
> }
>
> -s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime)
> -{
> - return ((curtime.tv_usec < lasttime.tv_usec) ?
> - 100 - lasttime.tv_usec + curtime.tv_usec :
> - curtime.tv_usec - lasttime.tv_usec);
> -}
> -EXPORT_SYMBOL(timeval_usec_diff);
> -
> -static inline void timeval_usec_add(struct timeval *curtime, u32 add_usec)
> -{
> - curtime->tv_usec += add_usec;
> - if (curtime->tv_usec >= 100) {
> - curtime->tv_usec -= 100;
> - curtime->tv_sec++;
> - }
> -}
> -
> /*
> * Sleep until gettimeofday() > waketime + add_usec
> * This needs to be as precise as possible, but as the delay is
> * usually between 2ms and 32ms, it is done using a scheduled msleep
> * followed by usleep (normally a busy-wait loop) for the remainder
> */
> -void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec)
> +void dvb_frontend_sleep_until(ktime_t waketime, u32 add_usec)
> {
> - struct timeval lasttime;
> + ktime_t lasttime;
> s32 delta, newdelta;
>
> - timeval_usec_add(waketime, add_usec);
> + ktime_add_us(waketime, add_usec);
>
> - do_gettimeofday(&lasttime);
> - delta = timeval_usec_diff(lasttime, *waketime);
> + lasttime = ktime_get_real();
> + delta = ktime_to_us(ktime_sub(lasttime, waketime));
> if (delta > 2500) {
> msleep((delta - 1500) / 1000);
> - do_gettimeofday(&lasttime);
> - newdelta = timeval_usec_diff(lasttime, *waketime);
> + lasttime = ktime_get_real();
> + newdelta = ktime_to_us(ktime_sub(lasttime, waketime));
> delta = (newdelta > delta) ? 0 : newdelta;
> }
> if (delta > 0)
> @@ -2458,24 +2442,24 @@ static int dvb_frontend_ioctl_legacy(struct file
> *file,
>* include the initialization or start bit
>*/
> unsigned long swcmd = ((unsigned long) parg) << 1;
> - struct timeval nexttime;
> - struct timeval tv[10];
> + ktime_t nexttime;
> + ktime_t tv[10];
> int i;
> u8 last = 1;
> if (dvb_frontend_debug)
> printk("%s switch command: 0x%04lx\n",
> __func__, swcmd);
> - do_gettimeofday(&nexttime);
> + nexttime = ktime_get_real();
> if (dvb_frontend_debug)
> tv[0] = nexttime;
> /* before sending a command, initialize by sending
>* a 32ms 18V to the switch
>*/
> fe->ops.set_voltage(fe, SEC_VOLTAGE_18);
> -
Re: [Y2038] [PATCH v4 06/10] cec: add HDMI CEC framework: y2038 question
Hi Arnd, On 05/04/2015 12:14 PM, Arnd Bergmann wrote: > On Monday 04 May 2015 09:42:36 Hans Verkuil wrote: >> Ping! (Added Arnd to the CC list) > > Hi Hans, > > sorry I missed this the first time > >> On 04/27/2015 09:40 AM, Hans Verkuil wrote: >>> Added the y2038 mailinglist since I would like to get their input for >>> this API. >>> >>> Y2038 experts, can you take a look at my comment in the code below? >>> >>> Thanks! >> >> Arnd, I just saw your patch series adding struct __kernel_timespec to >> uapi/linux/time.h. I get the feeling that it might take a few kernel >> cycles before we have a timespec64 available in userspace. Based on that >> I think this CEC API should drop the timestamps for now and wait until >> timespec64 becomes available before adding it. >> >> The timestamps are a nice-to-have, but not critical. So adding it later >> shouldn't be a problem. What is your opinion? > > It will take a little while for the patches to make it in, I would guess > 4.3 at the earliest. Using your own struct works just as well and would > be less ambiguous. > > However, for timestamps, I would recommend not using timespec anyway. > Instead, just use a single 64-bit nanosecond value from ktime_get_ns() > (or ktime_get_boot_ns() if you need a time that keeps ticking across > suspend). This is more efficient to get and simpler to use as long > as you don't need to convert from nanosecond to timespec. Possibly stupid follow-up question: is ktime_get_ns() just a different representation as ktime_get_ts64()? Or is there some offset between the two? They seem to be identical based on a quick test, but I'd like to be certain that that's always the case. Users need to be able to relate this timestamp to a struct timespec as returned by V4L2 (and others). Thanks! Hans ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v4 06/10] cec: add HDMI CEC framework: y2038 question
Ping! (Added Arnd to the CC list)
On 04/27/2015 09:40 AM, Hans Verkuil wrote:
> Added the y2038 mailinglist since I would like to get their input for
> this API.
>
> Y2038 experts, can you take a look at my comment in the code below?
>
> Thanks!
Arnd, I just saw your patch series adding struct __kernel_timespec to
uapi/linux/time.h. I get the feeling that it might take a few kernel
cycles before we have a timespec64 available in userspace. Based on that
I think this CEC API should drop the timestamps for now and wait until
timespec64 becomes available before adding it.
The timestamps are a nice-to-have, but not critical. So adding it later
shouldn't be a problem. What is your opinion?
Hans
>
> On 04/23/2015 03:03 PM, Kamil Debski wrote:
>> From: Hans Verkuil
>>
>> The added HDMI CEC framework provides a generic kernel interface for
>> HDMI CEC devices.
>>
>> Signed-off-by: Hans Verkuil
>> [k.deb...@samsung.com: Merged CEC Updates commit by Hans Verkuil]
>> [k.deb...@samsung.com: Merged Update author commit by Hans Verkuil]
>> [k.deb...@samsung.com: change kthread handling when setting logical
>> address]
>> [k.deb...@samsung.com: code cleanup and fixes]
>> [k.deb...@samsung.com: add missing CEC commands to match spec]
>> [k.deb...@samsung.com: add RC framework support]
>> [k.deb...@samsung.com: move and edit documentation]
>> [k.deb...@samsung.com: add vendor id reporting]
>> [k.deb...@samsung.com: add possibility to clear assigned logical
>> addresses]
>> [k.deb...@samsung.com: documentation fixes, clenaup and expansion]
>> [k.deb...@samsung.com: reorder of API structs and add reserved fields]
>> [k.deb...@samsung.com: fix handling of events and fix 32/64bit timespec
>> problem]
>> [k.deb...@samsung.com: add cec.h to include/uapi/linux/Kbuild]
>> Signed-off-by: Kamil Debski
>> ---
>> Documentation/cec.txt | 396
>> drivers/media/Kconfig |6 +
>> drivers/media/Makefile|2 +
>> drivers/media/cec.c | 1161
>> +
>> include/media/cec.h | 140 ++
>> include/uapi/linux/Kbuild |1 +
>> include/uapi/linux/cec.h | 303
>> 7 files changed, 2009 insertions(+)
>> create mode 100644 Documentation/cec.txt
>> create mode 100644 drivers/media/cec.c
>> create mode 100644 include/media/cec.h
>> create mode 100644 include/uapi/linux/cec.h
>>
>
>
>
>> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
>> index 4842a98..5854cfd 100644
>> --- a/include/uapi/linux/Kbuild
>> +++ b/include/uapi/linux/Kbuild
>> @@ -81,6 +81,7 @@ header-y += capi.h
>> header-y += cciss_defs.h
>> header-y += cciss_ioctl.h
>> header-y += cdrom.h
>> +header-y += cec.h
>> header-y += cgroupstats.h
>> header-y += chio.h
>> header-y += cm4000_cs.h
>> diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
>> new file mode 100644
>> index 000..bb6d66c
>> --- /dev/null
>> +++ b/include/uapi/linux/cec.h
>> @@ -0,0 +1,303 @@
>> +#ifndef _CEC_H
>> +#define _CEC_H
>> +
>> +#include
>> +
>> +struct cec_time {
>> +__u64 sec;
>> +__u64 nsec;
>> +};
>
> I don't like having to introduce a new struct for time here. Basically we are
> only doing this because there is still no public struct timespec64.
>
> When will that struct become available for use in a public API? If it is 4.2,
> then we can start using it. If it will take longer, then what alternative can
> we use to prevent having to change the API later?
>
> One alternative might be to drop it for now and just reserve space in the
> structs to add it later.
>
> Input from the y2038 experts will be welcome!
>
> Regards,
>
> Hans
>
>> +
>> +struct cec_msg {
>> +struct cec_time ts;
>> +__u32 len;
>> +__u32 status;
>> +__u32 timeout;
>> +/* timeout (in ms) is used to timeout CEC_RECEIVE.
>> + Set to 0 if you want to wait forever. */
>> +__u8 msg[16];
>> +__u8 reply;
>> +/* If non-zero, then wait for a reply with this opcode.
>> + If there was an error when sending the msg or FeatureAbort
>> + was returned, then reply is set to 0.
>> + If reply is non-zero upon return, then len/msg are set to
>> + the received message.
>> + If reply is zero upon return and status has the
>> + CEC_TX_STATUS_FEATURE_ABORT bit set, then len/msg are set to the
>> + receive
Re: [Y2038] [PATCH v4 06/10] cec: add HDMI CEC framework: y2038 question
Added the y2038 mailinglist since I would like to get their input for
this API.
Y2038 experts, can you take a look at my comments in the code below?
Thanks!
On 04/23/2015 03:03 PM, Kamil Debski wrote:
> From: Hans Verkuil
>
> The added HDMI CEC framework provides a generic kernel interface for
> HDMI CEC devices.
>
> Signed-off-by: Hans Verkuil
> [k.deb...@samsung.com: Merged CEC Updates commit by Hans Verkuil]
> [k.deb...@samsung.com: Merged Update author commit by Hans Verkuil]
> [k.deb...@samsung.com: change kthread handling when setting logical
> address]
> [k.deb...@samsung.com: code cleanup and fixes]
> [k.deb...@samsung.com: add missing CEC commands to match spec]
> [k.deb...@samsung.com: add RC framework support]
> [k.deb...@samsung.com: move and edit documentation]
> [k.deb...@samsung.com: add vendor id reporting]
> [k.deb...@samsung.com: add possibility to clear assigned logical
> addresses]
> [k.deb...@samsung.com: documentation fixes, clenaup and expansion]
> [k.deb...@samsung.com: reorder of API structs and add reserved fields]
> [k.deb...@samsung.com: fix handling of events and fix 32/64bit timespec
> problem]
> [k.deb...@samsung.com: add cec.h to include/uapi/linux/Kbuild]
> Signed-off-by: Kamil Debski
> ---
> Documentation/cec.txt | 396
> drivers/media/Kconfig |6 +
> drivers/media/Makefile|2 +
> drivers/media/cec.c | 1161
> +
> include/media/cec.h | 140 ++
> include/uapi/linux/Kbuild |1 +
> include/uapi/linux/cec.h | 303
> 7 files changed, 2009 insertions(+)
> create mode 100644 Documentation/cec.txt
> create mode 100644 drivers/media/cec.c
> create mode 100644 include/media/cec.h
> create mode 100644 include/uapi/linux/cec.h
>
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 4842a98..5854cfd 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -81,6 +81,7 @@ header-y += capi.h
> header-y += cciss_defs.h
> header-y += cciss_ioctl.h
> header-y += cdrom.h
> +header-y += cec.h
> header-y += cgroupstats.h
> header-y += chio.h
> header-y += cm4000_cs.h
> diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
> new file mode 100644
> index 000..bb6d66c
> --- /dev/null
> +++ b/include/uapi/linux/cec.h
> @@ -0,0 +1,303 @@
> +#ifndef _CEC_H
> +#define _CEC_H
> +
> +#include
> +
> +struct cec_time {
> + __u64 sec;
> + __u64 nsec;
> +};
I don't like having to introduce a new struct for time here. Basically we are
only doing this because there is still no public struct timespec64.
When will that struct become available for use in a public API? If it is 4.2,
then we can start using it. If it will take longer, then what alternative can
we use to prevent having to change the API later?
One alternative might be to drop it for now and just reserve space in the
structs to add it later.
Input from the y2038 experts will be welcome!
Regards,
Hans
> +
> +struct cec_msg {
> + struct cec_time ts;
> + __u32 len;
> + __u32 status;
> + __u32 timeout;
> + /* timeout (in ms) is used to timeout CEC_RECEIVE.
> +Set to 0 if you want to wait forever. */
> + __u8 msg[16];
> + __u8 reply;
> + /* If non-zero, then wait for a reply with this opcode.
> +If there was an error when sending the msg or FeatureAbort
> +was returned, then reply is set to 0.
> +If reply is non-zero upon return, then len/msg are set to
> +the received message.
> +If reply is zero upon return and status has the
> +CEC_TX_STATUS_FEATURE_ABORT bit set, then len/msg are set to the
> +received feature abort message.
> +If reply is zero upon return and status has the
> +CEC_TX_STATUS_REPLY_TIMEOUT
> +bit set, then no reply was seen at all.
> +This field is ignored with CEC_RECEIVE.
> +If reply is non-zero for CEC_TRANSMIT and the message is a broadcast,
> +then -EINVAL is returned.
> +if reply is non-zero, then timeout is set to 1000 (the required
> +maximum response time).
> + */
> + __u8 reserved[31];
> +};
> +
> +static inline __u8 cec_msg_initiator(const struct cec_msg *msg)
> +{
> + return msg->msg[0] >> 4;
> +}
> +
> +static inline __u8 cec_msg_destination(const struct cec_msg *msg)
> +{
> + return msg->msg[0] & 0xf;
> +}
> +
> +static inline bool cec_msg_is_broadcast(const struct cec_msg *msg)
> +{
> + return (msg->msg[0] & 0xf) == 0xf;
> +}
> +
> +/* cec status field */
> +#
