DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH 0/4] hns3 fixes
@ 2020-11-20 11:27 Lijun Ou
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 1/4] net/hns3: fix segment fault with the multi-TC Lijun Ou
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Lijun Ou @ 2020-11-20 11:27 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, linuxarm

Here series are fixes for hns3 PMD driver.

Chengchang Tang (1):
  net/hns3: fix unused queues with not disabled

Huisong Li (1):
  net/hns3: fix segment fault with the multi-TC

Lijun Ou (1):
  net/hns3: adjust printing MAC addresses in log

Min Hu (Conor) (1):
  net/hns3: fix FEC state query

 drivers/net/hns3/hns3_dcb.c       |  6 +++
 drivers/net/hns3/hns3_ethdev.c    | 91 +++++++++++++++++++++++----------------
 drivers/net/hns3/hns3_ethdev.h    |  2 +
 drivers/net/hns3/hns3_ethdev_vf.c | 32 +++++++-------
 drivers/net/hns3/hns3_rxtx.c      | 35 +++++++++++----
 5 files changed, 106 insertions(+), 60 deletions(-)

-- 
2.7.4


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

* [dpdk-dev] [PATCH 1/4] net/hns3: fix segment fault with the multi-TC
  2020-11-20 11:27 [dpdk-dev] [PATCH 0/4] hns3 fixes Lijun Ou
@ 2020-11-20 11:27 ` Lijun Ou
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 2/4] net/hns3: fix unused queues with not disabled Lijun Ou
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Lijun Ou @ 2020-11-20 11:27 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, linuxarm

From: Huisong Li <lihuisong@huawei.com>

The HW and SW DCB configurations need to be updated only
after the DCB configuration information changed. But the
change of tx/rx queue number is ignored. If user decreases
the number of tx queue after configuring multi-TC, the queue
mapping information in hns3_tc_queue_info can not be updated.
And then accessing the released queue resource in
"hns3_init_tx_ring_tc" will trigger a segment fault.

Fixes: 62e3ccc2b94c ("net/hns3: support flow control")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 drivers/net/hns3/hns3_dcb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hns3/hns3_dcb.c b/drivers/net/hns3/hns3_dcb.c
index ab02c87..fb50179 100644
--- a/drivers/net/hns3/hns3_dcb.c
+++ b/drivers/net/hns3/hns3_dcb.c
@@ -1351,6 +1351,8 @@ hns3_dcb_cfg_validate(struct hns3_adapter *hns, uint8_t *tc, bool *changed)
 {
 	struct rte_eth_dcb_rx_conf *dcb_rx_conf;
 	struct hns3_hw *hw = &hns->hw;
+	uint16_t nb_rx_q = hw->data->nb_rx_queues;
+	uint16_t nb_tx_q = hw->data->nb_tx_queues;
 	uint8_t max_tc = 0;
 	uint8_t pfc_en;
 	int i;
@@ -1376,6 +1378,10 @@ hns3_dcb_cfg_validate(struct hns3_adapter *hns, uint8_t *tc, bool *changed)
 	pfc_en = RTE_LEN2MASK((uint8_t)dcb_rx_conf->nb_tcs, uint8_t);
 	if (hw->dcb_info.pfc_en != pfc_en)
 		*changed = true;
+
+	/* tx/rx queue number is reconfigured. */
+	if (nb_rx_q != hw->used_rx_queues || nb_tx_q != hw->used_tx_queues)
+		*changed = true;
 }
 
 static int
-- 
2.7.4


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

* [dpdk-dev] [PATCH 2/4] net/hns3: fix unused queues with not disabled
  2020-11-20 11:27 [dpdk-dev] [PATCH 0/4] hns3 fixes Lijun Ou
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 1/4] net/hns3: fix segment fault with the multi-TC Lijun Ou
@ 2020-11-20 11:27 ` Lijun Ou
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 3/4] net/hns3: adjust printing MAC addresses in log Lijun Ou
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Lijun Ou @ 2020-11-20 11:27 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, linuxarm

From: Chengchang Tang <tangchengchang@huawei.com>

For kupeng 930, there are 3 registers to control the enable
status of a TQP(i.e. task queue pair, include a txq and a rxq).
One of them controls whether the TQP is enabled, and the other
two controls whether the rxq and txq are enabled. The registers
used to control the enabled status of the rxq and txq are enabled
by default. Therefore, after the TQP is enabled, the rxq and txq
are enabled by default. Currently, when the number of rxq is not
equal to the number of txq, the unused rxqs or txqs are not disabled
by driver, so these unused queues will be enabled in this situation.
And the related HW rings have not been initialized which could
lead to a hardware exception.

This patch fix it by disable these unused queues during enable the TQPs.

Fixes: fa29fe45a7b4 ("net/hns3: support queue start and stop")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index c76e635..88d3bab 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -353,6 +353,19 @@ hns3_update_all_queues_pvid_proc_en(struct hns3_hw *hw)
 	}
 }
 
+static void
+hns3_stop_unused_queue(void *tqp_base, enum hns3_ring_type queue_type)
+{
+	uint32_t reg_offset;
+	uint32_t reg;
+
+	reg_offset = queue_type == HNS3_RING_TYPE_TX ?
+				   HNS3_RING_TX_EN_REG : HNS3_RING_RX_EN_REG;
+	reg = hns3_read_reg(tqp_base, reg_offset);
+	reg &= ~BIT(HNS3_RING_EN_B);
+	hns3_write_reg(tqp_base, reg_offset, reg);
+}
+
 void
 hns3_enable_all_queues(struct hns3_hw *hw, bool en)
 {
@@ -368,16 +381,22 @@ hns3_enable_all_queues(struct hns3_hw *hw, bool en)
 		if (hns3_dev_indep_txrx_supported(hw)) {
 			rxq = i < nb_rx_q ? hw->data->rx_queues[i] : NULL;
 			txq = i < nb_tx_q ? hw->data->tx_queues[i] : NULL;
+
+			tqp_base = (void *)((char *)hw->io_base +
+					hns3_get_tqp_reg_offset(i));
 			/*
-			 * After initialization, rxq and txq won't be NULL at
-			 * the same time.
+			 * If queue struct is not initialized, it means the
+			 * related HW ring has not been initialized yet.
+			 * So, these queues should be disabled before enable
+			 * the tqps to avoid a HW exception since the queues
+			 * are enabled by default.
 			 */
-			if (rxq != NULL)
-				tqp_base = rxq->io_base;
-			else if (txq != NULL)
-				tqp_base = txq->io_base;
-			else
-				return;
+			if (rxq == NULL)
+				hns3_stop_unused_queue(tqp_base,
+							HNS3_RING_TYPE_RX);
+			if (txq == NULL)
+				hns3_stop_unused_queue(tqp_base,
+							HNS3_RING_TYPE_TX);
 		} else {
 			rxq = i < nb_rx_q ? hw->data->rx_queues[i] :
 			      hw->fkq_data.rx_queues[i - nb_rx_q];
-- 
2.7.4


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

* [dpdk-dev] [PATCH 3/4] net/hns3: adjust printing MAC addresses in log
  2020-11-20 11:27 [dpdk-dev] [PATCH 0/4] hns3 fixes Lijun Ou
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 1/4] net/hns3: fix segment fault with the multi-TC Lijun Ou
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 2/4] net/hns3: fix unused queues with not disabled Lijun Ou
@ 2020-11-20 11:27 ` Lijun Ou
  2020-11-20 14:25   ` Ferruh Yigit
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query Lijun Ou
  2020-11-20 14:38 ` [dpdk-dev] [PATCH 0/4] hns3 fixes Ferruh Yigit
  4 siblings, 1 reply; 13+ messages in thread
From: Lijun Ou @ 2020-11-20 11:27 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, linuxarm

Here the printing of MAC addresses is adjusted. After the
modification, only some bytes of the MAC address are
displayed.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c    | 53 +++++++++++++++++++++++----------------
 drivers/net/hns3/hns3_ethdev.h    |  2 ++
 drivers/net/hns3/hns3_ethdev_vf.c | 32 +++++++++++------------
 3 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index 2011378..d6d3f03 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -102,6 +102,15 @@ static int hns3_remove_mc_addr(struct hns3_hw *hw,
 static int hns3_restore_fec(struct hns3_hw *hw);
 static int hns3_query_dev_fec_info(struct rte_eth_dev *dev);
 
+void hns3_ether_format_addr(char *buf, uint16_t size,
+			    const struct rte_ether_addr *ether_addr)
+{
+	snprintf(buf, size, "%02X:**:**:**:%02X:%02X",
+		ether_addr->addr_bytes[0],
+		ether_addr->addr_bytes[4],
+		ether_addr->addr_bytes[5]);
+}
+
 static void
 hns3_pf_disable_irq0(struct hns3_hw *hw)
 {
@@ -1449,7 +1458,7 @@ hns3_add_uc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	/* check if mac addr is valid */
 	if (!rte_is_valid_assigned_ether_addr(mac_addr)) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "Add unicast mac addr err! addr(%s) invalid",
 			 mac_str);
@@ -1489,7 +1498,7 @@ hns3_add_uc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 		return -ENOSPC;
 	}
 
-	rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE, mac_addr);
+	hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE, mac_addr);
 
 	/* check if we just hit the duplicate */
 	if (ret == 0) {
@@ -1515,7 +1524,7 @@ hns3_add_mc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 		addr = &hw->mc_addrs[i];
 		/* Check if there are duplicate addresses */
 		if (rte_is_same_ether_addr(addr, mac_addr)) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw, "failed to add mc mac addr, same addrs"
 				 "(%s) is added by the set_mc_mac_addr_list "
@@ -1526,7 +1535,7 @@ hns3_add_mc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	ret = hns3_add_mc_addr(hw, mac_addr);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add mc mac addr(%s), ret = %d",
 			 mac_str, ret);
@@ -1542,7 +1551,7 @@ hns3_remove_mc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	ret = hns3_remove_mc_addr(hw, mac_addr);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to remove mc mac addr(%s), ret = %d",
 			 mac_str, ret);
@@ -1576,7 +1585,7 @@ hns3_add_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr,
 
 	if (ret) {
 		rte_spinlock_unlock(&hw->lock);
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add mac addr(%s), ret = %d", mac_str,
 			 ret);
@@ -1599,7 +1608,7 @@ hns3_remove_uc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	/* check if mac addr is valid */
 	if (!rte_is_valid_assigned_ether_addr(mac_addr)) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "remove unicast mac addr err! addr(%s) invalid",
 			 mac_str);
@@ -1635,7 +1644,7 @@ hns3_remove_mac_addr(struct rte_eth_dev *dev, uint32_t idx)
 		ret = hns3_remove_uc_addr_common(hw, mac_addr);
 	rte_spinlock_unlock(&hw->lock);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to remove mac addr(%s), ret = %d", mac_str,
 			 ret);
@@ -1666,7 +1675,7 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
 	if (default_addr_setted) {
 		ret = hns3_remove_uc_addr_common(hw, oaddr);
 		if (ret) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      oaddr);
 			hns3_warn(hw, "Remove old uc mac address(%s) fail: %d",
 				  mac_str, ret);
@@ -1677,7 +1686,7 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
 
 	ret = hns3_add_uc_addr_common(hw, mac_addr);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "Failed to set mac addr(%s): %d", mac_str, ret);
 		goto err_add_uc_addr;
@@ -1699,7 +1708,7 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
 err_pause_addr_cfg:
 	ret_val = hns3_remove_uc_addr_common(hw, mac_addr);
 	if (ret_val) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_warn(hw,
 			  "Failed to roll back to del setted mac addr(%s): %d",
@@ -1710,7 +1719,7 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
 	if (rm_succes) {
 		ret_val = hns3_add_uc_addr_common(hw, oaddr);
 		if (ret_val) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      oaddr);
 			hns3_warn(hw,
 				  "Failed to restore old uc mac addr(%s): %d",
@@ -1746,7 +1755,7 @@ hns3_configure_all_mac_addr(struct hns3_adapter *hns, bool del)
 
 		if (ret) {
 			err = ret;
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw, "failed to %s mac addr(%s) index:%d "
 				 "ret = %d.", del ? "remove" : "restore",
@@ -1795,7 +1804,7 @@ hns3_add_mc_addr(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	/* Check if mac addr is valid */
 	if (!rte_is_multicast_ether_addr(mac_addr)) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add mc mac addr, addr(%s) invalid",
 			 mac_str);
@@ -1823,7 +1832,7 @@ hns3_add_mc_addr(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 	if (ret) {
 		if (ret == -ENOSPC)
 			hns3_err(hw, "mc mac vlan table is full");
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add mc mac addr(%s): %d", mac_str, ret);
 	}
@@ -1842,7 +1851,7 @@ hns3_remove_mc_addr(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	/* Check if mac addr is valid */
 	if (!rte_is_multicast_ether_addr(mac_addr)) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "Failed to rm mc mac addr, addr(%s) invalid",
 			 mac_str);
@@ -1870,7 +1879,7 @@ hns3_remove_mc_addr(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 	}
 
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "Failed to rm mc mac addr(%s): %d", mac_str, ret);
 	}
@@ -1899,7 +1908,7 @@ hns3_set_mc_addr_chk_param(struct hns3_hw *hw,
 	for (i = 0; i < nb_mc_addr; i++) {
 		addr = &mc_addr_set[i];
 		if (!rte_is_multicast_ether_addr(addr)) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw,
 				 "failed to set mc mac addr, addr(%s) invalid.",
@@ -1910,7 +1919,7 @@ hns3_set_mc_addr_chk_param(struct hns3_hw *hw,
 		/* Check if there are duplicate addresses */
 		for (j = i + 1; j < nb_mc_addr; j++) {
 			if (rte_is_same_ether_addr(addr, &mc_addr_set[j])) {
-				rte_ether_format_addr(mac_str,
+				hns3_ether_format_addr(mac_str,
 						      RTE_ETHER_ADDR_FMT_SIZE,
 						      addr);
 				hns3_err(hw, "failed to set mc mac addr, "
@@ -1927,7 +1936,7 @@ hns3_set_mc_addr_chk_param(struct hns3_hw *hw,
 		for (j = 0; j < HNS3_UC_MACADDR_NUM; j++) {
 			if (rte_is_same_ether_addr(addr,
 						   &hw->data->mac_addrs[j])) {
-				rte_ether_format_addr(mac_str,
+				hns3_ether_format_addr(mac_str,
 						      RTE_ETHER_ADDR_FMT_SIZE,
 						      addr);
 				hns3_err(hw, "failed to set mc mac addr, "
@@ -2101,7 +2110,7 @@ hns3_configure_all_mc_mac_addr(struct hns3_adapter *hns, bool del)
 			ret = hns3_add_mc_addr(hw, addr);
 		if (ret) {
 			err = ret;
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_dbg(hw, "%s mc mac addr: %s failed for pf: ret = %d",
 				 del ? "Remove" : "Restore", mac_str, ret);
@@ -6160,7 +6169,7 @@ hns3_dev_init(struct rte_eth_dev *eth_dev)
 	eth_addr = (struct rte_ether_addr *)hw->mac.mac_addr;
 	if (!rte_is_valid_assigned_ether_addr(eth_addr)) {
 		rte_eth_random_addr(hw->mac.mac_addr);
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				(struct rte_ether_addr *)hw->mac.mac_addr);
 		hns3_warn(hw, "default mac_addr from firmware is an invalid "
 			  "unicast address, using random MAC address %s",
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index 4c40df1..31f78a1 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -935,6 +935,8 @@ int hns3_dev_filter_ctrl(struct rte_eth_dev *dev,
 bool hns3_is_reset_pending(struct hns3_adapter *hns);
 bool hns3vf_is_reset_pending(struct hns3_adapter *hns);
 void hns3_update_link_status(struct hns3_hw *hw);
+void hns3_ether_format_addr(char *buf, uint16_t size,
+			const struct rte_ether_addr *ether_addr);
 
 static inline bool
 is_reset_pending(struct hns3_adapter *hns)
diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
index 0366b9d..f09cabc 100644
--- a/drivers/net/hns3/hns3_ethdev_vf.c
+++ b/drivers/net/hns3/hns3_ethdev_vf.c
@@ -170,7 +170,7 @@ hns3vf_add_uc_mac_addr(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 				HNS3_MBX_MAC_VLAN_UC_ADD, mac_addr->addr_bytes,
 				RTE_ETHER_ADDR_LEN, false, NULL, 0);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add uc mac addr(%s), ret = %d",
 			 mac_str, ret);
@@ -190,7 +190,7 @@ hns3vf_remove_uc_mac_addr(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 				mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN,
 				false, NULL, 0);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add uc mac addr(%s), ret = %d",
 			 mac_str, ret);
@@ -210,7 +210,7 @@ hns3vf_add_mc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 		addr = &hw->mc_addrs[i];
 		/* Check if there are duplicate addresses */
 		if (rte_is_same_ether_addr(addr, mac_addr)) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw, "failed to add mc mac addr, same addrs"
 				 "(%s) is added by the set_mc_mac_addr_list "
@@ -221,7 +221,7 @@ hns3vf_add_mc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
 
 	ret = hns3vf_add_mc_mac_addr(hw, mac_addr);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add mc mac addr(%s), ret = %d",
 			 mac_str, ret);
@@ -256,7 +256,7 @@ hns3vf_add_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr,
 
 	rte_spinlock_unlock(&hw->lock);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to add mac addr(%s), ret = %d", mac_str,
 			 ret);
@@ -283,7 +283,7 @@ hns3vf_remove_mac_addr(struct rte_eth_dev *dev, uint32_t idx)
 
 	rte_spinlock_unlock(&hw->lock);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "failed to remove mac addr(%s), ret = %d",
 			 mac_str, ret);
@@ -324,12 +324,12 @@ hns3vf_set_default_mac_addr(struct rte_eth_dev *dev,
 		 * -EPREM to VF driver through mailbox.
 		 */
 		if (ret == -EPERM) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      old_addr);
 			hns3_warn(hw, "Has permanet mac addr(%s) for vf",
 				  mac_str);
 		} else {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      mac_addr);
 			hns3_err(hw, "Failed to set mac addr(%s) for vf: %d",
 				 mac_str, ret);
@@ -366,7 +366,7 @@ hns3vf_configure_mac_addr(struct hns3_adapter *hns, bool del)
 
 		if (ret) {
 			err = ret;
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw, "failed to %s mac addr(%s) index:%d "
 				 "ret = %d.", del ? "remove" : "restore",
@@ -388,7 +388,7 @@ hns3vf_add_mc_mac_addr(struct hns3_hw *hw,
 				mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN, false,
 				NULL, 0);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "Failed to add mc mac addr(%s) for vf: %d",
 			 mac_str, ret);
@@ -409,7 +409,7 @@ hns3vf_remove_mc_mac_addr(struct hns3_hw *hw,
 				mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN, false,
 				NULL, 0);
 	if (ret) {
-		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 				      mac_addr);
 		hns3_err(hw, "Failed to remove mc mac addr(%s) for vf: %d",
 			 mac_str, ret);
@@ -439,7 +439,7 @@ hns3vf_set_mc_addr_chk_param(struct hns3_hw *hw,
 	for (i = 0; i < nb_mc_addr; i++) {
 		addr = &mc_addr_set[i];
 		if (!rte_is_multicast_ether_addr(addr)) {
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw,
 				 "failed to set mc mac addr, addr(%s) invalid.",
@@ -450,7 +450,7 @@ hns3vf_set_mc_addr_chk_param(struct hns3_hw *hw,
 		/* Check if there are duplicate addresses */
 		for (j = i + 1; j < nb_mc_addr; j++) {
 			if (rte_is_same_ether_addr(addr, &mc_addr_set[j])) {
-				rte_ether_format_addr(mac_str,
+				hns3_ether_format_addr(mac_str,
 						      RTE_ETHER_ADDR_FMT_SIZE,
 						      addr);
 				hns3_err(hw, "failed to set mc mac addr, "
@@ -467,7 +467,7 @@ hns3vf_set_mc_addr_chk_param(struct hns3_hw *hw,
 		for (j = 0; j < HNS3_VF_UC_MACADDR_NUM; j++) {
 			if (rte_is_same_ether_addr(addr,
 						   &hw->data->mac_addrs[j])) {
-				rte_ether_format_addr(mac_str,
+				hns3_ether_format_addr(mac_str,
 						      RTE_ETHER_ADDR_FMT_SIZE,
 						      addr);
 				hns3_err(hw, "failed to set mc mac addr, "
@@ -550,7 +550,7 @@ hns3vf_configure_all_mc_mac_addr(struct hns3_adapter *hns, bool del)
 			ret = hns3vf_add_mc_mac_addr(hw, addr);
 		if (ret) {
 			err = ret;
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      addr);
 			hns3_err(hw, "Failed to %s mc mac addr: %s for vf: %d",
 				 del ? "Remove" : "Restore", mac_str, ret);
@@ -2468,7 +2468,7 @@ hns3vf_check_default_mac_change(struct hns3_hw *hw)
 		ret = rte_is_same_ether_addr(&hw->data->mac_addrs[0], hw_mac);
 		if (!ret) {
 			rte_ether_addr_copy(hw_mac, &hw->data->mac_addrs[0]);
-			rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
+			hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
 					      &hw->data->mac_addrs[0]);
 			hns3_warn(hw, "Default MAC address has been changed to:"
 				  " %s by the host PF kernel ethdev driver",
-- 
2.7.4


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

* [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query
  2020-11-20 11:27 [dpdk-dev] [PATCH 0/4] hns3 fixes Lijun Ou
                   ` (2 preceding siblings ...)
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 3/4] net/hns3: adjust printing MAC addresses in log Lijun Ou
@ 2020-11-20 11:27 ` Lijun Ou
  2020-11-20 14:33   ` Ferruh Yigit
  2020-11-20 14:38 ` [dpdk-dev] [PATCH 0/4] hns3 fixes Ferruh Yigit
  4 siblings, 1 reply; 13+ messages in thread
From: Lijun Ou @ 2020-11-20 11:27 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, linuxarm

From: "Min Hu (Conor)" <humin29@huawei.com>

As FEC is not supported below 10 Gbps, CMD(0x031A) offered
from Firmware read will return fail in 10 Gbps device.

This patch will prevent read this CMD when below 10 Gbps,
as this is non-sense.

Fixes: 9bf2ea8dbc65 ("net/hns3: support FEC")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index d6d3f03..faa7b0a 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -100,7 +100,7 @@ static int hns3_add_mc_addr(struct hns3_hw *hw,
 static int hns3_remove_mc_addr(struct hns3_hw *hw,
 			    struct rte_ether_addr *mac_addr);
 static int hns3_restore_fec(struct hns3_hw *hw);
-static int hns3_query_dev_fec_info(struct rte_eth_dev *dev);
+static int hns3_query_dev_fec_info(struct hns3_hw *hw);
 
 void hns3_ether_format_addr(char *buf, uint16_t size,
 			    const struct rte_ether_addr *ether_addr)
@@ -3010,13 +3010,6 @@ hns3_get_capability(struct hns3_hw *hw)
 	    device_id == HNS3_DEV_ID_200G_RDMA)
 		hns3_set_bit(hw->capability, HNS3_DEV_SUPPORT_DCB_B, 1);
 
-	ret = hns3_query_dev_fec_info(eth_dev);
-	if (ret) {
-		PMD_INIT_LOG(ERR,
-			     "failed to query FEC information, ret = %d", ret);
-		return ret;
-	}
-
 	/* Get PCI revision id */
 	ret = rte_pci_read_config(pci_dev, &revision, HNS3_PCI_REVISION_ID_LEN,
 				  HNS3_PCI_REVISION_ID);
@@ -3148,8 +3141,15 @@ hns3_get_configuration(struct hns3_hw *hw)
 	}
 
 	ret = hns3_get_board_configuration(hw);
-	if (ret)
+	if (ret) {
 		PMD_INIT_LOG(ERR, "failed to get board configuration: %d", ret);
+		return ret;
+	}
+
+	ret = hns3_query_dev_fec_info(hw);
+	if (ret)
+		PMD_INIT_LOG(ERR,
+			     "failed to query FEC information, ret = %d", ret);
 
 	return ret;
 }
@@ -5797,6 +5797,15 @@ get_current_fec_auto_state(struct hns3_hw *hw, uint8_t *state)
 	struct hns3_cmd_desc desc;
 	int ret;
 
+	/*
+	 * CMD(0x031A) read is not supported in device of link speed
+	 * below 10 Gbps.
+	 */
+	if (hw->mac.link_speed < ETH_SPEED_NUM_10G) {
+		*state = 0;
+		return 0;
+	}
+
 	hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_CONFIG_FEC_MODE, true);
 	req = (struct hns3_config_fec_cmd *)desc.data;
 	ret = hns3_cmd_send(hw, &desc, 1);
@@ -6003,14 +6012,15 @@ hns3_restore_fec(struct hns3_hw *hw)
 }
 
 static int
-hns3_query_dev_fec_info(struct rte_eth_dev *dev)
+hns3_query_dev_fec_info(struct hns3_hw *hw)
 {
-	struct hns3_adapter *hns = dev->data->dev_private;
-	struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(hns);
-	struct hns3_pf *pf = &hns->pf;
+	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
+	struct hns3_pf *pf = HNS3_DEV_PRIVATE_TO_PF(hns);
+	struct rte_eth_dev *eth_dev;
 	int ret;
 
-	ret = hns3_fec_get(dev, &pf->fec_mode);
+	eth_dev = &rte_eth_devices[hw->data->port_id];
+	ret = hns3_fec_get(eth_dev, &pf->fec_mode);
 	if (ret)
 		hns3_err(hw, "query device FEC info failed, ret = %d", ret);
 
-- 
2.7.4


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

* Re: [dpdk-dev] [PATCH 3/4] net/hns3: adjust printing MAC addresses in log
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 3/4] net/hns3: adjust printing MAC addresses in log Lijun Ou
@ 2020-11-20 14:25   ` Ferruh Yigit
  0 siblings, 0 replies; 13+ messages in thread
