On Sat, 2019-06-22 at 08:53 +0200, Hans Verkuil wrote:
> On 6/22/19 12:50 AM, 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@xxxxxxxxxxxxx>
> > ---
> > Changes from v1:
> > * Drop changes in the debug parameter semantics.
> > * Drop new module debug parameter.
> > * Add documentation.
> > * Add a debug error in all places where control can fail.
> > * Reorder the vdev parameter, to make the patch less invasive.
> > ---
> > Documentation/media/kapi/v4l2-dev.rst | 1 +
> > drivers/media/platform/omap3isp/ispvideo.c | 2 +-
> > drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++-----
> > drivers/media/v4l2-core/v4l2-ioctl.c | 12 +--
> > drivers/media/v4l2-core/v4l2-subdev.c | 6 +-
> > include/media/v4l2-ctrls.h | 9 +-
> > include/media/v4l2-ioctl.h | 2 +
> > 7 files changed, 100 insertions(+), 38 deletions(-)
> >
> > diff --git a/Documentation/media/kapi/v4l2-dev.rst b/Documentation/media/kapi/v4l2-dev.rst
> > index b359f1804bbe..4c5a15c53dbf 100644
> > --- a/Documentation/media/kapi/v4l2-dev.rst
> > +++ b/Documentation/media/kapi/v4l2-dev.rst
> > @@ -288,6 +288,7 @@ Mask Description
> > 0x08 Log the read and write file operations and the VIDIOC_QBUF and
> > VIDIOC_DQBUF ioctls.
> > 0x10 Log the poll file operation.
> > +0x20 Log error and messages in the control operations.
> > ===== ================================================================
> >
> > Video device cleanup
> > diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> > index 175bbed9a235..abc945cc05c9 100644
> > --- a/drivers/media/platform/omap3isp/ispvideo.c
> > +++ b/drivers/media/platform/omap3isp/ispvideo.c
> > @@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
> > ctrls.count = 1;
> > ctrls.controls = &ctrl;
> >
> > - ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, NULL, &ctrls);
> > + ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &video->video, NULL, &ctrls);
> > if (ret < 0) {
> > dev_warn(isp->dev, "no pixel rate control in subdev %s\n",
> > pipe->external->name);
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 2d7525e2d9eb..4ee703e04ef2 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -6,6 +6,8 @@
> >
> > */
> >
> > +#define pr_fmt(fmt) "v4l2-ctrls: " fmt
> > +
> > #include <linux/ctype.h>
> > #include <linux/mm.h>
> > #include <linux/slab.h>
> > @@ -16,6 +18,12 @@
> > #include <media/v4l2-event.h>
> > #include <media/v4l2-dev.h>
> >
> > +#define dprintk(vdev, fmt, arg...) do { \
> > + if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \
>
> This isn't very robust if vdev is NULL. I know it shouldn't, but it's
> better to check for this.
>
> I'd use this instead:
>
> if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
>
> That way nothing crashes if vdev is NULL, and you get a warning as well in that case.
>
OK, no problem.
Thanks,
Eze
