[dpdk-dev] [PATCH 0/3] bnxt fixes

[dpdk-dev] [PATCH 0/3] bnxt fixes

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Please apply.

Ajit Khaparde (1):
  net/bnxt: check flush status during ring free

Kalesh AP (2):
  net/bnxt: fix ver get HWRM command
  net/bnxt: fix VF info allocation

 drivers/net/bnxt/bnxt_ethdev.c |   2 +-
 drivers/net/bnxt/bnxt_hwrm.c   | 231 +++++++++++++++++++++++++++++++++--------
 drivers/net/bnxt/bnxt_hwrm.h   |   5 +-
 drivers/net/bnxt/bnxt_rxr.c    |  36 ++++++-
 drivers/net/bnxt/bnxt_rxr.h    |   1 +
 drivers/net/bnxt/bnxt_txr.c    |  36 +++++++
 drivers/net/bnxt/bnxt_txr.h    |   1 +
 7 files changed, 262 insertions(+), 50 deletions(-)

-- 
2.10.1

[dpdk-dev] [PATCH 1/3] net/bnxt: check flush status during ring free

[Kalesh A P]

From: Ajit Khaparde <ajit.khaparde@broadcom.com>

When host SW issues a HWRM_RING_FREE for Tx/Rx/AGG ring in HW,
the FW flushes the BDs associated with the ring and performs other
cleanup in the HW. The host software should ideally check for an
indication from the FW indicating this step has been completed
successfully to avoid unexpected errors during cleanup.

The FW issues a HWRM_DONE response to the RING_FREE request on
the corresponding CQ ring. Poll the CQs during cleanup and
ensure the HWRM_FREE command is completed not just based on the
value of valid bit but also the HWRM_DONE response for the ring.

If the HWRM_DONE response is not seen, force the cleanup to
complete just based on the valid bit.

Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_hwrm.c | 128 ++++++++++++++++++++++++++++++++++++++++---
 drivers/net/bnxt/bnxt_hwrm.h |   3 +-
 drivers/net/bnxt/bnxt_rxr.c  |  36 +++++++++++-
 drivers/net/bnxt/bnxt_rxr.h  |   1 +
 drivers/net/bnxt/bnxt_txr.c  |  36 ++++++++++++
 drivers/net/bnxt/bnxt_txr.h  |   1 +
 6 files changed, 196 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 0b5318e..02b0a21 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -75,6 +75,82 @@ static void bnxt_hwrm_set_pg_attr(struct bnxt_ring_mem_info *rmem,
 	}
 }
 
+static struct bnxt_cp_ring_info*
+bnxt_get_ring_info_by_id(struct bnxt *bp, uint16_t rid, uint16_t type)
+{
+	struct bnxt_cp_ring_info *cp_ring = NULL;
+	uint16_t i;
+
+	switch (type) {
+	case HWRM_RING_FREE_INPUT_RING_TYPE_RX:
+	case HWRM_RING_FREE_INPUT_RING_TYPE_RX_AGG:
+		/* FALLTHROUGH */
+		for (i = 0; i < bp->rx_cp_nr_rings; i++) {
+			struct bnxt_rx_queue *rxq = bp->rx_queues[i];
+
+			if (rxq->cp_ring->cp_ring_struct->fw_ring_id ==
+			    rte_cpu_to_le_16(rid)) {
+				return rxq->cp_ring;
+			}
+		}
+		break;
+	case HWRM_RING_FREE_INPUT_RING_TYPE_TX:
+		for (i = 0; i < bp->tx_cp_nr_rings; i++) {
+			struct bnxt_tx_queue *txq = bp->tx_queues[i];
+
+			if (txq->cp_ring->cp_ring_struct->fw_ring_id ==
+			    rte_cpu_to_le_16(rid)) {
+				return txq->cp_ring;
+			}
+		}
+		break;
+	default:
+		return cp_ring;
+	}
+	return cp_ring;
+}
+
+/* Complete a sweep of the CQ ring for the corresponding Tx/Rx/AGG ring.
+ * If the CMPL_BASE_TYPE_HWRM_DONE is not encountered by the last pass,
+ * before timeout, we force the done bit for the cleanup to proceed.
+ * Also if cpr is null, do nothing.. The HWRM command is  not for a
+ * Tx/Rx/AGG ring cleanup.
+ */
+static int
+bnxt_check_cq_hwrm_done(struct bnxt_cp_ring_info *cpr,
+			bool tx, bool rx, bool timeout)
+{
+	int done = 0;
+
+	if (cpr != NULL) {
+		if (tx)
+			done = bnxt_flush_tx_cmp(cpr);
+
+		if (rx)
+			done = bnxt_flush_rx_cmp(cpr);
+
+		if (done)
+			PMD_DRV_LOG(DEBUG, "HWRM DONE for %s ring\n",
+				    rx ? "Rx" : "Tx");
+
+		/* We are about to timeout and still haven't seen the
+		 * HWRM done for the Ring free. Force the cleanup.
+		 */
+		if (!done && timeout) {
+			done = 1;
+			PMD_DRV_LOG(DEBUG, "Timing out for %s ring\n",
+				    rx ? "Rx" : "Tx");
+		}
+	} else {
+		/* This HWRM command is not for a Tx/Rx/AGG ring cleanup.
+		 * Otherwise the cpr would have been valid. So do nothing.
+		 */
+		done = 1;
+	}
+
+	return done;
+}
+
 /*
  * HWRM Functions (sent to HWRM)
  * These are named bnxt_hwrm_*() and return 0 on success or -110 if the
@@ -97,6 +173,9 @@ static int bnxt_hwrm_send_message(struct bnxt *bp, void *msg,
 		GRCPF_REG_KONG_CHANNEL_OFFSET : GRCPF_REG_CHIMP_CHANNEL_OFFSET;
 	uint16_t mb_trigger_offset = use_kong_mb ?
 		GRCPF_REG_KONG_COMM_TRIGGER : GRCPF_REG_CHIMP_COMM_TRIGGER;
+	struct bnxt_cp_ring_info *cpr = NULL;
+	bool is_rx = false;
+	bool is_tx = false;
 	uint32_t timeout;
 
 	/* Do not send HWRM commands to firmware in error state */