From: Ferruh Yigit @ 2020-11-20 14:25 UTC (permalink / raw)
  To: Lijun Ou; +Cc: dev, linuxarm

On 11/20/2020 11:27 AM, Lijun Ou wrote:
> Here the printing of MAC addresses is adjusted. After the
> modification, only some bytes of the MAC address are
> displayed.
> 

Why logging only some bytes of the MAC address?

> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>   drivers/net/hns3/hns3_ethdev.c    | 53 +++++++++++++++++++++++----------------
>   drivers/net/hns3/hns3_ethdev.h    |  2 ++
>   drivers/net/hns3/hns3_ethdev_vf.c | 32 +++++++++++------------
>   3 files changed, 49 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index 2011378..d6d3f03 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -102,6 +102,15 @@ static int hns3_remove_mc_addr(struct hns3_hw *hw,
>   static int hns3_restore_fec(struct hns3_hw *hw);
>   static int hns3_query_dev_fec_info(struct rte_eth_dev *dev);
>   
> +void hns3_ether_format_addr(char *buf, uint16_t size,
> +			    const struct rte_ether_addr *ether_addr)
> +{
> +	snprintf(buf, size, "%02X:**:**:**:%02X:%02X",
> +		ether_addr->addr_bytes[0],
> +		ether_addr->addr_bytes[4],
> +		ether_addr->addr_bytes[5]);
> +}
> +
>   static void
>   hns3_pf_disable_irq0(struct hns3_hw *hw)
>   {
> @@ -1449,7 +1458,7 @@ hns3_add_uc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
>   
>   	/* check if mac addr is valid */
>   	if (!rte_is_valid_assigned_ether_addr(mac_addr)) {
> -		rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
> +		hns3_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
>   				      mac_addr);

Is all these interim variable only to log the mac address? If so why not use 
macros insted, something like:

#define FMT "%02x:%02x:%02x:%02x:%02x:%02x"
#define MAC(addr) addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]

hns3_err(fw, "Add unicast mac addr err! addr(" FMT ") invalid", MAC(mac_addr));


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

* Re: [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query Lijun Ou
@ 2020-11-20 14:33   ` Ferruh Yigit
  2020-11-20 14:35     ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2020-11-20 14:33 UTC (permalink / raw)
  To: Lijun Ou; +Cc: dev, linuxarm

On 11/20/2020 11:27 AM, Lijun Ou wrote:
> From: "Min Hu (Conor)" <humin29@huawei.com>
> 
> As FEC is not supported below 10 Gbps, CMD(0x031A) offered
> from Firmware read will return fail in 10 Gbps device.
> 
> This patch will prevent read this CMD when below 10 Gbps,
> as this is non-sense.
> 
> Fixes: 9bf2ea8dbc65 ("net/hns3: support FEC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>   drivers/net/hns3/hns3_ethdev.c | 38 ++++++++++++++++++++++++--------------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index d6d3f03..faa7b0a 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -100,7 +100,7 @@ static int hns3_add_mc_addr(struct hns3_hw *hw,
>   static int hns3_remove_mc_addr(struct hns3_hw *hw,
>   			    struct rte_ether_addr *mac_addr);
>   static int hns3_restore_fec(struct hns3_hw *hw);
> -static int hns3_query_dev_fec_info(struct rte_eth_dev *dev);
> +static int hns3_query_dev_fec_info(struct hns3_hw *hw);
>   
>   void hns3_ether_format_addr(char *buf, uint16_t size,
>   			    const struct rte_ether_addr *ether_addr)
> @@ -3010,13 +3010,6 @@ hns3_get_capability(struct hns3_hw *hw)
>   	    device_id == HNS3_DEV_ID_200G_RDMA)
>   		hns3_set_bit(hw->capability, HNS3_DEV_SUPPORT_DCB_B, 1);
>   
> -	ret = hns3_query_dev_fec_info(eth_dev);
> -	if (ret) {
> -		PMD_INIT_LOG(ERR,
> -			     "failed to query FEC information, ret = %d", ret);
> -		return ret;
> -	}
> -
>   	/* Get PCI revision id */
>   	ret = rte_pci_read_config(pci_dev, &revision, HNS3_PCI_REVISION_ID_LEN,
>   				  HNS3_PCI_REVISION_ID);
> @@ -3148,8 +3141,15 @@ hns3_get_configuration(struct hns3_hw *hw)
>   	}
>   
>   	ret = hns3_get_board_configuration(hw);
> -	if (ret)
> +	if (ret) {
>   		PMD_INIT_LOG(ERR, "failed to get board configuration: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = hns3_query_dev_fec_info(hw);
> +	if (ret)
> +		PMD_INIT_LOG(ERR,
> +			     "failed to query FEC information, ret = %d", ret);
>   
>   	return ret;
>   }
> @@ -5797,6 +5797,15 @@ get_current_fec_auto_state(struct hns3_hw *hw, uint8_t *state)
>   	struct hns3_cmd_desc desc;
>   	int ret;
>   
> +	/*
> +	 * CMD(0x031A) read is not supported in device of link speed
> +	 * below 10 Gbps.
> +	 */
> +	if (hw->mac.link_speed < ETH_SPEED_NUM_10G) {
> +		*state = 0;
> +		return 0;
> +	}
> +
>   	hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_CONFIG_FEC_MODE, true);
>   	req = (struct hns3_config_fec_cmd *)desc.data;
>   	ret = hns3_cmd_send(hw, &desc, 1);
> @@ -6003,14 +6012,15 @@ hns3_restore_fec(struct hns3_hw *hw)
>   }
>   
>   static int
> -hns3_query_dev_fec_info(struct rte_eth_dev *dev)
> +hns3_query_dev_fec_info(struct hns3_hw *hw)
>   {
> -	struct hns3_adapter *hns = dev->data->dev_private;
> -	struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(hns);
> -	struct hns3_pf *pf = &hns->pf;
> +	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
> +	struct hns3_pf *pf = HNS3_DEV_PRIVATE_TO_PF(hns);
> +	struct rte_eth_dev *eth_dev;
>   	int ret;
>   
> -	ret = hns3_fec_get(dev, &pf->fec_mode);
> +	eth_dev = &rte_eth_devices[hw->data->port_id];

Not specific to this patch, but it is not good idea to access global 
'rte_eth_devices' array directly. Why not store the 'eth_dev' in the device 
private data in the probe() and use it later?
Can you do a separate patch for this switch?
Thank you may not need to change the paramters of the 
'hns3_query_dev_fec_info()' or remove it from 'hns3_query_dev_fec_info()' since 
you will be able to access to 'eth_dev' easily.

> +	ret = hns3_fec_get(eth_dev, &pf->fec_mode);
>   	if (ret)
>   		hns3_err(hw, "query device FEC info failed, ret = %d", ret);
>   
> 


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

* Re: [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query
  2020-11-20 14:33   ` Ferruh Yigit
@ 2020-11-20 14:35     ` Ferruh Yigit
  2020-12-02 12:42       ` Min Hu (Connor)
  0 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2020-11-20 14:35 UTC (permalink / raw)
  To: Lijun Ou; +Cc: dev, linuxarm

On 11/20/2020 2:33 PM, Ferruh Yigit wrote:
> On 11/20/2020 11:27 AM, Lijun Ou wrote:
>> From: "Min Hu (Conor)" <humin29@huawei.com>
>>
>> As FEC is not supported below 10 Gbps, CMD(0x031A) offered
>> from Firmware read will return fail in 10 Gbps device.
>>
>> This patch will prevent read this CMD when below 10 Gbps,
>> as this is non-sense.
>>
>> Fixes: 9bf2ea8dbc65 ("net/hns3: support FEC")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>>   drivers/net/hns3/hns3_ethdev.c | 38 ++++++++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
>> index d6d3f03..faa7b0a 100644
>> --- a/drivers/net/hns3/hns3_ethdev.c
>> +++ b/drivers/net/hns3/hns3_ethdev.c
>> @@ -100,7 +100,7 @@ static int hns3_add_mc_addr(struct hns3_hw *hw,
>>   static int hns3_remove_mc_addr(struct hns3_hw *hw,
>>                   struct rte_ether_addr *mac_addr);
>>   static int hns3_restore_fec(struct hns3_hw *hw);
>> -static int hns3_query_dev_fec_info(struct rte_eth_dev *dev);
>> +static int hns3_query_dev_fec_info(struct hns3_hw *hw);
>>   void hns3_ether_format_addr(char *buf, uint16_t size,
>>                   const struct rte_ether_addr *ether_addr)
>> @@ -3010,13 +3010,6 @@ hns3_get_capability(struct hns3_hw *hw)
>>           device_id == HNS3_DEV_ID_200G_RDMA)
>>           hns3_set_bit(hw->capability, HNS3_DEV_SUPPORT_DCB_B, 1);
>> -    ret = hns3_query_dev_fec_info(eth_dev);
>> -    if (ret) {
>> -        PMD_INIT_LOG(ERR,
>> -                 "failed to query FEC information, ret = %d", ret);
>> -        return ret;
>> -    }
>> -
>>       /* Get PCI revision id */
>>       ret = rte_pci_read_config(pci_dev, &revision, HNS3_PCI_REVISION_ID_LEN,
>>                     HNS3_PCI_REVISION_ID);
>> @@ -3148,8 +3141,15 @@ hns3_get_configuration(struct hns3_hw *hw)
>>       }
>>       ret = hns3_get_board_configuration(hw);
>> -    if (ret)
>> +    if (ret) {
>>           PMD_INIT_LOG(ERR, "failed to get board configuration: %d", ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = hns3_query_dev_fec_info(hw);
>> +    if (ret)
>> +        PMD_INIT_LOG(ERR,
>> +                 "failed to query FEC information, ret = %d", ret);
>>       return ret;
>>   }
>> @@ -5797,6 +5797,15 @@ get_current_fec_auto_state(struct hns3_hw *hw, uint8_t 
>> *state)
>>       struct hns3_cmd_desc desc;
>>       int ret;
>> +    /*
>> +     * CMD(0x031A) read is not supported in device of link speed
>> +     * below 10 Gbps.
>> +     */

Also you can refer to "CMD(0x031A)" as 'HNS3_OPC_CONFIG_FEC_MODE', both in this 
comment and in the commit log, it is easier to understand than the numerical 
representation of the command.

>> +    if (hw->mac.link_speed < ETH_SPEED_NUM_10G) {
>> +        *state = 0;
>> +        return 0;
>> +    }
>> +
>>       hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_CONFIG_FEC_MODE, true);
>>       req = (struct hns3_config_fec_cmd *)desc.data;
>>       ret = hns3_cmd_send(hw, &desc, 1);
>> @@ -6003,14 +6012,15 @@ hns3_restore_fec(struct hns3_hw *hw)
>>   }
>>   static int
>> -hns3_query_dev_fec_info(struct rte_eth_dev *dev)
>> +hns3_query_dev_fec_info(struct hns3_hw *hw)
>>   {
>> -    struct hns3_adapter *hns = dev->data->dev_private;
>> -    struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(hns);
>> -    struct hns3_pf *pf = &hns->pf;
>> +    struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
>> +    struct hns3_pf *pf = HNS3_DEV_PRIVATE_TO_PF(hns);
>> +    struct rte_eth_dev *eth_dev;
>>       int ret;
>> -    ret = hns3_fec_get(dev, &pf->fec_mode);
>> +    eth_dev = &rte_eth_devices[hw->data->port_id];
> 
> Not specific to this patch, but it is not good idea to access global 
> 'rte_eth_devices' array directly. Why not store the 'eth_dev' in the device 
> private data in the probe() and use it later?
> Can you do a separate patch for this switch?
> Thank you may not need to change the paramters of the 
> 'hns3_query_dev_fec_info()' or remove it from 'hns3_query_dev_fec_info()' since 
> you will be able to access to 'eth_dev' easily.
> 
>> +    ret = hns3_fec_get(eth_dev, &pf->fec_mode);
>>       if (ret)
>>           hns3_err(hw, "query device FEC info failed, ret = %d", ret);
>>
> 


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

* Re: [dpdk-dev] [PATCH 0/4] hns3 fixes
  2020-11-20 11:27 [dpdk-dev] [PATCH 0/4] hns3 fixes Lijun Ou
                   ` (3 preceding siblings ...)
  2020-11-20 11:27 ` [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query Lijun Ou
@ 2020-11-20 14:38 ` Ferruh Yigit
  2020-11-20 14:58   ` oulijun
  4 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2020-11-20 14:38 UTC (permalink / raw)
  To: Lijun Ou; +Cc: dev, linuxarm

On 11/20/2020 11:27 AM, Lijun Ou wrote:
> Here series are fixes for hns3 PMD driver.
> 
> Chengchang Tang (1):
>    net/hns3: fix unused queues with not disabled
> 
> Huisong Li (1):
>    net/hns3: fix segment fault with the multi-TC
> 
> Lijun Ou (1):
>    net/hns3: adjust printing MAC addresses in log
> 
> Min Hu (Conor) (1):
>    net/hns3: fix FEC state query
> 

Hi Lijun,

How critical these fixes are?
We have -rc5 today, and release this Wednesday, so these will be last minute 
fixes, if these are not critical I suggest postponing to next release and fixes 
can be backported from there to LTS.

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

* Re: [dpdk-dev] [PATCH 0/4] hns3 fixes
  2020-11-20 14:38 ` [dpdk-dev] [PATCH 0/4] hns3 fixes Ferruh Yigit
@ 2020-11-20 14:58   ` oulijun
  2020-11-20 15:38     ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: oulijun @ 2020-11-20 14:58 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Linuxarm

Hi£¬Ferruh
the first and second patch are critical.



________________________________

Å·Àö¾ü
Mobile£º+86-18899774289<tel:+86-18899774289>
Email£ºoulijun@huawei.com<mailto:oulijun@huawei.com>


·¢¼þÈË£º Ferruh Yigit<ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>
ÊÕ¼þÈË£º oulijun<oulijun@huawei.com<mailto:oulijun@huawei.com>>
³­ËÍ£º dev<dev@dpdk.org<mailto:dev@dpdk.org>>;Linuxarm<linuxarm@huawei.com<mailto:linuxarm@huawei.com>>
Ö÷Ì⣺ Re: [PATCH 0/4] hns3 fixes
ʱ¼ä£º 2020-11-20 22:39:13

On 11/20/2020 11:27 AM, Lijun Ou wrote:
> Here series are fixes for hns3 PMD driver.
>
> Chengchang Tang (1):
>    net/hns3: fix unused queues with not disabled
>
> Huisong Li (1):
>    net/hns3: fix segment fault with the multi-TC
>
> Lijun Ou (1):
>    net/hns3: adjust printing MAC addresses in log
>
> Min Hu (Conor) (1):
>    net/hns3: fix FEC state query
>

Hi Lijun,

How critical these fixes are?
We have -rc5 today, and release this Wednesday, so these will be last minute
fixes, if these are not critical I suggest postponing to next release and fixes
can be backported from there to LTS.

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

* Re: [dpdk-dev] [PATCH 0/4] hns3 fixes
  2020-11-20 14:58   ` oulijun
@ 2020-11-20 15:38     ` Ferruh Yigit
  2020-11-20 17:53       ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2020-11-20 15:38 UTC (permalink / raw)
  To: oulijun; +Cc: dev, Linuxarm

On 11/20/2020 2:58 PM, oulijun wrote:

< Moved to the bottom, please do not top post >

>> 
>> --------------------------------------------------------------------------------
>> 
>> 欧丽军
>> Mobile:+86-18899774289 <tel:+86-18899774289>
>> Email:oulijun@huawei.com <mailto:oulijun@huawei.com>
>> 
>> 
>> *发件人: *Ferruh Yigit<ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>
>> *收件人: *oulijun<oulijun@huawei.com <mailto:oulijun@huawei.com>>
>> *抄送: *dev<dev@dpdk.org <mailto:dev@dpdk.org>>;Linuxarm<linuxarm@huawei.com 
>> <mailto:linuxarm@huawei.com>>
>> *主题: *Re: [PATCH 0/4] hns3 fixes
>> *时间: *2020-11-20 22:39:13
>> 
>> On 11/20/2020 11:27 AM, Lijun Ou wrote:
>>> Here series are fixes for hns3 PMD driver.
>>> 
>>> Chengchang Tang (1):
>>>    net/hns3: fix unused queues with not disabled
>>> 
>>> Huisong Li (1):
>>>    net/hns3: fix segment fault with the multi-TC
>>> 
>>> Lijun Ou (1):
>>>    net/hns3: adjust printing MAC addresses in log
>>> 
>>> Min Hu (Conor) (1):
>>>    net/hns3: fix FEC state query
>>> 
>> 
>> Hi Lijun,
>> 
>> How critical these fixes are?
>> We have -rc5 today, and release this Wednesday, so these will be last minute
>> fixes, if these are not critical I suggest postponing to next release and fixes
>> can be backported from there to LTS.
 >
 > Hi,Ferruh
 > the first and second patch are critical.
 >

OK, lets target first two patches for this release and defer rest.

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

* Re: [dpdk-dev] [PATCH 0/4] hns3 fixes
  2020-11-20 15:38     ` Ferruh Yigit
@ 2020-11-20 17:53       ` Ferruh Yigit
  0 siblings, 0 replies; 13+ messages in thread
From: Ferruh Yigit @ 2020-11-20 17:53 UTC (permalink / raw)
  To: oulijun; +Cc: dev, Linuxarm

On 11/20/2020 3:38 PM, Ferruh Yigit wrote:
> On 11/20/2020 2:58 PM, oulijun wrote:
> 
> < Moved to the bottom, please do not top post >
> 
>>>
>>> --------------------------------------------------------------------------------
>>>
>>> 欧丽军
>>> Mobile:+86-18899774289 <tel:+86-18899774289>
>>> Email:oulijun@huawei.com <mailto:oulijun@huawei.com>
>>>
>>>
>>> *发件人: *Ferruh Yigit<ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>
>>> *收件人: *oulijun<oulijun@huawei.com <mailto:oulijun@huawei.com>>
>>> *抄送: *dev<dev@dpdk.org <mailto:dev@dpdk.org>>;Linuxarm<linuxarm@huawei.com 
>>> <mailto:linuxarm@huawei.com>>
>>> *主题: *Re: [PATCH 0/4] hns3 fixes
>>> *时间: *2020-11-20 22:39:13
>>>
>>> On 11/20/2020 11:27 AM, Lijun Ou wrote:
>>>> Here series are fixes for hns3 PMD driver.
>>>>
>>>> Chengchang Tang (1):
>>>>    net/hns3: fix unused queues with not disabled
>>>>
>>>> Huisong Li (1):
>>>>    net/hns3: fix segment fault with the multi-TC
>>>>
>>>> Lijun Ou (1):
>>>>    net/hns3: adjust printing MAC addresses in log
>>>>
>>>> Min Hu (Conor) (1):
>>>>    net/hns3: fix FEC state query
>>>>
>>>
>>> Hi Lijun,
>>>
>>> How critical these fixes are?
>>> We have -rc5 today, and release this Wednesday, so these will be last minute
>>> fixes, if these are not critical I suggest postponing to next release and fixes
>>> can be backported from there to LTS.
>  >
>  > Hi,Ferruh
>  > the first and second patch are critical.
>  >
> 
> OK, lets target first two patches for this release and defer rest.

1/4 & 2/4 applied to dpdk-next-net/main, thanks.

3/4 & 4/4 deferred to next release and patchwork status updated accordingly.

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

* Re: [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query
  2020-11-20 14:35     ` Ferruh Yigit
@ 2020-12-02 12:42       ` Min Hu (Connor)
  0 siblings, 0 replies; 13+ messages in thread
From: Min Hu (Connor) @ 2020-12-02 12:42 UTC (permalink / raw)
  To: dev



在 2020/11/20 22:35, Ferruh Yigit 写道:
> On 11/20/2020 2:33 PM, Ferruh Yigit wrote:
>> On 11/20/2020 11:27 AM, Lijun Ou wrote:
>>> From: "Min Hu (Conor)" <humin29@huawei.com>
>>>
>>> As FEC is not supported below 10 Gbps, CMD(0x031A) offered
>>> from Firmware read will return fail in 10 Gbps device.
>>>
>>> This patch will prevent read this CMD when below 10 Gbps,
>>> as this is non-sense.
>>>
>>> Fixes: 9bf2ea8dbc65 ("net/hns3: support FEC")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> ---
>>>   drivers/net/hns3/hns3_ethdev.c | 38 
>>> ++++++++++++++++++++++++--------------
>>>   1 file changed, 24 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/net/hns3/hns3_ethdev.c 
>>> b/drivers/net/hns3/hns3_ethdev.c
>>> index d6d3f03..faa7b0a 100644
>>> --- a/drivers/net/hns3/hns3_ethdev.c
>>> +++ b/drivers/net/hns3/hns3_ethdev.c
>>> @@ -100,7 +100,7 @@ static int hns3_add_mc_addr(struct hns3_hw *hw,
>>>   static int hns3_remove_mc_addr(struct hns3_hw *hw,
>>>                   struct rte_ether_addr *mac_addr);
>>>   static int hns3_restore_fec(struct hns3_hw *hw);
>>> -static int hns3_query_dev_fec_info(struct rte_eth_dev *dev);
>>> +static int hns3_query_dev_fec_info(struct hns3_hw *hw);
>>>   void hns3_ether_format_addr(char *buf, uint16_t size,
>>>                   const struct rte_ether_addr *ether_addr)
>>> @@ -3010,13 +3010,6 @@ hns3_get_capability(struct hns3_hw *hw)
>>>           device_id == HNS3_DEV_ID_200G_RDMA)
>>>           hns3_set_bit(hw->capability, HNS3_DEV_SUPPORT_DCB_B, 1);
>>> -    ret = hns3_query_dev_fec_info(eth_dev);
>>> -    if (ret) {
>>> -        PMD_INIT_LOG(ERR,
>>> -                 "failed to query FEC information, ret = %d", ret);
>>> -        return ret;
>>> -    }
>>> -
>>>       /* Get PCI revision id */
>>>       ret = rte_pci_read_config(pci_dev, &revision, 
>>> HNS3_PCI_REVISION_ID_LEN,
>>>                     HNS3_PCI_REVISION_ID);
>>> @@ -3148,8 +3141,15 @@ hns3_get_configuration(struct hns3_hw *hw)
>>>       }
>>>       ret = hns3_get_board_configuration(hw);
>>> -    if (ret)
>>> +    if (ret) {
>>>           PMD_INIT_LOG(ERR, "failed to get board configuration: %d", 
>>> ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = hns3_query_dev_fec_info(hw);
>>> +    if (ret)
>>> +        PMD_INIT_LOG(ERR,
>>> +                 "failed to query FEC information, ret = %d", ret);
>>>       return ret;
>>>   }
>>> @@ -5797,6 +5797,15 @@ get_current_fec_auto_state(struct hns3_hw *hw, 
>>> uint8_t *state)
>>>       struct hns3_cmd_desc desc;
>>>       int ret;
>>> +    /*
>>> +     * CMD(0x031A) read is not supported in device of link speed
>>> +     * below 10 Gbps.
>>> +     */
> 
> Also you can refer to "CMD(0x031A)" as 'HNS3_OPC_CONFIG_FEC_MODE', both 
> in this comment and in the commit log, it is easier to understand than 
> the numerical representation of the command.
thanks, Ferruh, oulijun will fix it and send V2.

> 
>>> +    if (hw->mac.link_speed < ETH_SPEED_NUM_10G) {
>>> +        *state = 0;
>>> +        return 0;
>>> +    }
>>> +
>>>       hns3_cmd_setup_basic_desc(&desc, HNS3_OPC_CONFIG_FEC_MODE, true);
>>>       req = (struct hns3_config_fec_cmd *)desc.data;
>>>       ret = hns3_cmd_send(hw, &desc, 1);
>>> @@ -6003,14 +6012,15 @@ hns3_restore_fec(struct hns3_hw *hw)
>>>   }
>>>   static int
>>> -hns3_query_dev_fec_info(struct rte_eth_dev *dev)
>>> +hns3_query_dev_fec_info(struct hns3_hw *hw)
>>>   {
>>> -    struct hns3_adapter *hns = dev->data->dev_private;
>>> -    struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(hns);
>>> -    struct hns3_pf *pf = &hns->pf;
>>> +    struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
>>> +    struct hns3_pf *pf = HNS3_DEV_PRIVATE_TO_PF(hns);
>>> +    struct rte_eth_dev *eth_dev;
>>>       int ret;
>>> -    ret = hns3_fec_get(dev, &pf->fec_mode);
>>> +    eth_dev = &rte_eth_devices[hw->data->port_id];
>>
>> Not specific to this patch, but it is not good idea to access global 
>> 'rte_eth_devices' array directly. Why not store the 'eth_dev' in the 
>> device private data in the probe() and use it later?
>> Can you do a separate patch for this switch?
>> Thank you may not need to change the paramters of the 
>> 'hns3_query_dev_fec_info()' or remove it from 
>> 'hns3_query_dev_fec_info()' since you will be able to access to 
>> 'eth_dev' easily.
Hi, Ferruh, I will fix it by doing a separate patch.

>>
>>> +    ret = hns3_fec_get(eth_dev, &pf->fec_mode);
>>>       if (ret)
>>>           hns3_err(hw, "query device FEC info failed, ret = %d", ret);
>>>
>>
> 
> .

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

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 11:27 [dpdk-dev] [PATCH 0/4] hns3 fixes Lijun Ou
2020-11-20 11:27 ` [dpdk-dev] [PATCH 1/4] net/hns3: fix segment fault with the multi-TC Lijun Ou
2020-11-20 11:27 ` [dpdk-dev] [PATCH 2/4] net/hns3: fix unused queues with not disabled Lijun Ou
2020-11-20 11:27 ` [dpdk-dev] [PATCH 3/4] net/hns3: adjust printing MAC addresses in log Lijun Ou
2020-11-20 14:25   ` Ferruh Yigit
2020-11-20 11:27 ` [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query Lijun Ou
2020-11-20 14:33   ` Ferruh Yigit
2020-11-20 14:35     ` Ferruh Yigit
2020-12-02 12:42       ` Min Hu (Connor)
2020-11-20 14:38 ` [dpdk-dev] [PATCH 0/4] hns3 fixes Ferruh Yigit
2020-11-20 14:58   ` oulijun
2020-11-20 15:38     ` Ferruh Yigit
2020-11-20 17:53       ` Ferruh Yigit

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox