* [PATCH v3] media: v4l: ctrls: Add debug messages
@ 2019-06-22 10:28 Ezequiel Garcia
2019-06-27 7:38 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2019-06-22 10:28 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, kernel, Ezequiel Garcia
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@collabora.com>
---
Changes from v2:
* Check and noisy-warn if vdev is NULL.
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..1c8ae4501870 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 (!WARN_ON(!vdev) && ((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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: v4l: ctrls: Add debug messages
2019-06-22 10:28 [PATCH v3] media: v4l: ctrls: Add debug messages Ezequiel Garcia
@ 2019-06-27 7:38 ` Hans Verkuil
2019-07-19 21:53 ` Ezequiel Garcia
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2019-06-27 7:38 UTC (permalink / raw)
To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, kernel
Hi Ezequiel,
'checkpatch.pl --strict' finds way too many issues.
I'm pretty certain you didn't run it.
Please fix the issues and post a v4.
A lot (but not all) of the warnings report lines that are too long
and from what I can see most are easily fixed without reducing
readability.
Regards,
Hans
On 6/22/19 12:28 PM, Ezequiel Garcia wrote:
> 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@collabora.com>
> ---
> Changes from v2:
> * Check and noisy-warn if vdev is NULL.
>
> 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..1c8ae4501870 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 (!WARN_ON(!vdev) && ((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 */
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: v4l: ctrls: Add debug messages
2019-06-27 7:38 ` Hans Verkuil
@ 2019-07-19 21:53 ` Ezequiel Garcia
2019-07-20 9:36 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2019-07-19 21:53 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: Hans Verkuil, kernel
On Thu, 2019-06-27 at 09:38 +0200, Hans Verkuil wrote:
> Hi Ezequiel,
>
> 'checkpatch.pl --strict' finds way too many issues.
>
> I'm pretty certain you didn't run it.
>
Shame on me, I did run it and skipped fixing the long lines :-(
> Please fix the issues and post a v4.
>
> A lot (but not all) of the warnings report lines that are too long
> and from what I can see most are easily fixed without reducing
> readability.
>
I have fixed all the long lines. IMHO, the remaining warnings
are not worth fixing:
CHECK: Macro argument reuse 'vdev' - possible side-effects?
#77: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:21:
+#define dprintk(vdev, fmt, arg...) do { \
+ if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
+ printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
+ __func__, video_device_node_name(vdev), ##arg); \
+} while (0)
CHECK: Macro argument 'vdev' may be better as '(vdev)' to avoid precedence issues
#77: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:21:
+#define dprintk(vdev, fmt, arg...) do { \
+ if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
+ printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
+ __func__, video_device_node_name(vdev), ##arg); \
+} while (0)
WARNING: Prefer [subsystem eg: netdev]_dbg([subsystem]dev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
#79: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:23:
+ printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
CHECK: Comparison to NULL could be written "!ref"
#116: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:3255:
+ if (ref == NULL) {
CHECK: Comparison to NULL could be written "!hdl"
#249: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:3730:
+ if (hdl == NULL) {
I believe the dprintk macro is OK as-is. We use printk KERN_DEBUG because
we already have a parameter guarding it. This is consistent
with the v4l2 way.
The NULL comparison warning is valid, but it was already
like this before. I'd rather not change existing code.
I'll post a v4.
Ezequiel
> Regards,
>
> Hans
>
> On 6/22/19 12:28 PM, Ezequiel Garcia wrote:
> > 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@collabora.com>
> > ---
> > Changes from v2:
> > * Check and noisy-warn if vdev is NULL.
> >
> > 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..1c8ae4501870 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 (!WARN_ON(!vdev) && ((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 */
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: v4l: ctrls: Add debug messages
2019-07-19 21:53 ` Ezequiel Garcia
@ 2019-07-20 9:36 ` Hans Verkuil
2019-07-20 11:43 ` Ezequiel Garcia
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2019-07-20 9:36 UTC (permalink / raw)
To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, kernel
On 7/19/19 11:53 PM, Ezequiel Garcia wrote:
> On Thu, 2019-06-27 at 09:38 +0200, Hans Verkuil wrote:
>> Hi Ezequiel,
>>
>> 'checkpatch.pl --strict' finds way too many issues.
>>
>> I'm pretty certain you didn't run it.
>>
>
> Shame on me, I did run it and skipped fixing the long lines :-(
>
>> Please fix the issues and post a v4.
>>
>> A lot (but not all) of the warnings report lines that are too long
>> and from what I can see most are easily fixed without reducing
>> readability.
>>
>
> I have fixed all the long lines. IMHO, the remaining warnings
> are not worth fixing:
>
> CHECK: Macro argument reuse 'vdev' - possible side-effects?
> #77: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:21:
> +#define dprintk(vdev, fmt, arg...) do { \
> + if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
> + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
> + __func__, video_device_node_name(vdev), ##arg); \
> +} while (0)
You really need to use 'if (!WARN_ON(!(vdev)) &&' here. It is a little thing that
simply makes the code more robust.
>
> CHECK: Macro argument 'vdev' may be better as '(vdev)' to avoid precedence issues
> #77: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:21:
> +#define dprintk(vdev, fmt, arg...) do { \
> + if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
> + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
> + __func__, video_device_node_name(vdev), ##arg); \
> +} while (0)
>
> WARNING: Prefer [subsystem eg: netdev]_dbg([subsystem]dev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #79: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:23:
> + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
>
> CHECK: Comparison to NULL could be written "!ref"
> #116: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:3255:
> + if (ref == NULL) {
>
> CHECK: Comparison to NULL could be written "!hdl"
> #249: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:3730:
> + if (hdl == NULL) {
>
> I believe the dprintk macro is OK as-is. We use printk KERN_DEBUG because
> we already have a parameter guarding it. This is consistent
> with the v4l2 way.
>
> The NULL comparison warning is valid, but it was already
> like this before. I'd rather not change existing code.
>
> I'll post a v4.
Skipping the other warnings is OK, but the first really needs to be addressed.
So we need a v5, I'm afraid.
Regards,
Hans
>
> Ezequiel
>
>> Regards,
>>
>> Hans
>>
>> On 6/22/19 12:28 PM, Ezequiel Garcia wrote:
>>> 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@collabora.com>
>>> ---
>>> Changes from v2:
>>> * Check and noisy-warn if vdev is NULL.
>>>
>>> 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..1c8ae4501870 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 (!WARN_ON(!vdev) && ((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 */
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: v4l: ctrls: Add debug messages
2019-07-20 9:36 ` Hans Verkuil
@ 2019-07-20 11:43 ` Ezequiel Garcia
0 siblings, 0 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2019-07-20 11:43 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: Hans Verkuil, kernel
On Sat, 2019-07-20 at 11:36 +0200, Hans Verkuil wrote:
> On 7/19/19 11:53 PM, Ezequiel Garcia wrote:
> > On Thu, 2019-06-27 at 09:38 +0200, Hans Verkuil wrote:
> > > Hi Ezequiel,
> > >
> > > 'checkpatch.pl --strict' finds way too many issues.
> > >
> > > I'm pretty certain you didn't run it.
> > >
> >
> > Shame on me, I did run it and skipped fixing the long lines :-(
> >
> > > Please fix the issues and post a v4.
> > >
> > > A lot (but not all) of the warnings report lines that are too long
> > > and from what I can see most are easily fixed without reducing
> > > readability.
> > >
> >
> > I have fixed all the long lines. IMHO, the remaining warnings
> > are not worth fixing:
> >
> > CHECK: Macro argument reuse 'vdev' - possible side-effects?
> > #77: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:21:
> > +#define dprintk(vdev, fmt, arg...) do { \
> > + if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
> > + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
> > + __func__, video_device_node_name(vdev), ##arg); \
> > +} while (0)
>
> You really need to use 'if (!WARN_ON(!(vdev)) &&' here. It is a little thing that
> simply makes the code more robust.
>
> > CHECK: Macro argument 'vdev' may be better as '(vdev)' to avoid precedence issues
> > #77: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:21:
> > +#define dprintk(vdev, fmt, arg...) do { \
> > + if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
> > + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
> > + __func__, video_device_node_name(vdev), ##arg); \
> > +} while (0)
> >
> > WARNING: Prefer [subsystem eg: netdev]_dbg([subsystem]dev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> > #79: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:23:
> > + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \
> >
> > CHECK: Comparison to NULL could be written "!ref"
> > #116: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:3255:
> > + if (ref == NULL) {
> >
> > CHECK: Comparison to NULL could be written "!hdl"
> > #249: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:3730:
> > + if (hdl == NULL) {
> >
> > I believe the dprintk macro is OK as-is. We use printk KERN_DEBUG because
> > we already have a parameter guarding it. This is consistent
> > with the v4l2 way.
> >
> > The NULL comparison warning is valid, but it was already
> > like this before. I'd rather not change existing code.
> >
> > I'll post a v4.
>
> Skipping the other warnings is OK, but the first really needs to be addressed.
> So we need a v5, I'm afraid.
>
No worries!
> Regards,
>
> Hans
>
> > Ezequiel
> >
> > > Regards,
> > >
> > > Hans
> > >
> > > On 6/22/19 12:28 PM, Ezequiel Garcia wrote:
> > > > 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@collabora.com>
> > > > ---
> > > > Changes from v2:
> > > > * Check and noisy-warn if vdev is NULL.
> > > >
> > > > 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..1c8ae4501870 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 (!WARN_ON(!vdev) && ((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 */
> > > >
> > > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-20 11:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-22 10:28 [PATCH v3] media: v4l: ctrls: Add debug messages Ezequiel Garcia
2019-06-27 7:38 ` Hans Verkuil
2019-07-19 21:53 ` Ezequiel Garcia
2019-07-20 9:36 ` Hans Verkuil
2019-07-20 11:43 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).