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);
+ }
>
> kfree(data);
> return ret;
> --
> 2.43.0
>
>
--
Ricardo Ribalda
