[04/10] net/nfp: improve the logic readability

Message ID 20241010091716.3631747-5-chaoyong.he@corigine.com (mailing list archive)
State Superseded
Delegated to: Ferruh Yigit
Headers
Series modify some logic of NFP PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chaoyong He Oct. 10, 2024, 9:17 a.m. UTC
Try our best to make the logic in secondary proess the same with the
primary process and improve the readability, by add helper functions
and rename function.

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/net/nfp/nfp_ethdev.c     | 86 +++++++++++++++-----------------
 drivers/net/nfp/nfp_net_common.c | 22 +++++++-
 drivers/net/nfp/nfp_net_common.h |  3 +-
 3 files changed, 63 insertions(+), 48 deletions(-)
  

Comments

Stephen Hemminger Oct. 10, 2024, 3:10 p.m. UTC | #1
On Thu, 10 Oct 2024 17:17:10 +0800
Chaoyong He <chaoyong.he@corigine.com> wrote:

> +
> +port_cleanup:
> +	for (uint32_t j = 0; j < i; j++) {
> +		struct rte_eth_dev *eth_dev;
> +
> +		nfp_port_name_generate(port_name, sizeof(port_name), j, pf_dev);
> +		eth_dev = rte_eth_dev_get_by_name(port_name);
> +		if (eth_dev != NULL)
> +			rte_eth_dev_destroy(eth_dev, NULL);
> +	}
> +

You could skip the lookup if you kept an array of eth_dev's that were created?
  
Chaoyong He Oct. 11, 2024, 2:36 a.m. UTC | #2
> On Thu, 10 Oct 2024 17:17:10 +0800
> Chaoyong He <chaoyong.he@corigine.com> wrote:
> 
> > +
> > +port_cleanup:
> > +	for (uint32_t j = 0; j < i; j++) {
> > +		struct rte_eth_dev *eth_dev;
> > +
> > +		nfp_port_name_generate(port_name, sizeof(port_name), j,
> pf_dev);
> > +		eth_dev = rte_eth_dev_get_by_name(port_name);
> > +		if (eth_dev != NULL)
> > +			rte_eth_dev_destroy(eth_dev, NULL);
> > +	}
> > +
> 
> You could skip the lookup if you kept an array of eth_dev's that were created?

Yes, it does make sense and sounds a good idea.
Maybe we can do it in a future patch rather than in this one?

Anyway, thanks for your review.
  

Patch

diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index 121f82af6d..405386e882 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -1764,19 +1764,9 @@  nfp_enable_multi_pf(struct nfp_pf_dev *pf_dev)
 static bool
 nfp_app_fw_nic_total_phyports_check(struct nfp_pf_dev *pf_dev)
 {
-	int ret;
-	uint8_t id;
 	uint8_t total_phyports;
-	char vnic_name[RTE_ETH_NAME_MAX_LEN];
 
-	/* Read the number of vNIC's created for the PF */
-	id = nfp_function_id_get(pf_dev, 0);
-	snprintf(vnic_name, sizeof(vnic_name), "nfd_cfg_pf%u_num_ports", id);
-	total_phyports = nfp_rtsym_read_le(pf_dev->sym_tbl, vnic_name, &ret);
-	if (ret != 0 || total_phyports == 0 || total_phyports > 8) {
-		PMD_INIT_LOG(ERR, "%s symbol with wrong value", vnic_name);
-		return false;
-	}
+	total_phyports = nfp_net_get_phyports_from_fw(pf_dev);
 
 	if (pf_dev->multi_pf.enabled) {
 		if (!nfp_check_multi_pf_from_fw(total_phyports)) {
@@ -1797,6 +1787,20 @@  nfp_app_fw_nic_total_phyports_check(struct nfp_pf_dev *pf_dev)
 	return true;
 }
 
+static void
+nfp_port_name_generate(char *port_name,
+		size_t length,
+		int port_id,
+		struct nfp_pf_dev *pf_dev)
+{
+	const char *name = pf_dev->pci_dev->device.name;
+
+	if (pf_dev->multi_pf.enabled)
+		snprintf(port_name, length, "%s", name);
+	else
+		snprintf(port_name, length, "%s_port%u", name, port_id);
+}
+
 static int
 nfp_init_app_fw_nic(struct nfp_net_hw_priv *hw_priv)
 {
@@ -1849,12 +1853,7 @@  nfp_init_app_fw_nic(struct nfp_net_hw_priv *hw_priv)
 
 	/* Loop through all physical ports on PF */
 	for (i = 0; i < pf_dev->total_phyports; i++) {
-		if (pf_dev->multi_pf.enabled)
-			snprintf(port_name, sizeof(port_name), "%s",
-					pf_dev->pci_dev->device.name);
-		else
-			snprintf(port_name, sizeof(port_name), "%s_port%u",
-					pf_dev->pci_dev->device.name, i);
+		nfp_port_name_generate(port_name, sizeof(port_name), i, pf_dev);
 
 		id = nfp_function_id_get(pf_dev, i);
 		hw_init.idx = id;
@@ -1870,15 +1869,10 @@  nfp_init_app_fw_nic(struct nfp_net_hw_priv *hw_priv)
 	return 0;
 
 port_cleanup:
-	for (i = 0; i < pf_dev->total_phyports; i++) {
+	for (uint32_t j = 0; j < i; j++) {
 		struct rte_eth_dev *eth_dev;
 
-		if (pf_dev->multi_pf.enabled)
-			snprintf(port_name, sizeof(port_name), "%s",
-					pf_dev->pci_dev->device.name);
-		else
-			snprintf(port_name, sizeof(port_name), "%s_port%u",
-					pf_dev->pci_dev->device.name, i);
+		nfp_port_name_generate(port_name, sizeof(port_name), j, pf_dev);
 		eth_dev = rte_eth_dev_get_by_name(port_name);
 		if (eth_dev != NULL)
 			rte_eth_dev_destroy(eth_dev, nfp_net_uninit);
@@ -2369,7 +2363,7 @@  nfp_pf_init(struct rte_pci_device *pci_dev)
 	pf_dev->nfp_eth_table = nfp_eth_table;
 	pf_dev->multi_pf.enabled = nfp_check_multi_pf_from_nsp(pci_dev, cpp);
 	pf_dev->multi_pf.function_id = function_id;
-	pf_dev->total_phyports = nfp_net_get_port_num(pf_dev);
+	pf_dev->total_phyports = nfp_net_get_phyports_from_nsp(pf_dev);
 
 	ret = nfp_net_force_port_down(pf_dev);
 	if (ret != 0) {
@@ -2547,42 +2541,37 @@  static int
 nfp_secondary_init_app_fw_nic(struct nfp_net_hw_priv *hw_priv)
 {
 	uint32_t i;
-	int err = 0;
 	int ret = 0;
-	uint8_t function_id;
 	uint32_t total_vnics;
-	char pf_name[RTE_ETH_NAME_MAX_LEN];
+	char port_name[RTE_ETH_NAME_MAX_LEN];
 	struct nfp_pf_dev *pf_dev = hw_priv->pf_dev;
 
-	/* Read the number of vNIC's created for the PF */
-	function_id = (pf_dev->pci_dev->addr.function) & 0x07;
-	snprintf(pf_name, sizeof(pf_name), "nfd_cfg_pf%u_num_ports", function_id);
-	total_vnics = nfp_rtsym_read_le(pf_dev->sym_tbl, pf_name, &err);
-	if (err != 0 || total_vnics == 0 || total_vnics > 8) {
-		PMD_INIT_LOG(ERR, "%s symbol with wrong value", pf_name);
-		return -ENODEV;
-	}
+	total_vnics = nfp_net_get_phyports_from_fw(pf_dev);
 
 	for (i = 0; i < total_vnics; i++) {
-		char port_name[RTE_ETH_NAME_MAX_LEN];
-
-		if (nfp_check_multi_pf_from_fw(total_vnics))
-			snprintf(port_name, sizeof(port_name), "%s",
-					pf_dev->pci_dev->device.name);
-		else
-			snprintf(port_name, sizeof(port_name), "%s_port%u",
-					pf_dev->pci_dev->device.name, i);
+		nfp_port_name_generate(port_name, sizeof(port_name), i, pf_dev);
 
 		PMD_INIT_LOG(DEBUG, "Secondary attaching to port %s", port_name);
 		ret = rte_eth_dev_create(&pf_dev->pci_dev->device, port_name, 0,
 				NULL, NULL, nfp_secondary_net_init, hw_priv);
 		if (ret != 0) {
 			PMD_INIT_LOG(ERR, "Secondary process attach to port %s failed", port_name);
-			ret = -ENODEV;
-			break;
+			goto port_cleanup;
 		}
 	}
 
+	return 0;
+
+port_cleanup:
+	for (uint32_t j = 0; j < i; j++) {
+		struct rte_eth_dev *eth_dev;
+
+		nfp_port_name_generate(port_name, sizeof(port_name), j, pf_dev);
+		eth_dev = rte_eth_dev_get_by_name(port_name);
+		if (eth_dev != NULL)
+			rte_eth_dev_destroy(eth_dev, NULL);
+	}
+
 	return ret;
 }
 
@@ -2714,6 +2703,11 @@  nfp_pf_secondary_init(struct rte_pci_device *pci_dev)
 
 	pf_dev->sym_tbl = sym_tbl;
 
+	/* Read the number of physical ports from firmware */
+	pf_dev->multi_pf.function_id = function_id;
+	pf_dev->total_phyports = nfp_net_get_phyports_from_fw(pf_dev);
+	pf_dev->multi_pf.enabled = nfp_check_multi_pf_from_fw(pf_dev->total_phyports);
+
 	/* Read the app ID of the firmware loaded */
 	snprintf(app_name, sizeof(app_name), "_pf%u_net_app_id", function_id);
 	app_fw_id = nfp_rtsym_read_le(sym_tbl, app_name, &ret);
diff --git a/drivers/net/nfp/nfp_net_common.c b/drivers/net/nfp/nfp_net_common.c
index 99f6b61947..86a1fbfaf2 100644
--- a/drivers/net/nfp/nfp_net_common.c
+++ b/drivers/net/nfp/nfp_net_common.c
@@ -14,6 +14,7 @@ 
 #include "nfdk/nfp_nfdk.h"
 #include "nfpcore/nfp_mip.h"
 #include "nfpcore/nfp_nsp.h"
+#include "nfpcore/nfp_rtsym.h"
 #include "nfp_logs.h"
 #include "nfp_net_meta.h"
 
@@ -2724,7 +2725,7 @@  nfp_net_fec_set(struct rte_eth_dev *dev,
 }
 
 uint32_t
-nfp_net_get_port_num(struct nfp_pf_dev *pf_dev)
+nfp_net_get_phyports_from_nsp(struct nfp_pf_dev *pf_dev)
 {
 	if (pf_dev->multi_pf.enabled)
 		return 1;
@@ -2732,6 +2733,25 @@  nfp_net_get_port_num(struct nfp_pf_dev *pf_dev)
 		return pf_dev->nfp_eth_table->count;
 }
 
+uint32_t
+nfp_net_get_phyports_from_fw(struct nfp_pf_dev *pf_dev)
+{
+	int ret = 0;
+	uint8_t total_phyports;
+	char pf_name[RTE_ETH_NAME_MAX_LEN];
+
+	/* Read the number of vNIC's created for the PF */
+	snprintf(pf_name, sizeof(pf_name), "nfd_cfg_pf%u_num_ports",
+			pf_dev->multi_pf.function_id);
+	total_phyports = nfp_rtsym_read_le(pf_dev->sym_tbl, pf_name, &ret);
+	if (ret != 0 || total_phyports == 0 || total_phyports > 8) {
+		PMD_INIT_LOG(ERR, "%s symbol with wrong value", pf_name);
+		return 0;
+	}
+
+	return total_phyports;
+}
+
 uint8_t
 nfp_function_id_get(const struct nfp_pf_dev *pf_dev,
 		uint8_t port_id)
diff --git a/drivers/net/nfp/nfp_net_common.h b/drivers/net/nfp/nfp_net_common.h
index fb244383b7..8429db68f0 100644
--- a/drivers/net/nfp/nfp_net_common.h
+++ b/drivers/net/nfp/nfp_net_common.h
@@ -374,7 +374,8 @@  void nfp_net_get_fw_version(struct nfp_cpp *cpp,
 		uint32_t *fw_version);
 int nfp_net_txrwb_alloc(struct rte_eth_dev *eth_dev);
 void nfp_net_txrwb_free(struct rte_eth_dev *eth_dev);
-uint32_t nfp_net_get_port_num(struct nfp_pf_dev *pf_dev);
+uint32_t nfp_net_get_phyports_from_nsp(struct nfp_pf_dev *pf_dev);
+uint32_t nfp_net_get_phyports_from_fw(struct nfp_pf_dev *pf_dev);
 uint8_t nfp_function_id_get(const struct nfp_pf_dev *pf_dev,
 		uint8_t port_id);
 int nfp_net_vf_config_app_init(struct nfp_net_hw *net_hw,