From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f181.google.com (mail-qt0-f181.google.com [209.85.216.181]) by dpdk.org (Postfix) with ESMTP id 6105028F3 for ; Wed, 29 Mar 2017 00:39:01 +0200 (CEST) Received: by mail-qt0-f181.google.com with SMTP id x35so77017563qtc.2 for ; Tue, 28 Mar 2017 15:39:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=atomicrules-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=wHqYWfPpPIyq6U7pbjmZpm7S3OK5L3xeBtQmWGSFtMU=; b=U/0V3rEfKXteTeYFcriQmVJi7g8ZoSF/FNU762wMUEyV/dTcRBo9k+1sCNWxxJuDlL mZIvdBmEqT2lY66GLw5N9xs/FDmXz9I+7FdJ/vy0JzPQ2iDPCgaS4QFE4ouZMYUMBzdW 2g2QocsI9ExV3FFX5P4e3TB/FZ+e/npwnigmEO42lKORXT0NeUX0FqrnjssapjYStZ34 74J0CHmn1rM54tu9BxTyJcmCPtDwwNlN8EiemHhTkVa54sZw1OyBpfGdmI8Vwu/z0GF+ GQQe4q00EyQ20QCTn0IfRazS5rwUreAupKOkf4JY+KSckVk01i017rUsT9lwRQU/fnu8 /PMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=wHqYWfPpPIyq6U7pbjmZpm7S3OK5L3xeBtQmWGSFtMU=; b=p0adRImJj4vLwzfzPajEWXezLMU5/9XQ/SliFb9TmKt9TkB2j6l24bpugCfUocl3Gd h+PRN6vCC2oQWu6EgK5uuycnsbFYU1fOmA5jmibkQhpoQexw0lBzflfkdTcI+mDEOCFf Cwwa5mv92htPlXhwt70WkuT+m6SIkyh18MBhh/0/IclcGjLDV2onZt4sMWxffZEE8kdv f6+heezhXSHxL1AWE+Zgx6B0dRTuMpedPWZcdaX2KWKK9nn/+5VAQ3SVnYelGrtvxKM4 0U28IJGv+lFPPhf3eL8edOdGT5t1HkLw6mDDFEjsJulTRqeTVKXgYwhgFAiBBDrl9qeH FULA== X-Gm-Message-State: AFeK/H1SMw9Jdw0jPdZ4UErnHMvT2dGoT8oYMIiCOyPJ5jfBtb2nk4//zC1rs1Vl+L39T3R7c6I0LuxDVEefew== X-Received: by 10.237.62.98 with SMTP id m31mr30527575qtf.71.1490740740196; Tue, 28 Mar 2017 15:39:00 -0700 (PDT) MIME-Version: 1.0 Received: by 10.200.1.131 with HTTP; Tue, 28 Mar 2017 15:38:39 -0700 (PDT) In-Reply-To: References: <1490231015-31748-1-git-send-email-ed.czeck@atomicrules.com> <41265c4fa76265df0144d5d480e4553888df2ee8.1490309515.git.ed.czeck@atomicrules.com> From: Ed Czeck Date: Tue, 28 Mar 2017 18:38:39 -0400 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org, John Miller , Shepard Siegel , Stephen Hemminger Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v5 1/7] net/ark: PMD for Atomic Rules Arkville driver stub 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: Tue, 28 Mar 2017 22:39:01 -0000 On Tue, Mar 28, 2017 at 10:34 AM, Ferruh Yigit wrote: > > On 3/23/2017 10:59 PM, Ed Czeck wrote: > > Enable Arkville on supported configurations > > Add overview documentation > > Minimum driver support for valid compile > > Arkville PMD is not supported on ARM or PowerPC at this time > > > > v5: > > * Address comments from Ferruh Yigit > > * Added documentation on driver args > > * Makefile fixes > > * Safe argument processing > > * vdev args to dev args > > > > v4: > > * Address issues report from review > > * Add internal comments on driver arg > > * provide a bare-bones dev init to avoid compiler warnings > > > > v3: > > * Split large patch into several smaller ones > > > > Signed-off-by: Ed Czeck > > Signed-off-by: John Miller > > <...> > > > +Device Parameters > > +------------------- > > + > > +The ARK PMD supports a series of parameters that are used for packet routing > > +and for internal packet generation and packet checking. This section describes > > +the supported parameters. These features are primarily used for > > +diagnostics, testing, and performance verification. The nominal use > > +of Arkville does not require any configuration using these parameters. > > + > > +"Pkt_dir" > > + > > +The Packet Director controls connectivity between the Packet Generator, > > +Packet Checker, UDM, DDM, and external ingress and egress interfaces for > > +diagnostic purposes. It includes an internal loopback path from the DDM to the UDM. > > What are UDM and DDM are? These are clarified in the doc, mostly by removing reference to them. > > > > + > > +NOTE: Packets from the packet generator to the UDM are all directed to UDM RX > > +queue 0. Packets looped back from the DDM to the UDM are directed to the same > > +queue number they originated from. > > So a packet generator is part of this PMD. > > > Isn't it overkill for PMD to have packet generator feauture. Is it > really needs to be part of PMD? Can be an option to use external packet > generators? The packet generator is part of the arkville hardware. The configuration file is needed to control it behaviors. Its really not part of the PMD, just a control hook in the PMD to have access into the FPGA hardware. > > > + > > > +The packet generator parameter takes a file as its argument. The file contains configuration > > +parameters used internally for regression testing and are not intended to be published at this > > +level. > > If these config options are not planned to use by public, should the > stripped from public version of the PMD? Is there a benefit to keep them > in dpdk repo? Internally, we have had this discussion. The features/control are very useful for diagnostic and isolating Arkville hardware from customer logic. Our solution is to have a small set of features as device arguments. For the nominal case these will not be used. However, if there is a special case, we would like to have the option available to access them without dropping in a different PMD. > > > > + > > +/* Internal prototypes */ > > +static int eth_ark_check_args(struct ark_adapter *ark, const char *params); > > +static int eth_ark_dev_init(struct rte_eth_dev *dev); > > +static int eth_ark_dev_uninit(struct rte_eth_dev *eth_dev); > > +static int eth_ark_dev_configure(struct rte_eth_dev *dev); > > +static void eth_ark_dev_info_get(struct rte_eth_dev *dev, > > + struct rte_eth_dev_info *dev_info); > > This may be personal, but I am for reordering functions and removing > static function declarations, its your call. I'm leaving the order as is. I like to think that there might be a balance between static prototype declarations and reading the code. > > > > + > > +#define ARK_DEV_TO_PCI(eth_dev) \ > > + RTE_DEV_TO_PCI((eth_dev)->device) > > This macro can go to header file, ark_ethdev.h perhaps? Moved to .h file > > > <...> > > > + > > + if (pci_dev->device.devargs) > > + eth_ark_check_args(ark, pci_dev->device.devargs->args); > > + else > > + PMD_DRV_LOG(INFO, "No Device args found\n"); > > + > > + > > + ark->bar0 = (uint8_t *)pci_dev->mem_resource[0].addr; > > + ark->a_bar = (uint8_t *)pci_dev->mem_resource[2].addr; > > + > > + dev->dev_ops = &ark_eth_dev_ops; > > + > > + /* We process our args last as they require everything to be setup */ > > + if (pci_dev->device.devargs) > > + eth_ark_check_args(ark, pci_dev->device.devargs->args); > > + else > > + PMD_DRV_LOG(INFO, "No Device args found\n"); > > This is duplicate, please check ~10 lines above. Duplicate code removed. > > > Should check the return value of the function, is it OK to continue if > invalid devargs provided by user? Return values are now checked. > > > > + > > + return ret; > > +} > > + > > +static int > > +eth_ark_dev_uninit(struct rte_eth_dev *dev) > > +{ > > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > > + return 0; > > Should primary process check also required in eth_ark_dev_init() ? See comments in patch 7 for a explanation. > > > > + > > + if (rte_kvargs_process(kvlist, > > + ARK_PKTDIR_ARG, > > + &process_pktdir_arg, > > + ark) != 0) { > > + PMD_DRV_LOG(ERR, "Unable to parse arg %s\n", ARK_PKTDIR_ARG); > > No error returned for this case? Error are properly handled in this function and it caller. > > > > +RTE_PMD_REGISTER_PCI(eth_ark, rte_ark_pmd.pci_drv); > > Net device names updated, eth_xxx -> net_xxx. This should be net_ark. > Updated to net_ark Thanks, Ed.