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: Thu, 5 Oct 2017 10:43:33 +0200 Message-ID: <20171005084333.GA20961@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> 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-f51.google.com (mail-lf0-f51.google.com [209.85.215.51]) by dpdk.org (Postfix) with ESMTP id C4DE91B199 for ; Thu, 5 Oct 2017 10:43:35 +0200 (CEST) Received: by mail-lf0-f51.google.com with SMTP id b127so16099143lfe.9 for ; Thu, 05 Oct 2017 01:43:35 -0700 (PDT) Content-Disposition: inline In-Reply-To: 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 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 adap= ter. > >>> 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? > 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? What's left are features which go into features-patch. > >> > >>> > >>> 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 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? 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 proj= ects. > > 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. > 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/ > >> > >> <....> > >> > > > > -- > > - Tomasz Duszy=C5=84ski > > > -- - Tomasz Duszy=C5=84ski