DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups
@ 2019-10-01 12:53 Kevin Traynor
  2019-10-01 12:53 ` [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks Kevin Traynor
                   ` (2 more replies)
  0 siblings, 3 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 12:53 UTC (permalink / raw)
  To: dev; +Cc: 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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
  2019-10-01 12:53 [dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups Kevin Traynor
@ 2019-10-01 12:53 ` Kevin Traynor
  2019-10-04 10:57   ` Ferriter, Cian
  2019-10-30  7:52   ` David Marchand
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
  2 siblings, 2 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 12:53 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, cian.ferriter, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference
  2019-10-01 12:53 [dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups Kevin Traynor
  2019-10-01 12:53 ` [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks Kevin Traynor
@ 2019-10-01 13:03 ` Kevin Traynor
  2019-10-01 13:03   ` [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code Kevin Traynor
                     ` (7 more replies)
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
  2 siblings, 8 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:03 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, ndabilpuram, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
@ 2019-10-01 13:03   ` Kevin Traynor
  2019-10-02  1:28     ` Ajit Khaparde
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
                     ` (6 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:03 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, lance.richardson, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
  2019-10-01 13:03   ` [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code Kevin Traynor
@ 2019-10-01 13:04   ` Kevin Traynor
  2019-10-30  7:59     ` David Marchand
  2019-11-08 14:52     ` Xu, Rosen
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement Kevin Traynor
                     ` (5 subsequent siblings)
  7 siblings, 2 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:04 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, rosen.xu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
  2019-10-01 13:03   ` [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code Kevin Traynor
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
@ 2019-10-01 13:04   ` Kevin Traynor
  2019-10-30  8:01     ` [dpdk-dev] [dpdk-stable] " David Marchand
  2019-11-08 14:52     ` [dpdk-dev] " Xu, Rosen
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code Kevin Traynor
                     ` (4 subsequent siblings)
  7 siblings, 2 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:04 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, rosen.xu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
                     ` (2 preceding siblings ...)
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement Kevin Traynor
@ 2019-10-01 13:04   ` Kevin Traynor
  2019-10-30  8:04     ` [dpdk-dev] [dpdk-stable] " David Marchand
  2019-11-08 14:51     ` [dpdk-dev] " Xu, Rosen
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 7/9] compress/octeontx: " Kevin Traynor
                     ` (3 subsequent siblings)
  7 siblings, 2 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:04 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, rosen.xu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 7/9] compress/octeontx: remove commented out code
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
                     ` (3 preceding siblings ...)
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code Kevin Traynor
@ 2019-10-01 13:04   ` Kevin Traynor
  2019-10-30  8:06     ` David Marchand
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 8/9] event/opdl: " Kevin Traynor
                     ` (2 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:04 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, ssahu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 8/9] event/opdl: remove commented out code
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
                     ` (4 preceding siblings ...)
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 7/9] compress/octeontx: " Kevin Traynor
@ 2019-10-01 13:04   ` Kevin Traynor
  2019-10-03 10:50     ` Liang, Ma
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 9/9] net/bnxt: " Kevin Traynor
  2019-10-30  7:56   ` [dpdk-dev] [dpdk-stable] [PATCH 2/9] crypto/octeontx: fix possible NULL deference David Marchand
  7 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:04 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, liang.j.ma, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [PATCH 9/9] net/bnxt: remove commented out code
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
                     ` (5 preceding siblings ...)
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 8/9] event/opdl: " Kevin Traynor
@ 2019-10-01 13:04   ` Kevin Traynor
  2019-10-01 15:42     ` Ajit Khaparde
  2019-10-30  7:56   ` [dpdk-dev] [dpdk-stable] [PATCH 2/9] crypto/octeontx: fix possible NULL deference David Marchand
  7 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-10-01 13:04 UTC (permalink / raw)
  To: dev; +Cc: Kevin Traynor, ajit.khaparde, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 9/9] net/bnxt: remove commented out code
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 9/9] net/bnxt: " Kevin Traynor
@ 2019-10-01 15:42     ` Ajit Khaparde
  0 siblings, 0 replies; 50+ messages in thread
