[PATCH 0/5] dma/ioat: fix issues with stopping and restarting device

[PATCH 0/5] dma/ioat: fix issues with stopping and restarting device

[Bruce Richardson]

This patchset fixes a couple of problems with stopping and restarting an
ioat DMA device. Following the two fixes, a series of improvements are
made to the dmadev unit tests to properly validate that dmadevs work
correctly as they are started and stopped, and ensure that no other or
future drivers will suffer from issues.

Bruce Richardson (5):
  dma/ioat: fix device stop if no copies done
  dma/ioat: fix incorrectly set indexes after restart
  test/dmadev: check result for device stop
  test/dmadev: create separate function for single copy test
  test/dmadev: add tests for stopping and restarting dev

 app/test/test_dmadev.c         | 172 ++++++++++++++++++++++-----------
 drivers/dma/ioat/ioat_dmadev.c |  26 ++++-
 2 files changed, 137 insertions(+), 61 deletions(-)

--
2.37.2

[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 3/5] test/dmadev: check result for device stop

[Bruce Richardson]

The DMA device stop API can return an error value so check that return
value when running dmadev unit tests.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_dmadev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index fe62e98af8..4e1dbcaa19 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -837,7 +837,11 @@ test_dmadev_instance(int16_t dev_id)
 		goto err;

 	rte_mempool_free(pool);
-	rte_dma_stop(dev_id);
+
+	if (rte_dma_stop(dev_id) < 0) {
+		rte_mempool_free(pool);
+		ERR_RETURN("Error stopping device %u\n", dev_id);
+	}
 	rte_dma_stats_reset(dev_id, vchan);
 	return 0;

--
2.37.2

[PATCH 4/5] test/dmadev: create separate function for single copy test

[Bruce Richardson]

The copy tests for dmadev had separate blocks in the test function for
single copy and burst copies. Separate out the single-copy block to its
own function so that it can be re-used if necessary.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_dmadev.c | 120 ++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 56 deletions(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index 4e1dbcaa19..de787c14e2 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -175,77 +175,85 @@ do_multi_copies(int16_t dev_id, uint16_t vchan,
 }

 static int
-test_enqueue_copies(int16_t dev_id, uint16_t vchan)
+test_single_copy(int16_t dev_id, uint16_t vchan)
 {
-	enum rte_dma_status_code status;
-	unsigned int i;
+	uint16_t i;
 	uint16_t id;
+	enum rte_dma_status_code status;
+	struct rte_mbuf *src, *dst;
+	char *src_data, *dst_data;

-	/* test doing a single copy */
-	do {
-		struct rte_mbuf *src, *dst;
-		char *src_data, *dst_data;
+	src = rte_pktmbuf_alloc(pool);
+	dst = rte_pktmbuf_alloc(pool);
+	src_data = rte_pktmbuf_mtod(src, char *);
+	dst_data = rte_pktmbuf_mtod(dst, char *);

-		src = rte_pktmbuf_alloc(pool);
-		dst = rte_pktmbuf_alloc(pool);
-		src_data = rte_pktmbuf_mtod(src, char *);
-		dst_data = rte_pktmbuf_mtod(dst, char *);
+	for (i = 0; i < COPY_LEN; i++)
+		src_data[i] = rte_rand() & 0xFF;

-		for (i = 0; i < COPY_LEN; i++)
-			src_data[i] = rte_rand() & 0xFF;
+	id = rte_dma_copy(dev_id, vchan, rte_pktmbuf_iova(src), rte_pktmbuf_iova(dst),
+			COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT);
+	if (id != id_count)
+		ERR_RETURN("Error with rte_dma_copy, got %u, expected %u\n",
+				id, id_count);

-		id = rte_dma_copy(dev_id, vchan, rte_pktmbuf_iova(src), rte_pktmbuf_iova(dst),
-				COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT);
-		if (id != id_count)
-			ERR_RETURN("Error with rte_dma_copy, got %u, expected %u\n",
-					id, id_count);
+	/* give time for copy to finish, then check it was done */
+	await_hw(dev_id, vchan);

-		/* give time for copy to finish, then check it was done */
-		await_hw(dev_id, vchan);
+	for (i = 0; i < COPY_LEN; i++)
+		if (dst_data[i] != src_data[i])
+			ERR_RETURN("Data mismatch at char %u [Got %02x not %02x]\n", i,
+					dst_data[i], src_data[i]);
+
+	/* now check completion works */
+	id = ~id;
+	if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 1)
+		ERR_RETURN("Error with rte_dma_completed\n");
+
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received, %u [expected %u]\n",
+				id, id_count);
+
+	/* check for completed and id when no job done */
+	id = ~id;
+	if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 0)
+		ERR_RETURN("Error with rte_dma_completed when no job done\n");
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
+				id, id_count);
+
+	/* check for completed_status and id when no job done */
+	id = ~id;
+	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
+		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
+				id, id_count);

-		for (i = 0; i < COPY_LEN; i++)
-			if (dst_data[i] != src_data[i])
-				ERR_RETURN("Data mismatch at char %u [Got %02x not %02x]\n", i,
-						dst_data[i], src_data[i]);
-
-		/* now check completion works */
-		id = ~id;
-		if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 1)
-			ERR_RETURN("Error with rte_dma_completed\n");
-
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received, %u [expected %u]\n",
-					id, id_count);
-
-		/* check for completed and id when no job done */
-		id = ~id;
-		if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 0)
-			ERR_RETURN("Error with rte_dma_completed when no job done\n");
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
-					id, id_count);
-
-		/* check for completed_status and id when no job done */
-		id = ~id;
-		if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
-			ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
-					id, id_count);
+	rte_pktmbuf_free(src);
+	rte_pktmbuf_free(dst);

-		rte_pktmbuf_free(src);
-		rte_pktmbuf_free(dst);
+	/* now check completion returns nothing more */
+	if (rte_dma_completed(dev_id, 0, 1, NULL, NULL) != 0)
+		ERR_RETURN("Error with rte_dma_completed in empty check\n");
+
+	id_count++;

-		/* now check completion returns nothing more */
-		if (rte_dma_completed(dev_id, 0, 1, NULL, NULL) != 0)
-			ERR_RETURN("Error with rte_dma_completed in empty check\n");
+	return 0;
+}

