Re: [PATCH v5 2/2] media: nxp: imx8-isi: Add virtual channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Pipeline enablement unconditionally fails if the upstream subdevice does not implement the `get_frame_desc` operation.
- [Medium] The driver unconditionally accesses CSI-2 specific fields in the frame descriptor without validating the frame descriptor type.
--

commit 4d8a62dc345af085b3b0a217c07143648e85fd35
Author: Guoniu Zhou <guoniu.zhou@nxp.com>
Subject: media: nxp: imx8-isi: Add virtual channel support

This commit introduces virtual channel (VC) support to the i.MX8 ISI driver.
It retrieves the VC from the remote upstream subdevice's frame descriptor and
applies the required register configurations based on the platform's maximum
supported VCs.

> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> index a41c51dd9ce0f..03e0115b5b5a8 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c

[ ... ]

> +static int mxc_isi_get_vc(struct mxc_isi_pipe *pipe)
> +{
> +	struct mxc_isi_crossbar *xbar = &pipe->isi->crossbar;
> +	struct device *dev = pipe->isi->dev;
> +	struct v4l2_mbus_frame_desc fd = { };
> +	unsigned int source_pad = xbar->num_sinks + pipe->id;
> +	unsigned int max_vc;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = v4l2_subdev_call(&xbar->sd, pad, get_frame_desc,
> +			       source_pad, &fd);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to get source frame desc from pad %u\n",
> +			source_pad);
> +		return ret;
> +	}

[Severity: High]
If the remote upstream subdevice does not implement the optional
get_frame_desc operation, v4l2_subdev_call() will return -ENOIOCTLCMD.

Does this mean pipeline enablement will now unconditionally fail for any
sensor lacking get_frame_desc support?

Prior to this patch, the VC was implicitly hardcoded to 0, which worked
seamlessly for parallel and legacy CSI-2 sensors. Would it be better to
explicitly check for -ENOIOCTLCMD (and potentially -EOPNOTSUPP) here and
gracefully fallback to a default VC of 0 to prevent this regression?

> +
> +	/* Find stream 0 in the frame descriptor */
> +	for (i = 0; i < fd.num_entries; i++) {
> +		if (fd.entry[i].stream == 0)
> +			break;
> +	}
> +
> +	if (i == fd.num_entries) {
> +		dev_err(dev, "Failed to find stream from source frame desc\n");
> +		return -EPIPE;
> +	}
> +
> +	max_vc = pipe->isi->pdata->num_vc ? : 1;
> +
> +	/* Check virtual channel range */
> +	if (fd.entry[i].bus.csi2.vc >= max_vc) {

[Severity: Medium]
This code directly accesses fd.entry[i].bus.csi2.vc, but is it guaranteed
that the frame descriptor type is V4L2_MBUS_FRAME_DESC_TYPE_CSI2?

Since the i.MX8 ISI also supports parallel cameras, fd.type could be
V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL, making the access to bus.csi2
semantically invalid. 

Should there be a check to validate fd.type before accessing the CSI-2
specific union members to avoid potential undefined behavior?

> +		dev_err(dev, "Virtual channel %u exceeds maximum %u\n",
> +			fd.entry[i].bus.csi2.vc, max_vc - 1);
> +		return -EPIPE;
> +	}
> +
> +	return fd.entry[i].bus.csi2.vc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-isi_vc-v5-0-a38eb4fcd58e@oss.nxp.com?part=2