[PATCH V4 0/3] dmaengine: qcom_hidma: add error reporting

[PATCH V4 0/3] dmaengine: qcom_hidma: add error reporting

[Sinan Kaya]

Three changes in the error handling area:

1. There is a race condition between data transfer callback and descriptor
   free code. The callback routine may decide to clear the resources even
   though the descriptor has not yet been freed.

2. DMA Engine framework now supports direct error reporting to the client
   via the callback.

   Pass the DMA errors to the client by passing a result argument. The HW
   only supports a generic error when something goes wrong. That's why,
   using DMA_TRANS_ABORTED all the time.

3. The HIDMA driver is capable of error detection. However, the error was
   not being passed back to the client when tx_status API is called.

------------------------
Changes from v3 (http://www.spinics.net/lists/dmaengine/msg10742.html)
------------------------
- Clarify that the reset task is gone and also the reset behavior has
changed. The driver will no longer recover the HW automatically. The
driver depends on the client to call terminate_channel.


Sinan Kaya (3):
  dmaengine: qcom_hidma: release the descriptor before the callback
  dmaengine: qcom_hidma: report transfer errors with new interface
  dmaengine: qcom_hidma: add error reporting for tx_status

 drivers/dma/qcom/hidma.c    | 50 +++++++++++++++++++++++++++++++++++----------
 drivers/dma/qcom/hidma.h    |  2 +-
 drivers/dma/qcom/hidma_ll.c | 32 +++++++----------------------
 3 files changed, 47 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH V4 1/3] dmaengine: qcom_hidma: release the descriptor before the callback

[Sinan Kaya]

There is a race condition between data transfer callback and descriptor
free code. The callback routine may decide to clear the resources even
though the descriptor has not yet been freed.

Instead of calling the callback first and then releasing the memory,
this code is changing the order to return the descriptor back to the
free pool and then call the user provided callback.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/hidma.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index 1197fbf..b8493ba 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -111,6 +111,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 	struct dma_async_tx_descriptor *desc;
 	dma_cookie_t last_cookie;
 	struct hidma_desc *mdesc;
+	struct hidma_desc *next;
 	unsigned long irqflags;
 	struct list_head list;
 
@@ -122,8 +123,9 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 	spin_unlock_irqrestore(&mchan->lock, irqflags);
 
 	/* Execute callbacks and run dependencies */
