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 9221DA0679 for ; Thu, 28 Mar 2019 17:01:41 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6FDA41B39D; Thu, 28 Mar 2019 17:01:39 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 40ED71B2B0 for ; Thu, 28 Mar 2019 17:01:37 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2019 09:01:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,280,1549958400"; d="scan'208";a="138061416" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.46]) ([10.237.221.46]) by orsmga003.jf.intel.com with ESMTP; 28 Mar 2019 09:01:34 -0700 To: Rastislav Cernay , dev@dpdk.org References: <1551185824-5501-2-git-send-email-cernay@netcope.com> <1553256767-135084-1-git-send-email-cernay@netcope.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: Date: Thu, 28 Mar 2019 16:01:34 +0000 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: <1553256767-135084-1-git-send-email-cernay@netcope.com> Content-Type: text/plain; charset="UTF-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5] net/nfb: new netcope 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: <20190328160134.JZn-XpLCfhR6yxGeArOVRl5FETGMKB93_v5bYUoVKoc@z> On 3/22/2019 12:12 PM, Rastislav Cernay wrote: > From: Rastislav Cernay > > Added new net driver for Netcope nfb cards > > Signed-off-by: Rastislav Cernay > --- > v2: remove unnecessary cast > remove unnecessary zeroing > move declaration to not mix with code > restore skeleton example > v3: add release notes > add doc to doc index > add architecture limits to doc > edit features list > add .map file > change link to dependecies to official vendor site > move declarations out of code > remove false comments (rte_errno is set) > comments to c89 style > remove log from main rx loop > remove redundant code > v4: API is experimental > fixed meson build dependency > random initial MAC > stats->q_errors is for intput err only > move more declarations to a beginning > fixed err log in TX > use of pkg-config > v5: fixed pkg-config for new version of netcope-common > MAINTAINERS | 7 + > config/common_base | 4 + > devtools/test-build.sh | 1 + > doc/guides/nics/features/nfb.ini | 18 + > doc/guides/nics/index.rst | 1 + > doc/guides/nics/nfb.rst | 143 +++++++ > doc/guides/rel_notes/release_19_02.rst | 5 + Can you update the 19.05 release notes please? > drivers/net/Makefile | 1 + > drivers/net/meson.build | 1 + > drivers/net/nfb/Makefile | 44 +++ > drivers/net/nfb/meson.build | 15 + > drivers/net/nfb/nfb.h | 50 +++ > drivers/net/nfb/nfb_ethdev.c | 647 ++++++++++++++++++++++++++++++++ > drivers/net/nfb/nfb_rx.c | 127 +++++++ > drivers/net/nfb/nfb_rx.h | 231 ++++++++++++ > drivers/net/nfb/nfb_rxmode.c | 100 +++++ > drivers/net/nfb/nfb_rxmode.h | 96 +++++ > drivers/net/nfb/nfb_stats.c | 77 ++++ > drivers/net/nfb/nfb_stats.h | 56 +++ > drivers/net/nfb/nfb_tx.c | 113 ++++++ > drivers/net/nfb/nfb_tx.h | 222 +++++++++++ > drivers/net/nfb/rte_nfb_pmd_version.map | 4 + The filename should be "rte_pmd_nfb_version.map" as stated in Makefile. This brings the question, did you ever build dpdk as shared library or with meson build system with this PMD enabled :) Because it fails because of this wrong naming.. Even after renaming the .map file, meson build fails, I haven't dig the problem, can you please check it? <...> > @@ -360,6 +360,10 @@ CONFIG_RTE_LIBRTE_SFC_EFX_DEBUG=n > # Compile software PMD backed by SZEDATA2 device > # > CONFIG_RTE_LIBRTE_PMD_SZEDATA2=n > +# > +# Compile software PMD backed by NFB device > +# > +CONFIG_RTE_LIBRTE_PMD_NFB=n Can you please rename the config option to "CONFIG_RTE_LIBRTE_NFB_PMD"? I guess unintentionally, there is an convension, Physical PMDs have the config name: CONFIG_RTE_LIBRTE_.*_PMD Virtual PMDs have the config name: CONFIG_RTE_LIBRTE_PMD_.*=y I perfer same syntax for all, but it is what it is now, and I believe it doesn't worth the hassle of changing them. I think only 'SZEDATA2' breaks current login, and I guess you copied from it, but let's start NFB according convention: CONFIG_RTE_LIBRTE_NFB_PMD Also please leave a blank line between previous config block. <...> > +Prerequisites > +------------- > + > +This PMD requires kernel modules which are responsible for initialization and > +allocation of resources needed for nfb layer function. > +Communication between PMD and kernel modules is mediated by libnfb library. > +These kernel modules and library are not part of DPDK and must be installed > +separately: > + > +* **libnfb library** > + > + The library provides API for initialization of nfb transfers, receiving and > + transmitting data segments. > + > +* **Kernel modules** > + > + * nfb > + > + Kernel modules manage initialization of hardware, allocation and > + sharing of resources for user space applications. > + > +Dependencies can be found here: > +`Netcope common `_. I have a problem here, this requires 'netcope-common-6.4.0-1.x86_64.rpm' rpm to be installed [1], meanwhile szedata requires 'netcope-common-light-3.0.5-1.x86_64.rpm' to be installed, and these two packages conflicts with eachother. What is the way of having an environment that dependencies of two netcope HW can be provided at same time? This is really time consuming each time update system for it. [1] Is rpm only option, how people using Ubuntu for example will use your devices? > + > +Versions of the packages > +~~~~~~~~~~~~~~~~~~~~~~~~ > + > +The minimum version of the provided packages: > + > +* for DPDK from 19.02 Is it 19.05 now? <...> > +Example output: > + > +.. code-block:: console > + > + [...] > + EAL: PCI device 0000:06:00.0 on NUMA socket -1 > + EAL: probe driver: 1b26:c1c1 net_nfb > + PMD: Initializing NFB device (0000:06:00.0) > + PMD: Available DMA queues RX: 8 TX: 8 > + PMD: NFB device (0000:06:00.0) successfully initialized > + Interactive-mode selected > + Auto-start selected > + Configuring Port 0 (socket 0) > + Port 0: 00:11:17:00:00:00 > + Checking link statuses... > + Port 0 Link Up - speed 10000 Mbps - full-duplex > + Done > + Start automatic packet forwarding > + io packet forwarding - CRC stripping disabled - packets/burst=32 > + nb forwarding cores=2 - nb forwarding ports=1 > + RX queues=2 - RX desc=128 - RX free threshold=0 > + RX threshold registers: pthresh=0 hthresh=0 wthresh=0 > + TX queues=2 - TX desc=512 - TX free threshold=0 > + TX threshold registers: pthresh=0 hthresh=0 wthresh=0 > + TX RS bit threshold=0 - TXQ flags=0x0 > + testpmd> I think testpmd logging is not like this any more, can you please update the app log with latest code. <...> > @@ -0,0 +1,44 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2018 Cesnet > +# Copyright(c) 2018 Netcope Technologies, a.s. Should year be 2019, or 2018-2019? <...> > @@ -0,0 +1,15 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2018 Cesnet > +# Copyright(c) 2018 Netcope Technologies, a.s. > +# All rights reserved. > + > +dep = cc.find_library('nfb', required: false) > + > +build = dep.found() and cc.has_header('nfb/nfb.h', dependencies: dep) > + > +allow_experimental_apis = true Why allow_experimental only in meson, but not Makefile? Can you please add list of experimental APIs called just below this as comment? <...> > +/** > + * Default MAC addr > + */ > +static struct ether_addr eth_addr = { > + .addr_bytes = { 0x00, 0x11, 0x17, 0x00, 0x00, 0x00 } > +}; it looks like this can be static const. > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice > + * Why these tags are added, to say PMD is experimental? These only makes sense for public APIs which PMD doesn't have at all, these are doxygen comments for API documentation. Please remove them. Also I recognized '__rte_experimental' tags, again which are for internal checking and warning the applications if they are using experimental public API, it doesn't apply here. Please remove them. Last thing, the "EXPERIMENTAL" tag in .map file, that also doesn't apply here, please make it release version, 'DPDK_19.05" I think your note about being experimental in MAINTAINERS file and the note in release notes is good, Can you please also update the driver documentation too, nfb.rst, to say driver is experimental? I think those notes are good enough overall. btw, why do you prefer to have driver as experimental? What does it mean it being experimental from your point of view exactly? <...> > +static int __rte_experimental > +nfb_eth_mac_addr_set(struct rte_eth_dev *dev, > + struct ether_addr *mac_addr) > +{ > + unsigned int i; > + unsigned long long mac = 0; Would you prefer to use a fixed length deceleration, uint64_t? > + struct rte_eth_dev_data *data = dev->data; > + struct pmd_internals *internals = (struct pmd_internals *) > + data->dev_private; > + > + if (!is_valid_assigned_ether_addr(mac_addr)) > + return -EINVAL; > + > + for (i = 0; i < 6; i++) { Can you please use "ETHER_ADDR_LEN" instead of 6? <...> > + for (i = 0; i < nb_rx; i++) { > + if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) { > + stats->q_ipackets[i] = rx_queue[i].rx_pkts; > + stats->q_ibytes[i] = rx_queue[i].rx_bytes; > + } > + rx_total += stats->q_ipackets[i]; > + rx_total_bytes += stats->q_ibytes[i]; Why these are not protected with "RTE_ETHDEV_QUEUE_STAT_CNTRS" check, if "i >= RTE_ETHDEV_QUEUE_STAT_CNTRS" you shouldn't access to 'stats->q_.*[i]'