[PATCH v2 01/12] net/ngbe: fix failed to receive packets

[PATCH v2 01/12] net/ngbe: fix failed to receive packets

[Jiawen Wu]

Initialize Rx packet buffer before starting RxTx, ensure to receive
packets.

Fixes: 62fc35e63d0e ("net/ngbe: support Rx queue start/stop")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ngbe/base/ngbe_dummy.h |  4 ++++
 drivers/net/ngbe/base/ngbe_hw.c    | 27 +++++++++++++++++++++++++++
 drivers/net/ngbe/base/ngbe_hw.h    |  2 ++
 drivers/net/ngbe/base/ngbe_type.h  |  7 +++++++
 drivers/net/ngbe/ngbe_ethdev.c     |  1 +
 5 files changed, 41 insertions(+)

diff --git a/drivers/net/ngbe/base/ngbe_dummy.h b/drivers/net/ngbe/base/ngbe_dummy.h
index 61b0d82bfb..d74c9f7b54 100644
--- a/drivers/net/ngbe/base/ngbe_dummy.h
+++ b/drivers/net/ngbe/base/ngbe_dummy.h
@@ -114,6 +114,9 @@ static inline s32 ngbe_mac_get_link_capabilities_dummy(struct ngbe_hw *TUP0,
 {
 	return NGBE_ERR_OPS_DUMMY;
 }
+static inline void ngbe_setup_pba_dummy(struct ngbe_hw *TUP0)
+{
+}
 static inline s32 ngbe_mac_led_on_dummy(struct ngbe_hw *TUP0, u32 TUP1)
 {
 	return NGBE_ERR_OPS_DUMMY;
@@ -298,6 +301,7 @@ static inline void ngbe_init_ops_dummy(struct ngbe_hw *hw)
 	hw->mac.setup_link = ngbe_mac_setup_link_dummy;
 	hw->mac.check_link = ngbe_mac_check_link_dummy;
 	hw->mac.get_link_capabilities = ngbe_mac_get_link_capabilities_dummy;
+	hw->mac.setup_pba = ngbe_setup_pba_dummy;
 	hw->mac.led_on = ngbe_mac_led_on_dummy;
 	hw->mac.led_off = ngbe_mac_led_off_dummy;
 	hw->mac.set_rar = ngbe_mac_set_rar_dummy;
diff --git a/drivers/net/ngbe/base/ngbe_hw.c b/drivers/net/ngbe/base/ngbe_hw.c
index 0716357725..0b22ea0fb3 100644
--- a/drivers/net/ngbe/base/ngbe_hw.c
+++ b/drivers/net/ngbe/base/ngbe_hw.c
@@ -1609,6 +1609,30 @@ void ngbe_set_mac_anti_spoofing(struct ngbe_hw *hw, bool enable, int vf)
 	wr32(hw, NGBE_POOLTXASMAC, pfvfspoof);
 }
 
+/**
+ * ngbe_set_pba - Initialize Rx packet buffer
+ * @hw: pointer to hardware structure
+ * @headroom: reserve n KB of headroom
+ **/
+void ngbe_set_pba(struct ngbe_hw *hw)
+{
+	u32 rxpktsize = hw->mac.rx_pb_size;
+	u32 txpktsize, txpbthresh;
+
+	/* Reserve 256 KB of headroom */
+	rxpktsize -= 256;
+
+	rxpktsize <<= 10;
+	wr32(hw, NGBE_PBRXSIZE, rxpktsize);
+
+	/* Only support an equally distributed Tx packet buffer strategy. */
+	txpktsize = NGBE_PBTXSIZE_MAX;
+	txpbthresh = (txpktsize / 1024) - NGBE_TXPKT_SIZE_MAX;
+
+	wr32(hw, NGBE_PBTXSIZE, txpktsize);
+	wr32(hw, NGBE_PBTXDMATH, txpbthresh);
+}
+
 /**
  *  ngbe_set_vlan_anti_spoofing - Enable/Disable VLAN anti-spoofing
  *  @hw: pointer to hardware structure
@@ -1907,6 +1931,8 @@ s32 ngbe_init_ops_pf(struct ngbe_hw *hw)
 	mac->check_link = ngbe_check_mac_link_em;
 	mac->setup_link = ngbe_setup_mac_link_em;
 
+	mac->setup_pba = ngbe_set_pba;
+
 	/* Manageability interface */
 	mac->init_thermal_sensor_thresh = ngbe_init_thermal_sensor_thresh;
 	mac->check_overtemp = ngbe_mac_check_overtemp;
@@ -1928,6 +1954,7 @@ s32 ngbe_init_ops_pf(struct ngbe_hw *hw)
 	mac->mcft_size		= NGBE_EM_MC_TBL_SIZE;
 	mac->vft_size		= NGBE_EM_VFT_TBL_SIZE;
 	mac->num_rar_entries	= NGBE_EM_RAR_ENTRIES;
+	mac->rx_pb_size		= NGBE_EM_RX_PB_SIZE;
 	mac->max_rx_queues	= NGBE_EM_MAX_RX_QUEUES;
 	mac->max_tx_queues	= NGBE_EM_MAX_TX_QUEUES;
 
diff --git a/drivers/net/ngbe/base/ngbe_hw.h b/drivers/net/ngbe/base/ngbe_hw.h
index ad7e8fc2d9..b32cf87ff4 100644
--- a/drivers/net/ngbe/base/ngbe_hw.h
+++ b/drivers/net/ngbe/base/ngbe_hw.h
@@ -13,6 +13,7 @@
 #define NGBE_EM_RAR_ENTRIES   32
 #define NGBE_EM_MC_TBL_SIZE   32
 #define NGBE_EM_VFT_TBL_SIZE  128
+#define NGBE_EM_RX_PB_SIZE    42 /*KB*/
 
 s32 ngbe_init_hw(struct ngbe_hw *hw);
 s32 ngbe_start_hw(struct ngbe_hw *hw);
@@ -44,6 +45,7 @@ s32 ngbe_update_mc_addr_list(struct ngbe_hw *hw, u8 *mc_addr_list,
 				      ngbe_mc_addr_itr func, bool clear);
 s32 ngbe_disable_sec_rx_path(struct ngbe_hw *hw);
 s32 ngbe_enable_sec_rx_path(struct ngbe_hw *hw);
+void ngbe_set_pba(struct ngbe_hw *hw);
 
 s32 ngbe_setup_fc_em(struct ngbe_hw *hw);
 s32 ngbe_fc_enable(struct ngbe_hw *hw);
diff --git a/drivers/net/ngbe/base/ngbe_type.h b/drivers/net/ngbe/base/ngbe_type.h
index 12847b7272..269e087d50 100644
--- a/drivers/net/ngbe/base/ngbe_type.h
+++ b/drivers/net/ngbe/base/ngbe_type.h
@@ -11,6 +11,9 @@
 #define NGBE_FRAME_SIZE_MAX       (9728) /* Maximum frame size, +FCS */
 #define NGBE_FRAME_SIZE_DFT       (1522) /* Default frame size, +FCS */
 #define NGBE_NUM_POOL             (32)
+#define NGBE_PBRXSIZE_MAX         0x00080000 /* 512KB Packet Buffer */
+#define NGBE_PBTXSIZE_MAX         0x00005000 /* 20KB Packet Buffer */
+#define NGBE_TXPKT_SIZE_MAX       0xA /* Max Tx Packet size */
 #define NGBE_MAX_QP               (8)
 #define NGBE_MAX_UTA              128
 
@@ -269,6 +272,9 @@ struct ngbe_mac_info {
 	s32 (*get_link_capabilities)(struct ngbe_hw *hw,
 				      u32 *speed, bool *autoneg);
 
+	/* Packet Buffer manipulation */
+	void (*setup_pba)(struct ngbe_hw *hw);
+
 	/* LED */
 	s32 (*led_on)(struct ngbe_hw *hw, u32 index);
 	s32 (*led_off)(struct ngbe_hw *hw, u32 index);
@@ -311,6 +317,7 @@ struct ngbe_mac_info {
 	u32 mcft_size;
 	u32 vft_size;
 	u32 num_rar_entries;
+	u32 rx_pb_size;
 	u32 max_tx_queues;
 	u32 max_rx_queues;
 	bool get_link_status;
diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
index 0d66c32551..180489ce38 100644
--- a/drivers/net/ngbe/ngbe_ethdev.c
+++ b/drivers/net/ngbe/ngbe_ethdev.c
@@ -1004,6 +1004,7 @@ ngbe_dev_start(struct rte_eth_dev *dev)
 		goto error;
 	}
 
+	hw->mac.setup_pba(hw);
 	ngbe_configure_port(dev);
 
 	err = ngbe_dev_rxtx_start(dev);
-- 
2.21.0.windows.1

[PATCH v2 02/12] net/ngbe: fix link interrupt sometimes lost

[Jiawen Wu]

When the port is started and stopped continuously and quickly, one
interrupt cannot be handled in time, which will cause subsequent
interrupts to be lost, so that link status will cannot be updated.

Fixes: b9246b8fa280 ("net/ngbe: support link update")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ngbe/ngbe_ethdev.c | 115 +++++++++++----------------------
 drivers/net/ngbe/ngbe_ethdev.h |   1 +
 2 files changed, 39 insertions(+), 77 deletions(-)

diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
index 180489ce38..8e31234442 100644
--- a/drivers/net/ngbe/ngbe_ethdev.c
+++ b/drivers/net/ngbe/ngbe_ethdev.c
@@ -89,7 +89,6 @@ static int ngbe_dev_macsec_interrupt_setup(struct rte_eth_dev *dev);
 static int ngbe_dev_misc_interrupt_setup(struct rte_eth_dev *dev);
 static int ngbe_dev_rxq_interrupt_setup(struct rte_eth_dev *dev);
 static void ngbe_dev_interrupt_handler(void *param);
-static void ngbe_dev_interrupt_delayed_handler(void *param);
 static void ngbe_configure_msix(struct rte_eth_dev *dev);
 
 #define NGBE_SET_HWSTRIP(h, q) do {\
@@ -943,6 +942,9 @@ ngbe_dev_start(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* Stop the link setup handler before resetting the HW. */
+	rte_eal_alarm_cancel(ngbe_dev_setup_link_alarm_handler, dev);
+
 	/* disable uio/vfio intr/eventfd mapping */
 	rte_intr_disable(intr_handle);
 
@@ -1132,6 +1134,8 @@ ngbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	rte_eal_alarm_cancel(ngbe_dev_setup_link_alarm_handler, dev);
+
 	if ((hw->sub_system_id & NGBE_OEM_MASK) == NGBE_LY_M88E1512_SFP ||
 		(hw->sub_system_id & NGBE_OEM_MASK) == NGBE_LY_YT8521S_SFP) {
 		/* gpio0 is used to power on/off control*/
@@ -1801,6 +1805,24 @@ ngbe_dev_supported_ptypes_get(struct rte_eth_dev *dev)
 	return NULL;
 }
 
+void
+ngbe_dev_setup_link_alarm_handler(void *param)
+{
+	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
+	struct ngbe_hw *hw = ngbe_dev_hw(dev);
+	struct ngbe_interrupt *intr = ngbe_dev_intr(dev);
+	u32 speed;
+	bool autoneg = false;
+
+	speed = hw->phy.autoneg_advertised;
+	if (!speed)
+		hw->mac.get_link_capabilities(hw, &speed, &autoneg);
+
+	hw->mac.setup_link(hw, speed, true);
+
+	intr->flags &= ~NGBE_FLAG_NEED_LINK_CONFIG;
+}
+
 /* return 0 means link status changed, -1 means not changed */
 int
 ngbe_dev_link_update_share(struct rte_eth_dev *dev,
@@ -1838,8 +1860,16 @@ ngbe_dev_link_update_share(struct rte_eth_dev *dev,
 		return rte_eth_linkstatus_set(dev, &link);
 	}
 
-	if (!link_up)
+	if (!link_up) {
+		if (hw->phy.media_type == ngbe_media_type_fiber &&
+			hw->phy.type != ngbe_phy_mvl_sfi) {
+			intr->flags |= NGBE_FLAG_NEED_LINK_CONFIG;
+			rte_eal_alarm_set(10,
+				ngbe_dev_setup_link_alarm_handler, dev);
+		}
+
 		return rte_eth_linkstatus_set(dev, &link);
+	}
 
 	intr->flags &= ~NGBE_FLAG_NEED_LINK_CONFIG;
 	link.link_status = RTE_ETH_LINK_UP;
@@ -2062,9 +2092,6 @@ ngbe_dev_interrupt_get_status(struct rte_eth_dev *dev)
 	struct ngbe_hw *hw = ngbe_dev_hw(dev);
 	struct ngbe_interrupt *intr = ngbe_dev_intr(dev);
 
-	/* clear all cause mask */
-	ngbe_disable_intr(hw);
-
 	/* read-on-clear nic registers here */
 	eicr = ((u32 *)hw->isb_mem)[NGBE_ISB_MISC];
 	PMD_DRV_LOG(DEBUG, "eicr %x", eicr);
@@ -2084,6 +2111,8 @@ ngbe_dev_interrupt_get_status(struct rte_eth_dev *dev)
 	if (eicr & NGBE_ICRMISC_GPIO)
 		intr->flags |= NGBE_FLAG_NEED_LINK_UPDATE;
 
+	((u32 *)hw->isb_mem)[NGBE_ISB_MISC] = 0;
+
 	return 0;
 }
 
@@ -2136,7 +2165,6 @@ static int
 ngbe_dev_interrupt_action(struct rte_eth_dev *dev)
 {
 	struct ngbe_interrupt *intr = ngbe_dev_intr(dev);
-	int64_t timeout;
 
 	PMD_DRV_LOG(DEBUG, "intr action type %d", intr->flags);
 
@@ -2152,31 +2180,11 @@ ngbe_dev_interrupt_action(struct rte_eth_dev *dev)
 		rte_eth_linkstatus_get(dev, &link);
 
 		ngbe_dev_link_update(dev, 0);
-
-		/* likely to up */
-		if (link.link_status != RTE_ETH_LINK_UP)
-			/* handle it 1 sec later, wait it being stable */
-			timeout = NGBE_LINK_UP_CHECK_TIMEOUT;
-		/* likely to down */
-		else
-			/* handle it 4 sec later, wait it being stable */
-			timeout = NGBE_LINK_DOWN_CHECK_TIMEOUT;
-
+		intr->flags &= ~NGBE_FLAG_NEED_LINK_UPDATE;
 		ngbe_dev_link_status_print(dev);
-		if (rte_eal_alarm_set(timeout * 1000,
-				      ngbe_dev_interrupt_delayed_handler,
-				      (void *)dev) < 0) {
-			PMD_DRV_LOG(ERR, "Error setting alarm");
-		} else {
-			/* remember original mask */
-			intr->mask_misc_orig = intr->mask_misc;
-			/* only disable lsc interrupt */
-			intr->mask_misc &= ~NGBE_ICRMISC_PHY;
-
-			intr->mask_orig = intr->mask;
-			/* only disable all misc interrupts */
-			intr->mask &= ~(1ULL << NGBE_MISC_VEC_ID);
-		}
+		if (dev->data->dev_link.link_speed != link.link_speed)
+			rte_eth_dev_callback_process(dev,
+				RTE_ETH_EVENT_INTR_LSC, NULL);
 	}
 
 	PMD_DRV_LOG(DEBUG, "enable intr immediately");
@@ -2185,53 +2193,6 @@ ngbe_dev_interrupt_action(struct rte_eth_dev *dev)
 	return 0;
 }
 
-/**
- * Interrupt handler which shall be registered for alarm callback for delayed
- * handling specific interrupt to wait for the stable nic state. As the
- * NIC interrupt state is not stable for ngbe after link is just down,
- * it needs to wait 4 seconds to get the stable status.
- *
- * @param param
- *  The address of parameter (struct rte_eth_dev *) registered before.
- */
-static void
-ngbe_dev_interrupt_delayed_handler(void *param)
-{
-	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
-	struct ngbe_interrupt *intr = ngbe_dev_intr(dev);
-	struct ngbe_hw *hw = ngbe_dev_hw(dev);
-	uint32_t eicr;
-
-	ngbe_disable_intr(hw);
-
-	eicr = ((u32 *)hw->isb_mem)[NGBE_ISB_MISC];
-	if (eicr & NGBE_ICRMISC_VFMBX)
-		ngbe_pf_mbx_process(dev);
-
-	if (intr->flags & NGBE_FLAG_NEED_LINK_UPDATE) {
-		ngbe_dev_link_update(dev, 0);
-		intr->flags &= ~NGBE_FLAG_NEED_LINK_UPDATE;
-		ngbe_dev_link_status_print(dev);
-		rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC,
-					      NULL);
-	}
-
-	if (intr->flags & NGBE_FLAG_MACSEC) {
-		rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_MACSEC,
-					      NULL);
-		intr->flags &= ~NGBE_FLAG_MACSEC;
-	}
-
-	/* restore original mask */
-	intr->mask_misc = intr->mask_misc_orig;
-	intr->mask_misc_orig = 0;
-	intr->mask = intr->mask_orig;
-	intr->mask_orig = 0;
-
-	PMD_DRV_LOG(DEBUG, "enable intr in delayed handler S[%08x]", eicr);
-	ngbe_enable_intr(dev);
-}
-
 /**
  * Interrupt handler triggered by NIC  for handling
  * specific interrupt.
diff --git a/drivers/net/ngbe/ngbe_ethdev.h b/drivers/net/ngbe/ngbe_ethdev.h
index bb96f6a5e7..8d500fd38c 100644
--- a/drivers/net/ngbe/ngbe_ethdev.h
+++ b/drivers/net/ngbe/ngbe_ethdev.h
@@ -341,6 +341,7 @@ void ngbe_vlan_hw_strip_bitmap_set(struct rte_eth_dev *dev,
 		uint16_t queue, bool on);
 void ngbe_config_vlan_strip_on_all_queues(struct rte_eth_dev *dev,
 						  int mask);
+void ngbe_dev_setup_link_alarm_handler(void *param);
 void ngbe_read_stats_registers(struct ngbe_hw *hw,
 			   struct ngbe_hw_stats *hw_stats);
 
-- 
2.21.0.windows.1

[PATCH v2 03/12] net/ngbe: fix Tx pending

[Jiawen Wu]

Add commands requesting firmware to enable or disable PCIe bus master.
Disable PCIe master access to clear BME when stop hardware, and verify
there are no pending requests.

Move disabling Tx queue after disabling PCIe bus master, to ensure that
there are no packets left to cause Tx hang.

Fixes: 78710873c2f3 ("net/ngbe: add HW initialization")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ngbe/base/ngbe_hw.c   | 76 +++++++++++++++++++++++++++----
 drivers/net/ngbe/base/ngbe_hw.h   |  1 +
 drivers/net/ngbe/base/ngbe_mng.c  | 57 +++++++++++++++++++++++
 drivers/net/ngbe/base/ngbe_mng.h  | 21 +++++++++
 drivers/net/ngbe/base/ngbe_regs.h |  3 ++
 drivers/net/ngbe/base/ngbe_type.h |  3 ++
 drivers/net/ngbe/ngbe_ethdev.c    |  7 ++-
 7 files changed, 158 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ngbe/base/ngbe_hw.c b/drivers/net/ngbe/base/ngbe_hw.c
index 0b22ea0fb3..782fd71d29 100644
--- a/drivers/net/ngbe/base/ngbe_hw.c
+++ b/drivers/net/ngbe/base/ngbe_hw.c
@@ -350,8 +350,8 @@ void ngbe_set_lan_id_multi_port(struct ngbe_hw *hw)
  **/
 s32 ngbe_stop_hw(struct ngbe_hw *hw)
 {
-	u32 reg_val;
 	u16 i;
+	s32 status = 0;
 
 	DEBUGFUNC("ngbe_stop_hw");
 
@@ -372,16 +372,27 @@ s32 ngbe_stop_hw(struct ngbe_hw *hw)
 	wr32(hw, NGBE_ICRMISC, NGBE_ICRMISC_MASK);
 	wr32(hw, NGBE_ICR(0), NGBE_ICR_MASK);
 
-	/* Disable the transmit unit.  Each queue must be disabled. */
-	for (i = 0; i < hw->mac.max_tx_queues; i++)
-		wr32(hw, NGBE_TXCFG(i), NGBE_TXCFG_FLUSH);
+	wr32(hw, NGBE_BMECTL, 0x3);
 
 	/* Disable the receive unit by stopping each queue */
-	for (i = 0; i < hw->mac.max_rx_queues; i++) {
-		reg_val = rd32(hw, NGBE_RXCFG(i));
-		reg_val &= ~NGBE_RXCFG_ENA;
-		wr32(hw, NGBE_RXCFG(i), reg_val);
-	}
+	for (i = 0; i < hw->mac.max_rx_queues; i++)
+		wr32(hw, NGBE_RXCFG(i), 0);
+
+	/* flush all queues disables */
+	ngbe_flush(hw);
+	msec_delay(2);
+
+	/*
+	 * Prevent the PCI-E bus from hanging by disabling PCI-E master
+	 * access and verify no pending requests
+	 */
+	status = ngbe_set_pcie_master(hw, false);
+	if (status)
+		return status;
+
+	/* Disable the transmit unit.  Each queue must be disabled. */
+	for (i = 0; i < hw->mac.max_tx_queues; i++)
+		wr32(hw, NGBE_TXCFG(i), 0);
 
 	/* flush all queues disables */
 	ngbe_flush(hw);
@@ -1076,6 +1087,53 @@ void ngbe_fc_autoneg(struct ngbe_hw *hw)
 	}
 }
 
+/**
+ *  ngbe_set_pcie_master - Disable or Enable PCI-express master access
+ *  @hw: pointer to hardware structure
+ *
+ *  Disables PCI-Express master access and verifies there are no pending
+ *  requests. NGBE_ERR_MASTER_REQUESTS_PENDING is returned if master disable
+ *  bit hasn't caused the master requests to be disabled, else 0
+ *  is returned signifying master requests disabled.
+ **/
+s32 ngbe_set_pcie_master(struct ngbe_hw *hw, bool enable)
+{
+	s32 status = 0;
+	u16 addr = 0x04;
+	u32 data, i;
+
+	DEBUGFUNC("ngbe_set_pcie_master");
+
+	ngbe_hic_pcie_read(hw, addr, &data, 4);
+	if (enable)
+		data |= 0x04;
+	else
+		data &= ~0x04;
+
+	ngbe_hic_pcie_write(hw, addr, &data, 4);
+
+	if (enable)
+		goto out;
+
+	/* Exit if master requests are blocked */
+	if (!(rd32(hw, NGBE_BMEPEND)) ||
+	    NGBE_REMOVED(hw->hw_addr))
+		goto out;
+
+	/* Poll for master request bit to clear */
+	for (i = 0; i < NGBE_PCI_MASTER_DISABLE_TIMEOUT; i++) {
+		usec_delay(100);
+		if (!(rd32(hw, NGBE_BMEPEND)))
+			goto out;
+	}
+
+	DEBUGOUT("PCIe transaction pending bit also did not clear.\n");
+	status = NGBE_ERR_MASTER_REQUESTS_PENDING;
+
+out:
+	return status;
+}
+
 /**
  *  ngbe_acquire_swfw_sync - Acquire SWFW semaphore
  *  @hw: pointer to hardware structure
diff --git a/drivers/net/ngbe/base/ngbe_hw.h b/drivers/net/ngbe/base/ngbe_hw.h
index b32cf87ff4..7e0e23b195 100644
--- a/drivers/net/ngbe/base/ngbe_hw.h
+++ b/drivers/net/ngbe/base/ngbe_hw.h
@@ -54,6 +54,7 @@ void ngbe_fc_autoneg(struct ngbe_hw *hw);
 s32 ngbe_validate_mac_addr(u8 *mac_addr);
 s32 ngbe_acquire_swfw_sync(struct ngbe_hw *hw, u32 mask);
 void ngbe_release_swfw_sync(struct ngbe_hw *hw, u32 mask);
+s32 ngbe_set_pcie_master(struct ngbe_hw *hw, bool enable);
 
 s32 ngbe_set_vmdq(struct ngbe_hw *hw, u32 rar, u32 vmdq);
 s32 ngbe_clear_vmdq(struct ngbe_hw *hw, u32 rar, u32 vmdq);
diff --git a/drivers/net/ngbe/base/ngbe_mng.c b/drivers/net/ngbe/base/ngbe_mng.c
index a3dd8093ce..68e06e2c24 100644
--- a/drivers/net/ngbe/base/ngbe_mng.c
+++ b/drivers/net/ngbe/base/ngbe_mng.c
@@ -243,6 +243,63 @@ s32 ngbe_hic_sr_write(struct ngbe_hw *hw, u32 addr, u8 *buf, int len)
 	return err;
 }
 
+s32 ngbe_hic_pcie_read(struct ngbe_hw *hw, u16 addr, u32 *buf, int len)
+{
+	struct ngbe_hic_read_pcie command;
+	u32 value = 0;
+	int err, i = 0;
+
+	if (len > NGBE_PMMBX_DATA_SIZE)
+		return NGBE_ERR_HOST_INTERFACE_COMMAND;
+
+	memset(&command, 0, sizeof(command));
+	command.hdr.cmd = FW_PCIE_READ_CMD;
+	command.hdr.buf_len = sizeof(command) - sizeof(command.hdr);
+	command.hdr.checksum = FW_DEFAULT_CHECKSUM;
+	command.lan_id = hw->bus.lan_id;
+	command.addr = addr;
+
+	err = ngbe_host_interface_command(hw, (u32 *)&command,
+			sizeof(command), NGBE_HI_COMMAND_TIMEOUT, false);
+	if (err)
+		return err;
+
+	while (i < (len >> 2)) {
+		value = rd32a(hw, NGBE_MNGMBX, FW_PCIE_BUSMASTER_OFFSET + i);
+		((u32 *)buf)[i] = value;
+		i++;
+	}
+
+	return 0;
+}
+
+s32 ngbe_hic_pcie_write(struct ngbe_hw *hw, u16 addr, u32 *buf, int len)
+{
+	struct ngbe_hic_write_pcie command;
+	u32 value = 0;
+	int err, i = 0;
+
+	while (i < (len >> 2)) {
+		value = ((u32 *)buf)[i];
+		i++;
+	}
+
+	memset(&command, 0, sizeof(command));
+	command.hdr.cmd = FW_PCIE_WRITE_CMD;
+	command.hdr.buf_len = sizeof(command) - sizeof(command.hdr);
+	command.hdr.checksum = FW_DEFAULT_CHECKSUM;
+	command.lan_id = hw->bus.lan_id;
+	command.addr = addr;
+	command.data = value;
+
+	err = ngbe_host_interface_command(hw, (u32 *)&command,
+			sizeof(command), NGBE_HI_COMMAND_TIMEOUT, false);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 s32 ngbe_hic_check_cap(struct ngbe_hw *hw)
 {
 	struct ngbe_hic_read_shadow_ram command;
diff --git a/drivers/net/ngbe/base/ngbe_mng.h b/drivers/net/ngbe/base/ngbe_mng.h
index e3d0309cbc..321338a051 100644
--- a/drivers/net/ngbe/base/ngbe_mng.h
+++ b/drivers/net/ngbe/base/ngbe_mng.h
@@ -20,6 +20,9 @@
 #define FW_READ_SHADOW_RAM_LEN          0x6
 #define FW_WRITE_SHADOW_RAM_CMD         0x33
 #define FW_WRITE_SHADOW_RAM_LEN         0xA /* 8 plus 1 WORD to write */
