[PATCH 1/5] dma/ioat: fix device stop if no copies done

[PATCH 1/5] dma/ioat: fix device stop if no copies done

[Bruce Richardson]

The HW DMA devices supported by IOAT driver do not transition to
the "active" state until the first operation is started by the HW.
Therefore, if the user calls "rte_dma_stop()" on a device without
triggering any operations, the sequence of commands to be sent to
the HW is different, as is the final device state.

Update the IOAT driver "stop" function to take account of this
difference.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index 5906eb45aa..aff7bbbfde 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -166,17 +166,28 @@ static int
 ioat_dev_stop(struct rte_dma_dev *dev)
 {
 	struct ioat_dmadev *ioat = dev->fp_obj->dev_private;
+	unsigned int chansts;
 	uint32_t retry = 0;

-	ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
+	chansts = (unsigned int)(ioat->regs->chansts & IOAT_CHANSTS_STATUS);
+	if (chansts == IOAT_CHANSTS_ACTIVE || chansts == IOAT_CHANSTS_IDLE)
+		ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
+	else
+		ioat->regs->chancmd = IOAT_CHANCMD_RESET;

 	do {
 		rte_pause();
 		retry++;
-	} while ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) != IOAT_CHANSTS_SUSPENDED
-			&& retry < 200);
+		chansts = (unsigned int)(ioat->regs->chansts & IOAT_CHANSTS_STATUS);
+	} while (chansts != IOAT_CHANSTS_SUSPENDED &&
+			chansts != IOAT_CHANSTS_HALTED && retry < 200);
+
+	if (chansts == IOAT_CHANSTS_SUSPENDED || chansts == IOAT_CHANSTS_HALTED)
+		return 0;

-	return ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) == IOAT_CHANSTS_SUSPENDED) ? 0 : -1;
+	IOAT_PMD_WARN("Channel could not be suspended on stop. (chansts = %u [%s])",
+			chansts, chansts_readable[chansts]);
+	return -1;
 }

 /* Get device information of a device. */
--
2.37.2

[PATCH 2/5] dma/ioat: fix incorrectly set indexes after restart

[Bruce Richardson]

As part of the process of restarting a dma instance, the IOAT driver
will reset the HW addresses and state values. The read and write
indexes for SW use need to be similarly reset to keep HW and SW in
sync.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index aff7bbbfde..072eb17cd9 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -146,6 +146,13 @@ ioat_dev_start(struct rte_dma_dev *dev)
 	/* Prime the status register to be set to the last element. */
 	ioat->status = ioat->ring_addr + ((ioat->qcfg.nb_desc - 1) * DESC_SZ);

+	/* reset all counters */
+	ioat->next_read = 0;
+	ioat->next_write = 0;
+	ioat->last_write = 0;
+	ioat->offset = 0;
+	ioat->failure = 0;
+
 	printf("IOAT.status: %s [0x%"PRIx64"]\n",
 			chansts_readable[ioat->status & IOAT_CHANSTS_STATUS],
 			ioat->status);
--
2.37.2

[PATCH v2 1/6] dma/ioat: fix device stop if no copies done

[Bruce Richardson]

The HW DMA devices supported by IOAT driver do not transition to
the "active" state until the first operation is started by the HW.
Therefore, if the user calls "rte_dma_stop()" on a device without
triggering any operations, the sequence of commands to be sent to
the HW is different, as is the final device state.

Update the IOAT driver "stop" function to take account of this
difference.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index 5906eb45aa..aff7bbbfde 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -166,17 +166,28 @@ static int
 ioat_dev_stop(struct rte_dma_dev *dev)
 {
 	struct ioat_dmadev *ioat = dev->fp_obj->dev_private;
+	unsigned int chansts;
 	uint32_t retry = 0;
 
-	ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
+	chansts = (unsigned int)(ioat->regs->chansts & IOAT_CHANSTS_STATUS);
+	if (chansts == IOAT_CHANSTS_ACTIVE || chansts == IOAT_CHANSTS_IDLE)
+		ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
+	else
+		ioat->regs->chancmd = IOAT_CHANCMD_RESET;
 
 	do {
 		rte_pause();
 		retry++;
-	} while ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) != IOAT_CHANSTS_SUSPENDED
-			&& retry < 200);
+		chansts = (unsigned int)(ioat->regs->chansts & IOAT_CHANSTS_STATUS);
+	} while (chansts != IOAT_CHANSTS_SUSPENDED &&
+			chansts != IOAT_CHANSTS_HALTED && retry < 200);
+
+	if (chansts == IOAT_CHANSTS_SUSPENDED || chansts == IOAT_CHANSTS_HALTED)
+		return 0;
 
