Re: [PATCH v2 01/11] media: ioctl: Simulate v4l2_queryctrl with v4l2_query_ext_ctrl

[Hans Verkuil]

On 10/12/2024 10:28, Ricardo Ribalda wrote:
> v4l2_queryctrl is a subset of v4l2_query_ext_ctrl. If the driver does
> not implement v4l2_queryctrl we can implement it with
> v4l2_query_ext_ctrl.
> 
> Suggested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c   |  3 ++-
>  drivers/media/v4l2-core/v4l2-ioctl.c | 37 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 5bcaeeba4d09..252308a67fa8 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -572,7 +572,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  	   and that can't be tested here. If the bit for these control ioctls
>  	   is set, then the ioctl is valid. But if it is 0, then it can still
>  	   be valid if the filehandle passed the control handler. */
> -	if (vdev->ctrl_handler || ops->vidioc_queryctrl)
> +	if (vdev->ctrl_handler || ops->vidioc_queryctrl ||
> +	    ops->vidioc_query_ext_ctrl)
>  		__set_bit(_IOC_NR(VIDIOC_QUERYCTRL), valid_ioctls);
>  	if (vdev->ctrl_handler || ops->vidioc_query_ext_ctrl)
>  		__set_bit(_IOC_NR(VIDIOC_QUERY_EXT_CTRL), valid_ioctls);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 0304daa8471d..7d615ebc511e 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2284,9 +2284,11 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
>  	struct video_device *vfd = video_devdata(file);
> +	struct v4l2_query_ext_ctrl qec;

Zero this struct (qec = {}). The VIDIOC_QUERY_EXT_CTRL ioctl will zero everything
after the id field when called from userspace, but that does not happen if you call
the op directly, as you do below. So just zero it here instead.

Regards,

	Hans

>  	struct v4l2_queryctrl *p = arg;
>  	struct v4l2_fh *vfh =
>  		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +	int ret;
>  
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_queryctrl(vfh->ctrl_handler, p);
> @@ -2294,7 +2296,40 @@ static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
>  		return v4l2_queryctrl(vfd->ctrl_handler, p);
>  	if (ops->vidioc_queryctrl)
>  		return ops->vidioc_queryctrl(file, fh, p);
> -	return -ENOTTY;
> +	if (!ops->vidioc_query_ext_ctrl)
> +		return -ENOTTY;
> +
> +	/* Simulate query_ext_ctr using query_ctrl. */
> +	qec.id = p->id;
> +	ret = ops->vidioc_query_ext_ctrl(file, fh, &qec);
> +	if (ret)
> +		return ret;
> +
> +	p->id = qec.id;
> +	p->type = qec.type;
> +	p->flags = qec.flags;
> +	strscpy(p->name, qec.name, sizeof(p->name));
> +	switch (p->type) {
> +	case V4L2_CTRL_TYPE_INTEGER:
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +	case V4L2_CTRL_TYPE_MENU:
> +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> +	case V4L2_CTRL_TYPE_STRING:
> +	case V4L2_CTRL_TYPE_BITMASK:
> +		p->minimum = qec.minimum;
> +		p->maximum = qec.maximum;
> +		p->step = qec.step;
> +		p->default_value = qec.default_value;
> +		break;
> +	default:
> +		p->minimum = 0;
> +		p->maximum = 0;
> +		p->step = 0;
> +		p->default_value = 0;
> +		break;
> +	}
> +
> +	return 0;
>  }
>  
>  static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
>