* [PATCH V3 0/3] dmaengine: qcom_hidma: add error reporting
@ 2016-08-23 4:48 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: dmaengine, timur, cov, vinod.koul, jcm
Cc: agross, linux-arm-msm, linux-arm-kernel, Sinan Kaya
Three changes:
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 v2 (https://patchwork.kernel.org/patch/9253379/)
------------------------
- Implement the error passing to the client.
- Get rid of the reset in the interrupt handler when an error happens and
put the HW into disabled state. The only way to recover is for the client to
terminate the 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 | 1 +
drivers/dma/qcom/hidma_ll.c | 32 +++++++----------------------
3 files changed, 47 insertions(+), 36 deletions(-)
--
1.8.2.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 0/3] dmaengine: qcom_hidma: add error reporting
@ 2016-08-23 4:48 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: linux-arm-kernel
Three changes:
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 v2 (https://patchwork.kernel.org/patch/9253379/)
------------------------
- Implement the error passing to the client.
- Get rid of the reset in the interrupt handler when an error happens and
put the HW into disabled state. The only way to recover is for the client to
terminate the 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 | 1 +
drivers/dma/qcom/hidma_ll.c | 32 +++++++----------------------
3 files changed, 47 insertions(+), 36 deletions(-)
--
1.8.2.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 1/3] dmaengine: qcom_hidma: release the descriptor before the callback
2016-08-23 4:48 ` Sinan Kaya
@ 2016-08-23 4:48 ` Sinan Kaya
-1 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: dmaengine, timur, cov, vinod.koul, jcm
Cc: agross, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
Dan Williams, Lars-Peter Clausen, Andy Shevchenko, Dave Jiang,
linux-kernel
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.8.2.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH V3 1/3] dmaengine: qcom_hidma: release the descriptor before the callback
@ 2016-08-23 4:48 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: linux-arm-kernel
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.8.2.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH V3 2/3] dmaengine: qcom_hidma: report transfer errors with new interface
2016-08-23 4:48 ` Sinan Kaya
@ 2016-08-23 4:48 ` Sinan Kaya
-1 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: dmaengine, timur, cov, vinod.koul, jcm
Cc: agross, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
Dan Williams, Lars-Peter Clausen, Andy Shevchenko, Dave Jiang,
linux-kernel
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.8.2.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH V3 2/3] dmaengine: qcom_hidma: report transfer errors with new interface
@ 2016-08-23 4:48 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: linux-arm-kernel
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.8.2.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
2016-08-23 4:48 ` Sinan Kaya
@ 2016-08-23 4:48 ` Sinan Kaya
-1 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: dmaengine, timur, cov, vinod.koul, jcm
Cc: agross, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
Dan Williams, Lars-Peter Clausen, Andy Shevchenko, Dave Jiang,
linux-kernel
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.
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
drivers/dma/qcom/hidma.c | 30 +++++++++++++++++++++++++-----
drivers/dma/qcom/hidma.h | 1 +
drivers/dma/qcom/hidma_ll.c | 32 +++++++-------------------------
3 files changed, 33 insertions(+), 30 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..f78aef0 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -89,6 +89,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.8.2.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
@ 2016-08-23 4:48 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-23 4:48 UTC (permalink / raw)
To: linux-arm-kernel
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.
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
drivers/dma/qcom/hidma.c | 30 +++++++++++++++++++++++++-----
drivers/dma/qcom/hidma.h | 1 +
drivers/dma/qcom/hidma_ll.c | 32 +++++++-------------------------
3 files changed, 33 insertions(+), 30 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..f78aef0 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -89,6 +89,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.8.2.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH V3 0/3] dmaengine: qcom_hidma: add error reporting
2016-08-23 4:48 ` Sinan Kaya
@ 2016-08-29 15:30 ` Sinan Kaya
-1 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-29 15:30 UTC (permalink / raw)
To: dmaengine, vinod.koul
Cc: timur, cov, jcm, agross, linux-arm-msm, linux-arm-kernel
Hi Vinod,
On 8/23/2016 12:48 AM, Sinan Kaya wrote:
> Three changes:
> 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 v2 (https://patchwork.kernel.org/patch/9253379/)
> ------------------------
> - Implement the error passing to the client.
> - Get rid of the reset in the interrupt handler when an error happens and
> put the HW into disabled state. The only way to recover is for the client to
> terminate the 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 | 1 +
> drivers/dma/qcom/hidma_ll.c | 32 +++++++----------------------
> 3 files changed, 47 insertions(+), 36 deletions(-)
>
Any feedback here?
Does this implementation look like what you were expecting?
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 0/3] dmaengine: qcom_hidma: add error reporting
@ 2016-08-29 15:30 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-29 15:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi Vinod,
On 8/23/2016 12:48 AM, Sinan Kaya wrote:
> Three changes:
> 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 v2 (https://patchwork.kernel.org/patch/9253379/)
> ------------------------
> - Implement the error passing to the client.
> - Get rid of the reset in the interrupt handler when an error happens and
> put the HW into disabled state. The only way to recover is for the client to
> terminate the 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 | 1 +
> drivers/dma/qcom/hidma_ll.c | 32 +++++++----------------------
> 3 files changed, 47 insertions(+), 36 deletions(-)
>
Any feedback here?
Does this implementation look like what you were expecting?
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
2016-08-23 4:48 ` Sinan Kaya
@ 2016-08-30 17:04 ` Vinod Koul
-1 siblings, 0 replies; 18+ messages in thread
From: Vinod Koul @ 2016-08-30 17:04 UTC (permalink / raw)
To: Sinan Kaya
Cc: dmaengine, timur, cov, jcm, agross, linux-arm-msm,
linux-arm-kernel, Dan Williams, Lars-Peter Clausen,
Andy Shevchenko, Dave Jiang, linux-kernel
On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
> spin_lock_init(&lldev->lock);
> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
??
This is not described in changlog? If you are not using in anywhere else,
then rst_task variable should be removed too..
--
~Vinod
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
@ 2016-08-30 17:04 ` Vinod Koul
0 siblings, 0 replies; 18+ messages in thread
From: Vinod Koul @ 2016-08-30 17:04 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
> spin_lock_init(&lldev->lock);
> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
??
This is not described in changlog? If you are not using in anywhere else,
then rst_task variable should be removed too..
--
~Vinod
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
2016-08-30 17:04 ` Vinod Koul
@ 2016-08-30 17:37 ` Sinan Kaya
-1 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-30 17:37 UTC (permalink / raw)
To: Vinod Koul
Cc: dmaengine, timur, cov, jcm, agross, linux-arm-msm,
linux-arm-kernel, Dan Williams, Lars-Peter Clausen,
Andy Shevchenko, Dave Jiang, linux-kernel
On 8/30/2016 1:04 PM, Vinod Koul wrote:
> On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
>
>> spin_lock_init(&lldev->lock);
>> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
>
> ??
>
> This is not described in changlog? If you are not using in anywhere else,
> then rst_task variable should be removed too..
>
Sure, I can add more description. I thought I did mention that we are no longer
resetting the hardware in ISR.
I can certainly mention that rst_task variable is removed and can get rid of it.
It is no longer in use.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
@ 2016-08-30 17:37 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-30 17:37 UTC (permalink / raw)
To: linux-arm-kernel
On 8/30/2016 1:04 PM, Vinod Koul wrote:
> On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
>
>> spin_lock_init(&lldev->lock);
>> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
>
> ??
>
> This is not described in changlog? If you are not using in anywhere else,
> then rst_task variable should be removed too..
>
Sure, I can add more description. I thought I did mention that we are no longer
resetting the hardware in ISR.
I can certainly mention that rst_task variable is removed and can get rid of it.
It is no longer in use.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
2016-08-30 17:37 ` Sinan Kaya
@ 2016-08-30 17:51 ` Sinan Kaya
-1 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-30 17:51 UTC (permalink / raw)
To: Vinod Koul
Cc: dmaengine, timur, cov, jcm, agross, linux-arm-msm,
linux-arm-kernel, Dan Williams, Lars-Peter Clausen,
Andy Shevchenko, Dave Jiang, linux-kernel
On 8/30/2016 1:37 PM, Sinan Kaya wrote:
> On 8/30/2016 1:04 PM, Vinod Koul wrote:
>> On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
>>
>>> spin_lock_init(&lldev->lock);
>>> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
>>
>> ??
>>
>> This is not described in changlog? If you are not using in anywhere else,
>> then rst_task variable should be removed too..
>>
>
> Sure, I can add more description. I thought I did mention that we are no longer
> resetting the hardware in ISR.
>
> I can certainly mention that rst_task variable is removed and can get rid of it.
> It is no longer in use.
>
It looks like I mentioned this in the cover letter only not in the commit message.
I'll pull the description into the commit message too.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
@ 2016-08-30 17:51 ` Sinan Kaya
0 siblings, 0 replies; 18+ messages in thread
From: Sinan Kaya @ 2016-08-30 17:51 UTC (permalink / raw)
To: linux-arm-kernel
On 8/30/2016 1:37 PM, Sinan Kaya wrote:
> On 8/30/2016 1:04 PM, Vinod Koul wrote:
>> On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
>>
>>> spin_lock_init(&lldev->lock);
>>> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
>>
>> ??
>>
>> This is not described in changlog? If you are not using in anywhere else,
>> then rst_task variable should be removed too..
>>
>
> Sure, I can add more description. I thought I did mention that we are no longer
> resetting the hardware in ISR.
>
> I can certainly mention that rst_task variable is removed and can get rid of it.
> It is no longer in use.
>
It looks like I mentioned this in the cover letter only not in the commit message.
I'll pull the description into the commit message too.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
2016-08-30 17:51 ` Sinan Kaya
@ 2016-08-31 4:21 ` Vinod Koul
-1 siblings, 0 replies; 18+ messages in thread
From: Vinod Koul @ 2016-08-31 4:21 UTC (permalink / raw)
To: Sinan Kaya
Cc: dmaengine, timur, cov, jcm, agross, linux-arm-msm,
linux-arm-kernel, Dan Williams, Lars-Peter Clausen,
Andy Shevchenko, Dave Jiang, linux-kernel
On Tue, Aug 30, 2016 at 01:51:02PM -0400, Sinan Kaya wrote:
> On 8/30/2016 1:37 PM, Sinan Kaya wrote:
> > On 8/30/2016 1:04 PM, Vinod Koul wrote:
> >> On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
> >>
> >>> spin_lock_init(&lldev->lock);
> >>> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
> >>
> >> ??
> >>
> >> This is not described in changlog? If you are not using in anywhere else,
> >> then rst_task variable should be removed too..
> >>
> >
> > Sure, I can add more description. I thought I did mention that we are no longer
> > resetting the hardware in ISR.
> >
> > I can certainly mention that rst_task variable is removed and can get rid of it.
> > It is no longer in use.
> >
>
> It looks like I mentioned this in the cover letter only not in the commit message.
> I'll pull the description into the commit message too.
Sounds good to me, thanks
--
~Vinod
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status
@ 2016-08-31 4:21 ` Vinod Koul
0 siblings, 0 replies; 18+ messages in thread
From: Vinod Koul @ 2016-08-31 4:21 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 30, 2016 at 01:51:02PM -0400, Sinan Kaya wrote:
> On 8/30/2016 1:37 PM, Sinan Kaya wrote:
> > On 8/30/2016 1:04 PM, Vinod Koul wrote:
> >> On Tue, Aug 23, 2016 at 12:48:11AM -0400, Sinan Kaya wrote:
> >>
> >>> spin_lock_init(&lldev->lock);
> >>> - tasklet_init(&lldev->rst_task, hidma_ll_abort, (unsigned long)lldev);
> >>
> >> ??
> >>
> >> This is not described in changlog? If you are not using in anywhere else,
> >> then rst_task variable should be removed too..
> >>
> >
> > Sure, I can add more description. I thought I did mention that we are no longer
> > resetting the hardware in ISR.
> >
> > I can certainly mention that rst_task variable is removed and can get rid of it.
> > It is no longer in use.
> >
>
> It looks like I mentioned this in the cover letter only not in the commit message.
> I'll pull the description into the commit message too.
Sounds good to me, thanks
--
~Vinod
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-08-31 4:21 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-23 4:48 [PATCH V3 0/3] dmaengine: qcom_hidma: add error reporting Sinan Kaya
2016-08-23 4:48 ` Sinan Kaya
2016-08-23 4:48 ` [PATCH V3 1/3] dmaengine: qcom_hidma: release the descriptor before the callback Sinan Kaya
2016-08-23 4:48 ` Sinan Kaya
2016-08-23 4:48 ` [PATCH V3 2/3] dmaengine: qcom_hidma: report transfer errors with new interface Sinan Kaya
2016-08-23 4:48 ` Sinan Kaya
2016-08-23 4:48 ` [PATCH V3 3/3] dmaengine: qcom_hidma: add error reporting for tx_status Sinan Kaya
2016-08-23 4:48 ` Sinan Kaya
2016-08-30 17:04 ` Vinod Koul
2016-08-30 17:04 ` Vinod Koul
2016-08-30 17:37 ` Sinan Kaya
2016-08-30 17:37 ` Sinan Kaya
2016-08-30 17:51 ` Sinan Kaya
2016-08-30 17:51 ` Sinan Kaya
2016-08-31 4:21 ` Vinod Koul
2016-08-31 4:21 ` Vinod Koul
2016-08-29 15:30 ` [PATCH V3 0/3] dmaengine: qcom_hidma: add error reporting Sinan Kaya
2016-08-29 15:30 ` Sinan Kaya
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.