From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v5 7/7] net/ark: Arkville PMD component integration Date: Tue, 28 Mar 2017 15:38:13 +0100 Message-ID: References: <41265c4fa76265df0144d5d480e4553888df2ee8.1490309515.git.ed.czeck@atomicrules.com> <3d68039422963c92ef6f05ad4bd9b0b3497eb7bd.1490309515.git.ed.czeck@atomicrules.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: john.miller@atomicrules.com, shepard.siegel@atomicrules.com, stephen@networkplumber.org, Adrien Mazarguil To: Ed Czeck , dev@dpdk.org Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 48EFB29C7 for ; Tue, 28 Mar 2017 16:38:17 +0200 (CEST) In-Reply-To: <3d68039422963c92ef6f05ad4bd9b0b3497eb7bd.1490309515.git.ed.czeck@atomicrules.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 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 <...> > +[Features] > +Queue start/stop = Y > +Jumbo frame = Y > +Scattered Rx = Y > +Basic stats = Y > +Stats per queue = Y > +FW version = Y FW version is not required, as far as I can see, it requires fw_version_get eth_dev_ops implemented. <...> > + /* 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? > + /* > + * We will create additional devices based on the number of requested > + * ports > + */ > + 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? > + > + 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. > + } > + 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. >>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; > + port->eth_dev->data->dev_private = ark; > + port->eth_dev->driver = ark->eth_dev->driver; > + port->eth_dev->dev_ops = ark->eth_dev->dev_ops; > + port->eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst; > + port->eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst; > + > + rte_eth_copy_pci_info(port->eth_dev, pci_dev); > + > + port->eth_dev->data->mac_addrs = > + rte_zmalloc(name, ETHER_ADDR_LEN, 0); > + if (!port->eth_dev->data->mac_addrs) { > + PMD_DRV_LOG(ERR, > + "Memory allocation for MAC failed!" > + " Exiting.\n"); > + goto error; > + } > + ether_addr_copy(&adr, > + &port->eth_dev->data->mac_addrs[0]); > + > + if (ark->user_ext.dev_init) > + ark->user_data = > + ark->user_ext.dev_init(dev, ark->a_bar, p); > + } <...> > static int > eth_ark_dev_uninit(struct rte_eth_dev *dev) > { > + struct ark_adapter *ark = > + (struct ark_adapter *)dev->data->dev_private; > + > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > return 0; > > + if (ark->user_ext.dev_uninit) > + ark->user_ext.dev_uninit(dev, ark->user_data); > + > + ark_pktgen_uninit(ark->pg); > + ark_pktchkr_uninit(ark->pc); > + > dev->dev_ops = NULL; > dev->rx_pkt_burst = NULL; > dev->tx_pkt_burst = NULL; > + if (dev->data->mac_addrs) > + rte_free(dev->data->mac_addrs); > + if (dev->data) > + rte_free(dev->data); > + Shouldn't uninit go thorough all ports ("for (p = 0; p < pc; p++) ") and uninit them all? > return 0; > } > <...> > +/* > + * 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 declerations?