Skip to contentSkip to conversation

FAILED: patch "[PATCH] dmaengine: at_hdmac: Fix concurrency problems by removing" failed to apply to 4.19-stable tree

PATCH [email protected] 1 participant 1 message Nov 14, 2022

Subsystems (3)
Stable branch dmaengine Microchip AT91 DMA drivers
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Possible dependencies:

c6babed879fb ("dmaengine: at_hdmac: Fix concurrency problems by removing atc_complete_all()") 078a6506141a ("dmaengine: at_hdmac: Fix deadlocks")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From c6babed879fbe82796a601bf097649e07382db46 Mon Sep 17 00:00:00 2001
From: Tudor Ambarus <[email protected]>
Date: Tue, 25 Oct 2022 12:02:41 +0300
Subject: [PATCH] dmaengine: at_hdmac: Fix concurrency problems by removing
atc_complete_all()

atc_complete_all() had concurrency bugs, thus remove it:
1/ atc_complete_all() in its entirety was buggy, as when the atchan->queue list (the one that contains descriptors that are not yet issued to the hardware) contained descriptors, it fired just the first from the atchan->queue, but moved all the desc from atchan->queue to atchan->active_list and considered them all as fired. This could result in calling the completion of a descriptor that was not yet issued to the hardware.
2/ when in tasklet at atc_advance_work() time, atchan->active_list was queried without holding the lock of the chan. This can result in atchan->active_list concurrency problems between the tasklet and issue_pending().

Fixes dc78baa2b90b ("dmaengine: at_hdmac: new driver for the Atmel AHB DMA Controller")
Reported-by Peter Rosin <[email protected]>
Signed-off-by Tudor Ambarus <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/lkml/[email protected]/ open here
Acked-by Nicolas Ferre <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Vinod Koul <[email protected]>
drivers/dma/at_hdmac.c +4 -45
index deb4c6027436..f1e6fa6af6c2 100644
@@ -485,42 +485,6 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
 	dma_run_dependencies(txd);
 }
 
-/**
- * atc_complete_all - finish work for all transactions
- * @atchan: channel to complete transactions for
- *
- * Eventually submit queued descriptors if any
- *
- * Assume channel is idle while calling this function
- * Called with atchan->lock held and bh disabled
- */
-static void atc_complete_all(struct at_dma_chan *atchan)
-{
-	struct at_desc *desc, *_desc;
-	LIST_HEAD(list);
-	unsigned long flags;
-
-	dev_vdbg(chan2dev(&atchan->chan_common), "complete all\n");
-
-	spin_lock_irqsave(&atchan->lock, flags);
-
-	/*
-	 * Submit queued descriptors ASAP, i.e. before we go through
-	 * the completed ones.
-	 */
-	if (!list_empty(&atchan->queue))
-		atc_dostart(atchan, atc_first_queued(atchan));
-	/* empty active_list now it is completed */
-	list_splice_init(&atchan->active_list, &list);
-	/* empty queue list by moving descriptors (if any) to active_list */
-	list_splice_init(&atchan->queue, &atchan->active_list);
-
-	spin_unlock_irqrestore(&atchan->lock, flags);
-
-	list_for_each_entry_safe(desc, _desc, &list, desc_node)
-		atc_chain_complete(atchan, desc);
-}
-
 /**
  * atc_advance_work - at the end of a transaction, move forward
  * @atchan: channel where the transaction ended

@@ -528,25 +492,20 @@ static void atc_complete_all(struct at_dma_chan *atchan)
 static void atc_advance_work(struct at_dma_chan *atchan)
 {
 	unsigned long flags;
-	int ret;
 
 	dev_vdbg(chan2dev(&atchan->chan_common), "advance_work\n");
 
 	spin_lock_irqsave(&atchan->lock, flags);
-	ret = atc_chan_is_enabled(atchan);
+	if (atc_chan_is_enabled(atchan) || list_empty(&atchan->active_list))
+		return spin_unlock_irqrestore(&atchan->lock, flags);
 	spin_unlock_irqrestore(&atchan->lock, flags);
-	if (ret)
-		return;
-
-	if (list_empty(&atchan->active_list) ||
-	    list_is_singular(&atchan->active_list))
-		return atc_complete_all(atchan);
 
 	atc_chain_complete(atchan, atc_first_active(atchan));
 
 	/* advance work */
 	spin_lock_irqsave(&atchan->lock, flags);
-	atc_dostart(atchan, atc_first_active(atchan));
+	if (!list_empty(&atchan->active_list))
+		atc_dostart(atchan, atc_first_active(atchan));
 	spin_unlock_irqrestore(&atchan->lock, flags);
 }