On 11/08/17 23:08, Mauro Carvalho Chehab wrote:
> Em Fri, 11 Aug 2017 15:26:03 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> The ctrl_is_pointer used a hard-coded list of control IDs that besides being
>> outdated also wouldn't work for custom driver controls.
>>
>> Replaced by calling queryctrl and checking if the V4L2_CTRL_FLAG_HAS_PAYLOAD
>> flag was set.
>>
>> Note that get_v4l2_ext_controls32() will set the v4l2_ext_control 'size' field
>> to 0 if the control has no payload before passing it to the kernel. This
>> helps in put_v4l2_ext_controls32() since that function can just look at the
>> 'size' field instead of having to call queryctrl again. The reason we set
>> 'size' explicitly for non-pointer controls is that 'size' is ignored by the
>> kernel in that case. That makes 'size' useless as an indicator of a pointer
>> type in the put function since it can be any value. But setting it to 0 here
>> turns it into a useful indicator.
>>
>> Also added proper checks for the compat_alloc_user_space return value which
>> can be NULL, this was never done for some reason.
>
> On a quick preview, please split those extra checks you added on
> a separate patch.
>
> The logic for the remaining parts of this patch is not trivial. I'll look
> into it later.
>
>>
>> Tested with a 32-bit build of v4l2-ctl and the vivid driver.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index af8b4c5b0efa..a16338cc216e 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
<snip>
>> -/* The following function really belong in v4l2-common, but that causes
>> - a circular dependency between modules. We need to think about this, but
>> - for now this will do. */
>>
>> -/* Return non-zero if this control is a pointer type. Currently only
>> - type STRING is a pointer type. */
>> -static inline int ctrl_is_pointer(u32 id)
>> +/* Return non-zero if this control is a pointer type. */
>> +static inline int ctrl_is_pointer(struct file *file, u32 id)
>> {
>> - switch (id) {
>> - case V4L2_CID_RDS_TX_PS_NAME:
>> - case V4L2_CID_RDS_TX_RADIO_TEXT:
>> - return 1;
>> - default:
>> + struct video_device *vfd = video_devdata(file);
>> + const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops;
>> + void *fh = file->private_data;
>> + struct v4l2_fh *vfh =
>> + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>> + struct v4l2_queryctrl qctrl = { id };
>> + int err;
>> +
>> + if (!test_bit(_IOC_NR(VIDIOC_QUERYCTRL), vfd->valid_ioctls))
>> + err = -ENOTTY;
>> + else if (vfh && vfh->ctrl_handler)
>> + err = v4l2_queryctrl(vfh->ctrl_handler, &qctrl);
>> + else if (vfd->ctrl_handler)
>> + err = v4l2_queryctrl(vfd->ctrl_handler, &qctrl);
>> + else if (ops->vidioc_queryctrl)
>> + err = ops->vidioc_queryctrl(file, fh, &qctrl);
>> + else
>> + err = -ENOTTY;
>> +
>> + if (err)
>> return 0;
>> - }
>> +
>> + return qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD;
>> }
Mauro,
I'd like your opinion on something: the code to call queryctrl is identical to
the v4l_queryctrl() function in v4l2-ioctl.c. I have been debating with myself
whether or not to drop the 'static' from that v4l2-ioctl.c function and call
it from here. It's a bit unexpected to have this source calling a function in
v4l2-ioctl.c, but on the other hand it avoids having a copy of that function.
I'm leaning towards calling v4l_queryctrl from here, but I wonder what you
think.
Regards,
Hans
