From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 24/38] net/dpaa: add support for Tx and Rx queue setup Date: Thu, 29 Jun 2017 20:25:02 +0530 Message-ID: <0768ce6e-e23d-0562-fe34-b9e8dd908a45@nxp.com> References: <1497591668-3320-1-git-send-email-shreyansh.jain@nxp.com> <1497591668-3320-25-git-send-email-shreyansh.jain@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Ferruh Yigit Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0061.outbound.protection.outlook.com [104.47.40.61]) by dpdk.org (Postfix) with ESMTP id E830E2BE1 for ; Thu, 29 Jun 2017 16:46:16 +0200 (CEST) In-Reply-To: Content-Language: en-US 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 Wednesday 28 June 2017 09:15 PM, Ferruh Yigit wrote: > 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. Ok. I will fix this (splitting if possible, else update to commit). > > <...> >> --- 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 Ok. My understanding here was wrong - I incorrectly matched this to queue setup/teardown. I will remove this feature listing. (and a couple more as per your review comment on other patches). > >> 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. Well, I do remember that discussion and still continued with it because 1) I am not done with that dev_args changes and 2) I think this is more non-intrusive as this is specific to DPAA without need for expanding it towards dev_args (and impacting application arg list). You think this is no-go? If so, I will fix this. > > <...> >> + >> + 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. I will fix both the above. > > <...> >> + /* 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? bad miss from my side. *_queues should be released - I will fix this. (I will run some static analyzer and fix any other similar before next version) > >> + 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? Yes, actually, a portal needs to be initialized if not already - for all I/O operations to succeed. rte_dpaa_portal_init segragates calls if multiple entries are made for initialization. rte_dpaa_portal_init `-> _dpaa_portal_init() if not already initialized > > <...> >> + buf = (uint64_t)rte_dpaa_mem_ptov(bufs.addr) - bp_info->meta_data_size; >> + if (!buf) >> + goto out; > > goto is not required here. :) yes, I will remove this stupid miss. > >> + >> +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 >> + */ Ah, actually I was banking on logic that in case a driver doesn't release memory, the API caller (on getting less than nb_bufs) would do that. This is case for stopped interface. But, I agree, this is dirty fix. I will change this. >> + return 0; >> +} > > <...> > >