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 865BA1B6BA for ; Wed, 4 Oct 2017 18:59:17 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 04 Oct 2017 09:59:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,478,1500966000"; d="scan'208";a="906722396" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.26]) ([10.241.225.26]) by FMSMGA003.fm.intel.com with ESMTP; 04 Oct 2017 09:59:12 -0700 To: Tomasz Duszynski Cc: dev@dpdk.org, mw@semihalf.com, dima@marvell.com, nsamsono@marvell.com, Jianbo.liu@linaro.org, Jacek Siuda References: <1506594158-15721-2-git-send-email-tdu@semihalf.com> <1507031500-11473-1-git-send-email-tdu@semihalf.com> <1507031500-11473-3-git-send-email-tdu@semihalf.com> <20171004085959.GB5668@tdu> From: Ferruh Yigit Message-ID: Date: Wed, 4 Oct 2017 17:59:11 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171004085959.GB5668@tdu> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net 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: Wed, 04 Oct 2017 16:59:18 -0000 On 10/4/2017 9:59 AM, Tomasz Duszynski wrote: > On Wed, Oct 04, 2017 at 01:24:27AM +0100, Ferruh Yigit wrote: >> On 10/3/2017 12:51 PM, Tomasz Duszynski wrote: >>> Add support for the Marvell PPv2 (Packet Processor v2) 1/10 Gbps adapter. >>> Driver is based on external, publicly available, light-weight Marvell >>> MUSDK library that provides access to network packet processor. >>> >>> Driver comes with support for the following features: >>> >>> * Speed capabilities >>> * Link status >>> * Queue start/stop >>> * MTU update >>> * Jumbo frame >>> * Promiscuous mode >>> * Allmulticast mode >>> * Unicast MAC filter >>> * Multicast MAC filter >>> * RSS hash >>> * VLAN filter >>> * CRC offload >>> * L3 checksum offload >>> * L4 checksum offload >>> * Packet type parsing >>> * Basic stats >>> * Stats per queue >> >> I have more detailed comments but in high level, >> what do you think splitting this patch into three patches: >> - Skeleton >> - Add Rx/Tx support >> - Add features, like MTU update or Promiscuous etc.. support > If it's how submission process works then I think you left me with no > other option than splitting driver into nice patchset :). No, there is no defined submission process. > On the other > hand driver is really a wrapper to MUSDK library and thus quite easy to > follow. What are the benefits of such 3-way split? To help others review/understand your code. Big code chunks are scary and I believe most of details gets lost in big code chunks. When someone from community wants to understand and update/improve/fix your code, to help them by logically split the code that their focus can go into more narrow part. But this also means some effort in your side, so some kind of balance is required. I think splitting patch into smaller logical part is helpful for others, what do you think, is it too much effort? >> >>> >>> Driver was engineered cooperatively by Semihalf and Marvell teams. >>> >>> Semihalf: >>> Jacek Siuda >>> Tomasz Duszynski >>> >>> Marvell: >>> Dmitri Epshtein >>> Natalie Samsonov >>> >>> Signed-off-by: Jacek Siuda >>> Signed-off-by: Tomasz Duszynski >> >> <...> >> >>> +static struct rte_vdev_driver pmd_mrvl_drv = { >>> + .probe = rte_pmd_mrvl_probe, >>> + .remove = rte_pmd_mrvl_remove, >>> +}; >>> + >>> +RTE_PMD_REGISTER_VDEV(net_mrvl, pmd_mrvl_drv); >> >> Please help me understand. >> >> This driver implemented as virtual driver, because: >> With the help of custom kernel modules, musdk library already provides >> userspace datapath support. This PMD is an interface to musdk library. >> Is this correct? > That is right. Another reason this NIC is not PCI device. We support more bus now :). Out of curiosity, which bus is device on? >> >> If so, just thinking loud: >> - Why not implement this PMD directly on top of kernel interface, >> removing musdk layer completely? >> - How big problem that this PMD depends on custom kernel code? > I think the main reason is that MUSDK is already used in different projects. > Keeping multiple codebases offering similar functionality would be quite > demanding in terms of extra work needed. >> - How library and custom kernel code delivered? For which platforms? > Kernel and library sources are hosted on publicly available repository. I guess it would be nice to highlight custom kernel with external patches is required. This is not mentioned in "Prerequisites" section of the document. > Driver was tested on Armada 7k/8k SoCs. Can you please provide link to the HW mentioned in documentation? >> >> <....> >> > > -- > - Tomasz Duszyński >