linux-kernel - Re: [PATCH 01/10] media: ioctl: Simulate v4l2_queryctrl with v4l2_query_ext

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCt_rpf=RjTYR+Fmy4fsWb0z6J5NU7iKrERDmvUrcxZwig@mail.gmail.com>
Date: Mon, 9 Dec 2024 21:02:11 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Mike Isely <isely@...ox.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans de Goede <hdegoede@...hat.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Andy Shevchenko <andy@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH 01/10] media: ioctl: Simulate v4l2_queryctrl with v4l2_query_ext_ctrl

Hi Hans

On Mon, 9 Dec 2024 at 20:34, Hans Verkuil <hverkuil@...all.nl> wrote:
>
> On 09/12/2024 20:25, 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@...hat.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c   |  3 ++-
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 22 +++++++++++++++++++++-
> >  2 files changed, 23 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..a5562f2f1fc9 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;
> >       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,25 @@ 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;
> > +     strscpy(p->name, qec.name, sizeof(p->name));
> > +     p->minimum = qec.minimum;
> > +     p->maximum = qec.maximum;
> > +     p->step = qec.step;
> > +     p->default_value = qec.default_value;
> > +     p->flags = qec.flags;
>
> That's not quite correct. See v4l2_queryctrl() in v4l2-ctrls-api.c
> on how to do this: for types that VIDIOC_QUERYCTRL doesn't support,
> some of these fields must be set to 0.
>
> In fact, once vidioc_queryctrl has been removed, then you can also
> remove v4l2_queryctrl() and just rely on this code. Unless I missed
> something.

Thanks for the mega-fast review :)

I do not think that we can easily remove v4l2_queryctrl(). It is still
called by v4l2-subdev.c

We could do something to remove the code duplication... but it will
probably make the code more difficult to follow.

I will send a new version with the fix that you proposed, as well as:

-- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2290,10 +2290,6 @@ static int v4l_queryctrl(const struct
v4l2_ioctl_ops *ops,
                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);
-       if (vfd->ctrl_handler)
-               return v4l2_queryctrl(vfd->ctrl_handler, p);
        if (!ops->vidioc_query_ext_ctrl)
                return -ENOTTY;

>
> Regards,
>
>         Hans
>
> > +
> > +     return 0;
> >  }
> >
> >  static int v4l_query_ext_ctrl(const struct v4l2_ioctl_ops *ops,
> >
>


-- 
Ricardo Ribalda