Re: [PATCH v13 07/17] media: cadence: csi2rx: Move to .enable/disable_streams API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Source subdev streams are unconditionally enabled and disabled, breaking multi-stream reference counting.

Pre-existing issues:
- [High] Accessing the D-PHY lane control register after its peripheral clock is disabled and reset is asserted.
--

commit 5179e5527e6b2b1932479629edc9186d143ce51c
Author: Rishikesh Donadkar <r-donadkar@ti.com>
Subject: media: cadence: csi2rx: Move to .enable/disable_streams API

This patch updates the cadence csi2rx driver to use the V4L2
enable_streams() and disable_streams() API rather than s_stream(). It
also drops the local stream start/stop mutex, relying instead on V4L2
core serialization.

> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index cde690c6fdee6..2b25314ba6ab0 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
[ ... ]
> @@ -459,9 +448,6 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  	reset_control_assert(csi2rx->p_rst);
>  	clk_disable_unprepare(csi2rx->p_clk);
>  
> -	if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
> -		dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
> -
>  	if (csi2rx->dphy) {
>  		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);

[Severity: High]
This is a pre-existing issue, but can this sequence cause a bus hang?

This function asserts csi2rx->p_rst and disables csi2rx->p_clk, but then
subsequently writes to CSI2RX_DPHY_LANE_CTRL_REG. Writing to a memory-mapped
I/O register while its bus clock is turned off and the peripheral is held in
reset can cause synchronous external aborts on ARM platforms.

I notice this is resolved later in the series by commit a72fbe3bfad52
("media: cadence: csi2rx: Support runtime PM"), which moves the clock disable
sequence into runtime_suspend() so the register is accessed while clocks are
still enabled.

>  
> @@ -485,38 +471,56 @@ static int csi2rx_log_status(struct v4l2_subdev *sd)
[ ... ]
> +static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
> +				 struct v4l2_subdev_state *state, u32 pad,
> +				 u64 streams_mask)
>  {
>  	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> -	int ret = 0;
> -
> -	mutex_lock(&csi2rx->lock);
> -
> -	if (enable) {
> -		/*
> -		 * If we're not the first users, there's no need to
> -		 * enable the whole controller.
> -		 */
> -		if (!csi2rx->count) {
> -			ret = csi2rx_start(csi2rx);
> -			if (ret)
> -				goto out;
> -		}
> +	int ret;
>  
> -		csi2rx->count++;
> -	} else {
> -		csi2rx->count--;
> +	/*
> +	 * If we're not the first users, there's no need to
> +	 * enable the whole controller.
> +	 */
> +	if (!csi2rx->count) {
> +		ret = csi2rx_start(csi2rx);
> +		if (ret)
> +			return ret;
> +	}
>  
> -		/*
> -		 * Let the last user turn off the lights.
> -		 */
> +	/* Start streaming on the source */
> +	ret = v4l2_subdev_enable_streams(csi2rx->source_subdev, csi2rx->source_pad,
> +					 BIT_U64(0));

[Severity: High]
Will this break the ability to start multiple independent streams?

Since v4l2_subdev_enable_streams() is called unconditionally outside the
if (!csi2rx->count) block, a second stream start will call it again with the
exact same mask BIT_U64(0). Because the V4L2 core tracks enabled streams,
won't this second call fail and return -EALREADY?

I see this is fixed later in the series in commit dc92c5b7cb43f ("media:
cadence: csi2rx: add multistream support"), which translates pad-specific
stream masks into disjoint sink_streams masks to avoid the -EALREADY
collision, but does this intermediate patch break bisectability?

> +	if (ret) {
> +		dev_err(csi2rx->dev,
> +			"Failed to start streams %d on subdev\n", 0);
>  		if (!csi2rx->count)
>  			csi2rx_stop(csi2rx);
> +		return ret;
>  	}
>  
> -out:
> -	mutex_unlock(&csi2rx->lock);
> -	return ret;
> +	csi2rx->count++;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520120022.539913-1-r-donadkar@ti.com?part=7