Re: [PATCH v20 07/13] media: qcom: camss: csid: Add support for CSID 1080

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: camss: data race on csid aup_update and rup_update
- [Medium] media: camss: missing initialization of CSID_CSI2_RDIN_IRQ_MASK
--

commit 929222812c3090d4d0069fbedd13096bba380a7a
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.
This introduces CSID 1080 hardware resource descriptions and implements
hardware-specific register programming, reset sequence, and BUF_DONE
interrupt handling.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1080.c b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> 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);

[Severity: High]
Could these bitwise operations lead to a data race between process and
hardirq contexts?

The __csid_aup_update() and __csid_reg_update() functions are called from
process context via csid_configure_stream(). They perform non-atomic bitwise
OR operations on csid->aup_update and csid->rup_update.

Meanwhile, __csid_aup_rup_clear() performs non-atomic bitwise AND operations
on these same fields from hardirq context (called from csid_isr() when an
INFO_RUP_DONE event occurs).

If a hardirq preempts the process context during its load-modify-store cycle,
the ISR's clearing of the bits could be silently overwritten when the process
context writes back its loaded value. This could cause the tracking bits to
get stuck, potentially leading to unintended hardware updates.

Should these state updates be protected by a spinlock or use atomic bitops?

[ ... ]

> +/* RDI IRQ Status in wrapper */
> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)	(0x224 + (0x10 * (rdi)))
> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)	(0x228 + (0x10 * (rdi)))
> +#define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)	(0x22C + (0x10 * (rdi)))
> +#define		INFO_RUP_DONE				BIT(23)

[ ... ]

> +static int csid_reset(struct csid_device *csid)
> +{
> +	unsigned long time;
> +	u32 val;
> +	int i;
> +
> +	reinit_completion(&csid->reset_complete);
> +
> +	val = INFO_RST_DONE | BUF_DONE_IRQ_STATUS;
> +	writel(val, csid->base + CSID_TOP_IRQ_CLEAR);
> +	writel(val, csid->base + CSID_TOP_IRQ_MASK);
> +
> +	val = 0;
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_1080; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			/*
> +			 * Only need to clear buf done IRQ status here,
> +			 * RUP done IRQ status will be cleared once isr
> +			 * strobe generated by CSID_RST_CMD
> +			 */
> +			val |= BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i);
> +		}
> +	}
> +	writel(val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> +	writel(val, csid->base + CSID_BUF_DONE_IRQ_MASK);

[Severity: Medium]
Is the CSID_CSI2_RDIN_IRQ_MASK(i) register supposed to be initialized here to
enable the INFO_RUP_DONE interrupt?

The mask register is defined but never written to anywhere in this driver.
In csid_isr(), the driver polls CSID_CSI2_RDIN_IRQ_STATUS(i) to check for
INFO_RUP_DONE in order to clear the driver's software state tracking via
csid_subdev_reg_update().

If the hardware mask defaults to 0, the INFO_RUP_DONE status bit might never
be recorded. This would cause csid_subdev_reg_update() to be skipped,
allowing pending RUP updates to accumulate and causing redundant updates to
running streams.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-kaanapali-camss-v20-0-63e98ce9ac0c@oss.qualcomm.com?part=7