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

[Hans Verkuil]

On 12/14/19 10:44 PM, Arnd Bergmann wrote:
> On Sat, Dec 14, 2019 at 12:27 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> On 12/13/19 4:32 PM, Hans Verkuil wrote:
>>>>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs,
>>>>> and there doesn't appear to be an easy way to compile a C++ program with musl.
>>>>>
>>>>> If you happen to have a test environment where you can compile C++ with musl,
>>>>> then let me know and I can give instructions on how to run the compliance tests.
>>>>>
>>>>> If you can't test that, then I can merge this regardless, and hope for the best
>>>>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested.
>>>>
>>>> I've heard good things about the prebuilt toolchains from http://musl.cc/.
>>>> These seems to come with a libstdc++, but I have not tried that myself.
>>>
>>> I'll see if I can give those a spin, but if I can't get it to work quickly,
>>> then I don't plan on spending much time on it.
>>
>> I managed to build v4l2-ctl/compliance with those toolchains, but they seem to be
>> still using a 32-bit time_t.
>>
>> Do I need to get a specific version or do something special?
> 
> My mistake: only musl-1.2.0 and up have 64-bit time_t, but this isn't released
> yet. According to https://wiki.musl-libc.org/roadmap.html, the release
> was planned
> for last month, no idea how long it will take.
> 
> It appears that a snapshot build at
> http://more.musl.cc/7.5.0/x86_64-linux-musl/i686-linux-musl-native.tgz
> is new enough to have 64-bit time_t (according to include/bits/alltypes.h),
> but this is a month old as well, so it may have known bugs.

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.

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

With these changes (plus a bunch of fixes for v4l-utils) I was able to do full
compliance tests for 64-bit, 32-bit time32 under x86_64, 32-bit time64 under x86_64,
time32 under i686 and time64 under i686.

Arnd, if you can post a v6 with the previous fixes and this fix included, then
I'll make a pull request for this for kernel 5.6.

Regards,

	Hans


Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
---
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

 struct v4l2_buffer32_time32 {
 	__u32			index;
@@ -1280,7 +1284,7 @@ struct v4l2_event32 {
 	struct __kernel_timespec	timestamp;
 	__u32				id;
 	__u32				reserved[8];
-};
+} __attribute__ ((packed));

 struct v4l2_event32_time32 {
 	__u32				type;