-		id_count++;
+static int
+test_enqueue_copies(int16_t dev_id, uint16_t vchan)
+{
+	unsigned int i;

-	} while (0);
+	/* test doing a single copy */
+	if (test_single_copy(dev_id, vchan) < 0)
+		return -1;

 	/* test doing a multiple single copies */
 	do {
+		uint16_t id;
 		const uint16_t max_ops = 4;
 		struct rte_mbuf *src, *dst;
 		char *src_data, *dst_data;
--
2.37.2

[PATCH 5/5] test/dmadev: add tests for stopping and restarting dev

[Bruce Richardson]

Validate device operation when a device is stopped or restarted.

The only complication - and gap in the dmadev ABI specification - is
what happens to the job ids on restart. Some drivers reset them to 0,
while others continue where things left off. Take account of both
posibilities in the test case.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index de787c14e2..8fb73a41e2 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -304,6 +304,48 @@ test_enqueue_copies(int16_t dev_id, uint16_t vchan)
 			|| do_multi_copies(dev_id, vchan, 0, 0, 1);
 }
 
+static int
+test_stop_start(int16_t dev_id, uint16_t vchan)
+{
+	/* device is already started on input, should be (re)started on output */
+
+	uint16_t id = 0;
+	enum rte_dma_status_code status = RTE_DMA_STATUS_SUCCESSFUL;
+
+	/* - test stopping a device works ok,
+	 * - then do a start-stop without doing a copy
+	 * - finally restart the device
+	 * checking for errors at each stage, and validating we can still copy at the end.
+	 */
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device\n");
+
+	if (rte_dma_start(dev_id) < 0)
+		ERR_RETURN("Error restarting device\n");
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device after restart (no jobs executed)\n");
+
+	if (rte_dma_start(dev_id) < 0)
+		ERR_RETURN("Error restarting device after multiple stop-starts\n");
+
+	/* before doing a copy, we need to know what the next id will be it should
+	 * either be:
+	 * - the last completed job before start if driver does not reset id on stop
+	 * - or -1 i.e. next job is 0, if driver does reset the job ids on stop
+	 */
+	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
+		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
+	id += 1; /* id_count is next job id */
+	if (id != id_count && id != 0)
+		ERR_RETURN("Unexpected next id from device after stop-start. Got %u, expected %u or 0\n",
+				id, id_count);
+
+	id_count = id;
+	if (test_single_copy(dev_id, vchan) < 0)
+		ERR_RETURN("Error performing copy after device restart\n");
+	return 0;
+}
+
 /* Failure handling test cases - global macros and variables for those tests*/
 #define COMP_BURST_SZ	16
 #define OPT_FENCE(idx) ((fence && idx == 8) ? RTE_DMA_OP_FLAG_FENCE : 0)
@@ -819,6 +861,10 @@ test_dmadev_instance(int16_t dev_id)
 	if (runtest("copy", test_enqueue_copies, 640, dev_id, vchan, CHECK_ERRS) < 0)
 		goto err;
 
+	/* run tests stopping/starting devices and check jobs still work after restart */
+	if (runtest("stop-start", test_stop_start, 1, dev_id, vchan, CHECK_ERRS) < 0)
+		goto err;
+
 	/* run some burst capacity tests */
 	if (rte_dma_burst_capacity(dev_id, vchan) < 64)
 		printf("DMA Dev %u: insufficient burst capacity (64 required), skipping tests\n",
-- 
2.37.2

RE: [PATCH 0/5] dma/ioat: fix issues with stopping and restarting device

[Walsh, Conor]

Hi Bruce,

This patchset breaks the dmadev autotest for IOAT on IceLake.

Trace below:

### Test dmadev instance 0 [0000:00:01.0]
IOAT.status: ACTIVE [0x100242880]
DMA Dev 0: Running copy Tests
Ops submitted: 85120    Ops completed: 85120    Errors: 0
DMA Dev 0: Running stop-start Tests
IOAT.status: ACTIVE [0x100242880]
IOAT.status: ACTIVE [0x100242880]
Ops submitted: 1        Ops completed: 1        Errors: 0
DMA Dev 0: Running burst capacity Tests
Ops submitted: 65536    Ops completed: 65536    Errors: 0
DMA Dev 0: Running error handling Tests (errors expected)
In test_failure_in_full_burst:390 - Error, missing expected failed copy, 0. has_error is not set
In runtest:58 -
Error, not all submitted jobs are reported as completed
In test_dma:940 - Error, test failure for device 0
Test Faileded: 16       Ops completed: 0        Errors: 0
RTE>>IOAT: ioat_dmadev_remove(): Closing 0000:00:01.0 on NUMA node 0

Thanks,
Conor.


> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday 16 January 2023 15:37
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH 0/5] dma/ioat: fix issues with stopping and restarting device
> 
> This patchset fixes a couple of problems with stopping and restarting an
> ioat DMA device. Following the two fixes, a series of improvements are
> made to the dmadev unit tests to properly validate that dmadevs work
> correctly as they are started and stopped, and ensure that no other or
> future drivers will suffer from issues.
> 
> Bruce Richardson (5):
>   dma/ioat: fix device stop if no copies done
>   dma/ioat: fix incorrectly set indexes after restart
>   test/dmadev: check result for device stop
>   test/dmadev: create separate function for single copy test
>   test/dmadev: add tests for stopping and restarting dev
> 
>  app/test/test_dmadev.c         | 172 ++++++++++++++++++++++-----------
>  drivers/dma/ioat/ioat_dmadev.c |  26 ++++-
>  2 files changed, 137 insertions(+), 61 deletions(-)
> 
> --
> 2.37.2

Re: [PATCH 0/5] dma/ioat: fix issues with stopping and restarting device

[Bruce Richardson]

On Mon, Jan 16, 2023 at 04:09:19PM +0000, Walsh, Conor wrote:
> Hi Bruce,
> 
> This patchset breaks the dmadev autotest for IOAT on IceLake.
> 
> Trace below:
> 
> ### Test dmadev instance 0 [0000:00:01.0] IOAT.status: ACTIVE
> [0x100242880] DMA Dev 0: Running copy Tests Ops submitted: 85120    Ops
> completed: 85120    Errors: 0 DMA Dev 0: Running stop-start Tests
> IOAT.status: ACTIVE [0x100242880] IOAT.status: ACTIVE [0x100242880] Ops
> submitted: 1        Ops completed: 1        Errors: 0 DMA Dev 0: Running
> burst capacity Tests Ops submitted: 65536    Ops completed: 65536
> Errors: 0 DMA Dev 0: Running error handling Tests (errors expected) In
> test_failure_in_full_burst:390 - Error, missing expected failed copy, 0.
> has_error is not set In runtest:58 - Error, not all submitted jobs are
> reported as completed In test_dma:940 - Error, test failure for device 0
> Test Faileded: 16       Ops completed: 0        Errors: 0 RTE>>IOAT:
> ioat_dmadev_remove(): Closing 0000:00:01.0 on NUMA node 0
> 
> Thanks, Conor.
> 
Thanks for catching this, Conor. I was testing on a system using noiommu
mode, so those error tests were getting skipped. I'll investigate and do a
v2.

