Re: [PATCH v2 8/9] media: camss: vfe: add the gen4 block and SM8750 camera subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: camss: race on csid reg_update state between vfe_reg_update() and csid_isr()
--

commit 5ba0f2cd4903e9301f9d8d170289c6e55be33cde
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

media: camss: vfe: add the gen4 block and SM8750 camera subsystem

This commit adds VFE gen4 hardware support where buffer done, register update
completion, and frame start interrupts are handled by the CSID, and requires
256-byte alignment for DMA addresses and frame sizes. It also adds the
SM8750 camera subsystem resource definitions and compatible string.

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen4.c b/drivers/media/platform/qcom/camss/camss-vfe-gen4.c
> new file mode 100644
> index 0000000000000..2ee82898f37dc
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-gen4.c
> @@ -0,0 +1,212 @@

[ ... ]

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

[Severity: Medium]
Can vfe_reg_update() and vfe_reg_update_clear() in
drivers/media/platform/qcom/camss/camss-vfe-gen4.c race with csid_isr() in
drivers/media/platform/qcom/camss/camss-csid-980.c when updating the CSID
register update state?

On SM8750, vfe_res_8750 (using vfe_ops_gen4) is paired with csid_res_8750
(using csid_ops_980). When userspace queues a buffer via VIDIOC_QBUF while
output->gen2.active_num is below 2, vfe_queue_buffer_v2() acquires
vfe->output_lock and calls:

  vfe_reg_update()
    -> camss_reg_update(vfe->camss, vfe->id, port_id, false)
      -> csid_subdev_reg_update(csid, port_id, false)
        -> __csid_aup_update(csid, port_id)

In drivers/media/platform/qcom/camss/camss-csid-980.c, __csid_aup_update()
performs a non-atomic read-modify-write on csid->reg_update or
csid->aup_update and writes it to hardware:

	if (csid_is_lite(csid)) {
		csid->reg_update |= CSID_LITE_AUP_RDI(port_id);
		writel(csid->reg_update, csid->base + CSID_LITE_RUP_AUP_CMD);
	} else {
		csid->aup_update |= CSID_RUP_AUP_RDI(port_id);
		writel(csid->aup_update, csid->base + CSID_AUP_CMD);

		__csid_full_aup_rup_trigger(csid);
	}

Concurrently on another CPU, the CSID interrupt handler csid_isr() in
drivers/media/platform/qcom/camss/camss-csid-980.c handles INFO_RUP_DONE
without holding vfe->output_lock or any CSID lock:

			if (val & INFO_RUP_DONE)
				csid_subdev_reg_update(csid, i, true);

which calls __csid_aup_rup_clear():

	if (csid_is_lite(csid)) {
		csid->reg_update &= ~CSID_LITE_RUP_RDI(port_id);
		csid->reg_update &= ~CSID_LITE_AUP_RDI(port_id);
	} else {
		csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
		csid->rup_update &= ~CSID_RUP_AUP_RDI(port_id);
	}

A similar race can also happen when multiple RDI ports stream on the same
CSID and vfe_enable_output_v2(), csid_configure_stream(), or
vfe_queue_buffer_v2() run concurrently with csid_isr().

Could this unsynchronized read-modify-write lose a pending address update
(AUP) bit or re-write a stale RUP/AUP bit to CSID_AUP_CMD or
CSID_LITE_RUP_AUP_CMD, causing dropped buffer updates or stalled frame
capture?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028093003.722745-1-xiaqinghuan@mail.ustc.edu.cn?part=8