From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 2C8FC2C8 for ; Wed, 28 Jun 2017 17:45:09 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP; 28 Jun 2017 08:45:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,276,1496127600"; d="scan'208";a="1165698243" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.91]) ([10.237.220.91]) by fmsmga001.fm.intel.com with ESMTP; 28 Jun 2017 08:45:07 -0700 To: Shreyansh Jain , dev@dpdk.org Cc: hemant.agrawal@nxp.com References: <1497591668-3320-1-git-send-email-shreyansh.jain@nxp.com> <1497591668-3320-25-git-send-email-shreyansh.jain@nxp.com> From: Ferruh Yigit Message-ID: Date: Wed, 28 Jun 2017 16:45:07 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1497591668-3320-25-git-send-email-shreyansh.jain@nxp.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 24/38] net/dpaa: add support for Tx and Rx queue setup 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: Wed, 28 Jun 2017 15:45:10 -0000 On 6/16/2017 6:40 AM, Shreyansh Jain wrote: > Signed-off-by: Hemant Agrawal > Signed-off-by: Shreyansh Jain > --- > doc/guides/nics/features/dpaa.ini | 1 + > drivers/net/dpaa/Makefile | 4 + > drivers/net/dpaa/dpaa_ethdev.c | 279 ++++++++++++++++++++++++++++++++- > drivers/net/dpaa/dpaa_ethdev.h | 6 + > drivers/net/dpaa/dpaa_rxtx.c | 313 ++++++++++++++++++++++++++++++++++++++ > drivers/net/dpaa/dpaa_rxtx.h | 61 ++++++++ This patch adds initial rx/tx support, as well as rx/tx queue support as mentioned in patch subject. I would be for splitting patch, but even if patch not splitted, I would suggest updating patch suject and commit log to cover patch content. <...> > --- a/doc/guides/nics/features/dpaa.ini > +++ b/doc/guides/nics/features/dpaa.ini > @@ -4,5 +4,6 @@ > ; Refer to default.ini for the full list of available PMD features. > ; > [Features] > +Queue start/stop = Y This requires following dev_ops implemented: rx_queue_start, rx_queue_stop, tx_queue_start, tx_queue_stop > ARMv8 = Y > Usage doc = Y <...> > + > + /* Initialize Rx FQ's */ > + if (getenv("DPAA_NUM_RX_QUEUES")) I think this was disscussed before, should a PMD get config options from enviroment variable? Altough this works, I am for a more explicit method, like dev_args. <...> > + > + dpaa_intf->rx_queues = rte_zmalloc(NULL, > + sizeof(struct qman_fq) * num_rx_fqs, MAX_CACHELINE); A NULL check perhaps? And if multi-process support desired, this should be done only for primary process. <...> > + /* Allocate memory for storing MAC addresses */ > + eth_dev->data->mac_addrs = rte_zmalloc("mac_addr", > + ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER, 0); > + if (eth_dev->data->mac_addrs == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to " > + "store MAC addresses", > + ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER); Anything to cleanup before exit? > + return -ENOMEM; > + } <...> > +uint16_t dpaa_eth_queue_rx(void *q, > + struct rte_mbuf **bufs, > + uint16_t nb_bufs) > +{ > + struct qman_fq *fq = q; > + struct qm_dqrr_entry *dq; > + uint32_t num_rx = 0, ifid = ((struct dpaa_if *)fq->dpaa_intf)->ifid; > + int ret; > + > + ret = rte_dpaa_portal_init((void *)0); > + if (ret) { > + PMD_DRV_LOG(ERR, "Failure in affining portal"); > + return 0; > + } This is rx_pkt_burst function, right? Is it Ok to call rte_dpaa_portal_init() in Rx data path? <...> > + buf = (uint64_t)rte_dpaa_mem_ptov(bufs.addr) - bp_info->meta_data_size; > + if (!buf) > + goto out; goto is not required here. > + > +out: > + return (void *)buf; > +} > + <...> > +uint16_t dpaa_eth_tx_drop_all(void *q __rte_unused, > + struct rte_mbuf **bufs __rte_unused, > + uint16_t nb_bufs __rte_unused) > +{ > + PMD_TX_LOG(DEBUG, "Drop all packets"); Should mbufs freed here? > + > + /* Drop all incoming packets. No need to free packets here > + * because the rte_eth f/w frees up the packets through tx_buffer > + * callback in case this functions returns count less than nb_bufs > + */ > + return 0; > +} <...>