Re: [PATCH v9 16/22] media: uvcvideo: Return -EACCES to inactive controls
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: 2021-06-10 17:29:01
Also in:
lkml
Hi Ricardo, Thank you for the patch. On Fri, Mar 26, 2021 at 10:58:34AM +0100, 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.
Isn't the device supposed to stall the control set in that case, with the bRequestErrorCode set to "Wrong state", which uvc_query_ctrl() translates to -EACCES already ? Could you elaborate on why this patch is needed ?
quoted hunk ↗ jump to hunk
Suggested-by: Hans Verkuil <redacted> Signed-off-by: Ricardo Ribalda <redacted> --- drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 23 deletions(-)diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index bcebf9d1a46f..d9d4add1e813 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c@@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map) return "Unknown Control"; } +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; +} + static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, struct uvc_control *ctrl, 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;@@ -1104,18 +1127,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;
There's a small change in behaviour here, the driver used to return an error, now it will ignore it. Is it intentional ?
quoted hunk ↗ jump to hunk
- - 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);@@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, return 0; } -static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity, +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain, + struct uvc_entity *entity, struct v4l2_ext_controls *ctrls, - struct uvc_control *uvc_control) + struct uvc_control *err_control, + int ret) { struct uvc_control_mapping *mapping; struct uvc_control *ctrl_found; unsigned int i; - if (!entity) - return ctrls->count; + if (!entity) { + ctrls->error_idx = ctrls->count; + return ret; + } for (i = 0; i < ctrls->count; i++) { __uvc_find_control(entity, ctrls->controls[i].id, &mapping, &ctrl_found, 0); - if (uvc_control == ctrl_found) - return i; + if (err_control == ctrl_found) + break; } + ctrls->error_idx = i;
I think this line should be moved after the next check.
quoted hunk ↗ jump to hunk
+ + /* We could not find the control that failed. */ + if (i == ctrls->count) + return ret; - return ctrls->count; + if (uvc_ctrl_is_inactive(chain, err_control, mapping)) + return -EACCES; + + return ret; } int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,@@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); done: if (ret < 0 && ctrls) - ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls, - err_ctrl); + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl, + ret); mutex_unlock(&chain->ctrl_mutex); return ret; }
-- Regards, Laurent Pinchart