From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Duszynski Subject: Re: [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver Date: Fri, 6 Oct 2017 08:41:05 +0200 Message-ID: <20171006064105.GA11828@tdu> 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> <20171005084333.GA20961@tdu> <14bc8ee0-d474-0041-8cc9-22c2f881eef0@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: Tomasz Duszynski , dev@dpdk.org, mw@semihalf.com, dima@marvell.com, nsamsono@marvell.com, Jianbo.liu@linaro.org, Jacek Siuda To: Ferruh Yigit Return-path: Received: from mail-lf0-f48.google.com (mail-lf0-f48.google.com [209.85.215.48]) by dpdk.org (Postfix) with ESMTP id 0A5651B24C for ; Fri, 6 Oct 2017 08:41:08 +0200 (CEST) Received: by mail-lf0-f48.google.com with SMTP id q132so19269313lfe.5 for ; Thu, 05 Oct 2017 23:41:08 -0700 (PDT) Content-Disposition: inline In-Reply-To: <14bc8ee0-d474-0041-8cc9-22c2f881eef0@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Oct 05, 2017 at 06:29:12PM +0100, Ferruh Yigit wrote: > On 10/5/2017 9:43 AM, Tomasz Duszynski wrote: > > On Wed, Oct 04, 2017 at 05:59:11PM +0100, Ferruh Yigit wrote: > >> 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 ad= apter. > >>>>> Driver is based on external, publicly available, light-weight Marve= ll > >>>>> 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 c= an > >> 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 other= s, > >> what do you think, is it too much effort? > >> > > > > Fair enough. I'll split the driver as suggested. A few specific > > questions about functionality each patch should contain though. > > > > As for skeleton, I see others just put driver probing here. > > > > As for Rx/Tx support it seems that there's no common pattern. > > Functionality like starting/stopping device, queues configuration > > and all the other things related to Rx/Tx should be here as well? > > As you said there is no common pattern, but I think starting/stopping > device, queues configuration can go into skeleton and mainly Rx/Tx burst > functions can go into Rx/Tx patch. > But please what you think more reasonable matters here. > ACK > > > > What's left are features which go into features-patch. > > Yes. > And the .ini file, currently part of doc patch, can be part of this > features patch, it is helps more to see the code add feature and doc > documents it in same patch. > ACK > > > >>>> > >>>>> > >>>>> 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 =3D { > >>>>> + .probe =3D rte_pmd_mrvl_probe, > >>>>> + .remove =3D 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 provid= es > >>>> userspace datapath support. This PMD is an interface to musdk librar= y. > >>>> 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? > > > > Bus is called Aurora2. That's proprietary SoC interconnect fabric. > > > >> > >>>> > >>>> 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 pr= ojects. > >>> Keeping multiple codebases offering similar functionality would be qu= ite > >>> 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 repositor= y. > >> > >> 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. > >> > > > > ACK > > > >>> Driver was tested on Armada 7k/8k SoCs. > >> > >> Can you please provide link to the HW mentioned in documentation? > >> > > > > You can find some info here: > > > > https://www.marvell.com/embedded-processors/armada-70xx/ > > https://www.marvell.com/embedded-processors/armada-80xx/ > > Thanks, would you mind putting these links into driver documentation as > well? ACK > > > > >>>> > >>>> <....> > >>>> > >>> > >>> -- > >>> - Tomasz Duszy=C5=84ski > >>> > >> > > > > -- > > - Tomasz Duszy=C5=84ski > > > -- - Tomasz Duszy=C5=84ski