/Bruce

[PATCH v2 0/6] dma/ioat: fix issues with stopping and restarting device

[Bruce Richardson]

This patchset fixes a couple of problems with stopping and restarting an
ioat DMA device. Following the three fixes, a series of improvements are
made to the dmadev unit tests to properly validate that dmadevs work
correctly as they are started and stopped, and ensure that no other or
future drivers will suffer from issues.

v2:
* extra patch to fix issues with error reporting, as noted by Conor W.

Bruce Richardson (6):
  dma/ioat: fix device stop if no copies done
  dma/ioat: fix incorrectly set indexes after restart
  dma/ioat: fix incorrect error reporting on restart
  test/dmadev: check result for device stop
  test/dmadev: create separate function for single copy test
  test/dmadev: add tests for stopping and restarting dev

 app/test/test_dmadev.c         | 172 ++++++++++++++++++++++-----------
 drivers/dma/ioat/ioat_dmadev.c |  31 ++++--
 2 files changed, 140 insertions(+), 63 deletions(-)

--
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

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 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>

[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

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 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>

[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 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 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 v2 4/6] test/dmadev: check result for device stop

[Bruce Richardson]

The DMA device stop API can return an error value so check that return
value when running dmadev unit tests.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_dmadev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index fe62e98af8..4e1dbcaa19 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -837,7 +837,11 @@ test_dmadev_instance(int16_t dev_id)
 		goto err;
 
 	rte_mempool_free(pool);
-	rte_dma_stop(dev_id);
+
+	if (rte_dma_stop(dev_id) < 0) {
+		rte_mempool_free(pool);
+		ERR_RETURN("Error stopping device %u\n", dev_id);
+	}
 	rte_dma_stats_reset(dev_id, vchan);
 	return 0;
 
-- 
2.37.2

RE: [PATCH v2 4/6] test/dmadev: check result for device stop

[Walsh, Conor]

> The DMA device stop API can return an error value so check that return
> value when running dmadev unit tests.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

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

Re: [PATCH v2 4/6] test/dmadev: check result for device stop

[David Marchand]

Hi Chengwen,

On Mon, Jan 16, 2023 at 6:38 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> The DMA device stop API can return an error value so check that return
> value when running dmadev unit tests.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_dmadev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Can you review the last three patches of this series?
Thanks!


-- 
David Marchand

Re: [PATCH v2 4/6] test/dmadev: check result for device stop

[Kevin Laatz]

On 16/01/2023 17:37, Bruce Richardson wrote:
> The DMA device stop API can return an error value so check that return
> value when running dmadev unit tests.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   app/test/test_dmadev.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [PATCH v2 4/6] test/dmadev: check result for device stop

[fengchengwen]

Sorry late to see.

On 2023/1/17 1:37, Bruce Richardson wrote:
> The DMA device stop API can return an error value so check that return
> value when running dmadev unit tests.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_dmadev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> index fe62e98af8..4e1dbcaa19 100644
> --- a/app/test/test_dmadev.c
> +++ b/app/test/test_dmadev.c
> @@ -837,7 +837,11 @@ test_dmadev_instance(int16_t dev_id)
>  		goto err;
>  
>  	rte_mempool_free(pool);
> -	rte_dma_stop(dev_id);
> +
> +	if (rte_dma_stop(dev_id) < 0) {
> +		rte_mempool_free(pool);

The pool already freed in above.
I think just add ERR_RETURN here.

> +		ERR_RETURN("Error stopping device %u\n", dev_id);
> +	}
>  	rte_dma_stats_reset(dev_id, vchan);
>  	return 0;
>  
> 

Re: [PATCH v2 4/6] test/dmadev: check result for device stop

[Bruce Richardson]

On Wed, Feb 15, 2023 at 09:26:01AM +0800, fengchengwen wrote:
> Sorry late to see.
> 
> On 2023/1/17 1:37, Bruce Richardson wrote:
> > The DMA device stop API can return an error value so check that return
> > value when running dmadev unit tests.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  app/test/test_dmadev.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> > index fe62e98af8..4e1dbcaa19 100644
> > --- a/app/test/test_dmadev.c
> > +++ b/app/test/test_dmadev.c
> > @@ -837,7 +837,11 @@ test_dmadev_instance(int16_t dev_id)
> >  		goto err;
> >  
> >  	rte_mempool_free(pool);
> > -	rte_dma_stop(dev_id);
> > +
> > +	if (rte_dma_stop(dev_id) < 0) {
> > +		rte_mempool_free(pool);
> 
> The pool already freed in above.
> I think just add ERR_RETURN here.
>
Yep, good catch. Will fix in v3.

[PATCH v2 5/6] test/dmadev: create separate function for single copy test

[Bruce Richardson]

The copy tests for dmadev had separate blocks in the test function for
single copy and burst copies. Separate out the single-copy block to its
own function so that it can be re-used if necessary.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_dmadev.c | 120 ++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 56 deletions(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index 4e1dbcaa19..de787c14e2 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -175,77 +175,85 @@ do_multi_copies(int16_t dev_id, uint16_t vchan,
 }
 
 static int
-test_enqueue_copies(int16_t dev_id, uint16_t vchan)
+test_single_copy(int16_t dev_id, uint16_t vchan)
 {
-	enum rte_dma_status_code status;
-	unsigned int i;
+	uint16_t i;
 	uint16_t id;
+	enum rte_dma_status_code status;
+	struct rte_mbuf *src, *dst;
+	char *src_data, *dst_data;
 
-	/* test doing a single copy */
-	do {
-		struct rte_mbuf *src, *dst;
-		char *src_data, *dst_data;
+	src = rte_pktmbuf_alloc(pool);
+	dst = rte_pktmbuf_alloc(pool);
+	src_data = rte_pktmbuf_mtod(src, char *);
+	dst_data = rte_pktmbuf_mtod(dst, char *);
 
-		src = rte_pktmbuf_alloc(pool);
-		dst = rte_pktmbuf_alloc(pool);
-		src_data = rte_pktmbuf_mtod(src, char *);
-		dst_data = rte_pktmbuf_mtod(dst, char *);
+	for (i = 0; i < COPY_LEN; i++)
+		src_data[i] = rte_rand() & 0xFF;
 
-		for (i = 0; i < COPY_LEN; i++)
-			src_data[i] = rte_rand() & 0xFF;
+	id = rte_dma_copy(dev_id, vchan, rte_pktmbuf_iova(src), rte_pktmbuf_iova(dst),
+			COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT);
+	if (id != id_count)
+		ERR_RETURN("Error with rte_dma_copy, got %u, expected %u\n",
+				id, id_count);
 
-		id = rte_dma_copy(dev_id, vchan, rte_pktmbuf_iova(src), rte_pktmbuf_iova(dst),
-				COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT);
-		if (id != id_count)
-			ERR_RETURN("Error with rte_dma_copy, got %u, expected %u\n",
-					id, id_count);
+	/* give time for copy to finish, then check it was done */
+	await_hw(dev_id, vchan);
 
-		/* give time for copy to finish, then check it was done */
-		await_hw(dev_id, vchan);
+	for (i = 0; i < COPY_LEN; i++)
+		if (dst_data[i] != src_data[i])
+			ERR_RETURN("Data mismatch at char %u [Got %02x not %02x]\n", i,
+					dst_data[i], src_data[i]);
+
+	/* now check completion works */
+	id = ~id;
+	if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 1)
+		ERR_RETURN("Error with rte_dma_completed\n");
+
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received, %u [expected %u]\n",
+				id, id_count);
+
+	/* check for completed and id when no job done */
+	id = ~id;
+	if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 0)
+		ERR_RETURN("Error with rte_dma_completed when no job done\n");
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
+				id, id_count);
+
+	/* check for completed_status and id when no job done */
+	id = ~id;
+	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
+		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
+				id, id_count);
 
