Re: [PATCH v14 5/5] media: qcom: camss: vfe: Add support for VFE gen4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional issue of `CAMSS_INIT_BUF_COUNT` AUP_UPDATEs in CSID stream configuration violates the Kaanapali hardware constraint, mismatching the actual number of enqueued VFE buffers.

Pre-existing issues:
- [High] Cross-linked VFE and CSID pipelines will write `reg_update` commands to the wrong CSID instance.
- [High] Concurrent read-modify-write of `csid->aup_update` and `csid->rup_update` across process and interrupt contexts without synchronization.
--

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

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

Add Video Front End (VFE) version gen4 as found on the Kaanapali SoC.
The FULL front end modules in Kaanapali camera subsystem are called TFEs
(Thin Front End), however, retaining the name VFE at places to maintain
consistency and avoid unnecessary code changes.

> 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..d73d70898710a
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-gen4.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]
This isn't a bug introduced by this patch, but cross-linked VFE and CSID
pipelines will write reg_update commands to the wrong CSID instance.

When this callback executes to commit buffer addresses during runtime, it
passes vfe->id as the hw_id to camss_reg_update(). In camss_reg_update(), this
hw_id is used to index into the CSID array:

    csid = &camss->csid[hw_id];
    csid->res->hw_ops->reg_update(csid, port_id, is_clear);

If the media graph cross-links a CSID source pad to a mismatched VFE sink pad
(e.g., CSID 0 to VFE 1), the register update is erroneously dispatched to
CSID 1. Could this starve the active CSID of necessary buffer updates and
trigger an unclocked register access (and panic) if CSID 1 is powered off?

[Severity: High]
This is a pre-existing issue, but there appears to be a data race when updating
the CSID register bitmasks across process and interrupt contexts.

When userspace queues a buffer during an active stream, vfe_queue_buffer_v2()
runs in process context and delegates to __csid_aup_update() via this
reg_update callback. In __csid_aup_update(), a read-modify-write operation
is performed:

    csid->aup_update |= CSID_RUP_AUP_RDI(port_id);

Concurrently, a hardware interrupt can fire on another CPU, executing
csid_isr() in hardirq context. This delegates to __csid_aup_rup_clear() which
modifies the exact same bitmask:

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

Because neither context acquires a shared lock to protect csid->aup_update,
can this data race silently corrupt the bitmask and cause hardware commands to
be dropped or repeatedly dispatched?

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 319d191589884..826ab85a44a14 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
[ ... ]
> @@ -552,7 +554,9 @@ int vfe_enable_output_v2(struct vfe_line *line)
>  		output->gen2.active_num++;
>  		ops->vfe_wm_update(vfe, output->wm_idx[0],
>  				   output->buf[i]->addr[0], line);
> -		ops->reg_update(vfe, line->id);
> +
> +		if (!vfe->res->reg_update_after_csid_config)
> +			ops->reg_update(vfe, line->id);
>  	}

[Severity: High]
The commit message documents a Kaanapali hardware constraint: "the number of
AUP_UPDATEs should match the number of buffers enqueued to the write master
while it's being enabled."

Here in vfe_enable_output_v2(), active_num is accurately populated based on
available buffers, but since reg_update_after_csid_config is true, the register
update is deferred to the CSID.

However, csid_configure_stream() unconditionally loops CAMSS_INIT_BUF_COUNT
times:

for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
    __csid_aup_update(csid, i);

If userspace queues fewer than CAMSS_INIT_BUF_COUNT buffers (since
min_queued_buffers defaults to 0), this unconditional loop will violate the
hardware constraint. Does this mismatch instruct the hardware to ingest more
frames than there are configured buffer addresses, potentially causing the
hardware to process uninitialized memory or trigger IOMMU faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-kaanapali-camss-v14-0-e76f26aa6691@oss.qualcomm.com?part=5