From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 4D71D200 for ; Wed, 22 Nov 2017 01:02:08 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Nov 2017 16:02:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,432,1505804400"; d="scan'208";a="4997117" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.224.243]) ([10.241.224.243]) by orsmga003.jf.intel.com with ESMTP; 21 Nov 2017 16:02:07 -0800 To: Jingjing Wu , dev@dpdk.org Cc: wenzhuo.lu@intel.com References: <1508488012-82704-1-git-send-email-jingjing.wu@intel.com> <1508488012-82704-3-git-send-email-jingjing.wu@intel.com> From: Ferruh Yigit Message-ID: <375d4591-060b-befa-73c9-7d0d84bf031f@intel.com> Date: Tue, 21 Nov 2017 16:02:06 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1508488012-82704-3-git-send-email-jingjing.wu@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC 2/9] net/avf: initilization of avf PMD 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, 22 Nov 2017 00:02:08 -0000 On 10/20/2017 1:26 AM, Jingjing Wu wrote: > Signed-off-by: Jingjing Wu <...> > # > +# Compile burst-oriented AVF PMD driver > +# > +CONFIG_RTE_LIBRTE_AVF_PMD=y Lets start PMD disabled and enable it after it become functional. If you need to run a git bisect in the future on this commit, and you have a AVF supported device. Device will be probed but since this patch is missing avf_eth_dev_ops, I am not sure how app behaves, it may fail or crash, and you can't complete git bisect run because of this patch. <...> > @@ -69,6 +69,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e > DEPDIRS-i40e = $(core-libs) librte_hash > DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe > DEPDIRS-ixgbe = $(core-libs) librte_hash > +DIRS-$(CONFIG_RTE_LIBRTE_AVF_PMD) += avf Can you please add this in alphabetically sorted manner? > +DEPDIRS-avf = $(core-libs) This is changed in prev release, DEPDIRS removed and library dependency part moved to individual driver files as LDLIBS variable. <...> > +# > +# Add extra flags for base driver files (also known as shared code) > +# to disable warnings > +# > +ifeq ($(CONFIG_RTE_TOOLCHAIN_ICC),y) > +CFLAGS_BASE_DRIVER = -wd593 -wd188 > +else ifeq ($(CONFIG_RTE_TOOLCHAIN_CLANG),y) > +CFLAGS_BASE_DRIVER += -Wno-sign-compare > +CFLAGS_BASE_DRIVER += -Wno-unused-value > +CFLAGS_BASE_DRIVER += -Wno-unused-parameter > +CFLAGS_BASE_DRIVER += -Wno-strict-aliasing > +CFLAGS_BASE_DRIVER += -Wno-format > +CFLAGS_BASE_DRIVER += -Wno-missing-field-initializers > +CFLAGS_BASE_DRIVER += -Wno-pointer-to-int-cast > +CFLAGS_BASE_DRIVER += -Wno-format-nonliteral > +CFLAGS_BASE_DRIVER += -Wno-unused-variable Lots of these common for clang and gcc, can it be possible to remove duplication? > +else > +CFLAGS_BASE_DRIVER = -Wno-sign-compare > +CFLAGS_BASE_DRIVER += -Wno-unused-value > +CFLAGS_BASE_DRIVER += -Wno-unused-parameter > +CFLAGS_BASE_DRIVER += -Wno-strict-aliasing > +CFLAGS_BASE_DRIVER += -Wno-format > +CFLAGS_BASE_DRIVER += -Wno-missing-field-initializers > +CFLAGS_BASE_DRIVER += -Wno-pointer-to-int-cast > +CFLAGS_BASE_DRIVER += -Wno-format-nonliteral > +CFLAGS_BASE_DRIVER += -Wno-format-security > +CFLAGS_BASE_DRIVER += -Wno-unused-variable Are these options to remove warnings specific to this driver? Looks like copy-paste from old driver. I believe we should reduce number of disabled compiler warning as much as possible, what do you think removing them all and add back if it is needed? This may cause a few compile fix patches but if they are send before integration deadline, they can be squashed in next-net. <...> > +static int > +avf_init_vf(struct rte_eth_dev *dev) > +{ > + int i, err, bufsz; > + struct avf_adapter *adapter = > + AVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private); > + struct avf_hw *hw = AVF_DEV_PRIVATE_TO_HW(dev->data->dev_private); > + struct avf_info *vf = AVF_DEV_PRIVATE_TO_VF(dev->data->dev_private); > + uint16_t interval = > + avf_calc_itr_interval(AVF_QUEUE_ITR_INTERVAL_MAX); > + > + err = avf_set_mac_type(hw); > + if (err) { > + PMD_INIT_LOG(ERR, "set_mac_type failed: %d", err); You may want to dynamically register log type and level (rte_log_register, rte_log_set_level) for avf_logtype_init & avf_logtype_driver before start using loggig functions. <...> > +/* > + * virtual function driver struct > + */ > +static struct rte_pci_driver rte_avf_pmd = { > + .id_table = pci_id_avf_map, > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING, RTE_PCI_DRV_IOVA_AS_VA ? As it has been added to i40e vf driver. <...> > diff --git a/drivers/net/avf/rte_pmd_avf_version.map b/drivers/net/avf/rte_pmd_avf_version.map > new file mode 100644 > index 0000000..a70bd19 > --- /dev/null > +++ b/drivers/net/avf/rte_pmd_avf_version.map > @@ -0,0 +1,4 @@ > +DPDK_17.11 { Needs to be changed for new release. <...>