Re: [PATCH v21 08/14] 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: qcom: camss: data race on aup_update and rup_update bitmasks
- [Medium] media: qcom: camss: missing RDIN IRQ mask initialization for CSID 1080
--

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

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

This commit adds support for the CSID 1080 hardware found on the
Kaanapali SoC. It introduces hardware resource descriptions, reset
sequences, and interrupt handling. It also updates the CSID interface
to handle separate RUP and AUP registers.

> 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);

[Severity: High]
Can these non-atomic bitwise operations race with the interrupt handler?

__csid_aup_rup_clear() runs in hardirq context via csid_isr(), modifying
both csid->aup_update and csid->rup_update.

Concurrently, __csid_aup_update() and __csid_reg_update() perform non-atomic
bitwise OR operations on these same shared bitmasks from process context
during csid_configure_stream().

Without synchronization such as spin_lock_irqsave() or using atomic bitops,
could process context overwrite the interrupt handler's modifications,
resurrecting cleared bits or dropping newly set bits? This might lead to
missed or redundant hardware updates.

> +	writel(csid->rup_update, csid->base + CSID_RUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}

[ ... ]

> +static irqreturn_t csid_isr(int irq, void *dev)
> +{
> +	struct csid_device *csid = dev;
> +	u32 val, buf_done_val;
> +	u8 reset_done;
> +	int i;
> +
> +	val = readl(csid->base + CSID_TOP_IRQ_STATUS);
> +	writel(val, csid->base + CSID_TOP_IRQ_CLEAR);
> +
> +	reset_done = val & INFO_RST_DONE;
> +
> +	buf_done_val = readl(csid->base + CSID_BUF_DONE_IRQ_STATUS);
> +	writel(buf_done_val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_1080; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			val = readl(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
> +			writel(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
> +
> +			if (val & INFO_RUP_DONE)

[Severity: Medium]
Will this INFO_RUP_DONE status actually latch in hardware?

The CSID_CSI2_RDIN_IRQ_MASK macro is defined, but it doesn't seem to ever
be written to hardware to enable this interrupt during csid_reset() or
csid_configure_stream().

If the mask remains zero, the status bit might not be recorded. As a result,
csid_isr() would never observe INFO_RUP_DONE, failing to clear
csid->aup_update and leaking stale bitmasks across frames.

> +				csid_subdev_reg_update(csid, i, true);
> +
> +			if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i))
> +				camss_buf_done(csid->camss, csid->id, i);
> +		}
> +	}
> +
> +	val = IRQ_CMD_CLEAR;
> +	writel(val, csid->base + CSID_IRQ_CMD);
> +
> +	if (reset_done)
> +		complete(&csid->reset_complete);
> +
> +	return IRQ_HANDLED;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-kaanapali-camss-v21-0-04fd28d815c6@oss.qualcomm.com?part=8