diff mbox series

[v7,03/12] net/nfp: move app specific init logic to own function

Message ID 1660299750-10668-4-git-send-email-chaoyong.he@corigine.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series preparation for the rte_flow offload of nfp PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chaoyong He Aug. 12, 2022, 10:22 a.m. UTC 
  The NFP card can load different firmware applications.
This commit move the init logic of corenic app of the
secondary process into its own function.

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/nfp/nfp_ethdev.c | 90 +++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 30 deletions(-)

Comments

Ferruh Yigit Sept. 5, 2022, 3:38 p.m. UTC | #1 
On 8/12/2022 11:22 AM, Chaoyong He wrote:
> The NFP card can load different firmware applications.
> This commit move the init logic of corenic app of the
> secondary process into its own function.
> 
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>

<...>

> +	switch (app_id) {
> +	case NFP_APP_CORE_NIC:
> +		PMD_INIT_LOG(INFO, "Initializing coreNIC");
> +		ret = nfp_secondary_init_app_nic(pci_dev, sym_tbl, cpp);
> +		if (ret != 0) {
> +			PMD_INIT_LOG(ERR, "Could not initialize coreNIC!");
> +			goto sym_tbl_cleanup;
>   		}

If you are planning to add more FW app support, what do you think to add 
another abstraction for it? Something like

struct fw_ops {
	*init()
	*secondary_init()
	...
}

	...
	ret = fw_ops[app_id].secondary_init(...);
	...
Chaoyong He Sept. 6, 2022, 8:29 a.m. UTC | #2 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Monday, September 5, 2022 11:39 PM
> To: Chaoyong He <chaoyong.he@corigine.com>; dev@dpdk.org
> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
> <niklas.soderlund@corigine.com>
> Subject: Re: [PATCH v7 03/12] net/nfp: move app specific init logic to own
> function
> 
> On 8/12/2022 11:22 AM, Chaoyong He wrote:
> > The NFP card can load different firmware applications.
> > This commit move the init logic of corenic app of the secondary
> > process into its own function.
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> <...>
> 
> > +	switch (app_id) {
> > +	case NFP_APP_CORE_NIC:
> > +		PMD_INIT_LOG(INFO, "Initializing coreNIC");
> > +		ret = nfp_secondary_init_app_nic(pci_dev, sym_tbl, cpp);
> > +		if (ret != 0) {
> > +			PMD_INIT_LOG(ERR, "Could not initialize coreNIC!");
> > +			goto sym_tbl_cleanup;
> >   		}
> 
> If you are planning to add more FW app support, what do you think to add
> another abstraction for it? Something like
> 
> struct fw_ops {
> 	*init()
> 	*secondary_init()
> 	...
> }
> 
> 	...
> 	ret = fw_ops[app_id].secondary_init(...);
> 	...
> 

It does make sense if we can translate this switch statement into an array of function pointers.
But there are some problems:
1. The `app_id` is returned by the firmware, so we can't simply regard it as an index of the array.
     There should import some relation map logic. And we can't find a suitable upper limit for the
     array, so maybe we will always update it as we add more and more firmware apps in the future. 
     We also have to check the value of `fw_ops[app_id].secondary_init` before we invoke it.
2. Different firmware app may need different variables to initialize, which make it difficult to find a
     suitable function prototype when we declare the function pointer.

So the final logics seems will more complicated and maybe we can keep use the logics now?
Ferruh Yigit Sept. 6, 2022, 9:47 a.m. UTC | #3 
On 9/6/2022 9:29 AM, Chaoyong He wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
>> Sent: Monday, September 5, 2022 11:39 PM
>> To: Chaoyong He <chaoyong.he@corigine.com>; dev@dpdk.org
>> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
>> <niklas.soderlund@corigine.com>
>> Subject: Re: [PATCH v7 03/12] net/nfp: move app specific init logic to own
>> function
>>
>> On 8/12/2022 11:22 AM, Chaoyong He wrote:
>>> The NFP card can load different firmware applications.
>>> This commit move the init logic of corenic app of the secondary
>>> process into its own function.
>>>
>>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>
>> <...>
>>
>>> +   switch (app_id) {
>>> +   case NFP_APP_CORE_NIC:
>>> +           PMD_INIT_LOG(INFO, "Initializing coreNIC");
>>> +           ret = nfp_secondary_init_app_nic(pci_dev, sym_tbl, cpp);
>>> +           if (ret != 0) {
>>> +                   PMD_INIT_LOG(ERR, "Could not initialize coreNIC!");
>>> +                   goto sym_tbl_cleanup;
>>>              }
>>
>> If you are planning to add more FW app support, what do you think to add
>> another abstraction for it? Something like
>>
>> struct fw_ops {
>>        *init()
>>        *secondary_init()
>>        ...
>> }
>>
>>        ...
>>        ret = fw_ops[app_id].secondary_init(...);
>>        ...
>>
> 
> It does make sense if we can translate this switch statement into an array of function pointers.
> But there are some problems:
> 1. The `app_id` is returned by the firmware, so we can't simply regard it as an index of the array.
>       There should import some relation map logic. And we can't find a suitable upper limit for the
>       array, so maybe we will always update it as we add more and more firmware apps in the future.
>       We also have to check the value of `fw_ops[app_id].secondary_init` before we invoke it.
> 2. Different firmware app may need different variables to initialize, which make it difficult to find a
>       suitable function prototype when we declare the function pointer.
> 
> So the final logics seems will more complicated and maybe we can keep use the logics now?

Got it, above mentioned issues looks valid, so it is OK to keep as it is.
diff mbox series

Patch

diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index 088140a..a6a7e7e 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -992,6 +992,49 @@ 
 	return ret;
 }
 
+static int
+nfp_secondary_init_app_nic(struct rte_pci_device *pci_dev,
+		struct nfp_rtsym_table *sym_tbl,
+		struct nfp_cpp *cpp)
+{
+	int i;
+	int err = 0;
+	int ret = 0;
+	int total_vnics;
+	struct nfp_net_hw *hw;
+
+	/* Read the number of vNIC's created for the PF */
+	total_vnics = nfp_rtsym_read_le(sym_tbl, "nfd_cfg_pf0_num_ports", &err);
+	if (err != 0 || total_vnics <= 0 || total_vnics > 8) {
+		PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value");
+		return -ENODEV;
+	}
+
+	for (i = 0; i < total_vnics; i++) {
+		struct rte_eth_dev *eth_dev;
+		char port_name[RTE_ETH_NAME_MAX_LEN];
+		snprintf(port_name, sizeof(port_name), "%s_port%d",
+				pci_dev->device.name, i);
+
+		PMD_INIT_LOG(DEBUG, "Secondary attaching to port %s", port_name);
+		eth_dev = rte_eth_dev_attach_secondary(port_name);
+		if (eth_dev == NULL) {
+			PMD_INIT_LOG(ERR, "Secondary process attach to port %s failed", port_name);
+			ret = -ENODEV;
+			break;
+		}
+
+		eth_dev->process_private = cpp;
+		hw = NFP_NET_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
+		if (nfp_net_ethdev_ops_mount(hw, eth_dev))
+			return -EINVAL;
+
+		rte_eth_dev_probing_finish(eth_dev);
+	}
+
+	return ret;
+}
+
 /*
  * When attaching to the NFP4000/6000 PF on a secondary process there
  * is no need to initialise the PF again. Only minimal work is required
@@ -1000,12 +1043,10 @@ 
 static int
 nfp_pf_secondary_init(struct rte_pci_device *pci_dev)
 {
-	int i;
 	int err = 0;
 	int ret = 0;
-	int total_ports;
 	struct nfp_cpp *cpp;
-	struct nfp_net_hw *hw;
+	enum nfp_app_id app_id;
 	struct nfp_rtsym_table *sym_tbl;
 
 	if (pci_dev == NULL)
@@ -1039,37 +1080,26 @@ 
 		return -EIO;
 	}
 
-	total_ports = nfp_rtsym_read_le(sym_tbl, "nfd_cfg_pf0_num_ports", &err);
-	if (err != 0 || total_ports <= 0 || total_ports > 8) {
-		PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value");
-		ret = -ENODEV;
+	/* Read the app ID of the firmware loaded */
+	app_id = nfp_rtsym_read_le(sym_tbl, "_pf0_net_app_id", &err);
+	if (err != 0) {
+		PMD_INIT_LOG(ERR, "Couldn't read app_id from fw");
 		goto sym_tbl_cleanup;
 	}
 
-	for (i = 0; i < total_ports; i++) {
-		struct rte_eth_dev *eth_dev;
-		char port_name[RTE_ETH_NAME_MAX_LEN];
-
-		snprintf(port_name, sizeof(port_name), "%s_port%d",
-			 pci_dev->device.name, i);
-
-		PMD_DRV_LOG(DEBUG, "Secondary attaching to port %s", port_name);
-		eth_dev = rte_eth_dev_attach_secondary(port_name);
-		if (eth_dev == NULL) {
-			RTE_LOG(ERR, EAL,
-				"secondary process attach failed, ethdev doesn't exist");
-			ret = -ENODEV;
-			break;
+	switch (app_id) {
+	case NFP_APP_CORE_NIC:
+		PMD_INIT_LOG(INFO, "Initializing coreNIC");
+		ret = nfp_secondary_init_app_nic(pci_dev, sym_tbl, cpp);
+		if (ret != 0) {
+			PMD_INIT_LOG(ERR, "Could not initialize coreNIC!");
+			goto sym_tbl_cleanup;
 		}
-
-		hw = NFP_NET_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
-
-		if (nfp_net_ethdev_ops_mount(hw, eth_dev))
-			return -EINVAL;
-
-		eth_dev->process_private = cpp;
-
-		rte_eth_dev_probing_finish(eth_dev);
+		break;
+	default:
+		PMD_INIT_LOG(ERR, "Unsupported Firmware loaded");
+		ret = -EINVAL;
+		goto sym_tbl_cleanup;
 	}
 
 	if (ret != 0)