From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Steve Longerbeam <slongerbeam@gmail.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>,
linux-media@vger.kernel.org,
Philipp Zabel <p.zabel@pengutronix.de>,
Rui Miguel Silva <rmfrfs@gmail.com>
Subject: Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
Date: Wed, 1 Apr 2020 03:31:49 +0300 [thread overview]
Message-ID: <20200401003149.GH4767@pendragon.ideasonboard.com> (raw)
In-Reply-To: <02063ed3-e2de-8f15-bd08-84d3c319f79c@gmail.com>
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@ideasonboard.com>
> >>
> >> 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@ideasonboard.com>
> >>
> >> [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@gmail.com>
> >> ---
> >> 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 :-)
> > 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;
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-04-01 0:32 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 01/11] media: imx: utils: fix and simplify pixel format enumeration Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 02/11] media: imx: utils: fix media bus " Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 03/11] media: imx: utils: Inline init_mbus_colorimetry() in its caller Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 04/11] media: imx: utils: Handle Bayer format lookup through a selection flag Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 05/11] media: imx: utils: Simplify IPU format lookup and enumeration Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes Steve Longerbeam
2020-03-31 13:45 ` Hans Verkuil
2020-03-31 23:04 ` Steve Longerbeam
2020-04-01 0:31 ` Laurent Pinchart [this message]
2020-04-01 7:39 ` Hans Verkuil
2020-04-01 15:20 ` Steve Longerbeam
2020-04-01 15:31 ` Laurent Pinchart
2020-04-03 22:29 ` Steve Longerbeam
2020-04-03 22:38 ` Laurent Pinchart
2020-03-29 20:59 ` [PATCH v4 07/11] media: imx: utils: Remove unneeded argument to (find|enum)_format() Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 08/11] media: imx: utils: Rename format lookup and enumeration functions Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 09/11] media: imx: utils: Constify mbus argument to imx_media_mbus_fmt_to_* Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 10/11] media: imx: utils: Constify ipu_image argument to imx_media_ipu_image_to_mbus_fmt() Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 11/11] media: imx: utils: Split find|enum_format into fourcc and mbus functions Steve Longerbeam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200401003149.GH4767@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil@xs4all.nl \
--cc=linux-media@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=rmfrfs@gmail.com \
--cc=slongerbeam@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).