Re: [PATCH v24 10/15] media: qcom: camss: vfe: Add support for VFE 1080

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: qcom: camss: unsynchronized CSID RUP/AUP updates in vfe_ops_1080
--

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

media: qcom: camss: vfe: Add support for VFE 1080

This commit adds support for the VFE 1080 (TFE) hardware block found on the
Qualcomm Kaanapali SoC, including register configuration and subdevice
resource definitions.

> 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..68798f13fbea8
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-1080.c
> @@ -0,0 +1,213 @@

[ ... ]

> +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);
> +}
> +
> +static void vfe_reg_update_clear(struct vfe_device *vfe,
> +				 enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}
> +
> +static void vfe_addr_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_addr_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static void vfe_addr_update_clear(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_addr_update(vfe->camss, vfe->id, port_id, true);
> +}

[Severity: High]
As reported by Sashiko on v14 [1], can vfe_reg_update() and vfe_addr_update()
race with csid_isr() in drivers/media/platform/qcom/camss/camss-csid-1080.c
when updating csid->reg_update and csid->addr_update?

In drivers/media/platform/qcom/camss/camss-vfe.c, vfe_enable_output_v2(),
vfe_queue_buffer_v2(), and vfe_buf_done() invoke vfe_reg_update() and
vfe_addr_update() while holding vfe->output_lock. These forward through
camss_reg_update() and camss_addr_update() to __csid_reg_update() and
__csid_addr_update() in drivers/media/platform/qcom/camss/camss-csid-1080.c:

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

Concurrently, when an end-of-frame interrupt fires with INFO_RUP_DONE set,
csid_isr() in drivers/media/platform/qcom/camss/camss-csid-1080.c clears
those fields without holding vfe->output_lock or any CSID lock before calling
camss_buf_done():

    if (val & INFO_RUP_DONE) {
            __csid_addr_update_clear(csid, i);
            __csid_reg_update_clear(csid, i);
    }

If userspace queues a buffer via VIDIOC_QBUF -> vfe_queue_buffer_v2() when
output->gen2.active_num == 1 (or enables another RDI port on the same CSID
via VIDIOC_STREAMON -> vfe_enable_output_v2()) on one CPU while csid_isr()
processes INFO_RUP_DONE on another CPU, the unsynchronized read-modify-write
operations on csid->addr_update and csid->reg_update can clear a newly set
bit before writel() writes CSID_AUP_CMD or CSID_RUP_CMD, or clobber bits
across RDI ports. Would this cause the hardware to miss a buffer address
update while output->gen2.active_num is incremented, stalling capture?

[1] https://lore.kernel.org/r/20260601164403.8CE831F00893@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-kaanapali-camss-v24-0-d6b8f3f6f66f@oss.qualcomm.com?part=10