On 12/16/19 10:29 AM, Arnd Bergmann wrote:
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.
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