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

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The gen4 CSID hardware advertises support for 5 source streams, but the media pad arrays are hardcoded to a maximum of 4 source streams, rendering the 5th stream unreachable dead code.

Pre-existing issues:
- [High] Missing 0-check for `phy->lane_cnt` in `__csid_configure_rx()` causes an integer underflow, resulting in `0xFFFFFFFF` being written to the hardware register.
- [High] Lockless read-modify-write of `csid->aup_update` and `csid->rup_update` between process context and IRQ context causes lost register updates.
--

commit c0660384d930d560819871fc94413f7382296c54
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

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. In this CSID version,
RUP and AUP update values are split into two registers along with a SET
register.

> 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..6e5ebeefd010e
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen4.c
> @@ -0,0 +1,380 @@

[ ... ]

> +/* CSI2 RX interrupt registers */
> +#define CSID_CSI2_RX_IRQ_STATUS			0x1B0
> +#define CSID_CSI2_RX_IRQ_MASK			0x1B4
> +#define CSID_CSI2_RX_IRQ_CLEAR			0x1B8
> +#define CSID_CSI2_RX_IRQ_SET			0x1BC
> +
> +/* CSI2 RX Configuration */
> +#define CSID_CSI2_RX_CFG0			0x880
> +#define		CSI2_RX_CFG0_NUM_ACTIVE_LANES		0
> +#define		CSI2_RX_CFG0_DL0_INPUT_SEL		4
> +#define		CSI2_RX_CFG0_PHY_NUM_SEL		20
> +#define		CSI2_RX_CFG0_PHY_SEL_BASE_IDX		1
> +#define CSID_CSI2_RX_CFG1			0x884
> +#define		CSI2_RX_CFG1_ECC_CORRECTION_EN		BIT(0)
> +#define		CSI2_RX_CFG1_VC_MODE			BIT(2)
> +
> +#define MSM_CSID_MAX_SRC_STREAMS_GEN4		(csid_is_lite(csid) ? 4 : 5)

[Severity: Low]
Does this make the 5th stream unreachable dead code?

MSM_CSID_MAX_SRC_STREAMS_GEN4 evaluates to 5. However, MSM_CSID_PADS_NUM is
globally restricted to 5 (1 sink + 4 source pads) in camss-csid.h. During
link setup, valid array indices cap out at 4, preventing BIT(4) from ever
being set in csid->phy.en_vc.

Because of this, the loop executing i = 4 in stream configuration can never
run, rendering the 5th stream dead code.

> +
> +/* RDI Configuration */
> +#define CSID_RDI_CFG0(rdi)	(csid_is_lite(csid) ?\

[ ... ]

> +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);

[Severity: High]
This is a pre-existing issue, but does this lockless read-modify-write
cause lost register updates?

Process context uses non-atomic bitwise OR (|=) to trigger register updates
via __csid_aup_update() and __csid_reg_update(). Concurrently, the IRQ
service routine uses a non-atomic bitwise AND NOT (&= ~) inside
__csid_aup_rup_clear() to acknowledge completions.

If an interrupt pre-empts process context during the RMW instruction cycle,
the ISR's clearing operation will be silently overwritten by process context,
resulting in lost or phantom register updates.

> +}
> +
> +static void __csid_aup_update(struct csid_device *csid, int port_id)
> +{
> +	csid->aup_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->aup_update, csid->base + CSID_AUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_reg_update(struct csid_device *csid, int port_id)
> +{
> +	csid->rup_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->rup_update, csid->base + CSID_RUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +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;

[Severity: High]
This isn't a bug introduced by this patch, but does this cause an integer
underflow if phy->lane_cnt is 0?

When the test generator is enabled without an active input link,
phy->lane_cnt remains zero-initialized. This causes (phy->lane_cnt - 1) to
underflow to -1 (0xFFFFFFFF), which is then written directly into
CSID_CSI2_RX_CFG0, corrupting hardware configurations.

There is a safety fallback later in __csid_configure_rdi_stream():

	if (!lane_cnt)
		lane_cnt = 4;

However, this fallback acts as a dead store and does not protect the RX
path configured here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-kaanapali-camss-v14-0-e76f26aa6691@oss.qualcomm.com?part=4