Hi Ricardo, On 17/03/2021 17:45, Ricardo Ribalda wrote: > If a control is inactive return -EACCES to let the userspace know that > the value will not be applied automatically when the control is active > again. > > Signed-off-by: Ricardo Ribalda <[email protected]> > Suggested-by: Hans Verkuil <[email protected]>
In several of the patches in this series you added 'Suggested-by' or 'Credit-to'. Please use <[email protected]> so Cisco gets the credit as well :-) > --- > drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++++++------- > drivers/media/usb/uvc/uvc_v4l2.c | 11 ++++- > drivers/media/usb/uvc/uvcvideo.h | 3 +- > 3 files changed, 69 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c > index af1d4d9b8afb..26d3494b401b 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1046,10 +1046,62 @@ static int uvc_query_v4l2_class(struct > uvc_video_chain *chain, u32 req_id, > return 0; > } > > -int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id, bool > read) > +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping) > +{ > + struct uvc_control_mapping *master_map = NULL; > + struct uvc_control *master_ctrl = NULL; > + s32 val; > + int ret; > + > + if (!mapping->master_id) > + return false; > + > + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > + &master_ctrl, 0); > + > + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > + return false; > + > + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > + if (ret < 0 || val == mapping->master_manual) > + return false; > + > + return true; > +} > + > +int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id, Same typo: accesible -> accessible > + unsigned long ioctl) > { > struct uvc_control_mapping *mapping; > struct uvc_control *ctrl; > + bool read, try; > + > + switch (ioctl) { > + case VIDIOC_G_EXT_CTRLS: > + read = true; > + try = false; > + break; > + case VIDIOC_S_EXT_CTRLS: > + read = false; > + try = false; > + break; > + case VIDIOC_TRY_EXT_CTRLS: > + read = false; > + try = true; > + break; > + case VIDIOC_G_CTRL: > + read = true; > + try = false; > + break; > + case VIDIOC_S_CTRL: > + read = false; > + try = false; > + break; > + default: > + return -EINVAL; > + } That's ugly. Just remove the bools and switch and just check the ioctl below... > > if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0) > return -EACCES; > @@ -1064,6 +1116,9 @@ int uvc_ctrl_is_accesible(struct uvc_video_chain > *chain, u32 v4l2_id, bool read) > if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read) > return -EACCES; > > + if (!read && !try && uvc_ctrl_is_inactive(chain, ctrl, mapping)) and this becomes: if ((ioctl == VIDIOC_S_EXT_CTRLS || ioctl == VIDIOC_S_CTRL) && uvc_ctrl_is_inactive(chain, ctrl, mapping)) Much, much simpler! > + return -EACCES; > + > return 0; > } > > @@ -1086,8 +1141,6 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain > *chain, > struct uvc_control_mapping *mapping, > struct v4l2_queryctrl *v4l2_ctrl) > { > - struct uvc_control_mapping *master_map = NULL; > - struct uvc_control *master_ctrl = NULL; > const struct uvc_menu_info *menu; > unsigned int i; > > @@ -1103,18 +1156,8 @@ static int __uvc_query_v4l2_ctrl(struct > uvc_video_chain *chain, > if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > - if (mapping->master_id) > - __uvc_find_control(ctrl->entity, mapping->master_id, > - &master_map, &master_ctrl, 0); > - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > - s32 val; > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > - if (ret < 0) > - return ret; > - > - if (val != mapping->master_manual) > - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > - } > + if (uvc_ctrl_is_inactive(chain, ctrl, mapping)) > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > > if (!ctrl->cached) { > int ret = uvc_ctrl_populate_cache(chain, ctrl); > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c > b/drivers/media/usb/uvc/uvc_v4l2.c > index ce55b4bff687..8e9051a245c7 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -999,6 +999,10 @@ static int uvc_ioctl_g_ctrl(struct file *file, void *fh, > struct v4l2_ext_control xctrl; > int ret; > > + ret = uvc_ctrl_is_accesible(chain, ctrl->id, VIDIOC_G_CTRL); > + if (ret) > + return ret; > + > memset(&xctrl, 0, sizeof(xctrl)); > xctrl.id = ctrl->id; > > @@ -1023,6 +1027,10 @@ static int uvc_ioctl_s_ctrl(struct file *file, void > *fh, > struct v4l2_ext_control xctrl; > int ret; > > + ret = uvc_ctrl_is_accesible(chain, ctrl->id, VIDIOC_S_CTRL); > + if (ret) > + return ret; > + > memset(&xctrl, 0, sizeof(xctrl)); > xctrl.id = ctrl->id; > xctrl.value = ctrl->value; > @@ -1054,8 +1062,7 @@ static int uvc_ctrl_check_access(struct uvc_video_chain > *chain, > int ret = 0; > > for (i = 0; i < ctrls->count; ++ctrl, ++i) { > - ret = uvc_ctrl_is_accesible(chain, ctrl->id, > - ioctl == VIDIOC_G_EXT_CTRLS); > + ret = uvc_ctrl_is_accesible(chain, ctrl->id, ioctl); > if (ret) > break; > } > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h > index 3288b118264e..4e86d0983d58 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -902,7 +902,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle) > > int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control > *xctrl); > int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl); > -int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id, bool > read); > +int uvc_ctrl_is_accesible(struct uvc_video_chain *chain, u32 v4l2_id, > + unsigned long ioctl); > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry); > Regards, Hans
