From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 78D02A046B for ; Wed, 26 Jun 2019 10:51:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3EA4B2BEA; Wed, 26 Jun 2019 10:51:32 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 29834A3 for ; Wed, 26 Jun 2019 10:51:29 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jun 2019 01:51:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,419,1557212400"; d="scan'208";a="360239345" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.110.186]) by fmsmga005.fm.intel.com with ESMTP; 26 Jun 2019 01:51:25 -0700 Date: Wed, 26 Jun 2019 23:33:05 +0800 From: Ye Xiaolong To: Andy Pei Cc: dev@dpdk.org, roy.fan.zhang@intel.com, qi.z.zhang@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com, ferruh.yigit@intel.com, rosen.xu@intel.com Message-ID: <20190626153305.GB99403@intel.com> References: <1558602875-429451-1-git-send-email-andy.pei@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1558602875-429451-1-git-send-email-andy.pei@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH] net/i40e: i40e rework for ipn3ke X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Andy Better to use a more specific subject for this patch: net/i40e: get link status update for ipn3ke or something similar. On 05/23, Andy Pei wrote: >Add switch_mode argument for i40e PF to specify the >specific FPGA that i40e PF is connected to. >i40e PF get link status update via the connected >FPGA. > >Fixes: c60869e2b742 ("net/i40e: fix link status update") For me, this patch is rather a new feature add than a fix. >Cc: roy.fan.zhang@intel.com >Cc: qi.z.zhang@intel.com >Cc: jingjing.wu@intel.com >Cc: beilei.xing@intel.com >Cc: ferruh.yigit@intel.com >Cc: rosen.xu@intel.com As Ferruh suggested, currently we don't need this cc info in git log history, you can move the cc block after `---` mark, while all those names will still be picked-up and cc'ed by git send-email. > >Signed-off-by: Andy Pei >--- > drivers/net/i40e/i40e_ethdev.c | 128 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 122 insertions(+), 6 deletions(-) > >diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c >index cab440f..9873ea0 100644 >--- a/drivers/net/i40e/i40e_ethdev.c >+++ b/drivers/net/i40e/i40e_ethdev.c >@@ -39,11 +39,12 @@ > #include "i40e_regs.h" > #include "rte_pmd_i40e.h" > >-#define ETH_I40E_FLOATING_VEB_ARG "enable_floating_veb" >-#define ETH_I40E_FLOATING_VEB_LIST_ARG "floating_veb_list" >-#define ETH_I40E_SUPPORT_MULTI_DRIVER "support-multi-driver" >-#define ETH_I40E_QUEUE_NUM_PER_VF_ARG "queue-num-per-vf" >-#define ETH_I40E_USE_LATEST_VEC "use-latest-supported-vec" >+#define ETH_I40E_FLOATING_VEB_ARG "enable_floating_veb" >+#define ETH_I40E_FLOATING_VEB_LIST_ARG "floating_veb_list" >+#define ETH_I40E_SUPPORT_MULTI_DRIVER "support-multi-driver" >+#define ETH_I40E_QUEUE_NUM_PER_VF_ARG "queue-num-per-vf" >+#define ETH_I40E_USE_LATEST_VEC "use-latest-supported-vec" Above changes are not relevant. >+#define ETH_I40E_SWITCH_MODE_ARG "switch_mode" > > #define I40E_CLEAR_PXE_WAIT_MS 200 > >@@ -410,6 +411,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf, > ETH_I40E_SUPPORT_MULTI_DRIVER, > ETH_I40E_QUEUE_NUM_PER_VF_ARG, > ETH_I40E_USE_LATEST_VEC, >+ ETH_I40E_SWITCH_MODE_ARG, > NULL}; > > static const struct rte_pci_id pci_id_i40e_map[] = { >@@ -2784,6 +2786,80 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw) > } > } > >+static int >+i40e_pf_parse_switch_mode(const char *key __rte_unused, >+ const char *value, void *extra_args) >+{ >+ if (!value || !extra_args) >+ return -EINVAL; >+ >+ *(char **)extra_args = strdup(value); >+ When will you free the memory alloced by strdup. >+ if (!*(char **)extra_args) >+ return -ENOMEM; >+ >+ return 0; >+} >+ >+static void >+i40e_pf_switch_mode_link_update(const char *cfg_str, >+ struct rte_eth_dev **switch_ethdev) >+{ >+ char switch_name[RTE_ETH_NAME_MAX_LEN] = {0}; >+ char port_name[RTE_ETH_NAME_MAX_LEN] = {0}; >+ char switch_ethdev_name[RTE_ETH_NAME_MAX_LEN] = {0}; Above initializations are not needed. >+ uint16_t port_id; >+ const char *p_src; >+ char *p_dst; >+ int ret = -1; This initialization is not needed. >+ >+ /* An example of cfg_str is "IPN3KE_0@b3:00.0_0" */ >+ if (!strncmp(cfg_str, "IPN3KE", strlen("IPN3KE"))) { >+ p_src = cfg_str; >+ PMD_DRV_LOG(DEBUG, "cfg_str is %s", cfg_str); >+ >+ /* move over "IPN3KE" */ >+ while ((*p_src != '_') && (*p_src)) >+ p_src++; >+ >+ /* move over the first underline */ >+ p_src++; >+ >+ p_dst = switch_name; >+ while ((*p_src != '_') && (*p_src)) { >+ if (*p_src == '@') { >+ *p_dst++ = '|'; >+ p_src++; >+ } else >+ *p_dst++ = *p_src++; >+ } >+ *p_dst = 0; >+ PMD_DRV_LOG(DEBUG, "switch_name is %s", switch_name); >+ >+ /* move over the second underline */ >+ p_src++; >+ >+ p_dst = port_name; >+ while (*p_src) >+ *p_dst++ = *p_src++; >+ *p_dst = 0; >+ PMD_DRV_LOG(DEBUG, "port_name is %s", port_name); >+ >+ snprintf(switch_ethdev_name, sizeof(switch_ethdev_name), >+ "net_%s_representor_%s", switch_name, port_name); >+ PMD_DRV_LOG(DEBUG, "switch_ethdev_name is %s", >+ switch_ethdev_name); >+ >+ ret = rte_eth_dev_get_port_by_name(switch_ethdev_name, >+ &port_id); >+ if (ret) >+ *switch_ethdev = NULL; >+ else >+ *switch_ethdev = &rte_eth_devices[port_id]; >+ } else >+ *switch_ethdev = NULL; After read the body of this function, it's about getting a switch_ethdev by a name other than updating the link status, so the func name is confusing, better to use a more precise name, and return value can be struct *rte_eth_dev, then you wouldn't have to pass struct rte_eth_dev **. >+} >+ > int > i40e_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) >@@ -2792,6 +2868,11 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw) > struct rte_eth_link link; > bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false; > int ret; >+ struct rte_devargs *devargs; >+ struct rte_kvargs *kvlist = NULL; >+ struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); >+ struct rte_eth_dev *switch_ethdev; >+ char *switch_cfg_str = NULL; > > memset(&link, 0, sizeof(link)); > >@@ -2805,6 +2886,40 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw) > else > update_link_aq(hw, &link, enable_lse, wait_to_complete); > >+ devargs = pci_dev->device.devargs; >+ if (devargs) { >+ kvlist = rte_kvargs_parse(devargs->args, valid_keys); >+ if (kvlist != NULL) { >+ if (rte_kvargs_count(kvlist, ETH_I40E_SWITCH_MODE_ARG) >+ == 1) { >+ if (!rte_kvargs_process(kvlist, >+ ETH_I40E_SWITCH_MODE_ARG, >+ &i40e_pf_parse_switch_mode, >+ &switch_cfg_str)) { >+ >+ i40e_pf_switch_mode_link_update( >+ switch_cfg_str, >+ &switch_ethdev); >+ >+ if (switch_ethdev) { >+ rte_eth_linkstatus_get( >+ switch_ethdev, >+ &link); >+ } else { >+ link.link_duplex = >+ ETH_LINK_FULL_DUPLEX; >+ link.link_autoneg = >+ ETH_LINK_SPEED_FIXED; >+ link.link_speed = >+ ETH_SPEED_NUM_25G; >+ link.link_status = 0; >+ } >+ } >+ } >+ rte_kvargs_free(kvlist); >+ } >+ } >+ Better to wrap above code to a function to avoid too many levels of block nesting. Thanks, Xiaolong > ret = rte_eth_linkstatus_set(dev, &link); > i40e_notify_all_vfs_link_status(dev); > >@@ -12790,4 +12905,5 @@ struct i40e_customized_pctype* > ETH_I40E_FLOATING_VEB_LIST_ARG "=" > ETH_I40E_QUEUE_NUM_PER_VF_ARG "=1|2|4|8|16" > ETH_I40E_SUPPORT_MULTI_DRIVER "=1" >- ETH_I40E_USE_LATEST_VEC "=0|1"); >+ ETH_I40E_USE_LATEST_VEC "=0|1" >+ ETH_I40E_SWITCH_MODE_ARG "=IPN3KE"); >-- >1.8.3.1 >