From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id A26B8567E for ; Fri, 25 Nov 2016 13:43:30 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP; 25 Nov 2016 04:43:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,547,1473145200"; d="scan'208";a="905382650" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by orsmga003.jf.intel.com with ESMTP; 25 Nov 2016 04:43:27 -0800 To: Andrew Rybchenko , dev@dpdk.org References: <1479740470-6723-1-git-send-email-arybchenko@solarflare.com> <9cdced57-ba4c-fd0e-c509-904d333aab50@intel.com> <5d041f12-ec8d-77da-47ac-671eb3a39a5b@solarflare.com> <576edd32-01be-0210-f5f0-5a6d54450acf@intel.com> From: Ferruh Yigit Cc: Thomas Monjalon , Bruce Richardson Message-ID: Date: Fri, 25 Nov 2016 12:43:27 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 00/56] Solarflare libefx-based PMD X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Nov 2016 12:43:31 -0000 On 11/25/2016 12:02 PM, Andrew Rybchenko wrote: > On 11/25/2016 01:24 PM, Ferruh Yigit wrote: >> On 11/23/2016 7:49 AM, Andrew Rybchenko wrote: >>> On 11/23/2016 03:02 AM, Ferruh Yigit wrote: >>>> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote: >>>>> The patch series adds Solarflare libefx-based network PMD. >>>>> >>>>> This version of the driver supports Solarflare SFN7xxx and SFN8xxx >>>>> families of 10/40 Gbps adapters. >>>>> >>>>> libefx is a platform-independent library to implement drivers for >>>>> Solarflare network adapters. It provides unified adapter family >>>>> independent interface (if possible). FreeBSD [1] and illumos [2] >>>>> drivers are built on top of the library. >>>>> >>>>> The patch series could be logically structured into 5 sub-series: >>>>> 1. (1) add the driver skeleton including documentation >>>>> 2. (2-30) import libefx and include it in build with the latest patch >>>>> 3. (31-43) implement minimal device level operations in steps >>>>> 4. (44-51) implement Rx subsystem >>>>> 5. (52-56) implement Tx subsystem >>>>> >>>>> Functional driver with multi-queue support capable to send and receive >>>>> traffic appears with the last patch in the series. >>>>> >>>>> The following design decisions are made during development: >>>>> >>>>> 1. Since libefx uses positive errno return codes, positive errno >>>>> return codes are used inside the driver and coversion to negative >>>>> is done on return from eth_dev_ops callbacks. We think that it >>>>> is the less error-prone way. >>>>> >>>>> 2. Another Solarflare PMD with in-kernel part (for control operations) >>>>> is considered and could be added in the future. Code for data path >>>>> should be shared by these two drivers. libefx-based PMD is put into >>>>> 'efx' subdirectory to have a space for another PMD and shared code. >>>>> >>>>> 3. Own event queue (a way to deliver events from HW to host CPU) is >>>>> used for house-keeping (e.g. link status notifications), each Tx >>>>> and each Rx queue. No locks on datapath are requires in this case. >>>>> >>>>> 4. Alarm is used to periodically poll house-keeping event queue. >>>>> The event queue is used to deliver link status change notifications, >>>>> Rx/Tx queue flush events, SRAM events. It is not used on datapath. >>>>> The event queue polling is protected using spin-lock since >>>>> concurrent access from different contexts is possible (e.g. device >>>>> stop when polling alarm is running). >>>>> >>>>> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/ >>>>> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/ >>>>> >>>>> --- >>>>> >>>>> Andrew Rybchenko (49): >>>>> net/sfc: libefx-based PMD stub sufficient to build and init >>>>> net/sfc: import libefx base >>>>> net/sfc: import libefx register definitions >>>>> net/sfc: import libefx filters support >>>>> net/sfc: import libefx MCDI definition >>>>> net/sfc: import libefx MCDI implementation >>>>> net/sfc: import libefx MCDI logging support >>>>> net/sfc: import libefx MCDI proxy authorization support >>>>> net/sfc: import libefx 5xxx/6xxx family support >>>>> net/sfc: import libefx SFN7xxx family support >>>>> net/sfc: import libefx SFN8xxx family support >>>>> net/sfc: import libefx diagnostics support >>>>> net/sfc: import libefx built-in selftest support >>>>> net/sfc: import libefx software per-queue statistics support >>>>> net/sfc: import libefx PHY flags control support >>>>> net/sfc: import libefx PHY statistics support >>>>> net/sfc: import libefx PHY LEDs control support >>>>> net/sfc: import libefx MAC statistics support >>>>> net/sfc: import libefx event prefetch support >>>>> net/sfc: import libefx Rx scatter support >>>>> net/sfc: import libefx RSS support >>>>> net/sfc: import libefx loopback control support >>>>> net/sfc: import libefx monitors statistics support >>>>> net/sfc: import libefx support to access monitors via MCDI >>>>> net/sfc: import libefx support for Rx packed stream mode >>>>> net/sfc: import libefx NVRAM support >>>>> net/sfc: import libefx VPD support >>>>> net/sfc: import libefx bootrom configuration support >>>>> net/sfc: import libefx licensing support >>>>> net/sfc: implement dummy callback to get device information >>>>> net/sfc: implement driver operation to init device on attach >>>>> net/sfc: add device configure and close stubs >>>>> net/sfc: add device configuration checks >>>>> net/sfc: implement device start and stop operations >>>>> net/sfc: make available resources estimation and allocation >>>>> net/sfc: interrupts support sufficient for event queue init >>>>> net/sfc: implement event queue support >>>>> net/sfc: implement EVQ dummy exception handling >>>>> net/sfc: maintain management event queue >>>>> net/sfc: periodic management EVQ polling using alarm >>>>> net/sfc: minimum port control sufficient to receive traffic >>>>> net/sfc: implement Rx subsystem stubs >>>>> net/sfc: check configured rxmode >>>>> net/sfc: implement Rx queue setup release operations >>>>> net/sfc: calculate Rx buffer size which may be used >>>>> net/sfc: validate Rx queue buffers setup >>>>> net/sfc: implement Rx queue start and stop operations >>>>> net/sfc: implement device callback to Rx burst of packets >>>>> net/sfc: discard scattered packet on Rx correctly >>>>> >>>>> Artem Andreev (2): >>>>> net/sfc: include libefx in build >>>>> net/sfc: implement device operation to retrieve link info >>>>> >>>>> Ivan Malov (5): >>>>> net/sfc: provide basic stubs for Tx subsystem >>>>> net/sfc: add function to check configured Tx mode >>>>> net/sfc: add callbacks to set up and release Tx queues >>>>> net/sfc: implement transmit path start / stop >>>>> net/sfc: add callback to send bursts of packets >>>>> >>>> Hi Andrew, >>>> >>>> Thank you for the patch, I have encounter with a few minor issues, can >>>> you please check them [1]? >>>> >>>> Also folder structure is drivers/net/sfc/efx/, why /sfc/ >>>> layer is created? >>>> sfc is company name (solarflare communications), right? Other driver >>>> folders not structured based on company, what about using >>>> drivers/net/efx/* ? >>> I've tried to explain it above in item (2): >>> >>> >>> >>> >>> 2. Another Solarflare PMD with in-kernel part (for control operations) >>> is considered and could be added in the future. Code for data path >>> should be shared by these two drivers. libefx-based PMD is put into >>> 'efx' subdirectory to have a space for another PMD and shared code. >>> >>> <<< >>> >>> So, main reason is to have location for the code shared by two Solarflare >>> network PMDs. May be it better to relocate when we really have it. >>> I'm open for other ideas/suggestions. >> If there will be another PMD that shares code with current one, the >> logic seems good, but I am not sure about start using company names, I >> am not against it, just I don't know. > > I think that mlx4 and mlx5 are tightly bound to the company name (plus > adapter generation, I guess). > >> Let's relocate later, this buys some time to think / get feedback on issue. > > Do I understand correctly that you suggest to avoid extra level inside > for now > and relocate later if required? If so, that's fine for me. > > As for naming, we think that just "efx" is a bad idea. The prefix is > occupied by > the libefx functions and driver should use something else. We have chosen > "sfc" mainly to follow naming used in Linux kernel for Solarflare driver > (the first level of Ethernet driver names is company bound in the Linux > kernel). > If we avoid extra level as discussed above, I think "sfc_efx", "sfcefx" > (may be it > will look better nearby other drivers) or just "sfc" are fine for us. > Thomas, Bruce, any comment on this? >>>> [1]: >>>> 1- There are a few (non-base) checkpatch warnings, can you please check >>>> patch 36, 49, 50 and 55 please? >>> Thanks, I'll fix spelling in v2. >>> 36, 49 and 55 also ask to check multiple assignments. IMHO, it is the >>> right usage >>> of multiple assignment when logically bound variables must have the same >>> value. >>> >>>> 2- Got following compile issues, not investigated, directly sharing here: >>>> >>>> b) for icc getting following warnings: >>>> ======================================= >>>> icc: command line warning #10006: ignoring unknown option '-Wno-empty-body' >>>> icc: command line warning #10006: ignoring unknown option >>>> '-Waggregate-return' >>>> icc: command line warning #10006: ignoring unknown option >>>> '-Wbad-function-cast' >>>> icc: command line warning #10006: ignoring unknown option '-Wnested-externs' >>>> >>>> >>>> c) icc compiler errors: >>>> ======================================= >>>> In file included from >>>> .../x86_64-native-linuxapp-icc/include/rte_ethdev.h(185), >>>> from .../drivers/net/sfc/efx/sfc.h(35), >>>> from .../drivers/net/sfc/efx/sfc.c(37): >>>> .../x86_64-native-linuxapp-icc/include/rte_ether.h(258): warning #2203: >>>> cast discards qualifiers from target type >>>> uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes); >>>> ^ >>>> >>>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1157): warning #3179: deprecated >>>> conversion of string literal to char* (should be const char*) >>>> .../drivers/net/sfc/efx/base/ef10_filter.c(1276): warning #188: >>>> enumerated type mixed with another type >>>> : "unknown assertion"; >>>> ^ >>>> >>>> filter_flags = 0; >>>> ^ >>>> >>>> .../drivers/net/sfc/efx/base/efx_mcdi.c(1426): warning #188: enumerated >>>> type mixed with another type >>>> epp->ep_fixed_port_type = >>>> ^ >>>> >>>> .../drivers/net/sfc/efx/base/efx_nic.c(556): warning #188: enumerated >>>> type mixed with another type >>>> enp->en_family = 0; >>> Yes, I have no ICC compilers. I'll try to fix these warnings, but I >>> can't be sure without checking it. >>> Also we cannot claim ICC supported without building and testing the >>> generated binary. >> Please update the code at least to not break the icc compilation, >> specially since this PMD will be default enabled. If you prefer I can >> verify compilation offline before you send the patchset. > > I'll do. I would be thankful if you help with ICC check. Should I send the > patch series directly to you? Just for icc check, you can send directly to me to reduce the noise in the list, or you can send to the list, both are OK for me. > > Andrew.