Re: [PATCH v2] media: v4l: ctrls: Add debug messages

[Ezequiel Garcia <ezequiel@collabora.com>]

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@collabora.com>
> > ---
> > 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