Re: [PATCH v21 09/14] media: qcom: camss: vfe: Add support for VFE 1080 - sashiko-bot

From: sashiko-bot@kernel.org
To: "Hangxiang Ma" <hangxiang.ma@oss.qualcomm.com>
Cc: vkoul@kernel.org, olteanv@gmail.com, media-ci@linuxtv.org,
	linux-phy@lists.infradead.org, conor+dt@kernel.org,
	robh@kernel.org, devicetree@vger.kernel.org,
	neil.armstrong@linaro.org
Subject: Re: [PATCH v21 09/14] media: qcom: camss: vfe: Add support for VFE 1080
Date: Mon, 12 Oct 2026 07:00:43 +0000	[thread overview]
Message-ID: <sashiko-outbox-167764@kernel.org> (raw)
In-Reply-To: <20261011-kaanapali-camss-v21-9-04fd28d815c6@oss.qualcomm.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: camss: hardcoded AUP_UPDATE count violates hardware constraint
- [High] media: camss: data race on csid->aup_update and rup_update
--

commit bab5581422e8c33728a37abe32648420503fc60c
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.

> Kaanapali requires REG_UPDATE and AUP_UPDATE to be issued only after all
> CSID configuration has completed. In addition, the number of AUP_UPDATE
> requests must match the number of buffers queued to the write master
> while it is being enabled.

[Severity: High]
Does the current implementation actually enforce this requirement?

If userspace queues 1 buffer via V4L2 and calls STREAMON, the vb2_queue
lacks a min_queued_buffers limit in msm_video_register(). Consequently,
vfe_enable_output_v2() dynamically queues only 1 buffer to the hardware,
but csid_configure_stream() in camss-csid-1080.c unconditionally issues
CAMSS_INIT_BUF_COUNT (2) AUP_UPDATE commands.

Can this mismatch lead to write master state corruption or IOMMU faults
due to hardware desynchronization?

> 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 this introduce a data race on csid->aup_update and rup_update?

This call reaches __csid_aup_update() in process context. While local
interrupts are disabled via vfe->output_lock, there is no cross-CPU
synchronization.

Concurrently, csid_isr() fires in hardirq context (potentially on another CPU)
and calls __csid_aup_rup_clear(), which performs a bitwise clear on the same
state variables without holding a shared lock:

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

Can these concurrent non-atomic bitwise operations cause lost updates
and corrupt the CSID_AUP_CMD and CSID_RUP_CMD register states, leading
to stream stalls or hardware misconfiguration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-kaanapali-camss-v21-0-04fd28d815c6@oss.qualcomm.com?part=9