-	list_for_each_entry(mdesc, &list, node) {
+	list_for_each_entry_safe(mdesc, next, &list, node) {
 		enum dma_status llstat;
+		struct dmaengine_desc_callback cb;
 
 		desc = &mdesc->desc;
 
@@ -132,18 +134,18 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 		spin_unlock_irqrestore(&mchan->lock, irqflags);
 
 		llstat = hidma_ll_status(mdma->lldev, mdesc->tre_ch);
-		if (llstat == DMA_COMPLETE)
-			dmaengine_desc_get_callback_invoke(desc, NULL);
+		dmaengine_desc_get_callback(desc, &cb);
 
 		last_cookie = desc->cookie;
 		dma_run_dependencies(desc);
-	}
 
-	/* Free descriptors */
-	spin_lock_irqsave(&mchan->lock, irqflags);
-	list_splice_tail_init(&list, &mchan->free);
-	spin_unlock_irqrestore(&mchan->lock, irqflags);
+		spin_lock_irqsave(&mchan->lock, irqflags);
+		list_move(&mdesc->node, &mchan->free);
+		spin_unlock_irqrestore(&mchan->lock, irqflags);
 
+		if (llstat == DMA_COMPLETE)
+			dmaengine_desc_callback_invoke(&cb, NULL);
+	}
 }
 
 /*
-- 
1.9.1

[PATCH V4 2/3] dmaengine: qcom_hidma: report transfer errors with new interface

[Sinan Kaya]

Pass the DMA errors to the client by passing a result argument. The HW only
supports a generic error when something goes wrong. That's why, using
DMA_TRANS_ABORTED all the time.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/hidma.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index b8493ba..ea24863 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -126,6 +126,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 	list_for_each_entry_safe(mdesc, next, &list, node) {
 		enum dma_status llstat;
 		struct dmaengine_desc_callback cb;
+		struct dmaengine_result result;
 
 		desc = &mdesc->desc;
 
@@ -141,10 +142,15 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 
 		spin_lock_irqsave(&mchan->lock, irqflags);
 		list_move(&mdesc->node, &mchan->free);
-		spin_unlock_irqrestore(&mchan->lock, irqflags);
 
 		if (llstat == DMA_COMPLETE)
-			dmaengine_desc_callback_invoke(&cb, NULL);
+			result.result = DMA_TRANS_NOERROR;
+		else
+			result.result = DMA_TRANS_ABORTED;
+
+		spin_unlock_irqrestore(&mchan->lock, irqflags);
+
+		dmaengine_desc_callback_invoke(&cb, &result);
 	}
 }
 
-- 
1.9.1

[PATCH V4 3/3] dmaengine: qcom_hidma: add error reporting for tx_status

[Sinan Kaya]

The HIDMA driver is capable of error detection. However, the error was
not being passed back to the client when tx_status API is called.

Changing the error handling behavior to follow this oder.

1. dmaengine asserts error interrupt
2. Driver receives and mark's the txn as error
3. Driver completes the txn and intimates the client. No further
   submissions. Drop the locks before calling callback, as subsequent
   processing by client maybe in callback thread.
4. Client invokes status and you can return error
5. On error, client calls terminate_all. You can reset channel, free all
   descriptors in the active, pending and completed lists
6. Client prepares new txn and so on.

As part of this work, got rid of the reset in the interrupt handler when
an error happens and the HW is put into disabled state. The only way to
recover is for the client to terminate the channel.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/hidma.c    | 30 +++++++++++++++++++++++++-----
 drivers/dma/qcom/hidma.h    |  2 +-
 drivers/dma/qcom/hidma_ll.c | 32 +++++++-------------------------
 3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index ea24863..e244e10 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -129,6 +129,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 		struct dmaengine_result result;
 
 		desc = &mdesc->desc;
+		last_cookie = desc->cookie;
 
 		spin_lock_irqsave(&mchan->lock, irqflags);
 		dma_cookie_complete(desc);
@@ -137,15 +138,15 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 		llstat = hidma_ll_status(mdma->lldev, mdesc->tre_ch);
 		dmaengine_desc_get_callback(desc, &cb);
 
-		last_cookie = desc->cookie;
 		dma_run_dependencies(desc);
 
 		spin_lock_irqsave(&mchan->lock, irqflags);
 		list_move(&mdesc->node, &mchan->free);
 
-		if (llstat == DMA_COMPLETE)
+		if (llstat == DMA_COMPLETE) {
+			mchan->last_success = last_cookie;
 			result.result = DMA_TRANS_NOERROR;
-		else
+		} else
 			result.result = DMA_TRANS_ABORTED;
 
 		spin_unlock_irqrestore(&mchan->lock, irqflags);
@@ -246,6 +247,19 @@ static void hidma_issue_pending(struct dma_chan *dmach)
 		hidma_ll_start(dmadev->lldev);
 }
 
+static inline bool hidma_txn_is_success(dma_cookie_t cookie,
+		dma_cookie_t last_success, dma_cookie_t last_used)
+{
+	if (last_success <= last_used) {
+		if ((cookie <= last_success) || (cookie > last_used))
+			return true;
+	} else {
+		if ((cookie <= last_success) && (cookie > last_used))
+			return true;
+	}
+	return false;
+}
+
 static enum dma_status hidma_tx_status(struct dma_chan *dmach,
 				       dma_cookie_t cookie,
 				       struct dma_tx_state *txstate)
@@ -254,8 +268,13 @@ static enum dma_status hidma_tx_status(struct dma_chan *dmach,
 	enum dma_status ret;
 
 	ret = dma_cookie_status(dmach, cookie, txstate);
-	if (ret == DMA_COMPLETE)
-		return ret;
+	if (ret == DMA_COMPLETE) {
+		bool is_success;
+
+		is_success = hidma_txn_is_success(cookie, mchan->last_success,
+						  dmach->cookie);
+		return is_success ? ret : DMA_ERROR;
+	}
 
 	if (mchan->paused && (ret == DMA_IN_PROGRESS)) {
 		unsigned long flags;
@@ -406,6 +425,7 @@ static int hidma_terminate_channel(struct dma_chan *chan)
 	hidma_process_completed(mchan);
 
 	spin_lock_irqsave(&mchan->lock, irqflags);
+	mchan->last_success = 0;
 	list_splice_init(&mchan->active, &list);
 	list_splice_init(&mchan->prepared, &list);
 	list_splice_init(&mchan->completed, &list);
diff --git a/drivers/dma/qcom/hidma.h b/drivers/dma/qcom/hidma.h
index db413a5..e52e207 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -72,7 +72,6 @@ struct hidma_lldev {
 
 	u32 tre_write_offset;           /* TRE write location              */
 	struct tasklet_struct task;	/* task delivering notifications   */