-		for (i = 0; i < COPY_LEN; i++)
-			if (dst_data[i] != src_data[i])
-				ERR_RETURN("Data mismatch at char %u [Got %02x not %02x]\n", i,
-						dst_data[i], src_data[i]);
-
-		/* now check completion works */
-		id = ~id;
-		if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 1)
-			ERR_RETURN("Error with rte_dma_completed\n");
-
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received, %u [expected %u]\n",
-					id, id_count);
-
-		/* check for completed and id when no job done */
-		id = ~id;
-		if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 0)
-			ERR_RETURN("Error with rte_dma_completed when no job done\n");
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
-					id, id_count);
-
-		/* check for completed_status and id when no job done */
-		id = ~id;
-		if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
-			ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
-					id, id_count);
+	rte_pktmbuf_free(src);
+	rte_pktmbuf_free(dst);
 
-		rte_pktmbuf_free(src);
-		rte_pktmbuf_free(dst);
+	/* now check completion returns nothing more */
+	if (rte_dma_completed(dev_id, 0, 1, NULL, NULL) != 0)
+		ERR_RETURN("Error with rte_dma_completed in empty check\n");
+
+	id_count++;
 
-		/* now check completion returns nothing more */
-		if (rte_dma_completed(dev_id, 0, 1, NULL, NULL) != 0)
-			ERR_RETURN("Error with rte_dma_completed in empty check\n");
+	return 0;
+}
 