+#define FW_PCIE_READ_CMD		0xEC
+#define FW_PCIE_WRITE_CMD		0xED
+#define FW_PCIE_BUSMASTER_OFFSET        2
 #define FW_DEFAULT_CHECKSUM             0xFF /* checksum always 0xFF */
 #define FW_NVM_DATA_OFFSET              3
 #define FW_EEPROM_CHECK_STATUS		0xE9
@@ -76,8 +79,26 @@ struct ngbe_hic_write_shadow_ram {
 	u16 pad3;
 };
 
+struct ngbe_hic_read_pcie {
+	struct ngbe_hic_hdr hdr;
+	u8 lan_id;
+	u8 rsvd;
+	u16 addr;
+	u32 data;
+};
+
+struct ngbe_hic_write_pcie {
+	struct ngbe_hic_hdr hdr;
+	u8 lan_id;
+	u8 rsvd;
+	u16 addr;
+	u32 data;
+};
+
 s32 ngbe_hic_sr_read(struct ngbe_hw *hw, u32 addr, u8 *buf, int len);
 s32 ngbe_hic_sr_write(struct ngbe_hw *hw, u32 addr, u8 *buf, int len);
+s32 ngbe_hic_pcie_read(struct ngbe_hw *hw, u16 addr, u32 *buf, int len);
+s32 ngbe_hic_pcie_write(struct ngbe_hw *hw, u16 addr, u32 *buf, int len);
 
 s32 ngbe_hic_check_cap(struct ngbe_hw *hw);
 #endif /* _NGBE_MNG_H_ */
