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

[Hangxiang Ma]

On 9/29/2026 2:17 PM, sashiko-bot at 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 at 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.
>
Additional reply for this. As this relates to CAMSS arch so that a
separate series will take care this. Kaanapali series will continue
focusing on the platform enabling. Thanks.

Best Regards,
Hangxiang


On Tue, Sep 29, 2026 at 2:17 PM <sashiko-bot at 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 at 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