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

[Hans Verkuil]

On 12/16/19 10:29 AM, Arnd Bergmann wrote:
> On Sun, Dec 15, 2019 at 6:26 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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.

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