From: Ajit Khaparde @ 2019-10-01 15:42 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, dpdk stable

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code
  2019-10-01 13:03   ` [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code Kevin Traynor
@ 2019-10-02  1:28     ` Ajit Khaparde
  2019-10-30  7:43       ` David Marchand
  0 siblings, 1 reply; 50+ messages in thread
From: Ajit Khaparde @ 2019-10-02  1:28 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, Lance Richardson, dpdk stable

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 8/9] event/opdl: remove commented out code
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 8/9] event/opdl: " Kevin Traynor
@ 2019-10-03 10:50     ` Liang, Ma
  0 siblings, 0 replies; 50+ messages in thread
From: Liang, Ma @ 2019-10-03 10:50 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
  2019-10-01 12:53 ` [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks Kevin Traynor
@ 2019-10-04 10:57   ` Ferriter, Cian
  2019-10-30  7:52   ` David Marchand
  1 sibling, 0 replies; 50+ messages in thread
From: Ferriter, Cian @ 2019-10-04 10:57 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: stable

 

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code
  2019-10-02  1:28     ` Ajit Khaparde
@ 2019-10-30  7:43       ` David Marchand
  2019-10-30 16:27         ` Ajit Khaparde
  0 siblings, 1 reply; 50+ messages in thread
From: David Marchand @ 2019-10-30  7:43 UTC (permalink / raw)
  To: Ajit Khaparde, Kevin Traynor; +Cc: dev, Lance Richardson, dpdk stable

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
  2019-10-01 12:53 ` [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks Kevin Traynor
  2019-10-04 10:57   ` Ferriter, Cian
@ 2019-10-30  7:52   ` David Marchand
  2019-11-05 16:40     ` Kevin Traynor
  1 sibling, 1 reply; 50+ messages in thread
From: David Marchand @ 2019-10-30  7:52 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, cian.ferriter, dpdk stable

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH 2/9] crypto/octeontx: fix possible NULL deference
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
                     ` (6 preceding siblings ...)
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 9/9] net/bnxt: " Kevin Traynor
@ 2019-10-30  7:56   ` David Marchand
  7 siblings, 0 replies; 50+ messages in thread
From: David Marchand @ 2019-10-30  7:56 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, Nithin Dabilpuram, dpdk stable, anoobj

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
@ 2019-10-30  7:59     ` David Marchand
  2019-11-05 15:41       ` Kevin Traynor
  2019-11-08 14:35       ` Xu, Rosen
  2019-11-08 14:52     ` Xu, Rosen
  1 sibling, 2 replies; 50+ messages in thread
From: David Marchand @ 2019-10-30  7:59 UTC (permalink / raw)
  To: Rosen Xu; +Cc: dev, Kevin Traynor, dpdk stable, Xiaolong Ye

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH 5/9] net/ipn3ke: remove useless if statement
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement Kevin Traynor
@ 2019-10-30  8:01     ` David Marchand
  2019-11-08 14:35       ` Xu, Rosen
  2019-11-08 14:52     ` [dpdk-dev] " Xu, Rosen
  1 sibling, 1 reply; 50+ messages in thread
From: David Marchand @ 2019-10-30  8:01 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, Rosen Xu, dpdk stable

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH 6/9] net/ipn3ke: remove commented out code
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code Kevin Traynor
@ 2019-10-30  8:04     ` David Marchand
  2019-11-08 14:40       ` Xu, Rosen
  2019-11-08 14:51     ` [dpdk-dev] " Xu, Rosen
  1 sibling, 1 reply; 50+ messages in thread
From: David Marchand @ 2019-10-30  8:04 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, Rosen Xu, dpdk stable

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 7/9] compress/octeontx: remove commented out code
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 7/9] compress/octeontx: " Kevin Traynor
@ 2019-10-30  8:06     ` David Marchand
  0 siblings, 0 replies; 50+ messages in thread
From: David Marchand @ 2019-10-30  8:06 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, ssahu, dpdk stable, Ashish Gupta

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code
  2019-10-30  7:43       ` David Marchand
@ 2019-10-30 16:27         ` Ajit Khaparde
  2019-11-05 15:39           ` Kevin Traynor
  0 siblings, 1 reply; 50+ messages in thread
