Date:	Fri, 18 Sep 2015 11:49:50 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Arnd Bergmann <arnd@...db.de>
CC:	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	y2038@...ts.linaro.org,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	linux-api@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v2 8/9] [media] handle 64-bit time_t in v4l2_buffer

On 09/18/15 11:26, Arnd Bergmann wrote:
> On Friday 18 September 2015 09:18:45 Hans Verkuil wrote:
>> Hi Arnd,
>>
>> Thanks once again for working on this! Unfortunately, this approach won't
>> work, see my comments below.
>>
>> BTW, I would expect to see compile errors when compiling for 32 bit. Did
>> you try that?
> 
> I only tested on 32-bit, both ARM and x86, but had a longer set of patches
> applied below.
> 
>>> +static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to,
>>> +				  const struct v4l2_buffer *from)
>>> +{
>>> +	to->index		= from->index;
>>> +	to->type		= from->type;
>>> +	to->bytesused		= from->bytesused;
>>> +	to->flags		= from->flags;
>>> +	to->field		= from->field;
>>> +	to->timestamp.tv_sec	= from->timestamp.tv_sec;
>>> +	to->timestamp.tv_usec	= from->timestamp.tv_usec;
>>> +	to->timecode		= from->timecode;
>>> +	to->sequence		= from->sequence;
>>> +	to->memory		= from->memory;
>>> +	to->m.offset		= from->m.offset;
>>> +	to->length		= from->length;
>>> +	to->reserved2		= from->reserved2;
>>> +	to->reserved		= from->reserved;
>>> +}
>>
>> Is there a reason why you didn't use memcpy like you did for VIDIOC_DQEVENT (path 5/9)?
>> I would prefer that over these explicit assignments.
> 
> No strong reason. I went back and forth a bit on the m.offset field that
> is part of a union: In a previous version, I planned to move all the
> compat handling here, and doing the conversion one field at a time would
> make it easier to share the code between 32-bit and 64-bit kernels
> handling old 32-bit user space. This version doesn't do that, so I can
> use the memcpy approach instead.
> 
>>>  static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>>>  				struct file *file, void *fh, void *arg)
>>>  {
>>> @@ -2408,12 +2524,21 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>>  	IOCTL_INFO_FNC(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO),
>>>  	IOCTL_INFO_FNC(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE),
>>>  	IOCTL_INFO_FNC(VIDIOC_QUERYBUF, v4l_querybuf, v4l_print_buffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)),
>>> +#ifndef CONFIG_64BIT
>>> +	IOCTL_INFO_FNC(VIDIOC_QUERYBUF_TIME32, v4l_querybuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)),
>>> +#endif
>>
>> This doesn't work. These IOCTL macros use the ioctl nr as the index in the array. Since
>> VIDIOC_QUERYBUF and VIDIOC_QUERYBUF_TIME32 have the same index, this will fail.
> 
> Ah, I see. No idea why that did not cause a compile-time error. I got some
> errors for duplicate 'case' values when the structures are the same size
> (that's why we need the '#ifdef CONFIG_64BIT' in some places) but not here.
> 
>> I think (not 100% certain, there may be better suggestions) that the conversion is best
>> done in video_usercopy(): just before func() is called have a switch for the TIME32
>> variants, convert to the regular variant, call func() and convert back.
> 
> Does the handler have access to the _IOC_SIZE() value that was passed? If
> it does, we could add a conditional inside of v4l_querybuf().
> I did not see an easy way to do that though.

No, the v4l_ handlers don't have access to that. But I think it is much easier to
do the conversion at the first opportunity (video_usercopy), so no other code
needs to be modified. Easier to review and maintain that way.

>> My only concern here is that an additional v4l2_buffer struct (68 bytes) is needed on the
>> stack. I don't see any way around that, though.
> 
> Agreed.
> 
>>> +struct v4l2_buffer_time32 {
>>> +	__u32			index;
>>> +	__u32			type;
>>> +	__u32			bytesused;
>>> +	__u32			flags;
>>> +	__u32			field;
>>> +	struct {
>>> +		__s32		tv_sec;
>>> +		__s32		tv_usec;
>>> +	}			timestamp;
>>> +	struct v4l2_timecode	timecode;
>>> +	__u32			sequence;
>>> +
>>> +	/* memory location */
>>> +	__u32			memory;
>>> +	union {
>>> +		__u32           offset;
>>> +		unsigned long   userptr;
>>> +		struct v4l2_plane *planes;
>>> +		__s32		fd;
>>> +	} m;
>>> +	__u32			length;
>>> +	__u32			reserved2;
>>> +	__u32			reserved;
>>> +};
>>
>> Should this be part of a public API at all? Userspace will never use this struct
>> or the TIME32 ioctls in the source code, right? This would be more appropriate for
>> v4l2-ioctl.h.
> 
> Yes, makes sense. I think for the other structures I just enclosed them
> inside #ifdef __KERNEL__ so they get stripped at 'make headers_install'
> time, but I forgot to do that here.
> 
> My intention was to keep the structure close to the other one, so any
> changes to them would be more likely to make it into both versions.
> 
> Let me know if you prefer to have an #ifdef added here, or if I should
> move all three structures to v4l2-ioctl.h.

*If* the conversion takes place only in v4l2-ioctl.c, then it makes sense
have these structs + ioctls moved to v4l2-ioctl.h.

I noticed that v4l2-compat-ioctl32.c wasn't changed. Is that right? I have
unfortunately no time to double-check that, but it surprised me.

Regards,

	Hans

> Thanks a lot for the review!
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists