From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id F073F1B44A for ; Thu, 4 Apr 2019 21:02:32 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Apr 2019 12:02:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,309,1549958400"; d="scan'208";a="220629171" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.46]) ([10.237.221.46]) by orsmga001.jf.intel.com with ESMTP; 04 Apr 2019 12:02:26 -0700 To: Rosen Xu , dev@dpdk.org Cc: tianfei.zhang@intel.com, dan.wei@intel.com, andy.pei@intel.com, qiming.yang@intel.com, haiyue.wang@intel.com, santos.chen@intel.com, zhang.zhang@intel.com, david.lomartire@intel.com References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1554292065-186702-1-git-send-email-rosen.xu@intel.com> <1554292065-186702-5-git-send-email-rosen.xu@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJVBBMBAgA/AhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgBYhBNI2U4dCLsKE45mBx/kz60PfE2EfBQJbughWBQkHwjOGAAoJEPkz60Pf E2Eft84QAIbKWqhgqRfoiw/BbXbA1+qm2o4UgkCRQ0yJgt9QsnbpOmPKydHH0ixCliNz1J8e mRXCkMini1bTpnzp7spOjQGLeAFkNFz6BMq8YF2mVWbGEDE9WgnAxZdi0eLY7ZQnHbE6AxKL SXmpe9INb6z3ztseFt7mqje/W/6DWYIMnH3Yz9KzxujFWDcq8UCAvPkxVQXLTMpauhFgYeEx Nub5HbvhxTfUkapLwRQsSd/HbywzqZ3s/bbYMjj5JO3tgMiM9g9HOjv1G2f1dQjHi5YQiTZl 1eIIqQ3pTic6ROaiZqNmQFXPsoOOFfXF8nN2zg8kl/sSdoXWHhama5hbwwtl1vdaygQYlmdK H2ueiFh/UvT3WG3waNv2eZiEbHV8Rk52Xyn2w1G90lV0fYC6Ket1Xjoch7kjwbx793Kz/RfQ rmBY8/S4DTGn3oq3dMdQY+b6+7VMUeLMMh2CXYO9ErkOq+qNTD1IY+cBAkXnaDbQfz0zbste ZGWH74FAZ9nCpDOqbRTrBL42aMGhfOWEyeA1x7+hl6JZfabBWAuf4nnCXuorKHzBXTrf7u7p fXsKQClWRW77PF1VmzrtKNVSytQAmlCWApQIw20AarFipXmVdIjHmJPU611WoyxZPb4JTOxx 5cv9B+nr/RIB+v5dcStyHCCwO1be7nBDdCgd4F6kTQPLuQINBFfWTL4BEACnNA29e8TarUsB L5n6eLZHXcFvVwNLVlirWOClHXf44o2KnN3ww+eBEmKVfEFo9MSuGDNHS8Zw1NiGMYxLIUgd U6gGrVVs/VrQWL82pbMk6jCj98N+BXIri+6K1z+AImz7ax7iF1kDgRAnFWU0znWWBgM2mM8Y gDjcxfXk4sCKnvf6Gjo08Ey5zmqx7dekAKU2EEp8Q1EJY3jbymLdZWRP4AFFMTS1rGMk0/tt v71NBg1GobCcbNfn9chK/jhqxYhAJqq86RdJQkt3/9x1U1Oq0vXCt4JVVHmkxePtUiuWTTt+ aYlUAsKYZsWvncExvw77x2ArYDmaK0yfjh37wp0lY7DOJHFxoyT8tyWZlLci/VMRG2Ja33xj 0CN4C1yBg+QDeV3QFxQo42iA/ykdXPUR3ezmsND3XKvVLTC4DNb3V/EZQ7jBj64+bEK0VW4G B31VP00ApNQvSoczsIOAKdk97RNbpmPw6q10ILIB+9T1xbnFYzshzGF17oC0/GENIHATx8vZ masOZoDiOZQpeneLgnFE9JfzhLTxv6wNZcc/HLXRQVTkDsQr8ERtkAoHCf1E5+b5Yr7pfnE4 YuhET746o25S53ELUYPIs49qoJsEJL34/oexMfPGyPIlrbufiNyty5jc/1MRwUlhJlJ5IOHy ZUa+6CLR7GdImusFkPJUJwARAQABiQI8BBgBAgAmAhsMFiEE0jZTh0IuwoTjmYHH+TPrQ98T YR8FAlu6CHAFCQXE7zIACgkQ+TPrQ98TYR9nXxAAqNBgkYNyGuWUuy0GwDQCbu3iiMyH1+D7 llafPcK4NYy1Z4AYuVwC9nmLaoj+ozdqS3ncRo57ncRsKEJC46nDJJZYZ5LSJVn63Y3NBF86 lxQAgjj2oyZEwaLKtKbAFsXL43jv1pUGgSvWwYtDwHITXXFQto9rZEuUDRFSx4sg9OR+Q6/6 LY+nQQ3OdHlBkflzYMPcWgDcvcTAO6yasLEUf7UcYoSWTyMYjLB4QuNlXzTswzGVMssJF/vo V8lD1eqqaSUWG3STF6GVLQOr1NLvN5+kUBiEStHFxBpgSCvYY9sNV8FS6N24CAWMBl+10W+D 2h1yiiP5dOdPcBDYKsgqDD91/sP0WdyMJkwdQJtD49f9f+lYloxHnSAxMleOpyscg1pldw+i mPaUY1bmIknLhhkqfMmjywQOXpac5LRMibAAYkcB8v7y3kwELnt8mhqqZy6LUsqcWygNbH/W K3GGt5tRpeIXeJ25x8gg5EBQ0Jnvp/IbBYQfPLtXH0Myq2QuAhk/1q2yEIbVjS+7iowEZNyE 56K63WBJxsJPB2mvmLgn98GqB4G6GufP1ndS0XDti/2K0o8rep9xoY/JDGi0n0L0tk9BHyoP Y7kaEpu7UyY3nVdRLe5H1/MnFG8hdJ97WqnPS0buYZlrbTV0nRFL/NI2VABl18vEEXvNQiO+ vM8= Message-ID: <70cbe3d6-10da-7179-9f0b-556c7c3e3134@intel.com> Date: Thu, 4 Apr 2019 20:02:25 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <1554292065-186702-5-git-send-email-rosen.xu@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5 04/14] drivers/net/ipn3ke: add IPN3KE representor of PMD driver 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: , X-List-Received-Date: Thu, 04 Apr 2019 19:02:33 -0000 On 4/3/2019 12:47 PM, Rosen Xu wrote: > Add Intel FPGA Acceleration NIC IPN3KE representor of PMD driver. > > Signed-off-by: Rosen Xu > Signed-off-by: Andy Pei > Signed-off-by: Dan Wei <...> > +static void > +ipn3ke_rpst_dev_stop(__rte_unused struct rte_eth_dev *dev) > +{ 'dev' is used in this function, can drop '__rte_unused' > + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev); > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev); > + uint32_t val; > + > + if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) { > + } else { Why having empty 'if' condition and put everything into 'else', why not revert the check in if? Similar usage in many places. > + /* Disable the TX path */ Fix indentation please. > + val = 1; I assume this is the value to write to say disable Tx path, I wonder if there is a define MACRO for it? If not does it make sense to have one? <...> > +/* > + * Reset PF device only to re-initialize resources in PMD layer > + */ > +static int > +ipn3ke_rpst_dev_reset(struct rte_eth_dev *dev) > +{ > + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev); > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev); > + uint32_t val; > + > + if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) { > + } else { > + /* Disable the TX path */ > + val = 1; > + val &= IPN3KE_MAC_TX_PACKET_CONTROL_MASK; > + (*hw->f_mac_write)(hw, > + val, > + IPN3KE_MAC_TX_PACKET_CONTROL, > + rpst->port_id, > + 0); > + > + /* Disable the RX path */ > + val = 1; > + val &= IPN3KE_MAC_RX_TRANSFER_CONTROL_MASK; > + (*hw->f_mac_write)(hw, > + val, > + IPN3KE_MAC_RX_TRANSFER_CONTROL, > + rpst->port_id, > + 0); > + } Same exact block repeated third time now, and there is one more copy as 'val = 0', what do you think converting this into function which gets enable/disable as argument so that 'val' also can be detail of that function. <...> > +static int > +ipn3ke_rpst_xstats_get(__rte_unused struct rte_eth_dev *dev, > + __rte_unused struct rte_eth_xstat *xstats, > + __rte_unused unsigned int n) > +{ > + return 0; > +} > +static int ipn3ke_rpst_xstats_get_names(__rte_unused struct rte_eth_dev *dev, > + __rte_unused struct rte_eth_xstat_name *xstats_names, > + __rte_unused unsigned int limit) > +{ > + return 0; > +} Can you please apply same syntax to this function as others: static int .... Also can you please put a empty line between previous and this function. <...> > +static int > +ipn3ke_rpst_link_check(struct ipn3ke_rpst *rpst) > +{ > + struct ipn3ke_hw *hw; > + struct rte_rawdev *rawdev; > + struct rte_eth_link link; > + struct rte_eth_dev *pf; > + > + if (rpst == NULL) > + return -1; > + > + hw = rpst->hw; > + > + memset(&link, 0, sizeof(link)); > + > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > + link.link_autoneg = !(rpst->ethdev->data->dev_conf.link_speeds & > + ETH_LINK_SPEED_FIXED); > + > + rawdev = hw->rawdev; > + ipn3ke_update_link(rawdev, rpst->port_id, &link); > + > + if (!rpst->ori_linfo.link_status && > + link.link_status) { Better to put one more tab here, to make it easy to pick the condition part from the indendent code. <...> > + > +struct eth_dev_ops ipn3ke_rpst_dev_ops = { This can be "static const" > + .dev_infos_get = ipn3ke_rpst_dev_infos_get, > + > + .dev_configure = ipn3ke_rpst_dev_configure, > + .dev_start = ipn3ke_rpst_dev_start, > + .dev_stop = ipn3ke_rpst_dev_stop, > + .dev_close = ipn3ke_rpst_dev_close, > + .dev_reset = ipn3ke_rpst_dev_reset, > + > + .stats_get = ipn3ke_rpst_stats_get, > + .xstats_get = ipn3ke_rpst_xstats_get, > + .xstats_get_names = ipn3ke_rpst_xstats_get_names, > + .stats_reset = ipn3ke_rpst_stats_reset, > + .xstats_reset = ipn3ke_rpst_stats_reset, > + > + .filter_ctrl = ipn3ke_afu_filter_ctrl, > + > + .rx_queue_start = ipn3ke_rpst_rx_queue_start, > + .rx_queue_stop = ipn3ke_rpst_rx_queue_stop, > + .tx_queue_start = ipn3ke_rpst_tx_queue_start, > + .tx_queue_stop = ipn3ke_rpst_tx_queue_stop, > + .rx_queue_setup = ipn3ke_rpst_rx_queue_setup, > + .rx_queue_release = ipn3ke_rpst_rx_queue_release, > + .tx_queue_setup = ipn3ke_rpst_tx_queue_setup, > + .tx_queue_release = ipn3ke_rpst_tx_queue_release, > + > + .dev_set_link_up = ipn3ke_rpst_dev_set_link_up, > + .dev_set_link_down = ipn3ke_rpst_dev_set_link_down, > + .link_update = ipn3ke_rpst_link_update, > + > + .promiscuous_enable = ipn3ke_rpst_promiscuous_enable, > + .promiscuous_disable = ipn3ke_rpst_promiscuous_disable, > + .allmulticast_enable = ipn3ke_rpst_allmulticast_enable, > + .allmulticast_disable = ipn3ke_rpst_allmulticast_disable, > + .mac_addr_set = ipn3ke_rpst_mac_addr_set, > + .mtu_set = ipn3ke_rpst_mtu_set, > + > + .tm_ops_get = NULL, No need to set NULL ones. <...> > +int > +ipn3ke_rpst_init(struct rte_eth_dev *ethdev, void *init_params) > +{ > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(ethdev); > + struct ipn3ke_rpst *representor_param = > + (struct ipn3ke_rpst *)init_params; > + > + if (representor_param->port_id >= representor_param->hw->port_num) > + return -ENODEV; > + > + rpst->ethdev = ethdev; > + rpst->switch_domain_id = representor_param->switch_domain_id; > + rpst->port_id = representor_param->port_id; > + rpst->hw = representor_param->hw; > + rpst->i40e_pf_eth = NULL; > + rpst->i40e_pf_eth_port_id = 0xFFFF; > + > + ethdev->data->mac_addrs = rte_zmalloc("ipn3ke", ETHER_ADDR_LEN, 0); > + if (!ethdev->data->mac_addrs) { > + IPN3KE_AFU_PMD_ERR("Failed to " > + "allocated memory for storing mac address"); > + return -ENODEV; > + } > + > + /** representor shares the same driver as it's PF device */ > + /** > + * ethdev->device->driver = rpst->hw->eth_dev->device->driver; > + */ Please remove commented code. 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 2649EA0679 for ; Thu, 4 Apr 2019 21:02:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 543321B457; Thu, 4 Apr 2019 21:02:35 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id F073F1B44A for ; Thu, 4 Apr 2019 21:02:32 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Apr 2019 12:02:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,309,1549958400"; d="scan'208";a="220629171" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.46]) ([10.237.221.46]) by orsmga001.jf.intel.com with ESMTP; 04 Apr 2019 12:02:26 -0700 To: Rosen Xu , dev@dpdk.org Cc: tianfei.zhang@intel.com, dan.wei@intel.com, andy.pei@intel.com, qiming.yang@intel.com, haiyue.wang@intel.com, santos.chen@intel.com, zhang.zhang@intel.com, david.lomartire@intel.com References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1554292065-186702-1-git-send-email-rosen.xu@intel.com> <1554292065-186702-5-git-send-email-rosen.xu@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJVBBMBAgA/AhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgBYhBNI2U4dCLsKE45mBx/kz60PfE2EfBQJbughWBQkHwjOGAAoJEPkz60Pf E2Eft84QAIbKWqhgqRfoiw/BbXbA1+qm2o4UgkCRQ0yJgt9QsnbpOmPKydHH0ixCliNz1J8e mRXCkMini1bTpnzp7spOjQGLeAFkNFz6BMq8YF2mVWbGEDE9WgnAxZdi0eLY7ZQnHbE6AxKL SXmpe9INb6z3ztseFt7mqje/W/6DWYIMnH3Yz9KzxujFWDcq8UCAvPkxVQXLTMpauhFgYeEx Nub5HbvhxTfUkapLwRQsSd/HbywzqZ3s/bbYMjj5JO3tgMiM9g9HOjv1G2f1dQjHi5YQiTZl 1eIIqQ3pTic6ROaiZqNmQFXPsoOOFfXF8nN2zg8kl/sSdoXWHhama5hbwwtl1vdaygQYlmdK H2ueiFh/UvT3WG3waNv2eZiEbHV8Rk52Xyn2w1G90lV0fYC6Ket1Xjoch7kjwbx793Kz/RfQ rmBY8/S4DTGn3oq3dMdQY+b6+7VMUeLMMh2CXYO9ErkOq+qNTD1IY+cBAkXnaDbQfz0zbste ZGWH74FAZ9nCpDOqbRTrBL42aMGhfOWEyeA1x7+hl6JZfabBWAuf4nnCXuorKHzBXTrf7u7p fXsKQClWRW77PF1VmzrtKNVSytQAmlCWApQIw20AarFipXmVdIjHmJPU611WoyxZPb4JTOxx 5cv9B+nr/RIB+v5dcStyHCCwO1be7nBDdCgd4F6kTQPLuQINBFfWTL4BEACnNA29e8TarUsB L5n6eLZHXcFvVwNLVlirWOClHXf44o2KnN3ww+eBEmKVfEFo9MSuGDNHS8Zw1NiGMYxLIUgd U6gGrVVs/VrQWL82pbMk6jCj98N+BXIri+6K1z+AImz7ax7iF1kDgRAnFWU0znWWBgM2mM8Y gDjcxfXk4sCKnvf6Gjo08Ey5zmqx7dekAKU2EEp8Q1EJY3jbymLdZWRP4AFFMTS1rGMk0/tt v71NBg1GobCcbNfn9chK/jhqxYhAJqq86RdJQkt3/9x1U1Oq0vXCt4JVVHmkxePtUiuWTTt+ aYlUAsKYZsWvncExvw77x2ArYDmaK0yfjh37wp0lY7DOJHFxoyT8tyWZlLci/VMRG2Ja33xj 0CN4C1yBg+QDeV3QFxQo42iA/ykdXPUR3ezmsND3XKvVLTC4DNb3V/EZQ7jBj64+bEK0VW4G B31VP00ApNQvSoczsIOAKdk97RNbpmPw6q10ILIB+9T1xbnFYzshzGF17oC0/GENIHATx8vZ masOZoDiOZQpeneLgnFE9JfzhLTxv6wNZcc/HLXRQVTkDsQr8ERtkAoHCf1E5+b5Yr7pfnE4 YuhET746o25S53ELUYPIs49qoJsEJL34/oexMfPGyPIlrbufiNyty5jc/1MRwUlhJlJ5IOHy ZUa+6CLR7GdImusFkPJUJwARAQABiQI8BBgBAgAmAhsMFiEE0jZTh0IuwoTjmYHH+TPrQ98T YR8FAlu6CHAFCQXE7zIACgkQ+TPrQ98TYR9nXxAAqNBgkYNyGuWUuy0GwDQCbu3iiMyH1+D7 llafPcK4NYy1Z4AYuVwC9nmLaoj+ozdqS3ncRo57ncRsKEJC46nDJJZYZ5LSJVn63Y3NBF86 lxQAgjj2oyZEwaLKtKbAFsXL43jv1pUGgSvWwYtDwHITXXFQto9rZEuUDRFSx4sg9OR+Q6/6 LY+nQQ3OdHlBkflzYMPcWgDcvcTAO6yasLEUf7UcYoSWTyMYjLB4QuNlXzTswzGVMssJF/vo V8lD1eqqaSUWG3STF6GVLQOr1NLvN5+kUBiEStHFxBpgSCvYY9sNV8FS6N24CAWMBl+10W+D 2h1yiiP5dOdPcBDYKsgqDD91/sP0WdyMJkwdQJtD49f9f+lYloxHnSAxMleOpyscg1pldw+i mPaUY1bmIknLhhkqfMmjywQOXpac5LRMibAAYkcB8v7y3kwELnt8mhqqZy6LUsqcWygNbH/W K3GGt5tRpeIXeJ25x8gg5EBQ0Jnvp/IbBYQfPLtXH0Myq2QuAhk/1q2yEIbVjS+7iowEZNyE 56K63WBJxsJPB2mvmLgn98GqB4G6GufP1ndS0XDti/2K0o8rep9xoY/JDGi0n0L0tk9BHyoP Y7kaEpu7UyY3nVdRLe5H1/MnFG8hdJ97WqnPS0buYZlrbTV0nRFL/NI2VABl18vEEXvNQiO+ vM8= Message-ID: <70cbe3d6-10da-7179-9f0b-556c7c3e3134@intel.com> Date: Thu, 4 Apr 2019 20:02:25 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <1554292065-186702-5-git-send-email-rosen.xu@intel.com> Content-Type: text/plain; charset="UTF-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5 04/14] drivers/net/ipn3ke: add IPN3KE representor of PMD driver 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" Message-ID: <20190404190225.32ZYRnEsScvYZKlA7UkN0ab-T4-80DctqiRWnxXN2oU@z> On 4/3/2019 12:47 PM, Rosen Xu wrote: > Add Intel FPGA Acceleration NIC IPN3KE representor of PMD driver. > > Signed-off-by: Rosen Xu > Signed-off-by: Andy Pei > Signed-off-by: Dan Wei <...> > +static void > +ipn3ke_rpst_dev_stop(__rte_unused struct rte_eth_dev *dev) > +{ 'dev' is used in this function, can drop '__rte_unused' > + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev); > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev); > + uint32_t val; > + > + if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) { > + } else { Why having empty 'if' condition and put everything into 'else', why not revert the check in if? Similar usage in many places. > + /* Disable the TX path */ Fix indentation please. > + val = 1; I assume this is the value to write to say disable Tx path, I wonder if there is a define MACRO for it? If not does it make sense to have one? <...> > +/* > + * Reset PF device only to re-initialize resources in PMD layer > + */ > +static int > +ipn3ke_rpst_dev_reset(struct rte_eth_dev *dev) > +{ > + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev); > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev); > + uint32_t val; > + > + if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) { > + } else { > + /* Disable the TX path */ > + val = 1; > + val &= IPN3KE_MAC_TX_PACKET_CONTROL_MASK; > + (*hw->f_mac_write)(hw, > + val, > + IPN3KE_MAC_TX_PACKET_CONTROL, > + rpst->port_id, > + 0); > + > + /* Disable the RX path */ > + val = 1; > + val &= IPN3KE_MAC_RX_TRANSFER_CONTROL_MASK; > + (*hw->f_mac_write)(hw, > + val, > + IPN3KE_MAC_RX_TRANSFER_CONTROL, > + rpst->port_id, > + 0); > + } Same exact block repeated third time now, and there is one more copy as 'val = 0', what do you think converting this into function which gets enable/disable as argument so that 'val' also can be detail of that function. <...> > +static int > +ipn3ke_rpst_xstats_get(__rte_unused struct rte_eth_dev *dev, > + __rte_unused struct rte_eth_xstat *xstats, > + __rte_unused unsigned int n) > +{ > + return 0; > +} > +static int ipn3ke_rpst_xstats_get_names(__rte_unused struct rte_eth_dev *dev, > + __rte_unused struct rte_eth_xstat_name *xstats_names, > + __rte_unused unsigned int limit) > +{ > + return 0; > +} Can you please apply same syntax to this function as others: static int .... Also can you please put a empty line between previous and this function. <...> > +static int > +ipn3ke_rpst_link_check(struct ipn3ke_rpst *rpst) > +{ > + struct ipn3ke_hw *hw; > + struct rte_rawdev *rawdev; > + struct rte_eth_link link; > + struct rte_eth_dev *pf; > + > + if (rpst == NULL) > + return -1; > + > + hw = rpst->hw; > + > + memset(&link, 0, sizeof(link)); > + > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > + link.link_autoneg = !(rpst->ethdev->data->dev_conf.link_speeds & > + ETH_LINK_SPEED_FIXED); > + > + rawdev = hw->rawdev; > + ipn3ke_update_link(rawdev, rpst->port_id, &link); > + > + if (!rpst->ori_linfo.link_status && > + link.link_status) { Better to put one more tab here, to make it easy to pick the condition part from the indendent code. <...> > + > +struct eth_dev_ops ipn3ke_rpst_dev_ops = { This can be "static const" > + .dev_infos_get = ipn3ke_rpst_dev_infos_get, > + > + .dev_configure = ipn3ke_rpst_dev_configure, > + .dev_start = ipn3ke_rpst_dev_start, > + .dev_stop = ipn3ke_rpst_dev_stop, > + .dev_close = ipn3ke_rpst_dev_close, > + .dev_reset = ipn3ke_rpst_dev_reset, > + > + .stats_get = ipn3ke_rpst_stats_get, > + .xstats_get = ipn3ke_rpst_xstats_get, > + .xstats_get_names = ipn3ke_rpst_xstats_get_names, > + .stats_reset = ipn3ke_rpst_stats_reset, > + .xstats_reset = ipn3ke_rpst_stats_reset, > + > + .filter_ctrl = ipn3ke_afu_filter_ctrl, > + > + .rx_queue_start = ipn3ke_rpst_rx_queue_start, > + .rx_queue_stop = ipn3ke_rpst_rx_queue_stop, > + .tx_queue_start = ipn3ke_rpst_tx_queue_start, > + .tx_queue_stop = ipn3ke_rpst_tx_queue_stop, > + .rx_queue_setup = ipn3ke_rpst_rx_queue_setup, > + .rx_queue_release = ipn3ke_rpst_rx_queue_release, > + .tx_queue_setup = ipn3ke_rpst_tx_queue_setup, > + .tx_queue_release = ipn3ke_rpst_tx_queue_release, > + > + .dev_set_link_up = ipn3ke_rpst_dev_set_link_up, > + .dev_set_link_down = ipn3ke_rpst_dev_set_link_down, > + .link_update = ipn3ke_rpst_link_update, > + > + .promiscuous_enable = ipn3ke_rpst_promiscuous_enable, > + .promiscuous_disable = ipn3ke_rpst_promiscuous_disable, > + .allmulticast_enable = ipn3ke_rpst_allmulticast_enable, > + .allmulticast_disable = ipn3ke_rpst_allmulticast_disable, > + .mac_addr_set = ipn3ke_rpst_mac_addr_set, > + .mtu_set = ipn3ke_rpst_mtu_set, > + > + .tm_ops_get = NULL, No need to set NULL ones. <...> > +int > +ipn3ke_rpst_init(struct rte_eth_dev *ethdev, void *init_params) > +{ > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(ethdev); > + struct ipn3ke_rpst *representor_param = > + (struct ipn3ke_rpst *)init_params; > + > + if (representor_param->port_id >= representor_param->hw->port_num) > + return -ENODEV; > + > + rpst->ethdev = ethdev; > + rpst->switch_domain_id = representor_param->switch_domain_id; > + rpst->port_id = representor_param->port_id; > + rpst->hw = representor_param->hw; > + rpst->i40e_pf_eth = NULL; > + rpst->i40e_pf_eth_port_id = 0xFFFF; > + > + ethdev->data->mac_addrs = rte_zmalloc("ipn3ke", ETHER_ADDR_LEN, 0); > + if (!ethdev->data->mac_addrs) { > + IPN3KE_AFU_PMD_ERR("Failed to " > + "allocated memory for storing mac address"); > + return -ENODEV; > + } > + > + /** representor shares the same driver as it's PF device */ > + /** > + * ethdev->device->driver = rpst->hw->eth_dev->device->driver; > + */ Please remove commented code.