DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Ziyang Xuan <xuanziyang2@huawei.com>, dev@dpdk.org
Cc: cloud.wangxiaoyun@huawei.com, zhouguoyang@huawei.com,
	shahar.belkar@huawei.com, stephen@networkplumber.org,
	luoxianjun@huawei.com
Subject: Re: [dpdk-dev] [PATCH v4 08/11] net/hinic: add hinic PMD build and doc files
Date: Tue, 11 Jun 2019 16:56:17 +0100
Message-ID: <8433f9bb-7c3c-97e5-1214-d44851150287@intel.com> (raw)
In-Reply-To: <74886a52875fbfc99e45c9ae9fd6b549cfcb33a6.1559818024.git.xuanziyang2@huawei.com>

On 6/6/2019 12:06 PM, Ziyang Xuan wrote:
> Add build and doc files, and common code files.
> 
> Signed-off-by: Ziyang Xuan <xuanziyang2@huawei.com>
> ---
>  MAINTAINERS                                  |   9 +
>  config/common_base                           |   5 +
>  config/common_linux                          |   5 +
>  config/defconfig_arm-armv7a-linuxapp-gcc     |   1 +
>  config/defconfig_i686-native-linuxapp-gcc    |   5 +
>  config/defconfig_i686-native-linuxapp-icc    |   5 +
>  config/defconfig_ppc_64-power8-linuxapp-gcc  |   1 +
>  config/defconfig_x86_64-native-linuxapp-icc  |   5 +
>  config/defconfig_x86_x32-native-linuxapp-gcc |   5 +
>  doc/guides/nics/features/hinic.ini           |  37 +
>  doc/guides/nics/hinic.rst                    |  55 ++

Can you also update the release notes documentation
(doc/guides/rel_notes/release_19_08.rst) to announce new driver?

<...>

> @@ -0,0 +1,55 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright(c) 2017 Huawei Technologies Co., Ltd
> +
> +
> +HINIC Poll Mode Driver
> +======================
> +
> +The hinic PMD (librte_pmd_hinic) provides poll mode driver support for
> +25 Gbps Huawei Intelligent PCIE Network Adapters based on
> +the Huawei Ethernet Controller Hi1822.

Can you please provide a link to product documentation?

> +
> +
> +Features
> +--------
> +
> +- Multi arch support: x86_64, ARMv8.
> +- Multiple queues for TX and RX
> +- Receiver Side Scaling (RSS)
> +- MAC/VLAN filtering
> +- Checksum offload
> +- TSO offload
> +- Promiscuous mode
> +- Port hardware statistics
> +- Jumbo frames

Jumbo frame marked as not supported in above .ini file?

> +- Link state information
> +- Link flow control
> +- Scattered and gather for TX and RX
> +
> +Prerequisites
> +-------------
> +
> +- Follow the DPDK :ref:`Getting Started Guide for Linux <linux_gsg>` to setup the basic DPDK environment.
> +
> +Requires firmware 1.6.2.5

Please provide some link for this prerequisite.

<...>

> +#
> +# Add extra flags for base driver files (also known as shared code)
> +# to disable warnings
> +#
> +ifeq ($(CONFIG_RTE_TOOLCHAIN_ICC),y)
> +CFLAGS_BASE_DRIVER = -diag-disable 593
> +else
> +CFLAGS_BASE_DRIVER  = -Wno-sign-compare
> +CFLAGS_BASE_DRIVER += -Wno-unused-parameter
> +CFLAGS_BASE_DRIVER += -Wno-strict-aliasing
> +CFLAGS_BASE_DRIVER += -Wno-missing-field-initializers
> +CFLAGS_BASE_DRIVER += -Wno-pointer-to-int-cast
> +endif

Since icc is not supported (yet), can drop this part.

