linux-kernel - Re: [PATCH v7 10/16] media: cadence: csi2rx: add multistream support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <364c3b35-81a0-4e93-ad3b-a0fff3a29365@ideasonboard.com>
Date: Thu, 25 Sep 2025 15:44:13 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Rishikesh Donadkar <r-donadkar@...com>, jai.luthra@...ux.dev,
 laurent.pinchart@...asonboard.com, mripard@...nel.org
Cc: y-abhilashchandra@...com, devarsht@...com, s-jain1@...com,
 vigneshr@...com, mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 p.zabel@...gutronix.de, conor+dt@...nel.org, sakari.ailus@...ux.intel.com,
 hverkuil-cisco@...all.nl, jai.luthra@...asonboard.com,
 changhuang.liang@...rfivetech.com, jack.zhu@...rfivetech.com,
 sjoerd@...labora.com, hverkuil+cisco@...nel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v7 10/16] media: cadence: csi2rx: add multistream support

Hi,

On 11/09/2025 13:28, Rishikesh Donadkar wrote:
> From: Jai Luthra <j-luthra@...com>
> 
> Cadence CSI-2 bridge IP supports capturing multiple virtual "streams"
> of data over the same physical interface using MIPI Virtual Channels.
> 
> While the hardware IP supports usecases where streams coming in the sink
> pad can be broadcasted to multiple source pads, the driver will need
> significant re-architecture to make that possible. The two users of this
> IP in mainline linux are TI Shim and StarFive JH7110 CAMSS, and both
> have only integrated the first source pad i.e stream0 of this IP. So for
> now keep it simple and only allow 1-to-1 mapping of streams from sink to
> source, without any broadcasting.
> 
> The enable_streams() API in v4l2 supports passing a bitmask to enable
> each pad/stream combination individually on any media subdev. Use this
> API instead of  s_stream() API.
> 
> Implement the enable_stream and disable_stream hooks in place of the
> stream-unaware s_stream hook.
> 
> Implement a fallback s_stream hook that internally calls enable_stream
> on each source pad, for consumer drivers that don't use multi-stream
> APIs to still work. The helper function v4l2_subdev_s_stream_helper()
> form the v4l2 framework is not used here as it is meant only for the
> subedvs that have a single source pad and this hardware IP supports
> having multiple source pads.

<snip>

> +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);
> +	u64 sink_streams;
> +	int ret;
> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
> +						       CSI2RX_PAD_SINK,
> +						       &streams_mask);
> +
> +	guard(mutex)(&csi2rx->lock);

This looks a bit odd too. With enable/disable_streams, the state is
already locked. What is the mutex protecting?

j721e-csi2rx also has mutexes, and it's very unclear what they protect.
This should be described in the code.

I think in csi2rx the whole mutex can be just dropped.

j721e-csi2rx is a bit more complex, but there also I would consider if
and when the state lock protects the relevant parts already, and when
another lock is needed, what is the sequence to lock/unlock (e.g. always
csi->mutex first, then csi->subdev state lock), and make sure the code
follows that.

 Tomi