Re: [PATCH v19 08/12] media: qcom: camss: vfe: Add support for VFE 1080

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: camss: Hardware constraint violation on AUP_UPDATE count
- [High] media: camss: Hardcoded VFE-to-CSID mapping breaks dynamic media routing
- [High] media: camss: Data race on csid->aup_update and csid->rup_update
--

commit f4a9516b2392f133fd18f4aed515b9cb62df16ca
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
    
    media: qcom: camss: vfe: Add support for VFE 1080
    
    Add support for the VFE (Video Front End) 1080 found on Kaanapali SoC.
    In the Kaanapali camera subsystem, the front-end blocks are referred to
    as TFEs.

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

[ ... ]

> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);

[Severity: High]
Does hardcoding vfe->id introduce a regression for dynamic media routing?

The Media Controller framework allows dynamic routing, where any CSID source
could be linked to any VFE sink (e.g., CSID[0] to VFE[2]).

Here, camss_reg_update() is called with vfe->id, which indexes the CSID array
directly:

camss.c:camss_reg_update() {
	csid = &camss->csid[hw_id];
	csid->res->hw_ops->reg_update(csid, port_id, is_clear);
}

If CSID[0] is routed to VFE[2], the VFE buffer queue will update the
registers of CSID[2] instead of the active CSID[0], leaving the active
CSID without its AUP_UPDATE and stalling the stream.

[Severity: High]
Can this introduce a data race regression on the CSID shadow states?

When userspace queues new buffers via vfe_queue_buffer_v2(), it executes from
process context and eventually calls this vfe_reg_update(). This ends up in
__csid_aup_update() which performs an unprotected read-modify-write:

camss-csid-1080.c:__csid_aup_update() {
	csid->aup_update |= CSID_RUP_AUP_RDI(port_id);
}

At the same time, the CSID hardware interrupt handler csid_isr() processes
completions and performs a similar unprotected read-modify-write:

camss-csid-1080.c:__csid_aup_rup_clear() {
	csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
}

Since these modifications can interleave without locks, one CPU's state
update could be lost, potentially corrupting the CSID_AUP_CMD register state.

> +}

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 2680a9914765f..3acef6ba97dc9 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c

[ ... ]

> @@ -552,7 +554,9 @@ int vfe_enable_output_v2(struct vfe_line *line)
>  		output->gen2.active_num++;
>  		ops->vfe_wm_update(vfe, output->wm_idx[0],
>  				   output->buf[i]->addr[0], line);
> -		ops->reg_update(vfe, line->id);
> +
> +		if (!vfe->res->reg_update_after_csid_config)
> +			ops->reg_update(vfe, line->id);
>  	}

[Severity: High]
Does this introduce a regression by violating the AUP_UPDATE hardware
constraint?

The commit message states that "the number of AUP_UPDATE requests must match
the number of buffers queued to the write master while it is being enabled."

However, if userspace only queued 1 buffer, the loop in vfe_enable_output_v2()
will break early (as it loops up to CAMSS_INIT_BUF_COUNT):

	for (i = 0; i < CAMSS_INIT_BUF_COUNT; i++) {
		output->buf[i] = vfe_buf_get_pending(output);
		if (!output->buf[i])
			break;

This means the VFE write master gets configured with only 1 buffer.

Meanwhile, csid_configure_stream() in camss-csid-1080.c unconditionally issues
CAMSS_INIT_BUF_COUNT updates:

	for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
		__csid_aup_update(csid, i);

If the CSID sends 2 AUP_UPDATE commands but the VFE is only configured with
1 buffer, can this cause the hardware to perform out-of-bounds DMA or
trigger an IOMMU fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-kaanapali-camss-v19-0-b2c583ef50e0@oss.qualcomm.com?part=8

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy