Re: [PATCH v17 06/11] media: qcom: camss: csid: Add support for CSID Gen4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: qcom/camss: data race on csid->aup_update and rup_update
- [Medium] media: qcom/camss: integer underflow in __csid_configure_rx
--

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

media: qcom: camss: csid: Add support for CSID Gen4
    
Add support for the CSID Gen4 hardware found on Kaanapali SoC.
    
Introduce Gen4 resource descriptions and implement the hardware-specific
register programming, reset sequence, and BUF_DONE interrupt handling.
    
Gen4 splits RUP and AUP updates into separate registers and uses a SET
register to commit the updates. Update the CSID interface to support
both this scheme and the legacy combined reg_update mechanism.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen4.c b/drivers/media/platform/qcom/camss/camss-csid-gen4.c
> new file mode 100644
> index 0000000000000..4ff2f41f70f7e
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen4.c
[ ... ]
> +static void __csid_aup_rup_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +	csid->rup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
> +
> +static void __csid_aup_update(struct csid_device *csid, int port_id)
> +{
> +	csid->aup_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->aup_update, csid->base + CSID_AUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_reg_update(struct csid_device *csid, int port_id)
> +{
> +	csid->rup_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->rup_update, csid->base + CSID_RUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}

[Severity: High]
Does this introduce a data race on csid->aup_update and csid->rup_update?

If a stream is configured in process context via csid_set_stream(),
__csid_aup_update() and __csid_reg_update() modify the shared state with
non-atomic bitwise operations (|=).

If an interrupt (such as RUP_DONE) fires concurrently for an already running
stream, csid_isr() will execute and call __csid_aup_rup_clear(), which modifies
the same variables using non-atomic bitwise clear operations (&= ~).

Because these bitwise operations lack locking, could the interrupt's
modifications be lost, causing the hardware to receive an invalid AUP/RUP
command mask? This might lead to broken streaming when dynamically configuring
multiple virtual channels.

> +
> +static void __csid_configure_rx(struct csid_device *csid,
> +				struct csid_phy_config *phy)
> +{
> +	int val;
> +
> +	val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
> +	val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
> +	val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX)
> +	       << CSI2_RX_CFG0_PHY_NUM_SEL;
> +	writel(val, csid->base + CSID_CSI2_RX_CFG0);

[Severity: Medium]
Can this code underflow if phy->lane_cnt is 0?

If the device tree incorrectly omits the data-lanes property, phy->lane_cnt can
evaluate to 0.

While __csid_configure_rdi_stream() provides a fallback for this scenario:

    if (!lane_cnt)
        lane_cnt = 4;

__csid_configure_rx() omits this check. If phy->lane_cnt is 0,
(phy->lane_cnt - 1) underflows to -1. This would result in writing 0xFFFFFFFF
to CSID_CSI2_RX_CFG0, clobbering all its fields.

Should there be a similar fallback or validation check here to prevent
hardware misconfiguration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-kaanapali-camss-v17-0-dcf3fd37f76c@oss.qualcomm.com?part=6

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy