* [PATCH 1/5] dmaengine: Adding error handling flag
2016-06-27 23:09 [PATCH 0/5] dmaengine: ioatdma: DMA error reporting Dave Jiang
@ 2016-06-27 23:09 ` Dave Jiang
2016-06-27 23:09 ` [PATCH 2/5] dmaengine: IOATDMA: Add error handling to driver Dave Jiang
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Dave Jiang @ 2016-06-27 23:09 UTC (permalink / raw)
To: vinod.koul, allen.hubbe, jdmason; +Cc: dmaengine, linux-ntb
Adding error flag for the call back descriptor to notify upper layer that
an error has occurred with this particular DMA op.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
include/linux/dmaengine.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 0174337..6524881 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -453,6 +453,20 @@ struct dmaengine_unmap_data {
};
/**
+ * enum err_result_flags - result of DMA operations
+ * @ERR_DMA_NONE - no errors
+ * @ERR_DMA_READ - DMA read error
+ * @ERR_DMA_WRITE - DMA write error
+ * @ERR_DMA_ABORT - Operation aborted
+ */
+enum err_result_flags {
+ ERR_DMA_NONE = 0,
+ ERR_DMA_READ,
+ ERR_DMA_WRITE,
+ ERR_DMA_ABORT,
+};
+
+/**
* struct dma_async_tx_descriptor - async transaction descriptor
* ---dma generic offload fields---
* @cookie: tracking cookie for this transaction, set to -EBUSY if
@@ -480,6 +494,7 @@ struct dma_async_tx_descriptor {
dma_async_tx_callback callback;
void *callback_param;
struct dmaengine_unmap_data *unmap;
+ enum err_result_flags result;
#ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
struct dma_async_tx_descriptor *next;
struct dma_async_tx_descriptor *parent;
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/5] dmaengine: IOATDMA: Add error handling to driver
2016-06-27 23:09 [PATCH 0/5] dmaengine: ioatdma: DMA error reporting Dave Jiang
2016-06-27 23:09 ` [PATCH 1/5] dmaengine: Adding error handling flag Dave Jiang
@ 2016-06-27 23:09 ` Dave Jiang
2016-06-28 1:08 ` Allen Hubbe
2016-06-27 23:09 ` [PATCH 3/5] dmaengine: ioatdma: add error strings to chanerr output Dave Jiang
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Dave Jiang @ 2016-06-27 23:09 UTC (permalink / raw)
To: vinod.koul, allen.hubbe, jdmason; +Cc: dmaengine, linux-ntb
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/dma/ioat/dma.c | 136 ++++++++++++++++++++++++++++++++++++------
drivers/dma/ioat/registers.h | 2 +
2 files changed, 120 insertions(+), 18 deletions(-)
diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index bd09961..1bbc005 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -622,7 +622,8 @@ static void ioat_cleanup(struct ioatdma_chan *ioat_chan)
if (is_ioat_halted(*ioat_chan->completion)) {
u32 chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
- if (chanerr & IOAT_CHANERR_HANDLE_MASK) {
+ if (chanerr &
+ (IOAT_CHANERR_HANDLE_MASK | IOAT_CHANERR_RECOVER_MASK)) {
mod_timer(&ioat_chan->timer, jiffies + IDLE_TIMEOUT);
ioat_eh(ioat_chan);
}
@@ -652,6 +653,60 @@ static void ioat_restart_channel(struct ioatdma_chan *ioat_chan)
__ioat_restart_chan(ioat_chan);
}
+
+static void ioat_abort_descs(struct ioatdma_chan *ioat_chan)
+{
+ struct ioatdma_device *ioat_dma = ioat_chan->ioat_dma;
+ struct ioat_ring_ent *desc;
+ u16 active;
+ int idx = ioat_chan->tail, i;
+
+ /*
+ * We assume that the failed descriptor has been processed.
+ * Now we are just returning all the remaining submitted
+ * descriptors to abort.
+ */
+ active = ioat_ring_active(ioat_chan);
+
+ /* we skip the failed descriptor that tail points to */
+ for (i = 1; i < active; i++) {
+ struct dma_async_tx_descriptor *tx;
+
+ smp_read_barrier_depends();
+ prefetch(ioat_get_ring_ent(ioat_chan, idx + i + 1));
+ desc = ioat_get_ring_ent(ioat_chan, idx + i);
+ desc->txd.result |= ERR_DMA_ABORT;
+
+ tx = &desc->txd;
+ if (tx->cookie) {
+ dma_cookie_complete(tx);
+ dma_descriptor_unmap(tx);
+ if (tx->callback) {
+ tx->callback(tx->callback_param);
+ tx->callback = NULL;
+ }
+ }
+
+ /* skip extended descriptors */
+ if (desc_has_ext(desc)) {
+ WARN_ON(i + 1 >= active);
+ i++;
+ }
+
+ /* cleanup super extended descriptors */
+ if (desc->sed) {
+ ioat_free_sed(ioat_dma, desc->sed);
+ desc->sed = NULL;
+ }
+ }
+
+ smp_mb(); /* finish all descriptor reads before incrementing tail */
+ ioat_chan->tail = idx + i;
+
+ desc = ioat_get_ring_ent(ioat_chan, ioat_chan->tail);
+ ioat_chan->last_completion = *ioat_chan->completion = desc->txd.phys;
+}
+
static void ioat_eh(struct ioatdma_chan *ioat_chan)
{
struct pci_dev *pdev = to_pdev(ioat_chan);
@@ -662,6 +717,7 @@ static void ioat_eh(struct ioatdma_chan *ioat_chan)
u32 err_handled = 0;
u32 chanerr_int;
u32 chanerr;
+ bool abort = false;
/* cleanup so tail points to descriptor that caused the error */
if (ioat_cleanup_preamble(ioat_chan, &phys_complete))
@@ -697,30 +753,50 @@ static void ioat_eh(struct ioatdma_chan *ioat_chan)
break;
}
+ if (chanerr & IOAT_CHANERR_RECOVER_MASK) {
+ if (chanerr & IOAT_CHANERR_READ_DATA_ERR) {
+ desc->txd.result |= ERR_DMA_READ;
+ err_handled |= IOAT_CHANERR_READ_DATA_ERR;
+ } else if (chanerr & IOAT_CHANERR_WRITE_DATA_ERR) {
+ desc->txd.result |= ERR_DMA_WRITE;
+ err_handled |= IOAT_CHANERR_WRITE_DATA_ERR;
+ }
+
+ abort = true;
+ }
+
/* fault on unhandled error or spurious halt */
if (chanerr ^ err_handled || chanerr == 0) {
dev_err(to_dev(ioat_chan), "%s: fatal error (%x:%x)\n",
__func__, chanerr, err_handled);
BUG();
- } else { /* cleanup the faulty descriptor */
- tx = &desc->txd;
- if (tx->cookie) {
- dma_cookie_complete(tx);
- dma_descriptor_unmap(tx);
- if (tx->callback) {
- tx->callback(tx->callback_param);
- tx->callback = NULL;
- }
- }
}
- writel(chanerr, ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
- pci_write_config_dword(pdev, IOAT_PCI_CHANERR_INT_OFFSET, chanerr_int);
+ /* cleanup the faulty descriptor since we are continuing */
+ tx = &desc->txd;
+ if (tx->cookie) {
+ dma_cookie_complete(tx);
+ dma_descriptor_unmap(tx);
+ if (tx->callback) {
+ tx->callback(tx->callback_param);
+ tx->callback = NULL;
+ }
+ }
/* mark faulting descriptor as complete */
*ioat_chan->completion = desc->txd.phys;
spin_lock_bh(&ioat_chan->prep_lock);
+ /* we need abort all descriptors */
+ if (abort) {
+ ioat_abort_descs(ioat_chan);
+ /* clean up the channel, we could be in weird state */
+ ioat_reset_hw(ioat_chan);
+ }
+
+ writel(chanerr, ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
+ pci_write_config_dword(pdev, IOAT_PCI_CHANERR_INT_OFFSET, chanerr_int);
+
ioat_restart_channel(ioat_chan);
spin_unlock_bh(&ioat_chan->prep_lock);
}
@@ -753,10 +829,25 @@ void ioat_timer_event(unsigned long data)
chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
dev_err(to_dev(ioat_chan), "%s: Channel halted (%x)\n",
__func__, chanerr);
- if (test_bit(IOAT_RUN, &ioat_chan->state))
- BUG_ON(is_ioat_bug(chanerr));
- else /* we never got off the ground */
- return;
+ if (test_bit(IOAT_RUN, &ioat_chan->state)) {
+ spin_lock_bh(&ioat_chan->cleanup_lock);
+ spin_lock_bh(&ioat_chan->prep_lock);
+ set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
+ spin_unlock_bh(&ioat_chan->prep_lock);
+
+ ioat_abort_descs(ioat_chan);
+ dev_warn(to_dev(ioat_chan), "Reset channel...\n");
+ ioat_reset_hw(ioat_chan);
+ dev_warn(to_dev(ioat_chan), "Restart channel...\n");
+ ioat_restart_channel(ioat_chan);
+
+ spin_lock_bh(&ioat_chan->prep_lock);
+ clear_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
+ spin_unlock_bh(&ioat_chan->prep_lock);
+ spin_unlock_bh(&ioat_chan->cleanup_lock);
+ }
+
+ return;
}
spin_lock_bh(&ioat_chan->cleanup_lock);
@@ -780,14 +871,23 @@ void ioat_timer_event(unsigned long data)
u32 chanerr;
chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
- dev_warn(to_dev(ioat_chan), "Restarting channel...\n");
dev_warn(to_dev(ioat_chan), "CHANSTS: %#Lx CHANERR: %#x\n",
status, chanerr);
dev_warn(to_dev(ioat_chan), "Active descriptors: %d\n",
ioat_ring_active(ioat_chan));
spin_lock_bh(&ioat_chan->prep_lock);
+ set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
+ spin_unlock_bh(&ioat_chan->prep_lock);
+
+ ioat_abort_descs(ioat_chan);
+ dev_warn(to_dev(ioat_chan), "Resetting channel...\n");
+ ioat_reset_hw(ioat_chan);
+ dev_warn(to_dev(ioat_chan), "Restarting channel...\n");
ioat_restart_channel(ioat_chan);
+
+ spin_lock_bh(&ioat_chan->prep_lock);
+ clear_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
spin_unlock_bh(&ioat_chan->prep_lock);
spin_unlock_bh(&ioat_chan->cleanup_lock);
return;
diff --git a/drivers/dma/ioat/registers.h b/drivers/dma/ioat/registers.h
index 4994a36..faf20fa 100644
--- a/drivers/dma/ioat/registers.h
+++ b/drivers/dma/ioat/registers.h
@@ -233,6 +233,8 @@
#define IOAT_CHANERR_DESCRIPTOR_COUNT_ERR 0x40000
#define IOAT_CHANERR_HANDLE_MASK (IOAT_CHANERR_XOR_P_OR_CRC_ERR | IOAT_CHANERR_XOR_Q_ERR)
+#define IOAT_CHANERR_RECOVER_MASK (IOAT_CHANERR_READ_DATA_ERR | \
+ IOAT_CHANERR_WRITE_DATA_ERR)
#define IOAT_CHANERR_MASK_OFFSET 0x2C /* 32-bit Channel Error Register */
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/5] dmaengine: IOATDMA: Add error handling to driver
2016-06-27 23:09 ` [PATCH 2/5] dmaengine: IOATDMA: Add error handling to driver Dave Jiang
@ 2016-06-28 1:08 ` Allen Hubbe
2016-06-28 16:11 ` Jiang, Dave
0 siblings, 1 reply; 10+ messages in thread
From: Allen Hubbe @ 2016-06-28 1:08 UTC (permalink / raw)
To: Dave Jiang; +Cc: vinod.koul, Allen Hubbe, jdmason, dmaengine, linux-ntb
On Mon, Jun 27, 2016 at 7:09 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/dma/ioat/dma.c | 136 ++++++++++++++++++++++++++++++++++++------
> drivers/dma/ioat/registers.h | 2 +
> 2 files changed, 120 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
> index bd09961..1bbc005 100644
> --- a/drivers/dma/ioat/dma.c
> +++ b/drivers/dma/ioat/dma.c
> @@ -622,7 +622,8 @@ static void ioat_cleanup(struct ioatdma_chan *ioat_chan)
> if (is_ioat_halted(*ioat_chan->completion)) {
> u32 chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
>
> - if (chanerr & IOAT_CHANERR_HANDLE_MASK) {
> + if (chanerr &
> + (IOAT_CHANERR_HANDLE_MASK | IOAT_CHANERR_RECOVER_MASK)) {
> mod_timer(&ioat_chan->timer, jiffies + IDLE_TIMEOUT);
> ioat_eh(ioat_chan);
> }
> @@ -652,6 +653,60 @@ static void ioat_restart_channel(struct ioatdma_chan *ioat_chan)
> __ioat_restart_chan(ioat_chan);
> }
>
> +
> +static void ioat_abort_descs(struct ioatdma_chan *ioat_chan)
> +{
> + struct ioatdma_device *ioat_dma = ioat_chan->ioat_dma;
> + struct ioat_ring_ent *desc;
> + u16 active;
> + int idx = ioat_chan->tail, i;
> +
> + /*
> + * We assume that the failed descriptor has been processed.
> + * Now we are just returning all the remaining submitted
> + * descriptors to abort.
> + */
> + active = ioat_ring_active(ioat_chan);
> +
> + /* we skip the failed descriptor that tail points to */
> + for (i = 1; i < active; i++) {
> + struct dma_async_tx_descriptor *tx;
> +
> + smp_read_barrier_depends();
> + prefetch(ioat_get_ring_ent(ioat_chan, idx + i + 1));
> + desc = ioat_get_ring_ent(ioat_chan, idx + i);
> + desc->txd.result |= ERR_DMA_ABORT;
See below, txd.result doesn't seem to be a bit mask.
> +
> + tx = &desc->txd;
> + if (tx->cookie) {
> + dma_cookie_complete(tx);
> + dma_descriptor_unmap(tx);
> + if (tx->callback) {
> + tx->callback(tx->callback_param);
> + tx->callback = NULL;
> + }
> + }
> +
> + /* skip extended descriptors */
> + if (desc_has_ext(desc)) {
> + WARN_ON(i + 1 >= active);
> + i++;
Is it always true that extended descriptors (note: descriptors is
plural) means "exactly one?" Otherwise, ++ aught to be += how how
many. I didn't notice anything in dma_prep.c that would use more than
one extra.
> + }
> +
> + /* cleanup super extended descriptors */
> + if (desc->sed) {
> + ioat_free_sed(ioat_dma, desc->sed);
> + desc->sed = NULL;
> + }
> + }
> +
> + smp_mb(); /* finish all descriptor reads before incrementing tail */
> + ioat_chan->tail = idx + i;
Would it also be correct to say `idx + active` here? If so, that
might be more clear.
> +
> + desc = ioat_get_ring_ent(ioat_chan, ioat_chan->tail);
> + ioat_chan->last_completion = *ioat_chan->completion = desc->txd.phys;
> +}
> +
> static void ioat_eh(struct ioatdma_chan *ioat_chan)
> {
> struct pci_dev *pdev = to_pdev(ioat_chan);
> @@ -662,6 +717,7 @@ static void ioat_eh(struct ioatdma_chan *ioat_chan)
> u32 err_handled = 0;
> u32 chanerr_int;
> u32 chanerr;
> + bool abort = false;
>
> /* cleanup so tail points to descriptor that caused the error */
> if (ioat_cleanup_preamble(ioat_chan, &phys_complete))
> @@ -697,30 +753,50 @@ static void ioat_eh(struct ioatdma_chan *ioat_chan)
> break;
> }
>
> + if (chanerr & IOAT_CHANERR_RECOVER_MASK) {
> + if (chanerr & IOAT_CHANERR_READ_DATA_ERR) {
> + desc->txd.result |= ERR_DMA_READ;
In [PATCH 1/5] ERR_DMA_XYZ values do not look appropriate for use as a bit mask.
+enum err_result_flags {
+ ERR_DMA_NONE = 0,
+ ERR_DMA_READ,
+ ERR_DMA_WRITE,
+ ERR_DMA_ABORT,
+};
And in [PATCH 4/5] and [PATCH 5/5] txd.result is not used as a bit mask.
+ switch (txd->result) {
+ case ERR_DMA_READ:
+ case ERR_DMA_WRITE:
+ entry->errors++;
+ case ERR_DMA_ABORT:
+ flags = DESC_ABORT_FLAG;
+ break;
+ case ERR_DMA_NONE:
+ default:
+ break;
+ }
> + err_handled |= IOAT_CHANERR_READ_DATA_ERR;
> + } else if (chanerr & IOAT_CHANERR_WRITE_DATA_ERR) {
> + desc->txd.result |= ERR_DMA_WRITE;
> + err_handled |= IOAT_CHANERR_WRITE_DATA_ERR;
> + }
> +
> + abort = true;
> + }
> +
> /* fault on unhandled error or spurious halt */
> if (chanerr ^ err_handled || chanerr == 0) {
Not part of your change, but chanerr == 0 is confusing. Doesn't that
mean "no errors?" No errors would make it hard to justify BUG() that
follows.
> dev_err(to_dev(ioat_chan), "%s: fatal error (%x:%x)\n",
> __func__, chanerr, err_handled);
> BUG();
> - } else { /* cleanup the faulty descriptor */
> - tx = &desc->txd;
> - if (tx->cookie) {
> - dma_cookie_complete(tx);
> - dma_descriptor_unmap(tx);
> - if (tx->callback) {
> - tx->callback(tx->callback_param);
> - tx->callback = NULL;
> - }
> - }
> }
>
> - writel(chanerr, ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
> - pci_write_config_dword(pdev, IOAT_PCI_CHANERR_INT_OFFSET, chanerr_int);
> + /* cleanup the faulty descriptor since we are continuing */
> + tx = &desc->txd;
> + if (tx->cookie) {
> + dma_cookie_complete(tx);
> + dma_descriptor_unmap(tx);
> + if (tx->callback) {
> + tx->callback(tx->callback_param);
> + tx->callback = NULL;
> + }
> + }
>
> /* mark faulting descriptor as complete */
> *ioat_chan->completion = desc->txd.phys;
>
> spin_lock_bh(&ioat_chan->prep_lock);
> + /* we need abort all descriptors */
> + if (abort) {
> + ioat_abort_descs(ioat_chan);
> + /* clean up the channel, we could be in weird state */
> + ioat_reset_hw(ioat_chan);
> + }
> +
> + writel(chanerr, ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
> + pci_write_config_dword(pdev, IOAT_PCI_CHANERR_INT_OFFSET, chanerr_int);
> +
> ioat_restart_channel(ioat_chan);
> spin_unlock_bh(&ioat_chan->prep_lock);
> }
> @@ -753,10 +829,25 @@ void ioat_timer_event(unsigned long data)
> chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
> dev_err(to_dev(ioat_chan), "%s: Channel halted (%x)\n",
> __func__, chanerr);
> - if (test_bit(IOAT_RUN, &ioat_chan->state))
> - BUG_ON(is_ioat_bug(chanerr));
> - else /* we never got off the ground */
> - return;
> + if (test_bit(IOAT_RUN, &ioat_chan->state)) {
> + spin_lock_bh(&ioat_chan->cleanup_lock);
> + spin_lock_bh(&ioat_chan->prep_lock);
> + set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> + spin_unlock_bh(&ioat_chan->prep_lock);
> +
> + ioat_abort_descs(ioat_chan);
> + dev_warn(to_dev(ioat_chan), "Reset channel...\n");
> + ioat_reset_hw(ioat_chan);
> + dev_warn(to_dev(ioat_chan), "Restart channel...\n");
> + ioat_restart_channel(ioat_chan);
> +
> + spin_lock_bh(&ioat_chan->prep_lock);
> + clear_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> + spin_unlock_bh(&ioat_chan->prep_lock);
> + spin_unlock_bh(&ioat_chan->cleanup_lock);
> + }
> +
> + return;
> }
>
> spin_lock_bh(&ioat_chan->cleanup_lock);
> @@ -780,14 +871,23 @@ void ioat_timer_event(unsigned long data)
> u32 chanerr;
>
> chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
> - dev_warn(to_dev(ioat_chan), "Restarting channel...\n");
> dev_warn(to_dev(ioat_chan), "CHANSTS: %#Lx CHANERR: %#x\n",
> status, chanerr);
> dev_warn(to_dev(ioat_chan), "Active descriptors: %d\n",
> ioat_ring_active(ioat_chan));
>
> spin_lock_bh(&ioat_chan->prep_lock);
> + set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> + spin_unlock_bh(&ioat_chan->prep_lock);
> +
> + ioat_abort_descs(ioat_chan);
> + dev_warn(to_dev(ioat_chan), "Resetting channel...\n");
> + ioat_reset_hw(ioat_chan);
> + dev_warn(to_dev(ioat_chan), "Restarting channel...\n");
> ioat_restart_channel(ioat_chan);
> +
> + spin_lock_bh(&ioat_chan->prep_lock);
> + clear_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> spin_unlock_bh(&ioat_chan->prep_lock);
> spin_unlock_bh(&ioat_chan->cleanup_lock);
> return;
> diff --git a/drivers/dma/ioat/registers.h b/drivers/dma/ioat/registers.h
> index 4994a36..faf20fa 100644
> --- a/drivers/dma/ioat/registers.h
> +++ b/drivers/dma/ioat/registers.h
> @@ -233,6 +233,8 @@
> #define IOAT_CHANERR_DESCRIPTOR_COUNT_ERR 0x40000
>
> #define IOAT_CHANERR_HANDLE_MASK (IOAT_CHANERR_XOR_P_OR_CRC_ERR | IOAT_CHANERR_XOR_Q_ERR)
> +#define IOAT_CHANERR_RECOVER_MASK (IOAT_CHANERR_READ_DATA_ERR | \
> + IOAT_CHANERR_WRITE_DATA_ERR)
>
> #define IOAT_CHANERR_MASK_OFFSET 0x2C /* 32-bit Channel Error Register */
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/5] dmaengine: IOATDMA: Add error handling to driver
2016-06-28 1:08 ` Allen Hubbe
@ 2016-06-28 16:11 ` Jiang, Dave
0 siblings, 0 replies; 10+ messages in thread
From: Jiang, Dave @ 2016-06-28 16:11 UTC (permalink / raw)
To: allenbh@gmail.com
Cc: allen.hubbe@emc.com, dmaengine@vger.kernel.org, Williams, Dan J,
jdmason@kudzu.us, Koul, Vinod, linux-ntb@googlegroups.com
On Tue, 2016-06-28 at 01:08 +0000, Allen Hubbe wrote:
> On Mon, Jun 27, 2016 at 7:09 PM, Dave Jiang <dave.jiang@intel.com>
> wrote:
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> > drivers/dma/ioat/dma.c | 136
> > ++++++++++++++++++++++++++++++++++++------
> > drivers/dma/ioat/registers.h | 2 +
> > 2 files changed, 120 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
> > index bd09961..1bbc005 100644
> > --- a/drivers/dma/ioat/dma.c
> > +++ b/drivers/dma/ioat/dma.c
> > @@ -622,7 +622,8 @@ static void ioat_cleanup(struct ioatdma_chan
> > *ioat_chan)
> > if (is_ioat_halted(*ioat_chan->completion)) {
> > u32 chanerr = readl(ioat_chan->reg_base +
> > IOAT_CHANERR_OFFSET);
> >
> > - if (chanerr & IOAT_CHANERR_HANDLE_MASK) {
> > + if (chanerr &
> > + (IOAT_CHANERR_HANDLE_MASK |
> > IOAT_CHANERR_RECOVER_MASK)) {
> > mod_timer(&ioat_chan->timer, jiffies +
> > IDLE_TIMEOUT);
> > ioat_eh(ioat_chan);
> > }
> > @@ -652,6 +653,60 @@ static void ioat_restart_channel(struct
> > ioatdma_chan *ioat_chan)
> > __ioat_restart_chan(ioat_chan);
> > }
> >
> > +
> > +static void ioat_abort_descs(struct ioatdma_chan *ioat_chan)
> > +{
> > + struct ioatdma_device *ioat_dma = ioat_chan->ioat_dma;
> > + struct ioat_ring_ent *desc;
> > + u16 active;
> > + int idx = ioat_chan->tail, i;
> > +
> > + /*
> > + * We assume that the failed descriptor has been processed.
> > + * Now we are just returning all the remaining submitted
> > + * descriptors to abort.
> > + */
> > + active = ioat_ring_active(ioat_chan);
> > +
> > + /* we skip the failed descriptor that tail points to */
> > + for (i = 1; i < active; i++) {
> > + struct dma_async_tx_descriptor *tx;
> > +
> > + smp_read_barrier_depends();
> > + prefetch(ioat_get_ring_ent(ioat_chan, idx + i +
> > 1));
> > + desc = ioat_get_ring_ent(ioat_chan, idx + i);
> > + desc->txd.result |= ERR_DMA_ABORT;
>
> See below, txd.result doesn't seem to be a bit mask.
I'll fix that. It shouldn't be a bit mask.
>
> > +
> > + tx = &desc->txd;
> > + if (tx->cookie) {
> > + dma_cookie_complete(tx);
> > + dma_descriptor_unmap(tx);
> > + if (tx->callback) {
> > + tx->callback(tx->callback_param);
> > + tx->callback = NULL;
> > + }
> > + }
> > +
> > + /* skip extended descriptors */
> > + if (desc_has_ext(desc)) {
> > + WARN_ON(i + 1 >= active);
> > + i++;
>
> Is it always true that extended descriptors (note: descriptors is
> plural) means "exactly one?" Otherwise, ++ aught to be += how how
> many. I didn't notice anything in dma_prep.c that would use more
> than
> one extra.
There can only be 1 extended descriptor always.
>
> > + }
> > +
> > + /* cleanup super extended descriptors */
> > + if (desc->sed) {
> > + ioat_free_sed(ioat_dma, desc->sed);
> > + desc->sed = NULL;
> > + }
> > + }
> > +
> > + smp_mb(); /* finish all descriptor reads before
> > incrementing tail */
> > + ioat_chan->tail = idx + i;
>
> Would it also be correct to say `idx + active` here? If so, that
> might be more clear.
ok
>
> > +
> > + desc = ioat_get_ring_ent(ioat_chan, ioat_chan->tail);
> > + ioat_chan->last_completion = *ioat_chan->completion = desc-
> > >txd.phys;
> > +}
> > +
> > static void ioat_eh(struct ioatdma_chan *ioat_chan)
> > {
> > struct pci_dev *pdev = to_pdev(ioat_chan);
> > @@ -662,6 +717,7 @@ static void ioat_eh(struct ioatdma_chan
> > *ioat_chan)
> > u32 err_handled = 0;
> > u32 chanerr_int;
> > u32 chanerr;
> > + bool abort = false;
> >
> > /* cleanup so tail points to descriptor that caused the
> > error */
> > if (ioat_cleanup_preamble(ioat_chan, &phys_complete))
> > @@ -697,30 +753,50 @@ static void ioat_eh(struct ioatdma_chan
> > *ioat_chan)
> > break;
> > }
> >
> > + if (chanerr & IOAT_CHANERR_RECOVER_MASK) {
> > + if (chanerr & IOAT_CHANERR_READ_DATA_ERR) {
> > + desc->txd.result |= ERR_DMA_READ;
>
> In [PATCH 1/5] ERR_DMA_XYZ values do not look appropriate for use as
> a bit mask.
>
> +enum err_result_flags {
> + ERR_DMA_NONE = 0,
> + ERR_DMA_READ,
> + ERR_DMA_WRITE,
> + ERR_DMA_ABORT,
> +};
>
> And in [PATCH 4/5] and [PATCH 5/5] txd.result is not used as a bit
> mask.
>
> + switch (txd->result) {
> + case ERR_DMA_READ:
> + case ERR_DMA_WRITE:
> + entry->errors++;
> + case ERR_DMA_ABORT:
> + flags = DESC_ABORT_FLAG;
> + break;
> + case ERR_DMA_NONE:
> + default:
> + break;
> + }
>
> > + err_handled |= IOAT_CHANERR_READ_DATA_ERR;
> > + } else if (chanerr & IOAT_CHANERR_WRITE_DATA_ERR) {
> > + desc->txd.result |= ERR_DMA_WRITE;
> > + err_handled |= IOAT_CHANERR_WRITE_DATA_ERR;
> > + }
> > +
> > + abort = true;
> > + }
> > +
> > /* fault on unhandled error or spurious halt */
> > if (chanerr ^ err_handled || chanerr == 0) {
>
> Not part of your change, but chanerr == 0 is confusing. Doesn't that
> mean "no errors?" No errors would make it hard to justify BUG() that
> follows.
Legacy code. This is the beginning of me cleaning out all the BUG() in
the driver and put in proper error handling.
>
> > dev_err(to_dev(ioat_chan), "%s: fatal error
> > (%x:%x)\n",
> > __func__, chanerr, err_handled);
> > BUG();
> > - } else { /* cleanup the faulty descriptor */
> > - tx = &desc->txd;
> > - if (tx->cookie) {
> > - dma_cookie_complete(tx);
> > - dma_descriptor_unmap(tx);
> > - if (tx->callback) {
> > - tx->callback(tx->callback_param);
> > - tx->callback = NULL;
> > - }
> > - }
> > }
> >
> > - writel(chanerr, ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
> > - pci_write_config_dword(pdev, IOAT_PCI_CHANERR_INT_OFFSET,
> > chanerr_int);
> > + /* cleanup the faulty descriptor since we are continuing */
> > + tx = &desc->txd;
> > + if (tx->cookie) {
> > + dma_cookie_complete(tx);
> > + dma_descriptor_unmap(tx);
> > + if (tx->callback) {
> > + tx->callback(tx->callback_param);
> > + tx->callback = NULL;
> > + }
> > + }
> >
> > /* mark faulting descriptor as complete */
> > *ioat_chan->completion = desc->txd.phys;
> >
> > spin_lock_bh(&ioat_chan->prep_lock);
> > + /* we need abort all descriptors */
> > + if (abort) {
> > + ioat_abort_descs(ioat_chan);
> > + /* clean up the channel, we could be in weird state
> > */
> > + ioat_reset_hw(ioat_chan);
> > + }
> > +
> > + writel(chanerr, ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
> > + pci_write_config_dword(pdev, IOAT_PCI_CHANERR_INT_OFFSET,
> > chanerr_int);
> > +
> > ioat_restart_channel(ioat_chan);
> > spin_unlock_bh(&ioat_chan->prep_lock);
> > }
> > @@ -753,10 +829,25 @@ void ioat_timer_event(unsigned long data)
> > chanerr = readl(ioat_chan->reg_base +
> > IOAT_CHANERR_OFFSET);
> > dev_err(to_dev(ioat_chan), "%s: Channel halted
> > (%x)\n",
> > __func__, chanerr);
> > - if (test_bit(IOAT_RUN, &ioat_chan->state))
> > - BUG_ON(is_ioat_bug(chanerr));
> > - else /* we never got off the ground */
> > - return;
> > + if (test_bit(IOAT_RUN, &ioat_chan->state)) {
> > + spin_lock_bh(&ioat_chan->cleanup_lock);
> > + spin_lock_bh(&ioat_chan->prep_lock);
> > + set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> > + spin_unlock_bh(&ioat_chan->prep_lock);
> > +
> > + ioat_abort_descs(ioat_chan);
> > + dev_warn(to_dev(ioat_chan), "Reset
> > channel...\n");
> > + ioat_reset_hw(ioat_chan);
> > + dev_warn(to_dev(ioat_chan), "Restart
> > channel...\n");
> > + ioat_restart_channel(ioat_chan);
> > +
> > + spin_lock_bh(&ioat_chan->prep_lock);
> > + clear_bit(IOAT_CHAN_DOWN, &ioat_chan-
> > >state);
> > + spin_unlock_bh(&ioat_chan->prep_lock);
> > + spin_unlock_bh(&ioat_chan->cleanup_lock);
> > + }
> > +
> > + return;
> > }
> >
> > spin_lock_bh(&ioat_chan->cleanup_lock);
> > @@ -780,14 +871,23 @@ void ioat_timer_event(unsigned long data)
> > u32 chanerr;
> >
> > chanerr = readl(ioat_chan->reg_base +
> > IOAT_CHANERR_OFFSET);
> > - dev_warn(to_dev(ioat_chan), "Restarting
> > channel...\n");
> > dev_warn(to_dev(ioat_chan), "CHANSTS: %#Lx CHANERR:
> > %#x\n",
> > status, chanerr);
> > dev_warn(to_dev(ioat_chan), "Active descriptors:
> > %d\n",
> > ioat_ring_active(ioat_chan));
> >
> > spin_lock_bh(&ioat_chan->prep_lock);
> > + set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> > + spin_unlock_bh(&ioat_chan->prep_lock);
> > +
> > + ioat_abort_descs(ioat_chan);
> > + dev_warn(to_dev(ioat_chan), "Resetting
> > channel...\n");
> > + ioat_reset_hw(ioat_chan);
> > + dev_warn(to_dev(ioat_chan), "Restarting
> > channel...\n");
> > ioat_restart_channel(ioat_chan);
> > +
> > + spin_lock_bh(&ioat_chan->prep_lock);
> > + clear_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> > spin_unlock_bh(&ioat_chan->prep_lock);
> > spin_unlock_bh(&ioat_chan->cleanup_lock);
> > return;
> > diff --git a/drivers/dma/ioat/registers.h
> > b/drivers/dma/ioat/registers.h
> > index 4994a36..faf20fa 100644
> > --- a/drivers/dma/ioat/registers.h
> > +++ b/drivers/dma/ioat/registers.h
> > @@ -233,6 +233,8 @@
> > #define IOAT_CHANERR_DESCRIPTOR_COUNT_ERR 0x40000
> >
> > #define IOAT_CHANERR_HANDLE_MASK (IOAT_CHANERR_XOR_P_OR_CRC_ERR |
> > IOAT_CHANERR_XOR_Q_ERR)
> > +#define IOAT_CHANERR_RECOVER_MASK (IOAT_CHANERR_READ_DATA_ERR | \
> > + IOAT_CHANERR_WRITE_DATA_ERR)
> >
> > #define IOAT_CHANERR_MASK_OFFSET 0x2C /* 32-bit
> > Channel Error Register */
> >
> >
>
> --
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/5] dmaengine: ioatdma: add error strings to chanerr output
2016-06-27 23:09 [PATCH 0/5] dmaengine: ioatdma: DMA error reporting Dave Jiang
2016-06-27 23:09 ` [PATCH 1/5] dmaengine: Adding error handling flag Dave Jiang
2016-06-27 23:09 ` [PATCH 2/5] dmaengine: IOATDMA: Add error handling to driver Dave Jiang
@ 2016-06-27 23:09 ` Dave Jiang
2016-06-27 23:09 ` [PATCH 4/5] ntb: add DMA error handling for TX DMA Dave Jiang
2016-06-27 23:10 ` [PATCH 5/5] ntb: add DMA error handling for RX DMA Dave Jiang
4 siblings, 0 replies; 10+ messages in thread
From: Dave Jiang @ 2016-06-27 23:09 UTC (permalink / raw)
To: vinod.koul, allen.hubbe, jdmason; +Cc: dmaengine, linux-ntb
Provide a mechanism to translate CHANERR bits to English strings in order
to allow user to report more concise errors.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/dma/ioat/dma.c | 65 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index 1bbc005..68e0cad 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -38,8 +38,54 @@
#include "../dmaengine.h"
+static char *chanerr_str[] = {
+ "DMA Transfer Destination Address Error",
+ "Next Descriptor Address Error",
+ "Descriptor Error",
+ "Chan Address Value Error",
+ "CHANCMD Error",
+ "Chipset Uncorrectable Data Integrity Error",
+ "DMA Uncorrectable Data Integrity Error",
+ "Read Data Error",
+ "Write Data Error",
+ "Descriptor Control Error",
+ "Descriptor Transfer Size Error",
+ "Completion Address Error",
+ "Interrupt Configuration Error",
+ "Super extended descriptor Address Error",
+ "Unaffiliated Error",
+ "CRC or XOR P Error",
+ "XOR Q Error",
+ "Descriptor Count Error",
+ "DIF All F detect Error",
+ "Guard Tag verification Error",
+ "Application Tag verification Error",
+ "Reference Tag verification Error",
+ "Bundle Bit Error",
+ "Result DIF All F detect Error",
+ "Result Guard Tag verification Error",
+ "Result Application Tag verification Error",
+ "Result Reference Tag verification Error",
+ NULL
+};
+
static void ioat_eh(struct ioatdma_chan *ioat_chan);
+static void ioat_print_chanerrs(struct ioatdma_chan *ioat_chan, u32 chanerr)
+{
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ if ((chanerr >> i) & 1) {
+ if (chanerr_str[i]) {
+ dev_err(to_dev(ioat_chan), "Err(%d): %s\n",
+ i, chanerr_str[i]);
+ } else
+ break;
+ }
+ }
+}
+
/**
* ioat_dma_do_interrupt - handler used for single vector interrupt mode
* @irq: interrupt id
@@ -769,6 +815,11 @@ static void ioat_eh(struct ioatdma_chan *ioat_chan)
if (chanerr ^ err_handled || chanerr == 0) {
dev_err(to_dev(ioat_chan), "%s: fatal error (%x:%x)\n",
__func__, chanerr, err_handled);
+ dev_err(to_dev(ioat_chan), "Errors handled:\n");
+ ioat_print_chanerrs(ioat_chan, err_handled);
+ dev_err(to_dev(ioat_chan), "Errors not handled:\n");
+ ioat_print_chanerrs(ioat_chan, (chanerr & ~err_handled));
+
BUG();
}
@@ -829,6 +880,9 @@ void ioat_timer_event(unsigned long data)
chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
dev_err(to_dev(ioat_chan), "%s: Channel halted (%x)\n",
__func__, chanerr);
+ dev_err(to_dev(ioat_chan), "Errors:\n");
+ ioat_print_chanerrs(ioat_chan, chanerr);
+
if (test_bit(IOAT_RUN, &ioat_chan->state)) {
spin_lock_bh(&ioat_chan->cleanup_lock);
spin_lock_bh(&ioat_chan->prep_lock);
@@ -871,10 +925,13 @@ void ioat_timer_event(unsigned long data)
u32 chanerr;
chanerr = readl(ioat_chan->reg_base + IOAT_CHANERR_OFFSET);
- dev_warn(to_dev(ioat_chan), "CHANSTS: %#Lx CHANERR: %#x\n",
- status, chanerr);
- dev_warn(to_dev(ioat_chan), "Active descriptors: %d\n",
- ioat_ring_active(ioat_chan));
+ dev_err(to_dev(ioat_chan), "CHANSTS: %#Lx CHANERR: %#x\n",
+ status, chanerr);
+ dev_err(to_dev(ioat_chan), "Errors:\n");
+ ioat_print_chanerrs(ioat_chan, chanerr);
+
+ dev_dbg(to_dev(ioat_chan), "Active descriptors: %d\n",
+ ioat_ring_active(ioat_chan));
spin_lock_bh(&ioat_chan->prep_lock);
set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/5] ntb: add DMA error handling for TX DMA
2016-06-27 23:09 [PATCH 0/5] dmaengine: ioatdma: DMA error reporting Dave Jiang
` (2 preceding siblings ...)
2016-06-27 23:09 ` [PATCH 3/5] dmaengine: ioatdma: add error strings to chanerr output Dave Jiang
@ 2016-06-27 23:09 ` Dave Jiang
2016-06-28 1:53 ` Allen Hubbe
2016-06-27 23:10 ` [PATCH 5/5] ntb: add DMA error handling for RX DMA Dave Jiang
4 siblings, 1 reply; 10+ messages in thread
From: Dave Jiang @ 2016-06-27 23:09 UTC (permalink / raw)
To: vinod.koul, allen.hubbe, jdmason; +Cc: dmaengine, linux-ntb
Adding support on the tx DMA path to allow recovery of errors when DMA
responds with error status and abort all the subsequent ops.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/ntb/ntb_transport.c | 123 +++++++++++++++++++++++++++++++------------
1 file changed, 89 insertions(+), 34 deletions(-)
diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 2ef9d913..55a5ae0 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -102,6 +102,10 @@ struct ntb_queue_entry {
void *buf;
unsigned int len;
unsigned int flags;
+ struct dma_async_tx_descriptor *txd;
+ int retries;
+ int errors;
+ unsigned int tx_index;
struct ntb_transport_qp *qp;
union {
@@ -258,6 +262,9 @@ enum {
static void ntb_transport_rxc_db(unsigned long data);
static const struct ntb_ctx_ops ntb_transport_ops;
static struct ntb_client ntb_transport_client;
+static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
+ struct ntb_queue_entry *entry);
+static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void __iomem *offset);
static int ntb_transport_bus_match(struct device *dev,
struct device_driver *drv)
@@ -1444,21 +1451,46 @@ static void ntb_tx_copy_callback(void *data)
struct ntb_queue_entry *entry = data;
struct ntb_transport_qp *qp = entry->qp;
struct ntb_payload_header __iomem *hdr = entry->tx_hdr;
+ struct dma_async_tx_descriptor *txd;
+ unsigned int len;
+ bool abort = false;
+
+ txd = entry->txd;
+
+ /* we need to check DMA results if we are using DMA */
+ if (txd) {
+ switch (txd->result) {
+ case ERR_DMA_READ:
+ case ERR_DMA_WRITE:
+ entry->errors++;
+ case ERR_DMA_ABORT:
+ abort = true;
+ goto handle_tx;
+ case ERR_DMA_NONE:
+ default:
+ break;
+ }
+ }
iowrite32(entry->flags | DESC_DONE_FLAG, &hdr->flags);
ntb_peer_db_set(qp->ndev, BIT_ULL(qp->qp_num));
+handle_tx:
+
/* The entry length can only be zero if the packet is intended to be a
* "link down" or similar. Since no payload is being sent in these
* cases, there is nothing to add to the completion queue.
*/
if (entry->len > 0) {
- qp->tx_bytes += entry->len;
+ if (!abort) {
+ qp->tx_bytes += entry->len;
+ len = entry->len;
+ } else
+ len = 0;
if (qp->tx_handler)
- qp->tx_handler(qp, qp->cb_data, entry->cb_data,
- entry->len);
+ qp->tx_handler(qp, qp->cb_data, entry->cb_data, len);
}
ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry, &qp->tx_free_q);
@@ -1482,37 +1514,21 @@ static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void __iomem *offset)
ntb_tx_copy_callback(entry);
}
-static void ntb_async_tx(struct ntb_transport_qp *qp,
- struct ntb_queue_entry *entry)
+static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
+ struct ntb_queue_entry *entry)
{
- struct ntb_payload_header __iomem *hdr;
- struct dma_async_tx_descriptor *txd;
struct dma_chan *chan = qp->tx_dma_chan;
struct dma_device *device;
+ size_t len = entry->len;
+ void *buf = entry->buf;
size_t dest_off, buff_off;
struct dmaengine_unmap_data *unmap;
dma_addr_t dest;
dma_cookie_t cookie;
- void __iomem *offset;
- size_t len = entry->len;
- void *buf = entry->buf;
int retries = 0;
- offset = qp->tx_mw + qp->tx_max_frame * qp->tx_index;
- hdr = offset + qp->tx_max_frame - sizeof(struct ntb_payload_header);
- entry->tx_hdr = hdr;
-
- iowrite32(entry->len, &hdr->len);
- iowrite32((u32)qp->tx_pkts, &hdr->ver);
-
- if (!chan)
- goto err;
-
- if (len < copy_bytes)
- goto err;
-
device = chan->device;
- dest = qp->tx_mw_phys + qp->tx_max_frame * qp->tx_index;
+ dest = qp->tx_mw_phys + qp->tx_max_frame * entry->tx_index;
buff_off = (size_t)buf & ~PAGE_MASK;
dest_off = (size_t)dest & ~PAGE_MASK;
@@ -1532,39 +1548,74 @@ static void ntb_async_tx(struct ntb_transport_qp *qp,
unmap->to_cnt = 1;
for (retries = 0; retries < DMA_RETRIES; retries++) {
- txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0],
- len, DMA_PREP_INTERRUPT);
- if (txd)
+ entry->txd = device->device_prep_dma_memcpy(chan, dest,
+ unmap->addr[0], len,
+ DMA_PREP_INTERRUPT);
+ if (entry->txd)
break;
set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(DMA_OUT_RESOURCE_TO);
}
- if (!txd) {
+ if (!entry->txd) {
qp->dma_tx_prep_err++;
goto err_get_unmap;
}
- txd->callback = ntb_tx_copy_callback;
- txd->callback_param = entry;
- dma_set_unmap(txd, unmap);
+ entry->txd->callback = ntb_tx_copy_callback;
+ entry->txd->callback_param = entry;
+ dma_set_unmap(entry->txd, unmap);
- cookie = dmaengine_submit(txd);
+ cookie = dmaengine_submit(entry->txd);
if (dma_submit_error(cookie))
goto err_set_unmap;
dmaengine_unmap_put(unmap);
dma_async_issue_pending(chan);
- qp->tx_async++;
- return;
+ return 0;
err_set_unmap:
dmaengine_unmap_put(unmap);
err_get_unmap:
dmaengine_unmap_put(unmap);
err:
+ return -ENXIO;
+}
+
+static void ntb_async_tx(struct ntb_transport_qp *qp,
+ struct ntb_queue_entry *entry)
+{
+ struct ntb_payload_header __iomem *hdr;
+ struct dma_chan *chan = qp->tx_dma_chan;
+ void __iomem *offset;
+ int res;
+
+ entry->tx_index = qp->tx_index;
+ offset = qp->tx_mw + qp->tx_max_frame * entry->tx_index;
+ hdr = offset + qp->tx_max_frame - sizeof(struct ntb_payload_header);
+ entry->tx_hdr = hdr;
+
+ iowrite32(entry->len, &hdr->len);
+ iowrite32((u32)qp->tx_pkts, &hdr->ver);
+
+ if (!chan)
+ goto err;
+
+ if (entry->len < copy_bytes)
+ goto err;
+
+ res = ntb_async_tx_submit(qp, entry);
+ if (res < 0)
+ goto err;
+
+ if (!entry->retries)
+ qp->tx_async++;
+
+ return;
+
+err:
ntb_memcpy_tx(entry, offset);
qp->tx_memcpy++;
}
@@ -1940,6 +1991,10 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
entry->buf = data;
entry->len = len;
entry->flags = 0;
+ entry->errors = 0;
+ entry->retries = 0;
+ entry->tx_index = 0;
+ entry->txd = NULL;
rc = ntb_process_tx(qp, entry);
if (rc)
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 4/5] ntb: add DMA error handling for TX DMA
2016-06-27 23:09 ` [PATCH 4/5] ntb: add DMA error handling for TX DMA Dave Jiang
@ 2016-06-28 1:53 ` Allen Hubbe
2016-06-28 16:15 ` Jiang, Dave
0 siblings, 1 reply; 10+ messages in thread
From: Allen Hubbe @ 2016-06-28 1:53 UTC (permalink / raw)
To: Dave Jiang; +Cc: vinod.koul, Allen Hubbe, jdmason, dmaengine, linux-ntb
On Mon, Jun 27, 2016 at 7:09 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> Adding support on the tx DMA path to allow recovery of errors when DMA
> responds with error status and abort all the subsequent ops.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/ntb/ntb_transport.c | 123 +++++++++++++++++++++++++++++++------------
> 1 file changed, 89 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 2ef9d913..55a5ae0 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -102,6 +102,10 @@ struct ntb_queue_entry {
> void *buf;
> unsigned int len;
> unsigned int flags;
> + struct dma_async_tx_descriptor *txd;
> + int retries;
> + int errors;
> + unsigned int tx_index;
>
> struct ntb_transport_qp *qp;
> union {
> @@ -258,6 +262,9 @@ enum {
> static void ntb_transport_rxc_db(unsigned long data);
> static const struct ntb_ctx_ops ntb_transport_ops;
> static struct ntb_client ntb_transport_client;
> +static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
> + struct ntb_queue_entry *entry);
> +static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void __iomem *offset);
>
> static int ntb_transport_bus_match(struct device *dev,
> struct device_driver *drv)
> @@ -1444,21 +1451,46 @@ static void ntb_tx_copy_callback(void *data)
> struct ntb_queue_entry *entry = data;
> struct ntb_transport_qp *qp = entry->qp;
> struct ntb_payload_header __iomem *hdr = entry->tx_hdr;
> + struct dma_async_tx_descriptor *txd;
> + unsigned int len;
> + bool abort = false;
> +
> + txd = entry->txd;
> +
> + /* we need to check DMA results if we are using DMA */
> + if (txd) {
> + switch (txd->result) {
> + case ERR_DMA_READ:
> + case ERR_DMA_WRITE:
> + entry->errors++;
> + case ERR_DMA_ABORT:
> + abort = true;
> + goto handle_tx;
> + case ERR_DMA_NONE:
> + default:
> + break;
> + }
> + }
>
> iowrite32(entry->flags | DESC_DONE_FLAG, &hdr->flags);
>
> ntb_peer_db_set(qp->ndev, BIT_ULL(qp->qp_num));
>
> +handle_tx:
> +
> /* The entry length can only be zero if the packet is intended to be a
> * "link down" or similar. Since no payload is being sent in these
> * cases, there is nothing to add to the completion queue.
> */
> if (entry->len > 0) {
> - qp->tx_bytes += entry->len;
> + if (!abort) {
> + qp->tx_bytes += entry->len;
> + len = entry->len;
> + } else
> + len = 0;
Checkpatch will complain about different { } for then and else.
>
> if (qp->tx_handler)
> - qp->tx_handler(qp, qp->cb_data, entry->cb_data,
> - entry->len);
> + qp->tx_handler(qp, qp->cb_data, entry->cb_data, len);
> }
>
> ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry, &qp->tx_free_q);
> @@ -1482,37 +1514,21 @@ static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void __iomem *offset)
> ntb_tx_copy_callback(entry);
> }
>
> -static void ntb_async_tx(struct ntb_transport_qp *qp,
> - struct ntb_queue_entry *entry)
> +static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
> + struct ntb_queue_entry *entry)
> {
> - struct ntb_payload_header __iomem *hdr;
> - struct dma_async_tx_descriptor *txd;
> struct dma_chan *chan = qp->tx_dma_chan;
> struct dma_device *device;
> + size_t len = entry->len;
> + void *buf = entry->buf;
> size_t dest_off, buff_off;
> struct dmaengine_unmap_data *unmap;
> dma_addr_t dest;
> dma_cookie_t cookie;
> - void __iomem *offset;
> - size_t len = entry->len;
> - void *buf = entry->buf;
> int retries = 0;
>
> - offset = qp->tx_mw + qp->tx_max_frame * qp->tx_index;
> - hdr = offset + qp->tx_max_frame - sizeof(struct ntb_payload_header);
> - entry->tx_hdr = hdr;
> -
> - iowrite32(entry->len, &hdr->len);
> - iowrite32((u32)qp->tx_pkts, &hdr->ver);
> -
> - if (!chan)
> - goto err;
> -
> - if (len < copy_bytes)
> - goto err;
> -
> device = chan->device;
> - dest = qp->tx_mw_phys + qp->tx_max_frame * qp->tx_index;
> + dest = qp->tx_mw_phys + qp->tx_max_frame * entry->tx_index;
> buff_off = (size_t)buf & ~PAGE_MASK;
> dest_off = (size_t)dest & ~PAGE_MASK;
>
> @@ -1532,39 +1548,74 @@ static void ntb_async_tx(struct ntb_transport_qp *qp,
> unmap->to_cnt = 1;
>
> for (retries = 0; retries < DMA_RETRIES; retries++) {
> - txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0],
> - len, DMA_PREP_INTERRUPT);
> - if (txd)
> + entry->txd = device->device_prep_dma_memcpy(chan, dest,
> + unmap->addr[0], len,
> + DMA_PREP_INTERRUPT);
> + if (entry->txd)
> break;
>
> set_current_state(TASK_INTERRUPTIBLE);
> schedule_timeout(DMA_OUT_RESOURCE_TO);
> }
>
> - if (!txd) {
> + if (!entry->txd) {
> qp->dma_tx_prep_err++;
> goto err_get_unmap;
> }
>
> - txd->callback = ntb_tx_copy_callback;
> - txd->callback_param = entry;
> - dma_set_unmap(txd, unmap);
> + entry->txd->callback = ntb_tx_copy_callback;
> + entry->txd->callback_param = entry;
> + dma_set_unmap(entry->txd, unmap);
>
> - cookie = dmaengine_submit(txd);
> + cookie = dmaengine_submit(entry->txd);
> if (dma_submit_error(cookie))
> goto err_set_unmap;
>
> dmaengine_unmap_put(unmap);
>
> dma_async_issue_pending(chan);
> - qp->tx_async++;
>
> - return;
> + return 0;
> err_set_unmap:
> dmaengine_unmap_put(unmap);
> err_get_unmap:
> dmaengine_unmap_put(unmap);
> err:
> + return -ENXIO;
> +}
> +
> +static void ntb_async_tx(struct ntb_transport_qp *qp,
> + struct ntb_queue_entry *entry)
> +{
> + struct ntb_payload_header __iomem *hdr;
> + struct dma_chan *chan = qp->tx_dma_chan;
> + void __iomem *offset;
> + int res;
> +
> + entry->tx_index = qp->tx_index;
> + offset = qp->tx_mw + qp->tx_max_frame * entry->tx_index;
> + hdr = offset + qp->tx_max_frame - sizeof(struct ntb_payload_header);
> + entry->tx_hdr = hdr;
> +
> + iowrite32(entry->len, &hdr->len);
> + iowrite32((u32)qp->tx_pkts, &hdr->ver);
> +
> + if (!chan)
> + goto err;
> +
> + if (entry->len < copy_bytes)
> + goto err;
> +
> + res = ntb_async_tx_submit(qp, entry);
> + if (res < 0)
> + goto err;
> +
> + if (!entry->retries)
> + qp->tx_async++;
> +
> + return;
> +
> +err:
If there is an error building or submitting the dma descriptor, we
will fall back to cpu memcpy, but if there is an error indicated at
the time of the dma completion, we will not. This would be a problem
if we later notify the peer driver that some data has been
transferred, even though it has not been transferred.
This seems like it would be a likely situation (or, as likely as
encountering a dma engine error), because dma prep will begin to fail
as soon as the first error is encountered. After detecting an error,
in [PATCH 2/5] bit IOAT_CHAN_DOWN is set, causing dma prep to fail in
the mean time. Then, ntb_transport will immediately fall back to
transferring data via cpu memcpy, but I do not see any fall back to
cpu memcpy for the failed or aborted descriptors.
> ntb_memcpy_tx(entry, offset);
> qp->tx_memcpy++;
> }
> @@ -1940,6 +1991,10 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
> entry->buf = data;
> entry->len = len;
> entry->flags = 0;
> + entry->errors = 0;
> + entry->retries = 0;
> + entry->tx_index = 0;
> + entry->txd = NULL;
>
> rc = ntb_process_tx(qp, entry);
> if (rc)
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 4/5] ntb: add DMA error handling for TX DMA
2016-06-28 1:53 ` Allen Hubbe
@ 2016-06-28 16:15 ` Jiang, Dave
0 siblings, 0 replies; 10+ messages in thread
From: Jiang, Dave @ 2016-06-28 16:15 UTC (permalink / raw)
To: allenbh@gmail.com
Cc: allen.hubbe@emc.com, dmaengine@vger.kernel.org, jdmason@kudzu.us,
Koul, Vinod, linux-ntb@googlegroups.com
On Tue, 2016-06-28 at 01:53 +0000, Allen Hubbe wrote:
> On Mon, Jun 27, 2016 at 7:09 PM, Dave Jiang <dave.jiang@intel.com>
> wrote:
> > Adding support on the tx DMA path to allow recovery of errors when
> > DMA
> > responds with error status and abort all the subsequent ops.
> >
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> > drivers/ntb/ntb_transport.c | 123
> > +++++++++++++++++++++++++++++++------------
> > 1 file changed, 89 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/ntb/ntb_transport.c
> > b/drivers/ntb/ntb_transport.c
> > index 2ef9d913..55a5ae0 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -102,6 +102,10 @@ struct ntb_queue_entry {
> > void *buf;
> > unsigned int len;
> > unsigned int flags;
> > + struct dma_async_tx_descriptor *txd;
> > + int retries;
> > + int errors;
> > + unsigned int tx_index;
> >
> > struct ntb_transport_qp *qp;
> > union {
> > @@ -258,6 +262,9 @@ enum {
> > static void ntb_transport_rxc_db(unsigned long data);
> > static const struct ntb_ctx_ops ntb_transport_ops;
> > static struct ntb_client ntb_transport_client;
> > +static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
> > + struct ntb_queue_entry *entry);
> > +static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void
> > __iomem *offset);
> >
> > static int ntb_transport_bus_match(struct device *dev,
> > struct device_driver *drv)
> > @@ -1444,21 +1451,46 @@ static void ntb_tx_copy_callback(void
> > *data)
> > struct ntb_queue_entry *entry = data;
> > struct ntb_transport_qp *qp = entry->qp;
> > struct ntb_payload_header __iomem *hdr = entry->tx_hdr;
> > + struct dma_async_tx_descriptor *txd;
> > + unsigned int len;
> > + bool abort = false;
> > +
> > + txd = entry->txd;
> > +
> > + /* we need to check DMA results if we are using DMA */
> > + if (txd) {
> > + switch (txd->result) {
> > + case ERR_DMA_READ:
> > + case ERR_DMA_WRITE:
> > + entry->errors++;
> > + case ERR_DMA_ABORT:
> > + abort = true;
> > + goto handle_tx;
> > + case ERR_DMA_NONE:
> > + default:
> > + break;
> > + }
> > + }
> >
> > iowrite32(entry->flags | DESC_DONE_FLAG, &hdr->flags);
> >
> > ntb_peer_db_set(qp->ndev, BIT_ULL(qp->qp_num));
> >
> > +handle_tx:
> > +
> > /* The entry length can only be zero if the packet is
> > intended to be a
> > * "link down" or similar. Since no payload is being sent
> > in these
> > * cases, there is nothing to add to the completion queue.
> > */
> > if (entry->len > 0) {
> > - qp->tx_bytes += entry->len;
> > + if (!abort) {
> > + qp->tx_bytes += entry->len;
> > + len = entry->len;
> > + } else
> > + len = 0;
>
> Checkpatch will complain about different { } for then and else.
It didn't. But I will fix anyhow.
>
> >
> > if (qp->tx_handler)
> > - qp->tx_handler(qp, qp->cb_data, entry-
> > >cb_data,
> > - entry->len);
> > + qp->tx_handler(qp, qp->cb_data, entry-
> > >cb_data, len);
> > }
> >
> > ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry, &qp-
> > >tx_free_q);
> > @@ -1482,37 +1514,21 @@ static void ntb_memcpy_tx(struct
> > ntb_queue_entry *entry, void __iomem *offset)
> > ntb_tx_copy_callback(entry);
> > }
> >
> > -static void ntb_async_tx(struct ntb_transport_qp *qp,
> > - struct ntb_queue_entry *entry)
> > +static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
> > + struct ntb_queue_entry *entry)
> > {
> > - struct ntb_payload_header __iomem *hdr;
> > - struct dma_async_tx_descriptor *txd;
> > struct dma_chan *chan = qp->tx_dma_chan;
> > struct dma_device *device;
> > + size_t len = entry->len;
> > + void *buf = entry->buf;
> > size_t dest_off, buff_off;
> > struct dmaengine_unmap_data *unmap;
> > dma_addr_t dest;
> > dma_cookie_t cookie;
> > - void __iomem *offset;
> > - size_t len = entry->len;
> > - void *buf = entry->buf;
> > int retries = 0;
> >
> > - offset = qp->tx_mw + qp->tx_max_frame * qp->tx_index;
> > - hdr = offset + qp->tx_max_frame - sizeof(struct
> > ntb_payload_header);
> > - entry->tx_hdr = hdr;
> > -
> > - iowrite32(entry->len, &hdr->len);
> > - iowrite32((u32)qp->tx_pkts, &hdr->ver);
> > -
> > - if (!chan)
> > - goto err;
> > -
> > - if (len < copy_bytes)
> > - goto err;
> > -
> > device = chan->device;
> > - dest = qp->tx_mw_phys + qp->tx_max_frame * qp->tx_index;
> > + dest = qp->tx_mw_phys + qp->tx_max_frame * entry->tx_index;
> > buff_off = (size_t)buf & ~PAGE_MASK;
> > dest_off = (size_t)dest & ~PAGE_MASK;
> >
> > @@ -1532,39 +1548,74 @@ static void ntb_async_tx(struct
> > ntb_transport_qp *qp,
> > unmap->to_cnt = 1;
> >
> > for (retries = 0; retries < DMA_RETRIES; retries++) {
> > - txd = device->device_prep_dma_memcpy(chan, dest,
> > unmap->addr[0],
> > - len,
> > DMA_PREP_INTERRUPT);
> > - if (txd)
> > + entry->txd = device->device_prep_dma_memcpy(chan,
> > dest,
> > + unmap-
> > >addr[0], len,
> > + DMA_PRE
> > P_INTERRUPT);
> > + if (entry->txd)
> > break;
> >
> > set_current_state(TASK_INTERRUPTIBLE);
> > schedule_timeout(DMA_OUT_RESOURCE_TO);
> > }
> >
> > - if (!txd) {
> > + if (!entry->txd) {
> > qp->dma_tx_prep_err++;
> > goto err_get_unmap;
> > }
> >
> > - txd->callback = ntb_tx_copy_callback;
> > - txd->callback_param = entry;
> > - dma_set_unmap(txd, unmap);
> > + entry->txd->callback = ntb_tx_copy_callback;
> > + entry->txd->callback_param = entry;
> > + dma_set_unmap(entry->txd, unmap);
> >
> > - cookie = dmaengine_submit(txd);
> > + cookie = dmaengine_submit(entry->txd);
> > if (dma_submit_error(cookie))
> > goto err_set_unmap;
> >
> > dmaengine_unmap_put(unmap);
> >
> > dma_async_issue_pending(chan);
> > - qp->tx_async++;
> >
> > - return;
> > + return 0;
> > err_set_unmap:
> > dmaengine_unmap_put(unmap);
> > err_get_unmap:
> > dmaengine_unmap_put(unmap);
> > err:
> > + return -ENXIO;
> > +}
> > +
> > +static void ntb_async_tx(struct ntb_transport_qp *qp,
> > + struct ntb_queue_entry *entry)
> > +{
> > + struct ntb_payload_header __iomem *hdr;
> > + struct dma_chan *chan = qp->tx_dma_chan;
> > + void __iomem *offset;
> > + int res;
> > +
> > + entry->tx_index = qp->tx_index;
> > + offset = qp->tx_mw + qp->tx_max_frame * entry->tx_index;
> > + hdr = offset + qp->tx_max_frame - sizeof(struct
> > ntb_payload_header);
> > + entry->tx_hdr = hdr;
> > +
> > + iowrite32(entry->len, &hdr->len);
> > + iowrite32((u32)qp->tx_pkts, &hdr->ver);
> > +
> > + if (!chan)
> > + goto err;
> > +
> > + if (entry->len < copy_bytes)
> > + goto err;
> > +
> > + res = ntb_async_tx_submit(qp, entry);
> > + if (res < 0)
> > + goto err;
> > +
> > + if (!entry->retries)
> > + qp->tx_async++;
> > +
> > + return;
> > +
> > +err:
>
> If there is an error building or submitting the dma descriptor, we
> will fall back to cpu memcpy, but if there is an error indicated at
> the time of the dma completion, we will not. This would be a problem
> if we later notify the peer driver that some data has been
> transferred, even though it has not been transferred.
>
> This seems like it would be a likely situation (or, as likely as
> encountering a dma engine error), because dma prep will begin to fail
> as soon as the first error is encountered. After detecting an error,
> in [PATCH 2/5] bit IOAT_CHAN_DOWN is set, causing dma prep to fail in
> the mean time. Then, ntb_transport will immediately fall back to
> transferring data via cpu memcpy, but I do not see any fall back to
> cpu memcpy for the failed or aborted descriptors.
Ah I forget that it falls back to CPU copy. I was trying to just abort
everything and let the upper network layer handle it. I'll change it to
CPU copy instead.
>
> > ntb_memcpy_tx(entry, offset);
> > qp->tx_memcpy++;
> > }
> > @@ -1940,6 +1991,10 @@ int ntb_transport_tx_enqueue(struct
> > ntb_transport_qp *qp, void *cb, void *data,
> > entry->buf = data;
> > entry->len = len;
> > entry->flags = 0;
> > + entry->errors = 0;
> > + entry->retries = 0;
> > + entry->tx_index = 0;
> > + entry->txd = NULL;
> >
> > rc = ntb_process_tx(qp, entry);
> > if (rc)
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 5/5] ntb: add DMA error handling for RX DMA
2016-06-27 23:09 [PATCH 0/5] dmaengine: ioatdma: DMA error reporting Dave Jiang
` (3 preceding siblings ...)
2016-06-27 23:09 ` [PATCH 4/5] ntb: add DMA error handling for TX DMA Dave Jiang
@ 2016-06-27 23:10 ` Dave Jiang
4 siblings, 0 replies; 10+ messages in thread
From: Dave Jiang @ 2016-06-27 23:10 UTC (permalink / raw)
To: vinod.koul, allen.hubbe, jdmason; +Cc: dmaengine, linux-ntb
Adding support on the rx DMA path to allow recovery of errors when DMA
responds with error status and abort all the subsequent ops.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/ntb/ntb_transport.c | 99 ++++++++++++++++++++++++++++++++-----------
1 file changed, 74 insertions(+), 25 deletions(-)
diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 55a5ae0..435b206 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -106,13 +106,13 @@ struct ntb_queue_entry {
int retries;
int errors;
unsigned int tx_index;
+ unsigned int rx_index;
struct ntb_transport_qp *qp;
union {
struct ntb_payload_header __iomem *tx_hdr;
struct ntb_payload_header *rx_hdr;
};
- unsigned int index;
};
struct ntb_rx_info {
@@ -227,6 +227,7 @@ struct ntb_transport_ctx {
enum {
DESC_DONE_FLAG = BIT(0),
LINK_DOWN_FLAG = BIT(1),
+ DESC_ABORT_FLAG = BIT(2),
};
struct ntb_payload_header {
@@ -265,6 +266,9 @@ static struct ntb_client ntb_transport_client;
static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
struct ntb_queue_entry *entry);
static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void __iomem *offset);
+static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset);
+static void ntb_memcpy_rx(struct ntb_queue_entry *entry, void *offset);
+
static int ntb_transport_bus_match(struct device *dev,
struct device_driver *drv)
@@ -1204,14 +1208,18 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
while (!list_empty(&qp->rx_post_q)) {
entry = list_first_entry(&qp->rx_post_q,
struct ntb_queue_entry, entry);
- if (!(entry->flags & DESC_DONE_FLAG))
+ if (entry->flags & DESC_ABORT_FLAG)
+ len = 0;
+ else if (!(entry->flags & DESC_DONE_FLAG))
break;
+ else
+ len = entry->len;
+
entry->rx_hdr->flags = 0;
- iowrite32(entry->index, &qp->rx_info->entry);
+ iowrite32(entry->rx_index, &qp->rx_info->entry);
cb_data = entry->cb_data;
- len = entry->len;
list_move_tail(&entry->entry, &qp->rx_free_q);
@@ -1229,8 +1237,27 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
static void ntb_rx_copy_callback(void *data)
{
struct ntb_queue_entry *entry = data;
+ struct dma_async_tx_descriptor *txd;
+ unsigned int flags = DESC_DONE_FLAG;
- entry->flags |= DESC_DONE_FLAG;
+ txd = entry->txd;
+
+ /* we need to check DMA results if we are using DMA */
+ if (txd) {
+ switch (txd->result) {
+ case ERR_DMA_READ:
+ case ERR_DMA_WRITE:
+ entry->errors++;
+ case ERR_DMA_ABORT:
+ flags = DESC_ABORT_FLAG;
+ break;
+ case ERR_DMA_NONE:
+ default:
+ break;
+ }
+ }
+
+ entry->flags |= flags;
ntb_complete_rxc(entry->qp);
}
@@ -1248,9 +1275,8 @@ static void ntb_memcpy_rx(struct ntb_queue_entry *entry, void *offset)
ntb_rx_copy_callback(entry);
}
-static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset)
+static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
{
- struct dma_async_tx_descriptor *txd;
struct ntb_transport_qp *qp = entry->qp;
struct dma_chan *chan = qp->rx_dma_chan;
struct dma_device *device;
@@ -1261,13 +1287,6 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset)
int retries = 0;
len = entry->len;
-
- if (!chan)
- goto err;
-
- if (len < copy_bytes)
- goto err;
-
device = chan->device;
pay_off = (size_t)offset & ~PAGE_MASK;
buff_off = (size_t)buf & ~PAGE_MASK;
@@ -1295,26 +1314,27 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset)
unmap->from_cnt = 1;
for (retries = 0; retries < DMA_RETRIES; retries++) {
- txd = device->device_prep_dma_memcpy(chan, unmap->addr[1],
- unmap->addr[0], len,
- DMA_PREP_INTERRUPT);
- if (txd)
+ entry->txd = device->device_prep_dma_memcpy(chan,
+ unmap->addr[1],
+ unmap->addr[0], len,
+ DMA_PREP_INTERRUPT);
+ if (entry->txd)
break;
set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(DMA_OUT_RESOURCE_TO);
}
- if (!txd) {
+ if (!entry->txd) {
qp->dma_rx_prep_err++;
goto err_get_unmap;
}
- txd->callback = ntb_rx_copy_callback;
- txd->callback_param = entry;
- dma_set_unmap(txd, unmap);
+ entry->txd->callback = ntb_rx_copy_callback;
+ entry->txd->callback_param = entry;
+ dma_set_unmap(entry->txd, unmap);
- cookie = dmaengine_submit(txd);
+ cookie = dmaengine_submit(entry->txd);
if (dma_submit_error(cookie))
goto err_set_unmap;
@@ -1324,13 +1344,38 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset)
qp->rx_async++;
- return;
+ return 0;
err_set_unmap:
dmaengine_unmap_put(unmap);
err_get_unmap:
dmaengine_unmap_put(unmap);
err:
+ return -ENXIO;
+}
+
+static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset)
+{
+ struct ntb_transport_qp *qp = entry->qp;
+ struct dma_chan *chan = qp->rx_dma_chan;
+ int res;
+
+ if (!chan)
+ goto err;
+
+ if (entry->len < copy_bytes)
+ goto err;
+
+ res = ntb_async_rx_submit(entry, offset);
+ if (res < 0)
+ goto err;
+
+ if (!entry->retries)
+ qp->rx_async++;
+
+ return;
+
+err:
ntb_memcpy_rx(entry, offset);
qp->rx_memcpy++;
}
@@ -1376,7 +1421,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
}
entry->rx_hdr = hdr;
- entry->index = qp->rx_index;
+ entry->rx_index = qp->rx_index;
if (hdr->len > entry->len) {
dev_dbg(&qp->ndev->pdev->dev,
@@ -1949,6 +1994,10 @@ int ntb_transport_rx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
entry->buf = data;
entry->len = len;
entry->flags = 0;
+ entry->retries = 0;
+ entry->errors = 0;
+ entry->rx_index = 0;
+ entry->txd = NULL;
ntb_list_add(&qp->ntb_rx_q_lock, &entry->entry, &qp->rx_pend_q);
^ permalink raw reply related [flat|nested] 10+ messages in thread