[dpdk-dev] [PATCH v3 16/34] net/ice: support device initialization

Ferruh Yigit ferruh.yigit at intel.com
Wed Dec 12 19:17:48 CET 2018


On 12/12/2018 6:59 AM, Wenzhuo Lu wrote:
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
> Signed-off-by: Qiming Yang <qiming.yang at intel.com>
> Signed-off-by: Xiaoyun Li <xiaoyun.li at intel.com>
> Signed-off-by: Jingjing Wu <jingjing.wu at 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?

> +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?

<...>

> +#
> +# 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.

<...>

> +# 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.

> +
> +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?

<...>

> +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?

<...>

> +RTE_INIT(ice_init_log);
> +static void
> +ice_init_log(void)

Can merge these lines, please check other samples.

> +{
> +	ice_logtype_init = rte_log_register("pmd.ice.init");

pmd.net.ice.init

> +	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

<...>

> +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.

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.

<...>

> +#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?



More information about the dev mailing list