Re: [PATCH v23 09/15] media: qcom: camss: csid: Add support for CSID 1080

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: qcom: camss: out-of-bounds fmt access in __csid_configure_rdi_stream()
- [Medium] media: qcom: camss: csid-1080: race on addr_update and reg_update bitmasks
- [Low] media: qcom: camss: unused variable lane_cnt in __csid_configure_rdi_stream()
--

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

media: qcom: camss: csid: Add support for CSID 1080

This commit adds hardware resource definitions and driver support for the
CSID 1080 module on the Qualcomm Kaanapali SoC, including RUP/AUP register
update handling, stream configuration, reset, and BUF_DONE interrupt
handling.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1080.c b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> new file mode 100644
> index 0000000000000..3d90d31201fa5
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1080.c

[ ... ]

> +#define MSM_CSID_MAX_SRC_STREAMS_1080		(csid_is_lite(csid) ? 4 : 5)

[ ... ]

> +static void __csid_addr_update(struct csid_device *csid, int port_id)
> +{
> +	csid->addr_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->addr_update, csid->base + CSID_AUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_addr_update_clear(struct csid_device *csid, int port_id)
> +{
> +	csid->addr_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
> +
> +static void __csid_reg_update(struct csid_device *csid, int port_id)
> +{
> +	csid->reg_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->reg_update, csid->base + CSID_RUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_reg_update_clear(struct csid_device *csid, int port_id)
> +{
> +	csid->reg_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}

[ ... ]

> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
> +{
> +	u8 lane_cnt = csid->phy.lane_cnt;
> +	u32 val;
> +
> +	/* 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];
> +	const struct csid_format_info *format = csid_get_fmt_entry(csid->res->formats->formats,
> +								   csid->res->formats->nformats,
> +								   input_format->code);
> +
> +	if (!lane_cnt)
> +		lane_cnt = 4;

[Severity: Low]
In __csid_configure_rdi_stream(), lane_cnt is initialized and updated here
but never read anywhere in the function. Will this trigger an
-Wunused-but-set-variable warning?

> +
> +	val = RDI_CFG0_TIMESTAMP_EN;
> +	val |= RDI_CFG0_TIMESTAMP_STB_SEL;
> +	val |= RDI_CFG0_RETIME_BS;
> +
> +	/* note: for non-RDI path, this should be format->decode_format */
> +	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
> +	val |= vc << RDI_CFG0_VC;
> +	val |= format->data_type << RDI_CFG0_DT;
> +	writel(val, csid->base + CSID_RDI_CFG0(port));
> +
> +	val = RDI_CFG1_PACKING_FORMAT_MIPI;
> +	writel(val, csid->base + CSID_RDI_CFG1(port));
> +
> +	/* Configure pixel store using dedicated register in 1080 */
> +	if (!csid_is_lite(csid))
> +		__csid_configure_rdi_pix_store(csid, port);
> +
> +	val = 0;
> +	writel(val, csid->base + CSID_RDI_CTRL(port));
> +
> +	val = readl(csid->base + CSID_RDI_CFG0(port));
> +
> +	if (enable)
> +		val |= RDI_CFG0_EN;
> +
> +	writel(val, csid->base + CSID_RDI_CFG0(port));
> +}
> +
> +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_1080; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			__csid_configure_rdi_stream(csid, enable, i, 0);

[Severity: High]
Can this read past the end of csid->fmt[] on non-lite CSID 1080 instances?

For non-lite instances (CSID0, CSID1, CSID2 in csid_res_kaanapali),
MSM_CSID_MAX_SRC_STREAMS_1080 evaluates to 5, so csid_configure_stream()
iterates i from 0 to 4 and passes port = 4 into
__csid_configure_rdi_stream().

Inside __csid_configure_rdi_stream(), input_format is computed as:

	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + port];

However, in drivers/media/platform/qcom/camss/camss-csid.h,
MSM_CSID_PAD_FIRST_SRC is 1 and MSM_CSID_PADS_NUM is still 5:

	#define MSM_CSID_PAD_SINK 0
	#define MSM_CSID_PAD_FIRST_SRC 1
	#define MSM_CSID_PADS_NUM 5
	...
	struct csid_device {
		...
		struct media_pad pads[MSM_CSID_PADS_NUM];
		...
		struct v4l2_mbus_framefmt fmt[MSM_CSID_PADS_NUM];

When port is 4, MSM_CSID_PAD_FIRST_SRC + port is 5, which indexes one
element past the end of csid->fmt[5] when reading input_format->code (and
msm_csid_register_entity() only registers 4 source pads). Should
MSM_CSID_PADS_NUM and MSM_CSID_MAX_SRC_STREAMS be updated to accommodate
the 5th RDI stream, or should MSM_CSID_MAX_SRC_STREAMS_1080 be capped to
the number of source pads?

> +			__csid_configure_rx_vc(csid, 0);
> +
> +			for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
> +				__csid_addr_update(csid, i);
> +
> +			__csid_reg_update(csid, i);
> +
> +			__csid_ctrl_rdi(csid, enable, i);
> +		}
> +	}
> +}

[ ... ]

> +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_1080; 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_addr_update_clear(csid, i);
> +				__csid_reg_update_clear(csid, i);
> +			}
> +
> +			if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i))
> +				camss_buf_done(csid->camss, csid->id, i);

[Severity: Medium]
Could the unlocked updates to csid->addr_update and csid->reg_update here
race with vfe_queue_buffer_v2() and vfe_enable_output_v2(), or re-trigger
updates for other RDI ports?

In drivers/media/platform/qcom/camss/camss-vfe.c, vfe_queue_buffer_v2() and
vfe_enable_output_v2() call ops->addr_update() and ops->reg_update() while
holding vfe->output_lock:

	vfe_queue_buffer_v2()
	  spin_lock_irqsave(&vfe->output_lock, flags);
	  ops->addr_update(vfe, line->id)
	    -> csid_subdev_addr_update()
	      -> __csid_addr_update()
	  ops->reg_update(vfe, line->id)
	    -> csid_subdev_reg_update()
	      -> __csid_reg_update()

In csid_isr(), __csid_addr_update_clear() and __csid_reg_update_clear()
modify csid->addr_update and csid->reg_update without holding
vfe->output_lock or a CSID spinlock. If csid_isr() runs on one CPU while
vfe_queue_buffer_v2() or vfe_enable_output_v2() runs on another CPU, the
read-modify-write on csid->addr_update and csid->reg_update can lose a
newly set RDI bit or write a stale cleared bit back into CSID_AUP_CMD and
CSID_RUP_CMD.

Also, within this loop in csid_isr(), camss_buf_done() -> vfe_buf_done() is
called for port i before INFO_RUP_DONE is checked and cleared for ports
j > i. When vfe_buf_done() calls __csid_addr_update(csid, i), it writes the
entire csid->addr_update bitmask to CSID_AUP_CMD while completed ports
j > i still have their CSID_RUP_AUP_RDI(j) bits set. Should INFO_RUP_DONE
be processed for all ports before invoking camss_buf_done(), or does
CSID_AUP_CMD / CSID_RUP_CMD only need CSID_RUP_AUP_RDI(port_id) written
directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-kaanapali-camss-v23-0-95f250b18612@oss.qualcomm.com?part=9