From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f169.google.com (mail-qt0-f169.google.com [209.85.216.169]) by dpdk.org (Postfix) with ESMTP id 44BB328F3 for ; Wed, 29 Mar 2017 00:42:59 +0200 (CEST) Received: by mail-qt0-f169.google.com with SMTP id x35so77074247qtc.2 for ; Tue, 28 Mar 2017 15:42:59 -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=+l6ikVu92lMcDc5gahEIB+j3eov5q2TxJlhzQVQdu6k=; b=D2OLzliSVcz0X+urpxtY8L2l2xpwPIYheKEeDHp2zkU6Nh6/ceTIZ1KmtV9EgbtZIH NR6MdHdAgiB7hJy90EhvGnVvd3nwSn9xidVR1nnpMDclsqBEPMWXJDOPsTQmaZ7HFVoZ punJ8Ueh5RjYYEJVbeCVwTNgaoF23JQG1TXVFSiFjzUi+2MLBXMmwB6mvf+az9C9XuLl 5GKwyb8DwAW6VAxDM+RgiQjLo/SYP8E3jwid0ov0nuWEbaSr/rqAGfVwYbYfFhZfDGYB XJ3JrG6Rf8nb4dqEyqHBoTNqR+ywuBfesSotVoPOVC21KXslR0BCA80biKAv7/FBLpjO tWhA== 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=+l6ikVu92lMcDc5gahEIB+j3eov5q2TxJlhzQVQdu6k=; b=NdpcxC3oy4tD8O0kT/O8wx9YstRm3g5AY9G/sqpC0mduJUaijPpNE+NsgAyQ4d5c/m YSZILZ+Ye1/kfAJ9sGLNaz4dSujU0GevcP727MB9FHQkUWhz8XvsvGpjVbTwp6CB4Sks 6Oso0lUGre+MaKa1LHBII/GM1zNuIZJtgwjfY5Muys9ff0deuETUiLWv9eysRxkwPCYY mIXuUWCenp+4W+HbYIWMqds1pUIc4vcYyYUveayv21OPGQgtc0BxjYOKOm4YMkm7w2qX eOzQEAt1t7y34Rc0gzqyXeARBnxSb1PYkJnu1rJO2/0c74oipDhnavpQ6+yaCDxUU6ca 8DTA== X-Gm-Message-State: AFeK/H0+klhVqti0d/Kkp5lWGnfOVC7jGEnPiZsHtviVDavnuQjvv1yeseuXN1eJtMlwZRYc91nXSNcxdqaa7Q== X-Received: by 10.200.45.194 with SMTP id q2mr30851608qta.130.1490740978538; Tue, 28 Mar 2017 15:42:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.200.1.131 with HTTP; Tue, 28 Mar 2017 15:42:38 -0700 (PDT) In-Reply-To: References: <41265c4fa76265df0144d5d480e4553888df2ee8.1490309515.git.ed.czeck@atomicrules.com> <3d68039422963c92ef6f05ad4bd9b0b3497eb7bd.1490309515.git.ed.czeck@atomicrules.com> From: Ed Czeck Date: Tue, 28 Mar 2017 18:42:38 -0400 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org, John Miller , Shepard Siegel , Stephen Hemminger , Adrien Mazarguil Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v5 7/7] net/ark: Arkville PMD component integration 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:42:59 -0000 On Tue, Mar 28, 2017 at 10:38 AM, Ferruh Yigit wrote: > On 3/23/2017 11:01 PM, Ed Czeck wrote: > > * Flesh out device configuration > > * Add links dev_ops > > * allow dynamic extension loading > > > > Signed-off-by: Shepard Siegel > > Signed-off-by: John Miller > > Signed-off-by: Ed Czeck > > <...> > > > +FW version = Y > > FW version is not required, as far as I can see, it requires > fw_version_get eth_dev_ops implemented. > Entry removed. > > <...> > > > + /* We are a single function multi-port device. */ > > + const unsigned int numa_node = rte_socket_id(); > > + struct ether_addr adr; > > + > > + ret = ark_config_device(dev); > > dev->dev_ops = &ark_eth_dev_ops; > > > > + dev->data->mac_addrs = rte_zmalloc("ark", ETHER_ADDR_LEN, 0); > > + if (!dev->data->mac_addrs) { > > + PMD_DRV_LOG(ERR, > > + "Failed to allocated memory for storing mac > address" > > + ); > > + } > > + ether_addr_copy((struct ether_addr *)&adr, > &dev->data->mac_addrs[0]); > > "adr" has random value at this point, right? Why to copy it? > Code removed. The mac address is left as 0, since arkville does not include a mac. > > + int pc = 1; > > + int p; > > I am aware some people prefer the declaring variables close to context, > which is good idea, but if I remember correct, there was a patchset, > from Adrien, to make DPDK C99 compatible, will this break it? > I moved the declarations to the top of the function to match the dpdk style. > > + > > + if (ark->user_ext.dev_get_port_count) { > > + pc = ark->user_ext.dev_get_port_count(dev, > ark->user_data); > > + ark->num_ports = pc; > > + } else { > > + ark->num_ports = 1; > > Because pc has default value of "1", this if statement can be simplified. > Code has been simplified to 3 lines. > > > + } > > + for (p = 0; p < pc; p++) { > > + struct ark_port *port; > > + > > + port = &ark->port[p]; > > + struct rte_eth_dev_data *data = NULL; > > + > > + port->id = p; > > + > > + char name[RTE_ETH_NAME_MAX_LEN]; > > + > > + snprintf(name, sizeof(name), "arketh%d", > > + dev->data->port_id + p); > > + > > + if (p == 0) { > > + /* First port is already allocated by DPDK */ > > + port->eth_dev = ark->eth_dev; > > + continue; > > + } > > + > > + /* reserve an ethdev entry */ > > + port->eth_dev = rte_eth_dev_allocate(name); > > + if (!port->eth_dev) { > > + PMD_DRV_LOG(ERR, > > + "Could not allocate eth_dev for port > %d\n", > > + p); > > + goto error; > > + } > > + > > + data = rte_zmalloc_socket(name, sizeof(*data), 0, > numa_node); > > + if (!data) { > > + PMD_DRV_LOG(ERR, > > + "Could not allocate eth_dev for port > %d\n", > > + p); > > + goto error; > > + } > > + data->port_id = ark->eth_dev->data->port_id + p; > > "ark->eth_dev->data->port_id" is port_id of the first physical ARK > device, and it looks like each device may have multiple ports and "p" is > the port_id within same device. > Arkville is a single PCIE function with 1 or more ports, lets call this p. After initialization each there will be p additional devices, and each device will have exactly 1 port. We now understand the features of the rte_eth_dev_allocate(), and have simplified this code avoid the unnecessary copies. > > From DPDK point of view, port_id is a global value incremented one by > each eth port, so port_id is a unique value, why adding these two values? > > > + port->eth_dev->data = data; > > Why overwriting existing data value? > > > + port->eth_dev->device = &pci_dev->device; > > + > > > + ark->user_ext.dev_init(dev, ark->a_bar, p); > > + } > Unnecessary coping has been removed. > > <...> > > > static int > > eth_ark_dev_uninit(struct rte_eth_dev *dev) > > { > > > Shouldn't uninit go thorough all ports ("for (p = 0; p < pc; p++) ") and > uninit them all? > Arkville has one PCIE function, during initialization we determine the number of ports, and from that one device for each port will be created. At the end, there will be p devices, each with one port. So during device uninit, there is only 1 port. > > > <...> > > > +/* > > + * The following functions are optional and are directly mapped > > + * from the DPDK PMD ops structure. > > + * Each function if implemented is called after the ARK PMD > > + * implementation executes. > > + */ > > +int dev_configure(struct rte_eth_dev *dev, void *user_data); > > +int dev_start(struct rte_eth_dev *dev, void *user_data); > > +void dev_stop(struct rte_eth_dev *dev, void *user_data); > > +void dev_close(struct rte_eth_dev *dev, void *user_data); > > +int link_update(struct rte_eth_dev *dev, int wait_to_complete, > > + void *user_data); > > +int dev_set_link_up(struct rte_eth_dev *dev, void *user_data); > > +int dev_set_link_down(struct rte_eth_dev *dev, void *user_data); > > +void stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats, > > + void *user_data); > > +void stats_reset(struct rte_eth_dev *dev, void *user_data); > > +void mac_addr_add(struct rte_eth_dev *dev, > > + struct ether_addr *macadr, > > + uint32_t index, > > + uint32_t pool, > > + void *user_data); > > +void mac_addr_remove(struct rte_eth_dev *dev, uint32_t index, void > *user_data); > > +void mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr, > > + void *user_data); > > Where these functions are implemented? Do we need these declarations? > The following comment has been added to this file, which addresses your question. /* * This is the template file for users who which to define a dynamic * extension to the Arkville PMD. User's who create an extension * should include this file and define the necessary and desired * functions. * Only 1 function is required for an extension, dev_init(); all other * functions prototyped in this file are optional. */ Thanks Ed.