Re: [PATCH v23 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 59a42f5889e4e9fb6937e5811830bc3f796b016a
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 hardware block found on the
Kaanapali SoC, including write master configuration, CSID register and
address update forwarding, and Kaanapali VFE 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);
> +}

[Severity: High]
Can vfe_reg_update() and vfe_addr_update() race with csid_isr() when
updating csid->reg_update and csid->addr_update?

When userspace queues a buffer via vfe_queue_buffer_v2() or enables an
output line via vfe_enable_output_v2(), the VFE driver holds
vfe->output_lock and calls vfe_addr_update() or vfe_reg_update(), which
forwards through camss_addr_update() and camss_reg_update() to
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);
}

static void __csid_reg_update(struct csid_device *csid, int port_id)
{
	csid->reg_update |= CSID_RUP_AUP_RDI(port_id);
	writel(csid->reg_update, csid->base + CSID_RUP_CMD);

	__csid_aup_rup_trigger(csid);
}

Concurrently on another CPU, csid_isr() in camss-csid-1080.c clears those
same bitmasks upon INFO_RUP_DONE 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 (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i))
				camss_buf_done(csid->camss, csid->id, i);

If csid_isr() runs between the bitwise OR on csid->addr_update or
csid->reg_update and the writel() to CSID_AUP_CMD or CSID_RUP_CMD (or
races with updates on another RDI port of the same CSID), could the newly
set bit be cleared or clobbered before __csid_aup_rup_trigger() is called,
causing the hardware to miss a buffer address update while
output->gen2.active_num is incremented?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-kaanapali-camss-v23-0-95f250b18612@oss.qualcomm.com?part=10