This patchset contains the fixes for the comments on "v10 of Fix
v4l2-compliance errors series". In particular to the patches
-uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
-uvcvideo: improve error handling in uvc_query_ctrl()
And the patch:
-uvcvideo: Fix handling on Bitmask controls
To: Laurent Pinchart <[email protected]>
To: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Hans Verkuil <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
Changes in v2:
- Include "Get menu names from framework series"
https://lore.kernel.org/r/[email protected]
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Hans Verkuil (2):
media: uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
media: uvcvideo: improve error logging in uvc_query_ctrl()
Ricardo Ribalda (5):
media: uvcvideo: Return -EACCES for Wrong state error
media: uvcvideo: Do not return positive errors in uvc_query_ctrl()
media: uvcvideo: Fix handling on Bitmask controls
media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU
media: uvcvideo: Use standard names for menus
drivers/media/usb/uvc/uvc_ctrl.c | 230 ++++++++++++++++++++++++++++---------
drivers/media/usb/uvc/uvc_driver.c | 9 +-
drivers/media/usb/uvc/uvc_v4l2.c | 85 ++++++++++----
drivers/media/usb/uvc/uvc_video.c | 15 +--
drivers/media/usb/uvc/uvcvideo.h | 8 +-
include/uapi/linux/uvcvideo.h | 3 +-
6 files changed, 258 insertions(+), 92 deletions(-)
---
base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6
change-id: 20220920-resend-v4l2-compliance-4fdbe4fbd7b5
Best regards,
--
Ricardo Ribalda <[email protected]>
From: Hans Verkuil <[email protected]>
If __uvc_query_ctrl() failed with a non-EPIPE error, then
report that with dev_err. If an error code is obtained, then
report that with dev_dbg.
Reviewed-by: Ricardo Ribalda <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
---
drivers/media/usb/uvc/uvc_video.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 170a008f4006..2cf7f692c0bb 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -79,13 +79,14 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
if (likely(ret == size))
return 0;
- dev_err(&dev->udev->dev,
- "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
- uvc_query_name(query), cs, unit, ret, size);
-
- if (ret != -EPIPE)
+ if (ret != -EPIPE) {
+ dev_err(&dev->udev->dev,
+ "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
+ uvc_query_name(query), cs, unit, ret, size);
return ret;
+ }
+ /* reuse data[0] to request the error code. */
tmp = *(u8 *)data;
ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
For error 2 (Wrong state) return -EACCES instead of -EILSEQ.
EACCES is a much more appropriate error code. EILSEQ will return
"Invalid or incomplete multibyte or wide character." in strerror(),
which is a *very* confusing message.
Suggested-by: Hans Verkuil <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_video.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 2cf7f692c0bb..497073a50194 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -108,7 +108,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
case 1: /* Not ready */
return -EBUSY;
case 2: /* Wrong state */
- return -EILSEQ;
+ return -EACCES;
case 3: /* Power */
return -EREMOTE;
case 4: /* Out of range */
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
From: Hans Verkuil <[email protected]>
Check for inactive controls in uvc_ctrl_is_accessible().
Use the new value for the master_id controls if present,
otherwise use the existing value to determine if it is OK
to set the control. Doing this here avoids attempting to
set an inactive control, which will return an error from the
USB device, which returns an invalid errorcode.
This fixes:
warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
test VIDIOC_G/S_CTRL: OK
warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
warn: v4l2-test-controls.cpp(816): s_ext_ctrls returned EIO
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
Tested with:
v4l2-ctl -c auto_exposure=1
OK
v4l2-ctl -c exposure_time_absolute=251
OK
v4l2-ctl -c auto_exposure=3
OK
v4l2-ctl -c exposure_time_absolute=251
VIDIOC_S_EXT_CTRLS: failed: Input/output error
exposure_time_absolute: Input/output error
ERROR
v4l2-ctl -c auto_exposure=3,exposure_time_absolute=251,auto_exposure=1
v4l2-ctl -C auto_exposure,exposure_time_absolute
auto_exposure: 1
exposure_time_absolute: 251
Reviewed-by: Ricardo Ribalda <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
---
drivers/media/usb/uvc/uvc_ctrl.c | 47 +++++++++++++++++++++++++++++++++++++++-
drivers/media/usb/uvc/uvc_v4l2.c | 4 ++--
drivers/media/usb/uvc/uvcvideo.h | 3 ++-
3 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 8c208db9600b..7153ee5aabb1 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1064,11 +1064,33 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
return 0;
}
+/**
+ * uvc_ctrl_is_accessible() - Check if a control can be read/writen/tried.
+ * @chain: uvc_video_chain that the controls belong to.
+ * @v4l2_id: video4linux id of the control.
+ * @ctrl: Other controls that will be accessed in the ioctl.
+ * @ioctl: ioctl used to access the control.
+ *
+ * Check if a control can be accessed by a specicific ioctl operation,
+ * assuming that other controls are also going to be accessed by that ioctl.
+ * We need to check the value of the other controls, to support operations
+ * where a master value is changed with a slave value. Eg.
+ * auto_exposure=1,exposure_time_absolute=251
+ *
+ */
int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
- bool read)
+ const struct v4l2_ext_controls *ctrls,
+ unsigned long ioctl)
{
+ struct uvc_control_mapping *master_map = NULL;
+ struct uvc_control *master_ctrl = NULL;
struct uvc_control_mapping *mapping;
struct uvc_control *ctrl;
+ bool read = ioctl == VIDIOC_G_EXT_CTRLS;
+ bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
+ s32 val;
+ int ret;
+ int i;
if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
return -EACCES;
@@ -1083,6 +1105,29 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
return -EACCES;
+ if (read || try || !mapping->master_id)
+ return 0;
+
+ /*
+ * Iterate backwards in cases where the master control is accessed
+ * multiple times in the same ioctl. We want the last value.
+ */
+ for (i = ctrls->count - 1; i >= 0; i--) {
+ if (ctrls->controls[i].id == mapping->master_id)
+ return ctrls->controls[i].value ==
+ mapping->master_manual ? 0 : -EACCES;
+ }
+
+ __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 0;
+
+ ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
+ if (ret >= 0 && val != mapping->master_manual)
+ return -EACCES;
+
return 0;
}
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 4cc3fa6b8c98..d95168cdc2d1 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1020,8 +1020,8 @@ 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_accessible(chain, ctrl->id,
- ioctl == VIDIOC_G_EXT_CTRLS);
+ ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls,
+ ioctl);
if (ret)
break;
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 24c911aeebce..644d5fcf2eef 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -905,7 +905,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_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
- bool read);
+ const struct v4l2_ext_controls *ctrls,
+ unsigned long ioctl);
int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
struct uvc_xu_control_query *xqry);
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
If the returned size of the query does not match the expected size or it
is zero, return -EPIPE instead of 0 or a positive value.
Suggested-by: Laurent Pinchart <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_video.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 497073a50194..902f2817a743 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -83,7 +83,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
dev_err(&dev->udev->dev,
"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
uvc_query_name(query), cs, unit, ret, size);
- return ret;
+ return ret < 0 ? ret : -EPIPE;
}
/* reuse data[0] to request the error code. */
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
Instead of duplicating the menu info, use the one from the core.
Also, do not use extra memory for 1:1 mappings.
Signed-off-by: Ricardo Ribalda <[email protected]>
Suggested-by: Laurent Pinchart <[email protected]>
---
drivers/media/usb/uvc/uvc_ctrl.c | 116 +++++++++++++++++++++++++------------
drivers/media/usb/uvc/uvc_driver.c | 9 +--
drivers/media/usb/uvc/uvc_v4l2.c | 81 ++++++++++++++++++++------
drivers/media/usb/uvc/uvcvideo.h | 3 +-
include/uapi/linux/uvcvideo.h | 3 +-
5 files changed, 146 insertions(+), 66 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index df273b829961..5c28d8aace37 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -363,19 +363,31 @@ static const u32 uvc_control_classes[] = {
V4L2_CID_USER_CLASS,
};
-static const struct uvc_menu_info power_line_frequency_controls[] = {
- { 0, "Disabled" },
- { 1, "50 Hz" },
- { 2, "60 Hz" },
- { 3, "Auto" },
-};
+static const int exposure_auto_mapping[] = { 2, 1, 4, 8 };
-static const struct uvc_menu_info exposure_auto_controls[] = {
- { 2, "Auto Mode" },
- { 1, "Manual Mode" },
- { 4, "Shutter Priority Mode" },
- { 8, "Aperture Priority Mode" },
-};
+static u32 uvc_mapping_get_menu_value(struct uvc_control_mapping *mapping,
+ u32 idx)
+{
+ if (!test_bit(idx, &mapping->menu_mask))
+ return 0;
+
+ if (mapping->menu_mapping)
+ return mapping->menu_mapping[idx];
+
+ return idx;
+}
+
+static const char
+*uvc_mapping_get_menu_name(struct uvc_control_mapping *mapping, u32 idx)
+{
+ if (!test_bit(idx, &mapping->menu_mask))
+ return NULL;
+
+ if (mapping->menu_names)
+ return mapping->menu_names[idx];
+
+ return v4l2_ctrl_get_menu(mapping->id)[idx];
+}
static s32 uvc_ctrl_get_zoom(struct uvc_control_mapping *mapping,
u8 query, const u8 *data)
@@ -524,8 +536,8 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
.offset = 0,
.v4l2_type = V4L2_CTRL_TYPE_MENU,
.data_type = UVC_CTRL_DATA_TYPE_BITMASK,
- .menu_info = exposure_auto_controls,
- .menu_mask = BIT_MASK(ARRAY_SIZE(exposure_auto_controls)),
+ .menu_mapping = exposure_auto_mapping,
+ .menu_mask = GENMASK(ARRAY_SIZE(exposure_auto_mapping) - 1, 0),
.slave_ids = { V4L2_CID_EXPOSURE_ABSOLUTE, },
},
{
@@ -730,8 +742,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings_uvc11[] = {
.offset = 0,
.v4l2_type = V4L2_CTRL_TYPE_MENU,
.data_type = UVC_CTRL_DATA_TYPE_ENUM,
- .menu_info = power_line_frequency_controls,
- .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls) - 1),
+ .menu_mask = GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0),
},
};
@@ -744,8 +755,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings_uvc15[] = {
.offset = 0,
.v4l2_type = V4L2_CTRL_TYPE_MENU,
.data_type = UVC_CTRL_DATA_TYPE_ENUM,
- .menu_info = power_line_frequency_controls,
- .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls)),
+ .menu_mask = GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0),
},
};
@@ -972,13 +982,17 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
s32 value = mapping->get(mapping, UVC_GET_CUR, data);
if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
- const struct uvc_menu_info *menu = mapping->menu_info;
unsigned int i;
- for (i = 0; BIT(i) <= mapping->menu_mask; ++i, ++menu) {
+ for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
+ u32 menu_value;
+
if (!test_bit(i, &mapping->menu_mask))
continue;
- if (menu->value == value) {
+
+ menu_value = uvc_mapping_get_menu_value(mapping, i);
+
+ if (menu_value == value) {
value = i;
break;
}
@@ -1174,7 +1188,6 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
{
struct uvc_control_mapping *master_map = NULL;
struct uvc_control *master_ctrl = NULL;
- const struct uvc_menu_info *menu;
unsigned int i;
memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
@@ -1219,11 +1232,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
v4l2_ctrl->maximum = fls(mapping->menu_mask) - 1;
v4l2_ctrl->step = 1;
- menu = mapping->menu_info;
- for (i = 0; BIT(i) <= mapping->menu_mask; ++i, ++menu) {
+ for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
+ u32 menu_value;
+
if (!test_bit(i, &mapping->menu_mask))
continue;
- if (menu->value == v4l2_ctrl->default_value) {
+
+ menu_value = uvc_mapping_get_menu_value(mapping, i);
+
+ if (menu_value == v4l2_ctrl->default_value) {
v4l2_ctrl->default_value = i;
break;
}
@@ -1322,11 +1339,11 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
struct v4l2_querymenu *query_menu)
{
- const struct uvc_menu_info *menu_info;
struct uvc_control_mapping *mapping;
struct uvc_control *ctrl;
u32 index = query_menu->index;
u32 id = query_menu->id;
+ const char *name;
int ret;
memset(query_menu, 0, sizeof(*query_menu));
@@ -1348,22 +1365,28 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
goto done;
}
- menu_info = &mapping->menu_info[query_menu->index];
-
if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) {
+ u32 menu_value;
+
if (!ctrl->cached) {
ret = uvc_ctrl_populate_cache(chain, ctrl);
if (ret < 0)
goto done;
}
- if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & menu_info->value)) {
+ menu_value = uvc_mapping_get_menu_value(mapping,
+ query_menu->index);
+ if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & menu_value)) {
ret = -EINVAL;
goto done;
}
}
- strscpy(query_menu->name, menu_info->name, sizeof(query_menu->name));
+ name = uvc_mapping_get_menu_name(mapping, query_menu->index);
+ if (name)
+ strscpy(query_menu->name, name, sizeof(query_menu->name));
+ else
+ ret = -EINVAL;
done:
mutex_unlock(&chain->ctrl_mutex);
@@ -1865,7 +1888,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
if (!test_bit(xctrl->value, &mapping->menu_mask))
return -EINVAL;
- value = mapping->menu_info[xctrl->value].value;
+ value = uvc_mapping_get_menu_value(mapping, xctrl->value);
/*
* Valid menu indices are reported by the GET_RES request for
@@ -2311,12 +2334,28 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
INIT_LIST_HEAD(&map->ev_subs);
- size = sizeof(*mapping->menu_info) * fls(mapping->menu_mask);
- map->menu_info = kmemdup(mapping->menu_info, size, GFP_KERNEL);
- if (map->menu_info == NULL) {
- kfree(map->name);
- kfree(map);
- return -ENOMEM;
+ if (mapping->menu_mapping && mapping->menu_mask) {
+ size = sizeof(mapping->menu_mapping[0]) *
+ fls(mapping->menu_mask);
+ map->menu_mapping = kmemdup(mapping->menu_mapping, size,
+ GFP_KERNEL);
+ if (!map->menu_mapping) {
+ kfree(map->name);
+ kfree(map);
+ return -ENOMEM;
+ }
+ }
+ if (mapping->menu_names && mapping->menu_mask) {
+ size = sizeof(mapping->menu_names[0]) *
+ fls(mapping->menu_mask);
+ map->menu_names = kmemdup(mapping->menu_names, size,
+ GFP_KERNEL);
+ if (!map->menu_names) {
+ kfree(map->menu_mapping);
+ kfree(map->name);
+ kfree(map);
+ return -ENOMEM;
+ }
}
if (map->get == NULL)
@@ -2661,7 +2700,8 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
list_for_each_entry_safe(mapping, nm, &ctrl->info.mappings, list) {
list_del(&mapping->list);
- kfree(mapping->menu_info);
+ kfree(mapping->menu_names);
+ kfree(mapping->menu_mapping);
kfree(mapping->name);
kfree(mapping);
}
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index abdb9ca7eed6..97c267e75fa4 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2661,11 +2661,6 @@ MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
* Driver initialization and cleanup
*/
-static const struct uvc_menu_info power_line_frequency_controls_limited[] = {
- { 1, "50 Hz" },
- { 2, "60 Hz" },
-};
-
static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
.id = V4L2_CID_POWER_LINE_FREQUENCY,
.entity = UVC_GUID_UVC_PROCESSING,
@@ -2674,8 +2669,8 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
.offset = 0,
.v4l2_type = V4L2_CTRL_TYPE_MENU,
.data_type = UVC_CTRL_DATA_TYPE_ENUM,
- .menu_info = power_line_frequency_controls_limited,
- .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls_limited)),
+ .menu_mask = GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
+ V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
};
static const struct uvc_device_info uvc_ctrl_power_line_limited = {
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index e6792fd46bf5..89581c1559df 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -25,6 +25,64 @@
#include "uvcvideo.h"
+static int uvc_control_xu_2_mapping(struct uvc_control_mapping *map,
+ struct uvc_xu_control_mapping *xmap)
+{
+ char (*names)[UVC_MENU_NAME_LEN];
+ unsigned int i;
+ u32 *mapping;
+ size_t size;
+
+ /* Prevent excessive memory consumption, as well as integer
+ * overflows.
+ */
+ if (xmap->menu_count == 0 ||
+ xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES)
+ return -EINVAL;
+
+ map->menu_mask = BIT_MASK(xmap->menu_count);
+
+ size = xmap->menu_count * sizeof(*map->menu_mapping);
+ mapping = kzalloc(size, GFP_KERNEL);
+ if (!mapping)
+ return -ENOMEM;
+
+ for (i = 0; i < xmap->menu_count ; i++)
+ if (copy_from_user(&mapping[i], &xmap->menu_info[i].value,
+ sizeof(mapping[i]))) {
+ kfree(mapping);
+ return -ENOMEM;
+ }
+
+ map->menu_mapping = mapping;
+
+ /*
+ * Always use the standard naming if available.
+ */
+ if (v4l2_ctrl_get_menu(map->id))
+ return 0;
+
+ size = xmap->menu_count * sizeof(map->menu_names[0]);
+ names = kzalloc(size, GFP_KERNEL);
+ if (!names) {
+ kfree(mapping);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < xmap->menu_count ; i++) {
+ /* sizeof(names[i]) - 1: to take care of \0 */
+ if (copy_from_user(&names[i], &xmap->menu_info[i].name,
+ sizeof(names[i]) - 1)) {
+ kfree(names);
+ kfree(mapping);
+ return -ENOMEM;
+ }
+ }
+ map->menu_names = names;
+
+ return 0;
+}
+
/* ------------------------------------------------------------------------
* UVC ioctls
*/
@@ -32,7 +90,6 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
struct uvc_xu_control_mapping *xmap)
{
struct uvc_control_mapping *map;
- unsigned int size;
int ret;
map = kzalloc(sizeof(*map), GFP_KERNEL);
@@ -63,24 +120,9 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
break;
case V4L2_CTRL_TYPE_MENU:
- /*
- * Prevent excessive memory consumption, as well as integer
- * overflows.
- */
- if (xmap->menu_count == 0 ||
- xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
- ret = -EINVAL;
- goto free_map;
- }
-
- size = xmap->menu_count * sizeof(*map->menu_info);
- map->menu_info = memdup_user(xmap->menu_info, size);
- if (IS_ERR(map->menu_info)) {
- ret = PTR_ERR(map->menu_info);
+ ret = uvc_control_xu_2_mapping(map, xmap);
+ if (ret)
goto free_map;
- }
-
- map->menu_mask = BIT_MASK(xmap->menu_count);
break;
default:
@@ -92,7 +134,8 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
ret = uvc_ctrl_add_mapping(chain, map);
- kfree(map->menu_info);
+ kfree(map->menu_names);
+ kfree(map->menu_mapping);
free_map:
kfree(map);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7e2339fc256e..8ebd7ee2934d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -254,7 +254,8 @@ struct uvc_control_mapping {
enum v4l2_ctrl_type v4l2_type;
u32 data_type;
- const struct uvc_menu_info *menu_info;
+ const u32 *menu_mapping;
+ const char (*menu_names)[UVC_MENU_NAME_LEN];
unsigned long menu_mask;
u32 master_id;
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 8288137387c0..1b64b6aa40b5 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -36,9 +36,10 @@
UVC_CTRL_FLAG_GET_MAX | UVC_CTRL_FLAG_GET_RES | \
UVC_CTRL_FLAG_GET_DEF)
+#define UVC_MENU_NAME_LEN 32
struct uvc_menu_info {
__u32 value;
- __u8 name[32];
+ __u8 name[UVC_MENU_NAME_LEN];
};
struct uvc_xu_control_mapping {
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
Hi Ricardo and Hans,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:21:36PM +0100, Ricardo Ribalda wrote:
> From: Hans Verkuil <[email protected]>
>
> If __uvc_query_ctrl() failed with a non-EPIPE error, then
> report that with dev_err. If an error code is obtained, then
> report that with dev_dbg.
The commit message should explain why this is desirable.
> Reviewed-by: Ricardo Ribalda <[email protected]>
> Signed-off-by: Hans Verkuil <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_video.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 170a008f4006..2cf7f692c0bb 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -79,13 +79,14 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> if (likely(ret == size))
> return 0;
>
> - dev_err(&dev->udev->dev,
> - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> - uvc_query_name(query), cs, unit, ret, size);
> -
> - if (ret != -EPIPE)
> + if (ret != -EPIPE) {
> + dev_err(&dev->udev->dev,
> + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> + uvc_query_name(query), cs, unit, ret, size);
> return ret;
> + }
>
> + /* reuse data[0] to request the error code. */
s/reuse/Reuse/
> tmp = *(u8 *)data;
>
> ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
>
--
Regards,
Laurent Pinchart
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:21:38PM +0100, Ricardo Ribalda wrote:
> If the returned size of the query does not match the expected size or it
> is zero, return -EPIPE instead of 0 or a positive value.
The commit message should explain why: this will avoid confusing the
caller (and ultimately userspace) that doesn't expect a positive or zero
value.
> Suggested-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
I'll update the commit message locally.
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_video.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 497073a50194..902f2817a743 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -83,7 +83,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> dev_err(&dev->udev->dev,
> "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> uvc_query_name(query), cs, unit, ret, size);
> - return ret;
> + return ret < 0 ? ret : -EPIPE;
> }
>
> /* reuse data[0] to request the error code. */
>
--
Regards,
Laurent Pinchart
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:21:37PM +0100, Ricardo Ribalda wrote:
> For error 2 (Wrong state) return -EACCES instead of -EILSEQ.
> EACCES is a much more appropriate error code. EILSEQ will return
> "Invalid or incomplete multibyte or wide character." in strerror(),
> which is a *very* confusing message.
Unless there's an objection, I'd like to use the following text to
replace the commit message to provide more information:
Error 2 is defined by UVC as
Wrong State: The device is in a state that disallows the specific
request. The device will remain in this state until a specific action
from the host or the user is completed.
This is documented as happening happen when attempting to set the value
of a manual control when the device is in auto mode. While V4L2 allows
this, the closest error code defined by VIDIOC_S_CTRL is indeed EACCES:
EACCES
Attempt to set a read-only control or to get a write-only control.
Or if there is an attempt to set an inactive control and the driver
is not capable of caching the new value until the control is active
again.
Replace EILSEQ with EACCESS.
> Suggested-by: Hans Verkuil <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_video.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 2cf7f692c0bb..497073a50194 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -108,7 +108,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> case 1: /* Not ready */
> return -EBUSY;
> case 2: /* Wrong state */
> - return -EILSEQ;
> + return -EACCES;
> case 3: /* Power */
> return -EREMOTE;
> case 4: /* Out of range */
>
--
Regards,
Laurent Pinchart
Hi Ricardo and Hans,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:21:35PM +0100, Ricardo Ribalda wrote:
> From: Hans Verkuil <[email protected]>
>
> Check for inactive controls in uvc_ctrl_is_accessible().
> Use the new value for the master_id controls if present,
> otherwise use the existing value to determine if it is OK
> to set the control. Doing this here avoids attempting to
> set an inactive control, which will return an error from the
> USB device, which returns an invalid errorcode.
I'd write the subject line as
media: uvcvideo: Check for INACTIVE in uvc_ctrl_is_accessible()
and reflow the commit message to 72 columns here.
> This fixes:
> warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
> warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
> test VIDIOC_G/S_CTRL: OK
> warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
> warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
> warn: v4l2-test-controls.cpp(816): s_ext_ctrls returned EIO
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
>
> Tested with:
> v4l2-ctl -c auto_exposure=1
> OK
> v4l2-ctl -c exposure_time_absolute=251
> OK
> v4l2-ctl -c auto_exposure=3
> OK
> v4l2-ctl -c exposure_time_absolute=251
> VIDIOC_S_EXT_CTRLS: failed: Input/output error
> exposure_time_absolute: Input/output error
> ERROR
> v4l2-ctl -c auto_exposure=3,exposure_time_absolute=251,auto_exposure=1
> v4l2-ctl -C auto_exposure,exposure_time_absolute
> auto_exposure: 1
> exposure_time_absolute: 251
>
> Reviewed-by: Ricardo Ribalda <[email protected]>
> Signed-off-by: Hans Verkuil <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 47 +++++++++++++++++++++++++++++++++++++++-
> drivers/media/usb/uvc/uvc_v4l2.c | 4 ++--
> drivers/media/usb/uvc/uvcvideo.h | 3 ++-
> 3 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 8c208db9600b..7153ee5aabb1 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1064,11 +1064,33 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
> return 0;
> }
>
> +/**
> + * uvc_ctrl_is_accessible() - Check if a control can be read/writen/tried.
> + * @chain: uvc_video_chain that the controls belong to.
> + * @v4l2_id: video4linux id of the control.
> + * @ctrl: Other controls that will be accessed in the ioctl.
> + * @ioctl: ioctl used to access the control.
The driver doesn't use kerneldoc anywhere, so to match the current style
I'd use /* instead of /**, and drop the documentation of the function
arguments as it's trivial.
> + *
> + * Check if a control can be accessed by a specicific ioctl operation,
s/specicific/specific/
> + * assuming that other controls are also going to be accessed by that ioctl.
> + * We need to check the value of the other controls, to support operations
> + * where a master value is changed with a slave value. Eg.
> + * auto_exposure=1,exposure_time_absolute=251
> + *
Extra blank line.
I'd rephrase that a bit to make it more precise:
* Check if control @v4l2_id can be accessed by the given control @ioctl
* (VIDIOC_G_EXT_CTRLS, VIDIOC_TRY_EXT_CTRLS or VIDIOC_S_EXT_CTRLS).
*
* For set operations on slave controls, check if the master's value is set to
* manual, either in the others controls set in the same ioctl call, or from the
* master's current value. This catches VIDIOC_S_EXT_CTRLS calls that
* set both the master and slave control, such as for instance setting
* auto_exposure=1,exposure_time_absolute=251.
> + */
> int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> - bool read)
> + const struct v4l2_ext_controls *ctrls,
> + unsigned long ioctl)
> {
> + struct uvc_control_mapping *master_map = NULL;
> + struct uvc_control *master_ctrl = NULL;
> struct uvc_control_mapping *mapping;
> struct uvc_control *ctrl;
> + bool read = ioctl == VIDIOC_G_EXT_CTRLS;
> + bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
> + s32 val;
> + int ret;
> + int i;
>
> if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
> return -EACCES;
> @@ -1083,6 +1105,29 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
> return -EACCES;
>
> + if (read || try || !mapping->master_id)
I'd write this
if (ioctl != VIDIOC_S_EXT_CTRLS || !mapping->master_id)
and drop the try variable.
> + return 0;
> +
> + /*
> + * Iterate backwards in cases where the master control is accessed
> + * multiple times in the same ioctl. We want the last value.
> + */
> + for (i = ctrls->count - 1; i >= 0; i--) {
> + if (ctrls->controls[i].id == mapping->master_id)
> + return ctrls->controls[i].value ==
> + mapping->master_manual ? 0 : -EACCES;
> + }
> +
> + __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 0;
> +
> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> + if (ret >= 0 && val != mapping->master_manual)
> + return -EACCES;
> +
> return 0;
> }
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 4cc3fa6b8c98..d95168cdc2d1 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1020,8 +1020,8 @@ 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_accessible(chain, ctrl->id,
> - ioctl == VIDIOC_G_EXT_CTRLS);
> + ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls,
> + ioctl);
This holds on a single line.
Conditionally-Reviewed-by: Laurent Pinchart <[email protected]>
I can address all the review comments when applying.
> if (ret)
> break;
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 24c911aeebce..644d5fcf2eef 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -905,7 +905,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_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> - bool read);
> + const struct v4l2_ext_controls *ctrls,
> + unsigned long ioctl);
>
> int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> struct uvc_xu_control_query *xqry);
>
--
Regards,
Laurent Pinchart
Hi Ricardo,
On Fri, Dec 02, 2022 at 06:21:34PM +0100, Ricardo Ribalda wrote:
> This patchset contains the fixes for the comments on "v10 of Fix
> v4l2-compliance errors series". In particular to the patches
>
> -uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
> -uvcvideo: improve error handling in uvc_query_ctrl()
>
> And the patch:
> -uvcvideo: Fix handling on Bitmask controls
Patches 1/7, 3/7, 4/7 and 6/7 are fine (possibly with changes mentioned
in my review comments that I can handle when applying). I can apply them
to my tree already (with a minor conflict resolution between 2/7 and
3/7), but it may be easier for you to send a v3 of the whole series.
Please let me know what you'd prefer.
> To: Laurent Pinchart <[email protected]>
> To: Mauro Carvalho Chehab <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Hans Verkuil <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
>
> ---
> Changes in v2:
> - Include "Get menu names from framework series"
> https://lore.kernel.org/r/[email protected]
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> Hans Verkuil (2):
> media: uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
> media: uvcvideo: improve error logging in uvc_query_ctrl()
>
> Ricardo Ribalda (5):
> media: uvcvideo: Return -EACCES for Wrong state error
> media: uvcvideo: Do not return positive errors in uvc_query_ctrl()
> media: uvcvideo: Fix handling on Bitmask controls
> media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU
> media: uvcvideo: Use standard names for menus
>
> drivers/media/usb/uvc/uvc_ctrl.c | 230 ++++++++++++++++++++++++++++---------
> drivers/media/usb/uvc/uvc_driver.c | 9 +-
> drivers/media/usb/uvc/uvc_v4l2.c | 85 ++++++++++----
> drivers/media/usb/uvc/uvc_video.c | 15 +--
> drivers/media/usb/uvc/uvcvideo.h | 8 +-
> include/uapi/linux/uvcvideo.h | 3 +-
> 6 files changed, 258 insertions(+), 92 deletions(-)
> ---
> base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6
> change-id: 20220920-resend-v4l2-compliance-4fdbe4fbd7b5
--
Regards,
Laurent Pinchart
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:21:41PM +0100, Ricardo Ribalda wrote:
> Instead of duplicating the menu info, use the one from the core.
> Also, do not use extra memory for 1:1 mappings.
>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> Suggested-by: Laurent Pinchart <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 116 +++++++++++++++++++++++++------------
> drivers/media/usb/uvc/uvc_driver.c | 9 +--
> drivers/media/usb/uvc/uvc_v4l2.c | 81 ++++++++++++++++++++------
> drivers/media/usb/uvc/uvcvideo.h | 3 +-
> include/uapi/linux/uvcvideo.h | 3 +-
> 5 files changed, 146 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index df273b829961..5c28d8aace37 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -363,19 +363,31 @@ static const u32 uvc_control_classes[] = {
> V4L2_CID_USER_CLASS,
> };
>
> -static const struct uvc_menu_info power_line_frequency_controls[] = {
> - { 0, "Disabled" },
> - { 1, "50 Hz" },
> - { 2, "60 Hz" },
> - { 3, "Auto" },
> -};
> +static const int exposure_auto_mapping[] = { 2, 1, 4, 8 };
>
> -static const struct uvc_menu_info exposure_auto_controls[] = {
> - { 2, "Auto Mode" },
> - { 1, "Manual Mode" },
> - { 4, "Shutter Priority Mode" },
> - { 8, "Aperture Priority Mode" },
> -};
> +static u32 uvc_mapping_get_menu_value(struct uvc_control_mapping *mapping,
The mapping argument can be const.
> + u32 idx)
> +{
> + if (!test_bit(idx, &mapping->menu_mask))
> + return 0;
0 is a valid menu value, are all callers fine with not being able to
differentiate it from an error ?
> +
> + if (mapping->menu_mapping)
> + return mapping->menu_mapping[idx];
> +
> + return idx;
> +}
> +
> +static const char
> +*uvc_mapping_get_menu_name(struct uvc_control_mapping *mapping, u32 idx)
Here too. And the * belongs to the previous line.
> +{
> + if (!test_bit(idx, &mapping->menu_mask))
> + return NULL;
> +
> + if (mapping->menu_names)
> + return mapping->menu_names[idx];
> +
> + return v4l2_ctrl_get_menu(mapping->id)[idx];
> +}
>
> static s32 uvc_ctrl_get_zoom(struct uvc_control_mapping *mapping,
> u8 query, const u8 *data)
> @@ -524,8 +536,8 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
> .offset = 0,
> .v4l2_type = V4L2_CTRL_TYPE_MENU,
> .data_type = UVC_CTRL_DATA_TYPE_BITMASK,
> - .menu_info = exposure_auto_controls,
> - .menu_mask = BIT_MASK(ARRAY_SIZE(exposure_auto_controls)),
> + .menu_mapping = exposure_auto_mapping,
> + .menu_mask = GENMASK(ARRAY_SIZE(exposure_auto_mapping) - 1, 0),
> .slave_ids = { V4L2_CID_EXPOSURE_ABSOLUTE, },
> },
> {
> @@ -730,8 +742,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings_uvc11[] = {
> .offset = 0,
> .v4l2_type = V4L2_CTRL_TYPE_MENU,
> .data_type = UVC_CTRL_DATA_TYPE_ENUM,
> - .menu_info = power_line_frequency_controls,
> - .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls) - 1),
> + .menu_mask = GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0),
> },
> };
>
> @@ -744,8 +755,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings_uvc15[] = {
> .offset = 0,
> .v4l2_type = V4L2_CTRL_TYPE_MENU,
> .data_type = UVC_CTRL_DATA_TYPE_ENUM,
> - .menu_info = power_line_frequency_controls,
> - .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls)),
> + .menu_mask = GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0),
> },
> };
>
> @@ -972,13 +982,17 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> s32 value = mapping->get(mapping, UVC_GET_CUR, data);
>
> if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> - const struct uvc_menu_info *menu = mapping->menu_info;
> unsigned int i;
>
> - for (i = 0; BIT(i) <= mapping->menu_mask; ++i, ++menu) {
> + for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
> + u32 menu_value;
> +
> if (!test_bit(i, &mapping->menu_mask))
> continue;
> - if (menu->value == value) {
> +
> + menu_value = uvc_mapping_get_menu_value(mapping, i);
> +
> + if (menu_value == value) {
> value = i;
> break;
> }
> @@ -1174,7 +1188,6 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> {
> struct uvc_control_mapping *master_map = NULL;
> struct uvc_control *master_ctrl = NULL;
> - const struct uvc_menu_info *menu;
> unsigned int i;
>
> memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> @@ -1219,11 +1232,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> v4l2_ctrl->maximum = fls(mapping->menu_mask) - 1;
> v4l2_ctrl->step = 1;
>
> - menu = mapping->menu_info;
> - for (i = 0; BIT(i) <= mapping->menu_mask; ++i, ++menu) {
> + for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
> + u32 menu_value;
> +
> if (!test_bit(i, &mapping->menu_mask))
> continue;
> - if (menu->value == v4l2_ctrl->default_value) {
> +
> + menu_value = uvc_mapping_get_menu_value(mapping, i);
> +
> + if (menu_value == v4l2_ctrl->default_value) {
> v4l2_ctrl->default_value = i;
> break;
> }
> @@ -1322,11 +1339,11 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> struct v4l2_querymenu *query_menu)
> {
> - const struct uvc_menu_info *menu_info;
> struct uvc_control_mapping *mapping;
> struct uvc_control *ctrl;
> u32 index = query_menu->index;
> u32 id = query_menu->id;
> + const char *name;
> int ret;
>
> memset(query_menu, 0, sizeof(*query_menu));
> @@ -1348,22 +1365,28 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> goto done;
> }
>
> - menu_info = &mapping->menu_info[query_menu->index];
> -
> if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) {
> + u32 menu_value;
> +
> if (!ctrl->cached) {
> ret = uvc_ctrl_populate_cache(chain, ctrl);
> if (ret < 0)
> goto done;
> }
>
> - if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & menu_info->value)) {
> + menu_value = uvc_mapping_get_menu_value(mapping,
> + query_menu->index);
> + if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & menu_value)) {
> ret = -EINVAL;
> goto done;
> }
> }
>
> - strscpy(query_menu->name, menu_info->name, sizeof(query_menu->name));
> + name = uvc_mapping_get_menu_name(mapping, query_menu->index);
> + if (name)
> + strscpy(query_menu->name, name, sizeof(query_menu->name));
> + else
> + ret = -EINVAL;
if (!name) {
ret = -EINVAL;
goto done;
}
strscpy(query_menu->name, name, sizeof(query_menu->name));
>
> done:
> mutex_unlock(&chain->ctrl_mutex);
> @@ -1865,7 +1888,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> if (!test_bit(xctrl->value, &mapping->menu_mask))
> return -EINVAL;
>
> - value = mapping->menu_info[xctrl->value].value;
> + value = uvc_mapping_get_menu_value(mapping, xctrl->value);
>
> /*
> * Valid menu indices are reported by the GET_RES request for
> @@ -2311,12 +2334,28 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>
> INIT_LIST_HEAD(&map->ev_subs);
>
> - size = sizeof(*mapping->menu_info) * fls(mapping->menu_mask);
> - map->menu_info = kmemdup(mapping->menu_info, size, GFP_KERNEL);
> - if (map->menu_info == NULL) {
> - kfree(map->name);
> - kfree(map);
> - return -ENOMEM;
> + if (mapping->menu_mapping && mapping->menu_mask) {
> + size = sizeof(mapping->menu_mapping[0]) *
> + fls(mapping->menu_mask);
Misleading indentation.
size = sizeof(mapping->menu_mapping[0]) *
fls(mapping->menu_mask);
or better
size = sizeof(mapping->menu_mapping[0])
* fls(mapping->menu_mask);
> + map->menu_mapping = kmemdup(mapping->menu_mapping, size,
> + GFP_KERNEL);
> + if (!map->menu_mapping) {
> + kfree(map->name);
> + kfree(map);
> + return -ENOMEM;
> + }
> + }
> + if (mapping->menu_names && mapping->menu_mask) {
> + size = sizeof(mapping->menu_names[0]) *
> + fls(mapping->menu_mask);
Same here.
> + map->menu_names = kmemdup(mapping->menu_names, size,
> + GFP_KERNEL);
> + if (!map->menu_names) {
> + kfree(map->menu_mapping);
> + kfree(map->name);
> + kfree(map);
> + return -ENOMEM;
> + }
> }
>
> if (map->get == NULL)
> @@ -2661,7 +2700,8 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
>
> list_for_each_entry_safe(mapping, nm, &ctrl->info.mappings, list) {
> list_del(&mapping->list);
> - kfree(mapping->menu_info);
> + kfree(mapping->menu_names);
> + kfree(mapping->menu_mapping);
> kfree(mapping->name);
> kfree(mapping);
That's getting long. You could initialize all the pointer fields of
mapping to NULL just after the kmemdup() call above, which should
simplify all these error paths by allowing all kfree() calls to be moved
to an error label. Maybe this could be done in a patch before this one ?
> }
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index abdb9ca7eed6..97c267e75fa4 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2661,11 +2661,6 @@ MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> * Driver initialization and cleanup
> */
>
> -static const struct uvc_menu_info power_line_frequency_controls_limited[] = {
> - { 1, "50 Hz" },
> - { 2, "60 Hz" },
> -};
> -
> static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> .id = V4L2_CID_POWER_LINE_FREQUENCY,
> .entity = UVC_GUID_UVC_PROCESSING,
> @@ -2674,8 +2669,8 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> .offset = 0,
> .v4l2_type = V4L2_CTRL_TYPE_MENU,
> .data_type = UVC_CTRL_DATA_TYPE_ENUM,
> - .menu_info = power_line_frequency_controls_limited,
> - .menu_mask = BIT_MASK(ARRAY_SIZE(power_line_frequency_controls_limited)),
> + .menu_mask = GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> + V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
> };
>
> static const struct uvc_device_info uvc_ctrl_power_line_limited = {
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index e6792fd46bf5..89581c1559df 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -25,6 +25,64 @@
>
> #include "uvcvideo.h"
>
> +static int uvc_control_xu_2_mapping(struct uvc_control_mapping *map,
> + struct uvc_xu_control_mapping *xmap)
> +{
> + char (*names)[UVC_MENU_NAME_LEN];
> + unsigned int i;
> + u32 *mapping;
> + size_t size;
> +
> + /* Prevent excessive memory consumption, as well as integer
Wrong comment style.
> + * overflows.
> + */
> + if (xmap->menu_count == 0 ||
> + xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES)
> + return -EINVAL;
> +
> + map->menu_mask = BIT_MASK(xmap->menu_count);
> +
> + size = xmap->menu_count * sizeof(*map->menu_mapping);
> + mapping = kzalloc(size, GFP_KERNEL);
> + if (!mapping)
> + return -ENOMEM;
> +
> + for (i = 0; i < xmap->menu_count ; i++)
Missing braces.
> + if (copy_from_user(&mapping[i], &xmap->menu_info[i].value,
> + sizeof(mapping[i]))) {
> + kfree(mapping);
> + return -ENOMEM;
> + }
> +
> + map->menu_mapping = mapping;
> +
> + /*
> + * Always use the standard naming if available.
Holds on a single line.
> + */
> + if (v4l2_ctrl_get_menu(map->id))
> + return 0;
> +
> + size = xmap->menu_count * sizeof(map->menu_names[0]);
> + names = kzalloc(size, GFP_KERNEL);
> + if (!names) {
> + kfree(mapping);
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < xmap->menu_count ; i++) {
> + /* sizeof(names[i]) - 1: to take care of \0 */
> + if (copy_from_user(&names[i], &xmap->menu_info[i].name,
> + sizeof(names[i]) - 1)) {
> + kfree(names);
> + kfree(mapping);
> + return -ENOMEM;
map->menu_mapping isn't NULL when you return, that's asking for problem
later.
> + }
> + }
Add a blank line.
> + map->menu_names = names;
> +
> + return 0;
> +}
> +
> /* ------------------------------------------------------------------------
> * UVC ioctls
> */
> @@ -32,7 +90,6 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
> struct uvc_xu_control_mapping *xmap)
> {
> struct uvc_control_mapping *map;
> - unsigned int size;
> int ret;
>
> map = kzalloc(sizeof(*map), GFP_KERNEL);
> @@ -63,24 +120,9 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
> break;
>
> case V4L2_CTRL_TYPE_MENU:
> - /*
> - * Prevent excessive memory consumption, as well as integer
> - * overflows.
> - */
> - if (xmap->menu_count == 0 ||
> - xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
> - ret = -EINVAL;
> - goto free_map;
> - }
> -
> - size = xmap->menu_count * sizeof(*map->menu_info);
> - map->menu_info = memdup_user(xmap->menu_info, size);
> - if (IS_ERR(map->menu_info)) {
> - ret = PTR_ERR(map->menu_info);
> + ret = uvc_control_xu_2_mapping(map, xmap);
That's not a great function name, as uvc_ioctl_ctrl_map() deals with XUs
only.
> + if (ret)
> goto free_map;
> - }
> -
> - map->menu_mask = BIT_MASK(xmap->menu_count);
> break;
>
> default:
> @@ -92,7 +134,8 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>
> ret = uvc_ctrl_add_mapping(chain, map);
>
> - kfree(map->menu_info);
> + kfree(map->menu_names);
> + kfree(map->menu_mapping);
Freeing here what you've allocated in a separate function makes me
cringe :-S
> free_map:
> kfree(map);
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 7e2339fc256e..8ebd7ee2934d 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -254,7 +254,8 @@ struct uvc_control_mapping {
> enum v4l2_ctrl_type v4l2_type;
> u32 data_type;
>
> - const struct uvc_menu_info *menu_info;
> + const u32 *menu_mapping;
> + const char (*menu_names)[UVC_MENU_NAME_LEN];
> unsigned long menu_mask;
>
> u32 master_id;
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 8288137387c0..1b64b6aa40b5 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -36,9 +36,10 @@
> UVC_CTRL_FLAG_GET_MAX | UVC_CTRL_FLAG_GET_RES | \
> UVC_CTRL_FLAG_GET_DEF)
>
> +#define UVC_MENU_NAME_LEN 32
A blank line here would be nice.
> struct uvc_menu_info {
> __u32 value;
> - __u8 name[32];
> + __u8 name[UVC_MENU_NAME_LEN];
> };
>
> struct uvc_xu_control_mapping {
--
Regards,
Laurent Pinchart
Hi Laurent
On Thu, 29 Dec 2022 at 23:13, Laurent Pinchart
<[email protected]> wrote:
>
> Hi Ricardo,
>
> On Fri, Dec 02, 2022 at 06:21:34PM +0100, Ricardo Ribalda wrote:
> > This patchset contains the fixes for the comments on "v10 of Fix
> > v4l2-compliance errors series". In particular to the patches
> >
> > -uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
> > -uvcvideo: improve error handling in uvc_query_ctrl()
> >
> > And the patch:
> > -uvcvideo: Fix handling on Bitmask controls
>
> Patches 1/7, 3/7, 4/7 and 6/7 are fine (possibly with changes mentioned
> in my review comments that I can handle when applying). I can apply them
> to my tree already (with a minor conflict resolution between 2/7 and
> 3/7), but it may be easier for you to send a v3 of the whole series.
> Please let me know what you'd prefer.
Just sent a v3 of the patchset. If 1-8 are OK, feel free to to merge
them in your tree, we can add
"Use-standard-names-for-menus" later. It is not needed to pass the
compliance. It is on this set just because it depends on this set.
Thanks!
> > To: Laurent Pinchart <[email protected]>
> > To: Mauro Carvalho Chehab <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: Hans Verkuil <[email protected]>
> > Signed-off-by: Ricardo Ribalda <[email protected]>
> >
> > ---
> > Changes in v2:
> > - Include "Get menu names from framework series"
> > https://lore.kernel.org/r/[email protected]
> > - Link to v1: https://lore.kernel.org/r/[email protected]
> >
> > ---
> > Hans Verkuil (2):
> > media: uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
> > media: uvcvideo: improve error logging in uvc_query_ctrl()
> >
> > Ricardo Ribalda (5):
> > media: uvcvideo: Return -EACCES for Wrong state error
> > media: uvcvideo: Do not return positive errors in uvc_query_ctrl()
> > media: uvcvideo: Fix handling on Bitmask controls
> > media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU
> > media: uvcvideo: Use standard names for menus
> >
> > drivers/media/usb/uvc/uvc_ctrl.c | 230 ++++++++++++++++++++++++++++---------
> > drivers/media/usb/uvc/uvc_driver.c | 9 +-
> > drivers/media/usb/uvc/uvc_v4l2.c | 85 ++++++++++----
> > drivers/media/usb/uvc/uvc_video.c | 15 +--
> > drivers/media/usb/uvc/uvcvideo.h | 8 +-
> > include/uapi/linux/uvcvideo.h | 3 +-
> > 6 files changed, 258 insertions(+), 92 deletions(-)
> > ---
> > base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6
> > change-id: 20220920-resend-v4l2-compliance-4fdbe4fbd7b5
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda