Re: [PATCH v13 4/5] media: qcom: camss: csid: Add support for CSID gen4

On 2026-05-08 21:22 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Hardcoded virtual channel (VC) parameter breaks multi-VC support and leaves dead code.
> - [High] Out-of-bounds array access on `csid->fmt` when configuring the 5th stream.
> - [High] Concurrent read-modify-write on shared hardware update state without locking.
> - [High] RDI and RUP_DONE interrupts are permanently masked, causing hardware update states to drift.
> - [Medium] Integer underflow in `__csid_configure_rx()` when `lane_cnt` is zero causes severe register corruption.
> - [Low] Meaningless back-to-back hardware command triggers due to redundant loop.
> --
> 
> commit ac71c3367c14093dee35b321f0ec55ce9cc5f586
> Author: Hangxiang Ma <[email protected]>
> 
> media: qcom: camss: csid: Add support for CSID gen4
>     
> Add more detailed resource information for CSID devices along with the
> driver for CSID gen4 that is responsible for CSID register configuration,
> module reset and IRQ handling for BUF_DONE events. And aggregate a common
> definition CSI2_RX_CFG0_PHY_SEL_BASE_IDX into csid header file.
>     
> In this CSID version, RUP and AUP update values are split into two
> registers along with a SET register. Accordingly, enhance the CSID
> interface to accommodate both the legacy combined reg_update and the
> split RUP and AUP updates.
>     
> Co-developed-by: Atiya Kailany <[email protected]>
> Signed-off-by: Atiya Kailany <[email protected]>
> Signed-off-by: Hangxiang Ma <[email protected]>
> 
> [ ... ]
> 
> > diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen4.c b/drivers/media/platform/qcom/camss/camss-csid-gen4.c
> > new file mode 100644
> > index 0000000000000..41035352c4bb0
> > --- /dev/null
> > +++ b/drivers/media/platform/qcom/camss/camss-csid-gen4.c
> > @@ -0,0 +1,376 @@
> 
> [ ... ]
> 
> > +static void __csid_aup_rup_clear(struct csid_device *csid, int port_id)
> > +{
> > +	/* Hardware clears the registers upon consuming the settings */
> > +	csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
> > +	csid->rup_update &= ~CSID_RUP_AUP_RDI(port_id);
> > +}
> 
> The variables csid->aup_update and csid->rup_update track global hardware
> update bits and are modified via non-atomic operations here and during
> stream configuration.
> 
> Since these are accessed from both process context and ISR context, could a
> race condition occur where an interrupt firing during a read-modify-write
> cycle corrupts the shadow states, silently dropping AUP/RUP commands for
> other active ports?

Possibly.

Worth checking and flagging, there's no need to start introducing
additional locks just for this SoC since this is a pattern we have not a
SoC specific thing.
 
> [ ... ]
> 
> > +static void __csid_configure_rx(struct csid_device *csid,
> > +				struct csid_phy_config *phy)
> > +{
> > +	int val;
> > +
> > +	val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
> 
> If phy->lane_cnt is 0, will the subtraction underflow to 0xFFFFFFFF?

lane_cnt != 0 is validated long before here.
 
> This would result in writing all 1s to the register, potentially causing
> hardware misconfiguration.
> 
> There is a fallback condition explicitly handling a 0 lane count in
> __csid_configure_rdi_stream(), which sets lane_cnt to 4 if it is 0.
> Should a similar check be added here?
> 
> [ ... ]
> 
> > +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
> > +{
> > +	u32 val;
> > +	u8 lane_cnt = csid->phy.lane_cnt;
> > +
> > +	/* Source pads matching RDI channels on hardware.
> > +	 * E.g. Pad 1 -> RDI0, Pad 2 -> RDI1, etc.
> > +	 */
> > +	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + port];
> 
> The macro MSM_CSID_MAX_SRC_STREAMS_GEN4 evaluates to 5 for non-lite CSID
> hardware, meaning the loop index port can reach 4.
> 
> Since MSM_CSID_PAD_FIRST_SRC is 1, MSM_CSID_PAD_FIRST_SRC + port evaluates
> to 5.

That's worrying.

Would appreciate some follow-up from patch authors on this one "yes we
looked no that can't happen" "yes we looked no it can't happen"

Either is acceptable.

> However, the csid->fmt array is sized by MSM_CSID_PADS_NUM, which remains
> hardcoded to 5, meaning valid indices are 0 through 4.
> 
> Can this result in an out-of-bounds memory read when attempting to process
> the 5th stream?
> 
> [ ... ]
> 
> > +static void csid_configure_stream(struct csid_device *csid, u8 enable)
> > +{
> > +	u8 i, k;
> > +
> > +	__csid_configure_rx(csid, &csid->phy);
> > +
> > +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_GEN4; i++) {
> > +		if (csid->phy.en_vc & BIT(i)) {
> > +			__csid_configure_rdi_stream(csid, enable, i, 0);
> > +			__csid_configure_rx_vc(csid, 0);
> 
> The functions __csid_configure_rdi_stream() and __csid_configure_rx_vc()
> are invoked with a hardcoded 0 for the vc argument instead of passing the
> actual virtual channel or the loop index i.
> 
> Does this force all enabled streams to capture Virtual Channel 0, breaking
> the driver's ability to handle multiple virtual channels?

No, the driver's ability to handle VCs isn't there.
 
> Additionally, passing 0 to __csid_configure_rx_vc() makes the internal
> condition if (vc > 3) always evaluate to false. Is this code intended
> to be reachable?
> 
> > +
> > +			for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
> > +				__csid_aup_update(csid, i);
> 
> CAMSS_INIT_BUF_COUNT is intended for ping-pong buffer queues in the VFE.
> Since CSID does not manage memory buffers, this loop repeatedly triggers
> identical AUP commands to the hardware without any intervening state
> changes.
> 
> Is it necessary to repeatedly trigger these commands here?
> 
> [ ... ]
> 
> > +static irqreturn_t csid_isr(int irq, void *dev)
> > +{
> > +	struct csid_device *csid = dev;
> > +	u32 val, buf_done_val;
> > +	u8 reset_done;
> > +	int i;
> > +
> > +	val = readl(csid->base + CSID_TOP_IRQ_STATUS);
> > +	writel(val, csid->base + CSID_TOP_IRQ_CLEAR);
> > +
> > +	reset_done = val & INFO_RST_DONE;
> > +
> > +	buf_done_val = readl(csid->base + CSID_BUF_DONE_IRQ_STATUS);
> > +	writel(buf_done_val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> > +
> > +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_GEN4; i++) {
> > +		if (csid->phy.en_vc & BIT(i)) {
> > +			val = readl(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
> > +			writel(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
> > +
> > +			if (val & INFO_RUP_DONE)
> > +				csid_subdev_reg_update(csid, i, true);
> 
> The macro CSID_CSI2_RDIN_IRQ_MASK is defined in the source but never written
> to hardware.
> 
> Additionally, in csid_reset() below, only INFO_RST_DONE and
> BUF_DONE_IRQ_STATUS are unmasked in CSID_TOP_IRQ_MASK.
> 
> Without unmasking CSI2_RX_IRQ_STATUS at the top level and writing to
> CSID_CSI2_RDIN_IRQ_MASK, will the INFO_RUP_DONE interrupt ever fire?

Reset is 0xFFFFFFFF.
 
> If this interrupt is permanently masked, __csid_aup_rup_clear() won't be
> called, which might cause the software cache of the hardware triggers
> to permanently drift out of sync.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
>