From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vs1-f65.google.com (mail-vs1-f65.google.com [209.85.217.65]) by dpdk.org (Postfix) with ESMTP id 84A992B99 for ; Mon, 1 Apr 2019 14:53:04 +0200 (CEST) Received: by mail-vs1-f65.google.com with SMTP id a190so5414461vsd.0 for ; Mon, 01 Apr 2019 05:53:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netcope.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kAdn+vDvaq4pXvU1lL5SeNs07JhuFTt6wxS0/IOcD4s=; b=Oslhdv0AmtFWtTd3pI53xDUwobwWzsPAHZ7ANn70O7ygdUDQasoCV7qvsI5/HlPfZY grhk85nWy2MXZMW0K0235xnAKsbG722XRmn5seVyPJz3FySDmRPogQau7UYjC/JBSIrs xyjbh8+F7GGWVZgas13oJAjtglFnZOJL1WkKFDOkXAfvinSWduvKDcdr7G/zFZ7VIAbq rbHg6R9tZx3I/FGjggTR3dp2UMGpBsxdUioYHdNzQVJrucjEJPw8n7KGnexrj8d1prW9 9ORLIQIPT2c88qTjCO1AV2rg25l0Ey8n+dB9Cj32TeR48WWNkhEb8+m/YorTc4GxtYVg KOjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kAdn+vDvaq4pXvU1lL5SeNs07JhuFTt6wxS0/IOcD4s=; b=oesoCqBwHxPgf5sMUpY0bppxj/NJ84QDyRZlLCKJ+GYnNLKXD465A2G87SY+KnJ+ak FLLeCqDcvslTyQzxgvCBwYJ+quVCDuYuxqJjo3RhShnlkGf8IspgvWr9pDK2JO4APWJC jmHN8fjngA8FpQYHr3ErMikAB7RseZv0TOzKkC27PZZ2fTjSE97KOERPawd4Zu/k3lKR oZujr618oKRSYUs/yNu0NMK4YX/crnM9NPkCr3hZhfYdk8wYn8EpFyBE0VXxMi1aWidI J4BiijUQXPX2M2Sp1JXMF8wxbi0w7YSrf3OD3/QShmAfu0HrVLDBatLjqKiysnlyBDrO dbRA== X-Gm-Message-State: APjAAAVUndYfKZjcCqcZ1chQkKDUODl7bB1g1wU15js8L1nMkZ/rP7aV wmXROpM8IfLiWPKSQwm4voLJl3E0vq8cSAM7m1Ni5A== X-Google-Smtp-Source: APXvYqzT7QESvkj+8umaQ0jmydKcb/ZaxLoEQ5n+MC6neLRsbKTg/gJ4252wdFTgmzHYnTHnaWBlm2qcot120OwsvfI= X-Received: by 2002:a67:7bd5:: with SMTP id w204mr6401655vsc.122.1554123183912; Mon, 01 Apr 2019 05:53:03 -0700 (PDT) MIME-Version: 1.0 References: <1551185824-5501-2-git-send-email-cernay@netcope.com> <1553256767-135084-1-git-send-email-cernay@netcope.com> In-Reply-To: From: =?UTF-8?Q?Rastislav_=C4=8Cernay?= Date: Mon, 1 Apr 2019 16:55:43 +0200 Message-ID: To: Ferruh Yigit Cc: dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: , X-List-Received-Date: Mon, 01 Apr 2019 12:53:05 -0000 Hi Ferruh, I fixed most of issues you wrote, but before I send v6, I would like to mak= e sure everything is in order, mainly the experimental issue. 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. <...> Sadly, szedata2 and nfb dirvers can not coexist in same enviroment. There is no way to provide dependecies at same time. Is rpm only option, how people using Ubuntu for example will use your devices? <...> For now, yes, only option is rpm and support for Ubuntu will be added later= . Why allow_experimental only in meson, but not Makefile? <...> It is in Makefile too. CFLAGS +=3D -DALLOW_EXPERIMENTAL_API Can you please add list of experimental APIs called just below this as comment? <...> No experimental API is called in driver. btw, why do you prefer to have driver as experimental? What does it mean it being experimental from your point of view exactly? <...> I do not prefer to have driver experimental, only reason it is there is this mail: Luca Boccassi bluca@debian.org Tue, Mar 5, 11:41 PM to me, dev <...> > diff --git a/drivers/net/nfb/rte_nfb_pmd_version.map > b/drivers/net/nfb/rte_nfb_pmd_version.map > new file mode 100644 > index 0000000..97fd251 > --- /dev/null > +++ b/drivers/net/nfb/rte_nfb_pmd_version.map > @@ -0,0 +1,4 @@ > +DPDK_19.02 { > + > + local: *; > +}; These are all new symbols so they should be marked as experimental, please see doc/guides/contributing/versioning.rst So after reading doc/guides/contributing/versioning.rst I thought that all new drivers should be experimental as they are new before they stabilize, and during this time changes can be done to public functions without much hassle. Should I keep driver experimental? *Rastislav =C4=8Cernay I Software DeveloperNetcope Technologies, a.s.T: +42= 0 530 510 680 <+420%20530%20510%20680>A: Sochorova 3232/34, Brno, 616 00, Czech Republic W: www.netcope.com * On Thu, Mar 28, 2019 at 5:01 PM Ferruh Yigit wrote= : > 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 wi= th > 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=3Dn > > # Compile software PMD backed by SZEDATA2 device > > # > > CONFIG_RTE_LIBRTE_PMD_SZEDATA2=3Dn > > +# > > +# Compile software PMD backed by NFB device > > +# > > +CONFIG_RTE_LIBRTE_PMD_NFB=3Dn > > 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_.*=3Dy > 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 < > https://www.netcope.com/en/company/community-support/dpdk-libsze2#NFB>`_. > > 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=3D3= 2 > > + nb forwarding cores=3D2 - nb forwarding ports=3D1 > > + RX queues=3D2 - RX desc=3D128 - RX free threshold=3D0 > > + RX threshold registers: pthresh=3D0 hthresh=3D0 wthresh=3D0 > > + TX queues=3D2 - TX desc=3D512 - TX free threshold=3D0 > > + TX threshold registers: pthresh=3D0 hthresh=3D0 wthresh=3D0 > > + TX RS bit threshold=3D0 - TXQ flags=3D0x0 > > + 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 =3D cc.find_library('nfb', required: false) > > + > > +build =3D dep.found() and cc.has_header('nfb/nfb.h', dependencies: dep= ) > > + > > +allow_experimental_apis =3D 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 =3D { > > + .addr_bytes =3D { 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 =3D 0; > > Would you prefer to use a fixed length deceleration, uint64_t? > > > + struct rte_eth_dev_data *data =3D dev->data; > > + struct pmd_internals *internals =3D (struct pmd_internals *) > > + data->dev_private; > > + > > + if (!is_valid_assigned_ether_addr(mac_addr)) > > + return -EINVAL; > > + > > + for (i =3D 0; i < 6; i++) { > > Can you please use "ETHER_ADDR_LEN" instead of 6? > > <...> > > > + for (i =3D 0; i < nb_rx; i++) { > > + if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) { > > + stats->q_ipackets[i] =3D rx_queue[i].rx_pkts; > > + stats->q_ibytes[i] =3D rx_queue[i].rx_bytes; > > + } > > + rx_total +=3D stats->q_ipackets[i]; > > + rx_total_bytes +=3D stats->q_ibytes[i]; > > Why these are not protected with "RTE_ETHDEV_QUEUE_STAT_CNTRS" check, > if "i >=3D RTE_ETHDEV_QUEUE_STAT_CNTRS" you shouldn't access to > 'stats->q_.*[i]' > 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 32551A0679 for ; Mon, 1 Apr 2019 14:53:07 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 04395316B; Mon, 1 Apr 2019 14:53:07 +0200 (CEST) Received: from mail-vs1-f65.google.com (mail-vs1-f65.google.com [209.85.217.65]) by dpdk.org (Postfix) with ESMTP id 84A992B99 for ; Mon, 1 Apr 2019 14:53:04 +0200 (CEST) Received: by mail-vs1-f65.google.com with SMTP id a190so5414461vsd.0 for ; Mon, 01 Apr 2019 05:53:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netcope.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kAdn+vDvaq4pXvU1lL5SeNs07JhuFTt6wxS0/IOcD4s=; b=Oslhdv0AmtFWtTd3pI53xDUwobwWzsPAHZ7ANn70O7ygdUDQasoCV7qvsI5/HlPfZY grhk85nWy2MXZMW0K0235xnAKsbG722XRmn5seVyPJz3FySDmRPogQau7UYjC/JBSIrs xyjbh8+F7GGWVZgas13oJAjtglFnZOJL1WkKFDOkXAfvinSWduvKDcdr7G/zFZ7VIAbq rbHg6R9tZx3I/FGjggTR3dp2UMGpBsxdUioYHdNzQVJrucjEJPw8n7KGnexrj8d1prW9 9ORLIQIPT2c88qTjCO1AV2rg25l0Ey8n+dB9Cj32TeR48WWNkhEb8+m/YorTc4GxtYVg KOjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kAdn+vDvaq4pXvU1lL5SeNs07JhuFTt6wxS0/IOcD4s=; b=oesoCqBwHxPgf5sMUpY0bppxj/NJ84QDyRZlLCKJ+GYnNLKXD465A2G87SY+KnJ+ak FLLeCqDcvslTyQzxgvCBwYJ+quVCDuYuxqJjo3RhShnlkGf8IspgvWr9pDK2JO4APWJC jmHN8fjngA8FpQYHr3ErMikAB7RseZv0TOzKkC27PZZ2fTjSE97KOERPawd4Zu/k3lKR oZujr618oKRSYUs/yNu0NMK4YX/crnM9NPkCr3hZhfYdk8wYn8EpFyBE0VXxMi1aWidI J4BiijUQXPX2M2Sp1JXMF8wxbi0w7YSrf3OD3/QShmAfu0HrVLDBatLjqKiysnlyBDrO dbRA== X-Gm-Message-State: APjAAAVUndYfKZjcCqcZ1chQkKDUODl7bB1g1wU15js8L1nMkZ/rP7aV wmXROpM8IfLiWPKSQwm4voLJl3E0vq8cSAM7m1Ni5A== X-Google-Smtp-Source: APXvYqzT7QESvkj+8umaQ0jmydKcb/ZaxLoEQ5n+MC6neLRsbKTg/gJ4252wdFTgmzHYnTHnaWBlm2qcot120OwsvfI= X-Received: by 2002:a67:7bd5:: with SMTP id w204mr6401655vsc.122.1554123183912; Mon, 01 Apr 2019 05:53:03 -0700 (PDT) MIME-Version: 1.0 References: <1551185824-5501-2-git-send-email-cernay@netcope.com> <1553256767-135084-1-git-send-email-cernay@netcope.com> In-Reply-To: From: =?UTF-8?Q?Rastislav_=C4=8Cernay?= Date: Mon, 1 Apr 2019 16:55:43 +0200 Message-ID: To: Ferruh Yigit Cc: dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: <20190401145543.tH8AnaJAqIMc9G24vuOY6_fH169r0cY0HnqIxlxBD6w@z> Hi Ferruh, I fixed most of issues you wrote, but before I send v6, I would like to mak= e sure everything is in order, mainly the experimental issue. 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. <...> Sadly, szedata2 and nfb dirvers can not coexist in same enviroment. There is no way to provide dependecies at same time. Is rpm only option, how people using Ubuntu for example will use your devices? <...> For now, yes, only option is rpm and support for Ubuntu will be added later= . Why allow_experimental only in meson, but not Makefile? <...> It is in Makefile too. CFLAGS +=3D -DALLOW_EXPERIMENTAL_API Can you please add list of experimental APIs called just below this as comment? <...> No experimental API is called in driver. btw, why do you prefer to have driver as experimental? What does it mean it being experimental from your point of view exactly? <...> I do not prefer to have driver experimental, only reason it is there is this mail: Luca Boccassi bluca@debian.org Tue, Mar 5, 11:41 PM to me, dev <...> > diff --git a/drivers/net/nfb/rte_nfb_pmd_version.map > b/drivers/net/nfb/rte_nfb_pmd_version.map > new file mode 100644 > index 0000000..97fd251 > --- /dev/null > +++ b/drivers/net/nfb/rte_nfb_pmd_version.map > @@ -0,0 +1,4 @@ > +DPDK_19.02 { > + > + local: *; > +}; These are all new symbols so they should be marked as experimental, please see doc/guides/contributing/versioning.rst So after reading doc/guides/contributing/versioning.rst I thought that all new drivers should be experimental as they are new before they stabilize, and during this time changes can be done to public functions without much hassle. Should I keep driver experimental? *Rastislav =C4=8Cernay I Software DeveloperNetcope Technologies, a.s.T: +42= 0 530 510 680 <+420%20530%20510%20680>A: Sochorova 3232/34, Brno, 616 00, Czech Republic W: www.netcope.com * On Thu, Mar 28, 2019 at 5:01 PM Ferruh Yigit wrote= : > 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 wi= th > 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=3Dn > > # Compile software PMD backed by SZEDATA2 device > > # > > CONFIG_RTE_LIBRTE_PMD_SZEDATA2=3Dn > > +# > > +# Compile software PMD backed by NFB device > > +# > > +CONFIG_RTE_LIBRTE_PMD_NFB=3Dn > > 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_.*=3Dy > 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 < > https://www.netcope.com/en/company/community-support/dpdk-libsze2#NFB>`_. > > 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=3D3= 2 > > + nb forwarding cores=3D2 - nb forwarding ports=3D1 > > + RX queues=3D2 - RX desc=3D128 - RX free threshold=3D0 > > + RX threshold registers: pthresh=3D0 hthresh=3D0 wthresh=3D0 > > + TX queues=3D2 - TX desc=3D512 - TX free threshold=3D0 > > + TX threshold registers: pthresh=3D0 hthresh=3D0 wthresh=3D0 > > + TX RS bit threshold=3D0 - TXQ flags=3D0x0 > > + 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 =3D cc.find_library('nfb', required: false) > > + > > +build =3D dep.found() and cc.has_header('nfb/nfb.h', dependencies: dep= ) > > + > > +allow_experimental_apis =3D 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 =3D { > > + .addr_bytes =3D { 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 =3D 0; > > Would you prefer to use a fixed length deceleration, uint64_t? > > > + struct rte_eth_dev_data *data =3D dev->data; > > + struct pmd_internals *internals =3D (struct pmd_internals *) > > + data->dev_private; > > + > > + if (!is_valid_assigned_ether_addr(mac_addr)) > > + return -EINVAL; > > + > > + for (i =3D 0; i < 6; i++) { > > Can you please use "ETHER_ADDR_LEN" instead of 6? > > <...> > > > + for (i =3D 0; i < nb_rx; i++) { > > + if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) { > > + stats->q_ipackets[i] =3D rx_queue[i].rx_pkts; > > + stats->q_ibytes[i] =3D rx_queue[i].rx_bytes; > > + } > > + rx_total +=3D stats->q_ipackets[i]; > > + rx_total_bytes +=3D stats->q_ibytes[i]; > > Why these are not protected with "RTE_ETHDEV_QUEUE_STAT_CNTRS" check, > if "i >=3D RTE_ETHDEV_QUEUE_STAT_CNTRS" you shouldn't access to > 'stats->q_.*[i]' >