From: Ajit Khaparde @ 2019-10-30 16:27 UTC (permalink / raw)
  To: David Marchand; +Cc: Kevin Traynor, dev, Lance Richardson, dpdk stable

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
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code
  2019-10-30 16:27         ` Ajit Khaparde
@ 2019-11-05 15:39           ` Kevin Traynor
  0 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-05 15:39 UTC (permalink / raw)
  To: Ajit Khaparde, David Marchand; +Cc: dev, Lance Richardson, dpdk stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-10-30  7:59     ` David Marchand
@ 2019-11-05 15:41       ` Kevin Traynor
  2019-11-08 14:45         ` Xu, Rosen
  2019-11-08 14:35       ` Xu, Rosen
  1 sibling, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-11-05 15:41 UTC (permalink / raw)
  To: David Marchand, Rosen Xu; +Cc: dev, dpdk stable, Xiaolong Ye

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
  2019-10-30  7:52   ` David Marchand
@ 2019-11-05 16:40     ` Kevin Traynor
  2019-11-05 17:10       ` Ferriter, Cian
  0 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-11-05 16:40 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, cian.ferriter, dpdk stable, Yigit, Ferruh

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?


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
  2019-11-05 16:40     ` Kevin Traynor
@ 2019-11-05 17:10       ` Ferriter, Cian
  2019-11-06 19:03         ` Kevin Traynor
  0 siblings, 1 reply; 50+ messages in thread
From: Ferriter, Cian @ 2019-11-05 17:10 UTC (permalink / raw)
  To: Kevin Traynor, David Marchand; +Cc: dev, dpdk stable, Yigit, Ferruh



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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups
  2019-10-01 12:53 [dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups Kevin Traynor
  2019-10-01 12:53 ` [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks Kevin Traynor
  2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
@ 2019-11-06 19:01 ` Kevin Traynor
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 1/8] net/pcap: fix argument checks Kevin Traynor
                     ` (8 more replies)
  2 siblings, 9 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:01 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, 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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 1/8] net/pcap: fix argument checks
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
@ 2019-11-06 19:01   ` Kevin Traynor
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 2/8] common/cpt: fix possible NULL deference Kevin Traynor
                     ` (7 subsequent siblings)
  8 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:01 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, stable, Cian Ferriter

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 2/8] common/cpt: fix possible NULL deference
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 1/8] net/pcap: fix argument checks Kevin Traynor
@ 2019-11-06 19:01   ` Kevin Traynor
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:01 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, ndabilpuram, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 1/8] net/pcap: fix argument checks Kevin Traynor
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 2/8] common/cpt: fix possible NULL deference Kevin Traynor
@ 2019-11-06 19:01   ` Kevin Traynor
  2019-11-08 14:50     ` Xu, Rosen
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement Kevin Traynor
                     ` (5 subsequent siblings)
  8 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:01 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, rosen.xu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
                     ` (2 preceding siblings ...)
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
@ 2019-11-06 19:01   ` Kevin Traynor
  2019-11-08 14:49     ` Xu, Rosen
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code Kevin Traynor
                     ` (4 subsequent siblings)
  8 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:01 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, rosen.xu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
                     ` (3 preceding siblings ...)
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement Kevin Traynor
@ 2019-11-06 19:02   ` Kevin Traynor
  2019-11-08 14:50     ` Xu, Rosen
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 6/8] compress/octeontx: " Kevin Traynor
                     ` (3 subsequent siblings)
  8 siblings, 1 reply; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:02 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, rosen.xu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 6/8] compress/octeontx: remove commented out code
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
                     ` (4 preceding siblings ...)
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code Kevin Traynor
@ 2019-11-06 19:02   ` Kevin Traynor
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 7/8] event/opdl: " Kevin Traynor
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:02 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, ssahu, stable

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 7/8] event/opdl: remove commented out code
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
                     ` (5 preceding siblings ...)
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 6/8] compress/octeontx: " Kevin Traynor
@ 2019-11-06 19:02   ` Kevin Traynor
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 8/8] net/bnxt: " Kevin Traynor
  2019-11-08 14:07   ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups David Marchand
  8 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:02 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, stable, Liang Ma

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [dpdk-dev] [v2 PATCH 8/8] net/bnxt: remove commented out code
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
                     ` (6 preceding siblings ...)
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 7/8] event/opdl: " Kevin Traynor
@ 2019-11-06 19:02   ` Kevin Traynor
  2019-11-08 14:07   ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups David Marchand
  8 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:02 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Kevin Traynor, stable, Ajit Khaparde

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks
  2019-11-05 17:10       ` Ferriter, Cian
