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

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

Hi Sakari,

Thank you for the patch.

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.

> +
> +		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.

> +				 entry->bus.csi2.vc, entry->bus.csi2.dt);
> +
> +		dev_dbg(sd->dev,
> +			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",

s/lenght/length/

> +			entry->stream, entry->pixelcode, entry->length,
> +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> +			" LEN_MAX" : "",
> +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> +			" BLOB" : "", buf);

If no flags are set, you will get something like

	stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a

Maybe printing the hex value for the flags would be simpler and clearer
?

> +	}
> +
> +	return 0;
>  }
>  
>  static inline int check_edid(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart