Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Sep 22, 2023 at 01:27:46PM +0300, Tomi Valkeinen wrote:
> On 22/09/2023 13:22, Laurent Pinchart wrote:
> > On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote:
> >> On 22/09/2023 13:09, Sakari Ailus wrote:
> >>> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
> >>>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
> >>>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> >>>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> >>>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> >>>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> >>>>>>>>> Print debug level information on returned frame descriptors.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>>>>> ---
> >>>>>>>>>    drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> >>>>>>>>>    1 file changed, 31 insertions(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644
> >>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> @@ -15,6 +15,7 @@
> >>>>>>>>>    #include <linux/module.h>
> >>>>>>>>>    #include <linux/overflow.h>
> >>>>>>>>>    #include <linux/slab.h>
> >>>>>>>>> +#include <linux/string.h>
> >>>>>>>>>    #include <linux/types.h>
> >>>>>>>>>    #include <linux/version.h>
> >>>>>>>>>    #include <linux/videodev2.h>
> >>>>>>>>> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> >>>>>>>>>    static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >>>>>>>>>    			       struct v4l2_mbus_frame_desc *fd)
> >>>>>>>>>    {
> >>>>>>>>> +	unsigned int i;
> >>>>>>>>> +	int ret;
> >>>>>>>>> +
> >>>>>>>>>    	memset(fd, 0, sizeof(*fd));
> >>>>>>>>>    
> >>>>>>>>> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> >>>>>>>>> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> >>>>>>>>> +	if (ret)
> >>>>>>>>> +		return ret;
> >>>>>>>>> +
> >>>>>>>>> +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> >>>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> >>>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> >>>>>>>>> +		"unknown");
> >>>>>>>>> +
> >>>>>>>>> +	for (i = 0; i < fd->num_entries; i++) {
> >>>>>>>>> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> >>>>>>>>> +		char buf[20] = "";
> >>>>>>>>
> >>>>>>>> Should this be sized for the worst case ? The vc and dt should not be
> >>>>>>>> large, but a buffer overflow on the stack in debug code if a subdev
> >>>>>>>> returns an incorrect value would be bad.
> >>>>>>>
> >>>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in
> >>>>>>> practice here.
> >>>>>>
> >>>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt
> >>>>>
> >>>>> I can count only 17 --- there's no newline.
> >>>>>
> >>>>> I guess it's most probably either of these then. X-)
> >>>>>
> >>>>>> are u8. I'm just a bit worried we're opening the door to hard to debug
> >>>>>> problems if we later change the vc and dt types.
> >>>>>
> >>>>> I can add a WARN_ON() to cover this.
> >>>>>
> >>>>>>>>> +
> >>>>>>>>> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> >>>>>>>>> +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> >>>>>>>>
> >>>>>>>> 0x%02x would be one character shorter ;-) Same below.
> >>>>>>>
> >>>>>>> It would be, but I prefer the above notation as it's more generic.
> >>>>>>
> >>>>>> Out of curiosity, how so ?
> >>>>>
> >>>>> It works with data that would span more than 9 characters when printed.
> >>>>
> >>>> And 0x%02x doesn't ?
> >>>
> >>> Ah, it should indeed work, 0 is actually a flag here and part of the field
> >>> width or precision. I can use that in v4.
> >>
> >> Or %#04x, even shorter!
> > 
> > That's different though.
> > 
> > printf("0x%2.2x\n", 0);		-> 0x00
> > printf("0x%2.2x\n", 1);		-> 0x01
> > printf("0x%2.2x\n", 42);	-> 0x2a
> > 
> > printf("0x%02x\n", 0);		-> 0x00
> > printf("0x%02x\n", 1);		-> 0x01
> > printf("0x%02x\n", 42);		-> 0x2a
> > 
> > printf("%#2.2x\n", 0);		-> 00
> > printf("%#2.2x\n", 1);		-> 0x01
> > printf("%#2.2x\n", 42);		-> 0x2a
> > 
> > printf("%#02x\n", 0);		-> 00
> > printf("%#02x\n", 1);		-> 0x1
> > printf("%#02x\n", 42);		-> 0x2a
> 
> The length should be 4 there, not 2. But even after fixing that, the 0 
> case is printed wrong. Interesting, I haven't noticed that before. I 
> wonder why it behaves that way...

It's documented as such:

  #  The value should be converted to an "alternate form". [...] For x
     and X conversions, a nonzero result has the string "0x" (or "0X"
     for X conversions) prepended to it. [...]

> 0000
> 0x01
> 0x2a

-- 
Regards,

Laurent Pinchart