-	struct tasklet_struct rst_task;	/* task to reset HW                */
 	DECLARE_KFIFO_PTR(handoff_fifo,
 		struct hidma_tre *);    /* pending TREs FIFO               */
 };
@@ -89,6 +88,7 @@ struct hidma_chan {
 	bool				allocated;
 	char				dbg_name[16];
 	u32				dma_sig;
+	dma_cookie_t			last_success;
 
 	/*
 	 * active descriptor on this channel
diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
index ad20dfb..3224f24 100644
--- a/drivers/dma/qcom/hidma_ll.c
+++ b/drivers/dma/qcom/hidma_ll.c
@@ -381,27 +381,6 @@ static int hidma_ll_reset(struct hidma_lldev *lldev)
 }
 
 /*
- * Abort all transactions and perform a reset.
- */
-static void hidma_ll_abort(unsigned long arg)
-{
-	struct hidma_lldev *lldev = (struct hidma_lldev *)arg;
-	u8 err_code = HIDMA_EVRE_STATUS_ERROR;
-	u8 err_info = 0xFF;
-	int rc;
-
-	hidma_cleanup_pending_tre(lldev, err_info, err_code);
-
-	/* reset the channel for recovery */
-	rc = hidma_ll_setup(lldev);
-	if (rc) {
-		dev_err(lldev->dev, "channel reinitialize failed after error\n");
-		return;
-	}
-	writel(ENABLE_IRQS, lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
-}
-
-/*
  * The interrupt handler for HIDMA will try to consume as many pending
  * EVRE from the event queue as possible. Each EVRE has an associated
  * TRE that holds the user interface parameters. EVRE reports the
@@ -454,13 +433,18 @@ irqreturn_t hidma_ll_inthandler(int chirq, void *arg)
 
 	while (cause) {
 		if (cause & HIDMA_ERR_INT_MASK) {
-			dev_err(lldev->dev, "error 0x%x, resetting...\n",
+			dev_err(lldev->dev, "error 0x%x, disabling...\n",
 					cause);
 
 			/* Clear out pending interrupts */
 			writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
 
-			tasklet_schedule(&lldev->rst_task);
+			/* No further submissions. */
+			hidma_ll_disable(lldev);
+
+			/* Driver completes the txn and intimates the client.*/
+			hidma_cleanup_pending_tre(lldev, 0xFF,
+						  HIDMA_EVRE_STATUS_ERROR);
 			goto out;
 		}
 
@@ -808,7 +792,6 @@ struct hidma_lldev *hidma_ll_init(struct device *dev, u32 nr_tres,
 		return NULL;
 
 	spin_lock_init(&lldev->lock);
-	tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
 	tasklet_init(&lldev->task, hidma_ll_tre_complete, (unsigned long)lldev);
 	lldev->initialized = 1;
 	writel(ENABLE_IRQS, lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
@@ -831,7 +814,6 @@ int hidma_ll_uninit(struct hidma_lldev *lldev)
 
 	required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
 	tasklet_kill(&lldev->task);
-	tasklet_kill(&lldev->rst_task);
 	memset(lldev->trepool, 0, required_bytes);
 	lldev->trepool = NULL;
 	lldev->pending_tre_count = 0;
-- 
1.9.1

[PATCH V4 0/3] dmaengine: qcom_hidma: add error reporting

[Vinod Koul]

On Wed, Aug 31, 2016 at 11:10:26AM -0400, Sinan Kaya wrote:
> Three changes in the error handling area:
> 
> 1. There is a race condition between data transfer callback and descriptor
>    free code. The callback routine may decide to clear the resources even
>    though the descriptor has not yet been freed.
> 
> 2. DMA Engine framework now supports direct error reporting to the client
>    via the callback.
> 
>    Pass the DMA errors to the client by passing a result argument. The HW
>    only supports a generic error when something goes wrong. That's why,
>    using DMA_TRANS_ABORTED all the time.
> 
> 3. The HIDMA driver is capable of error detection. However, the error was
>    not being passed back to the client when tx_status API is called.

Applied all, thanks

-- 
~Vinod