Re: [dpdk-dev] [PATCH v4 08/11] net/hinic: add hinic PMD build and doc files

On 6/6/2019 12:06 PM, Ziyang Xuan wrote:

quoted

Add build and doc files, and common code files.

Signed-off-by: Ziyang Xuan <redacted>
---
 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?

OK, I will update the release note documentation at next version patchset.

<...>

quoted

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

OK, I will post the link of our product documentation in the hinic.rst at next patchset. 

quoted

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

I will remove it here.

quoted

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

quoted

+
+Requires firmware 1.6.2.5

Please provide some link for this prerequisite.

<...>

quoted

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

The part of icc can be delete. And I will delete the rest of compilation shield options by fix our codes.

quoted

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

quoted

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

OK.

quoted

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

OK.

<...>

quoted

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

I will delete these.

<...>

quoted

+
+void *hinic_dma_mem_zalloc(void *dev, size_t size, dma_addr_t

*dma_handle,

quoted

+			   unsigned int flag, unsigned int align)

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

OK.

<...>

quoted

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

quoted

+	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

OK.

<...>

quoted

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

Please don't have commented code.

I will fix it.

quoted

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

Is hash a dependency?

Yes, we used hash table in the file hinic_pmd_dpdev.c. It is better: deps += ['ethdev', 'pci', 'hash']