-	return ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) == IOAT_CHANSTS_SUSPENDED) ? 0 : -1;
+	IOAT_PMD_WARN("Channel could not be suspended on stop. (chansts = %u [%s])",
+			chansts, chansts_readable[chansts]);
+	return -1;
 }
 
 /* Get device information of a device. */
-- 
2.37.2

[PATCH v2 2/6] dma/ioat: fix incorrectly set indexes after restart

[Bruce Richardson]

As part of the process of restarting a dma instance, the IOAT driver
will reset the HW addresses and state values. The read and write
indexes for SW use need to be similarly reset to keep HW and SW in
sync.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index aff7bbbfde..072eb17cd9 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -146,6 +146,13 @@ ioat_dev_start(struct rte_dma_dev *dev)
 	/* Prime the status register to be set to the last element. */
 	ioat->status = ioat->ring_addr + ((ioat->qcfg.nb_desc - 1) * DESC_SZ);
 
+	/* reset all counters */
+	ioat->next_read = 0;
+	ioat->next_write = 0;
+	ioat->last_write = 0;
+	ioat->offset = 0;
+	ioat->failure = 0;
+
 	printf("IOAT.status: %s [0x%"PRIx64"]\n",
 			chansts_readable[ioat->status & IOAT_CHANSTS_STATUS],
 			ioat->status);
-- 
2.37.2

[PATCH v2 3/6] dma/ioat: fix incorrect error reporting on restart

[Bruce Richardson]

When the DMA device was stopped and restarted by the driver, the control
register specifying the behaviour on error was not getting correctly
reset. This caused unit tests to fail as explicitly introduced errors
were got getting reported back.

Fix by moving the setting of the register to the start function from the
probe function.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index 072eb17cd9..57c18c081d 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -142,6 +142,9 @@ ioat_dev_start(struct rte_dma_dev *dev)
 	ioat->regs->chainaddr = ioat->ring_addr;
 	/* Inform hardware of where to write the status/completions. */
 	ioat->regs->chancmp = ioat->status_addr;
+	/* Ensure channel control is set to abort on error, so we get status writeback. */
+	ioat->regs->chanctrl = IOAT_CHANCTRL_ANY_ERR_ABORT_EN |
+			IOAT_CHANCTRL_ERR_COMPLETION_EN;
 
 	/* Prime the status register to be set to the last element. */
 	ioat->status = ioat->ring_addr + ((ioat->qcfg.nb_desc - 1) * DESC_SZ);
@@ -682,8 +685,6 @@ ioat_dmadev_create(const char *name, struct rte_pci_device *dev)
 			return -EIO;
 		}
 	}
-	ioat->regs->chanctrl = IOAT_CHANCTRL_ANY_ERR_ABORT_EN |
-			IOAT_CHANCTRL_ERR_COMPLETION_EN;
 
 	dmadev->fp_obj->dev_private = ioat;
 
-- 
2.37.2

RE: [PATCH v2 1/6] dma/ioat: fix device stop if no copies done

[Walsh, Conor]

> The HW DMA devices supported by IOAT driver do not transition to
> the "active" state until the first operation is started by the HW.
> Therefore, if the user calls "rte_dma_stop()" on a device without
> triggering any operations, the sequence of commands to be sent to
> the HW is different, as is the final device state.
> 
> Update the IOAT driver "stop" function to take account of this
> difference.
> 
> Fixes: 583f046dd404 ("dma/ioat: add start and stop")
> Cc: conor.walsh@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Reviewed-by: Conor Walsh <conor.walsh@intel.com>

