at_hdmac does not release lock before the callback - deadlock

at_hdmac does not release lock before the callback - deadlock

[Jouko Haapaluoma]

Hello

It seems that the at_hdmac driver keeps the atchan->lock even when the device driver callback is called.
This will cause a deadlock in those cases when the device driver calls certain DMA Engine methods
from the callback function.

The DMA Engine documentation (https://www.kernel.org/doc/Documentation/dmaengine.txt) states:
" 
Although the async_tx API specifies that completion callback
routines cannot submit any new operations, this is not the
case for slave/cyclic DMA.

For slave DMA, the subsequent transaction may not be available
for submission prior to callback function being invoked, so
slave DMA callbacks are permitted to prepare and submit a new
transaction.

For cyclic DMA, a callback function may wish to terminate the
DMA via dmaengine_terminate_all().

Therefore, it is important that DMA engine drivers drop any
locks before calling the callback function which may cause a
deadlock.
"

According to the documentation it seems that the at_hdmac driver violates the DMA Engine API because
locking is not released before entering the callback function.

One possible deadlock situation:

atc_tasklet()
spin_lock_irqsave(&atchan->lock)
atc_handle_cyclic()
callback()
dmaengine_terminate_all()
atc_control()
spin_lock_irqsave(&atchan->lock) 


BR,
Jouko Haapaluoma

at_hdmac does not release lock before the callback - deadlock

[Jouko Haapaluoma]

After further investigation it seems that the deadlocks  would happen only when CONFIG_SMP is enabled or RT_PREEMPT patch is used. When CONFIG_SMP or RT_PREEMPT is not used, the spinlocks will only disable preemption which does not cause a deadlock. However, with the CONFIG_SMP or the RT_PREEMPT patch spinlocks are actual locks and deadlocks can occur.

This might explain why this problem hasn't been noticed before. At least the Atmel serial driver from linux4sam git causes deadlocks with the RT_PREEMPT because of this issue. 

BR,
Jouko Haapaluoma

[RFC PATCH 0/2] dmaengine: at_hdmac: fix locking according to slave DMA requirements

[Nicolas Ferre]

Hi,

This is an attempt to solve the locking issue that we currently have with the
Atmel at_hdmac dmaengine driver.
I tested these patches with several drivers but may need more coverage before
making them permanent...

Comments welcome.

Nicolas Ferre (2):
  dmaengine: at_hdmac: remove the call to issue_pending from tx_status
  dmaengine: at_hdmac: run callback function with no lock held nor
    interrupts disabled

 drivers/dma/at_hdmac.c | 128 +++++++++++++++++++++++++++----------------------
 1 file changed, 71 insertions(+), 57 deletions(-)

-- 
1.8.2.2

[PATCH 1/2] dmaengine: at_hdmac: remove the call to issue_pending from tx_status

[Nicolas Ferre]

Triggering the dmaengine from tx_status call seems awkward.
If data are remaining in the DMA FIFO, the normal operations
should drain them anyway.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/dma/at_hdmac.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index e2c04dc81e2a..b28759b6d1ca 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -268,8 +268,6 @@ static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan,
 static int atc_get_bytes_left(struct dma_chan *chan)
 {
 	struct at_dma_chan      *atchan = to_at_dma_chan(chan);
-	struct at_dma           *atdma = to_at_dma(chan->device);
-	int	chan_id = atchan->chan_common.chan_id;
 	struct at_desc *desc_first = atc_first_active(atchan);
 	struct at_desc *desc_cur;
 	int ret = 0, count = 0;
@@ -311,11 +309,6 @@ static int atc_get_bytes_left(struct dma_chan *chan)
 				<< (desc_first->tx_width);
 		ret = atchan->remain_desc - count;
 	}
-	/*
-	 * Check fifo empty.
-	 */
-	if (!(dma_readl(atdma, CHSR) & AT_DMA_EMPT(chan_id)))
-		atc_issue_pending(chan);
 
 out:
 	return ret;
-- 
1.8.2.2

[PATCH 2/2] dmaengine: at_hdmac: run callback function with no lock held nor interrupts disabled

[Nicolas Ferre]

Now, submission from callbacks are permitted as per dmaengine framework. So we
shouldn't hold any spinlock nor disable IRQs while calling callbacks.
As locks were taken by parent routines, spin_lock_irqsave() has to be called
inside all routines, wherever they are required.

The little used atc_issue_pending() function is made void.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/dma/at_hdmac.c | 121 +++++++++++++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 50 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index b28759b6d1ca..f7bf4065636c 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -268,10 +268,14 @@ static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan,
 static int atc_get_bytes_left(struct dma_chan *chan)
 {
 	struct at_dma_chan      *atchan = to_at_dma_chan(chan);
-	struct at_desc *desc_first = atc_first_active(atchan);
+	struct at_desc *desc_first;
 	struct at_desc *desc_cur;
+	unsigned long flags;
 	int ret = 0, count = 0;
 
+	spin_lock_irqsave(&atchan->lock, flags);
+	desc_first = atc_first_active(atchan);
+
 	/*
 	 * Initialize necessary values in the first time.
 	 * remain_desc record remain desc length.
@@ -311,6 +315,7 @@ static int atc_get_bytes_left(struct dma_chan *chan)
 	}
 
 out:
+	spin_unlock_irqrestore(&atchan->lock, flags);
 	return ret;
 }
 
@@ -318,12 +323,14 @@ out:
  * atc_chain_complete - finish work for one transaction chain
  * @atchan: channel we work on
  * @desc: descriptor at the head of the chain we want do complete
- *
- * Called with atchan->lock held and bh disabled */
+ */
 static void
 atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
 {
 	struct dma_async_tx_descriptor	*txd = &desc->txd;
+	unsigned long			flags;
+
+	spin_lock_irqsave(&atchan->lock, flags);
 
 	dev_vdbg(chan2dev(&atchan->chan_common),
 		"descriptor %u complete\n", txd->cookie);
@@ -337,6 +344,8 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
 	/* move myself to free_list */
 	list_move(&desc->desc_node, &atchan->free_list);
 
+	spin_unlock_irqrestore(&atchan->lock, flags);
+
 	dma_descriptor_unmap(txd);
 	/* for cyclic transfers,
 	 * no need to replay callback function while stopping */
@@ -344,10 +353,6 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
 		dma_async_tx_callback	callback = txd->callback;
 		void			*param = txd->callback_param;
 
-		/*
-		 * The API requires that no submissions are done from a
-		 * callback, so we don't need to drop the lock here
-		 */
 		if (callback)
 			callback(param);
 	}
@@ -362,15 +367,17 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
  * 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.
@@ -382,6 +389,8 @@ static void atc_complete_all(struct at_dma_chan *atchan)
 	/* 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);
 }
@@ -389,23 +398,35 @@ static void atc_complete_all(struct at_dma_chan *atchan)
 /**
  * atc_advance_work - at the end of a transaction, move forward
  * @atchan: channel where the transaction ended
- *
- * Called with atchan->lock held and bh disabled
  */
 static void atc_advance_work(struct at_dma_chan *atchan)
 {
+	unsigned long	flags;
+
 	dev_vdbg(chan2dev(&atchan->chan_common), "advance_work\n");
 
-	if (atc_chan_is_enabled(atchan))
+	spin_lock_irqsave(&atchan->lock, flags);
+
+	if (atc_chan_is_enabled(atchan)) {
+		spin_unlock_irqrestore(&atchan->lock, flags);
 		return;
+	}
 
 	if (list_empty(&atchan->active_list) ||
 	    list_is_singular(&atchan->active_list)) {
+		spin_unlock_irqrestore(&atchan->lock, flags);
 		atc_complete_all(atchan);
 	} else {
-		atc_chain_complete(atchan, atc_first_active(atchan));
+		struct at_desc *desc_first = atc_first_active(atchan);
+
+		spin_unlock_irqrestore(&atchan->lock, flags);
+		atc_chain_complete(atchan, desc_first);
+		barrier();
 		/* advance work */
-		atc_dostart(atchan, atc_first_active(atchan));
+		spin_lock_irqsave(&atchan->lock, flags);
+		desc_first = atc_first_active(atchan);
+		atc_dostart(atchan, desc_first);
+		spin_unlock_irqrestore(&atchan->lock, flags);
 	}
 }
 
@@ -413,13 +434,14 @@ static void atc_advance_work(struct at_dma_chan *atchan)
 /**
  * atc_handle_error - handle errors reported by DMA controller
  * @atchan: channel where error occurs
- *
- * Called with atchan->lock held and bh disabled
  */
 static void atc_handle_error(struct at_dma_chan *atchan)
 {
 	struct at_desc *bad_desc;
 	struct at_desc *child;
+	unsigned long flags;
+
+	spin_lock_irqsave(&atchan->lock, flags);
 
 	/*
 	 * The descriptor currently at the head of the active list is
@@ -452,6 +474,8 @@ static void atc_handle_error(struct at_dma_chan *atchan)
 	list_for_each_entry(child, &bad_desc->tx_list, desc_node)
 		atc_dump_lli(atchan, &child->lli);
 
+	spin_unlock_irqrestore(&atchan->lock, flags);
+
 	/* Pretend the descriptor completed successfully */
 	atc_chain_complete(atchan, bad_desc);
 }
@@ -459,19 +483,27 @@ static void atc_handle_error(struct at_dma_chan *atchan)
 /**
  * atc_handle_cyclic - at the end of a period, run callback function
  * @atchan: channel used for cyclic operations
- *
- * Called with atchan->lock held and bh disabled
  */
 static void atc_handle_cyclic(struct at_dma_chan *atchan)
 {
-	struct at_desc			*first = atc_first_active(atchan);
-	struct dma_async_tx_descriptor	*txd = &first->txd;
-	dma_async_tx_callback		callback = txd->callback;
-	void				*param = txd->callback_param;
+	struct at_desc			*first;
+	struct dma_async_tx_descriptor	*txd;
+	dma_async_tx_callback		callback;
+	void				*param;
+	u32				dscr;
+	unsigned long			flags;
+
+	spin_lock_irqsave(&atchan->lock, flags);
+	first = atc_first_active(atchan);
+	dscr = channel_readl(atchan, DSCR);
+	spin_unlock_irqrestore(&atchan->lock, flags);
+
+	txd = &first->txd;
+	callback = txd->callback;
+	param = txd->callback_param;
 
 	dev_vdbg(chan2dev(&atchan->chan_common),
-			"new cyclic period llp 0x%08x\n",
-			channel_readl(atchan, DSCR));
+			"new cyclic period llp 0x%08x\n", dscr);
 
 	if (callback)
 		callback(param);
@@ -482,17 +514,13 @@ static void atc_handle_cyclic(struct at_dma_chan *atchan)
 static void atc_tasklet(unsigned long data)
 {
 	struct at_dma_chan *atchan = (struct at_dma_chan *)data;
-	unsigned long flags;
 
-	spin_lock_irqsave(&atchan->lock, flags);
 	if (test_and_clear_bit(ATC_IS_ERROR, &atchan->status))
 		atc_handle_error(atchan);
 	else if (atc_chan_is_cyclic(atchan))
 		atc_handle_cyclic(atchan);
 	else
 		atc_advance_work(atchan);
-
-	spin_unlock_irqrestore(&atchan->lock, flags);
 }
 
 static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
@@ -1013,6 +1041,11 @@ static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		spin_unlock_irqrestore(&atchan->lock, flags);
 	} else if (cmd == DMA_TERMINATE_ALL) {
 		struct at_desc	*desc, *_desc;
+
+		/* Disable interrupts */
+		atc_disable_chan_irq(atdma, chan->chan_id);
+		tasklet_disable(&atchan->tasklet);
+
 		/*
 		 * This is only called when something went wrong elsewhere, so
 		 * we don't really care about the data. Just disable the
@@ -1033,14 +1066,22 @@ static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		list_splice_init(&atchan->active_list, &list);
 
 		/* Flush all pending and queued descriptors */
-		list_for_each_entry_safe(desc, _desc, &list, desc_node)
+		list_for_each_entry_safe(desc, _desc, &list, desc_node) {
+			spin_unlock_irqrestore(&atchan->lock, flags);
 			atc_chain_complete(atchan, desc);
+			spin_lock_irqsave(&atchan->lock, flags);
+		}
 
 		clear_bit(ATC_IS_PAUSED, &atchan->status);
 		/* if channel dedicated to cyclic operations, free it */
 		clear_bit(ATC_IS_CYCLIC, &atchan->status);
 
 		spin_unlock_irqrestore(&atchan->lock, flags);
+
+		/* Re-enable channel for future operations */
+		tasklet_enable(&atchan->tasklet);
+		atc_enable_chan_irq(atdma, chan->chan_id);
+
 	} else if (cmd == DMA_SLAVE_CONFIG) {
 		return set_runtime_config(chan, (struct dma_slave_config *)arg);
 	} else {
@@ -1065,8 +1106,6 @@ atc_tx_status(struct dma_chan *chan,
 		dma_cookie_t cookie,
 		struct dma_tx_state *txstate)
 {
-	struct at_dma_chan	*atchan = to_at_dma_chan(chan);
-	unsigned long		flags;
 	enum dma_status		ret;
 	int bytes = 0;
 
@@ -1080,13 +1119,9 @@ atc_tx_status(struct dma_chan *chan,
 	if (!txstate)
 		return DMA_ERROR;
 
-	spin_lock_irqsave(&atchan->lock, flags);
-
 	/*  Get number of bytes left in the active transactions */
 	bytes = atc_get_bytes_left(chan);
 
-	spin_unlock_irqrestore(&atchan->lock, flags);
-
 	if (unlikely(bytes < 0)) {
 		dev_vdbg(chan2dev(chan), "get residual bytes error\n");
 		return DMA_ERROR;
@@ -1101,24 +1136,10 @@ atc_tx_status(struct dma_chan *chan,
 }
 
 /**
- * atc_issue_pending - try to finish work
+ * atc_issue_pending - void function
  * @chan: target DMA channel
  */
-static void atc_issue_pending(struct dma_chan *chan)
-{
-	struct at_dma_chan	*atchan = to_at_dma_chan(chan);
-	unsigned long		flags;
-
-	dev_vdbg(chan2dev(chan), "issue_pending\n");
-
-	/* Not needed for cyclic transfers */
-	if (atc_chan_is_cyclic(atchan))
-		return;
-
-	spin_lock_irqsave(&atchan->lock, flags);
-	atc_advance_work(atchan);
-	spin_unlock_irqrestore(&atchan->lock, flags);
-}
+static void atc_issue_pending(struct dma_chan *chan) {}
 
 /**
  * atc_alloc_chan_resources - allocate resources for DMA channel
-- 
1.8.2.2

[PATCH 2/2] dmaengine: at_hdmac: run callback function with no lock held nor interrupts disabled

[Vinod Koul]

On Mon, Jan 27, 2014 at 03:23:24PM +0100, Nicolas Ferre wrote:
> Now, submission from callbacks are permitted as per dmaengine framework. So we
> shouldn't hold any spinlock nor disable IRQs while calling callbacks.
> As locks were taken by parent routines, spin_lock_irqsave() has to be called
> inside all routines, wherever they are required.
> 
> The little used atc_issue_pending() function is made void.
and why would that be? The client _should_ invoke issues pending to push the
desciptors..?

-- 
~Vinod
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/dma/at_hdmac.c | 121 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 71 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index b28759b6d1ca..f7bf4065636c 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -268,10 +268,14 @@ static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan,
>  static int atc_get_bytes_left(struct dma_chan *chan)
>  {
>  	struct at_dma_chan      *atchan = to_at_dma_chan(chan);
> -	struct at_desc *desc_first = atc_first_active(atchan);
> +	struct at_desc *desc_first;
>  	struct at_desc *desc_cur;
> +	unsigned long flags;
>  	int ret = 0, count = 0;
>  
> +	spin_lock_irqsave(&atchan->lock, flags);
> +	desc_first = atc_first_active(atchan);
> +
>  	/*
>  	 * Initialize necessary values in the first time.
>  	 * remain_desc record remain desc length.
> @@ -311,6 +315,7 @@ static int atc_get_bytes_left(struct dma_chan *chan)
>  	}
>  
>  out:
> +	spin_unlock_irqrestore(&atchan->lock, flags);
>  	return ret;
>  }
>  
> @@ -318,12 +323,14 @@ out:
>   * atc_chain_complete - finish work for one transaction chain
>   * @atchan: channel we work on
>   * @desc: descriptor at the head of the chain we want do complete
> - *
> - * Called with atchan->lock held and bh disabled */
> + */
>  static void
>  atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
>  {
>  	struct dma_async_tx_descriptor	*txd = &desc->txd;
> +	unsigned long			flags;
> +
> +	spin_lock_irqsave(&atchan->lock, flags);
>  
>  	dev_vdbg(chan2dev(&atchan->chan_common),
>  		"descriptor %u complete\n", txd->cookie);
> @@ -337,6 +344,8 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
>  	/* move myself to free_list */
>  	list_move(&desc->desc_node, &atchan->free_list);
>  
> +	spin_unlock_irqrestore(&atchan->lock, flags);
> +
>  	dma_descriptor_unmap(txd);
>  	/* for cyclic transfers,
>  	 * no need to replay callback function while stopping */
> @@ -344,10 +353,6 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
>  		dma_async_tx_callback	callback = txd->callback;
>  		void			*param = txd->callback_param;
>  
> -		/*
> -		 * The API requires that no submissions are done from a
> -		 * callback, so we don't need to drop the lock here
> -		 */
>  		if (callback)
>  			callback(param);
>  	}
> @@ -362,15 +367,17 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
>   * 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.
> @@ -382,6 +389,8 @@ static void atc_complete_all(struct at_dma_chan *atchan)
>  	/* 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);
>  }
> @@ -389,23 +398,35 @@ static void atc_complete_all(struct at_dma_chan *atchan)
>  /**
>   * atc_advance_work - at the end of a transaction, move forward
>   * @atchan: channel where the transaction ended
> - *
> - * Called with atchan->lock held and bh disabled
>   */
>  static void atc_advance_work(struct at_dma_chan *atchan)
>  {
> +	unsigned long	flags;
> +
>  	dev_vdbg(chan2dev(&atchan->chan_common), "advance_work\n");
>  
> -	if (atc_chan_is_enabled(atchan))
> +	spin_lock_irqsave(&atchan->lock, flags);
> +
> +	if (atc_chan_is_enabled(atchan)) {
> +		spin_unlock_irqrestore(&atchan->lock, flags);
>  		return;
> +	}
>  
>  	if (list_empty(&atchan->active_list) ||
>  	    list_is_singular(&atchan->active_list)) {
> +		spin_unlock_irqrestore(&atchan->lock, flags);
>  		atc_complete_all(atchan);
>  	} else {
> -		atc_chain_complete(atchan, atc_first_active(atchan));
> +		struct at_desc *desc_first = atc_first_active(atchan);
> +
> +		spin_unlock_irqrestore(&atchan->lock, flags);
> +		atc_chain_complete(atchan, desc_first);
> +		barrier();
>  		/* advance work */
> -		atc_dostart(atchan, atc_first_active(atchan));
> +		spin_lock_irqsave(&atchan->lock, flags);
> +		desc_first = atc_first_active(atchan);
> +		atc_dostart(atchan, desc_first);
> +		spin_unlock_irqrestore(&atchan->lock, flags);
>  	}
>  }
>  
> @@ -413,13 +434,14 @@ static void atc_advance_work(struct at_dma_chan *atchan)
>  /**
>   * atc_handle_error - handle errors reported by DMA controller
>   * @atchan: channel where error occurs
> - *
> - * Called with atchan->lock held and bh disabled
>   */
>  static void atc_handle_error(struct at_dma_chan *atchan)
>  {
>  	struct at_desc *bad_desc;
>  	struct at_desc *child;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&atchan->lock, flags);
>  
>  	/*
>  	 * The descriptor currently at the head of the active list is
> @@ -452,6 +474,8 @@ static void atc_handle_error(struct at_dma_chan *atchan)
>  	list_for_each_entry(child, &bad_desc->tx_list, desc_node)
>  		atc_dump_lli(atchan, &child->lli);
>  
> +	spin_unlock_irqrestore(&atchan->lock, flags);
> +
>  	/* Pretend the descriptor completed successfully */
>  	atc_chain_complete(atchan, bad_desc);
>  }
> @@ -459,19 +483,27 @@ static void atc_handle_error(struct at_dma_chan *atchan)
>  /**
>   * atc_handle_cyclic - at the end of a period, run callback function
>   * @atchan: channel used for cyclic operations
> - *
> - * Called with atchan->lock held and bh disabled
>   */
>  static void atc_handle_cyclic(struct at_dma_chan *atchan)
>  {
> -	struct at_desc			*first = atc_first_active(atchan);
> -	struct dma_async_tx_descriptor	*txd = &first->txd;
> -	dma_async_tx_callback		callback = txd->callback;
> -	void				*param = txd->callback_param;
> +	struct at_desc			*first;
> +	struct dma_async_tx_descriptor	*txd;
> +	dma_async_tx_callback		callback;
> +	void				*param;
> +	u32				dscr;
> +	unsigned long			flags;
> +
> +	spin_lock_irqsave(&atchan->lock, flags);
> +	first = atc_first_active(atchan);
> +	dscr = channel_readl(atchan, DSCR);
> +	spin_unlock_irqrestore(&atchan->lock, flags);
> +
> +	txd = &first->txd;
> +	callback = txd->callback;
> +	param = txd->callback_param;
>  
>  	dev_vdbg(chan2dev(&atchan->chan_common),
> -			"new cyclic period llp 0x%08x\n",
> -			channel_readl(atchan, DSCR));
> +			"new cyclic period llp 0x%08x\n", dscr);
>  
>  	if (callback)
>  		callback(param);
> @@ -482,17 +514,13 @@ static void atc_handle_cyclic(struct at_dma_chan *atchan)
>  static void atc_tasklet(unsigned long data)
>  {
>  	struct at_dma_chan *atchan = (struct at_dma_chan *)data;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&atchan->lock, flags);
>  	if (test_and_clear_bit(ATC_IS_ERROR, &atchan->status))
>  		atc_handle_error(atchan);
>  	else if (atc_chan_is_cyclic(atchan))
>  		atc_handle_cyclic(atchan);
>  	else
>  		atc_advance_work(atchan);
> -
> -	spin_unlock_irqrestore(&atchan->lock, flags);
>  }
>  
>  static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
> @@ -1013,6 +1041,11 @@ static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>  		spin_unlock_irqrestore(&atchan->lock, flags);
>  	} else if (cmd == DMA_TERMINATE_ALL) {
>  		struct at_desc	*desc, *_desc;
> +
> +		/* Disable interrupts */
> +		atc_disable_chan_irq(atdma, chan->chan_id);
> +		tasklet_disable(&atchan->tasklet);
> +
>  		/*
>  		 * This is only called when something went wrong elsewhere, so
>  		 * we don't really care about the data. Just disable the
> @@ -1033,14 +1066,22 @@ static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>  		list_splice_init(&atchan->active_list, &list);
>  
>  		/* Flush all pending and queued descriptors */
> -		list_for_each_entry_safe(desc, _desc, &list, desc_node)
> +		list_for_each_entry_safe(desc, _desc, &list, desc_node) {
> +			spin_unlock_irqrestore(&atchan->lock, flags);
>  			atc_chain_complete(atchan, desc);
> +			spin_lock_irqsave(&atchan->lock, flags);
> +		}
>  
>  		clear_bit(ATC_IS_PAUSED, &atchan->status);
>  		/* if channel dedicated to cyclic operations, free it */
>  		clear_bit(ATC_IS_CYCLIC, &atchan->status);
>  
>  		spin_unlock_irqrestore(&atchan->lock, flags);
> +
> +		/* Re-enable channel for future operations */
> +		tasklet_enable(&atchan->tasklet);
> +		atc_enable_chan_irq(atdma, chan->chan_id);
> +
>  	} else if (cmd == DMA_SLAVE_CONFIG) {
>  		return set_runtime_config(chan, (struct dma_slave_config *)arg);
>  	} else {
> @@ -1065,8 +1106,6 @@ atc_tx_status(struct dma_chan *chan,
>  		dma_cookie_t cookie,
>  		struct dma_tx_state *txstate)
>  {
> -	struct at_dma_chan	*atchan = to_at_dma_chan(chan);
> -	unsigned long		flags;
>  	enum dma_status		ret;
>  	int bytes = 0;
>  
> @@ -1080,13 +1119,9 @@ atc_tx_status(struct dma_chan *chan,
>  	if (!txstate)
>  		return DMA_ERROR;
>  
> -	spin_lock_irqsave(&atchan->lock, flags);
> -
>  	/*  Get number of bytes left in the active transactions */
>  	bytes = atc_get_bytes_left(chan);
>  
> -	spin_unlock_irqrestore(&atchan->lock, flags);
> -
>  	if (unlikely(bytes < 0)) {
>  		dev_vdbg(chan2dev(chan), "get residual bytes error\n");
>  		return DMA_ERROR;
> @@ -1101,24 +1136,10 @@ atc_tx_status(struct dma_chan *chan,
>  }
>  
>  /**
> - * atc_issue_pending - try to finish work
> + * atc_issue_pending - void function
>   * @chan: target DMA channel
>   */
> -static void atc_issue_pending(struct dma_chan *chan)
> -{
> -	struct at_dma_chan	*atchan = to_at_dma_chan(chan);
> -	unsigned long		flags;
> -
> -	dev_vdbg(chan2dev(chan), "issue_pending\n");
> -
> -	/* Not needed for cyclic transfers */
> -	if (atc_chan_is_cyclic(atchan))
> -		return;
> -
> -	spin_lock_irqsave(&atchan->lock, flags);
> -	atc_advance_work(atchan);
> -	spin_unlock_irqrestore(&atchan->lock, flags);
> -}
> +static void atc_issue_pending(struct dma_chan *chan) {}
>  
>  /**
>   * atc_alloc_chan_resources - allocate resources for DMA channel
> -- 
> 1.8.2.2
> 

-- 

[RFC PATCH 0/2] dmaengine: at_hdmac: fix locking according to slave DMA requirements

[Jouko Haapaluoma]

Hi

Thanks for the patches. We got the DMA working for now but the tasklet_disable() and tasklet_enable() had to be removed from atc_control().

If the device driver calls dmaengine_terminate_all() from the callback (like in our previous deadlock example), the tasklet_disable() will cause
another deadlock because the tasklet will then wait for itself to close.

The tasklet_disable() seems to be used to ensure that no tasklet is running when terminating the DMA transfers. This prevents the terminate_all from
happening in between the critical sections in the tasklet which are locked with atchan->lock. Maybe the tasklet needs to be implemented so that it can
detect and recover if the terminate_all was called in between the critical sections in the tasklet?

BR,
Jouko Haapaluoma