-		id_count++;
+static int
+test_enqueue_copies(int16_t dev_id, uint16_t vchan)
+{
+	unsigned int i;
 
-	} while (0);
+	/* test doing a single copy */
+	if (test_single_copy(dev_id, vchan) < 0)
+		return -1;
 
 	/* test doing a multiple single copies */
 	do {
+		uint16_t id;
 		const uint16_t max_ops = 4;
 		struct rte_mbuf *src, *dst;
 		char *src_data, *dst_data;
-- 
2.37.2

Re: [PATCH v2 5/6] test/dmadev: create separate function for single copy test

[Kevin Laatz]

On 16/01/2023 17:37, Bruce Richardson wrote:
> The copy tests for dmadev had separate blocks in the test function for
> single copy and burst copies. Separate out the single-copy block to its
> own function so that it can be re-used if necessary.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   app/test/test_dmadev.c | 120 ++++++++++++++++++++++-------------------
>   1 file changed, 64 insertions(+), 56 deletions(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [PATCH v2 5/6] test/dmadev: create separate function for single copy test

[fengchengwen]

On 2023/1/17 1:37, Bruce Richardson wrote:
> The copy tests for dmadev had separate blocks in the test function for
> single copy and burst copies. Separate out the single-copy block to its
> own function so that it can be re-used if necessary.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

[PATCH v2 6/6] test/dmadev: add tests for stopping and restarting dev

[Bruce Richardson]

Validate device operation when a device is stopped or restarted.

The only complication - and gap in the dmadev ABI specification - is
what happens to the job ids on restart. Some drivers reset them to 0,
while others continue where things left off. Take account of both
possibilities in the test case.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index de787c14e2..8fb73a41e2 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -304,6 +304,48 @@ test_enqueue_copies(int16_t dev_id, uint16_t vchan)
 			|| do_multi_copies(dev_id, vchan, 0, 0, 1);
 }
 
+static int
+test_stop_start(int16_t dev_id, uint16_t vchan)
+{
+	/* device is already started on input, should be (re)started on output */
+
+	uint16_t id = 0;
+	enum rte_dma_status_code status = RTE_DMA_STATUS_SUCCESSFUL;
+
+	/* - test stopping a device works ok,
+	 * - then do a start-stop without doing a copy
+	 * - finally restart the device
+	 * checking for errors at each stage, and validating we can still copy at the end.
+	 */
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device\n");
+
+	if (rte_dma_start(dev_id) < 0)
+		ERR_RETURN("Error restarting device\n");
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device after restart (no jobs executed)\n");
+
+	if (rte_dma_start(dev_id) < 0)
+		ERR_RETURN("Error restarting device after multiple stop-starts\n");
+
+	/* before doing a copy, we need to know what the next id will be it should
+	 * either be:
+	 * - the last completed job before start if driver does not reset id on stop
+	 * - or -1 i.e. next job is 0, if driver does reset the job ids on stop
+	 */
+	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
+		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
+	id += 1; /* id_count is next job id */
+	if (id != id_count && id != 0)
+		ERR_RETURN("Unexpected next id from device after stop-start. Got %u, expected %u or 0\n",
+				id, id_count);
+
+	id_count = id;
+	if (test_single_copy(dev_id, vchan) < 0)
+		ERR_RETURN("Error performing copy after device restart\n");
+	return 0;
+}
+
 /* Failure handling test cases - global macros and variables for those tests*/
 #define COMP_BURST_SZ	16
 #define OPT_FENCE(idx) ((fence && idx == 8) ? RTE_DMA_OP_FLAG_FENCE : 0)
@@ -819,6 +861,10 @@ test_dmadev_instance(int16_t dev_id)
 	if (runtest("copy", test_enqueue_copies, 640, dev_id, vchan, CHECK_ERRS) < 0)
 		goto err;
 
+	/* run tests stopping/starting devices and check jobs still work after restart */
+	if (runtest("stop-start", test_stop_start, 1, dev_id, vchan, CHECK_ERRS) < 0)
+		goto err;
+
 	/* run some burst capacity tests */
 	if (rte_dma_burst_capacity(dev_id, vchan) < 64)
 		printf("DMA Dev %u: insufficient burst capacity (64 required), skipping tests\n",
-- 
2.37.2

Re: [PATCH v2 6/6] test/dmadev: add tests for stopping and restarting dev

[Kevin Laatz]

On 16/01/2023 17:37, Bruce Richardson wrote:
> Validate device operation when a device is stopped or restarted.
>
> The only complication - and gap in the dmadev ABI specification - is
> what happens to the job ids on restart. Some drivers reset them to 0,
> while others continue where things left off. Take account of both
> possibilities in the test case.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [PATCH v2 6/6] test/dmadev: add tests for stopping and restarting dev

[fengchengwen]

On 2023/1/17 1:37, Bruce Richardson wrote:
> Validate device operation when a device is stopped or restarted.
> 
> The only complication - and gap in the dmadev ABI specification - is
> what happens to the job ids on restart. Some drivers reset them to 0,
> while others continue where things left off. Take account of both
> possibilities in the test case.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> index de787c14e2..8fb73a41e2 100644
> --- a/app/test/test_dmadev.c
> +++ b/app/test/test_dmadev.c
> @@ -304,6 +304,48 @@ test_enqueue_copies(int16_t dev_id, uint16_t vchan)
>  			|| do_multi_copies(dev_id, vchan, 0, 0, 1);
>  }
>  
> +static int
> +test_stop_start(int16_t dev_id, uint16_t vchan)
> +{
> +	/* device is already started on input, should be (re)started on output */
> +
> +	uint16_t id = 0;
> +	enum rte_dma_status_code status = RTE_DMA_STATUS_SUCCESSFUL;
> +
> +	/* - test stopping a device works ok,
> +	 * - then do a start-stop without doing a copy
> +	 * - finally restart the device
> +	 * checking for errors at each stage, and validating we can still copy at the end.
> +	 */
> +	if (rte_dma_stop(dev_id) < 0)
> +		ERR_RETURN("Error stopping device\n");
> +
> +	if (rte_dma_start(dev_id) < 0)
> +		ERR_RETURN("Error restarting device\n");
> +	if (rte_dma_stop(dev_id) < 0)
> +		ERR_RETURN("Error stopping device after restart (no jobs executed)\n");
> +
> +	if (rte_dma_start(dev_id) < 0)
> +		ERR_RETURN("Error restarting device after multiple stop-starts\n");
> +
> +	/* before doing a copy, we need to know what the next id will be it should
> +	 * either be:
> +	 * - the last completed job before start if driver does not reset id on stop
> +	 * - or -1 i.e. next job is 0, if driver does reset the job ids on stop
> +	 */
> +	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
> +		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
> +	id += 1; /* id_count is next job id */
> +	if (id != id_count && id != 0)
> +		ERR_RETURN("Unexpected next id from device after stop-start. Got %u, expected %u or 0\n",
> +				id, id_count);

Hi Bruce,

Suggest add a warn LOG to identify the id was not reset zero.
So that new driver could detect break ABI specification.

Thanks.

Re: [PATCH v2 6/6] test/dmadev: add tests for stopping and restarting dev

[Bruce Richardson]

On Wed, Feb 15, 2023 at 09:59:06AM +0800, fengchengwen wrote:
> On 2023/1/17 1:37, Bruce Richardson wrote:
> > Validate device operation when a device is stopped or restarted.
> > 
> > The only complication - and gap in the dmadev ABI specification - is
> > what happens to the job ids on restart. Some drivers reset them to 0,
> > while others continue where things left off. Take account of both
> > possibilities in the test case.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> ---
> > app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> > 
> > diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c index
> > de787c14e2..8fb73a41e2 100644 --- a/app/test/test_dmadev.c +++
> > b/app/test/test_dmadev.c @@ -304,6 +304,48 @@
> > test_enqueue_copies(int16_t dev_id, uint16_t vchan) ||
> > do_multi_copies(dev_id, vchan, 0, 0, 1); }
> >  
> > +static int +test_stop_start(int16_t dev_id, uint16_t vchan) +{ +	/*
> > device is already started on input, should be (re)started on output */
> > + +	uint16_t id = 0; +	enum rte_dma_status_code status =
> > RTE_DMA_STATUS_SUCCESSFUL; + +	/* - test stopping a device works
> > ok, +	 * - then do a start-stop without doing a copy +	 *
> > - finally restart the device +	 * checking for errors at each
> > stage, and validating we can still copy at the end.  +	 */ +	if
> > (rte_dma_stop(dev_id) < 0) +		ERR_RETURN("Error stopping
> > device\n"); + +	if (rte_dma_start(dev_id) < 0) +
> > ERR_RETURN("Error restarting device\n"); +	if (rte_dma_stop(dev_id) <
> > 0) +		ERR_RETURN("Error stopping device after restart (no
> > jobs executed)\n"); + +	if (rte_dma_start(dev_id) < 0) +
> > ERR_RETURN("Error restarting device after multiple stop-starts\n"); + +
> > /* before doing a copy, we need to know what the next id will be it
> > should +	 * either be: +	 * - the last completed job before start if
> > driver does not reset id on stop +	 * - or -1 i.e. next job is 0, if
> > driver does reset the job ids on stop +	 */ +	if
> > (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0) +
> > ERR_RETURN("Error with rte_dma_completed_status when no job done\n"); +
> > id += 1; /* id_count is next job id */ +	if (id != id_count && id !=
> > 0) +		ERR_RETURN("Unexpected next id from device after
> > stop-start. Got %u, expected %u or 0\n", +				id,
> > id_count);
> 
> Hi Bruce,
> 
> Suggest add a warn LOG to identify the id was not reset zero.  So that
> new driver could detect break ABI specification.
> 
Not sure that that is necessary. The actual ABI, nor the doxygen docs,
doesn't specify what happens to the values on doing stop and then start. My
thinking was that it should continue numbering as it would be equivalent to
suspend and resume, but other drivers appear to treat it as a "reset". I
suspect there are advantages and disadvantages to both schemes. Until we
decide on what the correct behaviour should be - or decide to allow both -
I don't think warning is the right thing to do here.

/Bruce

Re: [PATCH v2 6/6] test/dmadev: add tests for stopping and restarting dev

[fengchengwen]

On 2023/2/15 19:57, Bruce Richardson wrote:
> On Wed, Feb 15, 2023 at 09:59:06AM +0800, fengchengwen wrote:
>> On 2023/1/17 1:37, Bruce Richardson wrote:
>>> Validate device operation when a device is stopped or restarted.
>>>
>>> The only complication - and gap in the dmadev ABI specification - is
>>> what happens to the job ids on restart. Some drivers reset them to 0,
>>> while others continue where things left off. Take account of both
>>> possibilities in the test case.
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> ---
>>> app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 46 insertions(+)
>>>
>>> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c index
>>> de787c14e2..8fb73a41e2 100644 --- a/app/test/test_dmadev.c +++
>>> b/app/test/test_dmadev.c @@ -304,6 +304,48 @@
>>> test_enqueue_copies(int16_t dev_id, uint16_t vchan) ||
>>> do_multi_copies(dev_id, vchan, 0, 0, 1); }
>>>  
>>> +static int +test_stop_start(int16_t dev_id, uint16_t vchan) +{ +	/*
>>> device is already started on input, should be (re)started on output */
>>> + +	uint16_t id = 0; +	enum rte_dma_status_code status =
>>> RTE_DMA_STATUS_SUCCESSFUL; + +	/* - test stopping a device works
>>> ok, +	 * - then do a start-stop without doing a copy +	 *
>>> - finally restart the device +	 * checking for errors at each
>>> stage, and validating we can still copy at the end.  +	 */ +	if
>>> (rte_dma_stop(dev_id) < 0) +		ERR_RETURN("Error stopping
>>> device\n"); + +	if (rte_dma_start(dev_id) < 0) +
>>> ERR_RETURN("Error restarting device\n"); +	if (rte_dma_stop(dev_id) <
>>> 0) +		ERR_RETURN("Error stopping device after restart (no
>>> jobs executed)\n"); + +	if (rte_dma_start(dev_id) < 0) +
>>> ERR_RETURN("Error restarting device after multiple stop-starts\n"); + +
>>> /* before doing a copy, we need to know what the next id will be it
>>> should +	 * either be: +	 * - the last completed job before start if
>>> driver does not reset id on stop +	 * - or -1 i.e. next job is 0, if
>>> driver does reset the job ids on stop +	 */ +	if
>>> (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0) +
>>> ERR_RETURN("Error with rte_dma_completed_status when no job done\n"); +
>>> id += 1; /* id_count is next job id */ +	if (id != id_count && id !=
>>> 0) +		ERR_RETURN("Unexpected next id from device after
>>> stop-start. Got %u, expected %u or 0\n", +				id,
>>> id_count);
>>
>> Hi Bruce,
>>
>> Suggest add a warn LOG to identify the id was not reset zero.  So that
>> new driver could detect break ABI specification.
>>
> Not sure that that is necessary. The actual ABI, nor the doxygen docs,
> doesn't specify what happens to the values on doing stop and then start. My
> thinking was that it should continue numbering as it would be equivalent to
> suspend and resume, but other drivers appear to treat it as a "reset". I
> suspect there are advantages and disadvantages to both schemes. Until we
> decide on what the correct behaviour should be - or decide to allow both -
> I don't think warning is the right thing to do here.

In this point, agree to upstream this patch first, and then discuss the correct
behavior should be for restart scenario.

> 
> /Bruce
> 
> .
> 

Re: [PATCH v2 6/6] test/dmadev: add tests for stopping and restarting dev

[Bruce Richardson]

On Thu, Feb 16, 2023 at 09:24:38AM +0800, fengchengwen wrote:
> On 2023/2/15 19:57, Bruce Richardson wrote:
> > On Wed, Feb 15, 2023 at 09:59:06AM +0800, fengchengwen wrote:
> >> On 2023/1/17 1:37, Bruce Richardson wrote:
> >>> Validate device operation when a device is stopped or restarted.
> >>>
> >>> The only complication - and gap in the dmadev ABI specification - is
> >>> what happens to the job ids on restart. Some drivers reset them to 0,
> >>> while others continue where things left off. Take account of both
> >>> possibilities in the test case.
> >>>
> >>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> ---
> >>> app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 46 insertions(+)
> >>>
> >>> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c index
> >>> de787c14e2..8fb73a41e2 100644 --- a/app/test/test_dmadev.c +++
> >>> b/app/test/test_dmadev.c @@ -304,6 +304,48 @@
> >>> test_enqueue_copies(int16_t dev_id, uint16_t vchan) ||
> >>> do_multi_copies(dev_id, vchan, 0, 0, 1); }
> >>>  
> >>> +static int +test_stop_start(int16_t dev_id, uint16_t vchan) +{ +	/*
> >>> device is already started on input, should be (re)started on output */
> >>> + +	uint16_t id = 0; +	enum rte_dma_status_code status =
> >>> RTE_DMA_STATUS_SUCCESSFUL; + +	/* - test stopping a device works
> >>> ok, +	 * - then do a start-stop without doing a copy +	 *
> >>> - finally restart the device +	 * checking for errors at each
> >>> stage, and validating we can still copy at the end.  +	 */ +	if
> >>> (rte_dma_stop(dev_id) < 0) +		ERR_RETURN("Error stopping
> >>> device\n"); + +	if (rte_dma_start(dev_id) < 0) +
> >>> ERR_RETURN("Error restarting device\n"); +	if (rte_dma_stop(dev_id) <
> >>> 0) +		ERR_RETURN("Error stopping device after restart (no
> >>> jobs executed)\n"); + +	if (rte_dma_start(dev_id) < 0) +
> >>> ERR_RETURN("Error restarting device after multiple stop-starts\n"); + +
> >>> /* before doing a copy, we need to know what the next id will be it
> >>> should +	 * either be: +	 * - the last completed job before start if
> >>> driver does not reset id on stop +	 * - or -1 i.e. next job is 0, if
> >>> driver does reset the job ids on stop +	 */ +	if
> >>> (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0) +
> >>> ERR_RETURN("Error with rte_dma_completed_status when no job done\n"); +
> >>> id += 1; /* id_count is next job id */ +	if (id != id_count && id !=
> >>> 0) +		ERR_RETURN("Unexpected next id from device after
> >>> stop-start. Got %u, expected %u or 0\n", +				id,
> >>> id_count);
> >>
> >> Hi Bruce,
> >>
> >> Suggest add a warn LOG to identify the id was not reset zero.  So that
> >> new driver could detect break ABI specification.
> >>
> > Not sure that that is necessary. The actual ABI, nor the doxygen docs,
> > doesn't specify what happens to the values on doing stop and then start. My
> > thinking was that it should continue numbering as it would be equivalent to
> > suspend and resume, but other drivers appear to treat it as a "reset". I
> > suspect there are advantages and disadvantages to both schemes. Until we
> > decide on what the correct behaviour should be - or decide to allow both -
> > I don't think warning is the right thing to do here.
> 
> In this point, agree to upstream this patch first, and then discuss the correct
> behavior should be for restart scenario.
> 
+1. Thanks.

With this patch in place we will also be better able to help drivers
enforce the correct behaviour once we define it.

I'll do v3 keeping this as-is for now.

/Bruce

[PATCH v3 0/6] dma/ioat: fix issues with stopping and restarting device

[Bruce Richardson]

This patchset fixes a couple of problems with stopping and restarting an
ioat DMA device. Following the three fixes, a series of improvements are
made to the dmadev unit tests to properly validate that dmadevs work
correctly as they are started and stopped, and ensure that no other or
future drivers will suffer from issues.

v3:
* remove unnecessary mempool free on error (patch 4), as noted by Chengwen
v2:
* extra patch to fix issues with error reporting, as noted by Conor W.

Bruce Richardson (6):
  dma/ioat: fix device stop if no copies done
  dma/ioat: fix incorrectly set indexes after restart
  dma/ioat: fix incorrect error reporting on restart
  test/dmadev: check result for device stop
  test/dmadev: create separate function for single copy test
  test/dmadev: add tests for stopping and restarting dev

 app/test/test_dmadev.c         | 171 ++++++++++++++++++++++-----------
 drivers/dma/ioat/ioat_dmadev.c |  31 ++++--
 2 files changed, 139 insertions(+), 63 deletions(-)

--
2.37.2

[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

[PATCH v3 4/6] test/dmadev: check result for device stop

[Bruce Richardson]

The DMA device stop API can return an error value so check that return
value when running dmadev unit tests.

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>
---
 app/test/test_dmadev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index fe62e98af8..65226004d8 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -837,7 +837,10 @@ test_dmadev_instance(int16_t dev_id)
 		goto err;
 
 	rte_mempool_free(pool);
-	rte_dma_stop(dev_id);
+
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device %u\n", dev_id);
+
 	rte_dma_stats_reset(dev_id, vchan);
 	return 0;
 
-- 
2.37.2

Re: [PATCH v3 4/6] test/dmadev: check result for device stop

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2023/2/16 19:09, Bruce Richardson wrote:
> The DMA device stop API can return an error value so check that return
> value when running dmadev unit tests.
> 
> 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>
> ---
>  app/test/test_dmadev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> index fe62e98af8..65226004d8 100644
> --- a/app/test/test_dmadev.c
> +++ b/app/test/test_dmadev.c
> @@ -837,7 +837,10 @@ test_dmadev_instance(int16_t dev_id)
>  		goto err;
>  
>  	rte_mempool_free(pool);
> -	rte_dma_stop(dev_id);
> +
> +	if (rte_dma_stop(dev_id) < 0)
> +		ERR_RETURN("Error stopping device %u\n", dev_id);
> +
>  	rte_dma_stats_reset(dev_id, vchan);
>  	return 0;
>  
> 

[PATCH v3 5/6] test/dmadev: create separate function for single copy test

[Bruce Richardson]

The copy tests for dmadev had separate blocks in the test function for
single copy and burst copies. Separate out the single-copy block to its
own function so that it can be re-used if necessary.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_dmadev.c | 120 ++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 56 deletions(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index 65226004d8..0296c52d2a 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -175,77 +175,85 @@ do_multi_copies(int16_t dev_id, uint16_t vchan,
 }
 
 static int
-test_enqueue_copies(int16_t dev_id, uint16_t vchan)
+test_single_copy(int16_t dev_id, uint16_t vchan)
 {
-	enum rte_dma_status_code status;
-	unsigned int i;
+	uint16_t i;
 	uint16_t id;
+	enum rte_dma_status_code status;
+	struct rte_mbuf *src, *dst;
+	char *src_data, *dst_data;
 
-	/* test doing a single copy */
-	do {
-		struct rte_mbuf *src, *dst;
-		char *src_data, *dst_data;
+	src = rte_pktmbuf_alloc(pool);
+	dst = rte_pktmbuf_alloc(pool);
+	src_data = rte_pktmbuf_mtod(src, char *);
+	dst_data = rte_pktmbuf_mtod(dst, char *);
 
-		src = rte_pktmbuf_alloc(pool);
-		dst = rte_pktmbuf_alloc(pool);
-		src_data = rte_pktmbuf_mtod(src, char *);
-		dst_data = rte_pktmbuf_mtod(dst, char *);
+	for (i = 0; i < COPY_LEN; i++)
+		src_data[i] = rte_rand() & 0xFF;
 
-		for (i = 0; i < COPY_LEN; i++)
-			src_data[i] = rte_rand() & 0xFF;
+	id = rte_dma_copy(dev_id, vchan, rte_pktmbuf_iova(src), rte_pktmbuf_iova(dst),
+			COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT);
+	if (id != id_count)
+		ERR_RETURN("Error with rte_dma_copy, got %u, expected %u\n",
+				id, id_count);
 
-		id = rte_dma_copy(dev_id, vchan, rte_pktmbuf_iova(src), rte_pktmbuf_iova(dst),
-				COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT);
-		if (id != id_count)
-			ERR_RETURN("Error with rte_dma_copy, got %u, expected %u\n",
-					id, id_count);
+	/* give time for copy to finish, then check it was done */
+	await_hw(dev_id, vchan);
 
-		/* give time for copy to finish, then check it was done */
-		await_hw(dev_id, vchan);
+	for (i = 0; i < COPY_LEN; i++)
+		if (dst_data[i] != src_data[i])
+			ERR_RETURN("Data mismatch at char %u [Got %02x not %02x]\n", i,
+					dst_data[i], src_data[i]);
+
+	/* now check completion works */
+	id = ~id;
+	if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 1)
+		ERR_RETURN("Error with rte_dma_completed\n");
+
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received, %u [expected %u]\n",
+				id, id_count);
+
+	/* check for completed and id when no job done */
+	id = ~id;
+	if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 0)
+		ERR_RETURN("Error with rte_dma_completed when no job done\n");
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
+				id, id_count);
+
+	/* check for completed_status and id when no job done */
+	id = ~id;
+	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
+		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
+	if (id != id_count)
+		ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
+				id, id_count);
 
-		for (i = 0; i < COPY_LEN; i++)
-			if (dst_data[i] != src_data[i])
-				ERR_RETURN("Data mismatch at char %u [Got %02x not %02x]\n", i,
-						dst_data[i], src_data[i]);
-
-		/* now check completion works */
-		id = ~id;
-		if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 1)
-			ERR_RETURN("Error with rte_dma_completed\n");
-
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received, %u [expected %u]\n",
-					id, id_count);
-
-		/* check for completed and id when no job done */
-		id = ~id;
-		if (rte_dma_completed(dev_id, vchan, 1, &id, NULL) != 0)
-			ERR_RETURN("Error with rte_dma_completed when no job done\n");
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
-					id, id_count);
-
-		/* check for completed_status and id when no job done */
-		id = ~id;
-		if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
-			ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
-		if (id != id_count)
-			ERR_RETURN("Error:incorrect job id received when no job done, %u [expected %u]\n",
-					id, id_count);
+	rte_pktmbuf_free(src);
+	rte_pktmbuf_free(dst);
 
-		rte_pktmbuf_free(src);
-		rte_pktmbuf_free(dst);
+	/* now check completion returns nothing more */
+	if (rte_dma_completed(dev_id, 0, 1, NULL, NULL) != 0)
+		ERR_RETURN("Error with rte_dma_completed in empty check\n");
+
+	id_count++;
 
-		/* now check completion returns nothing more */
-		if (rte_dma_completed(dev_id, 0, 1, NULL, NULL) != 0)
-			ERR_RETURN("Error with rte_dma_completed in empty check\n");
+	return 0;
+}
 