@@ -153,14 +232,42 @@ static int bnxt_hwrm_send_message(struct bnxt *bp, void *msg,
 	 */
 	rte_io_mb();
 
+	/* Check ring flush is done.
+	 * This is valid only for Tx and Rx rings (including AGG rings).
+	 * The Tx and Rx rings should be freed once the HW confirms all
+	 * the internal buffers and BDs associated with the rings are
+	 * consumed and the corresponding DMA is handled.
+	 */
+	if (rte_cpu_to_le_16(req->cmpl_ring) != INVALID_HW_RING_ID) {
+		/* Check if the TxCQ matches. If that fails check if RxCQ
+		 * matches. And if neither match, is_rx = false, is_tx = false.
+		 */
+		cpr = bnxt_get_ring_info_by_id(bp, req->cmpl_ring,
+					       HWRM_RING_FREE_INPUT_RING_TYPE_TX);
+		if (cpr == NULL) {
+			/* Not a TxCQ. Check if the RxCQ matches. */
+			cpr =
+			bnxt_get_ring_info_by_id(bp, req->cmpl_ring,
+						 HWRM_RING_FREE_INPUT_RING_TYPE_RX);
+			if (cpr != NULL)
+				is_rx = true;
+		} else {
+			is_tx = true;
+		}
+	}
+
 	/* Poll for the valid bit */
 	for (i = 0; i < timeout; i++) {
+		int done;
+
+		done = bnxt_check_cq_hwrm_done(cpr, is_tx, is_rx,
+					       i == timeout - 1);
 		/* Sanity check on the resp->resp_len */
 		rte_io_rmb();
 		if (resp->resp_len && resp->resp_len <= bp->max_resp_len) {
 			/* Last byte of resp contains the valid key */
 			valid = (uint8_t *)resp + resp->resp_len - 1;
-			if (*valid == HWRM_RESP_VALID_KEY)
+			if (*valid == HWRM_RESP_VALID_KEY && done)
 				break;
 		}
 		rte_delay_us(1);
@@ -1672,7 +1779,8 @@ int bnxt_hwrm_ring_alloc(struct bnxt *bp,
 }
 
 int bnxt_hwrm_ring_free(struct bnxt *bp,
-			struct bnxt_ring *ring, uint32_t ring_type)
+			struct bnxt_ring *ring, uint32_t ring_type,
+			uint16_t cp_ring_id)
 {
 	int rc;
 	struct hwrm_ring_free_input req = {.req_type = 0 };
@@ -1682,6 +1790,7 @@ int bnxt_hwrm_ring_free(struct bnxt *bp,
 
 	req.ring_type = ring_type;
 	req.ring_id = rte_cpu_to_le_16(ring->fw_ring_id);
+	req.cmpl_ring = rte_cpu_to_le_16(cp_ring_id);
 
 	rc = bnxt_hwrm_send_message(bp, &req, sizeof(req), BNXT_USE_CHIMP_MB);
 
@@ -2541,7 +2650,8 @@ void bnxt_free_nq_ring(struct bnxt *bp, struct bnxt_cp_ring_info *cpr)
 	struct bnxt_ring *cp_ring = cpr->cp_ring_struct;
 
 	bnxt_hwrm_ring_free(bp, cp_ring,
-			    HWRM_RING_FREE_INPUT_RING_TYPE_NQ);
+			    HWRM_RING_FREE_INPUT_RING_TYPE_NQ,
+			    INVALID_HW_RING_ID);
 	cp_ring->fw_ring_id = INVALID_HW_RING_ID;
 	memset(cpr->cp_desc_ring, 0, cpr->cp_ring_struct->ring_size *
 				     sizeof(*cpr->cp_desc_ring));
@@ -2554,7 +2664,8 @@ void bnxt_free_cp_ring(struct bnxt *bp, struct bnxt_cp_ring_info *cpr)
 	struct bnxt_ring *cp_ring = cpr->cp_ring_struct;
 
 	bnxt_hwrm_ring_free(bp, cp_ring,
-			HWRM_RING_FREE_INPUT_RING_TYPE_L2_CMPL);
+			HWRM_RING_FREE_INPUT_RING_TYPE_L2_CMPL,
+			INVALID_HW_RING_ID);
 	cp_ring->fw_ring_id = INVALID_HW_RING_ID;
 	memset(cpr->cp_desc_ring, 0, cpr->cp_ring_struct->ring_size *
 			sizeof(*cpr->cp_desc_ring));
@@ -2571,7 +2682,8 @@ void bnxt_free_hwrm_rx_ring(struct bnxt *bp, int queue_index)
 
 	if (ring->fw_ring_id != INVALID_HW_RING_ID) {
 		bnxt_hwrm_ring_free(bp, ring,
-				    HWRM_RING_FREE_INPUT_RING_TYPE_RX);
+				    HWRM_RING_FREE_INPUT_RING_TYPE_RX,
+				    cpr->cp_ring_struct->fw_ring_id);
 		ring->fw_ring_id = INVALID_HW_RING_ID;
 		if (BNXT_HAS_RING_GRPS(bp))
 			bp->grp_info[queue_index].rx_fw_ring_id =
@@ -2582,7 +2694,8 @@ void bnxt_free_hwrm_rx_ring(struct bnxt *bp, int queue_index)
 		bnxt_hwrm_ring_free(bp, ring,
 				    BNXT_CHIP_P5(bp) ?
 				    HWRM_RING_FREE_INPUT_RING_TYPE_RX_AGG :
-				    HWRM_RING_FREE_INPUT_RING_TYPE_RX);
+				    HWRM_RING_FREE_INPUT_RING_TYPE_RX,
+				    cpr->cp_ring_struct->fw_ring_id);
 		if (BNXT_HAS_RING_GRPS(bp))
 			bp->grp_info[queue_index].ag_fw_ring_id =
 							INVALID_HW_RING_ID;
@@ -2607,7 +2720,8 @@ bnxt_free_all_hwrm_rings(struct bnxt *bp)
 
 		if (ring->fw_ring_id != INVALID_HW_RING_ID) {
 			bnxt_hwrm_ring_free(bp, ring,
-					HWRM_RING_FREE_INPUT_RING_TYPE_TX);
+					HWRM_RING_FREE_INPUT_RING_TYPE_TX,
+					cpr->cp_ring_struct->fw_ring_id);
 			ring->fw_ring_id = INVALID_HW_RING_ID;
 			memset(txr->tx_desc_ring, 0,
 					txr->tx_ring_struct->ring_size *
diff --git a/drivers/net/bnxt/bnxt_hwrm.h b/drivers/net/bnxt/bnxt_hwrm.h
index 785e321..3d05acf 100644
--- a/drivers/net/bnxt/bnxt_hwrm.h
+++ b/drivers/net/bnxt/bnxt_hwrm.h
@@ -162,7 +162,8 @@ int bnxt_hwrm_ring_alloc(struct bnxt *bp,
 			 uint32_t stats_ctx_id, uint32_t cmpl_ring_id,
 			 uint16_t tx_cosq_id);
 int bnxt_hwrm_ring_free(struct bnxt *bp,
-			struct bnxt_ring *ring, uint32_t ring_type);
+			struct bnxt_ring *ring, uint32_t ring_type,
+			uint16_t cp_ring_id);
 int bnxt_hwrm_ring_grp_alloc(struct bnxt *bp, unsigned int idx);
 int bnxt_hwrm_ring_grp_free(struct bnxt *bp, unsigned int idx);
 
diff --git a/drivers/net/bnxt/bnxt_rxr.c b/drivers/net/bnxt/bnxt_rxr.c
index 30d22b7..c72545a 100644
--- a/drivers/net/bnxt/bnxt_rxr.c
+++ b/drivers/net/bnxt/bnxt_rxr.c
@@ -991,7 +991,9 @@ uint16_t bnxt_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 					cpr->cp_ring_struct->ring_mask,
 					cpr->valid);
 
-		if ((CMP_TYPE(rxcmp) >= CMPL_BASE_TYPE_RX_TPA_START_V2) &&
+		if (CMP_TYPE(rxcmp) == CMPL_BASE_TYPE_HWRM_DONE) {
+			PMD_DRV_LOG(ERR, "Rx flush done\n");
+		} else if ((CMP_TYPE(rxcmp) >= CMPL_BASE_TYPE_RX_TPA_START_V2) &&
 		     (CMP_TYPE(rxcmp) <= RX_TPA_V2_ABUF_CMPL_TYPE_RX_TPA_AGG)) {
 			rc = bnxt_rx_pkt(&rx_pkts[nb_rx_pkts], rxq, &raw_cons);
 			if (!rc)
@@ -1291,3 +1293,35 @@ int bnxt_init_one_rx_ring(struct bnxt_rx_queue *rxq)
 
 	return 0;
 }
+
+/* Sweep the Rx completion queue till HWRM_DONE for ring flush is received.
+ * The mbufs will not be freed in this call.
+ * They will be freed during ring free as a part of mem cleanup.
+ */
+int bnxt_flush_rx_cmp(struct bnxt_cp_ring_info *cpr)
+{
+	struct bnxt_ring *cp_ring_struct = cpr->cp_ring_struct;
+	uint32_t ring_mask = cp_ring_struct->ring_mask;
+	uint32_t raw_cons = cpr->cp_raw_cons;
+	struct rx_pkt_cmpl *rxcmp;
+	uint32_t nb_rx = 0;
+	uint32_t cons;
+
+	do {
+		cons = RING_CMP(cpr->cp_ring_struct, raw_cons);
+		rxcmp = (struct rx_pkt_cmpl *)&cpr->cp_desc_ring[cons];
+
+		if (CMP_TYPE(rxcmp) == CMPL_BASE_TYPE_HWRM_DONE)
+			return 1;
+
+		raw_cons = NEXT_RAW_CMP(raw_cons);
+		nb_rx++;
+	} while (nb_rx < ring_mask);
+
+	cpr->cp_raw_cons = raw_cons;
+
+	/* Ring the completion queue doorbell. */
+	bnxt_db_cq(cpr);
+
+	return 0;
+}
diff --git a/drivers/net/bnxt/bnxt_rxr.h b/drivers/net/bnxt/bnxt_rxr.h
index 06d1084..a6fdd77 100644
--- a/drivers/net/bnxt/bnxt_rxr.h
+++ b/drivers/net/bnxt/bnxt_rxr.h
@@ -100,6 +100,7 @@ int bnxt_init_rx_ring_struct(struct bnxt_rx_queue *rxq, unsigned int socket_id);
 int bnxt_init_one_rx_ring(struct bnxt_rx_queue *rxq);
 int bnxt_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 int bnxt_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id);
+int bnxt_flush_rx_cmp(struct bnxt_cp_ring_info *cpr);
 
 #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
 uint16_t bnxt_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
index 2810906..01db0cc 100644
--- a/drivers/net/bnxt/bnxt_txr.c
+++ b/drivers/net/bnxt/bnxt_txr.c
@@ -569,3 +569,39 @@ int bnxt_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 
 	return 0;
 }
+
+/* Sweep the Tx completion queue till HWRM_DONE for ring flush is received.
+ * The mbufs will not be freed in this call.
+ * They will be freed during ring free as a part of mem cleanup.
+ */
+int bnxt_flush_tx_cmp(struct bnxt_cp_ring_info *cpr)
+{
+	uint32_t raw_cons = cpr->cp_raw_cons;
+	uint32_t cons;
+	uint32_t nb_tx_pkts = 0;
+	struct tx_cmpl *txcmp;
+	struct cmpl_base *cp_desc_ring = cpr->cp_desc_ring;
+	struct bnxt_ring *cp_ring_struct = cpr->cp_ring_struct;
+	uint32_t ring_mask = cp_ring_struct->ring_mask;
+	uint32_t opaque = 0;
+
+	do {
+		cons = RING_CMPL(ring_mask, raw_cons);
+		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
+
+		opaque = rte_cpu_to_le_32(txcmp->opaque);
+		raw_cons = NEXT_RAW_CMP(raw_cons);
+
+		if (CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2)
+			nb_tx_pkts += opaque;
+		else if (CMP_TYPE(txcmp) == HWRM_CMPL_TYPE_HWRM_DONE)
+			return 1;
+	} while (nb_tx_pkts < ring_mask);
+
+	if (nb_tx_pkts) {
+		cpr->cp_raw_cons = raw_cons;
+		bnxt_db_cq(cpr);
+	}
+
+	return 0;
+}
diff --git a/drivers/net/bnxt/bnxt_txr.h b/drivers/net/bnxt/bnxt_txr.h
index 281a3e2..91e10db 100644
--- a/drivers/net/bnxt/bnxt_txr.h
+++ b/drivers/net/bnxt/bnxt_txr.h
@@ -56,6 +56,7 @@ uint16_t bnxt_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 int bnxt_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id);
 int bnxt_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id);
+int bnxt_flush_tx_cmp(struct bnxt_cp_ring_info *cpr);
 
 #define PKT_TX_OIP_IIP_TCP_UDP_CKSUM	(PKT_TX_TCP_CKSUM | PKT_TX_UDP_CKSUM | \
 					PKT_TX_IP_CKSUM | PKT_TX_OUTER_IP_CKSUM)
-- 
2.10.1

[dpdk-dev] [PATCH 2/3] net/bnxt: fix ver get HWRM command

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Fix HWRM_VER_GET command to handle DEV_NOT_RDY state.

Driver should fail probe if the device is not ready.
Conversely, the HWRM_VER_GET poll after reset can safely
retry until the existing timeout is exceeded.

Fixes: 804e746c7b73 ("net/bnxt: add hardware resource manager init code")
Cc: stable@dpdk.org

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Randy Schacher <stuart.schacher@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_hwrm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 02b0a21..5ef0845 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -1217,6 +1217,11 @@ int bnxt_hwrm_ver_get(struct bnxt *bp, uint32_t timeout)
 	else
 		HWRM_CHECK_RESULT();
 
+	if (resp->flags & HWRM_VER_GET_OUTPUT_FLAGS_DEV_NOT_RDY) {
+		rc = -EAGAIN;
+		goto error;
+	}
+
 	PMD_DRV_LOG(INFO, "%d.%d.%d:%d.%d.%d.%d\n",
 		resp->hwrm_intf_maj_8b, resp->hwrm_intf_min_8b,
 		resp->hwrm_intf_upd_8b, resp->hwrm_fw_maj_8b,
@@ -6045,6 +6050,10 @@ int bnxt_hwrm_poll_ver_get(struct bnxt *bp)
 	rc = bnxt_hwrm_send_message(bp, &req, sizeof(req), BNXT_USE_CHIMP_MB);
 
 	HWRM_CHECK_RESULT_SILENT();
+
+	if (resp->flags & HWRM_VER_GET_OUTPUT_FLAGS_DEV_NOT_RDY)
+		rc = -EAGAIN;
+
 	HWRM_UNLOCK();
 
 	return rc;
-- 
2.10.1

[dpdk-dev] [PATCH 3/3] net/bnxt: fix VF info allocation

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

1. Renamed bnxt_hwrm_alloc_vf_info()/bnxt_hwrm_free_vf_info to
   bnxt_alloc_vf_info()/bnxt_free_vf_info as it does not
   issue any HWRM command to fw.
2. Fix missing unlock when memory allocation fails.

Fixes: b7778e8a1c00 ("net/bnxt: refactor to properly allocate resources for PF/VF")
Cc: stable@dpdk.org

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c |  2 +-
 drivers/net/bnxt/bnxt_hwrm.c   | 94 ++++++++++++++++++++++++------------------
 drivers/net/bnxt/bnxt_hwrm.h   |  2 +-
 3 files changed, 57 insertions(+), 41 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index c55a2d8..3898849 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -1557,7 +1557,7 @@ static void bnxt_drv_uninit(struct bnxt *bp)
 	rte_memzone_free((const struct rte_memzone *)bp->rx_mem_zone);
 	bp->rx_mem_zone = NULL;
 
-	bnxt_hwrm_free_vf_info(bp);
+	bnxt_free_vf_info(bp);
 
 	rte_free(bp->grp_info);
 	bp->grp_info = NULL;
diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 5ef0845..c16edc8 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -788,10 +788,13 @@ static int bnxt_hwrm_ptp_qcfg(struct bnxt *bp)
 	return 0;
 }
 
-void bnxt_hwrm_free_vf_info(struct bnxt *bp)
+void bnxt_free_vf_info(struct bnxt *bp)
 {
 	int i;
 
+	if (bp->pf->vf_info == NULL)
+		return;
+
 	for (i = 0; i < bp->pf->max_vfs; i++) {
 		rte_free(bp->pf->vf_info[i].vlan_table);
 		bp->pf->vf_info[i].vlan_table = NULL;
@@ -802,6 +805,50 @@ void bnxt_hwrm_free_vf_info(struct bnxt *bp)
 	bp->pf->vf_info = NULL;
 }
 
+static int bnxt_alloc_vf_info(struct bnxt *bp, uint16_t max_vfs)
+{
+	struct bnxt_child_vf_info *vf_info = bp->pf->vf_info;
+	int i;
+
+	if (vf_info)
+		bnxt_free_vf_info(bp);
+
+	vf_info = rte_zmalloc("bnxt_vf_info", sizeof(*vf_info) * max_vfs, 0);
+	if (vf_info == NULL) {
+		PMD_DRV_LOG(ERR, "Failed to alloc vf info\n");
+		return -ENOMEM;
+	}
+
+	bp->pf->max_vfs = max_vfs;
+	for (i = 0; i < max_vfs; i++) {
+		vf_info[i].fid = bp->pf->first_vf_id + i;
+		vf_info[i].vlan_table = rte_zmalloc("VF VLAN table",
+						    getpagesize(), getpagesize());
+		if (vf_info[i].vlan_table == NULL) {
+			PMD_DRV_LOG(ERR, "Failed to alloc VLAN table for VF %d\n", i);
+			goto err;
+		}
+		rte_mem_lock_page(vf_info[i].vlan_table);
+
+		vf_info[i].vlan_as_table = rte_zmalloc("VF VLAN AS table",
+						       getpagesize(), getpagesize());
+		if (vf_info[i].vlan_as_table == NULL) {
+			PMD_DRV_LOG(ERR, "Failed to alloc VLAN AS table for VF %d\n", i);
+			goto err;
+		}
+		rte_mem_lock_page(vf_info[i].vlan_as_table);
+
+		STAILQ_INIT(&vf_info[i].filter);
+	}
+
+	bp->pf->vf_info = vf_info;
+
+	return 0;
+err:
+	bnxt_free_vf_info(bp);
+	return -ENOMEM;
+}
+
 static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 {
 	int rc = 0;
@@ -809,7 +856,6 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	struct hwrm_func_qcaps_output *resp = bp->hwrm_cmd_resp_addr;
 	uint16_t new_max_vfs;
 	uint32_t flags;
-	int i;
 
 	HWRM_PREP(&req, HWRM_FUNC_QCAPS, BNXT_USE_CHIMP_MB);
 
@@ -827,43 +873,9 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 		bp->pf->total_vfs = rte_le_to_cpu_16(resp->max_vfs);
 		new_max_vfs = bp->pdev->max_vfs;
 		if (new_max_vfs != bp->pf->max_vfs) {
-			if (bp->pf->vf_info)
-				bnxt_hwrm_free_vf_info(bp);
-			bp->pf->vf_info = rte_zmalloc("bnxt_vf_info",
-			    sizeof(bp->pf->vf_info[0]) * new_max_vfs, 0);
-			if (bp->pf->vf_info == NULL) {
-				PMD_DRV_LOG(ERR, "Alloc vf info fail\n");
-				HWRM_UNLOCK();
-				return -ENOMEM;
-			}
-			bp->pf->max_vfs = new_max_vfs;
-			for (i = 0; i < new_max_vfs; i++) {
-				bp->pf->vf_info[i].fid =
-					bp->pf->first_vf_id + i;
-				bp->pf->vf_info[i].vlan_table =
-					rte_zmalloc("VF VLAN table",
-						    getpagesize(),
-						    getpagesize());
-				if (bp->pf->vf_info[i].vlan_table == NULL)
-					PMD_DRV_LOG(ERR,
-					"Fail to alloc VLAN table for VF %d\n",
-					i);
-				else
-					rte_mem_lock_page(
-						bp->pf->vf_info[i].vlan_table);
-				bp->pf->vf_info[i].vlan_as_table =
-					rte_zmalloc("VF VLAN AS table",
-						    getpagesize(),
-						    getpagesize());
-				if (bp->pf->vf_info[i].vlan_as_table == NULL)
-					PMD_DRV_LOG(ERR,
-					"Alloc VLAN AS table for VF %d fail\n",
-					i);
-				else
-					rte_mem_lock_page(
-					      bp->pf->vf_info[i].vlan_as_table);
-				STAILQ_INIT(&bp->pf->vf_info[i].filter);
-			}
+			rc = bnxt_alloc_vf_info(bp, new_max_vfs);
+			if (rc)
+				goto unlock;
 		}
 	}
 
@@ -922,6 +934,7 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	if (flags & HWRM_FUNC_QCAPS_OUTPUT_FLAGS_LINK_ADMIN_STATUS_SUPPORTED)
 		bp->fw_cap |= BNXT_FW_CAP_LINK_ADMIN;
 
+unlock:
 	HWRM_UNLOCK();
 
 	return rc;
@@ -932,6 +945,9 @@ int bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	int rc;
 
 	rc = __bnxt_hwrm_func_qcaps(bp);
+	if (rc == -ENOMEM)
+		return rc;
+
 	if (!rc && bp->hwrm_spec_code >= HWRM_SPEC_CODE_1_8_3) {
 		rc = bnxt_alloc_ctx_mem(bp);
 		if (rc)
diff --git a/drivers/net/bnxt/bnxt_hwrm.h b/drivers/net/bnxt/bnxt_hwrm.h
index 3d05acf..0c2e32c 100644
--- a/drivers/net/bnxt/bnxt_hwrm.h
+++ b/drivers/net/bnxt/bnxt_hwrm.h
@@ -297,7 +297,7 @@ int bnxt_hwrm_port_phy_qcaps(struct bnxt *bp);
 int bnxt_hwrm_oem_cmd(struct bnxt *bp, uint32_t entry_num);
 int bnxt_clear_one_vnic_filter(struct bnxt *bp,
 			       struct bnxt_filter_info *filter);
-void bnxt_hwrm_free_vf_info(struct bnxt *bp);
+void bnxt_free_vf_info(struct bnxt *bp);
 int bnxt_hwrm_first_vf_id_query(struct bnxt *bp, uint16_t fid,
 				uint16_t *first_vf_id);
 int bnxt_hwrm_cfa_pair_alloc(struct bnxt *bp, struct bnxt_representor *rep);
-- 
2.10.1

Re: [dpdk-dev] [PATCH 0/3] bnxt fixes

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Thu, Mar 4, 2021 at 12:45 AM Kalesh A P
<kalesh-anakkur.purayil@broadcom.com> wrote:
>
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>
> Please apply.
Patchset applied to dpdk-next-net-brcm. Thanks

>
> Ajit Khaparde (1):
>   net/bnxt: check flush status during ring free
>
> Kalesh AP (2):
>   net/bnxt: fix ver get HWRM command
>   net/bnxt: fix VF info allocation
>
>  drivers/net/bnxt/bnxt_ethdev.c |   2 +-
>  drivers/net/bnxt/bnxt_hwrm.c   | 231 +++++++++++++++++++++++++++++++++--------
>  drivers/net/bnxt/bnxt_hwrm.h   |   5 +-
>  drivers/net/bnxt/bnxt_rxr.c    |  36 ++++++-
>  drivers/net/bnxt/bnxt_rxr.h    |   1 +
>  drivers/net/bnxt/bnxt_txr.c    |  36 +++++++
>  drivers/net/bnxt/bnxt_txr.h    |   1 +
>  7 files changed, 262 insertions(+), 50 deletions(-)
>
> --
> 2.10.1
>

Re: [dpdk-dev] [PATCH 2/3] net/bnxt: fix ver get HWRM command

[David Marchand]

Hello,

On Thu, Mar 4, 2021 at 9:45 AM Kalesh A P
<kalesh-anakkur.purayil@broadcom.com> wrote:
>
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>
> Fix HWRM_VER_GET command to handle DEV_NOT_RDY state.
>
> Driver should fail probe if the device is not ready.
> Conversely, the HWRM_VER_GET poll after reset can safely
> retry until the existing timeout is exceeded.
>
> Fixes: 804e746c7b73 ("net/bnxt: add hardware resource manager init code")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Randy Schacher <stuart.schacher@broadcom.com>
> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>

This patch makes probing fail on a RHEL9 kernel with firmwares:
firmware-version: 20.6.143.0/pkg 20.06.04.06
and
firmware-version: 20.6.112.0

Simply reverting the patch fixes probing in my env.

This was reported by our QE team which is doing sanity checks on ovs
2.17 which uses dpdk v21.11.
https://bugzilla.redhat.com/show_bug.cgi?id=2055531


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/3] net/bnxt: fix ver get HWRM command

[David Marchand]

Kalesh, Ajit,

On Tue, Mar 1, 2022 at 1:53 PM David Marchand <david.marchand@redhat.com> wrote:
> On Thu, Mar 4, 2021 at 9:45 AM Kalesh A P
> <kalesh-anakkur.purayil@broadcom.com> wrote:
> >
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > Fix HWRM_VER_GET command to handle DEV_NOT_RDY state.
> >
> > Driver should fail probe if the device is not ready.
> > Conversely, the HWRM_VER_GET poll after reset can safely
> > retry until the existing timeout is exceeded.
> >
> > Fixes: 804e746c7b73 ("net/bnxt: add hardware resource manager init code")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Reviewed-by: Randy Schacher <stuart.schacher@broadcom.com>
> > Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
>
> This patch makes probing fail on a RHEL9 kernel with firmwares:
> firmware-version: 20.6.143.0/pkg 20.06.04.06
> and
> firmware-version: 20.6.112.0
>
> Simply reverting the patch fixes probing in my env.
>
> This was reported by our QE team which is doing sanity checks on ovs
> 2.17 which uses dpdk v21.11.
> https://bugzilla.redhat.com/show_bug.cgi?id=2055531

QE confirmed reverting this patch fixed initialisation in their setup.
I reproduced the issue on another system with RHEL 8.5, and confirmed
that reverting the patch or upgrading the firmware fix the issue.

I can understand new features in a PMD might require newer firmwares.
But breaking compatibility with older firmwares is a big no.

If there is no other option, I will send a revert for this patch.


Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/3] net/bnxt: fix ver get HWRM command

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

On Thu, Mar 17, 2022 at 3:11 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Kalesh, Ajit,
>
> On Tue, Mar 1, 2022 at 1:53 PM David Marchand <david.marchand@redhat.com> wrote:
> > On Thu, Mar 4, 2021 at 9:45 AM Kalesh A P
> > <kalesh-anakkur.purayil@broadcom.com> wrote:
> > >
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > Fix HWRM_VER_GET command to handle DEV_NOT_RDY state.
> > >
> > > Driver should fail probe if the device is not ready.
> > > Conversely, the HWRM_VER_GET poll after reset can safely
> > > retry until the existing timeout is exceeded.
> > >
> > > Fixes: 804e746c7b73 ("net/bnxt: add hardware resource manager init code")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > > Reviewed-by: Randy Schacher <stuart.schacher@broadcom.com>
> > > Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> >
> > This patch makes probing fail on a RHEL9 kernel with firmwares:
> > firmware-version: 20.6.143.0/pkg 20.06.04.06
> > and
> > firmware-version: 20.6.112.0
Oh wow! That's really old FW.

> >
> > Simply reverting the patch fixes probing in my env.
> >
> > This was reported by our QE team which is doing sanity checks on ovs
> > 2.17 which uses dpdk v21.11.
> > https://bugzilla.redhat.com/show_bug.cgi?id=2055531
>
> QE confirmed reverting this patch fixed initialisation in their setup.
> I reproduced the issue on another system with RHEL 8.5, and confirmed
> that reverting the patch or upgrading the firmware fix the issue.
>
> I can understand new features in a PMD might require newer firmwares.
> But breaking compatibility with older firmwares is a big no.
I agree.
Ideally this should not have been sent as a fix targeting the super-old commit
804e746c7b73.

>
> If there is no other option, I will send a revert for this patch.
A version check may help avoid a complete revert.
But that will require some testing, which means we are looking at
mid-next week for the fix.

Submit the revert patch, just in case it takes longer than that.

Thanks
Ajit


>
>
> Thanks.
>
> --
> David Marchand
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

Re: [dpdk-dev] [PATCH 2/3] net/bnxt: fix ver get HWRM command

[David Marchand]

Hi Ajit,

On Thu, Mar 17, 2022 at 5:48 PM Ajit Khaparde
<ajit.khaparde@broadcom.com> wrote:
> > On Tue, Mar 1, 2022 at 1:53 PM David Marchand <david.marchand@redhat.com> wrote:
> > > On Thu, Mar 4, 2021 at 9:45 AM Kalesh A P
> > > <kalesh-anakkur.purayil@broadcom.com> wrote:
> > > >
> > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > >
> > > > Fix HWRM_VER_GET command to handle DEV_NOT_RDY state.
> > > >
> > > > Driver should fail probe if the device is not ready.
> > > > Conversely, the HWRM_VER_GET poll after reset can safely
> > > > retry until the existing timeout is exceeded.
> > > >
> > > > Fixes: 804e746c7b73 ("net/bnxt: add hardware resource manager init code")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > > > Reviewed-by: Randy Schacher <stuart.schacher@broadcom.com>
> > > > Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> > >
> > > This patch makes probing fail on a RHEL9 kernel with firmwares:
> > > firmware-version: 20.6.143.0/pkg 20.06.04.06
> > > and
> > > firmware-version: 20.6.112.0
> Oh wow! That's really old FW.

Well, hard to tell it is old, from my side.

I found a few nics in our lab that show similar firmware versions.
As you can see, QE have some servers with such nics too.


> > If there is no other option, I will send a revert for this patch.
> A version check may help avoid a complete revert.
> But that will require some testing, which means we are looking at
> mid-next week for the fix.
>
> Submit the revert patch, just in case it takes longer than that.

I'll do that.
Thanks.


-- 
David Marchand