[dpdk-dev] [PATCH 1/7] regex/mlx5: fix jump to the wrong label

[dpdk-dev] [PATCH 1/7] regex/mlx5: fix jump to the wrong label

[Michael Baum]

The mlx5_regex_pci_probe function allocates a mlx5_regex_priv structure
using rte_zmalloc.

If the allocation fails, the function jumps to the dev_error label in
order to release previously allocated resources in the function.
However, in the dev_error label it attempts to refer to the internal
fields of the priv structure and if its allocation fails (as in this
case) it is actually dereferencing to NULL.

Replace the jump with an error label.

Fixes: 1db6ebd4ef58 ("regex/mlx5: fix crash on initialization failure")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_regex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regex/mlx5/mlx5_regex.c b/drivers/regex/mlx5/mlx5_regex.c
index 05048e7..c91c444 100644
--- a/drivers/regex/mlx5/mlx5_regex.c
+++ b/drivers/regex/mlx5/mlx5_regex.c
@@ -157,7 +157,7 @@
 	if (!priv) {
 		DRV_LOG(ERR, "Failed to allocate private memory.");
 		rte_errno = ENOMEM;
-		goto error;
+		goto dev_error;
 	}
 	priv->ctx = ctx;
 	priv->nb_engines = 2; /* attr.regexp_num_of_engines */
-- 
1.8.3.1

[dpdk-dev] [PATCH 2/7] regex/mlx5: fix iterator type in RXP engines management

[Michael Baum]

The mlx5_regex_rules_db_import function goes over all engines in the
loop and program rxp rules.

The iterator of the loop is called id and the variable representing the
number of engines is called priv->nb_engines.
The id variable is of uint8_t type while the priv->nb_engines variable
is of uint32_t type. The size of the priv->nb_engines variable is much
larger than the number of iterations allowed by the id type.
Theoretically there may be a situation where the value of the
priv->nb_engines will be greater than can be represented by 8 bits and
the loop will never end.

Change the type of id to uint32_t.

