Re: [PATCH 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 9a87e4e329c131d366318a8a449031db90e2ea2c
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

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

This commit adds support for the gen4 VFE block, where buffer done, register
update completion, and start of frame events come from the 980 CSID, and
requires 256-byte alignment for DMA addresses and frame sizes. It also
instantiates the CSIPHY, CSID, VFE, interconnect, and top-level resources for
SM8750.

> 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() race with csid_isr() on csid->aup_update,
csid->rup_update, and csid->reg_update?

When an active stream has fewer than two buffers in hardware
(output->gen2.active_num < 2) and userspace queues a buffer via VIDIOC_QBUF,
vfe_queue_buffer_v2() acquires vfe->output_lock and calls into CSID 980:

vfe_queue_buffer_v2()
  vfe_reg_update()
    camss_reg_update(..., false)
      csid_subdev_reg_update(..., false)
        __csid_aup_update()

In drivers/media/platform/qcom/camss/camss-csid-980.c, __csid_aup_update()
modifies csid->reg_update or csid->aup_update and then re-reads the field to
write the command register:

	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, csid_isr() in camss-csid-980.c handles
INFO_RUP_DONE without holding vfe->output_lock or any CSID lock:

csid_isr()
  if (val & INFO_RUP_DONE)
    csid_subdev_reg_update(csid, i, true)
      __csid_aup_rup_clear()

where __csid_aup_rup_clear() performs a non-atomic read-modify-write:

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

If __csid_aup_rup_clear() clears the bit between the |= and the writel() in
__csid_aup_update() (or races across RDI ports on the same CSID when
vfe_enable_output_v2() or vfe_queue_buffer_v2() updates one RDI line while
csid_isr() clears another), won't the writel() drop the pending AUP command
or write a stale RDI port update bit, stalling frame capture on that stream?

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