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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: data race on aup_update and rup_update masks between process and IRQ context
- [High] media: missing IRQ mask programming prevents INFO_RUP_DONE interrupts
- [Medium] media: camss: csid-1080: data race on aup_update and rup_update bitmasks
--

commit c1ffc650d77cc9e495d5b9cec2dfa9e85e2f485e
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.

> 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: High]
Does this code introduce a data race between process and hard IRQ context?

In process context, csid_configure_stream() updates these variables via
non-atomic bitwise OR (|=) using __csid_aup_update() and __csid_reg_update().

Concurrently, the hardware interrupt handler csid_isr() (running in hard IRQ
context) clears bits in these exact same variables via non-atomic bitwise AND
NOT (&= ~) through __csid_aup_rup_clear().

Because there is no spinlock or atomic bit operations protecting these
read-modify-write sequences, an interrupt firing on the same or another CPU
during stream configuration could cause a TOCTOU data race.

Could this lead to lost bit updates, resulting in hardware commands being
dropped or spuriously re-triggered?

[ ... ]

> +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)
> +				csid_subdev_reg_update(csid, i, true);

[Severity: High]
Is the INFO_RUP_DONE interrupt properly unmasked?

The driver relies on the INFO_RUP_DONE interrupt to clear the pending
aup_update mask in the ISR via csid_subdev_reg_update().

However, the driver never writes to CSID_CSI2_RDIN_IRQ_MASK(i) to unmask
these interrupts, nor does it enable the corresponding top-level aggregation
bit (CSI2_RX_IRQ_STATUS) in CSID_TOP_IRQ_MASK during setup or reset.

Because the interrupts appear to be permanently masked in hardware, the
RUP_DONE interrupt might never trigger the csid_isr() independently.

Could the aup_update state be left orphaned if no BUF_DONE interrupt happens
to fire, breaking asynchronous updates?

[ ... ]

> +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: High]
This appears to be where top-level masking explicitly omits
CSI2_RX_IRQ_STATUS, and CSID_CSI2_RDIN_IRQ_MASK is never programmed,
as noted above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-kaanapali-camss-v19-0-b2c583ef50e0@oss.qualcomm.com?part=7

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