[dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups

[dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups

[Kevin Traynor]

Patches 1-5 are Coverity fixes.
Patches 6-9 are removing commented out code.

Kevin Traynor (9):
  net/pcap: fix argument checks
  crypto/octeontx: fix possible NULL deference
  net/bnxt: remove logically dead code
  net/ipn3ke: fix incorrect commit check logic
  net/ipn3ke: remove useless if statement
  net/ipn3ke: remove commented out code
  compress/octeontx: remove commented out code
  event/opdl: remove commented out code
  net/bnxt: remove commented out code

 drivers/common/cpt/cpt_ucode.h               |   3 +-
 drivers/compress/octeontx/include/zip_regs.h |   8 --
 drivers/event/opdl/opdl_test.c               |   3 -
 drivers/net/bnxt/bnxt_ethdev.c               |   3 -
 drivers/net/bnxt/bnxt_hwrm.c                 | 118 +++++++++----------
 drivers/net/ipn3ke/ipn3ke_ethdev.c           |   3 +-
 drivers/net/ipn3ke/ipn3ke_ethdev.h           |   7 --
 drivers/net/ipn3ke/ipn3ke_tm.c               |   7 --
 drivers/net/pcap/rte_eth_pcap.c              |  19 +--
 9 files changed, 70 insertions(+), 101 deletions(-)

-- 
2.21.0

[dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

[Kevin Traynor]

Previously rx/tx_queues were passed into eth_from_pcaps_common()
as ptrs and were checked for being NULL.

In commit da6ba28f0540 ("net/pcap: use a struct to pass user options")
that changed to pass in a ptr to a pmd_devargs_all which contains
the rx/tx_queues.

The parameter checking was not updated as part of that commit and
coverity caught that there was still a check if rx/tx_queues were
NULL, apparently after they had been dereferenced.

Fix that by checking the pmd_devargs_all ptr and removing the NULL
checks on rx/tx_queues.

1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
    deref_ptr: Directly dereferencing pointer tx_queues.
1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
1235        unsigned int i;
1236
1237        /* do some parameter checking */
    CID 345004: Dereference before null check (REVERSE_INULL)
    [select issue]
1238        if (rx_queues == NULL && nb_rx_queues > 0)
1239                return -1;
    CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
    check_after_deref: Null-checking tx_queues suggests that it may be
    null, but it has already been dereferenced on all paths leading to
    the check.
1240        if (tx_queues == NULL && nb_tx_queues > 0)
1241                return -1;

Coverity issue: 345029
Coverity issue: 345044
Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
Cc: cian.ferriter@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 5489010b6..7cf00306e 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -1229,16 +1229,19 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev,
 {
 	struct pmd_process_private *pp;
-	struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
-	struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
-	const unsigned int nb_rx_queues = rx_queues->num_of_queue;
-	const unsigned int nb_tx_queues = tx_queues->num_of_queue;
+	struct pmd_devargs *rx_queues;
+	struct pmd_devargs *tx_queues;
+	unsigned int nb_rx_queues;
+	unsigned int nb_tx_queues;
 	unsigned int i;
 
-	/* do some parameter checking */
-	if (rx_queues == NULL && nb_rx_queues > 0)
-		return -1;
-	if (tx_queues == NULL && nb_tx_queues > 0)
+	if (devargs_all == NULL)
 		return -1;
 
+	rx_queues = &devargs_all->rx_queues;
+	tx_queues = &devargs_all->tx_queues;
+
+	nb_rx_queues = rx_queues->num_of_queue;
+	nb_tx_queues = tx_queues->num_of_queue;
+
 	if (pmd_init_internals(vdev, nb_rx_queues, nb_tx_queues, internals,
 			eth_dev) < 0)
-- 
2.21.0

[dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference

[Kevin Traynor]

Coverity complains that ctrl_flags is set to NULL at the start
of the function and it may not have been set before there is a
jump to fc_success and it is dereferenced.

Check for NULL before dereference.

312fc_success:
   CID 344983 (#1 of 1): Explicit null dereferenced
   (FORWARD_NULL)7. var_deref_op: Dereferencing null pointer ctrl_flags.
313        *ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);

Coverity issue: 344983
Fixes: 6cc54096520d ("crypto/octeontx: add supported sessions")
Cc: ndabilpuram@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

---

There may be further rework needed to set it to the correct value,
but for now at least prevent the NULL dereference.
---
 drivers/common/cpt/cpt_ucode.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/common/cpt/cpt_ucode.h b/drivers/common/cpt/cpt_ucode.h
index 7d9c31e17..fad978c6e 100644
--- a/drivers/common/cpt/cpt_ucode.h
+++ b/drivers/common/cpt/cpt_ucode.h
@@ -311,5 +311,6 @@ cpt_fc_ciph_set_key(void *ctx, cipher_type_t type, const uint8_t *key,
 
 fc_success:
-	*ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
+	if (ctrl_flags != NULL)
+		*ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
 
 success:
-- 
2.21.0

[dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code

[Kevin Traynor]

If rc contains a non-zero return code from bnxt_hwrm_send_message(),
HWRM_CHECK_RESULT_SILENT() will return.

Just after that code, there is an 'if (!rc) {...} else {...}'.
Coverity is complaining that this if else statement is dead code as
rc will always be 0 if that code is reached.

4309        rc = bnxt_hwrm_send_message(bp, &req, sizeof(req),
                   BNXT_USE_CHIMP_MB);
    cond_const: Condition rc, taking false branch.
    Now the value of rc is equal to 0.
4310        HWRM_CHECK_RESULT_SILENT();
4311
    const: At condition rc, the value of rc must be equal to 0.
    dead_error_condition: The condition !rc must be true.
4312        if (!rc) {

[snip]

4373        } else {
    CID 343450 (#1 of 1): Logically dead code
    (DEADCODE)dead_error_line: Execution cannot
    reach this statement: rc = 0;.
4374                rc = 0;
4375        }

Coverity issue: 343450
Fixes: f8168ca0e690 ("net/bnxt: support thor controller")
Cc: lance.richardson@broadcom.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/bnxt/bnxt_hwrm.c | 118 +++++++++++++++++------------------
 1 file changed, 56 insertions(+), 62 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 9883fb506..5378e3e9c 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -4298,5 +4298,7 @@ int bnxt_hwrm_func_backing_store_qcaps(struct bnxt *bp)
 	struct hwrm_func_backing_store_qcaps_output *resp =
 		bp->hwrm_cmd_resp_addr;
-	int rc;
+	struct bnxt_ctx_pg_info *ctx_pg;
+	struct bnxt_ctx_mem_info *ctx;
+	int rc, total_alloc_len, i;
 
 	if (!BNXT_CHIP_THOR(bp) ||
@@ -4310,68 +4312,60 @@ int bnxt_hwrm_func_backing_store_qcaps(struct bnxt *bp)
 	HWRM_CHECK_RESULT_SILENT();
 
-	if (!rc) {
-		struct bnxt_ctx_pg_info *ctx_pg;
-		struct bnxt_ctx_mem_info *ctx;
-		int total_alloc_len;
-		int i;
+	total_alloc_len = sizeof(*ctx);
+	ctx = rte_malloc("bnxt_ctx_mem", total_alloc_len,
+			 RTE_CACHE_LINE_SIZE);
+	if (!ctx) {
+		rc = -ENOMEM;
+		goto ctx_err;
+	}
+	memset(ctx, 0, total_alloc_len);
 
-		total_alloc_len = sizeof(*ctx);
-		ctx = rte_malloc("bnxt_ctx_mem", total_alloc_len,
-				 RTE_CACHE_LINE_SIZE);
-		if (!ctx) {
-			rc = -ENOMEM;
-			goto ctx_err;
-		}
-		memset(ctx, 0, total_alloc_len);
+	ctx_pg = rte_malloc("bnxt_ctx_pg_mem",
+			    sizeof(*ctx_pg) * BNXT_MAX_Q,
+			    RTE_CACHE_LINE_SIZE);
+	if (!ctx_pg) {
+		rc = -ENOMEM;
+		goto ctx_err;
+	}
+	for (i = 0; i < BNXT_MAX_Q; i++, ctx_pg++)
+		ctx->tqm_mem[i] = ctx_pg;
 
-		ctx_pg = rte_malloc("bnxt_ctx_pg_mem",
-				    sizeof(*ctx_pg) * BNXT_MAX_Q,
-				    RTE_CACHE_LINE_SIZE);
-		if (!ctx_pg) {
-			rc = -ENOMEM;
-			goto ctx_err;
-		}
-		for (i = 0; i < BNXT_MAX_Q; i++, ctx_pg++)
-			ctx->tqm_mem[i] = ctx_pg;
+	bp->ctx = ctx;
+	ctx->qp_max_entries = rte_le_to_cpu_32(resp->qp_max_entries);
+	ctx->qp_min_qp1_entries =
+		rte_le_to_cpu_16(resp->qp_min_qp1_entries);
+	ctx->qp_max_l2_entries =
+		rte_le_to_cpu_16(resp->qp_max_l2_entries);
+	ctx->qp_entry_size = rte_le_to_cpu_16(resp->qp_entry_size);
+	ctx->srq_max_l2_entries =
+		rte_le_to_cpu_16(resp->srq_max_l2_entries);
+	ctx->srq_max_entries = rte_le_to_cpu_32(resp->srq_max_entries);
+	ctx->srq_entry_size = rte_le_to_cpu_16(resp->srq_entry_size);
+	ctx->cq_max_l2_entries =
+		rte_le_to_cpu_16(resp->cq_max_l2_entries);
+	ctx->cq_max_entries = rte_le_to_cpu_32(resp->cq_max_entries);
+	ctx->cq_entry_size = rte_le_to_cpu_16(resp->cq_entry_size);
+	ctx->vnic_max_vnic_entries =
+		rte_le_to_cpu_16(resp->vnic_max_vnic_entries);
+	ctx->vnic_max_ring_table_entries =
+		rte_le_to_cpu_16(resp->vnic_max_ring_table_entries);
+	ctx->vnic_entry_size = rte_le_to_cpu_16(resp->vnic_entry_size);
+	ctx->stat_max_entries =
+		rte_le_to_cpu_32(resp->stat_max_entries);
+	ctx->stat_entry_size = rte_le_to_cpu_16(resp->stat_entry_size);
+	ctx->tqm_entry_size = rte_le_to_cpu_16(resp->tqm_entry_size);
+	ctx->tqm_min_entries_per_ring =
+		rte_le_to_cpu_32(resp->tqm_min_entries_per_ring);
+	ctx->tqm_max_entries_per_ring =
+		rte_le_to_cpu_32(resp->tqm_max_entries_per_ring);
+	ctx->tqm_entries_multiple = resp->tqm_entries_multiple;
+	if (!ctx->tqm_entries_multiple)
+		ctx->tqm_entries_multiple = 1;
+	ctx->mrav_max_entries =
+		rte_le_to_cpu_32(resp->mrav_max_entries);
+	ctx->mrav_entry_size = rte_le_to_cpu_16(resp->mrav_entry_size);
+	ctx->tim_entry_size = rte_le_to_cpu_16(resp->tim_entry_size);
+	ctx->tim_max_entries = rte_le_to_cpu_32(resp->tim_max_entries);
 
-		bp->ctx = ctx;
-		ctx->qp_max_entries = rte_le_to_cpu_32(resp->qp_max_entries);
-		ctx->qp_min_qp1_entries =
-			rte_le_to_cpu_16(resp->qp_min_qp1_entries);
-		ctx->qp_max_l2_entries =
-			rte_le_to_cpu_16(resp->qp_max_l2_entries);
-		ctx->qp_entry_size = rte_le_to_cpu_16(resp->qp_entry_size);
-		ctx->srq_max_l2_entries =
-			rte_le_to_cpu_16(resp->srq_max_l2_entries);
-		ctx->srq_max_entries = rte_le_to_cpu_32(resp->srq_max_entries);
-		ctx->srq_entry_size = rte_le_to_cpu_16(resp->srq_entry_size);
-		ctx->cq_max_l2_entries =
-			rte_le_to_cpu_16(resp->cq_max_l2_entries);
-		ctx->cq_max_entries = rte_le_to_cpu_32(resp->cq_max_entries);
-		ctx->cq_entry_size = rte_le_to_cpu_16(resp->cq_entry_size);
-		ctx->vnic_max_vnic_entries =
-			rte_le_to_cpu_16(resp->vnic_max_vnic_entries);
-		ctx->vnic_max_ring_table_entries =
-			rte_le_to_cpu_16(resp->vnic_max_ring_table_entries);
-		ctx->vnic_entry_size = rte_le_to_cpu_16(resp->vnic_entry_size);
-		ctx->stat_max_entries =
-			rte_le_to_cpu_32(resp->stat_max_entries);
-		ctx->stat_entry_size = rte_le_to_cpu_16(resp->stat_entry_size);
-		ctx->tqm_entry_size = rte_le_to_cpu_16(resp->tqm_entry_size);
-		ctx->tqm_min_entries_per_ring =
-			rte_le_to_cpu_32(resp->tqm_min_entries_per_ring);
-		ctx->tqm_max_entries_per_ring =
-			rte_le_to_cpu_32(resp->tqm_max_entries_per_ring);
-		ctx->tqm_entries_multiple = resp->tqm_entries_multiple;
-		if (!ctx->tqm_entries_multiple)
-			ctx->tqm_entries_multiple = 1;
-		ctx->mrav_max_entries =
-			rte_le_to_cpu_32(resp->mrav_max_entries);
-		ctx->mrav_entry_size = rte_le_to_cpu_16(resp->mrav_entry_size);
-		ctx->tim_entry_size = rte_le_to_cpu_16(resp->tim_entry_size);
-		ctx->tim_max_entries = rte_le_to_cpu_32(resp->tim_max_entries);
-	} else {
-		rc = 0;
-	}
 ctx_err:
 	HWRM_UNLOCK();
-- 
2.21.0

[dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[Kevin Traynor]

Coverity is complaining about identical code regardless of which branch
of the if else is taken. Functionally it means an error will always be
returned if this if else is hit. Remove the else branch.

    CID 337928 (#1 of 1): Identical code for different branches
    (IDENTICAL_BRANCHES)identical_branches: The same code is executed
    regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
    n->n_children != 0U is true, because the 'then' and 'else' branches
    are identical. Should one of the branches be modified, or the entire
    'if' statement replaced?
1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
1507          n->n_children != 0) {
1508          return -rte_tm_error_set(error,
1509                  EINVAL,
1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
1511                  NULL,
1512                  rte_strerror(EINVAL));
    else_branch: The else branch, identical to the then branch.
1513  } else {
1514          return -rte_tm_error_set(error,
1515                  EINVAL,
1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
1517                  NULL,
1518                  rte_strerror(EINVAL));
1519  }

Coverity issue: 337928
Fixes: c820468ac99c ("net/ipn3ke: support TM")
Cc: rosen.xu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
index adf02c157..a93145d59 100644
--- a/drivers/net/ipn3ke/ipn3ke_tm.c
+++ b/drivers/net/ipn3ke/ipn3ke_tm.c
@@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct rte_eth_dev *dev,
 						NULL,
 						rte_strerror(EINVAL));
-			} else {
-				return -rte_tm_error_set(error,
-						EINVAL,
-						RTE_TM_ERROR_TYPE_UNSPECIFIED,
-						NULL,
-						rte_strerror(EINVAL));
 			}
 		}
-- 
2.21.0

[dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement

[Kevin Traynor]

Coverity complains that this statement is not needed as the goto
label is on the next line anyway. Remove the if statement.

653        ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
   CID 337930 (#1 of 1): Identical code for different branches
   (IDENTICAL_BRANCHES)identical_branches: The same code is executed
   when the condition ret is true or false, because the code in the
   if-then branch and after the if statement is identical. Should
   the if statement be removed?
654        if (ret)
655                goto end;
   implicit_else: The code from the above if-then branch is identical
   to the code after the if statement.
656end:

Coverity issue: 337930
Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
Cc: rosen.xu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/ipn3ke/ipn3ke_ethdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.c b/drivers/net/ipn3ke/ipn3ke_ethdev.c
index c226d6313..282295f49 100644
--- a/drivers/net/ipn3ke/ipn3ke_ethdev.c
+++ b/drivers/net/ipn3ke/ipn3ke_ethdev.c
@@ -652,6 +652,5 @@ ipn3ke_cfg_probe(struct rte_vdev_device *dev)
 
 	ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
-	if (ret)
-		goto end;
+
 end:
 	if (kvlist)
-- 
2.21.0

[dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code

[Kevin Traynor]

These struct members and variable were commented out. Remove them.

Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
Fixes: c820468ac99c ("net/ipn3ke: support TM")
Cc: rosen.xu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/ipn3ke/ipn3ke_ethdev.h | 7 -------
 drivers/net/ipn3ke/ipn3ke_tm.c     | 1 -
 2 files changed, 8 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.h b/drivers/net/ipn3ke/ipn3ke_ethdev.h
index c7b336bbd..0d71dcd64 100644
--- a/drivers/net/ipn3ke/ipn3ke_ethdev.h
+++ b/drivers/net/ipn3ke/ipn3ke_ethdev.h
@@ -97,18 +97,11 @@ struct ipn3ke_tm_node {
 struct ipn3ke_tm_hierarchy {
 	struct ipn3ke_tm_node *port_node;
-	/*struct ipn3ke_tm_node_list vt_node_list;*/
-	/*struct ipn3ke_tm_node_list cos_node_list;*/
-
 	uint32_t n_shaper_profiles;
-	/*uint32_t n_shared_shapers;*/
 	uint32_t n_tdrop_profiles;
 	uint32_t n_vt_nodes;
 	uint32_t n_cos_nodes;
-
 	struct ipn3ke_tm_node *port_commit_node;
 	struct ipn3ke_tm_node_list vt_commit_node_list;
 	struct ipn3ke_tm_node_list cos_commit_node_list;
-
-	/*uint32_t n_tm_nodes[IPN3KE_TM_NODE_LEVEL_MAX];*/
 };
 
diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
index a93145d59..5a16c5f96 100644
--- a/drivers/net/ipn3ke/ipn3ke_tm.c
+++ b/drivers/net/ipn3ke/ipn3ke_tm.c
@@ -1088,5 +1088,4 @@ ipn3ke_tm_node_add_check_mount(uint32_t tm_id,
 	struct rte_tm_error *error)
 {
-	/*struct ipn3ke_tm_internals *tm = IPN3KE_DEV_PRIVATE_TO_TM(dev);*/
 	uint32_t node_index;
 	uint32_t parent_index;
-- 
2.21.0

[dpdk-dev] [PATCH 7/9] compress/octeontx: remove commented out code

[Kevin Traynor]

This code is commented out. Remove it.

Fixes: 43e610bb8565 ("compress/octeontx: introduce octeontx zip PMD")
Cc: ssahu@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/compress/octeontx/include/zip_regs.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/compress/octeontx/include/zip_regs.h b/drivers/compress/octeontx/include/zip_regs.h
index 04c3d75e9..96e538bb7 100644
--- a/drivers/compress/octeontx/include/zip_regs.h
+++ b/drivers/compress/octeontx/include/zip_regs.h
@@ -37,5 +37,4 @@ typedef union {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_vqx_ena_s cn; */
 } zip_vqx_ena_t;
 
@@ -65,5 +64,4 @@ typedef union {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_vqx_sbuf_addr_s cn; */
 } zip_vqx_sbuf_addr_t;
 
@@ -85,5 +83,4 @@ typedef union {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_quex_doorbell_s cn; */
 } zip_quex_doorbell_t;
 
@@ -105,5 +102,4 @@ union zip_nptr_s {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_nptr_s_s cn83xx; */
 };
 
@@ -198,5 +194,4 @@ union zip_inst_s {
 		/** Beginning of file */
 		uint64_t bf                    : 1;
-		// uint64_t reserved_3_4          : 2;
 		/** Comp/decomp operation */
 		uint64_t op                    : 2;
@@ -211,5 +206,4 @@ union zip_inst_s {
 		uint64_t dg                    : 1;
 		uint64_t ds                    : 1;
-		//uint64_t reserved_3_4          : 2;
 		uint64_t op                    : 2;
 		uint64_t bf                    : 1;
@@ -616,6 +610,4 @@ union zip_zres_s {
 #endif /* Word 7 - End */
 	} /** ZIP Result Structure */s;
-
-	/* struct zip_zres_s_s cn83xx; */
 };
 
-- 
2.21.0

[dpdk-dev] [PATCH 8/9] event/opdl: remove commented out code

[Kevin Traynor]

Some variables are commented out. Remove them.

Fixes: d548ef513cd7 ("event/opdl: add unit tests")
Cc: liang.j.ma@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/event/opdl/opdl_test.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/event/opdl/opdl_test.c b/drivers/event/opdl/opdl_test.c
index 5868ec1be..e7a32fbd3 100644
--- a/drivers/event/opdl/opdl_test.c
+++ b/drivers/event/opdl/opdl_test.c
@@ -696,7 +696,4 @@ static int
 single_link(struct test *t)
 {
-	/* const uint8_t rx_port = 0; */
-	/* const uint8_t w1_port = 1; */
-	/* const uint8_t w3_port = 3; */
 	const uint8_t tx_port = 2;
 	int err;
-- 
2.21.0

[dpdk-dev] [PATCH 9/9] net/bnxt: remove commented out code

[Kevin Traynor]

This commented out todo and code is old. Remove it.

Fixes: b7435d660a8c ("net/bnxt: add ntuple filtering support")
Cc: ajit.khaparde@broadcom.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 6685ee7d9..318d49857 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -2239,7 +2239,4 @@ parse_ntuple_filter(struct bnxt *bp,
 	}
 
-	//TODO Priority
-	//nfilter->priority = (uint8_t)filter->priority;
-
 	bfilter->enables = en;
 	return 0;
-- 
2.21.0

Re: [dpdk-dev] [PATCH 9/9] net/bnxt: remove commented out code

[Ajit Khaparde]

On Tue, Oct 1, 2019 at 6:04 AM Kevin Traynor <ktraynor@redhat.com> wrote:

> This commented out todo and code is old. Remove it.
>
> Fixes: b7435d660a8c ("net/bnxt: add ntuple filtering support")
> Cc: ajit.khaparde@broadcom.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

---
>  drivers/net/bnxt/bnxt_ethdev.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c
> index 6685ee7d9..318d49857 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -2239,7 +2239,4 @@ parse_ntuple_filter(struct bnxt *bp,
>         }
>
> -       //TODO Priority
> -       //nfilter->priority = (uint8_t)filter->priority;
> -
>         bfilter->enables = en;
>         return 0;
> --
> 2.21.0
>
>

Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code

[Ajit Khaparde]

On Tue, Oct 1, 2019 at 6:04 AM Kevin Traynor <ktraynor@redhat.com> wrote:

> If rc contains a non-zero return code from bnxt_hwrm_send_message(),
> HWRM_CHECK_RESULT_SILENT() will return.
>
> Just after that code, there is an 'if (!rc) {...} else {...}'.
> Coverity is complaining that this if else statement is dead code as
> rc will always be 0 if that code is reached.
>
> 4309        rc = bnxt_hwrm_send_message(bp, &req, sizeof(req),
>                    BNXT_USE_CHIMP_MB);
>     cond_const: Condition rc, taking false branch.
>     Now the value of rc is equal to 0.
> 4310        HWRM_CHECK_RESULT_SILENT();
> 4311
>     const: At condition rc, the value of rc must be equal to 0.
>     dead_error_condition: The condition !rc must be true.
> 4312        if (!rc) {
>
> [snip]
>
> 4373        } else {
>     CID 343450 (#1 of 1): Logically dead code
>     (DEADCODE)dead_error_line: Execution cannot
>     reach this statement: rc = 0;.
> 4374                rc = 0;
> 4375        }
>
> Coverity issue: 343450
> Fixes: f8168ca0e690 ("net/bnxt: support thor controller")
> Cc: lance.richardson@broadcom.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>


> ---
>  drivers/net/bnxt/bnxt_hwrm.c | 118 +++++++++++++++++------------------
>  1 file changed, 56 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
> index 9883fb506..5378e3e9c 100644
> --- a/drivers/net/bnxt/bnxt_hwrm.c
> +++ b/drivers/net/bnxt/bnxt_hwrm.c
> @@ -4298,5 +4298,7 @@ int bnxt_hwrm_func_backing_store_qcaps(struct bnxt
> *bp)
>         struct hwrm_func_backing_store_qcaps_output *resp =
>                 bp->hwrm_cmd_resp_addr;
> -       int rc;
> +       struct bnxt_ctx_pg_info *ctx_pg;
> +       struct bnxt_ctx_mem_info *ctx;
> +       int rc, total_alloc_len, i;
>
>         if (!BNXT_CHIP_THOR(bp) ||
> @@ -4310,68 +4312,60 @@ int bnxt_hwrm_func_backing_store_qcaps(struct bnxt
> *bp)
>         HWRM_CHECK_RESULT_SILENT();
>
> -       if (!rc) {
> -               struct bnxt_ctx_pg_info *ctx_pg;
> -               struct bnxt_ctx_mem_info *ctx;
> -               int total_alloc_len;
> -               int i;
> +       total_alloc_len = sizeof(*ctx);
> +       ctx = rte_malloc("bnxt_ctx_mem", total_alloc_len,
> +                        RTE_CACHE_LINE_SIZE);
> +       if (!ctx) {
> +               rc = -ENOMEM;
> +               goto ctx_err;
> +       }
> +       memset(ctx, 0, total_alloc_len);
>
> -               total_alloc_len = sizeof(*ctx);
> -               ctx = rte_malloc("bnxt_ctx_mem", total_alloc_len,
> -                                RTE_CACHE_LINE_SIZE);
> -               if (!ctx) {
> -                       rc = -ENOMEM;
> -                       goto ctx_err;
> -               }
> -               memset(ctx, 0, total_alloc_len);
> +       ctx_pg = rte_malloc("bnxt_ctx_pg_mem",
> +                           sizeof(*ctx_pg) * BNXT_MAX_Q,
> +                           RTE_CACHE_LINE_SIZE);
> +       if (!ctx_pg) {
> +               rc = -ENOMEM;
> +               goto ctx_err;
> +       }
> +       for (i = 0; i < BNXT_MAX_Q; i++, ctx_pg++)
> +               ctx->tqm_mem[i] = ctx_pg;
>
> -               ctx_pg = rte_malloc("bnxt_ctx_pg_mem",
> -                                   sizeof(*ctx_pg) * BNXT_MAX_Q,
> -                                   RTE_CACHE_LINE_SIZE);
> -               if (!ctx_pg) {
> -                       rc = -ENOMEM;
> -                       goto ctx_err;
> -               }
> -               for (i = 0; i < BNXT_MAX_Q; i++, ctx_pg++)
> -                       ctx->tqm_mem[i] = ctx_pg;
> +       bp->ctx = ctx;
> +       ctx->qp_max_entries = rte_le_to_cpu_32(resp->qp_max_entries);
> +       ctx->qp_min_qp1_entries =
> +               rte_le_to_cpu_16(resp->qp_min_qp1_entries);
> +       ctx->qp_max_l2_entries =
> +               rte_le_to_cpu_16(resp->qp_max_l2_entries);
> +       ctx->qp_entry_size = rte_le_to_cpu_16(resp->qp_entry_size);
> +       ctx->srq_max_l2_entries =
> +               rte_le_to_cpu_16(resp->srq_max_l2_entries);
> +       ctx->srq_max_entries = rte_le_to_cpu_32(resp->srq_max_entries);
> +       ctx->srq_entry_size = rte_le_to_cpu_16(resp->srq_entry_size);
> +       ctx->cq_max_l2_entries =
> +               rte_le_to_cpu_16(resp->cq_max_l2_entries);
> +       ctx->cq_max_entries = rte_le_to_cpu_32(resp->cq_max_entries);
> +       ctx->cq_entry_size = rte_le_to_cpu_16(resp->cq_entry_size);
> +       ctx->vnic_max_vnic_entries =
> +               rte_le_to_cpu_16(resp->vnic_max_vnic_entries);
> +       ctx->vnic_max_ring_table_entries =
> +               rte_le_to_cpu_16(resp->vnic_max_ring_table_entries);
> +       ctx->vnic_entry_size = rte_le_to_cpu_16(resp->vnic_entry_size);
> +       ctx->stat_max_entries =
> +               rte_le_to_cpu_32(resp->stat_max_entries);
> +       ctx->stat_entry_size = rte_le_to_cpu_16(resp->stat_entry_size);
> +       ctx->tqm_entry_size = rte_le_to_cpu_16(resp->tqm_entry_size);
> +       ctx->tqm_min_entries_per_ring =
> +               rte_le_to_cpu_32(resp->tqm_min_entries_per_ring);
> +       ctx->tqm_max_entries_per_ring =
> +               rte_le_to_cpu_32(resp->tqm_max_entries_per_ring);
> +       ctx->tqm_entries_multiple = resp->tqm_entries_multiple;
> +       if (!ctx->tqm_entries_multiple)
> +               ctx->tqm_entries_multiple = 1;
> +       ctx->mrav_max_entries =
> +               rte_le_to_cpu_32(resp->mrav_max_entries);
> +       ctx->mrav_entry_size = rte_le_to_cpu_16(resp->mrav_entry_size);
> +       ctx->tim_entry_size = rte_le_to_cpu_16(resp->tim_entry_size);
> +       ctx->tim_max_entries = rte_le_to_cpu_32(resp->tim_max_entries);
>
> -               bp->ctx = ctx;
> -               ctx->qp_max_entries =
> rte_le_to_cpu_32(resp->qp_max_entries);
> -               ctx->qp_min_qp1_entries =
> -                       rte_le_to_cpu_16(resp->qp_min_qp1_entries);
> -               ctx->qp_max_l2_entries =
> -                       rte_le_to_cpu_16(resp->qp_max_l2_entries);
> -               ctx->qp_entry_size = rte_le_to_cpu_16(resp->qp_entry_size);
> -               ctx->srq_max_l2_entries =
> -                       rte_le_to_cpu_16(resp->srq_max_l2_entries);
> -               ctx->srq_max_entries =
> rte_le_to_cpu_32(resp->srq_max_entries);
> -               ctx->srq_entry_size =
> rte_le_to_cpu_16(resp->srq_entry_size);
> -               ctx->cq_max_l2_entries =
> -                       rte_le_to_cpu_16(resp->cq_max_l2_entries);
> -               ctx->cq_max_entries =
> rte_le_to_cpu_32(resp->cq_max_entries);
> -               ctx->cq_entry_size = rte_le_to_cpu_16(resp->cq_entry_size);
> -               ctx->vnic_max_vnic_entries =
> -                       rte_le_to_cpu_16(resp->vnic_max_vnic_entries);
> -               ctx->vnic_max_ring_table_entries =
> -
>  rte_le_to_cpu_16(resp->vnic_max_ring_table_entries);
> -               ctx->vnic_entry_size =
> rte_le_to_cpu_16(resp->vnic_entry_size);
> -               ctx->stat_max_entries =
> -                       rte_le_to_cpu_32(resp->stat_max_entries);
> -               ctx->stat_entry_size =
> rte_le_to_cpu_16(resp->stat_entry_size);
> -               ctx->tqm_entry_size =
> rte_le_to_cpu_16(resp->tqm_entry_size);
> -               ctx->tqm_min_entries_per_ring =
> -                       rte_le_to_cpu_32(resp->tqm_min_entries_per_ring);
> -               ctx->tqm_max_entries_per_ring =
> -                       rte_le_to_cpu_32(resp->tqm_max_entries_per_ring);
> -               ctx->tqm_entries_multiple = resp->tqm_entries_multiple;
> -               if (!ctx->tqm_entries_multiple)
> -                       ctx->tqm_entries_multiple = 1;
> -               ctx->mrav_max_entries =
> -                       rte_le_to_cpu_32(resp->mrav_max_entries);
> -               ctx->mrav_entry_size =
> rte_le_to_cpu_16(resp->mrav_entry_size);
> -               ctx->tim_entry_size =
> rte_le_to_cpu_16(resp->tim_entry_size);
> -               ctx->tim_max_entries =
> rte_le_to_cpu_32(resp->tim_max_entries);
> -       } else {
> -               rc = 0;
> -       }
>  ctx_err:
>         HWRM_UNLOCK();
> --
> 2.21.0
>
>

Re: [dpdk-dev] [PATCH 8/9] event/opdl: remove commented out code

[Liang, Ma]

On 01 Oct 14:04, Kevin Traynor wrote:
> Some variables are commented out. Remove them.
> 
> Fixes: d548ef513cd7 ("event/opdl: add unit tests")
> Cc: liang.j.ma@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/event/opdl/opdl_test.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/event/opdl/opdl_test.c b/drivers/event/opdl/opdl_test.c
> index 5868ec1be..e7a32fbd3 100644
> --- a/drivers/event/opdl/opdl_test.c
> +++ b/drivers/event/opdl/opdl_test.c
> @@ -696,7 +696,4 @@ static int
>  single_link(struct test *t)
>  {
> -	/* const uint8_t rx_port = 0; */
> -	/* const uint8_t w1_port = 1; */
> -	/* const uint8_t w3_port = 3; */
>  	const uint8_t tx_port = 2;
>  	int err;
> -- 
> 2.21.0
> 
Acked-by:  Liang Ma  <liang.j.ma@intel.com>

Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

[Ferriter, Cian]

 

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday 1 October 2019 13:53
> To: dev@dpdk.org
> Cc: Kevin Traynor <ktraynor@redhat.com>; Ferriter, Cian
> <cian.ferriter@intel.com>; stable@dpdk.org
> Subject: [PATCH 1/9] net/pcap: fix argument checks
> 
> Previously rx/tx_queues were passed into eth_from_pcaps_common() as
> ptrs and were checked for being NULL.
> 
> In commit da6ba28f0540 ("net/pcap: use a struct to pass user options") that
> changed to pass in a ptr to a pmd_devargs_all which contains the
> rx/tx_queues.
> 
> The parameter checking was not updated as part of that commit and coverity
> caught that there was still a check if rx/tx_queues were NULL, apparently
> after they had been dereferenced.
> 
> Fix that by checking the pmd_devargs_all ptr and removing the NULL checks
> on rx/tx_queues.
> 
> 1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
> 1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
> 1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
>     deref_ptr: Directly dereferencing pointer tx_queues.
> 1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
> 1235        unsigned int i;
> 1236
> 1237        /* do some parameter checking */
>     CID 345004: Dereference before null check (REVERSE_INULL)
>     [select issue]
> 1238        if (rx_queues == NULL && nb_rx_queues > 0)
> 1239                return -1;
>     CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
>     check_after_deref: Null-checking tx_queues suggests that it may be
>     null, but it has already been dereferenced on all paths leading to
>     the check.
> 1240        if (tx_queues == NULL && nb_tx_queues > 0)
> 1241                return -1;
> 
> Coverity issue: 345029
> Coverity issue: 345044
> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
> Cc: cian.ferriter@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

Acked-by: Cian Ferriter <cian.ferriter@intel.com>


> ---
>  drivers/net/pcap/rte_eth_pcap.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c
> b/drivers/net/pcap/rte_eth_pcap.c index 5489010b6..7cf00306e 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -1229,16 +1229,19 @@ eth_from_pcaps_common(struct
> rte_vdev_device *vdev,  {
>  	struct pmd_process_private *pp;
> -	struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
> -	struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
> -	const unsigned int nb_rx_queues = rx_queues->num_of_queue;
> -	const unsigned int nb_tx_queues = tx_queues->num_of_queue;
> +	struct pmd_devargs *rx_queues;
> +	struct pmd_devargs *tx_queues;
> +	unsigned int nb_rx_queues;
> +	unsigned int nb_tx_queues;
>  	unsigned int i;
> 
> -	/* do some parameter checking */
> -	if (rx_queues == NULL && nb_rx_queues > 0)
> -		return -1;
> -	if (tx_queues == NULL && nb_tx_queues > 0)
> +	if (devargs_all == NULL)
>  		return -1;
> 
> +	rx_queues = &devargs_all->rx_queues;
> +	tx_queues = &devargs_all->tx_queues;
> +
> +	nb_rx_queues = rx_queues->num_of_queue;
> +	nb_tx_queues = tx_queues->num_of_queue;
> +
>  	if (pmd_init_internals(vdev, nb_rx_queues, nb_tx_queues,
> internals,
>  			eth_dev) < 0)
> --
> 2.21.0

Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code

[David Marchand]

Hello Ajit, Kevin,

On Wed, Oct 2, 2019 at 3:29 AM Ajit Khaparde <ajit.khaparde@broadcom.com> wrote:
>
> On Tue, Oct 1, 2019 at 6:04 AM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> > If rc contains a non-zero return code from bnxt_hwrm_send_message(),
> > HWRM_CHECK_RESULT_SILENT() will return.
> >
> > Just after that code, there is an 'if (!rc) {...} else {...}'.
> > Coverity is complaining that this if else statement is dead code as
> > rc will always be 0 if that code is reached.
> >
> > 4309        rc = bnxt_hwrm_send_message(bp, &req, sizeof(req),
> >                    BNXT_USE_CHIMP_MB);
> >     cond_const: Condition rc, taking false branch.
> >     Now the value of rc is equal to 0.
> > 4310        HWRM_CHECK_RESULT_SILENT();
> > 4311
> >     const: At condition rc, the value of rc must be equal to 0.
> >     dead_error_condition: The condition !rc must be true.
> > 4312        if (!rc) {
> >
> > [snip]
> >
> > 4373        } else {
> >     CID 343450 (#1 of 1): Logically dead code
> >     (DEADCODE)dead_error_line: Execution cannot
> >     reach this statement: rc = 0;.
> > 4374                rc = 0;
> > 4375        }
> >
> > Coverity issue: 343450
> > Fixes: f8168ca0e690 ("net/bnxt: support thor controller")
> > Cc: lance.richardson@broadcom.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> >
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

I can see a *really* close patch has been submitted the day after.
http://patchwork.dpdk.org/patch/58352/

And merged after a v3:
https://git.dpdk.org/dpdk/commit/?id=b4f74051165560aa81814433dea7e6eb0bdb32b9

So I suppose this patch can be dropped.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

[David Marchand]

On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> Previously rx/tx_queues were passed into eth_from_pcaps_common()
> as ptrs and were checked for being NULL.
>
> In commit da6ba28f0540 ("net/pcap: use a struct to pass user options")
> that changed to pass in a ptr to a pmd_devargs_all which contains
> the rx/tx_queues.
>
> The parameter checking was not updated as part of that commit and
> coverity caught that there was still a check if rx/tx_queues were
> NULL, apparently after they had been dereferenced.
>
> Fix that by checking the pmd_devargs_all ptr and removing the NULL
> checks on rx/tx_queues.
>
> 1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
> 1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
> 1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
>     deref_ptr: Directly dereferencing pointer tx_queues.
> 1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
> 1235        unsigned int i;
> 1236
> 1237        /* do some parameter checking */
>     CID 345004: Dereference before null check (REVERSE_INULL)
>     [select issue]
> 1238        if (rx_queues == NULL && nb_rx_queues > 0)
> 1239                return -1;
>     CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
>     check_after_deref: Null-checking tx_queues suggests that it may be
>     null, but it has already been dereferenced on all paths leading to
>     the check.
> 1240        if (tx_queues == NULL && nb_tx_queues > 0)
> 1241                return -1;
>
> Coverity issue: 345029
> Coverity issue: 345044
> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
> Cc: cian.ferriter@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

This patch hides the coverity warning.
But can't we just remove those checks?

Iiuc, the checks on NULL pointers are unnecessary since
https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebef7a3e60b.


-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/9] crypto/octeontx: fix possible NULL deference

[David Marchand]

On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> Coverity complains that ctrl_flags is set to NULL at the start
> of the function and it may not have been set before there is a
> jump to fc_success and it is dereferenced.
>
> Check for NULL before dereference.
>
> 312fc_success:
>    CID 344983 (#1 of 1): Explicit null dereferenced
>    (FORWARD_NULL)7. var_deref_op: Dereferencing null pointer ctrl_flags.
> 313        *ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
>
> Coverity issue: 344983
> Fixes: 6cc54096520d ("crypto/octeontx: add supported sessions")
> Cc: ndabilpuram@marvell.com
> Cc: stable@dpdk.org

Cc: maintainer

>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>
> ---
>
> There may be further rework needed to set it to the correct value,
> but for now at least prevent the NULL dereference.
> ---
>  drivers/common/cpt/cpt_ucode.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/common/cpt/cpt_ucode.h b/drivers/common/cpt/cpt_ucode.h
> index 7d9c31e17..fad978c6e 100644
> --- a/drivers/common/cpt/cpt_ucode.h
> +++ b/drivers/common/cpt/cpt_ucode.h
> @@ -311,5 +311,6 @@ cpt_fc_ciph_set_key(void *ctx, cipher_type_t type, const uint8_t *key,
>
>  fc_success:
> -       *ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
> +       if (ctrl_flags != NULL)
> +               *ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
>
>  success:
> --
> 2.21.0
>

Reviewed-by: David Marchand <david.marchand@redhat.com>

-- 
David Marchand

Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[David Marchand]

Hello Rosen,

Review please.

On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> Coverity is complaining about identical code regardless of which branch
> of the if else is taken. Functionally it means an error will always be
> returned if this if else is hit. Remove the else branch.
>
>     CID 337928 (#1 of 1): Identical code for different branches
>     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>     n->n_children != 0U is true, because the 'then' and 'else' branches
>     are identical. Should one of the branches be modified, or the entire
>     'if' statement replaced?
> 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> 1507          n->n_children != 0) {
> 1508          return -rte_tm_error_set(error,
> 1509                  EINVAL,
> 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> 1511                  NULL,
> 1512                  rte_strerror(EINVAL));
>     else_branch: The else branch, identical to the then branch.
> 1513  } else {
> 1514          return -rte_tm_error_set(error,
> 1515                  EINVAL,
> 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> 1517                  NULL,
> 1518                  rte_strerror(EINVAL));
> 1519  }
>
> Coverity issue: 337928
> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
> index adf02c157..a93145d59 100644
> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct rte_eth_dev *dev,
>                                                 NULL,
>                                                 rte_strerror(EINVAL));
> -                       } else {
> -                               return -rte_tm_error_set(error,
> -                                               EINVAL,
> -                                               RTE_TM_ERROR_TYPE_UNSPECIFIED,
> -                                               NULL,
> -                                               rte_strerror(EINVAL));
>                         }
>                 }
> --
> 2.21.0
>

-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 5/9] net/ipn3ke: remove useless if statement

[David Marchand]

On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> Coverity complains that this statement is not needed as the goto
> label is on the next line anyway. Remove the if statement.
>
> 653        ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
>    CID 337930 (#1 of 1): Identical code for different branches
>    (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>    when the condition ret is true or false, because the code in the
>    if-then branch and after the if statement is identical. Should
>    the if statement be removed?
> 654        if (ret)
> 655                goto end;
>    implicit_else: The code from the above if-then branch is identical
>    to the code after the if statement.
> 656end:
>
> Coverity issue: 337930
> Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.c b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> index c226d6313..282295f49 100644
> --- a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> @@ -652,6 +652,5 @@ ipn3ke_cfg_probe(struct rte_vdev_device *dev)
>
>         ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
> -       if (ret)
> -               goto end;
> +
>  end:
>         if (kvlist)
> --
> 2.21.0
>

Reviewed-by: David Marchand <david.marchand@redhat.com>

-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 6/9] net/ipn3ke: remove commented out code

[David Marchand]

On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> These struct members and variable were commented out. Remove them.
>
> Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_ethdev.h | 7 -------
>  drivers/net/ipn3ke/ipn3ke_tm.c     | 1 -
>  2 files changed, 8 deletions(-)
>
> diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.h b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> index c7b336bbd..0d71dcd64 100644
> --- a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> @@ -97,18 +97,11 @@ struct ipn3ke_tm_node {
>  struct ipn3ke_tm_hierarchy {
>         struct ipn3ke_tm_node *port_node;
> -       /*struct ipn3ke_tm_node_list vt_node_list;*/
> -       /*struct ipn3ke_tm_node_list cos_node_list;*/
> -
>         uint32_t n_shaper_profiles;
> -       /*uint32_t n_shared_shapers;*/
>         uint32_t n_tdrop_profiles;
>         uint32_t n_vt_nodes;
>         uint32_t n_cos_nodes;
> -
>         struct ipn3ke_tm_node *port_commit_node;
>         struct ipn3ke_tm_node_list vt_commit_node_list;
>         struct ipn3ke_tm_node_list cos_commit_node_list;
> -
> -       /*uint32_t n_tm_nodes[IPN3KE_TM_NODE_LEVEL_MAX];*/
>  };
>
> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
> index a93145d59..5a16c5f96 100644
> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> @@ -1088,5 +1088,4 @@ ipn3ke_tm_node_add_check_mount(uint32_t tm_id,
>         struct rte_tm_error *error)
>  {
> -       /*struct ipn3ke_tm_internals *tm = IPN3KE_DEV_PRIVATE_TO_TM(dev);*/
>         uint32_t node_index;
>         uint32_t parent_index;
> --
> 2.21.0
>

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [dpdk-dev] [PATCH 7/9] compress/octeontx: remove commented out code

[David Marchand]

On Tue, Oct 1, 2019 at 3:05 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> This code is commented out. Remove it.
>
> Fixes: 43e610bb8565 ("compress/octeontx: introduce octeontx zip PMD")
> Cc: ssahu@marvell.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/compress/octeontx/include/zip_regs.h | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/drivers/compress/octeontx/include/zip_regs.h b/drivers/compress/octeontx/include/zip_regs.h
> index 04c3d75e9..96e538bb7 100644
> --- a/drivers/compress/octeontx/include/zip_regs.h
> +++ b/drivers/compress/octeontx/include/zip_regs.h
> @@ -37,5 +37,4 @@ typedef union {
>  #endif /* Word 0 - End */
>         } s;
> -       /* struct zip_vqx_ena_s cn; */
>  } zip_vqx_ena_t;
>
> @@ -65,5 +64,4 @@ typedef union {
>  #endif /* Word 0 - End */
>         } s;
> -       /* struct zip_vqx_sbuf_addr_s cn; */
>  } zip_vqx_sbuf_addr_t;
>
> @@ -85,5 +83,4 @@ typedef union {
>  #endif /* Word 0 - End */
>         } s;
> -       /* struct zip_quex_doorbell_s cn; */
>  } zip_quex_doorbell_t;
>
> @@ -105,5 +102,4 @@ union zip_nptr_s {
>  #endif /* Word 0 - End */
>         } s;
> -       /* struct zip_nptr_s_s cn83xx; */
>  };
>
> @@ -198,5 +194,4 @@ union zip_inst_s {
>                 /** Beginning of file */
>                 uint64_t bf                    : 1;
> -               // uint64_t reserved_3_4          : 2;
>                 /** Comp/decomp operation */
>                 uint64_t op                    : 2;
> @@ -211,5 +206,4 @@ union zip_inst_s {
>                 uint64_t dg                    : 1;
>                 uint64_t ds                    : 1;
> -               //uint64_t reserved_3_4          : 2;
>                 uint64_t op                    : 2;
>                 uint64_t bf                    : 1;
> @@ -616,6 +610,4 @@ union zip_zres_s {
>  #endif /* Word 7 - End */
>         } /** ZIP Result Structure */s;
> -
> -       /* struct zip_zres_s_s cn83xx; */
>  };
>
> --
> 2.21.0
>

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code

[Ajit Khaparde]

On Wed, Oct 30, 2019 at 12:43 AM David Marchand <david.marchand@redhat.com>
wrote:

> Hello Ajit, Kevin,
>
> On Wed, Oct 2, 2019 at 3:29 AM Ajit Khaparde <ajit.khaparde@broadcom.com>
> wrote:
> >
> > On Tue, Oct 1, 2019 at 6:04 AM Kevin Traynor <ktraynor@redhat.com>
> wrote:
> >
> > > If rc contains a non-zero return code from bnxt_hwrm_send_message(),
> > > HWRM_CHECK_RESULT_SILENT() will return.
> > >
> > > Just after that code, there is an 'if (!rc) {...} else {...}'.
> > > Coverity is complaining that this if else statement is dead code as
> > > rc will always be 0 if that code is reached.
> > >
> > > 4309        rc = bnxt_hwrm_send_message(bp, &req, sizeof(req),
> > >                    BNXT_USE_CHIMP_MB);
> > >     cond_const: Condition rc, taking false branch.
> > >     Now the value of rc is equal to 0.
> > > 4310        HWRM_CHECK_RESULT_SILENT();
> > > 4311
> > >     const: At condition rc, the value of rc must be equal to 0.
> > >     dead_error_condition: The condition !rc must be true.
> > > 4312        if (!rc) {
> > >
> > > [snip]
> > >
> > > 4373        } else {
> > >     CID 343450 (#1 of 1): Logically dead code
> > >     (DEADCODE)dead_error_line: Execution cannot
> > >     reach this statement: rc = 0;.
> > > 4374                rc = 0;
> > > 4375        }
> > >
> > > Coverity issue: 343450
> > > Fixes: f8168ca0e690 ("net/bnxt: support thor controller")
> > > Cc: lance.richardson@broadcom.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> > >
> > Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> I can see a *really* close patch has been submitted the day after.
> http://patchwork.dpdk.org/patch/58352/
>
> And merged after a v3:
>
> https://git.dpdk.org/dpdk/commit/?id=b4f74051165560aa81814433dea7e6eb0bdb32b9
>
> So I suppose this patch can be dropped.
>
> Yes. That makes sense.
Thanks for checking.

Thanks
Ajit

>
> --
> David Marchand
>

Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code

[Kevin Traynor]

On 30/10/2019 16:27, Ajit Khaparde wrote:
> On Wed, Oct 30, 2019 at 12:43 AM David Marchand <david.marchand@redhat.com>
> wrote:
> 
>> Hello Ajit, Kevin,
>>
>> On Wed, Oct 2, 2019 at 3:29 AM Ajit Khaparde <ajit.khaparde@broadcom.com>
>> wrote:
>>>
>>> On Tue, Oct 1, 2019 at 6:04 AM Kevin Traynor <ktraynor@redhat.com>
>> wrote:
>>>
>>>> If rc contains a non-zero return code from bnxt_hwrm_send_message(),
>>>> HWRM_CHECK_RESULT_SILENT() will return.
>>>>
>>>> Just after that code, there is an 'if (!rc) {...} else {...}'.
>>>> Coverity is complaining that this if else statement is dead code as
>>>> rc will always be 0 if that code is reached.
>>>>
>>>> 4309        rc = bnxt_hwrm_send_message(bp, &req, sizeof(req),
>>>>                    BNXT_USE_CHIMP_MB);
>>>>     cond_const: Condition rc, taking false branch.
>>>>     Now the value of rc is equal to 0.
>>>> 4310        HWRM_CHECK_RESULT_SILENT();
>>>> 4311
>>>>     const: At condition rc, the value of rc must be equal to 0.
>>>>     dead_error_condition: The condition !rc must be true.
>>>> 4312        if (!rc) {
>>>>
>>>> [snip]
>>>>
>>>> 4373        } else {
>>>>     CID 343450 (#1 of 1): Logically dead code
>>>>     (DEADCODE)dead_error_line: Execution cannot
>>>>     reach this statement: rc = 0;.
>>>> 4374                rc = 0;
>>>> 4375        }
>>>>
>>>> Coverity issue: 343450
>>>> Fixes: f8168ca0e690 ("net/bnxt: support thor controller")
>>>> Cc: lance.richardson@broadcom.com
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>>>>
>>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>
>> I can see a *really* close patch has been submitted the day after.
>> http://patchwork.dpdk.org/patch/58352/
>>
>> And merged after a v3:
>>
>> https://git.dpdk.org/dpdk/commit/?id=b4f74051165560aa81814433dea7e6eb0bdb32b9
>>
>> So I suppose this patch can be dropped.
>>
>> Yes. That makes sense.
> Thanks for checking.
> 
> Thanks
> Ajit
> 

Thanks for catching it David.

>>
>> --
>> David Marchand
>>
> 

Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[Kevin Traynor]

On 30/10/2019 07:59, David Marchand wrote:
> Hello Rosen,
> 
> Review please.
> 

Ping Rosen.

> On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>>
>> Coverity is complaining about identical code regardless of which branch
>> of the if else is taken. Functionally it means an error will always be
>> returned if this if else is hit. Remove the else branch.
>>
>>     CID 337928 (#1 of 1): Identical code for different branches
>>     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>>     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>>     n->n_children != 0U is true, because the 'then' and 'else' branches
>>     are identical. Should one of the branches be modified, or the entire
>>     'if' statement replaced?
>> 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>> 1507          n->n_children != 0) {
>> 1508          return -rte_tm_error_set(error,
>> 1509                  EINVAL,
>> 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
>> 1511                  NULL,
>> 1512                  rte_strerror(EINVAL));
>>     else_branch: The else branch, identical to the then branch.
>> 1513  } else {
>> 1514          return -rte_tm_error_set(error,
>> 1515                  EINVAL,
>> 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
>> 1517                  NULL,
>> 1518                  rte_strerror(EINVAL));
>> 1519  }
>>
>> Coverity issue: 337928
>> Fixes: c820468ac99c ("net/ipn3ke: support TM")
>> Cc: rosen.xu@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>> ---
>>  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
>> index adf02c157..a93145d59 100644
>> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
>> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
>> @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct rte_eth_dev *dev,
>>                                                 NULL,
>>                                                 rte_strerror(EINVAL));
>> -                       } else {
>> -                               return -rte_tm_error_set(error,
>> -                                               EINVAL,
>> -                                               RTE_TM_ERROR_TYPE_UNSPECIFIED,
>> -                                               NULL,
>> -                                               rte_strerror(EINVAL));
>>                         }
>>                 }
>> --
>> 2.21.0
>>
> 

Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

[Kevin Traynor]

On 30/10/2019 07:52, David Marchand wrote:
> On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>>
>> Previously rx/tx_queues were passed into eth_from_pcaps_common()
>> as ptrs and were checked for being NULL.
>>
>> In commit da6ba28f0540 ("net/pcap: use a struct to pass user options")
>> that changed to pass in a ptr to a pmd_devargs_all which contains
>> the rx/tx_queues.
>>
>> The parameter checking was not updated as part of that commit and
>> coverity caught that there was still a check if rx/tx_queues were
>> NULL, apparently after they had been dereferenced.
>>
>> Fix that by checking the pmd_devargs_all ptr and removing the NULL
>> checks on rx/tx_queues.
>>
>> 1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
>> 1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
>> 1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
>>     deref_ptr: Directly dereferencing pointer tx_queues.
>> 1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
>> 1235        unsigned int i;
>> 1236
>> 1237        /* do some parameter checking */
>>     CID 345004: Dereference before null check (REVERSE_INULL)
>>     [select issue]
>> 1238        if (rx_queues == NULL && nb_rx_queues > 0)
>> 1239                return -1;
>>     CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
>>     check_after_deref: Null-checking tx_queues suggests that it may be
>>     null, but it has already been dereferenced on all paths leading to
>>     the check.
>> 1240        if (tx_queues == NULL && nb_tx_queues > 0)
>> 1241                return -1;
>>
>> Coverity issue: 345029
>> Coverity issue: 345044
>> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
>> Cc: cian.ferriter@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> 
> This patch hides the coverity warning.
> But can't we just remove those checks?
> 
> Iiuc, the checks on NULL pointers are unnecessary since
> https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebef7a3e60b.
> 
> 

Aside from the Coverity complaint about rxq/txq NULL check after use,
the checks didn't seem to make sense anymore as rxq/txq are part of
devargs_all struct now, so they were removed.

I added the NULL check on devargs_all to avoid a NULL deference while
getting them instead. The check isn't doing any harm, but looking at the
current paths to this fn. can see devargs_all would not be NULL, so I'm
ok to remove it too. Cian/Ferruh, wdyt?

Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

[Ferriter, Cian]

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday 5 November 2019 16:41
> To: David Marchand <david.marchand@redhat.com>
> Cc: dev <dev@dpdk.org>; Ferriter, Cian <cian.ferriter@intel.com>; dpdk
> stable <stable@dpdk.org>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
> 
> On 30/10/2019 07:52, David Marchand wrote:
> > On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com>
> wrote:
> >>
> >> Previously rx/tx_queues were passed into eth_from_pcaps_common() as
> >> ptrs and were checked for being NULL.
> >>
> >> In commit da6ba28f0540 ("net/pcap: use a struct to pass user
> >> options") that changed to pass in a ptr to a pmd_devargs_all which
> >> contains the rx/tx_queues.
> >>
> >> The parameter checking was not updated as part of that commit and
> >> coverity caught that there was still a check if rx/tx_queues were
> >> NULL, apparently after they had been dereferenced.
> >>
> >> Fix that by checking the pmd_devargs_all ptr and removing the NULL
> >> checks on rx/tx_queues.
> >>
> >> 1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
> >> 1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
> >> 1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
> >>     deref_ptr: Directly dereferencing pointer tx_queues.
> >> 1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
> >> 1235        unsigned int i;
> >> 1236
> >> 1237        /* do some parameter checking */
> >>     CID 345004: Dereference before null check (REVERSE_INULL)
> >>     [select issue]
> >> 1238        if (rx_queues == NULL && nb_rx_queues > 0)
> >> 1239                return -1;
> >>     CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
> >>     check_after_deref: Null-checking tx_queues suggests that it may be
> >>     null, but it has already been dereferenced on all paths leading to
> >>     the check.
> >> 1240        if (tx_queues == NULL && nb_tx_queues > 0)
> >> 1241                return -1;
> >>
> >> Coverity issue: 345029
> >> Coverity issue: 345044
> >> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
> >> Cc: cian.ferriter@intel.com
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> >
> > This patch hides the coverity warning.
> > But can't we just remove those checks?
> >
> > Iiuc, the checks on NULL pointers are unnecessary since
> >
> https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebe
> f7a3e60b.
> >
> >
> 
> Aside from the Coverity complaint about rxq/txq NULL check after use, the
> checks didn't seem to make sense anymore as rxq/txq are part of devargs_all
> struct now, so they were removed.
> 
> I added the NULL check on devargs_all to avoid a NULL deference while
> getting them instead. The check isn't doing any harm, but looking at the
> current paths to this fn. can see devargs_all would not be NULL, so I'm ok to
> remove it too. Cian/Ferruh, wdyt?

I'm OK to remove this. Looks like it's no longer necessary. Good find David!

[dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups

[Kevin Traynor]

Patches 1-4 are Coverity fixes.
Patches 5-8 are removing commented out code.

v2:
- 1/8: removed added NULL check for devargs_all
- 2/8: fixed headline to common/cpt
- v1 3/9 'net/bnxt: remove logically dead code'
  removed as duplicate patch already applied
 
Kevin Traynor (8):
  net/pcap: fix argument checks
  common/cpt: fix possible NULL deference
  net/ipn3ke: fix incorrect commit check logic
  net/ipn3ke: remove useless if statement
  net/ipn3ke: remove commented out code
  compress/octeontx: remove commented out code
  event/opdl: remove commented out code
  net/bnxt: remove commented out code

 drivers/common/cpt/cpt_ucode.h               | 3 ++-
 drivers/compress/octeontx/include/zip_regs.h | 8 --------
 drivers/event/opdl/opdl_test.c               | 3 ---
 drivers/net/bnxt/bnxt_ethdev.c               | 3 ---
 drivers/net/ipn3ke/ipn3ke_ethdev.c           | 3 +--
 drivers/net/ipn3ke/ipn3ke_ethdev.h           | 7 -------
 drivers/net/ipn3ke/ipn3ke_tm.c               | 7 -------
 drivers/net/pcap/rte_eth_pcap.c              | 6 ------
 8 files changed, 3 insertions(+), 37 deletions(-)

-- 
2.21.0

[dpdk-dev] [v2 PATCH 1/8] net/pcap: fix argument checks

[Kevin Traynor]

Previously rx/tx_queues were passed into eth_from_pcaps_common()
as ptrs and were checked for being NULL.

In commit da6ba28f0540 ("net/pcap: use a struct to pass user options")
that changed to pass in a ptr to a pmd_devargs_all which contains
the rx/tx_queues.

The parameter checking was not updated as part of that commit and
coverity caught that there was still a check if rx/tx_queues were
NULL, apparently after they had been dereferenced.

In fact as they are a members of the devargs_all struct, they will
not be NULL so remove those checks.

1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
    deref_ptr: Directly dereferencing pointer tx_queues.
1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
1235        unsigned int i;
1236
1237        /* do some parameter checking */
    CID 345004: Dereference before null check (REVERSE_INULL)
    [select issue]
1238        if (rx_queues == NULL && nb_rx_queues > 0)
1239                return -1;
    CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
    check_after_deref: Null-checking tx_queues suggests that it may be
    null, but it has already been dereferenced on all paths leading to
    the check.
1240        if (tx_queues == NULL && nb_tx_queues > 0)
1241                return -1;

Coverity issue: 345029
Coverity issue: 345044
Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Cian Ferriter <cian.ferriter@intel.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 5186d8fe5..aa7ef6fdb 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -1241,10 +1241,4 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev,
 	unsigned int i;
 
-	/* do some parameter checking */
-	if (rx_queues == NULL && nb_rx_queues > 0)
-		return -1;
-	if (tx_queues == NULL && nb_tx_queues > 0)
-		return -1;
-
 	if (pmd_init_internals(vdev, nb_rx_queues, nb_tx_queues, internals,
 			eth_dev) < 0)
-- 
2.21.0

[dpdk-dev] [v2 PATCH 2/8] common/cpt: fix possible NULL deference

[Kevin Traynor]

Coverity complains that ctrl_flags is set to NULL at the start
of the function and it may not have been set before there is a
jump to fc_success and it is dereferenced.

Check for NULL before dereference.

312fc_success:
   CID 344983 (#1 of 1): Explicit null dereferenced
   (FORWARD_NULL)7. var_deref_op: Dereferencing null pointer ctrl_flags.
313        *ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);

Coverity issue: 344983
Fixes: 6cc54096520d ("crypto/octeontx: add supported sessions")
Cc: ndabilpuram@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>

---

There may be further rework needed to set it to the correct value,
but for now at least prevent the NULL dereference.
---
 drivers/common/cpt/cpt_ucode.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/common/cpt/cpt_ucode.h b/drivers/common/cpt/cpt_ucode.h
index 0dac12ee3..d5a0135d7 100644
--- a/drivers/common/cpt/cpt_ucode.h
+++ b/drivers/common/cpt/cpt_ucode.h
@@ -311,5 +311,6 @@ cpt_fc_ciph_set_key(void *ctx, cipher_type_t type, const uint8_t *key,
 
 fc_success:
-	*ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
+	if (ctrl_flags != NULL)
+		*ctrl_flags = rte_cpu_to_be_64(*ctrl_flags);
 
 success:
-- 
2.21.0

[dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic

[Kevin Traynor]

Coverity is complaining about identical code regardless of which branch
of the if else is taken. Functionally it means an error will always be
returned if this if else is hit. Remove the else branch.

    CID 337928 (#1 of 1): Identical code for different branches
    (IDENTICAL_BRANCHES)identical_branches: The same code is executed
    regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
    n->n_children != 0U is true, because the 'then' and 'else' branches
    are identical. Should one of the branches be modified, or the entire
    'if' statement replaced?
1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
1507          n->n_children != 0) {
1508          return -rte_tm_error_set(error,
1509                  EINVAL,
1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
1511                  NULL,
1512                  rte_strerror(EINVAL));
    else_branch: The else branch, identical to the then branch.
1513  } else {
1514          return -rte_tm_error_set(error,
1515                  EINVAL,
1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
1517                  NULL,
1518                  rte_strerror(EINVAL));
1519  }

Coverity issue: 337928
Fixes: c820468ac99c ("net/ipn3ke: support TM")
Cc: rosen.xu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
index adf02c157..a93145d59 100644
--- a/drivers/net/ipn3ke/ipn3ke_tm.c
+++ b/drivers/net/ipn3ke/ipn3ke_tm.c
@@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct rte_eth_dev *dev,
 						NULL,
 						rte_strerror(EINVAL));
-			} else {
-				return -rte_tm_error_set(error,
-						EINVAL,
-						RTE_TM_ERROR_TYPE_UNSPECIFIED,
-						NULL,
-						rte_strerror(EINVAL));
 			}
 		}
-- 
2.21.0

[dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement

[Kevin Traynor]

Coverity complains that this statement is not needed as the goto
label is on the next line anyway. Remove the if statement.

653        ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
   CID 337930 (#1 of 1): Identical code for different branches
   (IDENTICAL_BRANCHES)identical_branches: The same code is executed
   when the condition ret is true or false, because the code in the
   if-then branch and after the if statement is identical. Should
   the if statement be removed?
654        if (ret)
655                goto end;
   implicit_else: The code from the above if-then branch is identical
   to the code after the if statement.
656end:

Coverity issue: 337930
Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
Cc: rosen.xu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ipn3ke/ipn3ke_ethdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.c b/drivers/net/ipn3ke/ipn3ke_ethdev.c
index 28d8aaf2d..af87fda6b 100644
--- a/drivers/net/ipn3ke/ipn3ke_ethdev.c
+++ b/drivers/net/ipn3ke/ipn3ke_ethdev.c
@@ -738,6 +738,5 @@ ipn3ke_cfg_probe(struct rte_vdev_device *dev)
 
 	ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
-	if (ret)
-		goto end;
+
 end:
 	if (kvlist)
-- 
2.21.0

[dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code

[Kevin Traynor]

These struct members and variable were commented out. Remove them.

Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
Fixes: c820468ac99c ("net/ipn3ke: support TM")
Cc: rosen.xu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ipn3ke/ipn3ke_ethdev.h | 7 -------
 drivers/net/ipn3ke/ipn3ke_tm.c     | 1 -
 2 files changed, 8 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.h b/drivers/net/ipn3ke/ipn3ke_ethdev.h
index 985f20352..9b0cf309c 100644
--- a/drivers/net/ipn3ke/ipn3ke_ethdev.h
+++ b/drivers/net/ipn3ke/ipn3ke_ethdev.h
@@ -97,18 +97,11 @@ struct ipn3ke_tm_node {
 struct ipn3ke_tm_hierarchy {
 	struct ipn3ke_tm_node *port_node;
-	/*struct ipn3ke_tm_node_list vt_node_list;*/
-	/*struct ipn3ke_tm_node_list cos_node_list;*/
-
 	uint32_t n_shaper_profiles;
-	/*uint32_t n_shared_shapers;*/
 	uint32_t n_tdrop_profiles;
 	uint32_t n_vt_nodes;
 	uint32_t n_cos_nodes;
-
 	struct ipn3ke_tm_node *port_commit_node;
 	struct ipn3ke_tm_node_list vt_commit_node_list;
 	struct ipn3ke_tm_node_list cos_commit_node_list;
-
-	/*uint32_t n_tm_nodes[IPN3KE_TM_NODE_LEVEL_MAX];*/
 };
 
diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c b/drivers/net/ipn3ke/ipn3ke_tm.c
index a93145d59..5a16c5f96 100644
--- a/drivers/net/ipn3ke/ipn3ke_tm.c
+++ b/drivers/net/ipn3ke/ipn3ke_tm.c
@@ -1088,5 +1088,4 @@ ipn3ke_tm_node_add_check_mount(uint32_t tm_id,
 	struct rte_tm_error *error)
 {
-	/*struct ipn3ke_tm_internals *tm = IPN3KE_DEV_PRIVATE_TO_TM(dev);*/
 	uint32_t node_index;
 	uint32_t parent_index;
-- 
2.21.0

[dpdk-dev] [v2 PATCH 6/8] compress/octeontx: remove commented out code

[Kevin Traynor]

This code is commented out. Remove it.

Fixes: 43e610bb8565 ("compress/octeontx: introduce octeontx zip PMD")
Cc: ssahu@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 drivers/compress/octeontx/include/zip_regs.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/compress/octeontx/include/zip_regs.h b/drivers/compress/octeontx/include/zip_regs.h
index 04c3d75e9..96e538bb7 100644
--- a/drivers/compress/octeontx/include/zip_regs.h
+++ b/drivers/compress/octeontx/include/zip_regs.h
@@ -37,5 +37,4 @@ typedef union {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_vqx_ena_s cn; */
 } zip_vqx_ena_t;
 
@@ -65,5 +64,4 @@ typedef union {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_vqx_sbuf_addr_s cn; */
 } zip_vqx_sbuf_addr_t;
 
@@ -85,5 +83,4 @@ typedef union {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_quex_doorbell_s cn; */
 } zip_quex_doorbell_t;
 
@@ -105,5 +102,4 @@ union zip_nptr_s {
 #endif /* Word 0 - End */
 	} s;
-	/* struct zip_nptr_s_s cn83xx; */
 };
 
@@ -198,5 +194,4 @@ union zip_inst_s {
 		/** Beginning of file */
 		uint64_t bf                    : 1;
-		// uint64_t reserved_3_4          : 2;
 		/** Comp/decomp operation */
 		uint64_t op                    : 2;
@@ -211,5 +206,4 @@ union zip_inst_s {
 		uint64_t dg                    : 1;
 		uint64_t ds                    : 1;
-		//uint64_t reserved_3_4          : 2;
 		uint64_t op                    : 2;
 		uint64_t bf                    : 1;
@@ -616,6 +610,4 @@ union zip_zres_s {
 #endif /* Word 7 - End */
 	} /** ZIP Result Structure */s;
-
-	/* struct zip_zres_s_s cn83xx; */
 };
 
-- 
2.21.0

[dpdk-dev] [v2 PATCH 7/8] event/opdl: remove commented out code

[Kevin Traynor]

Some variables are commented out. Remove them.

Fixes: d548ef513cd7 ("event/opdl: add unit tests")
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Liang Ma <liang.j.ma@intel.com>
---
 drivers/event/opdl/opdl_test.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/event/opdl/opdl_test.c b/drivers/event/opdl/opdl_test.c
index 5868ec1be..e7a32fbd3 100644
--- a/drivers/event/opdl/opdl_test.c
+++ b/drivers/event/opdl/opdl_test.c
@@ -696,7 +696,4 @@ static int
 single_link(struct test *t)
 {
-	/* const uint8_t rx_port = 0; */
-	/* const uint8_t w1_port = 1; */
-	/* const uint8_t w3_port = 3; */
 	const uint8_t tx_port = 2;
 	int err;
-- 
2.21.0

[dpdk-dev] [v2 PATCH 8/8] net/bnxt: remove commented out code

[Kevin Traynor]

This commented out todo and code is old. Remove it.

Fixes: b7435d660a8c ("net/bnxt: add ntuple filtering support")
Cc: stable@dpdk.org

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 7d9459f0a..58a4f98c9 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -2617,7 +2617,4 @@ parse_ntuple_filter(struct bnxt *bp,
 	}
 
-	//TODO Priority
-	//nfilter->priority = (uint8_t)filter->priority;
-
 	bfilter->enables = en;
 	return 0;
-- 
2.21.0

Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

[Kevin Traynor]

On 05/11/2019 17:10, Ferriter, Cian wrote:
> 
> 
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday 5 November 2019 16:41
>> To: David Marchand <david.marchand@redhat.com>
>> Cc: dev <dev@dpdk.org>; Ferriter, Cian <cian.ferriter@intel.com>; dpdk
>> stable <stable@dpdk.org>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
>>
>> On 30/10/2019 07:52, David Marchand wrote:
>>> On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com>
>> wrote:
>>>>
>>>> Previously rx/tx_queues were passed into eth_from_pcaps_common() as
>>>> ptrs and were checked for being NULL.
>>>>
>>>> In commit da6ba28f0540 ("net/pcap: use a struct to pass user
>>>> options") that changed to pass in a ptr to a pmd_devargs_all which
>>>> contains the rx/tx_queues.
>>>>
>>>> The parameter checking was not updated as part of that commit and
>>>> coverity caught that there was still a check if rx/tx_queues were
>>>> NULL, apparently after they had been dereferenced.
>>>>
>>>> Fix that by checking the pmd_devargs_all ptr and removing the NULL
>>>> checks on rx/tx_queues.
>>>>
>>>> 1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
>>>> 1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
>>>> 1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
>>>>     deref_ptr: Directly dereferencing pointer tx_queues.
>>>> 1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
>>>> 1235        unsigned int i;
>>>> 1236
>>>> 1237        /* do some parameter checking */
>>>>     CID 345004: Dereference before null check (REVERSE_INULL)
>>>>     [select issue]
>>>> 1238        if (rx_queues == NULL && nb_rx_queues > 0)
>>>> 1239                return -1;
>>>>     CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
>>>>     check_after_deref: Null-checking tx_queues suggests that it may be
>>>>     null, but it has already been dereferenced on all paths leading to
>>>>     the check.
>>>> 1240        if (tx_queues == NULL && nb_tx_queues > 0)
>>>> 1241                return -1;
>>>>
>>>> Coverity issue: 345029
>>>> Coverity issue: 345044
>>>> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
>>>> Cc: cian.ferriter@intel.com
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>>>
>>> This patch hides the coverity warning.
>>> But can't we just remove those checks?
>>>
>>> Iiuc, the checks on NULL pointers are unnecessary since
>>>
>> https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebe
>> f7a3e60b.
>>>
>>>
>>
>> Aside from the Coverity complaint about rxq/txq NULL check after use, the
>> checks didn't seem to make sense anymore as rxq/txq are part of devargs_all
>> struct now, so they were removed.
>>
>> I added the NULL check on devargs_all to avoid a NULL deference while
>> getting them instead. The check isn't doing any harm, but looking at the
>> current paths to this fn. can see devargs_all would not be NULL, so I'm ok to
>> remove it too. Cian/Ferruh, wdyt?
> 
> I'm OK to remove this. Looks like it's no longer necessary. Good find David!
> 

Thanks Cian, I kept your Ack for v2.

Re: [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups

[David Marchand]

On Wed, Nov 6, 2019 at 8:02 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> Patches 1-4 are Coverity fixes.
> Patches 5-8 are removing commented out code.
>
> v2:
> - 1/8: removed added NULL check for devargs_all
> - 2/8: fixed headline to common/cpt
> - v1 3/9 'net/bnxt: remove logically dead code'
>   removed as duplicate patch already applied
>
> Kevin Traynor (8):
>   net/pcap: fix argument checks
>   common/cpt: fix possible NULL deference
>   net/ipn3ke: fix incorrect commit check logic
>   net/ipn3ke: remove useless if statement
>   net/ipn3ke: remove commented out code
>   compress/octeontx: remove commented out code
>   event/opdl: remove commented out code
>   net/bnxt: remove commented out code

Dropped patch 3 since I did not see a review on it, and I was not
feeling comfortable with it.

Applied, thanks.


--
David Marchand

Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[Xu, Rosen]

Hi David,

Too many things in these days. I have reviewed it. Thanks a lot.

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, October 30, 2019 16:00
> To: Xu, Rosen <rosen.xu@intel.com>
> Cc: dev <dev@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; dpdk
> stable <stable@dpdk.org>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check
> logic
> 
> Hello Rosen,
> 
> Review please.
> 
> On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
> >
> > Coverity is complaining about identical code regardless of which
> > branch of the if else is taken. Functionally it means an error will
> > always be returned if this if else is hit. Remove the else branch.
> >
> >     CID 337928 (#1 of 1): Identical code for different branches
> >     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
> >     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> >     n->n_children != 0U is true, because the 'then' and 'else' branches
> >     are identical. Should one of the branches be modified, or the entire
> >     'if' statement replaced?
> > 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> > 1507          n->n_children != 0) {
> > 1508          return -rte_tm_error_set(error,
> > 1509                  EINVAL,
> > 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> > 1511                  NULL,
> > 1512                  rte_strerror(EINVAL));
> >     else_branch: The else branch, identical to the then branch.
> > 1513  } else {
> > 1514          return -rte_tm_error_set(error,
> > 1515                  EINVAL,
> > 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> > 1517                  NULL,
> > 1518                  rte_strerror(EINVAL));
> > 1519  }
> >
> > Coverity issue: 337928
> > Fixes: c820468ac99c ("net/ipn3ke: support TM")
> > Cc: rosen.xu@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> > ---
> >  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> > b/drivers/net/ipn3ke/ipn3ke_tm.c index adf02c157..a93145d59 100644
> > --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> > +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> > @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct
> rte_eth_dev *dev,
> >                                                 NULL,
> >                                                 rte_strerror(EINVAL));
> > -                       } else {
> > -                               return -rte_tm_error_set(error,
> > -                                               EINVAL,
> > -                                               RTE_TM_ERROR_TYPE_UNSPECIFIED,
> > -                                               NULL,
> > -                                               rte_strerror(EINVAL));
> >                         }
> >                 }
> > --
> > 2.21.0
> >
> 
> --
> David Marchand

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH 5/9] net/ipn3ke: remove useless if statement

[Xu, Rosen]

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, October 30, 2019 16:01
> To: Kevin Traynor <ktraynor@redhat.com>
> Cc: dev <dev@dpdk.org>; Xu, Rosen <rosen.xu@intel.com>; dpdk stable
> <stable@dpdk.org>
> Subject: Re: [dpdk-stable] [PATCH 5/9] net/ipn3ke: remove useless if
> statement
> 
> On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
> >
> > Coverity complains that this statement is not needed as the goto label
> > is on the next line anyway. Remove the if statement.
> >
> > 653        ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
> >    CID 337930 (#1 of 1): Identical code for different branches
> >    (IDENTICAL_BRANCHES)identical_branches: The same code is executed
> >    when the condition ret is true or false, because the code in the
> >    if-then branch and after the if statement is identical. Should
> >    the if statement be removed?
> > 654        if (ret)
> > 655                goto end;
> >    implicit_else: The code from the above if-then branch is identical
> >    to the code after the if statement.
> > 656end:
> >
> > Coverity issue: 337930
> > Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> > Cc: rosen.xu@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> > ---
> >  drivers/net/ipn3ke/ipn3ke_ethdev.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> > b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> > index c226d6313..282295f49 100644
> > --- a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> > +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> > @@ -652,6 +652,5 @@ ipn3ke_cfg_probe(struct rte_vdev_device *dev)
> >
> >         ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
> > -       if (ret)
> > -               goto end;
> > +
> >  end:
> >         if (kvlist)
> > --
> > 2.21.0
> >
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> --
> David Marchand

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH 6/9] net/ipn3ke: remove commented out code

[Xu, Rosen]

Hi,

Too busy these days, sorry for late reply.

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, October 30, 2019 16:04
> To: Kevin Traynor <ktraynor@redhat.com>
> Cc: dev <dev@dpdk.org>; Xu, Rosen <rosen.xu@intel.com>; dpdk stable
> <stable@dpdk.org>
> Subject: Re: [dpdk-stable] [PATCH 6/9] net/ipn3ke: remove commented out
> code
> 
> On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com> wrote:
> >
> > These struct members and variable were commented out. Remove them.
> >
> > Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> > Fixes: c820468ac99c ("net/ipn3ke: support TM")
> > Cc: rosen.xu@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> > ---
> >  drivers/net/ipn3ke/ipn3ke_ethdev.h | 7 -------
> >  drivers/net/ipn3ke/ipn3ke_tm.c     | 1 -
> >  2 files changed, 8 deletions(-)
> >
> > diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> > b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> > index c7b336bbd..0d71dcd64 100644
> > --- a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> > +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> > @@ -97,18 +97,11 @@ struct ipn3ke_tm_node {  struct
> > ipn3ke_tm_hierarchy {
> >         struct ipn3ke_tm_node *port_node;
> > -       /*struct ipn3ke_tm_node_list vt_node_list;*/
> > -       /*struct ipn3ke_tm_node_list cos_node_list;*/
> > -
> >         uint32_t n_shaper_profiles;
> > -       /*uint32_t n_shared_shapers;*/
> >         uint32_t n_tdrop_profiles;
> >         uint32_t n_vt_nodes;
> >         uint32_t n_cos_nodes;
> > -
> >         struct ipn3ke_tm_node *port_commit_node;
> >         struct ipn3ke_tm_node_list vt_commit_node_list;
> >         struct ipn3ke_tm_node_list cos_commit_node_list;
> > -
> > -       /*uint32_t n_tm_nodes[IPN3KE_TM_NODE_LEVEL_MAX];*/
> >  };
> >
> > diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> > b/drivers/net/ipn3ke/ipn3ke_tm.c index a93145d59..5a16c5f96 100644
> > --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> > +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> > @@ -1088,5 +1088,4 @@ ipn3ke_tm_node_add_check_mount(uint32_t
> tm_id,
> >         struct rte_tm_error *error)
> >  {
> > -       /*struct ipn3ke_tm_internals *tm =
> IPN3KE_DEV_PRIVATE_TO_TM(dev);*/
> >         uint32_t node_index;
> >         uint32_t parent_index;
> > --
> > 2.21.0
> >
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> 
> --
> David Marchand

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups

[Xu, Rosen]

Hi David,

Too busy today, sorry for late reply.

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Friday, November 08, 2019 22:08
> To: Kevin Traynor <ktraynor@redhat.com>
> Cc: dev <dev@dpdk.org>; Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong
> <xiaolong.ye@intel.com>
> Subject: Re: [v2 PATCH 0/8] Coverity fixes and other cleanups
> 
> On Wed, Nov 6, 2019 at 8:02 PM Kevin Traynor <ktraynor@redhat.com>
> wrote:
> >
> > Patches 1-4 are Coverity fixes.
> > Patches 5-8 are removing commented out code.
> >
> > v2:
> > - 1/8: removed added NULL check for devargs_all
> > - 2/8: fixed headline to common/cpt
> > - v1 3/9 'net/bnxt: remove logically dead code'
> >   removed as duplicate patch already applied
> >
> > Kevin Traynor (8):
> >   net/pcap: fix argument checks
> >   common/cpt: fix possible NULL deference
> >   net/ipn3ke: fix incorrect commit check logic
> >   net/ipn3ke: remove useless if statement
> >   net/ipn3ke: remove commented out code
> >   compress/octeontx: remove commented out code
> >   event/opdl: remove commented out code
> >   net/bnxt: remove commented out code
> 
> Dropped patch 3 since I did not see a review on it, and I was not feeling
> comfortable with it.
> 
> Applied, thanks.
> 
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[Xu, Rosen]

Hi Kevin,

Too many things in these days, sorry for late reply.

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday, November 05, 2019 23:42
> To: David Marchand <david.marchand@redhat.com>; Xu, Rosen
> <rosen.xu@intel.com>
> Cc: dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; Ye, Xiaolong
> <xiaolong.ye@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check
> logic
> 
> On 30/10/2019 07:59, David Marchand wrote:
> > Hello Rosen,
> >
> > Review please.
> >
> 
> Ping Rosen.
> 
> > On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com>
> wrote:
> >>
> >> Coverity is complaining about identical code regardless of which
> >> branch of the if else is taken. Functionally it means an error will
> >> always be returned if this if else is hit. Remove the else branch.
> >>
> >>     CID 337928 (#1 of 1): Identical code for different branches
> >>     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
> >>     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> >>     n->n_children != 0U is true, because the 'then' and 'else' branches
> >>     are identical. Should one of the branches be modified, or the entire
> >>     'if' statement replaced?

Yes, you are right.

> >> 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> >> 1507          n->n_children != 0) {
> >> 1508          return -rte_tm_error_set(error,
> >> 1509                  EINVAL,
> >> 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> >> 1511                  NULL,
> >> 1512                  rte_strerror(EINVAL));
> >>     else_branch: The else branch, identical to the then branch.
> >> 1513  } else {
> >> 1514          return -rte_tm_error_set(error,
> >> 1515                  EINVAL,
> >> 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> >> 1517                  NULL,
> >> 1518                  rte_strerror(EINVAL));
> >> 1519  }
> >>
> >> Coverity issue: 337928
> >> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> >> Cc: rosen.xu@intel.com
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> >> ---
> >>  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
> >>  1 file changed, 6 deletions(-)
> >>
> >> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> >> b/drivers/net/ipn3ke/ipn3ke_tm.c index adf02c157..a93145d59 100644
> >> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> >> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> >> @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct
> rte_eth_dev *dev,
> >>                                                 NULL,
> >>                                                 rte_strerror(EINVAL));
> >> -                       } else {
> >> -                               return -rte_tm_error_set(error,
> >> -                                               EINVAL,
> >> -                                               RTE_TM_ERROR_TYPE_UNSPECIFIED,
> >> -                                               NULL,
> >> -                                               rte_strerror(EINVAL));
> >>                         }
> >>                 }
> >> --
> >> 2.21.0
> >>
> >

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[Kevin Traynor]

On 08/11/2019 14:45, Xu, Rosen wrote:
> Hi Kevin,
> 
> Too many things in these days, sorry for late reply.
> 

Hi Rosen, no problem, thanks for the Ack.

Kevin.

>> -----Original Message-----
>> From: Kevin Traynor [mailto:ktraynor@redhat.com]
>> Sent: Tuesday, November 05, 2019 23:42
>> To: David Marchand <david.marchand@redhat.com>; Xu, Rosen
>> <rosen.xu@intel.com>
>> Cc: dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; Ye, Xiaolong
>> <xiaolong.ye@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check
>> logic
>>
>> On 30/10/2019 07:59, David Marchand wrote:
>>> Hello Rosen,
>>>
>>> Review please.
>>>
>>
>> Ping Rosen.
>>
>>> On Tue, Oct 1, 2019 at 3:04 PM Kevin Traynor <ktraynor@redhat.com>
>> wrote:
>>>>
>>>> Coverity is complaining about identical code regardless of which
>>>> branch of the if else is taken. Functionally it means an error will
>>>> always be returned if this if else is hit. Remove the else branch.
>>>>
>>>>     CID 337928 (#1 of 1): Identical code for different branches
>>>>     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>>>>     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>>>>     n->n_children != 0U is true, because the 'then' and 'else' branches
>>>>     are identical. Should one of the branches be modified, or the entire
>>>>     'if' statement replaced?
> 
> Yes, you are right.
> 
>>>> 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>>>> 1507          n->n_children != 0) {
>>>> 1508          return -rte_tm_error_set(error,
>>>> 1509                  EINVAL,
>>>> 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
>>>> 1511                  NULL,
>>>> 1512                  rte_strerror(EINVAL));
>>>>     else_branch: The else branch, identical to the then branch.
>>>> 1513  } else {
>>>> 1514          return -rte_tm_error_set(error,
>>>> 1515                  EINVAL,
>>>> 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
>>>> 1517                  NULL,
>>>> 1518                  rte_strerror(EINVAL));
>>>> 1519  }
>>>>
>>>> Coverity issue: 337928
>>>> Fixes: c820468ac99c ("net/ipn3ke: support TM")
>>>> Cc: rosen.xu@intel.com
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>>>> ---
>>>>  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
>>>>  1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
>>>> b/drivers/net/ipn3ke/ipn3ke_tm.c index adf02c157..a93145d59 100644
>>>> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
>>>> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
>>>> @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct
>> rte_eth_dev *dev,
>>>>                                                 NULL,
>>>>                                                 rte_strerror(EINVAL));
>>>> -                       } else {
>>>> -                               return -rte_tm_error_set(error,
>>>> -                                               EINVAL,
>>>> -                                               RTE_TM_ERROR_TYPE_UNSPECIFIED,
>>>> -                                               NULL,
>>>> -                                               rte_strerror(EINVAL));
>>>>                         }
>>>>                 }
>>>> --
>>>> 2.21.0
>>>>
>>>
> 
> Reviewed-by: Rosen Xu <rosen.xu@intel.com>
> 

Re: [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Thursday, November 07, 2019 3:02
> To: dev@dpdk.org
> Cc: david.marchand@redhat.com; Kevin Traynor <ktraynor@redhat.com>;
> Xu, Rosen <rosen.xu@intel.com>; stable@dpdk.org
> Subject: [v2 PATCH 4/8] net/ipn3ke: remove useless if statement
> 
> Coverity complains that this statement is not needed as the goto label is on
> the next line anyway. Remove the if statement.
> 
> 653        ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
>    CID 337930 (#1 of 1): Identical code for different branches
>    (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>    when the condition ret is true or false, because the code in the
>    if-then branch and after the if statement is identical. Should
>    the if statement be removed?
> 654        if (ret)
> 655                goto end;
>    implicit_else: The code from the above if-then branch is identical
>    to the code after the if statement.
> 656end:
> 
> Coverity issue: 337930
> Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> index 28d8aaf2d..af87fda6b 100644
> --- a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> @@ -738,6 +738,5 @@ ipn3ke_cfg_probe(struct rte_vdev_device *dev)
> 
>  	ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
> -	if (ret)
> -		goto end;
> +
>  end:
>  	if (kvlist)
> --
> 2.21.0

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Thursday, November 07, 2019 3:02
> To: dev@dpdk.org
> Cc: david.marchand@redhat.com; Kevin Traynor <ktraynor@redhat.com>;
> Xu, Rosen <rosen.xu@intel.com>; stable@dpdk.org
> Subject: [v2 PATCH 5/8] net/ipn3ke: remove commented out code
> 
> These struct members and variable were commented out. Remove them.
> 
> Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_ethdev.h | 7 -------
>  drivers/net/ipn3ke/ipn3ke_tm.c     | 1 -
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> index 985f20352..9b0cf309c 100644
> --- a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> @@ -97,18 +97,11 @@ struct ipn3ke_tm_node {  struct ipn3ke_tm_hierarchy
> {
>  	struct ipn3ke_tm_node *port_node;
> -	/*struct ipn3ke_tm_node_list vt_node_list;*/
> -	/*struct ipn3ke_tm_node_list cos_node_list;*/
> -
>  	uint32_t n_shaper_profiles;
> -	/*uint32_t n_shared_shapers;*/
>  	uint32_t n_tdrop_profiles;
>  	uint32_t n_vt_nodes;
>  	uint32_t n_cos_nodes;
> -
>  	struct ipn3ke_tm_node *port_commit_node;
>  	struct ipn3ke_tm_node_list vt_commit_node_list;
>  	struct ipn3ke_tm_node_list cos_commit_node_list;
> -
> -	/*uint32_t n_tm_nodes[IPN3KE_TM_NODE_LEVEL_MAX];*/
>  };

No problem.

> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> b/drivers/net/ipn3ke/ipn3ke_tm.c index a93145d59..5a16c5f96 100644
> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> @@ -1088,5 +1088,4 @@ ipn3ke_tm_node_add_check_mount(uint32_t
> tm_id,
>  	struct rte_tm_error *error)
>  {
> -	/*struct ipn3ke_tm_internals *tm =
> IPN3KE_DEV_PRIVATE_TO_TM(dev);*/
>  	uint32_t node_index;
>  	uint32_t parent_index;
> --
> 2.21.0

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Thursday, November 07, 2019 3:02
> To: dev@dpdk.org
> Cc: david.marchand@redhat.com; Kevin Traynor <ktraynor@redhat.com>;
> Xu, Rosen <rosen.xu@intel.com>; stable@dpdk.org
> Subject: [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic
> 
> Coverity is complaining about identical code regardless of which branch of
> the if else is taken. Functionally it means an error will always be returned if
> this if else is hit. Remove the else branch.
> 
>     CID 337928 (#1 of 1): Identical code for different branches
>     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>     n->n_children != 0U is true, because the 'then' and 'else' branches
>     are identical. Should one of the branches be modified, or the entire
>     'if' statement replaced?
> 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> 1507          n->n_children != 0) {
> 1508          return -rte_tm_error_set(error,
> 1509                  EINVAL,
> 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> 1511                  NULL,
> 1512                  rte_strerror(EINVAL));
>     else_branch: The else branch, identical to the then branch.
> 1513  } else {
> 1514          return -rte_tm_error_set(error,
> 1515                  EINVAL,
> 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> 1517                  NULL,
> 1518                  rte_strerror(EINVAL));
> 1519  }
> 
> Coverity issue: 337928
> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> b/drivers/net/ipn3ke/ipn3ke_tm.c index adf02c157..a93145d59 100644
> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct
> rte_eth_dev *dev,
>  						NULL,
>  						rte_strerror(EINVAL));
> -			} else {
> -				return -rte_tm_error_set(error,
> -						EINVAL,
> -
> 	RTE_TM_ERROR_TYPE_UNSPECIFIED,
> -						NULL,
> -						rte_strerror(EINVAL));
>  			}
>  		}
> --
> 2.21.0

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday, October 01, 2019 21:04
> To: dev@dpdk.org
> Cc: Kevin Traynor <ktraynor@redhat.com>; Xu, Rosen
> <rosen.xu@intel.com>; stable@dpdk.org
> Subject: [PATCH 6/9] net/ipn3ke: remove commented out code
> 
> These struct members and variable were commented out. Remove them.
> 
> Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_ethdev.h | 7 -------
>  drivers/net/ipn3ke/ipn3ke_tm.c     | 1 -
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> index c7b336bbd..0d71dcd64 100644
> --- a/drivers/net/ipn3ke/ipn3ke_ethdev.h
> +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.h
> @@ -97,18 +97,11 @@ struct ipn3ke_tm_node {  struct ipn3ke_tm_hierarchy
> {
>  	struct ipn3ke_tm_node *port_node;
> -	/*struct ipn3ke_tm_node_list vt_node_list;*/
> -	/*struct ipn3ke_tm_node_list cos_node_list;*/
> -
>  	uint32_t n_shaper_profiles;
> -	/*uint32_t n_shared_shapers;*/
>  	uint32_t n_tdrop_profiles;
>  	uint32_t n_vt_nodes;
>  	uint32_t n_cos_nodes;
> -
>  	struct ipn3ke_tm_node *port_commit_node;
>  	struct ipn3ke_tm_node_list vt_commit_node_list;
>  	struct ipn3ke_tm_node_list cos_commit_node_list;
> -
> -	/*uint32_t n_tm_nodes[IPN3KE_TM_NODE_LEVEL_MAX];*/
>  };
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> b/drivers/net/ipn3ke/ipn3ke_tm.c index a93145d59..5a16c5f96 100644
> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> @@ -1088,5 +1088,4 @@ ipn3ke_tm_node_add_check_mount(uint32_t
> tm_id,
>  	struct rte_tm_error *error)
>  {
> -	/*struct ipn3ke_tm_internals *tm =
> IPN3KE_DEV_PRIVATE_TO_TM(dev);*/
>  	uint32_t node_index;
>  	uint32_t parent_index;
> --
> 2.21.0

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday, October 01, 2019 21:04
> To: dev@dpdk.org
> Cc: Kevin Traynor <ktraynor@redhat.com>; Xu, Rosen
> <rosen.xu@intel.com>; stable@dpdk.org
> Subject: [PATCH 5/9] net/ipn3ke: remove useless if statement
> 
> Coverity complains that this statement is not needed as the goto label is on
> the next line anyway. Remove the if statement.
> 
> 653        ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
>    CID 337930 (#1 of 1): Identical code for different branches
>    (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>    when the condition ret is true or false, because the code in the
>    if-then branch and after the if statement is identical. Should
>    the if statement be removed?
> 654        if (ret)
> 655                goto end;
>    implicit_else: The code from the above if-then branch is identical
>    to the code after the if statement.
> 656end:
> 
> Coverity issue: 337930
> Fixes: c01c748e4ae6 ("net/ipn3ke: add new driver")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> index c226d6313..282295f49 100644
> --- a/drivers/net/ipn3ke/ipn3ke_ethdev.c
> +++ b/drivers/net/ipn3ke/ipn3ke_ethdev.c
> @@ -652,6 +652,5 @@ ipn3ke_cfg_probe(struct rte_vdev_device *dev)
> 
>  	ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name);
> -	if (ret)
> -		goto end;
> +
>  end:
>  	if (kvlist)
> --
> 2.21.0

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday, October 01, 2019 21:04
> To: dev@dpdk.org
> Cc: Kevin Traynor <ktraynor@redhat.com>; Xu, Rosen
> <rosen.xu@intel.com>; stable@dpdk.org
> Subject: [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
> 
> Coverity is complaining about identical code regardless of which branch of
> the if else is taken. Functionally it means an error will always be returned if
> this if else is hit. Remove the else branch.
> 
>     CID 337928 (#1 of 1): Identical code for different branches
>     (IDENTICAL_BRANCHES)identical_branches: The same code is executed
>     regardless of whether n->level != IPN3KE_TM_NODE_LEVEL_COS ||
>     n->n_children != 0U is true, because the 'then' and 'else' branches
>     are identical. Should one of the branches be modified, or the entire
>     'if' statement replaced?

Okay

> 1506  if (n->level != IPN3KE_TM_NODE_LEVEL_COS ||
> 1507          n->n_children != 0) {
> 1508          return -rte_tm_error_set(error,
> 1509                  EINVAL,
> 1510                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> 1511                  NULL,
> 1512                  rte_strerror(EINVAL));
>     else_branch: The else branch, identical to the then branch.
> 1513  } else {
> 1514          return -rte_tm_error_set(error,
> 1515                  EINVAL,
> 1516                  RTE_TM_ERROR_TYPE_UNSPECIFIED,
> 1517                  NULL,
> 1518                  rte_strerror(EINVAL));
> 1519  }
> 
> Coverity issue: 337928
> Fixes: c820468ac99c ("net/ipn3ke: support TM")
> Cc: rosen.xu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
>  drivers/net/ipn3ke/ipn3ke_tm.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ipn3ke/ipn3ke_tm.c
> b/drivers/net/ipn3ke/ipn3ke_tm.c index adf02c157..a93145d59 100644
> --- a/drivers/net/ipn3ke/ipn3ke_tm.c
> +++ b/drivers/net/ipn3ke/ipn3ke_tm.c
> @@ -1511,10 +1511,4 @@ ipn3ke_tm_hierarchy_commit_check(struct
> rte_eth_dev *dev,
>  						NULL,
>  						rte_strerror(EINVAL));
> -			} else {
> -				return -rte_tm_error_set(error,
> -						EINVAL,
> -
> 	RTE_TM_ERROR_TYPE_UNSPECIFIED,
> -						NULL,
> -						rte_strerror(EINVAL));
>  			}
>  		}
> --
> 2.21.0

Reviewed-by: Rosen Xu <rosen.xu@intel.com>

Re: [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups

[David Marchand]

On Fri, Nov 8, 2019 at 3:41 PM Xu, Rosen <rosen.xu@intel.com> wrote:
>
> Hi David,
>
> Too busy today, sorry for late reply.

Ok, no problem, I had not pushed yet.
So I put the patch 3 back in my tree with your Review tokens added
where applicable, and pushed the full series.


> > > Kevin Traynor (8):
> > >   net/pcap: fix argument checks
> > >   common/cpt: fix possible NULL deference
> > >   net/ipn3ke: fix incorrect commit check logic
> > >   net/ipn3ke: remove useless if statement
> > >   net/ipn3ke: remove commented out code
> > >   compress/octeontx: remove commented out code
> > >   event/opdl: remove commented out code
> > >   net/bnxt: remove commented out code
> >
> > Dropped patch 3 since I did not see a review on it, and I was not feeling
> > comfortable with it.
> >
> > Applied, thanks.


--
David Marchand