Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Laurent,

On 3/31/20 5:31 PM, Laurent Pinchart wrote:

Hello,

On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:

On 3/31/20 6:45 AM, Hans Verkuil wrote:

On 3/29/20 10:59 PM, Steve Longerbeam wrote:

From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

The imx_media_pixfmt structures includes a codes member that stores
media bus codes as a fixed array of 4 integers. The functions dealing
with the imx_media_pixfmt structures assume that the array of codes is
terminated by a 0 elements. This mechanism is fragile, as demonstrated

elements -> element


by several instances of the structure contained 4 non-zero codes.

contained -> that contained

Will fix above language.


Fix this by turning the array into a pointer, and providing an
IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
element at the end.

Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

[Fix a NULL deref when derefencing a NULL cc->codes on return from
   several calls to imx_media_find_format()]
Signed-off-by: Steve Longerbeam <slongerbeam@xxxxxxxxx>
---
   drivers/staging/media/imx/imx-media-capture.c |  4 +-
   drivers/staging/media/imx/imx-media-utils.c   | 88 +++++++++++--------
   drivers/staging/media/imx/imx-media.h         |  2 +-
   3 files changed, 53 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index d60b49ec4fa4..650c53289f6b 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
   	if (!cc)
   		return -EINVAL;
- fse.code = cc->codes[0]; 
+	fse.code = cc->codes ? cc->codes[0] : 0;

I'm wondering: wouldn't it be better to have a format without codes point to
an empty code list containing just 0? That would avoid all the cc->codes checks,
which are a bit fragile IMHO.

I'll modify this patch to declare a const codes array 'no_bus_fmts',
containing a single 0 value, that the in-memory-only imx_media_pixfmt
entries can point to.


It would be nice in that case if there is a WARN_ON or something during probe
time that checks that the codes field in pixel_formats[] is never NULL.

I'm not so keen on that idea. How about a comment that the
in-memory-only entries should point to no_bus_fmts ?

I don't like either of those options I'm afraid. Someone will one day
forget to point to that array when adding a new format, and we'll have a
bug. With code in just a few locations that checks for NULL, we're safe
as the compiler will initialize the pointer to NULL for us. Let's use
the compiler when it helps us, I don't see what removing the NULL check
in the code could bring us except issues :-)
I wasn't planning on removing the NULL checks, only to add the comment to advise setting .codes = no_bus_fmts. Although I don't really think that's necessary either. 

Steve




As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
what 'cc' stands for.

It's an abbreviation of FourCC. I know, it's obscure and not really
accurate. I will add a non-functional patch that makes the naming
consistent, using 'fmt' for imx_media_pixfmt pointers throughout.


   	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
   	if (ret)
@@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
   	if (!cc)
   		return -EINVAL;
- fie.code = cc->codes[0]; 
+	fie.code = cc->codes ? cc->codes[0] : 0;
ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, 
   			       NULL, &fie);
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 981a8b540a3c..da010eef0ae6 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -7,6 +7,12 @@
   #include <linux/module.h>
   #include "imx-media.h"
+#define IMX_BUS_FMTS(fmts...) \ 
+	(const u32[]) {		\
+		fmts,		\
+		0		\
+	}
+
   /*
    * List of supported pixel formats for the subdevs.
    */
@@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
   	/*** YUV formats start here ***/
   	{
   		.fourcc	= V4L2_PIX_FMT_UYVY,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_UYVY8_2X8,
   			MEDIA_BUS_FMT_UYVY8_1X16
-		},
+		),
   		.cs     = IPUV3_COLORSPACE_YUV,
   		.bpp    = 16,
   	}, {
   		.fourcc	= V4L2_PIX_FMT_YUYV,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_YUYV8_2X8,
   			MEDIA_BUS_FMT_YUYV8_1X16
-		},
+		),
   		.cs     = IPUV3_COLORSPACE_YUV,
   		.bpp    = 16,
   	}, {
@@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
   	/*** RGB formats start here ***/
   	{
   		.fourcc	= V4L2_PIX_FMT_RGB565,
-		.codes  = {MEDIA_BUS_FMT_RGB565_2X8_LE},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.cycles = 2,
   	}, {
   		.fourcc	= V4L2_PIX_FMT_RGB24,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_RGB888_1X24,
   			MEDIA_BUS_FMT_RGB888_2X12_LE
-		},
+		),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 24,
   	}, {
@@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
   		.bpp    = 24,
   	}, {
   		.fourcc	= V4L2_PIX_FMT_XRGB32,
-		.codes  = {MEDIA_BUS_FMT_ARGB8888_1X32},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 32,
   		.ipufmt = true,
@@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
   	/*** raw bayer and grayscale formats start here ***/
   	{
   		.fourcc = V4L2_PIX_FMT_SBGGR8,
-		.codes  = {MEDIA_BUS_FMT_SBGGR8_1X8},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 8,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SGBRG8,
-		.codes  = {MEDIA_BUS_FMT_SGBRG8_1X8},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 8,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SGRBG8,
-		.codes  = {MEDIA_BUS_FMT_SGRBG8_1X8},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 8,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SRGGB8,
-		.codes  = {MEDIA_BUS_FMT_SRGGB8_1X8},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 8,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SBGGR16,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_SBGGR10_1X10,
   			MEDIA_BUS_FMT_SBGGR12_1X12,
   			MEDIA_BUS_FMT_SBGGR14_1X14,
   			MEDIA_BUS_FMT_SBGGR16_1X16
-		},
+		),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SGBRG16,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_SGBRG10_1X10,
   			MEDIA_BUS_FMT_SGBRG12_1X12,
   			MEDIA_BUS_FMT_SGBRG14_1X14,
