From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ed Czeck Subject: Re: [PATCH v4 1/7] net/ark: PMD for Atomic Rules Arkville driver stub Date: Thu, 23 Mar 2017 15:46:13 -0400 Message-ID: References: <1490231015-31748-1-git-send-email-ed.czeck@atomicrules.com> <0d1c21b6-6163-12f0-cbe4-d23c06a809e9@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: dev@dpdk.org To: Ferruh Yigit Return-path: Received: from mail-qk0-f179.google.com (mail-qk0-f179.google.com [209.85.220.179]) by dpdk.org (Postfix) with ESMTP id 8D9241B53 for ; Thu, 23 Mar 2017 20:46:34 +0100 (CET) Received: by mail-qk0-f179.google.com with SMTP id p22so50112537qka.3 for ; Thu, 23 Mar 2017 12:46:34 -0700 (PDT) In-Reply-To: <0d1c21b6-6163-12f0-cbe4-d23c06a809e9@intel.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" In the next patch set, v5 the following are addressed. > > > > +CONFIG_RTE_LIBRTE_ARK_PMD=n > Is it not tested or known that it is not supported? > > CONFIG_RTE_LIBRTE_IXGBE_PMD=n > > CONFIG_RTE_LIBRTE_I40E_PMD=n > > CONFIG_RTE_LIBRTE_VIRTIO_PMD=y The Ark/PMD is not supported on those architectures at this time. This is also reflected in doc/guides/nic/ark.rst > Can you also please document the device arguments in this file? > Device Parameters > ----------------- > "Pkt_gen" > "Pkt_chkr" > "Pkt_dir" doc/guides/nic/ark.rst has been update to include documentation for these arguments. > > diff --git a/doc/guides/nics/features/ark.ini b/doc/guides/nics/features/ark.ini > ... > Features can be added with the patch that adds functionality. I believe > above features not supported with current patch. The ark.ini file has been removed in this patch and will be added in a later patch > > +# > > +SRCS-y += ark_ethdev.c > Please use SRCS-y only in comment, for actual usage please prefer: > SRCS-$(CONFIG_RTE_LIBRTE_ARK_PMD) Changed per request. > > > +#define ARK_TRACE_ON(fmt, ...) \ > > + PMD_DRV_LOG(ERR, fmt, ##__VA_ARGS__) > > + > > +#define ARK_TRACE_OFF(fmt, ...) \ > > + do {if (0) PMD_DRV_LOG(ERR, fmt, ##__VA_ARGS__); } while (0) > why not just "do { } while(0)" ? A do while body always executes at least once. The if (0) is required. > This is debug option but prints only "ERR" level log, shouldn't this be > DEBUG. > Also there are dpdk wide log level option, helps optimizing out some > code, if you use only ERR type, you won't be benefiting from it. Changed to DEBUG > > __rte_unused not required. Removed. > > + ARK_DEBUG_TRACE("eth_ark_dev_init(struct rte_eth_dev *dev)\n"); > > + gark[0] = ark; > Is it OK to have this hardcoded index "0"? When there are two instance > of this device, this value be overwritten by second one. Code refactored moving this variable from global scope to instance specific. > dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE; I do not understand your comment. > > memset not required, since already done by ethdev abstraction layer, > specially desc_lim values already overwritten below. Deleted. > > > + dev_info->max_rx_pktlen = (16 * 1024) - 128; > > + dev_info->min_rx_bufsize = 1024; > Using some macros instead of hardcoded values helps to understand values. Done > > > The general usage of DEBUG_TRACE is providing backtrace log, function > enterance / exit informations. I guess, that is why it has been > controlled by different config option. > Here what you need looks like regular debugging functions, PMD_DRV_LOG / > RTE_LOG variant. I'm unclear on your request or comment. Are you suggesting that we use the dpdk versions of debug? The advantage of a local version is that they can be enabled without all the debug traces. > Can this overflow args variable? Any way to prevent possible crash? > What happens if somebody, by accident, provides a file as device > argument which is larger than 256 bytes? Code has been fixed to avoid overflow. >> > +/* >> >> > + * Although Arkville is a physical device we take advantage of the virtual >> >> > + * device initialization as a per test runtime initialization for >> >> > + * regression testing. Parameters are passed into the virtual device to >> >> > + * configure the packet generator, packet director and packet checker. >> >> > + */ >> >> Why not providing these arguments via physical device? Code has been changed to use dev arg instead of vdev args. >> +++ b/drivers/net/ark/rte_pmd_ark_version.map > s/DPDK_2.0/DPDK_17.05 Fixed.