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.yi...@xilinx.com>
Sent: Monday, September 5, 2022 11:39 PM
To: Chaoyong He <chaoyong...@corigine.com>; dev@dpdk.org
Cc: oss-drivers <oss-driv...@corigine.com>; Niklas Soderlund
<niklas.soderl...@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...@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderl...@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.
