[PATCH 0/3] remove usage of rte_panic() and ASSERT() from PMD

[PATCH 0/3] remove usage of rte_panic() and ASSERT() from PMD

[Chaoyong He]

This patch series is aimed to allow the DPDK app to deal with
the error as necessary, by:
* Remove the usage of rte_panic()
* Remove the usage of ASSERT()

James Hershaw (3):
  net/nfp: remove panic usage during reconfig
  net/nfp: remove panic usage for multisegment Tx
  net/nfp: remove ASSERT() macro

 drivers/net/nfp/flower/nfp_flower.c | 11 ++++-------
 drivers/net/nfp/nfp_common.c        | 24 ++++++++++++------------
 drivers/net/nfp/nfp_logs.h          |  2 --
 drivers/net/nfp/nfp_rxtx.c          | 14 +++++---------
 4 files changed, 21 insertions(+), 30 deletions(-)

-- 
2.29.3

[PATCH 1/3] net/nfp: remove panic usage during reconfig

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

Remove usage of the rte_panic() function from the nfp PMD. It is better
to return error values and allow the application to handle the error.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_common.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
index 4f21d9978d..7a60add3a6 100644
--- a/drivers/net/nfp/nfp_common.c
+++ b/drivers/net/nfp/nfp_common.c
@@ -63,8 +63,10 @@ __nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t update)
 	PMD_DRV_LOG(DEBUG, "Writing to the configuration queue (%p)...",
 		    hw->qcp_cfg);
 
-	if (hw->qcp_cfg == NULL)
-		rte_panic("Bad configuration queue pointer\n");
+	if (hw->qcp_cfg == NULL) {
+		PMD_INIT_LOG(ERR, "Bad configuration queue pointer");
+		return -ENXIO;
+	}
 
 	nfp_qcp_ptr_add(hw->qcp_cfg, NFP_QCP_WRITE_PTR, 1);
 
@@ -85,7 +87,7 @@ __nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t update)
 		if (cnt >= NFP_NET_POLL_TIMEOUT) {
 			PMD_INIT_LOG(ERR, "Reconfig timeout for 0x%08x after"
 					  " %dms", update, cnt);
-			rte_panic("Exiting\n");
+			return -EIO;
 		}
 		nanosleep(&wait, 0); /* waiting for a 1ms */
 	}
@@ -121,16 +123,14 @@ nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t ctrl, uint32_t update)
 
 	rte_spinlock_unlock(&hw->reconfig_lock);
 
-	if (!err)
-		return 0;
+	if (err != 0) {
+		PMD_INIT_LOG(ERR, "Error nfp_net reconfig for ctrl: %x update: %x",
+			     ctrl, update);
+		return -EIO;
+	}
+
+	return 0;
 
-	/*
-	 * Reconfig errors imply situations where they can be handled.
-	 * Otherwise, rte_panic is called inside __nfp_net_reconfig
-	 */
-	PMD_INIT_LOG(ERR, "Error nfp_net reconfig for ctrl: %x update: %x",
-		     ctrl, update);
-	return -EIO;
 }
 
 /*
-- 
2.29.3

[PATCH 2/3] net/nfp: remove panic usage for multisegment Tx

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

Remove usage of the rte_panic() function from the nfp PMD.

Update the nfp_net_nfd3_xmit_pkts() function to match the functionality
of the nfp_net_nfdk_xmit_pkts(), rather than use rte_panic()

Update the logging within both the nfp_net_nfd3_xmit_pkts() and
nfp_net_nfdk_xmit_pkts() functions to only make use of a single
log when encountering a transmission with multiple segments when
NFP_NET_CFG_CTRL_GATHER is not set, and to change the log-level to
ERR.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/flower/nfp_flower.c | 3 +--
 drivers/net/nfp/nfp_rxtx.c          | 7 +++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/nfp/flower/nfp_flower.c b/drivers/net/nfp/flower/nfp_flower.c
index 7b46dc0f6a..e0e94e0a36 100644
--- a/drivers/net/nfp/flower/nfp_flower.c
+++ b/drivers/net/nfp/flower/nfp_flower.c
@@ -532,8 +532,7 @@ nfp_flower_pf_xmit_pkts(void *tx_queue,
 
 		if (unlikely(pkt->nb_segs > 1 &&
 				!(hw->cap & NFP_NET_CFG_CTRL_GATHER))) {
-			PMD_INIT_LOG(INFO, "NFP_NET_CFG_CTRL_GATHER not set");
-			PMD_INIT_LOG(INFO, "Multisegment packet unsupported");
+			PMD_INIT_LOG(ERR, "Multisegment packet not supported");
 			goto xmit_end;
 		}
 
diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
index 4a7574fd65..5f23128ac9 100644
--- a/drivers/net/nfp/nfp_rxtx.c
+++ b/drivers/net/nfp/nfp_rxtx.c
@@ -986,8 +986,8 @@ nfp_net_nfd3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 
 		if (unlikely(pkt->nb_segs > 1 &&
 			     !(hw->cap & NFP_NET_CFG_CTRL_GATHER))) {
-			PMD_INIT_LOG(INFO, "NFP_NET_CFG_CTRL_GATHER not set");
-			rte_panic("Multisegment packet unsupported\n");
+			PMD_INIT_LOG(ERR, "Multisegment packet not supported");
+			goto xmit_end;
 		}
 
 		/* Checking if we have enough descriptors */
@@ -1466,8 +1466,7 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 
 		if (unlikely(pkt->nb_segs > 1 &&
 				!(hw->cap & NFP_NET_CFG_CTRL_GATHER))) {
-			PMD_INIT_LOG(INFO, "NFP_NET_CFG_CTRL_GATHER not set");
-			PMD_INIT_LOG(INFO, "Multisegment packet unsupported");
+			PMD_INIT_LOG(ERR, "Multisegment packet not supported");
 			goto xmit_end;
 		}
 
