Re: [PATCH v13 14/17] media: ti: j721e-csi2rx: Change the drain architecture for multistream

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Data race and waitqueue corruption due to `init_completion()` being called outside the lock after the `STOPPED` state is set in `ti_csi2rx_stop_dma()`.
- [High] `ti_csi2rx_dma_callback` unconditionally overwrites the `TI_CSI2RX_DMA_STOPPED` state, leading to endless draining and a 50ms timeout during streamoff.
- [High] Removal of the DMA restart logic causes a permanent stream stall if any DMA transaction fails to prepare or submit.
- [High] Pre-existing drain transactions can trigger a premature completion of the barrier wait in `ti_csi2rx_stop_dma`.
- [Medium] `ti_csi2rx_stop_dma` unconditionally blocks on `wait_for_completion_timeout`, causing a guaranteed 50ms delay on already-stopped streams or error paths.
- [Medium] The continuously draining architecture generates an excessively high interrupt rate during buffer starvation.
--

commit 3364f8fb9264e28b1fb58dc86804662c48056485
Author: Rishikesh Donadkar <r-donadkar@ti.com>

media: ti: j721e-csi2rx: Change the drain architecture for multistream

This patch introduces a continuous drain architecture for multistream
CSI2RX scenarios to prevent buffer starvation on one stream from blocking
the shared HW FIFO. It uses a 32KiB drain buffer and waits for a
completion barrier at the end of the stream.

> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
[ ... ]
> @@ -608,9 +613,32 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
[ ... ]
> +	if (list_empty(&dma->queue)) {
> +		if (ti_csi2rx_drain_dma(ctx))
> +			dev_warn(ctx->csi->dev, "DMA drain failed\n");
> +	} else {
> +		ti_csi2rx_dma_submit_pending(ctx);
> +	}
> +	spin_unlock_irqrestore(&dma->lock, flags);
>  }

[Severity: Medium]
For high-bandwidth CSI-2 streams, a 32KiB buffer fills very quickly.
During user-space buffer starvation, this architecture continually loops
32KiB drain transactions.

Does this generate an excessively high interrupt rate (tens of thousands
per second) that could degrade overall system performance?

[ ... ]
> @@ -702,14 +720,24 @@ static void ti_csi2rx_dma_callback(void *param)
[ ... ]
> -	if (list_empty(&dma->submitted))
> -		dma->state = TI_CSI2RX_DMA_IDLE;
> -
> +	if (list_empty(&dma->submitted)) {
> +		dma->state = TI_CSI2RX_DMA_DRAINING;
> +		if (ti_csi2rx_drain_dma(ctx))
> +			dev_warn(ctx->csi->dev,
> +				 "DMA drain failed on one of the transactions\n");
> +	}
>  	spin_unlock_irqrestore(&dma->lock, flags);
>  }

[Severity: High]
If ti_csi2rx_stop_dma() concurrently sets the state to
TI_CSI2RX_DMA_STOPPED, does this blindly overwrite it?

If the state gets overwritten to TI_CSI2RX_DMA_DRAINING,
ti_csi2rx_drain_callback() might observe DRAINING instead of STOPPED,
triggering recursive drains and causing the streamoff operation to hit
its 50ms timeout.

[ ... ]
> @@ -754,6 +782,8 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>  	dma->state = TI_CSI2RX_DMA_STOPPED;
>  	spin_unlock_irqrestore(&dma->lock, flags);
>  
> +	init_completion(&ctx->drain_complete);

[Severity: High]
If a concurrent drain DMA completes precisely after the lock is released
but before init_completion() is called, its callback will observe the
STOPPED state and call complete(&ctx->drain_complete).

Can this concurrent execution of complete() and init_completion() cause a
data race on the completion's waitqueue spinlock or lost completion events?
Should init_completion() be called before modifying the state?

[Severity: High]
Additionally, if the state was already TI_CSI2RX_DMA_DRAINING when
ti_csi2rx_stop_dma() is called, there may be a pre-existing drain
transaction currently running.

When this pre-existing drain finishes, its callback observes the new
STOPPED state and immediately calls complete(&ctx->drain_complete).

Does this wake up the wait_for_completion_timeout() below prematurely,
causing the driver to abort via dmaengine_terminate_sync() before the
actual barrier drain has finished clearing the hardware FIFO?

[ ... ]
> @@ -762,11 +792,19 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
[ ... ]
>  		ret = ti_csi2rx_drain_dma(ctx);
> -		if (ret && ret != -ETIMEDOUT)
> +		if (ret)
>  			dev_warn(ctx->csi->dev,
>  				 "Failed to drain DMA. Next frame might be bogus\n");
>  	}
>  
> +	/* We wait for the drain to complete so that the stream stops
> +	 * cleanly, making sure the shared hardware FIFO is cleared of
> +	 * data from the current stream. No more data will be coming from
> +	 * the source after this.
> +	 */
> +	wait_for_completion_timeout(&ctx->drain_complete,
> +				    msecs_to_jiffies(DRAIN_TIMEOUT_MS));

[Severity: Medium]
If the stream was already stopped (e.g., during error handling) or if
ti_csi2rx_drain_dma() failed to prepare a descriptor, no DMA transaction
is queued.

Since no callback will ever signal ctx->drain_complete in these paths, does
this cause the function to unnecessarily block for the full 50ms timeout
every time?

[ ... ]
> @@ -829,57 +867,14 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
[ ... ]
> -	/*
> -	 * Usually the DMA callback takes care of queueing the pending buffers.
> -	 * But if DMA has stalled due to lack of buffers, restart it now.
> -	 */
> -	if (dma->state == TI_CSI2RX_DMA_IDLE) {
[ ... ]
> -	} else {
> -		list_add_tail(&buf->list, &dma->queue);
> -	}
> +	list_add_tail(&buf->list, &dma->queue);
>  	spin_unlock_irqrestore(&dma->lock, flags);

[Severity: High]
The previous architecture actively restarted the DMA engine if it was idle.
By removing this restart logic, the driver now relies entirely on an
unbroken chain of DMA callbacks.

If ti_csi2rx_drain_dma() fails to prepare or submit a descriptor (e.g., due
to memory pressure), the active DMA chain breaks and the DMA engine halts.

Without a way to kickstart the DMA here, do newly queued buffers just
accumulate in dma->queue, resulting in a silent, permanent stream deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520120022.539913-1-r-donadkar@ti.com?part=14