RE: [PATCH v2 2/6] dma/ioat: fix incorrectly set indexes after restart

[Walsh, Conor]

> As part of the process of restarting a dma instance, the IOAT driver
> will reset the HW addresses and state values. The read and write
> indexes for SW use need to be similarly reset to keep HW and SW in
> sync.
> 
> Fixes: 583f046dd404 ("dma/ioat: add start and stop")
> Cc: conor.walsh@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Reviewed-by: Conor Walsh <conor.walsh@intel.com>

RE: [PATCH v2 3/6] dma/ioat: fix incorrect error reporting on restart

[Walsh, Conor]

> When the DMA device was stopped and restarted by the driver, the control
> register specifying the behaviour on error was not getting correctly
> reset. This caused unit tests to fail as explicitly introduced errors
> were got getting reported back.
> 
> Fix by moving the setting of the register to the start function from the
> probe function.
> 
> Fixes: 583f046dd404 ("dma/ioat: add start and stop")
> Cc: conor.walsh@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Reviewed-by: Conor Walsh <conor.walsh@intel.com>

Re: [PATCH v2 1/6] dma/ioat: fix device stop if no copies done

[Kevin Laatz]

On 16/01/2023 17:37, Bruce Richardson wrote:
> The HW DMA devices supported by IOAT driver do not transition to
> the "active" state until the first operation is started by the HW.
> Therefore, if the user calls "rte_dma_stop()" on a device without
> triggering any operations, the sequence of commands to be sent to
> the HW is different, as is the final device state.
>
> Update the IOAT driver "stop" function to take account of this
> difference.
>
> Fixes: 583f046dd404 ("dma/ioat: add start and stop")
> Cc: conor.walsh@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/dma/ioat/ioat_dmadev.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [PATCH v2 2/6] dma/ioat: fix incorrectly set indexes after restart

[Kevin Laatz]

On 16/01/2023 17:37, Bruce Richardson wrote:
> As part of the process of restarting a dma instance, the IOAT driver
> will reset the HW addresses and state values. The read and write
> indexes for SW use need to be similarly reset to keep HW and SW in
> sync.
>
> Fixes: 583f046dd404 ("dma/ioat: add start and stop")
> Cc: conor.walsh@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/dma/ioat/ioat_dmadev.c | 7 +++++++
>   1 file changed, 7 insertions(+)
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [PATCH v2 3/6] dma/ioat: fix incorrect error reporting on restart

[Kevin Laatz]

On 16/01/2023 17:37, Bruce Richardson wrote:
> When the DMA device was stopped and restarted by the driver, the control
> register specifying the behaviour on error was not getting correctly
> reset. This caused unit tests to fail as explicitly introduced errors
> were got getting reported back.
>
> Fix by moving the setting of the register to the start function from the
> probe function.
>
> Fixes: 583f046dd404 ("dma/ioat: add start and stop")
> Cc: conor.walsh@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/dma/ioat/ioat_dmadev.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

[PATCH v3 1/6] dma/ioat: fix device stop if no copies done

[Bruce Richardson]

The HW DMA devices supported by IOAT driver do not transition to
the "active" state until the first operation is started by the HW.
Therefore, if the user calls "rte_dma_stop()" on a device without
triggering any operations, the sequence of commands to be sent to
the HW is different, as is the final device state.

Update the IOAT driver "stop" function to take account of this
difference.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: Conor Walsh <conor.walsh@intel.com>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index 5906eb45aa..aff7bbbfde 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -166,17 +166,28 @@ static int
 ioat_dev_stop(struct rte_dma_dev *dev)
 {
 	struct ioat_dmadev *ioat = dev->fp_obj->dev_private;
+	unsigned int chansts;
 	uint32_t retry = 0;
 
-	ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
+	chansts = (unsigned int)(ioat->regs->chansts & IOAT_CHANSTS_STATUS);
+	if (chansts == IOAT_CHANSTS_ACTIVE || chansts == IOAT_CHANSTS_IDLE)
+		ioat->regs->chancmd = IOAT_CHANCMD_SUSPEND;
+	else
+		ioat->regs->chancmd = IOAT_CHANCMD_RESET;
 
 	do {
 		rte_pause();
 		retry++;
-	} while ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) != IOAT_CHANSTS_SUSPENDED
-			&& retry < 200);
+		chansts = (unsigned int)(ioat->regs->chansts & IOAT_CHANSTS_STATUS);
+	} while (chansts != IOAT_CHANSTS_SUSPENDED &&
+			chansts != IOAT_CHANSTS_HALTED && retry < 200);
+
+	if (chansts == IOAT_CHANSTS_SUSPENDED || chansts == IOAT_CHANSTS_HALTED)
+		return 0;
 
