Re: [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI

[Arnd Bergmann <arnd@arndb.de>]

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