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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Data Race / Unprotected State Modification
--

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

media: qcom: camss: csid: Add support for CSID 1080

Add support for the CSID 1080 hardware found on Kaanapali SoC.
Introduce CSID 1080 hardware resource descriptions and implement the
hardware-specific register programming, reset sequence, and BUF_DONE
interrupt handling. CSID 1080 splits RUP and AUP updates into separate
registers and uses a SET register to commit the updates.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1080.c b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> new file mode 100644
> index 0000000000000..46e443e05195a
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1080.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: Low]
Can these un-synchronized read-modify-write operations race between hardirq
and process contexts?

When starting multiple streams in csid_configure_stream(), RDI 0 could
complete a frame and trigger the BUF_DONE interrupt while RDI 1 is still being
configured. This would cause csid_isr() to call __csid_aup_rup_clear()
concurrently with __csid_aup_update() or __csid_reg_update() executing in
process context.

Since these updates to csid->aup_update and csid->rup_update are unprotected,
a concurrent modification might cause bit updates to be lost. While this might
only lead to a harmless redundant trigger or prevent a redundant write, it is
undefined behavior in C and can trigger KCSAN warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-kaanapali-camss-v18-0-87ba4ebe0db1@oss.qualcomm.com?part=6