Re: [PATCH v4 3/6] media: synopsys: Add support for multiple streams

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect pad arguments to `v4l2_subdev_state_xlate_streams()` silently corrupt stream masks and cause PM runtime underflows.
- [High] Propagating a remote subdevice disable error causes state desynchronization and double PM runtime put on retry.
--

commit dc1fc68d798970cf70a238272f82256db2545536
Author: Guoniu Zhou <guoniu.zhou@oss.nxp.com>

media: synopsys: Add support for multiple streams

The driver is updated to support multiple concurrent streams by tracking
enabled streams with a bitmask, instead of only supporting single stream
operation. The hardware is now initialized only once for the first stream.

> diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> index f45466ede2bb7..92178a3dec5d5 100644
> --- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> +++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
[ ... ]
> @@ -539,26 +540,33 @@ static int dw_mipi_csi2rx_enable_streams(struct v4l2_subdev *sd,
>  					       DW_MIPI_CSI2RX_PAD_SRC,
>  					       &streams_mask);
>  
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		goto err;
> +	if (!csi2->enabled_streams) {
> +		ret = pm_runtime_resume_and_get(dev);

Does this sequence properly handle the streams_mask translation?

The .enable_streams() and .disable_streams() operations are exclusively
invoked on source pads, meaning streams_mask represents the streams on
DW_MIPI_CSI2RX_PAD_SRC.

When calling v4l2_subdev_state_xlate_streams(), the driver passes
DW_MIPI_CSI2RX_PAD_SINK as pad0 and DW_MIPI_CSI2RX_PAD_SRC as pad1. However,
the helper expects *streams to be the mask for pad0.

Because the helper evaluates the SRC streams mask against route->sink_stream,
translation will fail for any asymmetric routing where sink and source stream
IDs differ.

On failure, streams_mask is mutated to 0. In enable_streams(), this results
in pm_runtime_resume_and_get() executing, but csi2->enabled_streams remaining
0.

Since the V4L2 core still marks the stream as enabled, userspace could
interleave symmetric and asymmetric stream enable/disable calls (e.g., enable
symmetric -> PM get; enable asymmetric -> no PM get; disable symmetric ->
PM put; disable asymmetric -> PM put).

Could this sequence underflow the PM runtime usage counter and PHY power
count, breaking device power management?

[ ... ]
> @@ -583,10 +591,15 @@ static int dw_mipi_csi2rx_disable_streams(struct v4l2_subdev *sd,
>  					       &streams_mask);
>  
>  	ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask);
> +	if (ret)
> +		dev_err(dev, "failed to disable streams on remote subdev: %d\n", ret);
>  
> -	dw_mipi_csi2rx_stop(csi2);
> +	csi2->enabled_streams &= ~streams_mask;
>  
> -	pm_runtime_put(dev);
> +	if (!csi2->enabled_streams) {
> +		dw_mipi_csi2rx_stop(csi2);
> +		pm_runtime_put(dev);
> +	}
>  
>  	return ret;

Could propagating the remote disable error here cause state synchronization
issues with the V4L2 core?

If v4l2_subdev_disable_streams() on the remote subdevice fails, the driver
proceeds to clear its local state (csi2->enabled_streams &= ~streams_mask),
shuts down the hardware, and calls pm_runtime_put() if the mask becomes 0.

The driver then returns the error to the V4L2 core. Because of this error,
the V4L2 core aborts and skips marking the stream as disabled, meaning the
framework still considers the stream active.

If userspace retries the disable operation, the driver will be invoked again.
Since csi2->enabled_streams was already cleared, !csi2->enabled_streams would
evaluate to true again. This results in a redundant dw_mipi_csi2rx_stop() and
pm_runtime_put() being executed.

Could this lead to a PM usage underflow and illegal writes to powered-off
PHY registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-csi2_imx95-v4-0-84ea4bb78a88@oss.nxp.com?part=3