On Mon, Jun 03, 2024 at 01:49:51PM +0200, Ricardo Ribalda wrote:
> Hi Daniel
>
> Thanks for the patch. Some minor nits.
>
> Feel free to ignore it if you prefer your style.
>
> On Sun, 2 Jun 2024 at 08:52, Daniel Schaefer <dhs@xxxxxxxxxx> wrote:
> >
> > When the UVC device has a control that is readonly it doesn't set the
> > SET_CUR flag. For example the privacy control has SET_CUR flag set in
> > the defaults in the `uvc_ctrls` variable. Even if the device does not
> > have it set, it's not cleared by uvc_ctrl_get_flags.
> >
> > Originally written with assignment in commit 859086ae3636 ("media:
> > uvcvideo: Apply flags from device to actual properties"). But changed to
> > |= in commit 0dc68cabdb62 ("media: uvcvideo: Prevent setting unavailable
> > flags"). It would not clear the default flags.
> >
> > With this patch applied the correct flags are reported to user space.
> > Tested with:
> >
> > ```
> > > v4l2-ctl --list-ctrls | grep privacy
> > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
> > ```
> >
> > Cc: Edgar Thier <info@xxxxxxxxxxxxxx>
> > Cc: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Cc: Kieran Levin <ktl@xxxxxxxxxx>
> > Signed-off-by: Daniel Schaefer <dhs@xxxxxxxxxx>
> Fixes: 0dc68cabdb62 ("media: uvcvideo: Prevent setting unavailable flags")
>
> Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++---------
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4b685f883e4d..f50542e26542 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2031,15 +2031,23 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> > else
> > ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> > dev->intfnum, info->selector, data, 1);
> > - if (!ret)
> > - info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
> > - UVC_CTRL_FLAG_GET_CUR : 0)
> > - | (data[0] & UVC_CONTROL_CAP_SET ?
> > - UVC_CTRL_FLAG_SET_CUR : 0)
> > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> > - UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> > - | (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> > - UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> > + if (!ret) {
> > + info->flags = (data[0] & UVC_CONTROL_CAP_GET)
> > + ? (info->flags | UVC_CTRL_FLAG_GET_CUR)
> > + : (info->flags & ~UVC_CTRL_FLAG_GET_CUR);
> > +
> > + info->flags = (data[0] & UVC_CONTROL_CAP_SET)
> > + ? (info->flags | UVC_CTRL_FLAG_SET_CUR)
> > + : (info->flags & ~UVC_CTRL_FLAG_SET_CUR);
> > +
> > + info->flags = (data[0] & UVC_CONTROL_CAP_AUTOUPDATE)
> > + ? (info->flags | UVC_CTRL_FLAG_AUTO_UPDATE)
> > + : (info->flags & ~UVC_CTRL_FLAG_AUTO_UPDATE);
> > +
> > + info->flags = (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS)
> > + ? (info->flags | UVC_CTRL_FLAG_ASYNCHRONOUS)
> > + : (info->flags & ~UVC_CTRL_FLAG_ASYNCHRONOUS);
> > + }
>
> nit: I would have done it as
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 4b685f883e4d..c453a67e1407 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2031,7 +2031,12 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> else
> ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> dev->intfnum, info->selector, data, 1);
> - if (!ret)
> + if (!ret) {
> + info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
> + UVC_CTRL_FLAG_SET_CUR |
> + UVC_CTRL_FLAG_AUTO_UPDATE |
> + UVC_CTRL_FLAG_ASYNCHRONOUS);
> +
> info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
> UVC_CTRL_FLAG_GET_CUR : 0)
> | (data[0] & UVC_CONTROL_CAP_SET ?
> @@ -2040,6 +2045,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> | (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> + }
I prefer that slightly too.
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
I'll make the change in my tree, no need to send a v2.
> >
> > kfree(data);
> > return ret;
--
Regards,
Laurent Pinchart
