Re: [PATCH v7 6/8] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing NULL check for `remote_pad` in stream control operations causes a kernel panic.
- [High] The driver fails to validate or clamp user-provided `width` and `height` in `imx_cpi_set_fmt`.
- [Medium] Selecting a non-existent Kconfig symbol `V4L2_1TO1`.
- [Medium] Hardware and clocks remain enabled if the driver is unbound while streaming.
- [Medium] Incorrect initial format set on the source pad during subdev state initialization.
--

commit ff679b5b2d589a01fdb96969df5b227ed2493b95
Author: Alice Yuan <alice.yuan@nxp.com>

media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)

This patch adds a V4L2 sub-device driver for the CPI controller found on
i.MX8QXP, i.MX8QM, and i.MX93 SoCs. The controller supports parallel camera
sensors and enables image data capture through a parallel interface.

> diff --git a/drivers/media/platform/nxp/Kconfig b/drivers/media/platform/nxp/Kconfig
> index 40e3436669e21..90f7c792003f2 100644
> --- a/drivers/media/platform/nxp/Kconfig
> +++ b/drivers/media/platform/nxp/Kconfig
> @@ -39,6 +39,18 @@ config VIDEO_IMX_MIPI_CSIS
>  	  Video4Linux2 sub-device driver for the MIPI CSI-2 CSIS receiver
>  	  v3.3/v3.6.3 found on some i.MX7 and i.MX8 SoCs.
>  
> +config VIDEO_IMX_PARALLEL_CPI
> +	tristate "NXP i.MX8/i.MX9 Parallel CPI Driver"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	depends on VIDEO_DEV
> +	select MEDIA_CONTROLLER
> +	select V4L2_1TO1

[Severity: Medium]
Is the V4L2_1TO1 symbol defined anywhere in the kernel configuration tree?
Selecting a non-existent Kconfig symbol has no effect and might cause silent
warnings or leave required features disabled.

> +	select V4L2_FWNODE
> +	select VIDEO_V4L2_SUBDEV_API
> +	help

[ ... ]

> diff --git a/drivers/media/platform/nxp/imx-parallel-cpi.c b/drivers/media/platform/nxp/imx-parallel-cpi.c
> new file mode 100644
> index 0000000000000..735d4a89f7b30
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx-parallel-cpi.c
> @@ -0,0 +1,629 @@

[ ... ]

> +static int imx_cpi_set_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *sd_state,
> +			   struct v4l2_subdev_format *sdformat)
> +{
> +	struct imx_cpi_pix_format const *pcpidev_fmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	/*
> +	 * The Parallel cpi can't transcode in any way, the source format
> +	 * can't be modified.
> +	 */
> +	if (sdformat->pad == V4L2_SUBDEV_1TO1_PADS_SOURCE)
> +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> +
> +	pcpidev_fmt = find_imx_cpi_format(sdformat->format.code);
> +	if (!pcpidev_fmt)
> +		pcpidev_fmt = &imx_cpi_formats[0];
> +
> +	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
> +
> +	fmt->code = pcpidev_fmt->code;
> +	fmt->width = sdformat->format.width;
> +	fmt->height = sdformat->format.height;

[Severity: High]
Are the user-supplied width and height validated or clamped before being
saved to the subdevice state?

If a user sets the width to 0 via the VIDIOC_SUBDEV_S_FMT ioctl, could this
cause an arithmetic underflow later when calculating the hardware register
configuration in imx_cpi_config_ctrl_reg1():
        
pixel_width = format->width - 1;

> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = sdformat->format.colorspace;

[ ... ]

> +static int imx_cpi_init_state(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_subdev_route routes[] = {
> +		{
> +			.sink_pad = V4L2_SUBDEV_1TO1_PADS_SINK,
> +			.sink_stream = 0,
> +			.source_pad = V4L2_SUBDEV_1TO1_PADS_SOURCE,
> +			.source_stream = 0,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +	};
> +	struct v4l2_subdev_krouting routing = {
> +		.len_routes = ARRAY_SIZE(routes),
> +		.num_routes = ARRAY_SIZE(routes),
> +		.routes = routes,
> +	};
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +	fmt->code = imx_cpi_formats[0].code;
> +	fmt->width = 1920;
> +	fmt->height = 1080;
> +
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(false,
> +							  fmt->colorspace,
> +							  fmt->ycbcr_enc);
> +
> +	return v4l2_subdev_set_routing_with_fmt(sd, state, &routing, fmt);

[Severity: Medium]
Does v4l2_subdev_set_routing_with_fmt() copy the sink format directly to
the source pad without accounting for hardware unpacking?

Since the IMX CPI unpacks data, will this cause the subdevice to expose
an inconsistent default format? Would it be better to initialize the
source pad to natively output MEDIA_BUS_FMT_UYVY8_1X16 instead?

> +}
> +
> +static int imx_cpi_disable_streams(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state, u32 pad,
> +				   u64 streams_mask)
> +{
> +	struct imx_cpi_device *pcpidev = sd_to_imx_cpi_device(sd);
> +	struct media_pad *sink_pad, *remote_pad;
> +	struct device *dev = pcpidev->dev;
> +	struct v4l2_subdev *remote_sd;
> +	u64 mask;
> +	int ret;
> +
> +	sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
> +	remote_pad = media_pad_remote_pad_first(sink_pad);
> +	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);

[Severity: High]
If the subdevice is not linked, could media_pad_remote_pad_first() return
NULL?

Since the subdevice is registered with a device node, userspace can invoke
stream operations directly. Could dereferencing remote_pad->entity cause a
kernel panic in that scenario?

This identical pattern also appears in imx_cpi_enable_streams().

> +
> +	mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
> +					       V4L2_SUBDEV_1TO1_PADS_SOURCE,
> +					       &streams_mask);

[ ... ]

> +static void imx_cpi_remove(struct platform_device *pdev)
> +{
> +	struct imx_cpi_device *pcpidev = platform_get_drvdata(pdev);
> +
> +	media_async_subdev_cleanup(&pcpidev->sd);
> +}

[Severity: Medium]
If the driver is unbound via sysfs while actively streaming, does this
leave the hardware running and leak the PM runtime usage counter?

Since imx_cpi_enable_streams() increments the PM usage counter via
pm_runtime_resume_and_get(), should there be explicit calls to stop the
streams and put the PM counter in the remove path before cleanup?

> +
> +static int imx_cpi_probe(struct platform_device *pdev)
> +{

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-imx8qxp_pcam-v7-0-b47d9e363400@nxp.com?part=6