-		id_count++;
+static int
+test_enqueue_copies(int16_t dev_id, uint16_t vchan)
+{
+	unsigned int i;
 
-	} while (0);
+	/* test doing a single copy */
+	if (test_single_copy(dev_id, vchan) < 0)
+		return -1;
 
 	/* test doing a multiple single copies */
 	do {
+		uint16_t id;
 		const uint16_t max_ops = 4;
 		struct rte_mbuf *src, *dst;
 		char *src_data, *dst_data;
-- 
2.37.2

[PATCH v3 6/6] test/dmadev: add tests for stopping and restarting dev

[Bruce Richardson]

Validate device operation when a device is stopped or restarted.

The only complication - and gap in the dmadev ABI specification - is
what happens to the job ids on restart. Some drivers reset them to 0,
while others continue where things left off. Take account of both
possibilities in the test case.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>
---
 app/test/test_dmadev.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index 0296c52d2a..0736ff2a18 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -304,6 +304,48 @@ test_enqueue_copies(int16_t dev_id, uint16_t vchan)
 			|| do_multi_copies(dev_id, vchan, 0, 0, 1);
 }
 
+static int
+test_stop_start(int16_t dev_id, uint16_t vchan)
+{
+	/* device is already started on input, should be (re)started on output */
+
+	uint16_t id = 0;
+	enum rte_dma_status_code status = RTE_DMA_STATUS_SUCCESSFUL;
+
+	/* - test stopping a device works ok,
+	 * - then do a start-stop without doing a copy
+	 * - finally restart the device
+	 * checking for errors at each stage, and validating we can still copy at the end.
+	 */
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device\n");
+
+	if (rte_dma_start(dev_id) < 0)
+		ERR_RETURN("Error restarting device\n");
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device after restart (no jobs executed)\n");
+
+	if (rte_dma_start(dev_id) < 0)
+		ERR_RETURN("Error restarting device after multiple stop-starts\n");
+
+	/* before doing a copy, we need to know what the next id will be it should
+	 * either be:
+	 * - the last completed job before start if driver does not reset id on stop
+	 * - or -1 i.e. next job is 0, if driver does reset the job ids on stop
+	 */
+	if (rte_dma_completed_status(dev_id, vchan, 1, &id, &status) != 0)
+		ERR_RETURN("Error with rte_dma_completed_status when no job done\n");
+	id += 1; /* id_count is next job id */
+	if (id != id_count && id != 0)
+		ERR_RETURN("Unexpected next id from device after stop-start. Got %u, expected %u or 0\n",
+				id, id_count);
+
+	id_count = id;
+	if (test_single_copy(dev_id, vchan) < 0)
+		ERR_RETURN("Error performing copy after device restart\n");
+	return 0;
+}
+
 /* Failure handling test cases - global macros and variables for those tests*/
 #define COMP_BURST_SZ	16
 #define OPT_FENCE(idx) ((fence && idx == 8) ? RTE_DMA_OP_FLAG_FENCE : 0)