> +
> +OBJS_BASE_DRIVER=$(sort $(patsubst %.c,%.o,$(notdir $(wildcard $(SRCDIR)/base/*.c))))
> +$(foreach obj, $(OBJS_BASE_DRIVER), $(eval CFLAGS_$(obj)+=$(CFLAGS_BASE_DRIVER)))

And when icc part removed, I think nothing is left to set "CFLAGS_BASE_DRIVER",
so this part can be removed too.

> +
> +VPATH += $(SRCDIR)/base
> +
> +#
> +# all source are stored in SRCS-y
> +#
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_api_cmd.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_cfg.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_cmdq.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_eqs.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_hwdev.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_hwif.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_mgmt.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_niccfg.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_nicio.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_qp.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_wq.c
> +
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_dpdev.c
> +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_ethdev.c
> +#SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_rx.c
> +#SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_tx.c

Please remove commented lines.

> +
> +# this lib depends upon:
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_eal lib/librte_ether
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_mempool lib/librte_mbuf
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_net lib/librte_hash
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_kvargs
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_net
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_ring

We stop using DEPDIRS, below LDLIBS is replacing it, can drop above lines.

<...>

> +headers = [
> +	'hinic_compat.h',
> +	'hinic_csr.h',
> +	'hinic_ctx_def.h',
> +	'hinic_pmd_api_cmd.h',
> +	'hinic_pmd_cfg.h',
> +	'hinic_pmd_cmdq.h',
> +	'hinic_pmd_dpdev.h',
> +	'hinic_pmd_eqs.h',
> +	'hinic_pmd_hwdev.h',
> +	'hinic_pmd_hw.h',
> +	'hinic_pmd_hwif.h',
> +	'hinic_pmd_hw_mgmt.h',
> +	'hinic_pmd_mgmt.h',
> +	'hinic_pmd_mgmt_interface.h',
> +	'hinic_pmd_niccfg.h',
> +	'hinic_pmd_nic.h',
> +	'hinic_pmd_nicio.h',
> +	'hinic_pmd_qp.h',
> +	'hinic_pmd_wq.h',
> +	'hinic_port_cmd.h',
> +	'hinic_qe_def.h',
> +]

Does headers need to be defined for the PMD?

<...>

> +
> +void *hinic_dma_mem_zalloc(void *dev, size_t size, dma_addr_t *dma_handle,
> +			   unsigned int flag, unsigned int align)

Why not many of the functions are not 'static'? Please make them 'static' as
much as possible.

<...>

> +int32_t hinic_nic_dev_create(struct rte_eth_dev *eth_dev)
> +{
> +	struct hinic_nic_dev *nic_dev =
> +				HINIC_ETH_DEV_TO_PRIVATE_NIC_DEV(eth_dev);
> +	int rc;
> +
> +	nic_dev->hwdev =
> +		(struct hinic_hwdev *)rte_zmalloc("hinic_hwdev",
> +						  sizeof(*nic_dev->hwdev),
> +						  RTE_CACHE_LINE_SIZE);

There was a patch by Stephen [1] very recently to remove the casting from and to
the "void *" which is not required by 'C'.
To not need similar cleanups later, can you please check them at first place.

[1]https://patches.dpdk.org/project/dpdk/list/?series=4816

<...>

> +sources = files(
> +	'hinic_pmd_dpdev.c',
> +	'hinic_pmd_ethdev.c',
> +#	'hinic_pmd_rx.c',
> +#	'hinic_pmd_tx.c'

Please don't have commented code.

> +	)
> +
> +deps += 'ethdev'
> +deps += 'pci'
> +deps += 'hash'

Is hash a dependency?

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06 11:04 [dpdk-dev] [PATCH v4 00/11] A new net PMD - hinic Ziyang Xuan
2019-06-06 11:05 ` [dpdk-dev] [PATCH v4 05/11] net/hinic/base: add eqs and context code Ziyang Xuan
2019-06-06 11:06 ` [dpdk-dev] [PATCH v4 06/11] net/hinic/base: add code for nic business Ziyang Xuan
2019-06-06 11:06 ` [dpdk-dev] [PATCH v4 08/11] net/hinic: add hinic PMD build and doc files Ziyang Xuan
2019-06-11 15:56   ` Ferruh Yigit [this message]
2019-06-06 11:06 ` [dpdk-dev] [PATCH v4 10/11] net/hinic: add TX module Ziyang Xuan
2019-06-06 11:07 ` [dpdk-dev] [PATCH v4 11/11] net/hinic: add support for basic device operations Ziyang Xuan
2019-06-11 16:02   ` Ferruh Yigit
2019-06-06 11:13 ` [dpdk-dev] [PATCH v4 01/11] net/hinic/base: add registers for Huawei Hi1822 NIC Ziyang Xuan
2019-06-06 11:04   ` Ziyang Xuan
2019-06-06 11:14 ` [dpdk-dev] [PATCH v4 02/11] net/hinic/base: add command channels code Ziyang Xuan
2019-06-06 11:05   ` Ziyang Xuan
2019-06-06 11:15 ` [dpdk-dev] [PATCH v4 03/11] net/hinic/base: add mgmt module interactive code Ziyang Xuan
2019-06-06 11:05   ` Ziyang Xuan
2019-06-06 11:15 ` [dpdk-dev] [PATCH v4 04/11] net/hinic/base: add code about hardware operation Ziyang Xuan
2019-06-06 11:05   ` Ziyang Xuan
2019-06-06 11:17 ` [dpdk-dev] [PATCH v4 07/11] net/hinic/base: add various headers Ziyang Xuan
2019-06-06 11:06   ` Ziyang Xuan
2019-06-11 16:04   ` Ferruh Yigit
2019-06-06 11:18 ` [dpdk-dev] [PATCH v4 09/11] net/hinic: add RX module Ziyang Xuan
2019-06-06 11:06   ` Ziyang Xuan
2019-06-11 15:57   ` Ferruh Yigit
2019-06-12 14:36     ` [dpdk-dev] 答复: " Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions)
2019-06-12 15:00 [dpdk-dev] [PATCH v4 08/11] net/hinic: add hinic PMD build and doc files Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions)

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8433f9bb-7c3c-97e5-1214-d44851150287@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=cloud.wangxiaoyun@huawei.com \
    --cc=dev@dpdk.org \
    --cc=luoxianjun@huawei.com \
    --cc=shahar.belkar@huawei.com \
    --cc=stephen@networkplumber.org \
    --cc=xuanziyang2@huawei.com \
    --cc=zhouguoyang@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox