[dpdk-dev] [PATCH 0/8] bnxt PMD fixes

[dpdk-dev] [PATCH 0/8] bnxt PMD fixes

[Kalesh A P]

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

This patchset contains bug fixes in bnxt PMD.

Ajit Khaparde (1):
  net/bnxt: fix switch domain allocation

Damodharam Ammepalli (3):
  net/bnxt: allow Tx only or Rx only configs in PMD
  net/bnxt: disallow MTU change when device is started
  net/bnxt: cleanups in MTU set callback

Kalesh AP (2):
  net/bnxt: reduce the verbosity of a log
  net/bnxt: fix setting forced speed

Somnath Kotur (2):
  net/bnxt: remove assert for zero data len in Tx path
  net/bnxt: fix the check for autoneg enablement in the PHY FW

 drivers/net/bnxt/bnxt_ethdev.c | 95 ++++++++++++++++++------------------------
 drivers/net/bnxt/bnxt_hwrm.c   | 45 +++++++++++++-------
 drivers/net/bnxt/bnxt_hwrm.h   |  3 ++
 drivers/net/bnxt/bnxt_rxq.c    |  9 +---
 drivers/net/bnxt/bnxt_txr.c    | 29 +++++++++++--
 5 files changed, 101 insertions(+), 80 deletions(-)

-- 
2.10.1

[dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Kalesh A P]

From: Somnath Kotur <somnath.kotur@broadcom.com>

Currently the PMD tries to detect a potential 0 byte DMA by
using RTE_VERIFY.
But since RTE_VERIFY internally calls rte_panic() it is fatal to
the application and some applications want to avoid that.
So return an error from the bnxt xmit handler if such a bad pkt is
encountered by logging an error message, dumping the pkt header and
dump the current stack as well

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
index 7a7196a..67e0167 100644
--- a/drivers/net/bnxt/bnxt_txr.c
+++ b/drivers/net/bnxt/bnxt_txr.c
@@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
 	return false;
 }
 
+static bool
+bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
+{
+	const char *type_str = "Data len";
+	uint16_t len_to_check = tx_pkt->data_len;
+
+	if (data_len_chk == 0) {
+		type_str = "TSO Seg size";
+		len_to_check = tx_pkt->tso_segsz;
+	}
+
+	if (len_to_check == 0) {
+		PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
+		rte_pktmbuf_dump(stdout, tx_pkt, 64);
+		rte_dump_stack();
+		return true;
+	}
+	return false;
+}
+
 static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
 				struct bnxt_tx_queue *txq,
 				uint16_t *coal_pkts,
@@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
 	}
 
 	/* Check non zero data_len */
-	RTE_VERIFY(tx_pkt->data_len);
+	if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
+		return -EIO;
 
 	prod = RING_IDX(ring, txr->tx_raw_prod);
 	tx_buf = &txr->tx_buf_ring[prod];
@@ -256,7 +277,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
 			 */
 			txbd1->kid_or_ts_low_hdr_size = hdr_size >> 1;
 			txbd1->kid_or_ts_high_mss = tx_pkt->tso_segsz;
-			RTE_VERIFY(txbd1->kid_or_ts_high_mss);
+			if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 0)))
+				return -EIO;
 
 		} else if ((tx_pkt->ol_flags & PKT_TX_OIP_IIP_TCP_UDP_CKSUM) ==
 			   PKT_TX_OIP_IIP_TCP_UDP_CKSUM) {
@@ -330,7 +352,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
 	m_seg = tx_pkt->next;
 	while (m_seg) {
 		/* Check non zero data_len */
-		RTE_VERIFY(m_seg->data_len);
+		if (unlikely(bnxt_zero_data_len_tso_segsz(m_seg, 1)))
+			return -EIO;
 		txr->tx_raw_prod = RING_NEXT(txr->tx_raw_prod);
 
 		prod = RING_IDX(ring, txr->tx_raw_prod);
-- 
2.10.1

[dpdk-dev] [PATCH 2/8] net/bnxt: fix switch domain allocation

[Kalesh A P]

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

Allocate switch domain after the trusted VF capability is queried
from the FW. Currently we are calling the function earlier.
Since the switch domain is allocated only for PFs or trusted VF,
the current location of code fails to allocate the domain during init.
But during cleanup we try to free the domain incorrectly.
Fix the behavior by changing the sequence of function calls.

Fixes: 3127f99274b67 ("net/bnxt: refactor init/uninit")
Cc: stable@dpdk.org

Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 45 +++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 0f0f40b..34f2149 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -5287,6 +5287,25 @@ bnxt_init_locks(struct bnxt *bp)
 	return err;
 }
 
+/* This should be called after we have queried trusted VF cap */
+static int bnxt_alloc_switch_domain(struct bnxt *bp)
+{
+	int rc = 0;
+
+	if (BNXT_PF(bp) || BNXT_VF_IS_TRUSTED(bp)) {
+		rc = rte_eth_switch_domain_alloc(&bp->switch_domain_id);
+		if (rc)
+			PMD_DRV_LOG(ERR,
+				    "Failed to alloc switch domain: %d\n", rc);
+		else
+			PMD_DRV_LOG(INFO,
+				    "Switch domain allocated %d\n",
+				    bp->switch_domain_id);
+	}
+
+	return rc;
+}
+
 static int bnxt_init_resources(struct bnxt *bp, bool reconfig_dev)
 {
 	int rc = 0;
@@ -5295,6 +5314,10 @@ static int bnxt_init_resources(struct bnxt *bp, bool reconfig_dev)
 	if (rc)
 		return rc;
 
+	rc = bnxt_alloc_switch_domain(bp);
+	if (rc)
+		return rc;
+
 	if (!reconfig_dev) {
 		rc = bnxt_setup_mac_addr(bp->eth_dev);
 		if (rc)
@@ -5734,24 +5757,6 @@ bnxt_parse_dev_args(struct bnxt *bp, struct rte_devargs *devargs)
 	return ret;
 }
 
-static int bnxt_alloc_switch_domain(struct bnxt *bp)
-{
-	int rc = 0;
-
-	if (BNXT_PF(bp) || BNXT_VF_IS_TRUSTED(bp)) {
-		rc = rte_eth_switch_domain_alloc(&bp->switch_domain_id);
-		if (rc)
-			PMD_DRV_LOG(ERR,
-				    "Failed to alloc switch domain: %d\n", rc);
-		else
-			PMD_DRV_LOG(INFO,
-				    "Switch domain allocated %d\n",
-				    bp->switch_domain_id);
-	}
-
-	return rc;
-}
-
 /* Allocate and initialize various fields in bnxt struct that
  * need to be allocated/destroyed only once in the lifetime of the driver
  */
@@ -5828,10 +5833,6 @@ static int bnxt_drv_init(struct rte_eth_dev *eth_dev)
 	if (rc)
 		return rc;
 
-	rc = bnxt_alloc_switch_domain(bp);
-	if (rc)
-		return rc;
-
 	return rc;
 }
 
-- 
2.10.1

[dpdk-dev] [PATCH 3/8] net/bnxt: reduce the verbosity of a log

[Kalesh A P]

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

Broadcom HW drops packets when there are no descriptors available.
It does not matter what flag the application specifies in "rx_drop_en"
when configuring the Rx ring.

Reduce the verbosity of the log to print the status of the "rx_drop_en"
when configuring the Rx ring.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_rxq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_rxq.c b/drivers/net/bnxt/bnxt_rxq.c
index ef2ef83..f4b6417 100644
--- a/drivers/net/bnxt/bnxt_rxq.c
+++ b/drivers/net/bnxt/bnxt_rxq.c
@@ -360,9 +360,8 @@ int bnxt_rx_queue_setup_op(struct rte_eth_dev *eth_dev,
 	rxq->rx_free_thresh =
 		RTE_MIN(rte_align32pow2(nb_desc) / 4, RTE_BNXT_MAX_RX_BURST);
 
-	if (rx_conf->rx_drop_en != BNXT_DEFAULT_RX_DROP_EN)
-		PMD_DRV_LOG(NOTICE,
-			    "Per-queue config of drop-en is not supported.\n");
+	PMD_DRV_LOG(DEBUG,
+		    "App supplied RXQ drop_en status : %d\n", rx_conf->rx_drop_en);
 	rxq->drop_en = BNXT_DEFAULT_RX_DROP_EN;
 
 	PMD_DRV_LOG(DEBUG, "RX Buf MTU %d\n", eth_dev->data->mtu);
-- 
2.10.1

[dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only configs in PMD

[Kalesh A P]

From: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>

Currently, we fail the init/probe of pmd if eth_dev->data->nb_tx_queues
or eth_dev->data->nb_rx_queues is 0. We are removing this check.

Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
Cc: stable@dpdk.org

Signed-off-by: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 34f2149..8181e1f 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -723,7 +723,7 @@ static int bnxt_alloc_prev_ring_stats(struct bnxt *bp)
 					     sizeof(struct bnxt_ring_stats) *
 					     bp->tx_cp_nr_rings,
 					     0);
-	if (bp->prev_tx_ring_stats == NULL)
+	if (bp->tx_cp_nr_rings > 0 && bp->prev_tx_ring_stats == NULL)
 		goto error;
 
 	return 0;
@@ -1567,11 +1567,6 @@ int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
 	int vlan_mask = 0;
 	int rc, retry_cnt = BNXT_IF_CHANGE_RETRY_COUNT;
 
-	if (!eth_dev->data->nb_tx_queues || !eth_dev->data->nb_rx_queues) {
-		PMD_DRV_LOG(ERR, "Queues are not configured yet!\n");
-		return -EINVAL;
-	}
-
 	if (bp->rx_cp_nr_rings > RTE_ETHDEV_QUEUE_STAT_CNTRS)
 		PMD_DRV_LOG(ERR,
 			    "RxQ cnt %d > RTE_ETHDEV_QUEUE_STAT_CNTRS %d\n",
-- 
2.10.1

[dpdk-dev] [PATCH 5/8] net/bnxt: fix setting forced speed

[Kalesh A P]

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

The "active_fec_signal_mode" in HWRM_PORT_PHY_QCFG response
does not return correct value till the link is up. Driver cannot
rely on active_fec_signal_mode while setting forced speed.

While setting forced speed of 50G/100G/200G, check if PAM4 speeds
are supported for the port first and then populate the HWRM request
accordingly.

Also, If PAM4 speed is supported, use PAM4 supported speed while
reporting speed capabilities.

Fixes: c23f9ded0391 ("net/bnxt: support 200G PAM4 link")
Cc: stable@dpdk.org

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 11 ++++++-----
 drivers/net/bnxt/bnxt_hwrm.c   | 27 +++++++++++++++++++--------
 drivers/net/bnxt/bnxt_hwrm.h   |  3 +++
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 8181e1f..4e4791c 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -903,6 +903,7 @@ static int bnxt_shutdown_nic(struct bnxt *bp)
 
 uint32_t bnxt_get_speed_capabilities(struct bnxt *bp)
 {
+	uint32_t pam4_link_speed = 0;
 	uint32_t link_speed = 0;
 	uint32_t speed_capa = 0;
 
@@ -912,8 +913,8 @@ uint32_t bnxt_get_speed_capabilities(struct bnxt *bp)
 	link_speed = bp->link_info->support_speeds;
 
 	/* If PAM4 is configured, use PAM4 supported speed */
-	if (link_speed == 0 && bp->link_info->support_pam4_speeds > 0)
-		link_speed = bp->link_info->support_pam4_speeds;
+	if (bp->link_info->support_pam4_speeds > 0)
+		pam4_link_speed = bp->link_info->support_pam4_speeds;
 
 	if (link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_LINK_SPEED_100MB)
 		speed_capa |= RTE_ETH_LINK_SPEED_100M;
@@ -935,11 +936,11 @@ uint32_t bnxt_get_speed_capabilities(struct bnxt *bp)
 		speed_capa |= RTE_ETH_LINK_SPEED_50G;
 	if (link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_SPEEDS_100GB)
 		speed_capa |= RTE_ETH_LINK_SPEED_100G;
-	if (link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_50G)
+	if (pam4_link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_50G)
 		speed_capa |= RTE_ETH_LINK_SPEED_50G;
-	if (link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_100G)
+	if (pam4_link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_100G)
 		speed_capa |= RTE_ETH_LINK_SPEED_100G;
-	if (link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_200G)
+	if (pam4_link_speed & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_200G)
 		speed_capa |= RTE_ETH_LINK_SPEED_200G;
 
 	if (bp->link_info->auto_mode ==
diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 9eb8b8d..206ac20 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -2970,7 +2970,7 @@ static uint16_t bnxt_check_eth_link_autoneg(uint32_t conf_link)
 }
 
 static uint16_t bnxt_parse_eth_link_speed(uint32_t conf_link_speed,
-					  uint16_t pam4_link)
+					  struct bnxt_link_info *link_info)
 {
 	uint16_t eth_link_speed = 0;
 
@@ -3009,18 +3009,29 @@ static uint16_t bnxt_parse_eth_link_speed(uint32_t conf_link_speed,
 			HWRM_PORT_PHY_CFG_INPUT_FORCE_LINK_SPEED_40GB;
 		break;
 	case RTE_ETH_LINK_SPEED_50G:
-		eth_link_speed = pam4_link ?
-			HWRM_PORT_PHY_CFG_INPUT_FORCE_PAM4_LINK_SPEED_50GB :
-			HWRM_PORT_PHY_CFG_INPUT_FORCE_LINK_SPEED_50GB;
+		if (link_info->support_pam4_speeds &
+		    HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_50G) {
+			eth_link_speed = HWRM_PORT_PHY_CFG_INPUT_FORCE_PAM4_LINK_SPEED_50GB;
+			link_info->link_signal_mode = BNXT_SIG_MODE_PAM4;
+		} else {
+			eth_link_speed = HWRM_PORT_PHY_CFG_INPUT_FORCE_LINK_SPEED_50GB;
+			link_info->link_signal_mode = BNXT_SIG_MODE_NRZ;
+		}
 		break;
 	case RTE_ETH_LINK_SPEED_100G:
-		eth_link_speed = pam4_link ?
-			HWRM_PORT_PHY_CFG_INPUT_FORCE_PAM4_LINK_SPEED_100GB :
-			HWRM_PORT_PHY_CFG_INPUT_FORCE_LINK_SPEED_100GB;
+		if (link_info->support_pam4_speeds &
+		    HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_PAM4_SPEEDS_100G) {
+			eth_link_speed = HWRM_PORT_PHY_CFG_INPUT_FORCE_PAM4_LINK_SPEED_100GB;
+			link_info->link_signal_mode = BNXT_SIG_MODE_PAM4;
+		} else {
+			eth_link_speed = HWRM_PORT_PHY_CFG_INPUT_FORCE_LINK_SPEED_100GB;
+			link_info->link_signal_mode = BNXT_SIG_MODE_NRZ;
+		}
 		break;
 	case RTE_ETH_LINK_SPEED_200G:
 		eth_link_speed =
 			HWRM_PORT_PHY_CFG_INPUT_FORCE_PAM4_LINK_SPEED_200GB;
+		link_info->link_signal_mode = BNXT_SIG_MODE_PAM4;
 		break;
 	default:
 		PMD_DRV_LOG(ERR,
@@ -3242,7 +3253,7 @@ int bnxt_set_hwrm_link_config(struct bnxt *bp, bool link_up)
 		autoneg = 0;
 
 	speed = bnxt_parse_eth_link_speed(dev_conf->link_speeds,
-					  bp->link_info->link_signal_mode);
+					  bp->link_info);
 	link_req.phy_flags = HWRM_PORT_PHY_CFG_INPUT_FLAGS_RESET_PHY;
 	/* Autoneg can be done only when the FW allows. */
 	if (autoneg == 1 &&
diff --git a/drivers/net/bnxt/bnxt_hwrm.h b/drivers/net/bnxt/bnxt_hwrm.h
index 77f8521..a82d9fb 100644
--- a/drivers/net/bnxt/bnxt_hwrm.h
+++ b/drivers/net/bnxt/bnxt_hwrm.h
@@ -141,6 +141,9 @@ struct bnxt_pf_resource_info {
 	 BNXT_TUNNELED_OFFLOADS_CAP_GRE_EN(bp)   &&		\
 	 BNXT_TUNNELED_OFFLOADS_CAP_IPINIP_EN(bp))
 
+#define BNXT_SIG_MODE_NRZ	HWRM_PORT_PHY_QCFG_OUTPUT_SIGNAL_MODE_NRZ
+#define BNXT_SIG_MODE_PAM4	HWRM_PORT_PHY_QCFG_OUTPUT_SIGNAL_MODE_PAM4
+
 int bnxt_hwrm_cfa_l2_clear_rx_mask(struct bnxt *bp,
 				   struct bnxt_vnic_info *vnic);
 int bnxt_hwrm_cfa_l2_set_rx_mask(struct bnxt *bp, struct bnxt_vnic_info *vnic,
-- 
2.10.1

[dpdk-dev] [PATCH 6/8] net/bnxt: disallow MTU change when device is started

[Kalesh A P]

From: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>

With this patch, bnxt_mtu_set_op() will return an error code if the
device has already started. The user application will have to take
care to bring down device before invoking the mtu_set()

Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
Cc: stable@dpdk.org

Signed-off-by: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 4e4791c..f040cdc 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -3019,9 +3019,7 @@ bnxt_tx_burst_mode_get(struct rte_eth_dev *dev, __rte_unused uint16_t queue_id,
 
 int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
 {
-	uint32_t overhead = BNXT_MAX_PKT_LEN - BNXT_MAX_MTU;
 	struct bnxt *bp = eth_dev->data->dev_private;
-	uint32_t new_pkt_size;
 	uint32_t rc;
 	uint32_t i;
 
@@ -3029,25 +3027,15 @@ int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
 	if (rc)
 		return rc;
 
+	/* Return if port is active */
+	if (eth_dev->data->dev_started) {
+		PMD_DRV_LOG(ERR, "Stop port before changing MTU\n");
+		return -EPERM;
+	}
+
 	/* Exit if receive queues are not configured yet */
 	if (!eth_dev->data->nb_rx_queues)
-		return rc;
-
-	new_pkt_size = new_mtu + overhead;
-
-	/*
-	 * Disallow any MTU change that would require scattered receive support
-	 * if it is not already enabled.
-	 */
-	if (eth_dev->data->dev_started &&
-	    !eth_dev->data->scattered_rx &&
-	    (new_pkt_size >
-	     eth_dev->data->min_rx_buf_size - RTE_PKTMBUF_HEADROOM)) {
-		PMD_DRV_LOG(ERR,
-			    "MTU change would require scattered rx support. ");
-		PMD_DRV_LOG(ERR, "Stop port before changing MTU.\n");
-		return -EINVAL;
-	}
+		return -ENOTSUP;
 
 	if (new_mtu > RTE_ETHER_MTU)
 		bp->flags |= BNXT_FLAG_JUMBO;
@@ -3056,7 +3044,7 @@ int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
 
 	/* Is there a change in mtu setting? */
 	if (eth_dev->data->mtu == new_mtu)
-		return rc;
+		return 0;
 
 	for (i = 0; i < bp->nr_vnics; i++) {
 		struct bnxt_vnic_info *vnic = &bp->vnic_info[i];
-- 
2.10.1

[dpdk-dev] [PATCH 7/8] net/bnxt: cleanups in MTU set callback

[Kalesh A P]

From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>

Minor cleanup in bnxt_mtu_set_op() to move pre-mature
setting of jumbo flag post mtu check and remove
a redundant mtu set operation from rxq vnic configs.

Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
Cc: stable@dpdk.org

Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 8 ++++----
 drivers/net/bnxt/bnxt_rxq.c    | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index f040cdc..e275d3a 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -3037,15 +3037,15 @@ int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
 	if (!eth_dev->data->nb_rx_queues)
 		return -ENOTSUP;
 
+	/* Is there a change in mtu setting? */
+	if (eth_dev->data->mtu == new_mtu)
+		return 0;
+
 	if (new_mtu > RTE_ETHER_MTU)
 		bp->flags |= BNXT_FLAG_JUMBO;
 	else
 		bp->flags &= ~BNXT_FLAG_JUMBO;
 
-	/* Is there a change in mtu setting? */
-	if (eth_dev->data->mtu == new_mtu)
-		return 0;
-
 	for (i = 0; i < bp->nr_vnics; i++) {
 		struct bnxt_vnic_info *vnic = &bp->vnic_info[i];
 		uint16_t size = 0;
diff --git a/drivers/net/bnxt/bnxt_rxq.c b/drivers/net/bnxt/bnxt_rxq.c
index f4b6417..fabbbd4 100644
--- a/drivers/net/bnxt/bnxt_rxq.c
+++ b/drivers/net/bnxt/bnxt_rxq.c
@@ -402,10 +402,6 @@ int bnxt_rx_queue_setup_op(struct rte_eth_dev *eth_dev,
 	rxq->rx_started = rxq->rx_deferred_start ? false : true;
 	rxq->vnic = BNXT_GET_DEFAULT_VNIC(bp);
 
-	/* Configure mtu if it is different from what was configured before */
-	if (!queue_idx)
-		bnxt_mtu_set_op(eth_dev, eth_dev->data->mtu);
-
 	return 0;
 err:
 	bnxt_rx_queue_release_op(eth_dev, queue_idx);
-- 
2.10.1

[dpdk-dev] [PATCH 8/8] net/bnxt: fix the check for autoneg enablement in the PHY FW

[Kalesh A P]

From: Somnath Kotur <somnath.kotur@broadcom.com>

The current combination of checks to determine whether autoneg is
enabled in the card is a bit convoluted and may become incorrect
as well in the future as one of the fields being used - auto_link_speed
might become deprecated.
Switch to using the 'auto_mode' field obtained from the response of
HWRM_PHY_QCFG cmd as that is always deterministically set by the PHY
FW.
Fixed a bug in the 40G check to only look for the bit setting and
not the actual value.
Also, check the forced speeds first before trying to enforce the
auto speeds

Allow the user to set autoneg speed in all cases except for PAM4 200G
as PAM4 200G will come up only in forced mode.

Fixes: c23f9ded0391 ("net/bnxt: support 200G PAM4 link")

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

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 206ac20..9c52573 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -3235,9 +3235,11 @@ int bnxt_set_hwrm_link_config(struct bnxt *bp, bool link_up)
 	if (!link_up)
 		goto port_phy_cfg;
 
+	/* Get user requested autoneg setting */
 	autoneg = bnxt_check_eth_link_autoneg(dev_conf->link_speeds);
+
 	if (BNXT_CHIP_P5(bp) &&
-	    dev_conf->link_speeds == RTE_ETH_LINK_SPEED_40G) {
+	    dev_conf->link_speeds & RTE_ETH_LINK_SPEED_40G) {
 		/* 40G is not supported as part of media auto detect.
 		 * The speed should be forced and autoneg disabled
 		 * to configure 40G speed.
@@ -3246,11 +3248,13 @@ int bnxt_set_hwrm_link_config(struct bnxt *bp, bool link_up)
 		autoneg = 0;
 	}
 
-	/* No auto speeds and no auto_pam4_link. Disable autoneg */
-	if (bp->link_info->auto_link_speed == 0 &&
-	    bp->link_info->link_signal_mode &&
-	    bp->link_info->auto_pam4_link_speed_mask == 0)
+	/* Override based on current Autoneg setting in PHY for 200G */
+	if (autoneg == 1 && BNXT_CHIP_P5(bp) && bp->link_info->auto_mode == 0 &&
+	    bp->link_info->force_pam4_link_speed ==
+	    HWRM_PORT_PHY_CFG_INPUT_FORCE_PAM4_LINK_SPEED_200GB) {
 		autoneg = 0;
+		PMD_DRV_LOG(DEBUG, "Disabling autoneg for 200G\n");
+	}
 
 	speed = bnxt_parse_eth_link_speed(dev_conf->link_speeds,
 					  bp->link_info);
@@ -3283,14 +3287,14 @@ int bnxt_set_hwrm_link_config(struct bnxt *bp, bool link_up)
 		else if (bp->link_info->force_pam4_link_speed)
 			link_req.link_speed =
 				bp->link_info->force_pam4_link_speed;
+		else if (bp->link_info->force_link_speed)
+			link_req.link_speed = bp->link_info->force_link_speed;
 		else if (bp->link_info->auto_pam4_link_speed_mask)
 			link_req.link_speed =
 				bp->link_info->auto_pam4_link_speed_mask;
 		else if (bp->link_info->support_pam4_speeds)
 			link_req.link_speed =
 				bp->link_info->support_pam4_speeds;
-		else if (bp->link_info->force_link_speed)
-			link_req.link_speed = bp->link_info->force_link_speed;
 		else
 			link_req.link_speed = bp->link_info->auto_link_speed;
 		/* Auto PAM4 link speed is zero, but auto_link_speed is not
-- 
2.10.1

Re: [dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Ferruh Yigit]

On 6/15/2022 3:56 PM, Kalesh A P wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> From: Somnath Kotur <somnath.kotur@broadcom.com>
> 
> Currently the PMD tries to detect a potential 0 byte DMA by
> using RTE_VERIFY.
> But since RTE_VERIFY internally calls rte_panic() it is fatal to
> the application and some applications want to avoid that.
> So return an error from the bnxt xmit handler if such a bad pkt is
> encountered by logging an error message, dumping the pkt header and
> dump the current stack as well
> 
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>   drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> index 7a7196a..67e0167 100644
> --- a/drivers/net/bnxt/bnxt_txr.c
> +++ b/drivers/net/bnxt/bnxt_txr.c
> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
>          return false;
>   }
> 
> +static bool
> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> +{
> +       const char *type_str = "Data len";
> +       uint16_t len_to_check = tx_pkt->data_len;
> +
> +       if (data_len_chk == 0) {
> +               type_str = "TSO Seg size";
> +               len_to_check = tx_pkt->tso_segsz;
> +       }
> +
> +       if (len_to_check == 0) {
> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> +               rte_dump_stack();
> +               return true;
> +       }
> +       return false;
> +}
> +
>   static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>                                  struct bnxt_tx_queue *txq,
>                                  uint16_t *coal_pkts,
> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>          }
> 
>          /* Check non zero data_len */
> -       RTE_VERIFY(tx_pkt->data_len);
> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> +               return -EIO;
> 

Some PMDs does the similar verification in the 'rte_eth_tx_prepare()' 
API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx 
data path code, do you want to do the same?

Re: [dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only configs in PMD

[Ferruh Yigit]

On 6/15/2022 3:56 PM, Kalesh A P wrote:

> 
> From: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>
> 
> Currently, we fail the init/probe of pmd if eth_dev->data->nb_tx_queues
> or eth_dev->data->nb_rx_queues is 0. We are removing this check.
> 

Is there a valid usecase for Rx only or Tx only config?
I assume testpmd doesn't support it, how are you testing this?

> Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> ---
>   drivers/net/bnxt/bnxt_ethdev.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 34f2149..8181e1f 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -723,7 +723,7 @@ static int bnxt_alloc_prev_ring_stats(struct bnxt *bp)
>                                               sizeof(struct bnxt_ring_stats) *
>                                               bp->tx_cp_nr_rings,
>                                               0);
> -       if (bp->prev_tx_ring_stats == NULL)
> +       if (bp->tx_cp_nr_rings > 0 && bp->prev_tx_ring_stats == NULL)
>                  goto error;
> 
>          return 0;
> @@ -1567,11 +1567,6 @@ int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
>          int vlan_mask = 0;
>          int rc, retry_cnt = BNXT_IF_CHANGE_RETRY_COUNT;
> 
> -       if (!eth_dev->data->nb_tx_queues || !eth_dev->data->nb_rx_queues) {
> -               PMD_DRV_LOG(ERR, "Queues are not configured yet!\n");
> -               return -EINVAL;
> -       }
> -
>          if (bp->rx_cp_nr_rings > RTE_ETHDEV_QUEUE_STAT_CNTRS)
>                  PMD_DRV_LOG(ERR,
>                              "RxQ cnt %d > RTE_ETHDEV_QUEUE_STAT_CNTRS %d\n",
> --
> 2.10.1
> 

Re: [dpdk-dev] [PATCH 8/8] net/bnxt: fix the check for autoneg enablement in the PHY FW

[Ferruh Yigit]

On 6/15/2022 3:57 PM, Kalesh A P wrote:
> 
> From: Somnath Kotur<somnath.kotur@broadcom.com>
> 
> The current combination of checks to determine whether autoneg is
> enabled in the card is a bit convoluted and may become incorrect
> as well in the future as one of the fields being used - auto_link_speed
> might become deprecated.
> Switch to using the 'auto_mode' field obtained from the response of
> HWRM_PHY_QCFG cmd as that is always deterministically set by the PHY
> FW.
> Fixed a bug in the 40G check to only look for the bit setting and
> not the actual value.
> Also, check the forced speeds first before trying to enforce the
> auto speeds
> 
> Allow the user to set autoneg speed in all cases except for PAM4 200G
> as PAM4 200G will come up only in forced mode.
> 
> Fixes: c23f9ded0391 ("net/bnxt: support 200G PAM4 link")

Is stable tag (stable@dpdk.org) omitted explicitly?

Re: [dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Ajit Khaparde]

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

On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 6/15/2022 3:56 PM, Kalesh A P wrote:
> > CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> >
> >
> > From: Somnath Kotur <somnath.kotur@broadcom.com>
> >
> > Currently the PMD tries to detect a potential 0 byte DMA by
> > using RTE_VERIFY.
> > But since RTE_VERIFY internally calls rte_panic() it is fatal to
> > the application and some applications want to avoid that.
> > So return an error from the bnxt xmit handler if such a bad pkt is
> > encountered by logging an error message, dumping the pkt header and
> > dump the current stack as well
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > ---
> >   drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
> >   1 file changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> > index 7a7196a..67e0167 100644
> > --- a/drivers/net/bnxt/bnxt_txr.c
> > +++ b/drivers/net/bnxt/bnxt_txr.c
> > @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
> >          return false;
> >   }
> >
> > +static bool
> > +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> > +{
> > +       const char *type_str = "Data len";
> > +       uint16_t len_to_check = tx_pkt->data_len;
> > +
> > +       if (data_len_chk == 0) {
> > +               type_str = "TSO Seg size";
> > +               len_to_check = tx_pkt->tso_segsz;
> > +       }
> > +
> > +       if (len_to_check == 0) {
> > +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> > +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> > +               rte_dump_stack();
> > +               return true;
> > +       }
> > +       return false;
> > +}
> > +
> >   static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >                                  struct bnxt_tx_queue *txq,
> >                                  uint16_t *coal_pkts,
> > @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >          }
> >
> >          /* Check non zero data_len */
> > -       RTE_VERIFY(tx_pkt->data_len);
> > +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> > +               return -EIO;
> >
>
> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
> data path code, do you want to do the same?


When we originally added these checks, we were not sure how prevalent
is the usage of tx_pkt_prepare() dev_op by various applications.

We will stick with this patch for now  and implement that
rte_eth_tx_prepare() in the next release?

Thanks

>

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

Re: [dpdk-dev] [PATCH 8/8] net/bnxt: fix the check for autoneg enablement in the PHY FW

[Ajit Khaparde]

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

On Thu, Jun 16, 2022 at 10:04 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 6/15/2022 3:57 PM, Kalesh A P wrote:
> >
> > From: Somnath Kotur<somnath.kotur@broadcom.com>
> >
> > The current combination of checks to determine whether autoneg is
> > enabled in the card is a bit convoluted and may become incorrect
> > as well in the future as one of the fields being used - auto_link_speed
> > might become deprecated.
> > Switch to using the 'auto_mode' field obtained from the response of
> > HWRM_PHY_QCFG cmd as that is always deterministically set by the PHY
> > FW.
> > Fixed a bug in the 40G check to only look for the bit setting and
> > not the actual value.
> > Also, check the forced speeds first before trying to enforce the
> > auto speeds
> >
> > Allow the user to set autoneg speed in all cases except for PAM4 200G
> > as PAM4 200G will come up only in forced mode.
> >
> > Fixes: c23f9ded0391 ("net/bnxt: support 200G PAM4 link")
>
> Is stable tag (stable@dpdk.org) omitted explicitly?
I think it was an oversight.

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

Re: [dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Ferruh Yigit]

On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>>
>> On 6/15/2022 3:56 PM, Kalesh A P wrote:
>>>
>>> From: Somnath Kotur <somnath.kotur@broadcom.com>
>>>
>>> Currently the PMD tries to detect a potential 0 byte DMA by
>>> using RTE_VERIFY.
>>> But since RTE_VERIFY internally calls rte_panic() it is fatal to
>>> the application and some applications want to avoid that.
>>> So return an error from the bnxt xmit handler if such a bad pkt is
>>> encountered by logging an error message, dumping the pkt header and
>>> dump the current stack as well
>>>
>>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>> ---
>>>    drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
>>>    1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
>>> index 7a7196a..67e0167 100644
>>> --- a/drivers/net/bnxt/bnxt_txr.c
>>> +++ b/drivers/net/bnxt/bnxt_txr.c
>>> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
>>>           return false;
>>>    }
>>>
>>> +static bool
>>> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
>>> +{
>>> +       const char *type_str = "Data len";
>>> +       uint16_t len_to_check = tx_pkt->data_len;
>>> +
>>> +       if (data_len_chk == 0) {
>>> +               type_str = "TSO Seg size";
>>> +               len_to_check = tx_pkt->tso_segsz;
>>> +       }
>>> +
>>> +       if (len_to_check == 0) {
>>> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
>>> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
>>> +               rte_dump_stack();
>>> +               return true;
>>> +       }
>>> +       return false;
>>> +}
>>> +
>>>    static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>>>                                   struct bnxt_tx_queue *txq,
>>>                                   uint16_t *coal_pkts,
>>> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>>>           }
>>>
>>>           /* Check non zero data_len */
>>> -       RTE_VERIFY(tx_pkt->data_len);
>>> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
>>> +               return -EIO;
>>>
>>
>> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
>> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
>> data path code, do you want to do the same?
> 
> 
> When we originally added these checks, we were not sure how prevalent
> is the usage of tx_pkt_prepare() dev_op by various applications.
> 
> We will stick with this patch for now  and implement that
> rte_eth_tx_prepare() in the next release?
> 

'rte_eth_tx_prepare()' is not mandatory, so yes there may be 
applications that are not calling this API.

If we have a consensus to have checks in the prepare function, I think 
it helps both to PMDs and applications.

Re: [dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Ajit Khaparde]

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

On Mon, Jun 20, 2022 at 3:55 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> > On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
> >>
> >> On 6/15/2022 3:56 PM, Kalesh A P wrote:
> >>>
> >>> From: Somnath Kotur <somnath.kotur@broadcom.com>
> >>>
> >>> Currently the PMD tries to detect a potential 0 byte DMA by
> >>> using RTE_VERIFY.
> >>> But since RTE_VERIFY internally calls rte_panic() it is fatal to
> >>> the application and some applications want to avoid that.
> >>> So return an error from the bnxt xmit handler if such a bad pkt is
> >>> encountered by logging an error message, dumping the pkt header and
> >>> dump the current stack as well
> >>>
> >>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> >>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> >>> ---
> >>>    drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
> >>>    1 file changed, 26 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> >>> index 7a7196a..67e0167 100644
> >>> --- a/drivers/net/bnxt/bnxt_txr.c
> >>> +++ b/drivers/net/bnxt/bnxt_txr.c
> >>> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
> >>>           return false;
> >>>    }
> >>>
> >>> +static bool
> >>> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> >>> +{
> >>> +       const char *type_str = "Data len";
> >>> +       uint16_t len_to_check = tx_pkt->data_len;
> >>> +
> >>> +       if (data_len_chk == 0) {
> >>> +               type_str = "TSO Seg size";
> >>> +               len_to_check = tx_pkt->tso_segsz;
> >>> +       }
> >>> +
> >>> +       if (len_to_check == 0) {
> >>> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> >>> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> >>> +               rte_dump_stack();
> >>> +               return true;
> >>> +       }
> >>> +       return false;
> >>> +}
> >>> +
> >>>    static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >>>                                   struct bnxt_tx_queue *txq,
> >>>                                   uint16_t *coal_pkts,
> >>> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >>>           }
> >>>
> >>>           /* Check non zero data_len */
> >>> -       RTE_VERIFY(tx_pkt->data_len);
> >>> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> >>> +               return -EIO;
> >>>
> >>
> >> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
> >> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
> >> data path code, do you want to do the same?
> >
> >
> > When we originally added these checks, we were not sure how prevalent
> > is the usage of tx_pkt_prepare() dev_op by various applications.
> >
> > We will stick with this patch for now  and implement that
> > rte_eth_tx_prepare() in the next release?
> >
>
> 'rte_eth_tx_prepare()' is not mandatory, so yes there may be
> applications that are not calling this API.
>
> If we have a consensus to have checks in the prepare function, I think
> it helps both to PMDs and applications.
I agree.
Once the changes for tx_pkt_prepare() are ready,
we will remove the checks from the Tx burst handler.
BNXT PMD will carry the current checks till then.

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

Re: [dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only configs in PMD

[Kalesh Anakkur Purayil]

[-- Attachment #1.1: Type: text/plain, Size: 2238 bytes --]

Hi Damo,

Could you please respond to Ferruh's question?

Regards,
Kalesh

On Thu, Jun 16, 2022 at 10:33 PM Ferruh Yigit <ferruh.yigit@xilinx.com>
wrote:

> On 6/15/2022 3:56 PM, Kalesh A P wrote:
>
> >
> > From: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>
> >
> > Currently, we fail the init/probe of pmd if eth_dev->data->nb_tx_queues
> > or eth_dev->data->nb_rx_queues is 0. We are removing this check.
> >
>
> Is there a valid usecase for Rx only or Tx only config?
> I assume testpmd doesn't support it, how are you testing this?
>
> > Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Damodharam Ammepalli <damodharam.ammepall@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > ---
> >   drivers/net/bnxt/bnxt_ethdev.c | 7 +------
> >   1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c
> > index 34f2149..8181e1f 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -723,7 +723,7 @@ static int bnxt_alloc_prev_ring_stats(struct bnxt
> *bp)
> >                                               sizeof(struct
> bnxt_ring_stats) *
> >                                               bp->tx_cp_nr_rings,
> >                                               0);
> > -       if (bp->prev_tx_ring_stats == NULL)
> > +       if (bp->tx_cp_nr_rings > 0 && bp->prev_tx_ring_stats == NULL)
> >                  goto error;
> >
> >          return 0;
> > @@ -1567,11 +1567,6 @@ int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
> >          int vlan_mask = 0;
> >          int rc, retry_cnt = BNXT_IF_CHANGE_RETRY_COUNT;
> >
> > -       if (!eth_dev->data->nb_tx_queues ||
> !eth_dev->data->nb_rx_queues) {
> > -               PMD_DRV_LOG(ERR, "Queues are not configured yet!\n");
> > -               return -EINVAL;
> > -       }
> > -
> >          if (bp->rx_cp_nr_rings > RTE_ETHDEV_QUEUE_STAT_CNTRS)
> >                  PMD_DRV_LOG(ERR,
> >                              "RxQ cnt %d > RTE_ETHDEV_QUEUE_STAT_CNTRS
> %d\n",
> > --
> > 2.10.1
> >
>
>

-- 
Regards,
Kalesh A P

[-- Attachment #1.2: Type: text/html, Size: 3573 bytes --]

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

RE: [dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only configs in PMD

[Damodharam Ammepalli]

[-- Attachment #1.1: Type: text/plain, Size: 4071 bytes --]

Hi Ferruh,

Please see my inline responses [Damo];

 

Thanks

Damo

 

From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com> 
Sent: Monday, June 20, 2022 9:47 PM
To: Ferruh Yigit <ferruh.yigit@xilinx.com>; Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
Cc: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>; dpdk-dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only configs in PMD

 

Hi Damo,

 

Could you please respond to Ferruh's question?

 

Regards,

Kalesh

 

On Thu, Jun 16, 2022 at 10:33 PM Ferruh Yigit <ferruh.yigit@xilinx.com <mailto:ferruh.yigit@xilinx.com> > wrote:

On 6/15/2022 3:56 PM, Kalesh A P wrote:

> 
> From: Damodharam Ammepalli <damodharam.ammepall@broadcom.com <mailto:damodharam.ammepall@broadcom.com> >
> 
> Currently, we fail the init/probe of pmd if eth_dev->data->nb_tx_queues
> or eth_dev->data->nb_rx_queues is 0. We are removing this check.
> 

Is there a valid usecase for Rx only or Tx only config?
I assume testpmd doesn't support it, how are you testing this?
[Damo]:

Yes. There is a valid use case. We are trying to address a

customer request, to deploy an application in Rxonly mode.

This is the sample testpmd command we used in our unit tests.

./build/app/dpdk-testpmd -c 0xff  -n 4 --log-level="pmd.",7

--socket-mem 0,1024  -- --forward-mode=rxonly --txq=0 -i

 

We can update the commit headline to indicate Rx only configuration to

prevent misunderstanding.

Please let me know and we will address accordingly.


> Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
> Cc: stable@dpdk.org <mailto:stable@dpdk.org> 
> 
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepall@broadcom.com <mailto:damodharam.ammepall@broadcom.com> >
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com <mailto:ajit.khaparde@broadcom.com> >
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com <mailto:somnath.kotur@broadcom.com> >
> ---
>   drivers/net/bnxt/bnxt_ethdev.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 34f2149..8181e1f 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -723,7 +723,7 @@ static int bnxt_alloc_prev_ring_stats(struct bnxt *bp)
>                                               sizeof(struct bnxt_ring_stats) *
>                                               bp->tx_cp_nr_rings,
>                                               0);
> -       if (bp->prev_tx_ring_stats == NULL)
> +       if (bp->tx_cp_nr_rings > 0 && bp->prev_tx_ring_stats == NULL)
>                  goto error;
> 
>          return 0;
> @@ -1567,11 +1567,6 @@ int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
>          int vlan_mask = 0;
>          int rc, retry_cnt = BNXT_IF_CHANGE_RETRY_COUNT;
> 
> -       if (!eth_dev->data->nb_tx_queues || !eth_dev->data->nb_rx_queues) {
> -               PMD_DRV_LOG(ERR, "Queues are not configured yet!\n");
> -               return -EINVAL;
> -       }
> -
>          if (bp->rx_cp_nr_rings > RTE_ETHDEV_QUEUE_STAT_CNTRS)
>                  PMD_DRV_LOG(ERR,
>                              "RxQ cnt %d > RTE_ETHDEV_QUEUE_STAT_CNTRS %d\n",
> --
> 2.10.1
> 




 

-- 

Regards,

Kalesh A P


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #1.2: Type: text/html, Size: 10522 bytes --]

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

Re: [dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only configs in PMD

[Ferruh Yigit]

On 6/21/2022 5:54 AM, Damodharam Ammepalli wrote:
> Hi Ferruh,
> 
> Please see my inline responses [Damo];
> 
> Thanks
> 
> Damo
> 
> *From:*Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com>
> *Sent:* Monday, June 20, 2022 9:47 PM
> *To:* Ferruh Yigit <ferruh.yigit@xilinx.com>; Damodharam Ammepalli 
> <damodharam.ammepalli@broadcom.com>
> *Cc:* Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>; dpdk-dev 
> <dev@dpdk.org>
> *Subject:* Re: [dpdk-dev] [PATCH 4/8] net/bnxt: allow Tx only or Rx only 
> configs in PMD
> 
> Hi Damo,
> 
> Could you please respond to Ferruh's question?
> 
> Regards,
> 
> Kalesh
> 
> On Thu, Jun 16, 2022 at 10:33 PM Ferruh Yigit <ferruh.yigit@xilinx.com 
> <mailto:ferruh.yigit@xilinx.com>> wrote:
> 
>     On 6/15/2022 3:56 PM, Kalesh A P wrote:
> 
>      >
>      > From: Damodharam Ammepalli <damodharam.ammepall@broadcom.com
>     <mailto:damodharam.ammepall@broadcom.com>>
>      >
>      > Currently, we fail the init/probe of pmd if
>     eth_dev->data->nb_tx_queues
>      > or eth_dev->data->nb_rx_queues is 0. We are removing this check.
>      >
> 
>     Is there a valid usecase for Rx only or Tx only config?
>     I assume testpmd doesn't support it, how are you testing this?
>     [Damo]:
> 
>     Yes. There is a valid use case. We are trying to address a
> 
>     customer request, to deploy an application in Rxonly mode.
> 

OK, I just want to confirm this is a valid usecase.

>     This is the sample testpmd command we used in our unit tests.
> 
>     ./build/app/dpdk-testpmd -c 0xff  -n 4 --log-level="pmd.",7
> 
>     --socket-mem 0,1024  -- --forward-mode=rxonly --txq=0 -i
> 

Got it, testpmd 'rxonly', and possibly 'txonly', mode is working.

>     We can update the commit headline to indicate Rx only configuration to
> 
>     prevent misunderstanding.
> 
>     Please let me know and we will address accordingly.
> 

I think it is OK as it is, thanks for clarification.

> 
>      > Fixes: daef48efe5e5 ("net/bnxt: support set MTU")
>      > Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>      >
>      > Signed-off-by: Damodharam Ammepalli
>     <damodharam.ammepall@broadcom.com
>     <mailto:damodharam.ammepall@broadcom.com>>
>      > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com
>     <mailto:ajit.khaparde@broadcom.com>>
>      > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com
>     <mailto:somnath.kotur@broadcom.com>>
>      > ---
>      >   drivers/net/bnxt/bnxt_ethdev.c | 7 +------
>      >   1 file changed, 1 insertion(+), 6 deletions(-)
>      >
>      > diff --git a/drivers/net/bnxt/bnxt_ethdev.c
>     b/drivers/net/bnxt/bnxt_ethdev.c
>      > index 34f2149..8181e1f 100644
>      > --- a/drivers/net/bnxt/bnxt_ethdev.c
>      > +++ b/drivers/net/bnxt/bnxt_ethdev.c
>      > @@ -723,7 +723,7 @@ static int bnxt_alloc_prev_ring_stats(struct
>     bnxt *bp)
>      >                                               sizeof(struct
>     bnxt_ring_stats) *
>      >                                               bp->tx_cp_nr_rings,
>      >                                               0);
>      > -       if (bp->prev_tx_ring_stats == NULL)
>      > +       if (bp->tx_cp_nr_rings > 0 && bp->prev_tx_ring_stats == NULL)
>      >                  goto error;
>      >
>      >          return 0;
>      > @@ -1567,11 +1567,6 @@ int bnxt_dev_start_op(struct rte_eth_dev
>     *eth_dev)
>      >          int vlan_mask = 0;
>      >          int rc, retry_cnt = BNXT_IF_CHANGE_RETRY_COUNT;
>      >
>      > -       if (!eth_dev->data->nb_tx_queues ||
>     !eth_dev->data->nb_rx_queues) {
>      > -               PMD_DRV_LOG(ERR, "Queues are not configured yet!\n");
>      > -               return -EINVAL;
>      > -       }
>      > -
>      >          if (bp->rx_cp_nr_rings > RTE_ETHDEV_QUEUE_STAT_CNTRS)
>      >                  PMD_DRV_LOG(ERR,
>      >                              "RxQ cnt %d >
>     RTE_ETHDEV_QUEUE_STAT_CNTRS %d\n",
>      > --
>      > 2.10.1
>      >
> 
> 
> -- 
> 
> Regards,
> 
> Kalesh A P
> 
> 
> This electronic communication and the information and any files 
> transmitted with it, or attached to it, are confidential and are 
> intended solely for the use of the individual or entity to whom it is 
> addressed and may contain information that is confidential, legally 
> privileged, protected by privacy laws, or otherwise restricted from 
> disclosure to anyone else. If you are not the intended recipient or the 
> person responsible for delivering the e-mail to the intended recipient, 
> you are hereby notified that any use, copying, distributing, 
> dissemination, forwarding, printing, or copying of this e-mail is 
> strictly prohibited. If you received this e-mail in error, please return 
> the e-mail to the sender, delete it from your computer, and destroy any 
> printed copy of it.

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

[Ajit Khaparde]

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

On Wed, Jun 15, 2022 at 7:57 AM Kalesh A P
<kalesh-anakkur.purayil@broadcom.com> wrote:
>
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>
> This patchset contains bug fixes in bnxt PMD.

Patchset applied to dpdk-next-net-brcm. Thanks

>
> Ajit Khaparde (1):
>   net/bnxt: fix switch domain allocation
>
> Damodharam Ammepalli (3):
>   net/bnxt: allow Tx only or Rx only configs in PMD
>   net/bnxt: disallow MTU change when device is started
>   net/bnxt: cleanups in MTU set callback
>
> Kalesh AP (2):
>   net/bnxt: reduce the verbosity of a log
>   net/bnxt: fix setting forced speed
>
> Somnath Kotur (2):
>   net/bnxt: remove assert for zero data len in Tx path
>   net/bnxt: fix the check for autoneg enablement in the PHY FW
>
>  drivers/net/bnxt/bnxt_ethdev.c | 95 ++++++++++++++++++------------------------
>  drivers/net/bnxt/bnxt_hwrm.c   | 45 +++++++++++++-------
>  drivers/net/bnxt/bnxt_hwrm.h   |  3 ++
>  drivers/net/bnxt/bnxt_rxq.c    |  9 +---
>  drivers/net/bnxt/bnxt_txr.c    | 29 +++++++++++--
>  5 files changed, 101 insertions(+), 80 deletions(-)
>
> --
> 2.10.1
>

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

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

[Thomas Monjalon]

26/06/2022 22:45, Ajit Khaparde:
> On Wed, Jun 15, 2022 at 7:57 AM Kalesh A P
> <kalesh-anakkur.purayil@broadcom.com> wrote:
> >
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > This patchset contains bug fixes in bnxt PMD.
> 
> Patchset applied to dpdk-next-net-brcm. Thanks

It is very late for -rc2, but it has found its way at last minute.

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

[Ajit Khaparde]

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

On Sun, Jun 26, 2022 at 7:08 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/06/2022 22:45, Ajit Khaparde:
> > On Wed, Jun 15, 2022 at 7:57 AM Kalesh A P
> > <kalesh-anakkur.purayil@broadcom.com> wrote:
> > >
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > This patchset contains bug fixes in bnxt PMD.
> >
> > Patchset applied to dpdk-next-net-brcm. Thanks
>
> It is very late for -rc2, but it has found its way at last minute.
Thanks Thomas.

>
>

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