-	return ((ioat->regs->chansts & IOAT_CHANSTS_STATUS) == IOAT_CHANSTS_SUSPENDED) ? 0 : -1;
+	IOAT_PMD_WARN("Channel could not be suspended on stop. (chansts = %u [%s])",
+			chansts, chansts_readable[chansts]);
+	return -1;
 }
 
 /* Get device information of a device. */
-- 
2.37.2

[PATCH v3 2/6] dma/ioat: fix incorrectly set indexes after restart

[Bruce Richardson]

As part of the process of restarting a dma instance, the IOAT driver
will reset the HW addresses and state values. The read and write
indexes for SW use need to be similarly reset to keep HW and SW in
sync.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: Conor Walsh <conor.walsh@intel.com>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index aff7bbbfde..072eb17cd9 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -146,6 +146,13 @@ ioat_dev_start(struct rte_dma_dev *dev)
 	/* Prime the status register to be set to the last element. */
 	ioat->status = ioat->ring_addr + ((ioat->qcfg.nb_desc - 1) * DESC_SZ);
 
+	/* reset all counters */
+	ioat->next_read = 0;
+	ioat->next_write = 0;
+	ioat->last_write = 0;
+	ioat->offset = 0;
+	ioat->failure = 0;
+
 	printf("IOAT.status: %s [0x%"PRIx64"]\n",
 			chansts_readable[ioat->status & IOAT_CHANSTS_STATUS],
 			ioat->status);
-- 
2.37.2

[PATCH v3 3/6] dma/ioat: fix incorrect error reporting on restart

[Bruce Richardson]

When the DMA device was stopped and restarted by the driver, the control
register specifying the behaviour on error was not getting correctly
reset. This caused unit tests to fail as explicitly introduced errors
were got getting reported back.

Fix by moving the setting of the register to the start function from the
probe function.

Fixes: 583f046dd404 ("dma/ioat: add start and stop")
Cc: conor.walsh@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: Conor Walsh <conor.walsh@intel.com>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/ioat/ioat_dmadev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
index 072eb17cd9..57c18c081d 100644
--- a/drivers/dma/ioat/ioat_dmadev.c
+++ b/drivers/dma/ioat/ioat_dmadev.c
@@ -142,6 +142,9 @@ ioat_dev_start(struct rte_dma_dev *dev)
 	ioat->regs->chainaddr = ioat->ring_addr;
 	/* Inform hardware of where to write the status/completions. */
 	ioat->regs->chancmp = ioat->status_addr;
+	/* Ensure channel control is set to abort on error, so we get status writeback. */
+	ioat->regs->chanctrl = IOAT_CHANCTRL_ANY_ERR_ABORT_EN |
+			IOAT_CHANCTRL_ERR_COMPLETION_EN;
 
 	/* Prime the status register to be set to the last element. */
 	ioat->status = ioat->ring_addr + ((ioat->qcfg.nb_desc - 1) * DESC_SZ);
@@ -682,8 +685,6 @@ ioat_dmadev_create(const char *name, struct rte_pci_device *dev)
 			return -EIO;
 		}
 	}
-	ioat->regs->chanctrl = IOAT_CHANCTRL_ANY_ERR_ABORT_EN |
-			IOAT_CHANCTRL_ERR_COMPLETION_EN;
 
 	dmadev->fp_obj->dev_private = ioat;
 
-- 
2.37.2