[RFC PATCH] mtd: nand: qpic_common: terminate DMA channels on transfer timeout

[RFC PATCH] mtd: nand: qpic_common: terminate DMA channels on transfer timeout

[Samarth Narang]

When DMA completion times out in qcom_submit_descs(), the driver unmaps
the descriptors' scatterlists without stopping the BAM DMA channels.
Since the issued descriptors may still be active, the RX and TX channels
can continue accessing the caller's buffer after it has been unmapped
and returned to the NAND core. The ADM path has a similar issue when
dma_sync_wait() times out.

Terminate the affected channels with dmaengine_terminate_sync() before
unmapping, ensuring that outstanding DMA transfers are stopped before
their mappings are released. On BAM, termination resets the pipe and
discards pending descriptors.

Fixes: 381dd24539a8 ("mtd: nand: qcom: add BAM DMA descriptor handling")
Fixes: c76b78d8ec05 ("mtd: nand: Qualcomm NAND controller driver")
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
Questions for the maintainers: BAM pipe resets may raise a fatal error
if the attached engine remains active, so it is unclear whether the NAND
controller must first be stopped or reset. The success path also assumes
that completion of the final command descriptor implies completion of
the RX data descriptor, which may require confirmation of the hardware
ordering guarantees.

This issue was identified by a research static analyzer for DMA
lifecycle ordering and has not been validated on hardware. The patch has
only been compile-tested on arm64 with W=1.

 drivers/mtd/nand/qpic_common.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c
index 4f3e4dd76..250154a08 100644
--- a/drivers/mtd/nand/qpic_common.c
+++ b/drivers/mtd/nand/qpic_common.c
@@ -592,11 +592,18 @@ int qcom_submit_descs(struct qcom_nand_controller *nandc)
 		dma_async_issue_pending(nandc->cmd_chan);
 
 		if (!wait_for_completion_timeout(&bam_txn->txn_done,
-						 QPIC_NAND_COMPLETION_TIMEOUT))
+						 QPIC_NAND_COMPLETION_TIMEOUT)) {
+			/* Retire every issued pipe before unmapping its buffers. */
+			dmaengine_terminate_sync(nandc->tx_chan);
+			dmaengine_terminate_sync(nandc->rx_chan);
+			dmaengine_terminate_sync(nandc->cmd_chan);
 			ret = -ETIMEDOUT;
+		}
 	} else {
-		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)
+		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE) {
+			dmaengine_terminate_sync(nandc->chan);
 			ret = -ETIMEDOUT;
+		}
 	}
 
 err_unmap_free_desc:

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC PATCH] mtd: nand: qpic_common: terminate DMA channels on transfer timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] mtd: nand: qcom_submit_descs() unmaps active ADM DMA and wedges channel
- [High] mtd: nand: qcom_submit_descs() terminates data channels before cmd_chan
- [High] mtd: nand: qcom_submit_descs() races with BAM vchan_complete() on timeout
--

commit 5c1d11395c541be6cd910adf561bf0f14c7faa0e
Author: Samarth Narang <samarth.colleges@gmail.com>

mtd: nand: qpic_common: terminate DMA channels on transfer timeout

This commit adds dmaengine_terminate_sync() calls on the BAM and ADM DMA
channels when a transfer times out in qcom_submit_descs() before unmapping
the scatterlists.

> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c
> index 4f3e4dd766daf..250154a085746 100644
> --- a/drivers/mtd/nand/qpic_common.c
> +++ b/drivers/mtd/nand/qpic_common.c
> @@ -592,11 +592,18 @@ int qcom_submit_descs(struct qcom_nand_controller *nandc)
>  		dma_async_issue_pending(nandc->cmd_chan);
>  
>  		if (!wait_for_completion_timeout(&bam_txn->txn_done,
> -						 QPIC_NAND_COMPLETION_TIMEOUT))
> +						 QPIC_NAND_COMPLETION_TIMEOUT)) {
> +			/* Retire every issued pipe before unmapping its buffers. */
> +			dmaengine_terminate_sync(nandc->tx_chan);
> +			dmaengine_terminate_sync(nandc->rx_chan);
> +			dmaengine_terminate_sync(nandc->cmd_chan);

[Severity: High]
Should nandc->cmd_chan be terminated before nandc->tx_chan and
nandc->rx_chan?

When starting a BAM transaction in qcom_submit_descs(), descriptors are
issued on nandc->tx_chan and nandc->rx_chan first and nandc->cmd_chan last,
because cmd_chan writes register commands (such as NAND_FLASH_CMD and
NAND_EXEC_CMD) that trigger the QPIC controller to transfer codeword data
over tx_chan and rx_chan.

In bam_dma_terminate_all(), resetting a pipe with pending descriptors calls
bam_chan_init_hw(), which requires the caller to stop activity on the
attached hardware engine first:

drivers/dma/qcom/bam_dma.c:bam_dma_terminate_all() {
	...
	if (!list_empty(&bchan->desc_list)) {
		async_desc = list_first_entry(&bchan->desc_list,
					      struct bam_async_desc, desc_node);
		bam_chan_init_hw(bchan, async_desc->dir);
	}
	...
}

If nandc->tx_chan and nandc->rx_chan are reset while nandc->cmd_chan is
still active during a multi-codeword transfer, can cmd_chan advance to a
subsequent codeword's command descriptor and trigger QPIC data accesses on
the disabled or empty data pipes, causing a fatal BAM error?

[Severity: High]
Also, does dmaengine_terminate_sync(nandc->cmd_chan) synchronize against a
concurrent vchan_complete() tasklet on timeout?

In drivers/dma/qcom/bam_dma.c, bam_dma_probe() does not set
bdev->common.device_synchronize:

drivers/dma/qcom/bam_dma.c:bam_dma_probe() {
	...
	bdev->common.device_terminate_all = bam_dma_terminate_all;
	bdev->common.device_issue_pending = bam_issue_pending;
	bdev->common.device_tx_status = bam_tx_status;
	...
}

Because device_synchronize is NULL, dmaengine_synchronize() is a no-op. If
bam_dma_irq() -> process_channel_irqs() -> vchan_cookie_complete()
completes bam_txn->last_cmd_desc right as wait_for_completion_timeout()
expires, the vchan_complete() tasklet can splice vc->desc_completed into
its local list and drop vc->lock before bam_dma_terminate_all() acquires
vc->lock:

drivers/dma/virt-dma.c:vchan_complete() {
	...
	spin_lock_irq(&vc->lock);
	list_splice_tail_init(&vc->desc_completed, &head);
	...
	spin_unlock_irq(&vc->lock);
	...
}

If qcom_submit_descs() returns while vchan_complete() is about to invoke
qcom_qpic_bam_dma_done(bam_txn), can that late callback either prematurely
complete bam_txn->txn_done for a subsequent transaction (after
qcom_clear_bam_transaction() reinitializes it) or trigger a use-after-free
if qcom_free_bam_transaction() frees bam_txn?

>  			ret = -ETIMEDOUT;
> +		}
>  	} else {
> -		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)
> +		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE) {
> +			dmaengine_terminate_sync(nandc->chan);

[Severity: Critical]
On the ADM path, does dmaengine_terminate_sync(nandc->chan) actually wait
for the active transfer to stop before err_unmap_free_desc unmaps
desc->adm_sgl, or can it leave the channel permanently wedged?

In drivers/dma/qcom/qcom_adm.c, adm_start_dma() removes the active
descriptor from achan->vc.desc_issued and stores it in achan->curr_txd.
adm_dma_probe() does not populate device_synchronize, and
adm_terminate_all() only collects descriptors remaining on the vchan lists
and writes ADM_CH_FLUSH_STATE0 asynchronously without waiting for the flush
interrupt or clearing achan->curr_txd:

drivers/dma/qcom/qcom_adm.c:adm_terminate_all() {
	...
	spin_lock_irqsave(&achan->vc.lock, flags);
	vchan_get_all_descriptors(&achan->vc, &head);

	/* send flush command to terminate current transaction */
	writel_relaxed(0x0,
		       adev->regs + ADM_CH_FLUSH_STATE0(achan->id, adev->ee));

	spin_unlock_irqrestore(&achan->vc.lock, flags);
	...
}

Because adm_terminate_all() returns immediately while the ADM hardware is
still flushing the active transfer in achan->curr_txd, won't jumping to
err_unmap_free_desc call dma_unmap_sg() on desc->adm_sgl while DMA is still
active?

Furthermore, since achan->curr_txd is only cleared in adm_dma_irq(), if the
timeout occurred because the peripheral CRCI flow-control handshake is
stuck and the flush interrupt never fires, won't achan->curr_txd (along
with async_desc->cpl and its DMA mapping) be leaked and remain non-NULL,
preventing adm_issue_pending() from starting any future transfers on
nandc->chan:

drivers/dma/qcom/qcom_adm.c:adm_issue_pending() {
	...
	if (vchan_issue_pending(&achan->vc) && !achan->curr_txd)
		adm_start_dma(achan);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030150646.2942368-1-samarth.colleges@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/