| Message ID | 20231201071907.3080126-6-yunkec@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement UVC v1.5 ROI | expand |
Hi Yunke,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yunke-Cao/media-v4l2_ctrl-Add-V4L2_CTRL_TYPE_RECT/20231201-152501
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20231201071907.3080126-6-yunkec%40google.com
patch subject: [PATCH v14 05/11] media: uvcvideo: Add support for compound controls
config: powerpc64-randconfig-r081-20231204 (https://download.01.org/0day-ci/archive/20231206/202312060523.tldbovNM-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231206/202312060523.tldbovNM-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202312060523.tldbovNM-lkp@intel.com/
smatch warnings:
drivers/media/usb/uvc/uvc_ctrl.c:2065 __uvc_ctrl_set_compound() warn: maybe return -EFAULT instead of the bytes remaining?
vim +2065 drivers/media/usb/uvc/uvc_ctrl.c
1a1a2af88920e9 Yunke Cao 2023-12-01 2040 static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping,
1a1a2af88920e9 Yunke Cao 2023-12-01 2041 struct v4l2_ext_control *xctrl,
1a1a2af88920e9 Yunke Cao 2023-12-01 2042 struct uvc_control *ctrl)
1a1a2af88920e9 Yunke Cao 2023-12-01 2043 {
1a1a2af88920e9 Yunke Cao 2023-12-01 2044 u8 *data;
1a1a2af88920e9 Yunke Cao 2023-12-01 2045 int ret;
1a1a2af88920e9 Yunke Cao 2023-12-01 2046
1a1a2af88920e9 Yunke Cao 2023-12-01 2047 if (xctrl->size != mapping->v4l2_size / 8)
1a1a2af88920e9 Yunke Cao 2023-12-01 2048 return -EINVAL;
1a1a2af88920e9 Yunke Cao 2023-12-01 2049
1a1a2af88920e9 Yunke Cao 2023-12-01 2050 data = kmalloc(xctrl->size, GFP_KERNEL);
1a1a2af88920e9 Yunke Cao 2023-12-01 2051 if (!data)
1a1a2af88920e9 Yunke Cao 2023-12-01 2052 return -ENOMEM;
1a1a2af88920e9 Yunke Cao 2023-12-01 2053
1a1a2af88920e9 Yunke Cao 2023-12-01 2054 ret = copy_from_user(data, xctrl->ptr, xctrl->size);
copy_from_user() never returns negatives, it returns the number of bytes
remaining to be copied. This should be:
if (copy_from_user(data, xctrl->ptr, xctrl->size)) {
ret = -EFAULT;
goto out;
}
1a1a2af88920e9 Yunke Cao 2023-12-01 2055 if (ret < 0)
1a1a2af88920e9 Yunke Cao 2023-12-01 2056 goto out;
1a1a2af88920e9 Yunke Cao 2023-12-01 2057
1a1a2af88920e9 Yunke Cao 2023-12-01 2058 ret = mapping->set_compound(mapping, data,
1a1a2af88920e9 Yunke Cao 2023-12-01 2059 uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
1a1a2af88920e9 Yunke Cao 2023-12-01 2060
1a1a2af88920e9 Yunke Cao 2023-12-01 2061 __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl);
1a1a2af88920e9 Yunke Cao 2023-12-01 2062
1a1a2af88920e9 Yunke Cao 2023-12-01 2063 out:
1a1a2af88920e9 Yunke Cao 2023-12-01 2064 kfree(data);
1a1a2af88920e9 Yunke Cao 2023-12-01 @2065 return ret;
1a1a2af88920e9 Yunke Cao 2023-12-01 2066 }
Hi Yunke, Thank you for the patch. On Fri, Dec 01, 2023 at 04:18:56PM +0900, Yunke Cao wrote: > Supports getting/setting current value. > Supports getting default value. > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND. Please write a better commit message. > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > Changelog since v11: > - No change. > Changelog since v10: > - Rewrite some logic in __uvc_find_control(). > - Remove a duplicate check in __uvc_ctrl_get_compound_cur(). > - Thanks, Daniel! > Changelog since v9: > - Make __uvc_ctrl_set_compound() static. > Changelog since v8: > - No change. > Changelog since v7: > - Fixed comments styles, indentation and a few other style issues. > - Renamed uvc_g/set_array() to uvc_g/set_compound(). > - Moved size check to __uvc_ctrl_add_mapping(). > - After setting a new value, copy it back to user. > - In __uvc_ctrl_set_compound(), check size before allocating. > > drivers/media/usb/uvc/uvc_ctrl.c | 179 +++++++++++++++++++++++++++---- > drivers/media/usb/uvc/uvcvideo.h | 4 + > 2 files changed, 164 insertions(+), 19 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 98254b93eb46..aae2480496b7 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -884,6 +884,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > } > } > > +/* > + * Extract the byte array specified by mapping->offset and mapping->data_size > + * stored at 'data' to the output array 'data_out'. > + */ > +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data, > + u8 *data_out) > +{ > + memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8); > + return 0; > +} > + > +/* > + * Copy the byte array 'data_in' to the destination specified by mapping->offset > + * and mapping->data_size stored at 'data'. > + */ > +static int uvc_set_compound(struct uvc_control_mapping *mapping, > + const u8 *data_in, u8 *data) > +{ > + memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8); > + return 0; > +} > + I may be missing something, but isn't this essentially dead code ? This series adds support for a single compound control, with custom get/set handlers, so these two default handlers will never be called. I don't think it makes sense to have default handlers for compound controls, I don't foresee any use case where a simple mempcy() will do the right thing. > static bool > uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) > { > @@ -906,7 +928,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, > > static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > struct uvc_control_mapping **mapping, struct uvc_control **control, > - int next) > + int next, int next_compound) > { > struct uvc_control *ctrl; > struct uvc_control_mapping *map; > @@ -921,14 +943,16 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > continue; > > list_for_each_entry(map, &ctrl->info.mappings, list) { > - if ((map->id == v4l2_id) && !next) { > + if (map->id == v4l2_id && !next && !next_compound) { > *control = ctrl; > *mapping = map; > return; > } > > if ((*mapping == NULL || (*mapping)->id > map->id) && > - (map->id > v4l2_id) && next) { > + (map->id > v4l2_id) && > + (uvc_ctrl_mapping_is_compound(map) ? > + next_compound : next)) { > *control = ctrl; > *mapping = map; > } > @@ -942,6 +966,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > struct uvc_control *ctrl = NULL; > struct uvc_entity *entity; > int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; > + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; > > *mapping = NULL; > > @@ -950,12 +975,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > /* Find the control. */ > list_for_each_entry(entity, &chain->entities, chain) { > - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); > - if (ctrl && !next) > + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, > + next_compound); > + if (ctrl && !next && !next_compound) > return ctrl; > } > > - if (ctrl == NULL && !next) > + if (!ctrl && !next && !next_compound) > uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", > v4l2_id); > > @@ -1101,12 +1127,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > return 0; > } > > +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping, > + struct uvc_control *ctrl, > + int id, > + struct v4l2_ext_control *xctrl) > +{ > + u8 size; > + u8 *data; > + int ret; > + > + size = mapping->v4l2_size / 8; > + if (xctrl->size < size) { > + xctrl->size = size; > + return -ENOSPC; > + } > + > + data = kmalloc(size, GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data); > + if (ret < 0) > + goto out; > + > + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; > + > +out: > + kfree(data); > + return ret; > +} > + > +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping, > + struct v4l2_ext_control *xctrl) > +{ > + int ret; > + > + ret = __uvc_ctrl_load_cur(chain, ctrl); > + if (ret < 0) > + return ret; > + > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, > + xctrl); > +} > + > static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, > u32 found_id) > { > - bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > unsigned int i; > > + bool find_next = req_id & > + (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND); > + > req_id &= V4L2_CTRL_ID_MASK; > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > @@ -1194,7 +1267,7 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > } > > __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > - &master_ctrl, 0); > + &master_ctrl, 0, 0); > > if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > return 0; > @@ -1262,7 +1335,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > if (mapping->master_id) > __uvc_find_control(ctrl->entity, mapping->master_id, > - &master_map, &master_ctrl, 0); > + &master_map, &master_ctrl, 0, 0); > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > s32 val = 0; > int ret; > @@ -1278,6 +1351,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > } > > + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; > + v4l2_ctrl->default_value = 0; > + v4l2_ctrl->minimum = 0; > + v4l2_ctrl->maximum = 0; > + v4l2_ctrl->step = 0; > + return 0; > + } > + > if (!ctrl->cached) { > int ret = uvc_ctrl_populate_cache(chain, ctrl); > if (ret < 0) > @@ -1533,7 +1615,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > s32 val = 0; > > - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); > + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); > if (ctrl == NULL) > return; > > @@ -1843,7 +1925,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > for (i = 0; i < ctrls->count; i++) { > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > - &ctrl_found, 0); > + &ctrl_found, 0, 0); > if (uvc_control == ctrl_found) > return i; > } > @@ -1891,7 +1973,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > return -EINVAL; > > if (uvc_ctrl_mapping_is_compound(mapping)) > - return -EINVAL; > + return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl); > else > return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > } > @@ -1912,6 +1994,22 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, > return 0; > } > > +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping, > + struct v4l2_ext_control *xctrl) > +{ > + int ret; > + > + if (!ctrl->cached) { > + ret = uvc_ctrl_populate_cache(chain, ctrl); > + if (ret < 0) > + return ret; > + } > + > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl); > +} > + > int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > struct v4l2_ext_control *xctrl) > { > @@ -1929,7 +2027,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > } > > if (uvc_ctrl_mapping_is_compound(mapping)) > - ret = -EINVAL; > + ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping, > + xctrl); > else > ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl); > > @@ -1938,6 +2037,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > return ret; > } > > +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, > + struct v4l2_ext_control *xctrl, > + struct uvc_control *ctrl) > +{ > + u8 *data; > + int ret; > + > + if (xctrl->size != mapping->v4l2_size / 8) > + return -EINVAL; > + > + data = kmalloc(xctrl->size, GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + ret = copy_from_user(data, xctrl->ptr, xctrl->size); > + if (ret < 0) > + goto out; > + > + ret = mapping->set_compound(mapping, data, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > + > + __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl); Why do you need to call __uvc_ctrl_get_compound() here ? > + > +out: > + kfree(data); > + return ret; > +} > + > int uvc_ctrl_set(struct uvc_fh *handle, > struct v4l2_ext_control *xctrl) > { > @@ -2052,13 +2179,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > ctrl->info.size); > } > > - if (uvc_ctrl_mapping_is_compound(mapping)) > - return -EINVAL; > - else > + if (uvc_ctrl_mapping_is_compound(mapping)) { > + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); > + if (ret < 0) > + return ret; > + } else > mapping->set(mapping, value, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); I really don't like how handling of compound controls is scattered everywhere :-( That makes the code difficult to read, and thus more error-prone. Could this be refactored to simplify the implementation ? In particular, I'm thinking about - Moving the copy_from_user() and copy_to_user() towards the top of the call stack, directly in uvc_ctrl_get(), uvc_ctrl_get_boundary() and uvc_ctrl_set() - Merging the .[gs]et() and .[gs]et_compound() functions (see below) - Moving the value clamping from uvc_set_compound_rect() to uvc_ctrl_set(), with the rest of the clamping code > > - > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > ctrl->handle = handle; > > @@ -2468,10 +2596,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > goto err_nomem; > } > > - if (map->get == NULL) > + if (uvc_ctrl_mapping_is_compound(map)) { > + if (map->data_size != map->v4l2_size) > + return -EINVAL; If the two values have to be the same, why do we have two fields ? > + > + /* Only supports byte-aligned data. */ > + if (WARN_ON(map->offset % 8 || map->data_size % 8)) > + return -EINVAL; > + } > + > + if (!map->get && !uvc_ctrl_mapping_is_compound(map)) > map->get = uvc_get_le_value; > - if (map->set == NULL) > + if (!map->set && !uvc_ctrl_mapping_is_compound(map)) > map->set = uvc_set_le_value; > + if (!map->get_compound && uvc_ctrl_mapping_is_compound(map)) > + map->get_compound = uvc_get_compound; > + if (!map->set_compound && uvc_ctrl_mapping_is_compound(map)) > + map->set_compound = uvc_set_compound; > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 7bc41270ed94..11805b729c22 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -129,8 +129,12 @@ struct uvc_control_mapping { > > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > const u8 *data); > + int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data, > + u8 *data_out); > void (*set)(struct uvc_control_mapping *mapping, s32 value, > u8 *data); > + int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in, > + u8 *data); Instead of adding new functions, I think we could modify the existing .get() and .set() handlers to be usable for compound controls. int (*get)(struct uvc_control_mapping *mapping, u8 query, const u8 *data_in, void *data_out); void (*set)(struct uvc_control_mapping *mapping, const void *data_in, u8 *data_out); For additional safety, you could pass the size of the void * buffer to the functions. > }; > > struct uvc_control {
Hi Laurent, Thank you for the review! On Sat, Dec 9, 2023 at 12:07 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Yunke, > > Thank you for the patch. > > On Fri, Dec 01, 2023 at 04:18:56PM +0900, Yunke Cao wrote: > > Supports getting/setting current value. > > Supports getting default value. > > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND. > > Please write a better commit message. > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > Signed-off-by: Yunke Cao <yunkec@google.com> > > --- > > Changelog since v11: > > - No change. > > Changelog since v10: > > - Rewrite some logic in __uvc_find_control(). > > - Remove a duplicate check in __uvc_ctrl_get_compound_cur(). > > - Thanks, Daniel! > > Changelog since v9: > > - Make __uvc_ctrl_set_compound() static. > > Changelog since v8: > > - No change. > > Changelog since v7: > > - Fixed comments styles, indentation and a few other style issues. > > - Renamed uvc_g/set_array() to uvc_g/set_compound(). > > - Moved size check to __uvc_ctrl_add_mapping(). > > - After setting a new value, copy it back to user. > > - In __uvc_ctrl_set_compound(), check size before allocating. > > > > drivers/media/usb/uvc/uvc_ctrl.c | 179 +++++++++++++++++++++++++++---- > > drivers/media/usb/uvc/uvcvideo.h | 4 + > > 2 files changed, 164 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 98254b93eb46..aae2480496b7 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -884,6 +884,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > > } > > } > > > > +/* > > + * Extract the byte array specified by mapping->offset and mapping->data_size > > + * stored at 'data' to the output array 'data_out'. > > + */ > > +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data, > > + u8 *data_out) > > +{ > > + memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8); > > + return 0; > > +} > > + > > +/* > > + * Copy the byte array 'data_in' to the destination specified by mapping->offset > > + * and mapping->data_size stored at 'data'. > > + */ > > +static int uvc_set_compound(struct uvc_control_mapping *mapping, > > + const u8 *data_in, u8 *data) > > +{ > > + memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8); > > + return 0; > > +} > > + > > I may be missing something, but isn't this essentially dead code ? This > series adds support for a single compound control, with custom get/set > handlers, so these two default handlers will never be called. > > I don't think it makes sense to have default handlers for compound > controls, I don't foresee any use case where a simple mempcy() will do > the right thing. > Yes, they are dead code. I thought they would be useful for some controls. I will remove them in v15. > > static bool > > uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) > > { > > @@ -906,7 +928,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, > > > > static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > struct uvc_control_mapping **mapping, struct uvc_control **control, > > - int next) > > + int next, int next_compound) > > { > > struct uvc_control *ctrl; > > struct uvc_control_mapping *map; > > @@ -921,14 +943,16 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > continue; > > > > list_for_each_entry(map, &ctrl->info.mappings, list) { > > - if ((map->id == v4l2_id) && !next) { > > + if (map->id == v4l2_id && !next && !next_compound) { > > *control = ctrl; > > *mapping = map; > > return; > > } > > > > if ((*mapping == NULL || (*mapping)->id > map->id) && > > - (map->id > v4l2_id) && next) { > > + (map->id > v4l2_id) && > > + (uvc_ctrl_mapping_is_compound(map) ? > > + next_compound : next)) { > > *control = ctrl; > > *mapping = map; > > } > > @@ -942,6 +966,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > struct uvc_control *ctrl = NULL; > > struct uvc_entity *entity; > > int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; > > > > *mapping = NULL; > > > > @@ -950,12 +975,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > > > /* Find the control. */ > > list_for_each_entry(entity, &chain->entities, chain) { > > - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); > > - if (ctrl && !next) > > + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, > > + next_compound); > > + if (ctrl && !next && !next_compound) > > return ctrl; > > } > > > > - if (ctrl == NULL && !next) > > + if (!ctrl && !next && !next_compound) > > uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", > > v4l2_id); > > > > @@ -1101,12 +1127,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > > return 0; > > } > > > > +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping, > > + struct uvc_control *ctrl, > > + int id, > > + struct v4l2_ext_control *xctrl) > > +{ > > + u8 size; > > + u8 *data; > > + int ret; > > + > > + size = mapping->v4l2_size / 8; > > + if (xctrl->size < size) { > > + xctrl->size = size; > > + return -ENOSPC; > > + } > > + > > + data = kmalloc(size, GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data); > > + if (ret < 0) > > + goto out; > > + > > + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; > > + > > +out: > > + kfree(data); > > + return ret; > > +} > > + > > +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl, > > + struct uvc_control_mapping *mapping, > > + struct v4l2_ext_control *xctrl) > > +{ > > + int ret; > > + > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > + if (ret < 0) > > + return ret; > > + > > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, > > + xctrl); > > +} > > + > > static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, > > u32 found_id) > > { > > - bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > unsigned int i; > > > > + bool find_next = req_id & > > + (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND); > > + > > req_id &= V4L2_CTRL_ID_MASK; > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > @@ -1194,7 +1267,7 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > } > > > > __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > > - &master_ctrl, 0); > > + &master_ctrl, 0, 0); > > > > if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > > return 0; > > @@ -1262,7 +1335,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > if (mapping->master_id) > > __uvc_find_control(ctrl->entity, mapping->master_id, > > - &master_map, &master_ctrl, 0); > > + &master_map, &master_ctrl, 0, 0); > > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > > s32 val = 0; > > int ret; > > @@ -1278,6 +1351,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > > } > > > > + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { > > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; > > + v4l2_ctrl->default_value = 0; > > + v4l2_ctrl->minimum = 0; > > + v4l2_ctrl->maximum = 0; > > + v4l2_ctrl->step = 0; > > + return 0; > > + } > > + > > if (!ctrl->cached) { > > int ret = uvc_ctrl_populate_cache(chain, ctrl); > > if (ret < 0) > > @@ -1533,7 +1615,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > > s32 val = 0; > > > > - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); > > + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); > > if (ctrl == NULL) > > return; > > > > @@ -1843,7 +1925,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > > > for (i = 0; i < ctrls->count; i++) { > > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > > - &ctrl_found, 0); > > + &ctrl_found, 0, 0); > > if (uvc_control == ctrl_found) > > return i; > > } > > @@ -1891,7 +1973,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > return -EINVAL; > > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > - return -EINVAL; > > + return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl); > > else > > return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > > } > > @@ -1912,6 +1994,22 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, > > return 0; > > } > > > > +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl, > > + struct uvc_control_mapping *mapping, > > + struct v4l2_ext_control *xctrl) > > +{ > > + int ret; > > + > > + if (!ctrl->cached) { > > + ret = uvc_ctrl_populate_cache(chain, ctrl); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl); > > +} > > + > > int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > struct v4l2_ext_control *xctrl) > > { > > @@ -1929,7 +2027,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > } > > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > - ret = -EINVAL; > > + ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping, > > + xctrl); > > else > > ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl); > > > > @@ -1938,6 +2037,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > return ret; > > } > > > > +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, > > + struct v4l2_ext_control *xctrl, > > + struct uvc_control *ctrl) > > +{ > > + u8 *data; > > + int ret; > > + > > + if (xctrl->size != mapping->v4l2_size / 8) > > + return -EINVAL; > > + > > + data = kmalloc(xctrl->size, GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + ret = copy_from_user(data, xctrl->ptr, xctrl->size); > > + if (ret < 0) > > + goto out; > > + > > + ret = mapping->set_compound(mapping, data, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > + > > + __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl); > > Why do you need to call __uvc_ctrl_get_compound() here ? > I was trying to copy the clamped control value back to the user. I guess we should do it in uvc_ctrl_set() after the refactor. > > + > > +out: > > + kfree(data); > > + return ret; > > +} > > + > > int uvc_ctrl_set(struct uvc_fh *handle, > > struct v4l2_ext_control *xctrl) > > { > > @@ -2052,13 +2179,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > ctrl->info.size); > > } > > > > - if (uvc_ctrl_mapping_is_compound(mapping)) > > - return -EINVAL; > > - else > > + if (uvc_ctrl_mapping_is_compound(mapping)) { > > + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); > > + if (ret < 0) > > + return ret; > > + } else > > mapping->set(mapping, value, > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > I really don't like how handling of compound controls is scattered > everywhere :-( That makes the code difficult to read, and thus more > error-prone. > > Could this be refactored to simplify the implementation ? In particular, > I'm thinking about > > - Moving the copy_from_user() and copy_to_user() towards the top of the > call stack, directly in uvc_ctrl_get(), uvc_ctrl_get_boundary() and > uvc_ctrl_set() > Sorry, I'm not sure I understand it. For the uvc_ctrl_get() case, do you mean we should get rid of __uvc_ctrl_get_compound_cur() and call __uvc_ctrl_load_cur(), mapping->get() and copy_to_user() directly in uvc_ctrl_get()? Best, Yunke > - Merging the .[gs]et() and .[gs]et_compound() functions (see below) > > - Moving the value clamping from uvc_set_compound_rect() to > uvc_ctrl_set(), with the rest of the clamping code > > > > > - > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > ctrl->handle = handle; > > > > @@ -2468,10 +2596,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > > goto err_nomem; > > } > > > > - if (map->get == NULL) > > + if (uvc_ctrl_mapping_is_compound(map)) { > > + if (map->data_size != map->v4l2_size) > > + return -EINVAL; > > If the two values have to be the same, why do we have two fields ? > > > + > > + /* Only supports byte-aligned data. */ > > + if (WARN_ON(map->offset % 8 || map->data_size % 8)) > > + return -EINVAL; > > + } > > + > > + if (!map->get && !uvc_ctrl_mapping_is_compound(map)) > > map->get = uvc_get_le_value; > > - if (map->set == NULL) > > + if (!map->set && !uvc_ctrl_mapping_is_compound(map)) > > map->set = uvc_set_le_value; > > + if (!map->get_compound && uvc_ctrl_mapping_is_compound(map)) > > + map->get_compound = uvc_get_compound; > > + if (!map->set_compound && uvc_ctrl_mapping_is_compound(map)) > > + map->set_compound = uvc_set_compound; > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 7bc41270ed94..11805b729c22 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -129,8 +129,12 @@ struct uvc_control_mapping { > > > > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > > const u8 *data); > > + int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data, > > + u8 *data_out); > > void (*set)(struct uvc_control_mapping *mapping, s32 value, > > u8 *data); > > + int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in, > > + u8 *data); > > Instead of adding new functions, I think we could modify the existing > .get() and .set() handlers to be usable for compound controls. > > int (*get)(struct uvc_control_mapping *mapping, u8 query, > const u8 *data_in, void *data_out); > void (*set)(struct uvc_control_mapping *mapping, const void *data_in, > u8 *data_out); > > For additional safety, you could pass the size of the void * buffer to > the functions. > > > }; > > > > struct uvc_control { > > -- > Regards, > > Laurent Pinchart
Hi Yunke, On Wed, Dec 13, 2023 at 04:38:40PM +0900, Yunke Cao wrote: > On Sat, Dec 9, 2023 at 12:07 AM Laurent Pinchart wrote: > > On Fri, Dec 01, 2023 at 04:18:56PM +0900, Yunke Cao wrote: > > > Supports getting/setting current value. > > > Supports getting default value. > > > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND. > > > > Please write a better commit message. > > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > --- > > > Changelog since v11: > > > - No change. > > > Changelog since v10: > > > - Rewrite some logic in __uvc_find_control(). > > > - Remove a duplicate check in __uvc_ctrl_get_compound_cur(). > > > - Thanks, Daniel! > > > Changelog since v9: > > > - Make __uvc_ctrl_set_compound() static. > > > Changelog since v8: > > > - No change. > > > Changelog since v7: > > > - Fixed comments styles, indentation and a few other style issues. > > > - Renamed uvc_g/set_array() to uvc_g/set_compound(). > > > - Moved size check to __uvc_ctrl_add_mapping(). > > > - After setting a new value, copy it back to user. > > > - In __uvc_ctrl_set_compound(), check size before allocating. > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 179 +++++++++++++++++++++++++++---- > > > drivers/media/usb/uvc/uvcvideo.h | 4 + > > > 2 files changed, 164 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index 98254b93eb46..aae2480496b7 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -884,6 +884,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > > > } > > > } > > > > > > +/* > > > + * Extract the byte array specified by mapping->offset and mapping->data_size > > > + * stored at 'data' to the output array 'data_out'. > > > + */ > > > +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data, > > > + u8 *data_out) > > > +{ > > > + memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8); > > > + return 0; > > > +} > > > + > > > +/* > > > + * Copy the byte array 'data_in' to the destination specified by mapping->offset > > > + * and mapping->data_size stored at 'data'. > > > + */ > > > +static int uvc_set_compound(struct uvc_control_mapping *mapping, > > > + const u8 *data_in, u8 *data) > > > +{ > > > + memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8); > > > + return 0; > > > +} > > > + > > > > I may be missing something, but isn't this essentially dead code ? This > > series adds support for a single compound control, with custom get/set > > handlers, so these two default handlers will never be called. > > > > I don't think it makes sense to have default handlers for compound > > controls, I don't foresee any use case where a simple mempcy() will do > > the right thing. > > Yes, they are dead code. I thought they would be useful for some > controls. I will remove them in v15. > > > > static bool > > > uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) > > > { > > > @@ -906,7 +928,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, > > > > > > static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > > struct uvc_control_mapping **mapping, struct uvc_control **control, > > > - int next) > > > + int next, int next_compound) > > > { > > > struct uvc_control *ctrl; > > > struct uvc_control_mapping *map; > > > @@ -921,14 +943,16 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > > continue; > > > > > > list_for_each_entry(map, &ctrl->info.mappings, list) { > > > - if ((map->id == v4l2_id) && !next) { > > > + if (map->id == v4l2_id && !next && !next_compound) { > > > *control = ctrl; > > > *mapping = map; > > > return; > > > } > > > > > > if ((*mapping == NULL || (*mapping)->id > map->id) && > > > - (map->id > v4l2_id) && next) { > > > + (map->id > v4l2_id) && > > > + (uvc_ctrl_mapping_is_compound(map) ? > > > + next_compound : next)) { > > > *control = ctrl; > > > *mapping = map; > > > } > > > @@ -942,6 +966,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > > struct uvc_control *ctrl = NULL; > > > struct uvc_entity *entity; > > > int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > > + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; > > > > > > *mapping = NULL; > > > > > > @@ -950,12 +975,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > > > > > /* Find the control. */ > > > list_for_each_entry(entity, &chain->entities, chain) { > > > - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); > > > - if (ctrl && !next) > > > + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, > > > + next_compound); > > > + if (ctrl && !next && !next_compound) > > > return ctrl; > > > } > > > > > > - if (ctrl == NULL && !next) > > > + if (!ctrl && !next && !next_compound) > > > uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", > > > v4l2_id); > > > > > > @@ -1101,12 +1127,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > > > return 0; > > > } > > > > > > +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping, > > > + struct uvc_control *ctrl, > > > + int id, > > > + struct v4l2_ext_control *xctrl) > > > +{ > > > + u8 size; > > > + u8 *data; > > > + int ret; > > > + > > > + size = mapping->v4l2_size / 8; > > > + if (xctrl->size < size) { > > > + xctrl->size = size; > > > + return -ENOSPC; > > > + } > > > + > > > + data = kmalloc(size, GFP_KERNEL); > > > + if (!data) > > > + return -ENOMEM; > > > + > > > + ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data); > > > + if (ret < 0) > > > + goto out; > > > + > > > + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; > > > + > > > +out: > > > + kfree(data); > > > + return ret; > > > +} > > > + > > > +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain, > > > + struct uvc_control *ctrl, > > > + struct uvc_control_mapping *mapping, > > > + struct v4l2_ext_control *xctrl) > > > +{ > > > + int ret; > > > + > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > + if (ret < 0) > > > + return ret; > > > + > > > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, > > > + xctrl); > > > +} > > > + > > > static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, > > > u32 found_id) > > > { > > > - bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > > unsigned int i; > > > > > > + bool find_next = req_id & > > > + (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND); > > > + > > > req_id &= V4L2_CTRL_ID_MASK; > > > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > > @@ -1194,7 +1267,7 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > > } > > > > > > __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > > > - &master_ctrl, 0); > > > + &master_ctrl, 0, 0); > > > > > > if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > > > return 0; > > > @@ -1262,7 +1335,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > > > if (mapping->master_id) > > > __uvc_find_control(ctrl->entity, mapping->master_id, > > > - &master_map, &master_ctrl, 0); > > > + &master_map, &master_ctrl, 0, 0); > > > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > > > s32 val = 0; > > > int ret; > > > @@ -1278,6 +1351,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > > > } > > > > > > + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { > > > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; > > > + v4l2_ctrl->default_value = 0; > > > + v4l2_ctrl->minimum = 0; > > > + v4l2_ctrl->maximum = 0; > > > + v4l2_ctrl->step = 0; > > > + return 0; > > > + } > > > + > > > if (!ctrl->cached) { > > > int ret = uvc_ctrl_populate_cache(chain, ctrl); > > > if (ret < 0) > > > @@ -1533,7 +1615,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > > > s32 val = 0; > > > > > > - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); > > > + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); > > > if (ctrl == NULL) > > > return; > > > > > > @@ -1843,7 +1925,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > > > > > for (i = 0; i < ctrls->count; i++) { > > > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > > > - &ctrl_found, 0); > > > + &ctrl_found, 0, 0); > > > if (uvc_control == ctrl_found) > > > return i; > > > } > > > @@ -1891,7 +1973,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > > return -EINVAL; > > > > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > > - return -EINVAL; > > > + return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl); > > > else > > > return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > > > } > > > @@ -1912,6 +1994,22 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, > > > return 0; > > > } > > > > > > +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain, > > > + struct uvc_control *ctrl, > > > + struct uvc_control_mapping *mapping, > > > + struct v4l2_ext_control *xctrl) > > > +{ > > > + int ret; > > > + > > > + if (!ctrl->cached) { > > > + ret = uvc_ctrl_populate_cache(chain, ctrl); > > > + if (ret < 0) > > > + return ret; > > > + } > > > + > > > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl); > > > +} > > > + > > > int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > > struct v4l2_ext_control *xctrl) > > > { > > > @@ -1929,7 +2027,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > > } > > > > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > > - ret = -EINVAL; > > > + ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping, > > > + xctrl); > > > else > > > ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl); > > > > > > @@ -1938,6 +2037,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > > return ret; > > > } > > > > > > +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, > > > + struct v4l2_ext_control *xctrl, > > > + struct uvc_control *ctrl) > > > +{ > > > + u8 *data; > > > + int ret; > > > + > > > + if (xctrl->size != mapping->v4l2_size / 8) > > > + return -EINVAL; > > > + > > > + data = kmalloc(xctrl->size, GFP_KERNEL); > > > + if (!data) > > > + return -ENOMEM; > > > + > > > + ret = copy_from_user(data, xctrl->ptr, xctrl->size); > > > + if (ret < 0) > > > + goto out; > > > + > > > + ret = mapping->set_compound(mapping, data, > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > + > > > + __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl); > > > > Why do you need to call __uvc_ctrl_get_compound() here ? > > I was trying to copy the clamped control value back to the user. > I guess we should do it in uvc_ctrl_set() after the refactor. I think the code would be easier to understand that way, yes. > > > + > > > +out: > > > + kfree(data); > > > + return ret; > > > +} > > > + > > > int uvc_ctrl_set(struct uvc_fh *handle, > > > struct v4l2_ext_control *xctrl) > > > { > > > @@ -2052,13 +2179,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > ctrl->info.size); > > > } > > > > > > - if (uvc_ctrl_mapping_is_compound(mapping)) > > > - return -EINVAL; > > > - else > > > + if (uvc_ctrl_mapping_is_compound(mapping)) { > > > + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); > > > + if (ret < 0) > > > + return ret; > > > + } else > > > mapping->set(mapping, value, > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > I really don't like how handling of compound controls is scattered > > everywhere :-( That makes the code difficult to read, and thus more > > error-prone. > > > > Could this be refactored to simplify the implementation ? In particular, > > I'm thinking about > > > > - Moving the copy_from_user() and copy_to_user() towards the top of the > > call stack, directly in uvc_ctrl_get(), uvc_ctrl_get_boundary() and > > uvc_ctrl_set() > > Sorry, I'm not sure I understand it. > For the uvc_ctrl_get() case, do you mean we should get rid of > __uvc_ctrl_get_compound_cur() > and call __uvc_ctrl_load_cur(), mapping->get() and copy_to_user() > directly in uvc_ctrl_get()? What I meant is that the copy_from_user() and copy_to_user() calls are hidden deep in the call stack. Would it be possible to move copy_to_user() to the end of uvc_ctrl_get() and uvc_ctrl_set(), to avoid dealing with __user pointers anywhere but at the top level ? I don't mind keeping __uvc_ctrl_get_compound_cur() if it makes sense. > > - Merging the .[gs]et() and .[gs]et_compound() functions (see below) > > > > - Moving the value clamping from uvc_set_compound_rect() to > > uvc_ctrl_set(), with the rest of the clamping code > > > > > > > > - > > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > > ctrl->handle = handle; > > > > > > @@ -2468,10 +2596,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > > > goto err_nomem; > > > } > > > > > > - if (map->get == NULL) > > > + if (uvc_ctrl_mapping_is_compound(map)) { > > > + if (map->data_size != map->v4l2_size) > > > + return -EINVAL; > > > > If the two values have to be the same, why do we have two fields ? > > > > > + > > > + /* Only supports byte-aligned data. */ > > > + if (WARN_ON(map->offset % 8 || map->data_size % 8)) > > > + return -EINVAL; > > > + } > > > + > > > + if (!map->get && !uvc_ctrl_mapping_is_compound(map)) > > > map->get = uvc_get_le_value; > > > - if (map->set == NULL) > > > + if (!map->set && !uvc_ctrl_mapping_is_compound(map)) > > > map->set = uvc_set_le_value; > > > + if (!map->get_compound && uvc_ctrl_mapping_is_compound(map)) > > > + map->get_compound = uvc_get_compound; > > > + if (!map->set_compound && uvc_ctrl_mapping_is_compound(map)) > > > + map->set_compound = uvc_set_compound; > > > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > > if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > index 7bc41270ed94..11805b729c22 100644 > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -129,8 +129,12 @@ struct uvc_control_mapping { > > > > > > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > > > const u8 *data); > > > + int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data, > > > + u8 *data_out); > > > void (*set)(struct uvc_control_mapping *mapping, s32 value, > > > u8 *data); > > > + int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in, > > > + u8 *data); > > > > Instead of adding new functions, I think we could modify the existing > > .get() and .set() handlers to be usable for compound controls. > > > > int (*get)(struct uvc_control_mapping *mapping, u8 query, > > const u8 *data_in, void *data_out); > > void (*set)(struct uvc_control_mapping *mapping, const void *data_in, > > u8 *data_out); > > > > For additional safety, you could pass the size of the void * buffer to > > the functions. > > > > > }; > > > > > > struct uvc_control {
Hi Laurent, On Mon, Dec 18, 2023 at 12:27 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Yunke, > > On Wed, Dec 13, 2023 at 04:38:40PM +0900, Yunke Cao wrote: > > On Sat, Dec 9, 2023 at 12:07 AM Laurent Pinchart wrote: > > > On Fri, Dec 01, 2023 at 04:18:56PM +0900, Yunke Cao wrote: > > > > Supports getting/setting current value. > > > > Supports getting default value. > > > > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND. > > > > > > Please write a better commit message. > > > > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > > --- > > > > Changelog since v11: > > > > - No change. > > > > Changelog since v10: > > > > - Rewrite some logic in __uvc_find_control(). > > > > - Remove a duplicate check in __uvc_ctrl_get_compound_cur(). > > > > - Thanks, Daniel! > > > > Changelog since v9: > > > > - Make __uvc_ctrl_set_compound() static. > > > > Changelog since v8: > > > > - No change. > > > > Changelog since v7: > > > > - Fixed comments styles, indentation and a few other style issues. > > > > - Renamed uvc_g/set_array() to uvc_g/set_compound(). > > > > - Moved size check to __uvc_ctrl_add_mapping(). > > > > - After setting a new value, copy it back to user. > > > > - In __uvc_ctrl_set_compound(), check size before allocating. > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 179 +++++++++++++++++++++++++++---- > > > > drivers/media/usb/uvc/uvcvideo.h | 4 + > > > > 2 files changed, 164 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > > index 98254b93eb46..aae2480496b7 100644 > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > > @@ -884,6 +884,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > > > > } > > > > } > > > > > > > > +/* > > > > + * Extract the byte array specified by mapping->offset and mapping->data_size > > > > + * stored at 'data' to the output array 'data_out'. > > > > + */ > > > > +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data, > > > > + u8 *data_out) > > > > +{ > > > > + memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8); > > > > + return 0; > > > > +} > > > > + > > > > +/* > > > > + * Copy the byte array 'data_in' to the destination specified by mapping->offset > > > > + * and mapping->data_size stored at 'data'. > > > > + */ > > > > +static int uvc_set_compound(struct uvc_control_mapping *mapping, > > > > + const u8 *data_in, u8 *data) > > > > +{ > > > > + memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8); > > > > + return 0; > > > > +} > > > > + > > > > > > I may be missing something, but isn't this essentially dead code ? This > > > series adds support for a single compound control, with custom get/set > > > handlers, so these two default handlers will never be called. > > > > > > I don't think it makes sense to have default handlers for compound > > > controls, I don't foresee any use case where a simple mempcy() will do > > > the right thing. > > > > Yes, they are dead code. I thought they would be useful for some > > controls. I will remove them in v15. > > > > > > static bool > > > > uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) > > > > { > > > > @@ -906,7 +928,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, > > > > > > > > static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > > > struct uvc_control_mapping **mapping, struct uvc_control **control, > > > > - int next) > > > > + int next, int next_compound) > > > > { > > > > struct uvc_control *ctrl; > > > > struct uvc_control_mapping *map; > > > > @@ -921,14 +943,16 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > > > continue; > > > > > > > > list_for_each_entry(map, &ctrl->info.mappings, list) { > > > > - if ((map->id == v4l2_id) && !next) { > > > > + if (map->id == v4l2_id && !next && !next_compound) { > > > > *control = ctrl; > > > > *mapping = map; > > > > return; > > > > } > > > > > > > > if ((*mapping == NULL || (*mapping)->id > map->id) && > > > > - (map->id > v4l2_id) && next) { > > > > + (map->id > v4l2_id) && > > > > + (uvc_ctrl_mapping_is_compound(map) ? > > > > + next_compound : next)) { > > > > *control = ctrl; > > > > *mapping = map; > > > > } > > > > @@ -942,6 +966,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > > > struct uvc_control *ctrl = NULL; > > > > struct uvc_entity *entity; > > > > int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > > > + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; > > > > > > > > *mapping = NULL; > > > > > > > > @@ -950,12 +975,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > > > > > > > /* Find the control. */ > > > > list_for_each_entry(entity, &chain->entities, chain) { > > > > - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); > > > > - if (ctrl && !next) > > > > + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, > > > > + next_compound); > > > > + if (ctrl && !next && !next_compound) > > > > return ctrl; > > > > } > > > > > > > > - if (ctrl == NULL && !next) > > > > + if (!ctrl && !next && !next_compound) > > > > uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", > > > > v4l2_id); > > > > > > > > @@ -1101,12 +1127,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > > > > return 0; > > > > } > > > > > > > > +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping, > > > > + struct uvc_control *ctrl, > > > > + int id, > > > > + struct v4l2_ext_control *xctrl) > > > > +{ > > > > + u8 size; > > > > + u8 *data; > > > > + int ret; > > > > + > > > > + size = mapping->v4l2_size / 8; > > > > + if (xctrl->size < size) { > > > > + xctrl->size = size; > > > > + return -ENOSPC; > > > > + } > > > > + > > > > + data = kmalloc(size, GFP_KERNEL); > > > > + if (!data) > > > > + return -ENOMEM; > > > > + > > > > + ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data); > > > > + if (ret < 0) > > > > + goto out; > > > > + > > > > + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; > > > > + > > > > +out: > > > > + kfree(data); > > > > + return ret; > > > > +} > > > > + > > > > +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain, > > > > + struct uvc_control *ctrl, > > > > + struct uvc_control_mapping *mapping, > > > > + struct v4l2_ext_control *xctrl) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, > > > > + xctrl); > > > > +} > > > > + > > > > static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, > > > > u32 found_id) > > > > { > > > > - bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > > > unsigned int i; > > > > > > > > + bool find_next = req_id & > > > > + (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND); > > > > + > > > > req_id &= V4L2_CTRL_ID_MASK; > > > > > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > > > @@ -1194,7 +1267,7 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > > > } > > > > > > > > __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, > > > > - &master_ctrl, 0); > > > > + &master_ctrl, 0, 0); > > > > > > > > if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) > > > > return 0; > > > > @@ -1262,7 +1335,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > > > > > if (mapping->master_id) > > > > __uvc_find_control(ctrl->entity, mapping->master_id, > > > > - &master_map, &master_ctrl, 0); > > > > + &master_map, &master_ctrl, 0, 0); > > > > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > > > > s32 val = 0; > > > > int ret; > > > > @@ -1278,6 +1351,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > > > > } > > > > > > > > + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { > > > > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; > > > > + v4l2_ctrl->default_value = 0; > > > > + v4l2_ctrl->minimum = 0; > > > > + v4l2_ctrl->maximum = 0; > > > > + v4l2_ctrl->step = 0; > > > > + return 0; > > > > + } > > > > + > > > > if (!ctrl->cached) { > > > > int ret = uvc_ctrl_populate_cache(chain, ctrl); > > > > if (ret < 0) > > > > @@ -1533,7 +1615,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > > > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > > > > s32 val = 0; > > > > > > > > - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); > > > > + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); > > > > if (ctrl == NULL) > > > > return; > > > > > > > > @@ -1843,7 +1925,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > > > > > > > for (i = 0; i < ctrls->count; i++) { > > > > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > > > > - &ctrl_found, 0); > > > > + &ctrl_found, 0, 0); > > > > if (uvc_control == ctrl_found) > > > > return i; > > > > } > > > > @@ -1891,7 +1973,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > > > return -EINVAL; > > > > > > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > > > - return -EINVAL; > > > > + return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl); > > > > else > > > > return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > > > > } > > > > @@ -1912,6 +1994,22 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, > > > > return 0; > > > > } > > > > > > > > +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain, > > > > + struct uvc_control *ctrl, > > > > + struct uvc_control_mapping *mapping, > > > > + struct v4l2_ext_control *xctrl) > > > > +{ > > > > + int ret; > > > > + > > > > + if (!ctrl->cached) { > > > > + ret = uvc_ctrl_populate_cache(chain, ctrl); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > + > > > > + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl); > > > > +} > > > > + > > > > int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > > > struct v4l2_ext_control *xctrl) > > > > { > > > > @@ -1929,7 +2027,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > > > } > > > > > > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > > > - ret = -EINVAL; > > > > + ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping, > > > > + xctrl); > > > > else > > > > ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl); > > > > > > > > @@ -1938,6 +2037,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, > > > > return ret; > > > > } > > > > > > > > +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, > > > > + struct v4l2_ext_control *xctrl, > > > > + struct uvc_control *ctrl) > > > > +{ > > > > + u8 *data; > > > > + int ret; > > > > + > > > > + if (xctrl->size != mapping->v4l2_size / 8) > > > > + return -EINVAL; > > > > + > > > > + data = kmalloc(xctrl->size, GFP_KERNEL); > > > > + if (!data) > > > > + return -ENOMEM; > > > > + > > > > + ret = copy_from_user(data, xctrl->ptr, xctrl->size); > > > > + if (ret < 0) > > > > + goto out; > > > > + > > > > + ret = mapping->set_compound(mapping, data, > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > + > > > > + __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl); > > > > > > Why do you need to call __uvc_ctrl_get_compound() here ? > > > > I was trying to copy the clamped control value back to the user. > > I guess we should do it in uvc_ctrl_set() after the refactor. > > I think the code would be easier to understand that way, yes. > > > > > + > > > > +out: > > > > + kfree(data); > > > > + return ret; > > > > +} > > > > + > > > > int uvc_ctrl_set(struct uvc_fh *handle, > > > > struct v4l2_ext_control *xctrl) > > > > { > > > > @@ -2052,13 +2179,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > > ctrl->info.size); > > > > } > > > > > > > > - if (uvc_ctrl_mapping_is_compound(mapping)) > > > > - return -EINVAL; > > > > - else > > > > + if (uvc_ctrl_mapping_is_compound(mapping)) { > > > > + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); > > > > + if (ret < 0) > > > > + return ret; > > > > + } else > > > > mapping->set(mapping, value, > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > I really don't like how handling of compound controls is scattered > > > everywhere :-( That makes the code difficult to read, and thus more > > > error-prone. > > > > > > Could this be refactored to simplify the implementation ? In particular, > > > I'm thinking about > > > > > > - Moving the copy_from_user() and copy_to_user() towards the top of the > > > call stack, directly in uvc_ctrl_get(), uvc_ctrl_get_boundary() and > > > uvc_ctrl_set() > > > > Sorry, I'm not sure I understand it. > > For the uvc_ctrl_get() case, do you mean we should get rid of > > __uvc_ctrl_get_compound_cur() > > and call __uvc_ctrl_load_cur(), mapping->get() and copy_to_user() > > directly in uvc_ctrl_get()? > > What I meant is that the copy_from_user() and copy_to_user() calls are > hidden deep in the call stack. Would it be possible to move > copy_to_user() to the end of uvc_ctrl_get() and uvc_ctrl_set(), to avoid > dealing with __user pointers anywhere but at the top level ? I don't > mind keeping __uvc_ctrl_get_compound_cur() if it makes sense. > Ah, I see. Let me give it a try. Thanks! Yunke > > > - Merging the .[gs]et() and .[gs]et_compound() functions (see below) > > > > > > - Moving the value clamping from uvc_set_compound_rect() to > > > uvc_ctrl_set(), with the rest of the clamping code > > > > > > > > > > > - > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > > > ctrl->handle = handle; > > > > > > > > @@ -2468,10 +2596,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > > > > goto err_nomem; > > > > } > > > > > > > > - if (map->get == NULL) > > > > + if (uvc_ctrl_mapping_is_compound(map)) { > > > > + if (map->data_size != map->v4l2_size) > > > > + return -EINVAL; > > > > > > If the two values have to be the same, why do we have two fields ? > > > > > > > + > > > > + /* Only supports byte-aligned data. */ > > > > + if (WARN_ON(map->offset % 8 || map->data_size % 8)) > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (!map->get && !uvc_ctrl_mapping_is_compound(map)) > > > > map->get = uvc_get_le_value; > > > > - if (map->set == NULL) > > > > + if (!map->set && !uvc_ctrl_mapping_is_compound(map)) > > > > map->set = uvc_set_le_value; > > > > + if (!map->get_compound && uvc_ctrl_mapping_is_compound(map)) > > > > + map->get_compound = uvc_get_compound; > > > > + if (!map->set_compound && uvc_ctrl_mapping_is_compound(map)) > > > > + map->set_compound = uvc_set_compound; > > > > > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > > > if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > index 7bc41270ed94..11805b729c22 100644 > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > @@ -129,8 +129,12 @@ struct uvc_control_mapping { > > > > > > > > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > > > > const u8 *data); > > > > + int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data, > > > > + u8 *data_out); > > > > void (*set)(struct uvc_control_mapping *mapping, s32 value, > > > > u8 *data); > > > > + int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in, > > > > + u8 *data); > > > > > > Instead of adding new functions, I think we could modify the existing > > > .get() and .set() handlers to be usable for compound controls. > > > > > > int (*get)(struct uvc_control_mapping *mapping, u8 query, > > > const u8 *data_in, void *data_out); > > > void (*set)(struct uvc_control_mapping *mapping, const void *data_in, > > > u8 *data_out); > > > > > > For additional safety, you could pass the size of the void * buffer to > > > the functions. > > > > > > > }; > > > > > > > > struct uvc_control { > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 98254b93eb46..aae2480496b7 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -884,6 +884,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, } } +/* + * Extract the byte array specified by mapping->offset and mapping->data_size + * stored at 'data' to the output array 'data_out'. + */ +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data, + u8 *data_out) +{ + memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8); + return 0; +} + +/* + * Copy the byte array 'data_in' to the destination specified by mapping->offset + * and mapping->data_size stored at 'data'. + */ +static int uvc_set_compound(struct uvc_control_mapping *mapping, + const u8 *data_in, u8 *data) +{ + memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8); + return 0; +} + static bool uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) { @@ -906,7 +928,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, struct uvc_control_mapping **mapping, struct uvc_control **control, - int next) + int next, int next_compound) { struct uvc_control *ctrl; struct uvc_control_mapping *map; @@ -921,14 +943,16 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, continue; list_for_each_entry(map, &ctrl->info.mappings, list) { - if ((map->id == v4l2_id) && !next) { + if (map->id == v4l2_id && !next && !next_compound) { *control = ctrl; *mapping = map; return; } if ((*mapping == NULL || (*mapping)->id > map->id) && - (map->id > v4l2_id) && next) { + (map->id > v4l2_id) && + (uvc_ctrl_mapping_is_compound(map) ? + next_compound : next)) { *control = ctrl; *mapping = map; } @@ -942,6 +966,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, struct uvc_control *ctrl = NULL; struct uvc_entity *entity; int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; *mapping = NULL; @@ -950,12 +975,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, /* Find the control. */ list_for_each_entry(entity, &chain->entities, chain) { - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); - if (ctrl && !next) + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, + next_compound); + if (ctrl && !next && !next_compound) return ctrl; } - if (ctrl == NULL && !next) + if (!ctrl && !next && !next_compound) uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", v4l2_id); @@ -1101,12 +1127,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, return 0; } +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping, + struct uvc_control *ctrl, + int id, + struct v4l2_ext_control *xctrl) +{ + u8 size; + u8 *data; + int ret; + + size = mapping->v4l2_size / 8; + if (xctrl->size < size) { + xctrl->size = size; + return -ENOSPC; + } + + data = kmalloc(size, GFP_KERNEL); + if (!data) + return -ENOMEM; + + ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data); + if (ret < 0) + goto out; + + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; + +out: + kfree(data); + return ret; +} + +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain, + struct uvc_control *ctrl, + struct uvc_control_mapping *mapping, + struct v4l2_ext_control *xctrl) +{ + int ret; + + ret = __uvc_ctrl_load_cur(chain, ctrl); + if (ret < 0) + return ret; + + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, + xctrl); +} + static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, u32 found_id) { - bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; unsigned int i; + bool find_next = req_id & + (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND); + req_id &= V4L2_CTRL_ID_MASK; for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { @@ -1194,7 +1267,7 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, } __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, - &master_ctrl, 0); + &master_ctrl, 0, 0); if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) return 0; @@ -1262,7 +1335,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, if (mapping->master_id) __uvc_find_control(ctrl->entity, mapping->master_id, - &master_map, &master_ctrl, 0); + &master_map, &master_ctrl, 0, 0); if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { s32 val = 0; int ret; @@ -1278,6 +1351,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; } + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; + v4l2_ctrl->default_value = 0; + v4l2_ctrl->minimum = 0; + v4l2_ctrl->maximum = 0; + v4l2_ctrl->step = 0; + return 0; + } + if (!ctrl->cached) { int ret = uvc_ctrl_populate_cache(chain, ctrl); if (ret < 0) @@ -1533,7 +1615,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; s32 val = 0; - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); if (ctrl == NULL) return; @@ -1843,7 +1925,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, for (i = 0; i < ctrls->count; i++) { __uvc_find_control(entity, ctrls->controls[i].id, &mapping, - &ctrl_found, 0); + &ctrl_found, 0, 0); if (uvc_control == ctrl_found) return i; } @@ -1891,7 +1973,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, return -EINVAL; if (uvc_ctrl_mapping_is_compound(mapping)) - return -EINVAL; + return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl); else return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); } @@ -1912,6 +1994,22 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, return 0; } +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain, + struct uvc_control *ctrl, + struct uvc_control_mapping *mapping, + struct v4l2_ext_control *xctrl) +{ + int ret; + + if (!ctrl->cached) { + ret = uvc_ctrl_populate_cache(chain, ctrl); + if (ret < 0) + return ret; + } + + return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl); +} + int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl) { @@ -1929,7 +2027,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, } if (uvc_ctrl_mapping_is_compound(mapping)) - ret = -EINVAL; + ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping, + xctrl); else ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl); @@ -1938,6 +2037,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain, return ret; } +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, + struct v4l2_ext_control *xctrl, + struct uvc_control *ctrl) +{ + u8 *data; + int ret; + + if (xctrl->size != mapping->v4l2_size / 8) + return -EINVAL; + + data = kmalloc(xctrl->size, GFP_KERNEL); + if (!data) + return -ENOMEM; + + ret = copy_from_user(data, xctrl->ptr, xctrl->size); + if (ret < 0) + goto out; + + ret = mapping->set_compound(mapping, data, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); + + __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl); + +out: + kfree(data); + return ret; +} + int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl) { @@ -2052,13 +2179,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, ctrl->info.size); } - if (uvc_ctrl_mapping_is_compound(mapping)) - return -EINVAL; - else + if (uvc_ctrl_mapping_is_compound(mapping)) { + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); + if (ret < 0) + return ret; + } else mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) ctrl->handle = handle; @@ -2468,10 +2596,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, goto err_nomem; } - if (map->get == NULL) + if (uvc_ctrl_mapping_is_compound(map)) { + if (map->data_size != map->v4l2_size) + return -EINVAL; + + /* Only supports byte-aligned data. */ + if (WARN_ON(map->offset % 8 || map->data_size % 8)) + return -EINVAL; + } + + if (!map->get && !uvc_ctrl_mapping_is_compound(map)) map->get = uvc_get_le_value; - if (map->set == NULL) + if (!map->set && !uvc_ctrl_mapping_is_compound(map)) map->set = uvc_set_le_value; + if (!map->get_compound && uvc_ctrl_mapping_is_compound(map)) + map->get_compound = uvc_get_compound; + if (!map->set_compound && uvc_ctrl_mapping_is_compound(map)) + map->set_compound = uvc_set_compound; for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 7bc41270ed94..11805b729c22 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -129,8 +129,12 @@ struct uvc_control_mapping { s32 (*get)(struct uvc_control_mapping *mapping, u8 query, const u8 *data); + int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data, + u8 *data_out); void (*set)(struct uvc_control_mapping *mapping, s32 value, u8 *data); + int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in, + u8 *data); }; struct uvc_control {