@ 2019-11-06 19:03         ` Kevin Traynor
  0 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-06 19:03 UTC (permalink / raw)
  To: Ferriter, Cian, David Marchand; +Cc: dev, dpdk stable, Yigit, Ferruh

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.


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups
  2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
                     ` (7 preceding siblings ...)
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 8/8] net/bnxt: " Kevin Traynor
@ 2019-11-08 14:07   ` David Marchand
  2019-11-08 14:41     ` Xu, Rosen
  8 siblings, 1 reply; 50+ messages in thread
From: David Marchand @ 2019-11-08 14:07 UTC (permalink / raw)
  To: Kevin Traynor; +Cc: dev, Rosen Xu, Xiaolong Ye

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-10-30  7:59     ` David Marchand
  2019-11-05 15:41       ` Kevin Traynor
@ 2019-11-08 14:35       ` Xu, Rosen
  1 sibling, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:35 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Kevin Traynor, dpdk stable, Ye, Xiaolong

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH 5/9] net/ipn3ke: remove useless if statement
  2019-10-30  8:01     ` [dpdk-dev] [dpdk-stable] " David Marchand
@ 2019-11-08 14:35       ` Xu, Rosen
  0 siblings, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:35 UTC (permalink / raw)
  To: David Marchand, Kevin Traynor; +Cc: dev, dpdk stable



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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH 6/9] net/ipn3ke: remove commented out code
  2019-10-30  8:04     ` [dpdk-dev] [dpdk-stable] " David Marchand
@ 2019-11-08 14:40       ` Xu, Rosen
  0 siblings, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:40 UTC (permalink / raw)
  To: David Marchand, Kevin Traynor; +Cc: dev, dpdk stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups
  2019-11-08 14:07   ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups David Marchand
@ 2019-11-08 14:41     ` Xu, Rosen
  2019-11-08 15:15       ` David Marchand
  0 siblings, 1 reply; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:41 UTC (permalink / raw)
  To: David Marchand, Kevin Traynor; +Cc: dev, Ye, Xiaolong

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-11-05 15:41       ` Kevin Traynor
@ 2019-11-08 14:45         ` Xu, Rosen
  2019-11-08 14:47           ` Kevin Traynor
  0 siblings, 1 reply; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:45 UTC (permalink / raw)
  To: Kevin Traynor, David Marchand; +Cc: dev, dpdk stable, Ye, Xiaolong

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-11-08 14:45         ` Xu, Rosen
@ 2019-11-08 14:47           ` Kevin Traynor
  0 siblings, 0 replies; 50+ messages in thread
From: Kevin Traynor @ 2019-11-08 14:47 UTC (permalink / raw)
  To: Xu, Rosen, David Marchand; +Cc: dev, dpdk stable, Ye, Xiaolong

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement Kevin Traynor
@ 2019-11-08 14:49     ` Xu, Rosen
  0 siblings, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:49 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: david.marchand, stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code
  2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code Kevin Traynor
@ 2019-11-08 14:50     ` Xu, Rosen
  0 siblings, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:50 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: david.marchand, stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic
  2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
@ 2019-11-08 14:50     ` Xu, Rosen
  0 siblings, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:50 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: david.marchand, stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code Kevin Traynor
  2019-10-30  8:04     ` [dpdk-dev] [dpdk-stable] " David Marchand
@ 2019-11-08 14:51     ` Xu, Rosen
  1 sibling, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:51 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement Kevin Traynor
  2019-10-30  8:01     ` [dpdk-dev] [dpdk-stable] " David Marchand
