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

[Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>]

On 9/29/2026 2:17 PM, sashiko-bot@kernel.org wrote:
> 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.
> 
Ack

>> +
>> +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?
> 

Ack

Best Regards,
Hangxiang

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