Re: [dpdk-dev] [PATCH 4/4] net/hns3: fix FEC state query

[Min Hu (Connor)]

在 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)" <humi...@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: sta...@dpdk.org

Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
Signed-off-by: Lijun Ou <ouli...@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);

.