Re: [PATCH v22 08/14] media: qcom: camss: csid: Add support for CSID 1080

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: qcom: camss: out-of-bounds fmt[] read in __csid_configure_rdi_stream()
- [Medium] media: qcom: camss: unsynchronized aup_update and rup_update in csid-1080
--

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

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

This commit adds hardware resource definitions, register programming, reset
handling, and interrupt handling for the CSID 1080 module found on the
Kaanapali SoC. It also extends the CSID interface to support separate RUP and
AUP update registers committed via a shared SET register.

> 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..9830fa0f8d24e
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> @@ -0,0 +1,402 @@

[ ... ]

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

[ ... ]

> +static void __csid_aup_rup_trigger(struct csid_device *csid)
> +{
> +	/* trigger SET in combined register */
> +	writel(RUP_SET, csid->base + CSID_RUP_AUP_CMD);
> +}
> +
> +static void __csid_addr_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_addr_update_clear(struct csid_device *csid, int port_id)
> +{
> +	csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
> +
> +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_reg_update_clear(struct csid_device *csid, int port_id)
> +{
> +	csid->rup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}

[Severity: Medium]
Can concurrent updates to csid->aup_update and csid->rup_update race with
csid_isr()?

During streaming, vfe_queue_buffer_v2() and vfe_enable_output_v2() in
drivers/media/platform/qcom/camss/camss-vfe.c call ops->addr_update(),
ops->reg_update(), ops->addr_update_clear(), and ops->reg_update_clear()
while holding vfe->output_lock.

However, csid_isr() in drivers/media/platform/qcom/camss/camss-csid-1080.c
clears those same bitmasks in hardirq context when INFO_RUP_DONE is set,
without holding vfe->output_lock or any CSID-level spinlock:

drivers/media/platform/qcom/camss/camss-csid-1080.c:csid_isr() {
	...
			if (val & INFO_RUP_DONE) {
				__csid_addr_update_clear(csid, i);
				__csid_reg_update_clear(csid, i);
			}
	...
}

In addition, csid_configure_stream() calls __csid_addr_update() and
__csid_reg_update() in process context without holding a lock after
vfe_enable_output_v2() has already set output->state to VFE_OUTPUT_ON and
while csid->irq is enabled.

Could these unsynchronized read-modify-write operations on csid->aup_update
and csid->rup_update, along with the writes to CSID_AUP_CMD, CSID_RUP_CMD,
and CSID_RUP_AUP_CMD, clobber a newly set RDI bit or write a just-cleared RDI
bit back into the command registers?

[ ... ]

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

[Severity: High]
Does __csid_configure_rdi_stream() read past the end of csid->fmt[] when
configuring the fifth stream on non-lite CSID 1080 instances?

For non-lite CSID 1080 instances (CSID0, CSID1, and CSID2 in
csid_res_kaanapali), MSM_CSID_MAX_SRC_STREAMS_1080 evaluates to 5. In
csid_configure_stream(), the loop iterates i from 0 to 4 and calls
__csid_configure_rdi_stream(csid, enable, i, 0) when BIT(i) is set in
csid->phy.en_vc.

However, in drivers/media/platform/qcom/camss/camss-csid.h, MSM_CSID_PADS_NUM
is still defined as 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 (RDI4), MSM_CSID_PAD_FIRST_SRC + port evaluates to 5, which is
one element past the end of the 5-element csid->fmt[] array. In addition,
msm_csid_register_entity() in drivers/media/platform/qcom/camss/camss-csid.c
only initializes MSM_CSID_PADS_NUM pads on the media entity, leaving RDI4
without a source media pad.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-kaanapali-camss-v22-0-7da3ea49c8a1@oss.qualcomm.com?part=8