Fixes: b34d816363b5 ("regex/mlx5: support rules import")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_rxp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regex/mlx5/mlx5_rxp.c b/drivers/regex/mlx5/mlx5_rxp.c
index 7936a52..e54d2b8 100644
--- a/drivers/regex/mlx5/mlx5_rxp.c
+++ b/drivers/regex/mlx5/mlx5_rxp.c
@@ -921,7 +921,7 @@
 {
 	struct mlx5_regex_priv *priv = dev->data->dev_private;
 	struct mlx5_rxp_ctl_rules_pgm *rules = NULL;
-	uint8_t id;
+	uint32_t id;
 	int ret;
 
 	if (priv->prog_mode == MLX5_RXP_MODE_NOT_DEFINED) {
-- 
1.8.3.1

[dpdk-dev] [PATCH 3/7] regex/mlx5: fix unnecessary init in RXP handle

[Michael Baum]

The rxp_poll_csr_for_value function defines a variable named ret. It is
the return value of the function, and It is updated to 0 by default
later in the function.
Similarly the rxp_init_rtru function also defines a variable named ret.
The function assigns into it return values from functions during the
function.

In both functions they initialize the ret variable when defining it.
however, in both cases they do not use any ret variable before assigning
into them different values, so the initializations are unnecessary.

Clean the aforementioned unnecessary initializations.

Fixes: e3dbbf718ebc ("regex/mlx5: support configuration")
Fixes: b34d816363b5 ("regex/mlx5: support rules import")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_rxp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regex/mlx5/mlx5_rxp.c b/drivers/regex/mlx5/mlx5_rxp.c
index e54d2b8..41fbae7 100644
--- a/drivers/regex/mlx5/mlx5_rxp.c
+++ b/drivers/regex/mlx5/mlx5_rxp.c
@@ -225,7 +225,7 @@
 		       uint32_t expected_mask, uint32_t timeout_ms, uint8_t id)
 {
 	unsigned int i;
-	int ret = 0;
+	int ret;
 
 	ret = -EBUSY;
 	for (i = 0; i < timeout_ms; i++) {
@@ -276,7 +276,7 @@
 	uint32_t poll_value;
 	uint32_t expected_value;
 	uint32_t expected_mask;
-	int ret = 0;
+	int ret;
 
 	/* Read the rtru ctrl CSR. */
 	ret = mlx5_devx_regex_register_read(ctx, id, MLX5_RXP_RTRU_CSR_CTRL,
-- 
1.8.3.1

[dpdk-dev] [PATCH 4/7] regex/mlx5: fix unchecked return value in RXP handle

[Michael Baum]

The rxp_flush_rules function tries to read and write to the register
several times using DevX API, and when it fails the function returns an
error.
Similarly the rxp_init_eng function also tries to write to the register
several times, and if writing is failed, it returns an error too.

Both functions have one write that the function does not check if it
succeeded, overriding the return value from the write function without
using it.

Add a check for this writing, and return an error in case of failure.

Fixes: b34d816363b5 ("regex/mlx5: support rules import")
Fixes: e3dbbf718ebc ("regex/mlx5: support configuration")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_rxp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regex/mlx5/mlx5_rxp.c b/drivers/regex/mlx5/mlx5_rxp.c
index 41fbae7..ba78cc0 100644
--- a/drivers/regex/mlx5/mlx5_rxp.c
+++ b/drivers/regex/mlx5/mlx5_rxp.c
@@ -194,6 +194,10 @@
 	val |= MLX5_RXP_RTRU_CSR_CTRL_GO;
 	ret = mlx5_devx_regex_register_write(ctx, id, MLX5_RXP_RTRU_CSR_CTRL,
 					     val);
+	if (ret) {
+		DRV_LOG(ERR, "CSR write failed!");
+		return -1;
+	}
 	ret = rxp_poll_csr_for_value(ctx, &val, MLX5_RXP_RTRU_CSR_STATUS,
 				     MLX5_RXP_RTRU_CSR_STATUS_UPDATE_DONE,
 				     MLX5_RXP_RTRU_CSR_STATUS_UPDATE_DONE,
@@ -554,6 +558,8 @@
 		return ret;
 	ctrl &= ~MLX5_RXP_CSR_CTRL_INIT;
 	ret = mlx5_devx_regex_register_write(ctx, id, MLX5_RXP_CSR_CTRL, ctrl);
+	if (ret)
+		return ret;
 	rte_delay_us(20000);
 	ret = rxp_poll_csr_for_value(ctx, &ctrl, MLX5_RXP_CSR_STATUS,
 				     MLX5_RXP_CSR_STATUS_INIT_DONE,
-- 
1.8.3.1

[dpdk-dev] [PATCH 5/7] regex/mlx5: improve error messages in RXP rules flush

[Michael Baum]

During the rules flush, the rxp_poll_csr_for_value function is called
twice. The rxp_poll_csr_for_value function can fail for two reasons:
1. It could not read the value from register, in which case the
function returns -1.
2. It read a value, but not the value it expected to receive. In this
case it returns -EBUZY.

When the function fails it prints an error message that is relevant only
for a second type of failure. Moreover, for failure of the first type it
prints a value of an uninitialized variable.
In case of success, the function prints a debug message about the number
of cycles it took. This line was probably copied by mistake, since the
variable it reads from, is always equal to 0 and is not an indicator of
the number of cycles.

Remove the incorrect line about the cycles, and reduce the error print
only for the relevant error.

Fixes: b34d816363b5 ("regex/mlx5: support rules import")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_rxp.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/regex/mlx5/mlx5_rxp.c b/drivers/regex/mlx5/mlx5_rxp.c
index ba78cc0..fcbc766 100644
--- a/drivers/regex/mlx5/mlx5_rxp.c
+++ b/drivers/regex/mlx5/mlx5_rxp.c
@@ -179,12 +179,14 @@
 				     count, ~0,
 				     MLX5_RXP_POLL_CSR_FOR_VALUE_TIMEOUT, id);
 	if (ret < 0) {
-		DRV_LOG(ERR, "Rules not rx by RXP: credit: %d, depth: %d", val,
-			fifo_depth);
+		if (ret == -EBUSY)
+			DRV_LOG(ERR, "Rules not rx by RXP: credit: %d, depth:"
+				" %d", val, fifo_depth);
+		else
+			DRV_LOG(ERR, "CSR poll failed, can't read value!");
 		return ret;
 	}
 	DRV_LOG(DEBUG, "RTRU FIFO depth: 0x%x", fifo_depth);
-	DRV_LOG(DEBUG, "Rules flush took %d cycles.", ret);
 	ret = mlx5_devx_regex_register_read(ctx, id, MLX5_RXP_RTRU_CSR_CTRL,
 					    &val);
 	if (ret) {
@@ -203,10 +205,12 @@
 				     MLX5_RXP_RTRU_CSR_STATUS_UPDATE_DONE,
 				     MLX5_RXP_POLL_CSR_FOR_VALUE_TIMEOUT, id);
 	if (ret < 0) {
-		DRV_LOG(ERR, "Rules update timeout: 0x%08X", val);
+		if (ret == -EBUSY)
+			DRV_LOG(ERR, "Rules update timeout: 0x%08X", val);
+		else
+			DRV_LOG(ERR, "CSR poll failed, can't read value!");
 		return ret;
 	}
-	DRV_LOG(DEBUG, "Rules update took %d cycles", ret);
 	if (mlx5_devx_regex_register_read(ctx, id, MLX5_RXP_RTRU_CSR_CTRL,
 					  &val)) {
 		DRV_LOG(ERR, "CSR read failed!");
@@ -215,7 +219,7 @@
 	val &= ~(MLX5_RXP_RTRU_CSR_CTRL_GO);
 	if (mlx5_devx_regex_register_write(ctx, id, MLX5_RXP_RTRU_CSR_CTRL,
 					   val)) {
-		DRV_LOG(ERR, "CSR write write failed!");
+		DRV_LOG(ERR, "CSR write failed!");
 		return -1;
 	}
 
-- 
1.8.3.1

[dpdk-dev] [PATCH 6/7] regex/mlx5: improve constants type in QP buffers creation

[Michael Baum]

The constant representing the size of the metadata is defined as a
regular number (32-bit signed), even though all of its uses request an
unsigned int variable.
Similarly the constant representing the maximal output is also defined
as a regular number, even though all of its uses request an unsigned int
variable.

Change the type of the above constants to unsigned.

Fixes: 5f41b66d12cd ("regex/mlx5: setup fast path")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_regex_fastpath.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/regex/mlx5/mlx5_regex_fastpath.c b/drivers/regex/mlx5/mlx5_regex_fastpath.c
index 2549547..32ba19f 100644
--- a/drivers/regex/mlx5/mlx5_regex_fastpath.c
+++ b/drivers/regex/mlx5/mlx5_regex_fastpath.c
@@ -3,6 +3,8 @@
  */
 
 #include <unistd.h>
+#include <strings.h>
+#include <stdint.h>
 #include <sys/mman.h>
 
 #include <rte_malloc.h>
@@ -17,15 +19,14 @@
 #include <mlx5_glue.h>
 #include <mlx5_common.h>
 #include <mlx5_prm.h>
-#include <strings.h>
 
 #include "mlx5_regex_utils.h"
 #include "mlx5_rxp.h"
 #include "mlx5_regex.h"
 
 #define MLX5_REGEX_MAX_WQE_INDEX 0xffff
-#define MLX5_REGEX_METADATA_SIZE 64
-#define MLX5_REGEX_MAX_OUTPUT (1 << 11)
+#define MLX5_REGEX_METADATA_SIZE UINT32_C(64)
+#define MLX5_REGEX_MAX_OUTPUT (UINT32_C(1) << 11)
 #define MLX5_REGEX_WQE_CTRL_OFFSET 12
 #define MLX5_REGEX_WQE_METADATA_OFFSET 16
 #define MLX5_REGEX_WQE_GATHER_OFFSET 32
-- 
1.8.3.1

Re: [dpdk-dev] [dpdk-stable] [PATCH 6/7] regex/mlx5: improve constants type in QP buffers creation

[Thomas Monjalon]

18/11/2020 18:00, Michael Baum:
> -#define MLX5_REGEX_MAX_OUTPUT (1 << 11)
> +#define MLX5_REGEX_MAX_OUTPUT (UINT32_C(1) << 11)

Even better: RTE_BIT32(11)

I will change it while merging.

[dpdk-dev] [PATCH 7/7] regex/mlx5: fix QP setuping error flow

[Michael Baum]

In regex QP setup, the PMD creates some SQ objects.

When SQ object creation is failed, the previous SQ objects memory were
not freed what caused a memory leak.

Free them.

Fixes: 54fa1f6a67d7 ("regex/mlx5: add teardown for fastpath buffers")

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/regex/mlx5/mlx5_regex_control.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/regex/mlx5/mlx5_regex_control.c b/drivers/regex/mlx5/mlx5_regex_control.c
index 88b3d1a..d6f452b 100644
--- a/drivers/regex/mlx5/mlx5_regex_control.c
+++ b/drivers/regex/mlx5/mlx5_regex_control.c
@@ -336,6 +336,7 @@
 	struct mlx5_regex_priv *priv = dev->data->dev_private;
 	struct mlx5_regex_qp *qp;
 	int i;
+	int nb_sq_config = 0;
 	int ret;
 	uint16_t log_desc;
 
@@ -364,8 +365,9 @@
 		ret = regex_ctrl_create_sq(priv, qp, i, log_desc);
 		if (ret) {
 			DRV_LOG(ERR, "Can't create sq.");
-			goto err_sq;
+			goto err_btree;
 		}
+		nb_sq_config++;
 	}
 
 	ret = mlx5_mr_btree_init(&qp->mr_ctrl.cache_bh, MLX5_MR_BTREE_CACHE_N,
@@ -385,9 +387,8 @@
 err_fp:
 	mlx5_mr_btree_free(&qp->mr_ctrl.cache_bh);
 err_btree:
-	for (i = 0; i < qp->nb_obj; i++)
+	for (i = 0; i < nb_sq_config; i++)
 		regex_ctrl_destroy_sq(priv, qp, i);
-err_sq:
 	regex_ctrl_destroy_cq(priv, &qp->cq);
 err_cq:
 	rte_free(qp->sqs);
-- 
1.8.3.1

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/7] regex/mlx5: fix jump to the wrong label

[Thomas Monjalon]

18/11/2020 18:00, Michael Baum:
> The mlx5_regex_pci_probe function allocates a mlx5_regex_priv structure
> using rte_zmalloc.
> 
> If the allocation fails, the function jumps to the dev_error label in
> order to release previously allocated resources in the function.
> However, in the dev_error label it attempts to refer to the internal
> fields of the priv structure and if its allocation fails (as in this
> case) it is actually dereferencing to NULL.
> 
> Replace the jump with an error label.
> 
> Fixes: 1db6ebd4ef58 ("regex/mlx5: fix crash on initialization failure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Michael Baum <michaelba@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>

Series applied, thanks