From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH 24/38] net/dpaa: add support for Tx and Rx queue setup Date: Thu, 29 Jun 2017 16:41:18 +0100 Message-ID: <7865dc41-c509-cdec-8c24-c357330f294b@intel.com> References: <1497591668-3320-1-git-send-email-shreyansh.jain@nxp.com> <1497591668-3320-25-git-send-email-shreyansh.jain@nxp.com> <0768ce6e-e23d-0562-fe34-b9e8dd908a45@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, hemant.agrawal@nxp.com To: Shreyansh Jain Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 51FA12B96 for ; Thu, 29 Jun 2017 17:41:53 +0200 (CEST) In-Reply-To: <0768ce6e-e23d-0562-fe34-b9e8dd908a45@nxp.com> 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 6/29/2017 3:55 PM, Shreyansh Jain wrote: > 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 >>> --- <...> >> >>> + >>> + /* 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. Proving argument looks more clear to me, it is more visible, and for example if multiple process will be run, environment variables can be confusing. But this is not no-go, I would like to hear other comments. Also I recognized that mlx and ark drivers are also using this. But however this is implemented, this should be clearly documented, right now this is a hidden config. <...> >>> +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. I missed your logic here indeed, this looks a valid option too, its your call. > >>> + return 0; >>> +} >> >> <...> >> >> >