Re: [PATCH v6 7/9] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Hardcoded pad indices lead to a NULL pointer dereference in `imx_cpi_set_fmt()` if the device tree specifies fewer than 2 endpoints.
- [High] Hardcoded pad indices cause memory corruption and use of uninitialized flags if device tree endpoints are reordered.
- [High] Pending asynchronous PM runtime suspend is cancelled during device removal, permanently leaking hardware clocks.
- [Medium] The Kconfig symbol `VIDEO_IMX_PARALLEL_CPI` selects a non-existent symbol `V4L2_1TO1`.
- [Medium] Unchecked return value from `of_device_get_match_data()` leads to a NULL pointer dereference if bound via sysfs.
- [Medium] Incorrect pad arguments passed to `v4l2_subdev_state_xlate_streams()`.
- [Low] The V2 register macros for `CPI_CTRL_V2_REG1` overlap at bit 16 and exceed the 32-bit register width.
--

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

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

This commit adds a V4L2 sub-device driver for the CPI controller found
on i.MX8QXP, i.MX8QM, and i.MX93 SoCs. It 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
[ ... ]
> +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 this selecting a non-existent Kconfig symbol? V4L2_1TO1 does not appear
to exist in the kernel tree. Selecting a missing symbol can cause unmet
dependency warnings or silent build failures.

> +	select V4L2_FWNODE
> +	select VIDEO_V4L2_SUBDEV_API
> +	help
> +	  Video4Linux2 sub-device driver for PARALLEL CPI receiver found
> +	  on some iMX8 and iMX9 SoCs.

> 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..0f74b51608715
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx-parallel-cpi.c
> @@ -0,0 +1,629 @@
[ ... ]
> +/* CPI INTERFACE CONTROL REG1 */
> +#define CPI_CTRL_REG1_PIXEL_WIDTH(v)		FIELD_PREP(GENMASK(15, 0), (v))
> +#define CPI_CTRL_REG1_VSYNC_PULSE(v)		FIELD_PREP(GENMASK(31, 16), (v))
> +
> +#define CPI_CTRL_V2_REG1_PIXEL_WIDTH(v)		FIELD_PREP(GENMASK(16, 0), (v))
> +#define CPI_CTRL_V2_REG1_VSYNC_PULSE(v)		FIELD_PREP(GENMASK(31, 16), (v))

[Severity: Low]
Do these V2 register macros overlap? The GENMASK(16, 0) and GENMASK(31, 16)
overlap at bit 16, resulting in a 33-bit span for what appears to be a
32-bit register. While currently unused, could this corrupt register
values if utilized in the future?

> +
> +/* Need match field DATA_TYPE_IN definition at CPI CTRL register */
[ ... ]
> +static void imx_cpi_hw_config(struct imx_cpi_device *pcpidev,
> +			      const struct imx_cpi_pix_format *pcpidev_fmt)
> +{
> +	u32 flags = pcpidev->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK].vep.bus.parallel.flags;

[Severity: High]
Can hardcoding the pad index lead to reading uninitialized memory here?

The pads array is populated based on the order endpoints appear in the
Device Tree. If a device tree defines the source endpoint before the
sink endpoint, pads[0] will correspond to the source pad instead of
the sink pad.

> +	const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
> +	bool hsync_pol = flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH;
[ ... ]
> +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;
[ ... ]
> +	/* Propagate the format from sink to source. */
> +	fmt = v4l2_subdev_state_get_format(sd_state, V4L2_SUBDEV_1TO1_PADS_SOURCE);
> +	*fmt = sdformat->format;

[Severity: High]
Can this result in a NULL pointer dereference?

If a misconfigured device tree only specifies a single endpoint,
media_async_register_subdev() will allocate exactly one pad, meaning
sd->entity.num_pads = 1.

When v4l2_subdev_state_get_format() is explicitly called with
V4L2_SUBDEV_1TO1_PADS_SOURCE (index 1), it will return NULL since the
index is out of bounds. The pointer is then blindly dereferenced.

> +
> +	/* The format on the source pad might change due to unpacking. */
> +	fmt->code = pcpidev_fmt->output;
> +
> +	return 0;
> +}
[ ... ]
> +static int imx_cpi_enable_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);
> +	const struct imx_cpi_pix_format *pcpidev_fmt;
> +	const struct v4l2_mbus_framefmt *format;
> +	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 device tree endpoints are reordered as mentioned previously, can
this cast the wrong entity type to a v4l2_subdev?

If pads[0] corresponds to the source pad, its remote entity will be a
video_device, not a v4l2_subdev. Calling media_entity_to_v4l2_subdev()
on a video_device produces an invalid pointer that will crash the kernel
when dereferenced.

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

[Severity: Medium]
Are the pad arguments reversed here?

Since v4l2_subdev_enable_streams() is called on the source pad,
streams_mask represents streams on the source pad. Passing
V4L2_SUBDEV_1TO1_PADS_SINK as the first argument informs the helper that
streams_mask belongs to the sink pad. Could this silently break routing
logic if asymmetrical stream IDs are ever used?

> +
> +	format = v4l2_subdev_state_get_format(state, V4L2_SUBDEV_1TO1_PADS_SINK);
[ ... ]
> +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: High]
Could this leak hardware clocks if an asynchronous suspend is pending?

When streams are disabled, pm_runtime_put_autosuspend() is called to
queue an async suspend. If the driver is unbound before the async work
executes, imx_cpi_remove() will run.

Since the driver relies on devm_pm_runtime_enable() for PM cleanup,
pm_runtime_disable() will flush and cancel the pending async suspend,
which leaves the device permanently powered on. Should this call
pm_runtime_put_sync() or explicitly suspend the device if an async put
was previously used?

> +
> +static int imx_cpi_probe(struct platform_device *pdev)
> +{
> +	struct imx_cpi_device *pcpidev;
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_subdev *sd;
> +	int ret;
> +
> +	pcpidev = devm_kzalloc(dev, sizeof(*pcpidev), GFP_KERNEL);
> +	if (!pcpidev)
> +		return -ENOMEM;
> +
> +	pcpidev->dev = dev;
> +	platform_set_drvdata(pdev, pcpidev);
> +
> +	pcpidev->pdata = of_device_get_match_data(dev);

[Severity: Medium]
Does this missing NULL check lead to a crash when binding via sysfs?

If an administrator manually binds the driver to a platform device without
an Open Firmware node via sysfs, of_device_get_match_data() returns NULL.
The driver will later unconditionally dereference pcpidev->pdata (such as
in imx_cpi_sw_reset()), leading to a kernel panic instead of gracefully
failing.

> +	pcpidev->mode = PI_GATE_CLOCK_MODE;
> +
> +	pcpidev->regs = devm_platform_ioremap_resource(pdev, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=7