-			MEDIA_BUS_FMT_SGBRG16_1X16,
-		},
+			MEDIA_BUS_FMT_SGBRG16_1X16
+		),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SGRBG16,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_SGRBG10_1X10,
   			MEDIA_BUS_FMT_SGRBG12_1X12,
   			MEDIA_BUS_FMT_SGRBG14_1X14,
-			MEDIA_BUS_FMT_SGRBG16_1X16,
-		},
+			MEDIA_BUS_FMT_SGRBG16_1X16
+		),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_SRGGB16,
-		.codes  = {
+		.codes  = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_SRGGB10_1X10,
   			MEDIA_BUS_FMT_SRGGB12_1X12,
   			MEDIA_BUS_FMT_SRGGB14_1X14,
-			MEDIA_BUS_FMT_SRGGB16_1X16,
-		},
+			MEDIA_BUS_FMT_SRGGB16_1X16
+		),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_GREY,
-		.codes = {
+		.codes = IMX_BUS_FMTS(
   			MEDIA_BUS_FMT_Y8_1X8,
   			MEDIA_BUS_FMT_Y10_1X10,
-			MEDIA_BUS_FMT_Y12_1X12,
-		},
+			MEDIA_BUS_FMT_Y12_1X12
+		),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 8,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_Y10,
-		.codes = {MEDIA_BUS_FMT_Y10_1X10},
+		.codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.bayer  = true,
   	}, {
   		.fourcc = V4L2_PIX_FMT_Y12,
-		.codes = {MEDIA_BUS_FMT_Y12_1X12},
+		.codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 16,
   		.bayer  = true,
@@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
   			 CS_SEL_YUV : CS_SEL_RGB);
if (!(fmt_cs_sel & cs_sel) || 
-		    (!allow_non_mbus && !fmt->codes[0]))
+		    (!allow_non_mbus && !fmt->codes))
   			continue;
if (fourcc && fmt->fourcc == fourcc) 
   			return fmt;
- if (!code) 
+		if (!code || !fmt->codes)
   			continue;
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) { 
+		for (j = 0; fmt->codes[j]; j++) {
   			if (code == fmt->codes[j])
   				return fmt;
   		}
@@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
   			 CS_SEL_YUV : CS_SEL_RGB);
if (!(fmt_cs_sel & cs_sel) || 
-		    (!allow_non_mbus && !fmt->codes[0]))
+		    (!allow_non_mbus && !fmt->codes))
   			continue;
if (fourcc && index == 0) { 
@@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
   			continue;
   		}
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) { 
+		for (j = 0; fmt->codes[j]; j++) {
   			if (index == 0) {
   				*code = fmt->codes[j];
   				return 0;
@@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
   static const struct imx_media_pixfmt ipu_formats[] = {
   	{
   		.fourcc = V4L2_PIX_FMT_YUV32,
-		.codes  = {MEDIA_BUS_FMT_AYUV8_1X32},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
   		.cs     = IPUV3_COLORSPACE_YUV,
   		.bpp    = 32,
   		.ipufmt = true,
   	}, {
   		.fourcc	= V4L2_PIX_FMT_XRGB32,
-		.codes  = {MEDIA_BUS_FMT_ARGB8888_1X32},
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
   		.cs     = IPUV3_COLORSPACE_RGB,
   		.bpp    = 32,
   		.ipufmt = true,
@@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
   		    (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
   			continue;
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) { 
+		if (!fmt->codes)
+			continue;
+
+		for (j = 0; fmt->codes[j]; j++) {
   			if (code == fmt->codes[j])
   				return fmt;
   		}
@@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
   		    (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
   			continue;
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) { 
+		if (!fmt->codes)
+			continue;
+
+		for (j = 0; fmt->codes[j]; j++) {
   			if (index == 0) {
   				*code = fmt->codes[j];
   				return 0;
@@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
   	const struct imx_media_pixfmt *fmt;
fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY); 
-	if (!fmt)
+	if (!fmt || !fmt->codes)
   		return -EINVAL;
memset(mbus, 0, sizeof(*mbus)); 
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 652673a703cd..917b4db02985 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -69,7 +69,7 @@ enum {
struct imx_media_pixfmt { 
   	u32     fourcc;
-	u32     codes[4];
+	const u32 *codes;
   	int     bpp;     /* total bpp */
   	/* cycles per pixel for generic (bayer) formats for the parallel bus */
   	int	cycles;
[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux