Currently, the v4l2 control code is a bit silent on errors.
Add debug messages on (hopefully) most of the error paths.
Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
---
Changes from v1:
* Drop changes in the debug parameter semantics.
* Drop new module debug parameter.
* Add documentation.
* Add a debug error in all places where control can fail.
* Reorder the vdev parameter, to make the patch less invasive.
---
Documentation/media/kapi/v4l2-dev.rst | 1 +
drivers/media/platform/omap3isp/ispvideo.c | 2 +-
drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++-----
drivers/media/v4l2-core/v4l2-ioctl.c | 12 +--
drivers/media/v4l2-core/v4l2-subdev.c | 6 +-
include/media/v4l2-ctrls.h | 9 +-
include/media/v4l2-ioctl.h | 2 +
7 files changed, 100 insertions(+), 38 deletions(-)
diff --git a/Documentation/media/kapi/v4l2-dev.rst b/Documentation/media/kapi/v4l2-dev.rst
index b359f1804bbe..4c5a15c53dbf 100644
--- a/Documentation/media/kapi/v4l2-dev.rst
+++ b/Documentation/media/kapi/v4l2-dev.rst
@@ -288,6 +288,7 @@ Mask Description
0x08 Log the read and write file operations and the VIDIOC_QBUF and
VIDIOC_DQBUF ioctls.
0x10 Log the poll file operation.
+0x20 Log error and messages in the control operations.
===== ================================================================
Video device cleanup
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 175bbed9a235..abc945cc05c9 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
ctrls.count = 1;
ctrls.controls = &ctrl;
- ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, NULL, &ctrls);
+ ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &video->video, NULL, &ctrls);
if (ret < 0) {
dev_warn(isp->dev, "no pixel rate control in subdev %s\n",
pipe->external->name);
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 2d7525e2d9eb..4ee703e04ef2 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -6,6 +6,8 @@
*/
+#define pr_fmt(fmt) "v4l2-ctrls: " fmt
+
#include <linux/ctype.h>
#include <linux/mm.h>
#include <linux/slab.h>
@@ -16,6 +18,12 @@
#include <media/v4l2-event.h>
#include <media/v4l2-dev.h>
+#define dprintk(vdev, fmt, arg...) do { \
+ if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \
+ printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
+ __func__, video_device_node_name(vdev), ##arg); \
+} while (0)
+
#define has_op(master, op) \
(master->ops && master->ops->op)
#define call_op(master, op) \
@@ -3211,6 +3219,7 @@ static int v4l2_ctrl_request_bind(struct media_request *req,
static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
struct v4l2_ctrl_helper *helpers,
+ struct video_device *vdev,
bool get)
{
struct v4l2_ctrl_helper *h;
@@ -3228,20 +3237,28 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
if (cs->which &&
cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
- V4L2_CTRL_ID2WHICH(id) != cs->which)
+ V4L2_CTRL_ID2WHICH(id) != cs->which) {
+ dprintk(vdev, "invalid which 0x%x or control id 0x%x\n", cs->which, id);
return -EINVAL;
+ }
/* Old-style private controls are not allowed for
extended controls */
- if (id >= V4L2_CID_PRIVATE_BASE)
+ if (id >= V4L2_CID_PRIVATE_BASE) {
+ dprintk(vdev, "old-style private controls not allowed for extended controls\n");
return -EINVAL;
+ }
ref = find_ref_lock(hdl, id);
- if (ref == NULL)
+ if (ref == NULL) {
+ dprintk(vdev, "cannot find control id 0x%x\n", id);
return -EINVAL;
+ }
h->ref = ref;
ctrl = ref->ctrl;
- if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
+ if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
+ dprintk(vdev, "control id 0x%x is disabled\n", id);
return -EINVAL;
+ }
if (ctrl->cluster[0]->ncontrols > 1)
have_clusters = true;
@@ -3251,10 +3268,16 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
unsigned tot_size = ctrl->elems * ctrl->elem_size;
if (c->size < tot_size) {
+ /*
+ * In the get case the application first queries
+ * to obtain the size of the control.
+ */
if (get) {
c->size = tot_size;
return -ENOSPC;
}
+ dprintk(vdev, "pointer control id 0x%x size too small, %d bytes but %d bytes needed\n",
+ id, c->size, tot_size);
return -EFAULT;
}
c->size = tot_size;
@@ -3315,7 +3338,8 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
/* Get extended controls. Allocates the helpers array if needed. */
static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
- struct v4l2_ext_controls *cs)
+ struct v4l2_ext_controls *cs,
+ struct video_device *vdev)
{
struct v4l2_ctrl_helper helper[4];
struct v4l2_ctrl_helper *helpers = helper;
@@ -3341,7 +3365,7 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
return -ENOMEM;
}
- ret = prepare_ext_ctrls(hdl, cs, helpers, true);
+ ret = prepare_ext_ctrls(hdl, cs, helpers, vdev, true);
cs->error_idx = cs->count;
for (i = 0; !ret && i < cs->count; i++)
@@ -3434,8 +3458,8 @@ v4l2_ctrls_find_req_obj(struct v4l2_ctrl_handler *hdl,
return obj;
}
-int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
- struct v4l2_ext_controls *cs)
+int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev,
+ struct media_device *mdev, struct v4l2_ext_controls *cs)
{
struct media_request_object *obj = NULL;
struct media_request *req = NULL;
@@ -3471,7 +3495,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
req_obj);
}
- ret = v4l2_g_ext_ctrls_common(hdl, cs);
+ ret = v4l2_g_ext_ctrls_common(hdl, cs, vdev);
if (obj) {
media_request_unlock_for_access(req);
@@ -3614,7 +3638,9 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
/* Validate controls. */
static int validate_ctrls(struct v4l2_ext_controls *cs,
- struct v4l2_ctrl_helper *helpers, bool set)
+ struct v4l2_ctrl_helper *helpers,
+ struct video_device *vdev,
+ bool set)
{
unsigned i;
int ret = 0;
@@ -3626,16 +3652,20 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
cs->error_idx = i;
- if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+ if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+ dprintk(vdev, "control id 0x%x is read-only\n", ctrl->id);
return -EACCES;
+ }
/* This test is also done in try_set_control_cluster() which
is called in atomic context, so that has the final say,
but it makes sense to do an up-front check as well. Once
an error occurs in try_set_control_cluster() some other
controls may have been set already and we want to do a
best-effort to avoid that. */
- if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
+ if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)) {
+ dprintk(vdev, "control id 0x%x is grabbed, cannot set\n", ctrl->id);
return -EBUSY;
+ }
/*
* Skip validation for now if the payload needs to be copied
* from userspace into kernelspace. We'll validate those later.
@@ -3670,7 +3700,8 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
/* Try or try-and-set controls */
static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
struct v4l2_ctrl_handler *hdl,
- struct v4l2_ext_controls *cs, bool set)
+ struct v4l2_ext_controls *cs,
+ struct video_device *vdev, bool set)
{
struct v4l2_ctrl_helper helper[4];
struct v4l2_ctrl_helper *helpers = helper;
@@ -3680,13 +3711,17 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
cs->error_idx = cs->count;
/* Default value cannot be changed */
- if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+ if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ dprintk(vdev, "%s: cannot change default value\n", video_device_node_name(vdev));
return -EINVAL;
+ }
cs->which = V4L2_CTRL_ID2WHICH(cs->which);
- if (hdl == NULL)
+ if (hdl == NULL) {
+ dprintk(vdev, "%s: invalid null control handler\n", video_device_node_name(vdev));
return -EINVAL;
+ }
if (cs->count == 0)
return class_check(hdl, cs->which);
@@ -3697,9 +3732,9 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
if (!helpers)
return -ENOMEM;
}
- ret = prepare_ext_ctrls(hdl, cs, helpers, false);
+ ret = prepare_ext_ctrls(hdl, cs, helpers, vdev, false);
if (!ret)
- ret = validate_ctrls(cs, helpers, set);
+ ret = validate_ctrls(cs, helpers, vdev, set);
if (ret && set)
cs->error_idx = cs->count;
for (i = 0; !ret && i < cs->count; i++) {
@@ -3784,7 +3819,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
}
static int try_set_ext_ctrls(struct v4l2_fh *fh,
- struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+ struct v4l2_ctrl_handler *hdl,
+ struct video_device *vdev, struct media_device *mdev,
struct v4l2_ext_controls *cs, bool set)
{
struct media_request_object *obj = NULL;
@@ -3792,21 +3828,32 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
int ret;
if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
- if (!mdev || cs->request_fd < 0)
+ if (!mdev) {
+ dprintk(vdev, "%s: missing media device\n", video_device_node_name(vdev));
+ return -EINVAL;
+ }
+
+ if (cs->request_fd < 0) {
+ dprintk(vdev, "%s: invalid request fd %d\n", video_device_node_name(vdev), cs->request_fd);
return -EINVAL;
+ }
req = media_request_get_by_fd(mdev, cs->request_fd);
- if (IS_ERR(req))
+ if (IS_ERR(req)) {
+ dprintk(vdev, "%s: cannot find request fd %d\n", video_device_node_name(vdev), cs->request_fd);
return PTR_ERR(req);
+ }
ret = media_request_lock_for_update(req);
if (ret) {
+ dprintk(vdev, "%s: cannot lock request fd %d\n", video_device_node_name(vdev), cs->request_fd);
media_request_put(req);
return ret;
}
obj = v4l2_ctrls_find_req_obj(hdl, req, set);
if (IS_ERR(obj)) {
+ dprintk(vdev, "%s: cannot find request object for request fd %d\n", video_device_node_name(vdev), cs->request_fd);
media_request_unlock_for_update(req);
media_request_put(req);
return PTR_ERR(obj);
@@ -3815,7 +3862,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
req_obj);
}
- ret = try_set_ext_ctrls_common(fh, hdl, cs, set);
+ ret = try_set_ext_ctrls_common(fh, hdl, cs, vdev, set);
+ if (ret)
+ dprintk(vdev, "%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret);
if (obj) {
media_request_unlock_for_update(req);
@@ -3826,17 +3875,22 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
return ret;
}
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+ struct video_device *vdev,
+ struct media_device *mdev,
struct v4l2_ext_controls *cs)
{
- return try_set_ext_ctrls(NULL, hdl, mdev, cs, false);
+ return try_set_ext_ctrls(NULL, hdl, vdev, mdev, cs, false);
}
EXPORT_SYMBOL(v4l2_try_ext_ctrls);
-int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
- struct media_device *mdev, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct v4l2_fh *fh,
+ struct v4l2_ctrl_handler *hdl,
+ struct video_device *vdev,
+ struct media_device *mdev,
+ struct v4l2_ext_controls *cs)
{
- return try_set_ext_ctrls(fh, hdl, mdev, cs, true);
+ return try_set_ext_ctrls(fh, hdl, vdev, mdev, cs, true);
}
EXPORT_SYMBOL(v4l2_s_ext_ctrls);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index b1f4b991dba6..e95efea1a9ca 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2165,9 +2165,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
p->error_idx = p->count;
if (vfh && vfh->ctrl_handler)
- return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
+ return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);
if (vfd->ctrl_handler)
- return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
+ return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);
if (ops->vidioc_g_ext_ctrls == NULL)
return -ENOTTY;
return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :
@@ -2184,9 +2184,9 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
p->error_idx = p->count;
if (vfh && vfh->ctrl_handler)
- return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
+ return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);
if (vfd->ctrl_handler)
- return v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
+ return v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);
if (ops->vidioc_s_ext_ctrls == NULL)
return -ENOTTY;
return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :
@@ -2203,9 +2203,9 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
p->error_idx = p->count;
if (vfh && vfh->ctrl_handler)
- return v4l2_try_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
+ return v4l2_try_ext_ctrls(vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);
if (vfd->ctrl_handler)
- return v4l2_try_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
+ return v4l2_try_ext_ctrls(vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);
if (ops->vidioc_try_ext_ctrls == NULL)
return -ENOTTY;
return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f24978b80440..1b5edd3b1e6c 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -211,19 +211,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
if (!vfh->ctrl_handler)
return -ENOTTY;
return v4l2_g_ext_ctrls(vfh->ctrl_handler,
- sd->v4l2_dev->mdev, arg);
+ vdev, sd->v4l2_dev->mdev, arg);
case VIDIOC_S_EXT_CTRLS:
if (!vfh->ctrl_handler)
return -ENOTTY;
return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler,
- sd->v4l2_dev->mdev, arg);
+ vdev, sd->v4l2_dev->mdev, arg);
case VIDIOC_TRY_EXT_CTRLS:
if (!vfh->ctrl_handler)
return -ENOTTY;
return v4l2_try_ext_ctrls(vfh->ctrl_handler,
- sd->v4l2_dev->mdev, arg);
+ vdev, sd->v4l2_dev->mdev, arg);
case VIDIOC_DQEVENT:
if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index b4433483af23..c08d6cc56743 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -1265,25 +1265,28 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
* :ref:`VIDIOC_G_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl
*
* @hdl: pointer to &struct v4l2_ctrl_handler
+ * @vdev: pointer to &struct video_device
* @mdev: pointer to &struct media_device
* @c: pointer to &struct v4l2_ext_controls
*
* If hdl == NULL then they will all return -EINVAL.
*/
-int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
- struct v4l2_ext_controls *c);
+int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev,
+ struct media_device *mdev, struct v4l2_ext_controls *c);
/**
* v4l2_try_ext_ctrls - Helper function to implement
* :ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl
*
* @hdl: pointer to &struct v4l2_ctrl_handler
+ * @vdev: pointer to &struct video_device
* @mdev: pointer to &struct media_device
* @c: pointer to &struct v4l2_ext_controls
*
* If hdl == NULL then they will all return -EINVAL.
*/
int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+ struct video_device *vdev,
struct media_device *mdev,
struct v4l2_ext_controls *c);
@@ -1293,12 +1296,14 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
*
* @fh: pointer to &struct v4l2_fh
* @hdl: pointer to &struct v4l2_ctrl_handler
+ * @vdev: pointer to &struct video_device
* @mdev: pointer to &struct media_device
* @c: pointer to &struct v4l2_ext_controls
*
* If hdl == NULL then they will all return -EINVAL.
*/
int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+ struct video_device *vdev,
struct media_device *mdev,
struct v4l2_ext_controls *c);
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 400f2e46c108..4bba65a59d46 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -602,6 +602,8 @@ struct v4l2_ioctl_ops {
#define V4L2_DEV_DEBUG_STREAMING 0x08
/* Log poll() */
#define V4L2_DEV_DEBUG_POLL 0x10
+/* Log controls */
+#define V4L2_DEV_DEBUG_CTRL 0x20
/* Video standard functions */
--
2.20.1
