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
