lkml.org 
[lkml]   [2019]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
From
Date
SubjectRe: [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
On Sun, Dec 15, 2019 at 6:26 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Ah, great, that worked, after applying the patch below.
>
> Both struct v4l2_buffer32 and v4l2_event32 need to be packed, otherwise you would
> get an additional 4 bytes since the 64 bit compiler wants to align the 8 byte tv_secs
> to an 8 byte boundary. But that's not what the i686 compiler does.

Thanks so much for the testing and finding this issue. It would be much more
embarrassing to find it later, given that I explained how it's supposed to work
in the comment above v4l2_event32 and in the documentation I just submitted
but got it wrong anyway ;-)

> If I remember correctly, packed is only needed for CONFIG_X86_64.

Correct.

> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 3bbf47d950e0..c01492cf6160 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -492,7 +492,11 @@ struct v4l2_buffer32 {
> __u32 length;
> __u32 reserved2;
> __s32 request_fd;
> +#ifdef CONFIG_X86_64
> +} __attribute__ ((packed));
> +#else
> };
> +#endif

I would prefer to write it like this instead to avoid the #ifdef, the
effect should
be the same:

--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -475,8 +475,8 @@ struct v4l2_buffer32 {
__u32 flags;
__u32 field; /* enum v4l2_field */
struct {
- long long tv_sec;
- long long tv_usec;
+ compat_s64 tv_sec;
+ compat_s64 tv_usec;
} timestamp;
struct v4l2_timecode timecode;
__u32 sequence;
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -1277,7 +1277,10 @@ struct v4l2_event32 {
} u;
__u32 pending;
__u32 sequence;
- struct __kernel_timespec timestamp;
+ struct {
+ compat_s64 tv_sec;
+ compat_s64 tv_usec;
+ } timestamp;
__u32 id;
__u32 reserved[8];
};
If you agree, I'll push out a modified branch with that version and send out
that series to the list again.

There is one more complication that I just noticed: The "struct v4l2_buffer32"
definition has always been defined in a way that works for i386 user space
but is broken for x32 user space. The version I used accidentally fixed x32
while breaking i386. With the change above, it's back to missing x32 support
(so nothing changed).

There is no way to fix the uapi definition of v4l2_buffer to have x32 and i386
use the same format, because applications may be using old headers, but
I suppose I could add yet another version of the struct to correctly deal with
x32, or just add a comment like

--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -468,6 +468,10 @@ struct v4l2_plane32 {
__u32 reserved[11];
};

+/*
+ * This is correct for all architectures including i386, but not x32,
+ * which has different alignment requirements for timestamp
+ */
struct v4l2_buffer32 {
__u32 index;
__u32 type; /* enum v4l2_buf_type */

Arnd

\
 
 \ /
  Last update: 2019-12-16 10:30    [W:0.049 / U:1.608 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site