Hi Ferruh, > -----Original Message----- > From: Yigit, Ferruh > Sent: Thursday, December 13, 2018 2:18 AM > To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org > Cc: Yang, Qiming <qiming.y...@intel.com>; Li, Xiaoyun > <xiaoyun...@intel.com>; Wu, Jingjing <jingjing...@intel.com> > Subject: Re: [dpdk-dev] [PATCH v3 16/34] net/ice: support device > initialization > > On 12/12/2018 6:59 AM, Wenzhuo Lu wrote: > > Signed-off-by: Wenzhuo Lu <wenzhuo...@intel.com> > > Signed-off-by: Qiming Yang <qiming.y...@intel.com> > > Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com> > > Signed-off-by: Jingjing Wu <jingjing...@intel.com> > > <...> > > > @@ -297,6 +297,15 @@ > CONFIG_RTE_LIBRTE_FM10K_RX_OLFLAGS_ENABLE=y > > CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y > > > > # > > +# Compile burst-oriented ICE PMD driver # > CONFIG_RTE_LIBRTE_ICE_PMD=y > > +CONFIG_RTE_LIBRTE_ICE_DEBUG_RX=n > CONFIG_RTE_LIBRTE_ICE_DEBUG_TX=n > > +CONFIG_RTE_LIBRTE_ICE_DEBUG_TX_FREE=n > > +CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC=y > > Is there a way to convert this into runtime config? Does it needs to be > compile time config? If only considering the functionality, we can totally remove this macro. That's why it's set to 'y' by default. We introduce this macro for the users to improve performance for some specific cases. For example, if the MTU is small enough, we can totally remove the code wrapped by this macro. So the performance is better. Considering the purpose, to achieve the best performance, it's hard to make it a runtime configure.
> > > +CONFIG_RTE_LIBRTE_ICE_16BYTE_RX_DESC=n > > Some of these config options documented in ice.rst, but document is > introduced as last patch, what do you think adding documentation as the > feature added? Good suggestion. I'll change it in v4. > > <...> > > > +# > > +# 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 > > +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 > > + > > +ifeq ($(shell test $(GCC_VERSION) -ge 44 && echo 1), 1) > > +CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable endif > > Are all these special warning disable cases for ice? It looks like can be copy > paste from all driver, I suggest starting from empty exception list, we can > add them if we need but lets not start with existing list already. Totally agree, will handle it in v4. > > <...> > > > +# this lib depends upon: > > +DEPDIRS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += lib/librte_eal > > +lib/librte_ether > > +DEPDIRS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += lib/librte_mempool > > +lib/librte_mbuf > > +DEPDIRS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += lib/librte_net > > +DEPDIRS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += lib/librte_kvargs > > As far as I remember we removed DEPDIRS from makefiles, there is no more > dynamic dependency resolving, so it should be safe to remove above lines. I don't understand. This is to handle the compile error in v1, like, https://patches.dpdk.org/patch/48286/ We have to have it. > > > + > > +include $(RTE_SDK)/mk/rte.lib.mk > > diff --git a/drivers/net/ice/ice_ethdev.c > > b/drivers/net/ice/ice_ethdev.c new file mode 100644 index > > 0000000..e0bf15c > > --- /dev/null > > +++ b/drivers/net/ice/ice_ethdev.c > > @@ -0,0 +1,640 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2018 Intel Corporation */ > > + > > +#include <rte_ethdev_pci.h> > > + > > +#include "base/ice_sched.h" > > +#include "ice_ethdev.h" > > +#include "ice_rxtx.h" > > + > > +#define ICE_MAX_QP_NUM "max_queue_pair_num" > > When documentation is added into this patch, can you also add this runtime > config to that please? The macro? It's not a configuration. It’s a string used internally. > > <...> > > > +static int > > +ice_dev_init(struct rte_eth_dev *dev) { > > + struct rte_pci_device *pci_dev; > > + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data- > >dev_private); > > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data- > >dev_private); > > + int ret; > > + > > + dev->dev_ops = &ice_eth_dev_ops; > > + > > + pci_dev = RTE_DEV_TO_PCI(dev->device); > > + > > + rte_eth_copy_pci_info(dev, pci_dev); > > This is done by rte_eth_dev_pci_generic_probe(), do we need here? No, we only need the info, don’t want to probe. > > <...> > > > +RTE_INIT(ice_init_log); > > +static void > > +ice_init_log(void) > > Can merge these lines, please check other samples. Will change it in v4. > > > +{ > > + ice_logtype_init = rte_log_register("pmd.ice.init"); > > pmd.net.ice.init Will change it in v4. > > > + if (ice_logtype_init >= 0) > > + rte_log_set_level(ice_logtype_init, RTE_LOG_NOTICE); > > + ice_logtype_driver = rte_log_register("pmd.ice.driver"); > > pmd.net.ice.driver Will change it in v4. > > <...> > > > +static void > > +ice_dev_close(struct rte_eth_dev *dev) { > > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data- > >dev_private); > > + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data- > >dev_private); > > + > > + ice_res_pool_destroy(&pf->msix_pool); > > + ice_release_vsi(pf->main_vsi); > > + > > + ice_shutdown_all_ctrlq(hw); > > +} > > I am mostly for ordering functions in a way that it doesn't require the > forward declaration, which is mostly helps reading the code since the > function order is close the call order. I just want to align the order with ice_eth_dev_ops. But anyway I can move it forward. > > It is up to you but also for sake of consistancy I think better to move this > function up, and leave probe/remove/init_log functions as last functions in > file. So, it's a bad try to show the strong connection of probe/remove with dev_init/uninit :) I'll move them to the bottom of this file. > > <...> > > > +#define ICE_FLAG_ALL (ICE_FLAG_RSS | \ > > + ICE_FLAG_DCB | \ > > + ICE_FLAG_VMDQ | \ > > + ICE_FLAG_SRIOV | \ > > + ICE_FLAG_HEADER_SPLIT_DISABLED | \ > > + ICE_FLAG_HEADER_SPLIT_ENABLED | \ > > + ICE_FLAG_FDIR | \ > > + ICE_FLAG_VXLAN | \ > > + ICE_FLAG_RSS_AQ_CAPABLE | \ > > + ICE_FLAG_VF_MAC_BY_PF) > > + > > +#define ICE_RSS_OFFLOAD_ALL ( \ > > + ETH_RSS_FRAG_IPV4 | \ > > + ETH_RSS_NONFRAG_IPV4_TCP | \ > > + ETH_RSS_NONFRAG_IPV4_UDP | \ > > + ETH_RSS_NONFRAG_IPV4_SCTP | \ > > + ETH_RSS_NONFRAG_IPV4_OTHER | \ > > + ETH_RSS_FRAG_IPV6 | \ > > + ETH_RSS_NONFRAG_IPV6_TCP | \ > > + ETH_RSS_NONFRAG_IPV6_UDP | \ > > + ETH_RSS_NONFRAG_IPV6_SCTP | \ > > + ETH_RSS_NONFRAG_IPV6_OTHER | \ > > + ETH_RSS_L2_PAYLOAD) > > ICE_RSS_OFFLOAD_ALL is not used at all until this patchset. I think it makes > more logical to add code when it is added, otherwise it is hard to have a > complete logic in signle patch and harder to observe any possible issue. > What do you think re-arranging them? Sure. I'll move it to another patch.