diff --git a/drivers/net/ngbe/base/ngbe_regs.h b/drivers/net/ngbe/base/ngbe_regs.h
index 872b008c46..e84bfdf88a 100644
--- a/drivers/net/ngbe/base/ngbe_regs.h
+++ b/drivers/net/ngbe/base/ngbe_regs.h
@@ -866,6 +866,9 @@ enum ngbe_5tuple_protocol {
  * PF(Physical Function) Registers
  ******************************************************************************/
 /* Interrupt */
+#define NGBE_BMECTL		0x012020
+#define   NGBE_BMECTL_VFDRP	MS(1, 0x1)
+#define   NGBE_BMECTL_PFDRP	MS(0, 0x1)
 #define NGBE_ICRMISC		0x000100
 #define   NGBE_ICRMISC_MASK	MS(8, 0xFFFFFF)
 #define   NGBE_ICRMISC_RST	MS(10, 0x1) /* device reset event */
diff --git a/drivers/net/ngbe/base/ngbe_type.h b/drivers/net/ngbe/base/ngbe_type.h
index 269e087d50..4c995e7397 100644
--- a/drivers/net/ngbe/base/ngbe_type.h
+++ b/drivers/net/ngbe/base/ngbe_type.h
@@ -17,6 +17,9 @@
 #define NGBE_MAX_QP               (8)
 #define NGBE_MAX_UTA              128
 
+#define NGBE_PCI_MASTER_DISABLE_TIMEOUT	800
+
+
 #define NGBE_ALIGN		128 /* as intel did */
 #define NGBE_ISB_SIZE		16
 
diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
index 8e31234442..30c9e68579 100644
--- a/drivers/net/ngbe/ngbe_ethdev.c
+++ b/drivers/net/ngbe/ngbe_ethdev.c
@@ -950,7 +950,6 @@ ngbe_dev_start(struct rte_eth_dev *dev)
 
 	/* stop adapter */
 	hw->adapter_stopped = 0;
-	ngbe_stop_hw(hw);
 
 	/* reinitialize adapter, this calls reset and start */
 	hw->nb_rx_queues = dev->data->nb_rx_queues;
@@ -961,6 +960,8 @@ ngbe_dev_start(struct rte_eth_dev *dev)
 	hw->mac.start_hw(hw);
 	hw->mac.get_link_status = true;
 
+	ngbe_set_pcie_master(hw, true);
+
 	/* configure PF module if SRIOV enabled */
 	ngbe_pf_host_configure(dev);
 
@@ -1174,6 +1175,8 @@ ngbe_dev_stop(struct rte_eth_dev *dev)
 	rte_intr_efd_disable(intr_handle);
 	rte_intr_vec_list_free(intr_handle);
 
+	ngbe_set_pcie_master(hw, true);
+
 	adapter->rss_reta_updated = 0;
 
 	hw->adapter_stopped = true;
@@ -1202,6 +1205,8 @@ ngbe_dev_close(struct rte_eth_dev *dev)
 
 	ngbe_dev_free_queues(dev);
 
+	ngbe_set_pcie_master(hw, false);
+
 	/* reprogram the RAR[0] in case user changed it. */
 	ngbe_set_rar(hw, 0, hw->mac.addr, 0, true);
 
-- 
2.21.0.windows.1

Re: [PATCH v2 03/12] net/ngbe: fix Tx pending

[Ferruh Yigit]

On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> Add commands requesting firmware to enable or disable PCIe bus master.
> Disable PCIe master access to clear BME when stop hardware, and verify
> there are no pending requests.
> 
> Move disabling Tx queue after disabling PCIe bus master, to ensure that
> there are no packets left to cause Tx hang.
> 

Thanks for update, what do you think about following title:
net/ngbe: fix Tx hang on queue disable

> Fixes: 78710873c2f3 ("net/ngbe: add HW initialization")
> Cc:stable@dpdk.org
> 
> Signed-off-by: Jiawen Wu<jiawenwu@trustnetic.com>

[PATCH v2 04/12] net/ngbe: fix RxTx packet statistics

[Jiawen Wu]

Fix specific length packet statistics caused by wrong register addresses.

Fixes: ed5f3bd3373e ("net/ngbe: define registers")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ngbe/base/ngbe_regs.h | 48 +++++++++++++++----------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ngbe/base/ngbe_regs.h b/drivers/net/ngbe/base/ngbe_regs.h
index e84bfdf88a..6432ad8736 100644
--- a/drivers/net/ngbe/base/ngbe_regs.h
+++ b/drivers/net/ngbe/base/ngbe_regs.h
@@ -785,30 +785,30 @@ enum ngbe_5tuple_protocol {
 #define NGBE_MACRXERRCRCH           0x01192C
 #define NGBE_MACRXERRLENL           0x011978
 #define NGBE_MACRXERRLENH           0x01197C
-#define NGBE_MACRX1TO64L            0x001940
-#define NGBE_MACRX1TO64H            0x001944
-#define NGBE_MACRX65TO127L          0x001948
-#define NGBE_MACRX65TO127H          0x00194C
-#define NGBE_MACRX128TO255L         0x001950
-#define NGBE_MACRX128TO255H         0x001954
-#define NGBE_MACRX256TO511L         0x001958
-#define NGBE_MACRX256TO511H         0x00195C
-#define NGBE_MACRX512TO1023L        0x001960
-#define NGBE_MACRX512TO1023H        0x001964
-#define NGBE_MACRX1024TOMAXL        0x001968
-#define NGBE_MACRX1024TOMAXH        0x00196C
-#define NGBE_MACTX1TO64L            0x001834
-#define NGBE_MACTX1TO64H            0x001838
-#define NGBE_MACTX65TO127L          0x00183C
-#define NGBE_MACTX65TO127H          0x001840
-#define NGBE_MACTX128TO255L         0x001844
-#define NGBE_MACTX128TO255H         0x001848
-#define NGBE_MACTX256TO511L         0x00184C
-#define NGBE_MACTX256TO511H         0x001850
-#define NGBE_MACTX512TO1023L        0x001854
-#define NGBE_MACTX512TO1023H        0x001858
-#define NGBE_MACTX1024TOMAXL        0x00185C
-#define NGBE_MACTX1024TOMAXH        0x001860
+#define NGBE_MACRX1TO64L            0x011940
+#define NGBE_MACRX1TO64H            0x011944
+#define NGBE_MACRX65TO127L          0x011948
+#define NGBE_MACRX65TO127H          0x01194C
+#define NGBE_MACRX128TO255L         0x011950
+#define NGBE_MACRX128TO255H         0x011954
+#define NGBE_MACRX256TO511L         0x011958
+#define NGBE_MACRX256TO511H         0x01195C
+#define NGBE_MACRX512TO1023L        0x011960
+#define NGBE_MACRX512TO1023H        0x011964
+#define NGBE_MACRX1024TOMAXL        0x011968
+#define NGBE_MACRX1024TOMAXH        0x01196C
+#define NGBE_MACTX1TO64L            0x011834
+#define NGBE_MACTX1TO64H            0x011838
+#define NGBE_MACTX65TO127L          0x01183C
+#define NGBE_MACTX65TO127H          0x011840
+#define NGBE_MACTX128TO255L         0x011844
+#define NGBE_MACTX128TO255H         0x011848
+#define NGBE_MACTX256TO511L         0x01184C
+#define NGBE_MACTX256TO511H         0x011850
+#define NGBE_MACTX512TO1023L        0x011854
+#define NGBE_MACTX512TO1023H        0x011858
+#define NGBE_MACTX1024TOMAXL        0x01185C
+#define NGBE_MACTX1024TOMAXH        0x011860
 
 #define NGBE_MACRXUNDERSIZE         0x011938
 #define NGBE_MACRXOVERSIZE          0x01193C
-- 
2.21.0.windows.1

[PATCH v2 08/12] net/ngbe: fix debug log

[Jiawen Wu]

Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
that double line was added by using 'DEBUGOUT'.

Fixes: cc934df178ab ("net/ngbe: add log and error types")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ngbe/ngbe_logs.h b/drivers/net/ngbe/ngbe_logs.h
index fd306419e6..b7d78fb400 100644
--- a/drivers/net/ngbe/ngbe_logs.h
+++ b/drivers/net/ngbe/ngbe_logs.h
@@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
 		"%s(): " fmt "\n", __func__, ##args)
 
 extern int ngbe_logtype_driver;
-#define PMD_DRV_LOG(level, fmt, args...) \
+#define PMD_TLOG_DRIVER(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
-		"%s(): " fmt "\n", __func__, ##args)
+		"%s(): " fmt, __func__, ##args)
+
+#define PMD_DRV_LOG(level, fmt, args...) \
+	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
 
 #ifdef RTE_ETHDEV_DEBUG_RX
 extern int ngbe_logtype_rx;
@@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
 #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
 #endif
 
-#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
+#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt, ##args)
 
 #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
 #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
-#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
+#define DEBUGFUNC(fmt)            do { } while (0)
 
 #endif /* _NGBE_LOGS_H_ */
-- 
2.21.0.windows.1

Re: [PATCH v2 08/12] net/ngbe: fix debug log

[Ferruh Yigit]

On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> that double line was added by using 'DEBUGOUT'.
> 
> Fixes: cc934df178ab ("net/ngbe: add log and error types")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ngbe/ngbe_logs.h b/drivers/net/ngbe/ngbe_logs.h
> index fd306419e6..b7d78fb400 100644
> --- a/drivers/net/ngbe/ngbe_logs.h
> +++ b/drivers/net/ngbe/ngbe_logs.h
> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
>   		"%s(): " fmt "\n", __func__, ##args)
>   
>   extern int ngbe_logtype_driver;
> -#define PMD_DRV_LOG(level, fmt, args...) \
> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>   	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> -		"%s(): " fmt "\n", __func__, ##args)
> +		"%s(): " fmt, __func__, ##args)
> +
> +#define PMD_DRV_LOG(level, fmt, args...) \
> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
>   

Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same thing,
but one appends '\n' other doesn't, this is very error prune and there
are already wrong usage in the driver.
I think no need to add complexity for something as simple as this, what do
you think to unify the DEBUG level macros, at least unify the line ending
behavior?


>   #ifdef RTE_ETHDEV_DEBUG_RX
>   extern int ngbe_logtype_rx;
> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
>   #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>   #endif
>   
> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt, ##args)
>   
>   #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>   #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")

What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
As far as I can see they are for same reason, and both macros are
used. If they are for same reason can you please unify the usage?

> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> +#define DEBUGFUNC(fmt)            do { } while (0)

If 'DEBUGFUNC' can be removed, why not removing from the code, instead
of above define? This is your driver, you have full control on it.

RE: [PATCH v2 08/12] net/ngbe: fix debug log

[Jiawen Wu]

On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> > Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> > that double line was added by using 'DEBUGOUT'.
> >
> > Fixes: cc934df178ab ("net/ngbe: add log and error types")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >   drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ngbe/ngbe_logs.h
> > b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
> > --- a/drivers/net/ngbe/ngbe_logs.h
> > +++ b/drivers/net/ngbe/ngbe_logs.h
> > @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
> >   		"%s(): " fmt "\n", __func__, ##args)
> >
> >   extern int ngbe_logtype_driver;
> > -#define PMD_DRV_LOG(level, fmt, args...) \
> > +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> >   	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> > -		"%s(): " fmt "\n", __func__, ##args)
> > +		"%s(): " fmt, __func__, ##args)
> > +
> > +#define PMD_DRV_LOG(level, fmt, args...) \
> > +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> >
> 
> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same thing,
> but one appends '\n' other doesn't, this is very error prune and there are
> already wrong usage in the driver.
> I think no need to add complexity for something as simple as this, what do you
> think to unify the DEBUG level macros, at least unify the line ending behavior?
> 
> 
> >   #ifdef RTE_ETHDEV_DEBUG_RX
> >   extern int ngbe_logtype_rx;
> > @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
> >   #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> >   #endif
> >
> > -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
> > +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
> ##args)
> >
> >   #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> >   #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> 
> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
> As far as I can see they are for same reason, and both macros are used. If they
> are for same reason can you please unify the usage?
> 
> > -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> > +#define DEBUGFUNC(fmt)            do { } while (0)
> 
> If 'DEBUGFUNC' can be removed, why not removing from the code, instead of
> above define? This is your driver, you have full control on it.

Okay. I just realize that this is going to be a big change, so I want to keep it to a minimum.
Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already contains the function name.

Re: [PATCH v2 08/12] net/ngbe: fix debug log

[Ferruh Yigit]

On 2/10/2022 8:03 AM, Jiawen Wu wrote:
> On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
>> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
>>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
>>> that double line was added by using 'DEBUGOUT'.
>>>
>>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>> ---
>>>    drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ngbe/ngbe_logs.h
>>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
>>> --- a/drivers/net/ngbe/ngbe_logs.h
>>> +++ b/drivers/net/ngbe/ngbe_logs.h
>>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
>>>    		"%s(): " fmt "\n", __func__, ##args)
>>>
>>>    extern int ngbe_logtype_driver;
>>> -#define PMD_DRV_LOG(level, fmt, args...) \
>>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>>>    	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
>>> -		"%s(): " fmt "\n", __func__, ##args)
>>> +		"%s(): " fmt, __func__, ##args)
>>> +
>>> +#define PMD_DRV_LOG(level, fmt, args...) \
>>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
>>>
>>
>> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same thing,
>> but one appends '\n' other doesn't, this is very error prune and there are
>> already wrong usage in the driver.
>> I think no need to add complexity for something as simple as this, what do you
>> think to unify the DEBUG level macros, at least unify the line ending behavior?
>>
>>
>>>    #ifdef RTE_ETHDEV_DEBUG_RX
>>>    extern int ngbe_logtype_rx;
>>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
>>>    #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>>>    #endif
>>>
>>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
>>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
>> ##args)
>>>
>>>    #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>>>    #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
>>
>> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
>> As far as I can see they are for same reason, and both macros are used. If they
>> are for same reason can you please unify the usage?
>>
>>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
>>> +#define DEBUGFUNC(fmt)            do { } while (0)
>>
>> If 'DEBUGFUNC' can be removed, why not removing from the code, instead of
>> above define? This is your driver, you have full control on it.
> 
> Okay. I just realize that this is going to be a big change, so I want to keep it to a minimum.
> Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already contains the function name.
> 

If it will be big, we can move the fix out of this set, to not block set,
and send a log fix later as a separate patch, what do you think?

RE: [PATCH v2 08/12] net/ngbe: fix debug log

[Jiawen Wu]

On February 10, 2022 5:03 PM, Ferruh Yigit wrote:
> On 2/10/2022 8:03 AM, Jiawen Wu wrote:
> > On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
> >> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> >>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> >>> that double line was added by using 'DEBUGOUT'.
> >>>
> >>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> >>> ---
> >>>    drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
> >>>    1 file changed, 7 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ngbe/ngbe_logs.h
> >>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
> >>> --- a/drivers/net/ngbe/ngbe_logs.h
> >>> +++ b/drivers/net/ngbe/ngbe_logs.h
> >>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
> >>>    		"%s(): " fmt "\n", __func__, ##args)
> >>>
> >>>    extern int ngbe_logtype_driver;
> >>> -#define PMD_DRV_LOG(level, fmt, args...) \
> >>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> >>>    	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> >>> -		"%s(): " fmt "\n", __func__, ##args)
> >>> +		"%s(): " fmt, __func__, ##args)
> >>> +
> >>> +#define PMD_DRV_LOG(level, fmt, args...) \
> >>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> >>>
> >>
> >> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same
> >> thing, but one appends '\n' other doesn't, this is very error prune
> >> and there are already wrong usage in the driver.
> >> I think no need to add complexity for something as simple as this,
> >> what do you think to unify the DEBUG level macros, at least unify the line
> ending behavior?
> >>
> >>
> >>>    #ifdef RTE_ETHDEV_DEBUG_RX
> >>>    extern int ngbe_logtype_rx;
> >>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
> >>>    #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> >>>    #endif
> >>>
> >>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt,
> ##args)
> >>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
> >> ##args)
> >>>
> >>>    #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> >>>    #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> >>
> >> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
> >> As far as I can see they are for same reason, and both macros are
> >> used. If they are for same reason can you please unify the usage?
> >>
> >>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> >>> +#define DEBUGFUNC(fmt)            do { } while (0)
> >>
> >> If 'DEBUGFUNC' can be removed, why not removing from the code,
> >> instead of above define? This is your driver, you have full control on it.
> >
> > Okay. I just realize that this is going to be a big change, so I want to keep it to
> a minimum.
> > Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already
> contains the function name.
> >
> 
> If it will be big, we can move the fix out of this set, to not block set, and send a
> log fix later as a separate patch, what do you think?

I think it's actually better.

Re: [PATCH v2 08/12] net/ngbe: fix debug log

[Ferruh Yigit]

On 2/10/2022 9:49 AM, Jiawen Wu wrote:
> On February 10, 2022 5:03 PM, Ferruh Yigit wrote:
>> On 2/10/2022 8:03 AM, Jiawen Wu wrote:
>>> On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
>>>> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
>>>>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
>>>>> that double line was added by using 'DEBUGOUT'.
>>>>>
>>>>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>>>> ---
>>>>>     drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
>>>>>     1 file changed, 7 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ngbe/ngbe_logs.h
>>>>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
>>>>> --- a/drivers/net/ngbe/ngbe_logs.h
>>>>> +++ b/drivers/net/ngbe/ngbe_logs.h
>>>>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
>>>>>     		"%s(): " fmt "\n", __func__, ##args)
>>>>>
>>>>>     extern int ngbe_logtype_driver;
>>>>> -#define PMD_DRV_LOG(level, fmt, args...) \
>>>>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>>>>>     	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
>>>>> -		"%s(): " fmt "\n", __func__, ##args)
>>>>> +		"%s(): " fmt, __func__, ##args)
>>>>> +
>>>>> +#define PMD_DRV_LOG(level, fmt, args...) \
>>>>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
>>>>>
>>>>
>>>> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same
>>>> thing, but one appends '\n' other doesn't, this is very error prune
>>>> and there are already wrong usage in the driver.
>>>> I think no need to add complexity for something as simple as this,
>>>> what do you think to unify the DEBUG level macros, at least unify the line
>> ending behavior?
>>>>
>>>>
>>>>>     #ifdef RTE_ETHDEV_DEBUG_RX
>>>>>     extern int ngbe_logtype_rx;
>>>>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
>>>>>     #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>>>>>     #endif
>>>>>
>>>>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt,
>> ##args)
>>>>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
>>>> ##args)
>>>>>
>>>>>     #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>>>>>     #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
>>>>
>>>> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
>>>> As far as I can see they are for same reason, and both macros are
>>>> used. If they are for same reason can you please unify the usage?
>>>>
>>>>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
>>>>> +#define DEBUGFUNC(fmt)            do { } while (0)
>>>>
>>>> If 'DEBUGFUNC' can be removed, why not removing from the code,
>>>> instead of above define? This is your driver, you have full control on it.
>>>
>>> Okay. I just realize that this is going to be a big change, so I want to keep it to
>> a minimum.
>>> Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already
>> contains the function name.
>>>
>>
>> If it will be big, we can move the fix out of this set, to not block set, and send a
>> log fix later as a separate patch, what do you think?
> 
> I think it's actually better.
> 

As long as you commit to send log fix after -rc1, I am OK.

Are you planning to send a new version of this set, or will it work
if I just drop the patch 8/12 & 10/12 of this set and continue with this version?

RE: [PATCH v2 08/12] net/ngbe: fix debug log

[Jiawen Wu]

On February 10, 2022 6:16 PM, Ferruh Yigit wrote:
> On 2/10/2022 9:49 AM, Jiawen Wu wrote:
> > On February 10, 2022 5:03 PM, Ferruh Yigit wrote:
> >> On 2/10/2022 8:03 AM, Jiawen Wu wrote:
> >>> On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
> >>>> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> >>>>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And
> >>>>> fix that double line was added by using 'DEBUGOUT'.
> >>>>>
> >>>>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> >>>>> ---
> >>>>>     drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
> >>>>>     1 file changed, 7 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/net/ngbe/ngbe_logs.h
> >>>>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
> >>>>> --- a/drivers/net/ngbe/ngbe_logs.h
> >>>>> +++ b/drivers/net/ngbe/ngbe_logs.h
> >>>>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
> >>>>>     		"%s(): " fmt "\n", __func__, ##args)
> >>>>>
> >>>>>     extern int ngbe_logtype_driver; -#define PMD_DRV_LOG(level,
> >>>>> fmt, args...) \
> >>>>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> >>>>>     	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> >>>>> -		"%s(): " fmt "\n", __func__, ##args)
> >>>>> +		"%s(): " fmt, __func__, ##args)
> >>>>> +
> >>>>> +#define PMD_DRV_LOG(level, fmt, args...) \
> >>>>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> >>>>>
> >>>>
> >>>> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same
> >>>> thing, but one appends '\n' other doesn't, this is very error prune
> >>>> and there are already wrong usage in the driver.
> >>>> I think no need to add complexity for something as simple as this,
> >>>> what do you think to unify the DEBUG level macros, at least unify
> >>>> the line
> >> ending behavior?
> >>>>
> >>>>
> >>>>>     #ifdef RTE_ETHDEV_DEBUG_RX
> >>>>>     extern int ngbe_logtype_rx;
> >>>>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
> >>>>>     #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> >>>>>     #endif
> >>>>>
> >>>>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt,
> >> ##args)
> >>>>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG,
> fmt,
> >>>> ##args)
> >>>>>
> >>>>>     #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> >>>>>     #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> >>>>
> >>>> What is difference between 'PMD_INIT_FUNC_TRACE' and
> 'DEBUGFUNC'?
> >>>> As far as I can see they are for same reason, and both macros are
> >>>> used. If they are for same reason can you please unify the usage?
> >>>>
> >>>>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> >>>>> +#define DEBUGFUNC(fmt)            do { } while (0)
> >>>>
> >>>> If 'DEBUGFUNC' can be removed, why not removing from the code,
> >>>> instead of above define? This is your driver, you have full control on it.
> >>>
> >>> Okay. I just realize that this is going to be a big change, so I
> >>> want to keep it to
> >> a minimum.
> >>> Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already
> >> contains the function name.
> >>>
> >>
> >> If it will be big, we can move the fix out of this set, to not block
> >> set, and send a log fix later as a separate patch, what do you think?
> >
> > I think it's actually better.
> >
> 
> As long as you commit to send log fix after -rc1, I am OK.
> 
> Are you planning to send a new version of this set, or will it work if I just drop
> the patch 8/12 & 10/12 of this set and continue with this version?

It works, you can continue with this version. Thanks Ferruh.

[PATCH v2 10/12] net/txgbe: fix debug log

[Jiawen Wu]

Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
that double line was added by using 'DEBUGOUT'.

Fixes: 7dc117068a7c ("net/txgbe: support probe and remove")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/txgbe_logs.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/txgbe/txgbe_logs.h b/drivers/net/txgbe/txgbe_logs.h
index 67e9bfb3af..8fa80a69b2 100644
--- a/drivers/net/txgbe/txgbe_logs.h
+++ b/drivers/net/txgbe/txgbe_logs.h
@@ -17,10 +17,13 @@ extern int txgbe_logtype_init;
 		"%s(): " fmt "\n", __func__, ##args)
 
 extern int txgbe_logtype_driver;
-#define PMD_DRV_LOG(level, fmt, args...) \
+#define PMD_TLOG_DRIVER(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, txgbe_logtype_driver, \
 		"%s(): " fmt "\n", __func__, ##args)
 
+#define PMD_DRV_LOG(level, fmt, args...) \
+	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
+
 #ifdef RTE_LIBRTE_TXGBE_DEBUG_RX
 extern int txgbe_logtype_rx;
 #define PMD_RX_LOG(level, fmt, args...) \
@@ -48,11 +51,11 @@ extern int txgbe_logtype_tx_free;
 #define PMD_TX_FREE_LOG(level, fmt, args...) do { } while (0)
 #endif
 
-#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
+#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt, ##args)
 
 #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
 #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
-#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
+#define DEBUGFUNC(fmt)            do { } while (0)
 
 extern int txgbe_logtype_bp;
 #define BP_LOG(fmt, args...) \
-- 
2.21.0.windows.1

Re: [PATCH v2 10/12] net/txgbe: fix debug log

[Ferruh Yigit]

On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> that double line was added by using 'DEBUGOUT'.
> 
> Fixes: 7dc117068a7c ("net/txgbe: support probe and remove")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

Comments to ngbe patch (8/12) applies here too.

> ---
>   drivers/net/txgbe/txgbe_logs.h | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/txgbe/txgbe_logs.h b/drivers/net/txgbe/txgbe_logs.h
> index 67e9bfb3af..8fa80a69b2 100644
> --- a/drivers/net/txgbe/txgbe_logs.h
> +++ b/drivers/net/txgbe/txgbe_logs.h
> @@ -17,10 +17,13 @@ extern int txgbe_logtype_init;
>   		"%s(): " fmt "\n", __func__, ##args)
>   
>   extern int txgbe_logtype_driver;
> -#define PMD_DRV_LOG(level, fmt, args...) \
> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>   	rte_log(RTE_LOG_ ## level, txgbe_logtype_driver, \
>   		"%s(): " fmt "\n", __func__, ##args)
>   
> +#define PMD_DRV_LOG(level, fmt, args...) \
> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> +
>   #ifdef RTE_LIBRTE_TXGBE_DEBUG_RX
>   extern int txgbe_logtype_rx;
>   #define PMD_RX_LOG(level, fmt, args...) \
> @@ -48,11 +51,11 @@ extern int txgbe_logtype_tx_free;
>   #define PMD_TX_FREE_LOG(level, fmt, args...) do { } while (0)
>   #endif
>   
> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt, ##args)
>   
>   #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>   #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> +#define DEBUGFUNC(fmt)            do { } while (0)
>   
>   extern int txgbe_logtype_bp;
>   #define BP_LOG(fmt, args...) \

[PATCH v2 11/12] net/txgbe: fix to set link up and down

[Jiawen Wu]

Add hw->dev_start status in the flow of setting link up/down, to avoid
obtaining link status inconsistent with the settings.

Fixes: 12a653eb53e1 ("net/txgbe: fix link status when device stopped")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/txgbe_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 4799a60116..6bc209e661 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -1937,6 +1937,7 @@ txgbe_dev_set_link_up(struct rte_eth_dev *dev)
 	} else {
 		/* Turn on the laser */
 		hw->mac.enable_tx_laser(hw);
+		hw->dev_start = true;
 		txgbe_dev_link_update(dev, 0);
 	}
 
@@ -1957,6 +1958,7 @@ txgbe_dev_set_link_down(struct rte_eth_dev *dev)
 	} else {
 		/* Turn off the laser */
 		hw->mac.disable_tx_laser(hw);
+		hw->dev_start = false;
 		txgbe_dev_link_update(dev, 0);
 	}
 
-- 
2.21.0.windows.1

[PATCH v2 12/12] net/txgbe: fix KR auto-negotiation

[Jiawen Wu]

Fix failure to enter auto-negotiation mode on some firmware versions for
KR NICs.

Fixes: f611dada1af8 ("net/txgbe: update link setup process of backplane NICs")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/base/txgbe_phy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/txgbe/base/txgbe_phy.c b/drivers/net/txgbe/base/txgbe_phy.c
index 3f5229ecc2..3fb929f37a 100644
--- a/drivers/net/txgbe/base/txgbe_phy.c
+++ b/drivers/net/txgbe/base/txgbe_phy.c
@@ -1455,6 +1455,10 @@ txgbe_set_link_to_kr(struct txgbe_hw *hw, bool autoneg)
 		if (!(hw->devarg.auto_neg == 1)) {
 			wr32_epcs(hw, SR_AN_CTRL, 0);
 			wr32_epcs(hw, VR_AN_KR_MODE_CL, 0);
+		} else {
+			value = rd32_epcs(hw, TXGBE_PHY_TX_EQ_CTL1);
+			value &= ~(1 << 6);
+			wr32_epcs(hw, TXGBE_PHY_TX_EQ_CTL1, value);
 		}
 		if (hw->devarg.present == 1) {
 			value = rd32_epcs(hw, TXGBE_PHY_TX_EQ_CTL1);
-- 
2.21.0.windows.1