Re: [PATCH v20 08/13] 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: vfe: AUP_UPDATE mismatch during stream enable violates hardware constraint
- [High] media: csid: lockless read-modify-write race on csid->aup_update from process and hardirq
- [Medium] media: vfe: hardcoded vfe->id mapping breaks Media Controller cross-routing
--

commit d6f09b2d35220bf1f91873f665e2de70196c2e83
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.
The driver continues to use VFE naming, and Kaanapali requires REG_UPDATE
and AUP_UPDATE to be issued only after CSID configuration.

> 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: Medium]
The CAMSS driver framework allows flexible stream routing, meaning userspace
can link any CSID pad to any VFE pad via the Media Controller graph (e.g.,
routing csid0 to vfe1).

By hardcoding the hardware ID to the local VFE's ID (vfe->id), if csid0 is
routed to vfe1, vfe_reg_update() will look up csid1 and incorrectly issue the
buffer AUP_UPDATE commands to the csid1 hardware instead of the active csid0.

Could this drop queued buffers on the active stream and corrupt the register
state of the inactive CSID?

[Severity: High]
This wires up vfe_reg_update() to camss_reg_update(), bridging VFE's buffer
queue path into the CSID register update path.

vfe_queue_buffer_v2() executes in process context and acquires
vfe->output_lock (which disables local interrupts but does not prevent
execution on other CPUs), and it triggers vfe_reg_update() ->
__csid_aup_update(). This performs a non-atomic read-modify-write on shared
state:

    csid->aup_update |= CSID_RUP_AUP_RDI(port_id);

Concurrently, csid_isr() can execute in hardirq context on another CPU. It
calls __csid_aup_rup_clear(), which modifies the exact same variable
locklessly:

    csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);

Since __csid_aup_update() and __csid_aup_rup_clear() use non-atomic standard
bitwise operators (|= and &=) from competing contexts without a shared
spinlock, does this form a TOCTOU data race that will corrupt
csid->aup_update? Could this result in lost register updates, dropped buffers,
and pipeline stalls?

[ ... ]

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

[ ... ]

> @@ -552,7 +553,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]
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."
Here in vfe_enable_output_v2(), the loop fetching queued buffers breaks early
if fewer than CAMSS_INIT_BUF_COUNT (2) buffers are available, meaning it is
valid for userspace to queue only 0 or 1 buffers.

However, csid_configure_stream() in camss-csid-1080.c indiscriminately loops
and issues exactly 2 AUP_UPDATE requests unconditionally, ignoring the VFE's
actual active_num.

Does this mismatch violate the hardware constraint and cause the VFE Write
Master to fetch uninitialized buffer addresses (0x0) for the missing updates,
potentially leading to DMA faults or physical memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-kaanapali-camss-v20-0-63e98ce9ac0c@oss.qualcomm.com?part=8