@@ -819,6 +861,10 @@ test_dmadev_instance(int16_t dev_id)
 	if (runtest("copy", test_enqueue_copies, 640, dev_id, vchan, CHECK_ERRS) < 0)
 		goto err;
 
+	/* run tests stopping/starting devices and check jobs still work after restart */
+	if (runtest("stop-start", test_stop_start, 1, dev_id, vchan, CHECK_ERRS) < 0)
+		goto err;
+
 	/* run some burst capacity tests */
 	if (rte_dma_burst_capacity(dev_id, vchan) < 64)
 		printf("DMA Dev %u: insufficient burst capacity (64 required), skipping tests\n",
-- 
2.37.2

Re: [PATCH v3 6/6] test/dmadev: add tests for stopping and restarting dev

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2023/2/16 19:09, Bruce Richardson wrote:
> Validate device operation when a device is stopped or restarted.
> 
> The only complication - and gap in the dmadev ABI specification - is
> what happens to the job ids on restart. Some drivers reset them to 0,
> while others continue where things left off. Take account of both
> possibilities in the test case.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Kevin Laatz <kevin.laatz@intel.com>

...

Re: [PATCH v3 0/6] dma/ioat: fix issues with stopping and restarting device

[Thomas Monjalon]

> Bruce Richardson (6):
>   dma/ioat: fix device stop if no copies done
>   dma/ioat: fix incorrectly set indexes after restart
>   dma/ioat: fix incorrect error reporting on restart
>   test/dmadev: check result for device stop
>   test/dmadev: create separate function for single copy test
>   test/dmadev: add tests for stopping and restarting dev

Applied, thanks.