@ 2019-11-08 14:52     ` Xu, Rosen
  1 sibling, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:52 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic
  2019-10-01 13:04   ` [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
  2019-10-30  7:59     ` David Marchand
@ 2019-11-08 14:52     ` Xu, Rosen
  1 sibling, 0 replies; 50+ messages in thread
From: Xu, Rosen @ 2019-11-08 14:52 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: stable

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>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups
  2019-11-08 14:41     ` Xu, Rosen
@ 2019-11-08 15:15       ` David Marchand
  0 siblings, 0 replies; 50+ messages in thread
From: David Marchand @ 2019-11-08 15:15 UTC (permalink / raw)
  To: Xu, Rosen; +Cc: Kevin Traynor, dev, Ye, Xiaolong

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2019-11-08 15:16 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 12:53 [dpdk-dev] [PATCH 0/9] Coverity fixes and other cleanups Kevin Traynor
2019-10-01 12:53 ` [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks Kevin Traynor
2019-10-04 10:57   ` Ferriter, Cian
2019-10-30  7:52   ` David Marchand
2019-11-05 16:40     ` Kevin Traynor
2019-11-05 17:10       ` Ferriter, Cian
2019-11-06 19:03         ` Kevin Traynor
2019-10-01 13:03 ` [dpdk-dev] [PATCH 2/9] crypto/octeontx: fix possible NULL deference Kevin Traynor
2019-10-01 13:03   ` [dpdk-dev] [PATCH 3/9] net/bnxt: remove logically dead code Kevin Traynor
2019-10-02  1:28     ` Ajit Khaparde
2019-10-30  7:43       ` David Marchand
2019-10-30 16:27         ` Ajit Khaparde
2019-11-05 15:39           ` Kevin Traynor
2019-10-01 13:04   ` [dpdk-dev] [PATCH 4/9] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
2019-10-30  7:59     ` David Marchand
2019-11-05 15:41       ` Kevin Traynor
2019-11-08 14:45         ` Xu, Rosen
2019-11-08 14:47           ` Kevin Traynor
2019-11-08 14:35       ` Xu, Rosen
2019-11-08 14:52     ` Xu, Rosen
2019-10-01 13:04   ` [dpdk-dev] [PATCH 5/9] net/ipn3ke: remove useless if statement Kevin Traynor
2019-10-30  8:01     ` [dpdk-dev] [dpdk-stable] " David Marchand
2019-11-08 14:35       ` Xu, Rosen
2019-11-08 14:52     ` [dpdk-dev] " Xu, Rosen
2019-10-01 13:04   ` [dpdk-dev] [PATCH 6/9] net/ipn3ke: remove commented out code Kevin Traynor
2019-10-30  8:04     ` [dpdk-dev] [dpdk-stable] " David Marchand
2019-11-08 14:40       ` Xu, Rosen
2019-11-08 14:51     ` [dpdk-dev] " Xu, Rosen
2019-10-01 13:04   ` [dpdk-dev] [PATCH 7/9] compress/octeontx: " Kevin Traynor
2019-10-30  8:06     ` David Marchand
2019-10-01 13:04   ` [dpdk-dev] [PATCH 8/9] event/opdl: " Kevin Traynor
2019-10-03 10:50     ` Liang, Ma
2019-10-01 13:04   ` [dpdk-dev] [PATCH 9/9] net/bnxt: " Kevin Traynor
2019-10-01 15:42     ` Ajit Khaparde
2019-10-30  7:56   ` [dpdk-dev] [dpdk-stable] [PATCH 2/9] crypto/octeontx: fix possible NULL deference David Marchand
2019-11-06 19:01 ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups Kevin Traynor
2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 1/8] net/pcap: fix argument checks Kevin Traynor
2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 2/8] common/cpt: fix possible NULL deference Kevin Traynor
2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 3/8] net/ipn3ke: fix incorrect commit check logic Kevin Traynor
2019-11-08 14:50     ` Xu, Rosen
2019-11-06 19:01   ` [dpdk-dev] [v2 PATCH 4/8] net/ipn3ke: remove useless if statement Kevin Traynor
2019-11-08 14:49     ` Xu, Rosen
2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 5/8] net/ipn3ke: remove commented out code Kevin Traynor
2019-11-08 14:50     ` Xu, Rosen
2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 6/8] compress/octeontx: " Kevin Traynor
2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 7/8] event/opdl: " Kevin Traynor
2019-11-06 19:02   ` [dpdk-dev] [v2 PATCH 8/8] net/bnxt: " Kevin Traynor
2019-11-08 14:07   ` [dpdk-dev] [v2 PATCH 0/8] Coverity fixes and other cleanups David Marchand
2019-11-08 14:41     ` Xu, Rosen
2019-11-08 15:15       ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).