-- 
2.29.3

[PATCH 3/3] net/nfp: remove ASSERT() macro

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

Remove usage of the rte_panic() function from the nfp PMD.

As the ASSERT(x) macro was only used twice in the PMD, it is better
remove it altogether and rework the code where it was used as it
is a very small change to make.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/flower/nfp_flower.c | 8 +++-----
 drivers/net/nfp/nfp_logs.h          | 2 --
 drivers/net/nfp/nfp_rxtx.c          | 7 ++-----
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/nfp/flower/nfp_flower.c b/drivers/net/nfp/flower/nfp_flower.c
index e0e94e0a36..f1424a010d 100644
--- a/drivers/net/nfp/flower/nfp_flower.c
+++ b/drivers/net/nfp/flower/nfp_flower.c
@@ -491,7 +491,7 @@ nfp_flower_pf_xmit_pkts(void *tx_queue,
 		struct rte_mbuf **tx_pkts,
 		uint16_t nb_pkts)
 {
-	int i = 0;
+	int i;
 	int pkt_size;
 	int dma_size;
 	uint64_t dma_addr;
@@ -522,7 +522,7 @@ nfp_flower_pf_xmit_pkts(void *tx_queue,
 	issued_descs = 0;
 
 	/* Sending packets */
-	while ((i < nb_pkts) && free_descs) {
+	for (i = 0; i < nb_pkts && free_descs > 0; i++) {
 		/* Grabbing the mbuf linked to the current descriptor */
 		lmbuf = &txq->txbufs[txq->wr_p].mbuf;
 		/* Warming the cache for releasing the mbuf later on */
@@ -561,7 +561,7 @@ nfp_flower_pf_xmit_pkts(void *tx_queue,
 		 */
 		pkt_size = pkt->pkt_len;
 
-		while (pkt != NULL) {
+		while (pkt != NULL && free_descs > 0) {
 			/* Copying TSO, VLAN and cksum info */
 			*txds = txd;
 
@@ -583,7 +583,6 @@ nfp_flower_pf_xmit_pkts(void *tx_queue,
 			txds->data_len = txd.data_len;
 			txds->dma_addr_hi = (dma_addr >> 32) & 0xff;
 			txds->dma_addr_lo = (dma_addr & 0xffffffff);
-			ASSERT(free_descs > 0);
 			free_descs--;
 
 			txq->wr_p++;
@@ -607,7 +606,6 @@ nfp_flower_pf_xmit_pkts(void *tx_queue,
 			lmbuf = &txq->txbufs[txq->wr_p].mbuf;
 			issued_descs++;
 		}
-		i++;
 	}
 
 xmit_end:
diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
index 5da384e53f..b7632ee72c 100644
--- a/drivers/net/nfp/nfp_logs.h
+++ b/drivers/net/nfp/nfp_logs.h
@@ -24,10 +24,8 @@ extern int nfp_logtype_init;
 #ifdef RTE_ETHDEV_DEBUG_TX
 #define PMD_TX_LOG(level, fmt, args...) \
 	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
-#define ASSERT(x) if (!(x)) rte_panic("NFP_NET: x")
 #else
 #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
-#define ASSERT(x) do { } while (0)
 #endif
 
 extern int nfp_logtype_cpp;
diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
index 5f23128ac9..a5907cc428 100644
--- a/drivers/net/nfp/nfp_rxtx.c
+++ b/drivers/net/nfp/nfp_rxtx.c
@@ -968,12 +968,11 @@ nfp_net_nfd3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 
 	pkt = *tx_pkts;
 
-	i = 0;
 	issued_descs = 0;
 	PMD_TX_LOG(DEBUG, "queue: %u. Sending %u packets",
 		   txq->qidx, nb_pkts);
 	/* Sending packets */
-	while ((i < nb_pkts) && free_descs) {
+	for (i = 0; i < nb_pkts && free_descs > 0; i++) {
 		memset(&meta_data, 0, sizeof(meta_data));
 		/* Grabbing the mbuf linked to the current descriptor */
 		lmbuf = &txq->txbufs[txq->wr_p].mbuf;
@@ -1010,7 +1009,7 @@ nfp_net_nfd3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 		 */
 		pkt_size = pkt->pkt_len;
 
-		while (pkt) {
+		while (pkt != NULL && free_descs > 0) {
 			/* Copying TSO, VLAN and cksum info */
 			*txds = txd;
 
@@ -1034,7 +1033,6 @@ nfp_net_nfd3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 			txds->data_len = txd.data_len;
 			txds->dma_addr_hi = (dma_addr >> 32) & 0xff;
 			txds->dma_addr_lo = (dma_addr & 0xffffffff);
-			ASSERT(free_descs > 0);
 			free_descs--;
 
 			txq->wr_p++;
@@ -1061,7 +1059,6 @@ nfp_net_nfd3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 			lmbuf = &txq->txbufs[txq->wr_p].mbuf;
 			issued_descs++;
 		}
-		i++;
 	}
 
 xmit_end:
-- 
2.29.3

Re: [PATCH 0/3] remove usage of rte_panic() and ASSERT() from PMD

[Ferruh Yigit]

On 2/3/2023 7:57 AM, Chaoyong He wrote:
> This patch series is aimed to allow the DPDK app to deal with
> the error as necessary, by:
> * Remove the usage of rte_panic()
> * Remove the usage of ASSERT()
> 
> James Hershaw (3):
>   net/nfp: remove panic usage during reconfig
>   net/nfp: remove panic usage for multisegment Tx
>   net/nfp: remove ASSERT() macro


Thanks for cleanup!

Series applied to dpdk-next-net/main, thanks.