Re: [PATCH v2 00/16] net/zxdh: updated net zxdh driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 21 Feb 2025 10:03:28 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:

> V2:
>   - modify CI some error results(checkpatches warnings、Wrong headline format)
>   - fix warnings when enable extra warnings.
>   - modify apply memcpy script for coccinelle and unnecessary init and
>     unneccessary cast of void when use malloc.
> 
> V1:
>   - updated net zxdh driver.
>     optimize init and some ops.
>     provided csum/lro/tso 、extend stats、fw_version、module_info 、meter, etc.
> 
> Junlong Wang (16):
>   net/zxdh: optimize np dtb channel initialization
>   net/zxdh: optimize queue res alloc/free process
>   net/zxdh: optimize link update process
>   net/zxdh: update Rx/Tx to latest
>   net/zxdh: provided PF/VF msg intr callback
>   net/zxdh: optimize MAC ops
>   net/zxdh: optimize promisc ops
>   net/zxdh: optimize VLAN filter/offload ops
>   net/zxdh: optimize RSS/RETA hash config/update/get
>   net/zxdh: optimize MTU set ops
>   net/zxdh: optimize basic stats ops
>   net/zxdh: provided CSUM/TSO/LRO config
>   net/zxdh: provided rxq/txq info get implementations
>   net/zxdh: provide extended stats ops implementations
>   net/zxdh: provide ptypes FW version EEPROM ops
>   net/zxdh: provide meter ops implementations
> 
>  doc/guides/nics/features/zxdh.ini  |   11 +
>  doc/guides/nics/zxdh.rst           |    5 +
>  drivers/net/zxdh/meson.build       |    1 +
>  drivers/net/zxdh/zxdh_common.c     |   46 +-
>  drivers/net/zxdh/zxdh_common.h     |    3 +
>  drivers/net/zxdh/zxdh_ethdev.c     |  724 +++++++++++++---
>  drivers/net/zxdh/zxdh_ethdev.h     |   69 +-
>  drivers/net/zxdh/zxdh_ethdev_ops.c |  916 ++++++++++++++++++---
>  drivers/net/zxdh/zxdh_ethdev_ops.h |   53 +-
>  drivers/net/zxdh/zxdh_msg.c        |  985 +++++++++++++++++++++-
>  drivers/net/zxdh/zxdh_msg.h        |  175 +++-
>  drivers/net/zxdh/zxdh_mtr.c        | 1223 ++++++++++++++++++++++++++++
>  drivers/net/zxdh/zxdh_mtr.h        |  114 +++
>  drivers/net/zxdh/zxdh_np.c         |  811 ++++++++++++++++--
>  drivers/net/zxdh/zxdh_np.h         |  262 ++++++
>  drivers/net/zxdh/zxdh_pci.c        |   10 -
>  drivers/net/zxdh/zxdh_queue.c      |  132 +--
>  drivers/net/zxdh/zxdh_queue.h      |  118 +--
>  drivers/net/zxdh/zxdh_rxtx.c       |  695 +++++++++-------
>  drivers/net/zxdh/zxdh_rxtx.h       |   27 +
>  drivers/net/zxdh/zxdh_tables.c     |  378 +++++++--
>  drivers/net/zxdh/zxdh_tables.h     |  220 +++--
>  22 files changed, 6040 insertions(+), 938 deletions(-)
>  create mode 100644 drivers/net/zxdh/zxdh_mtr.c
>  create mode 100644 drivers/net/zxdh/zxdh_mtr.h

Still several checklist items, some of these are likely pre-existing conditions.
The one that matters are the build issue with RTE_ASSERT enabled.

Mark items with:
    ✔ passed
    ✘ Failed
    o N/A

Basic hygiene
    ✔ Look at CI results in patchwork
    ✔ Merge cleanly with git am
    ✔ Run checkpatches
      - bug in checkpatch around __rte_packed_begin macros
      - warning from stub

    ✔ Run check-git-log
    ✔ Run check-symbol-maps.sh
    ✔ Run check-doc-vs-code
    ✔ Run check-spdk-tag

Builds
    ✔ Normal Gcc build; make sure driver is built!
    ✔ Use latest experimental Gcc 15 to catch new warnings
    ✔ Clang build using current version (clang-19)
    ✔ Doc build
    o Build for 32 bit x86
    o Cross build for Windows (if applicable)
    ✔ Debug build
    x Enable asserts
[1594/3244] Compiling C object drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_msg.c.o
FAILED: drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_msg.c.o
cc -Idrivers/libtmp_rte_net_zxdh.a.p -Idrivers -I../drivers -Idrivers/net/zxdh -I../drivers/net/zxdh -Ilib/ethdev -I../lib/ethdev -I. -I.. -Iconfig -I../config -Ilib/eal/include -I../lib/eal/include -Ilib/eal/linux/include -I../lib/eal/linux/include -Ilib/eal/x86/include -I../lib/eal/x86/include -I../kernel/linux -Ilib/eal/common -I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs -I../lib/kvargs -Ilib/log -I../lib/log -Ilib/metrics -I../lib/metrics -Ilib/telemetry -I../lib/telemetry -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf -Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter -I../lib/meter -Idrivers/bus/pci -I../drivers/bus/pci -I../drivers/bus/pci/linux -Ilib/pci -I../lib/pci -Idrivers/bus/vdev -I../drivers/bus/vdev -I/usr/include/x86_64-linux-gnu -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=c11 -O3 -include rte_config.h -Wvla -Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned -Wno-missing-field-initializers -D_GNU_SOURCE -DRTE_ENABLE_ASSERT -DRTE_LIBRTE_ETHDEV_DEBUG=1 -fPIC -march=native -mrtm -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation -Wno-address-of-packed-member -Wno-vla -DRTE_LOG_DEFAULT_LOGTYPE=pmd.net.zxdh -MD -MQ drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_msg.c.o -MF drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_msg.c.o.d -o drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_msg.c.o -c ../drivers/net/zxdh/zxdh_msg.c
In file included from ../lib/eal/x86/include/rte_spinlock.h:11,
                 from ../drivers/net/zxdh/zxdh_msg.c:9:
../drivers/net/zxdh/zxdh_msg.c: In function ‘zxdh_vf_promisc_set’:
../drivers/net/zxdh/zxdh_msg.c:1450:41: error: ‘res_info’ undeclared (first use in this function)
 1450 |         RTE_ASSERT(!cfg_data || !hw || !res_info || !res_len);
      |                                         ^~~~~~~~
../lib/eal/include/rte_branch_prediction.h:43:45: note: in definition of macro ‘unlikely’
   43 | #define unlikely(x)     __builtin_expect(!!(x), 0)
      |                                             ^
../lib/eal/include/rte_debug.h:47:25: note: in expansion of macro ‘RTE_VERIFY’
   47 | #define RTE_ASSERT(exp) RTE_VERIFY(exp)
      |                         ^~~~~~~~~~
../drivers/net/zxdh/zxdh_msg.c:1450:9: note: in expansion of macro ‘RTE_ASSERT’
 1450 |         RTE_ASSERT(!cfg_data || !hw || !res_info || !res_len);
      |         ^~~~~~~~~~
../drivers/net/zxdh/zxdh_msg.c:1450:41: note: each undeclared identifier is reported only once for each function it appears in
 1450 |         RTE_ASSERT(!cfg_data || !hw || !res_info || !res_len);
      |                                         ^~~~~~~~
../lib/eal/include/rte_branch_prediction.h:43:45: note: in definition of macro ‘unlikely’
   43 | #define unlikely(x)     __builtin_expect(!!(x), 0)
      |                                             ^
../lib/eal/include/rte_debug.h:47:25: note: in expansion of macro ‘RTE_VERIFY’
   47 | #define RTE_ASSERT(exp) RTE_VERIFY(exp)
      |                         ^~~~~~~~~~
../drivers/net/zxdh/zxdh_msg.c:1450:9: note: in expansion of macro ‘RTE_ASSERT’
 1450 |         RTE_ASSERT(!cfg_data || !hw || !res_info || !res_len);
      |         ^~~~~~~~~~
[1603/3244] Compiling C object drivers/libtmp_rte_net_virtio.a.p/net_virtio_virtio_rxtx.c.o
ninja: build stopped: subcommand failed.

    ✔ Test meson builds

Experimental builds:
    ✘ Enable address sanitizer for undefined checks

[1603/3244] Compiling C object drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_np.c.o
In function ‘zxdh_np_dtb_smmu0_write_entry_data’,
    inlined from ‘zxdh_np_dtb_se_smmu0_ind_write’ at ../drivers/net/zxdh/zxdh_np.c:1104:7,
    inlined from ‘zxdh_np_dtb_eram_one_entry’ at ../drivers/net/zxdh/zxdh_np.c:1181:8,
    inlined from ‘zxdh_np_dtb_table_entry_write’ at ../drivers/net/zxdh/zxdh_np.c:1452:9:
../drivers/net/zxdh/zxdh_np.c:75:12: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
   75 |         if ((rc) != 0) {\
      |            ^
../drivers/net/zxdh/zxdh_np.c:1051:17: note: in expansion of macro ‘ZXDH_COMM_CHECK_RC_NO_ASSERT’
 1051 |                 ZXDH_COMM_CHECK_RC_NO_ASSERT(rc, "dpp_dtb_write_table_cmd");
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/zxdh/zxdh_np.c: In function ‘zxdh_np_dtb_table_entry_write’:
../drivers/net/zxdh/zxdh_np.c:983:26: note: ‘rc’ was declared here
  983 |         uint32_t         rc;
      |                          ^~

    ✘ Enable extra warnings (edit meson.build) for
        -Wvla, -Wformat-truncation, -Waddress-of-packed-member

[1600/3244] Compiling C object drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_queue.c.o
../drivers/net/zxdh/zxdh_queue.c: In function ‘zxdh_dev_rx_queue_setup_finish’:
../drivers/net/zxdh/zxdh_queue.c:357:24: warning: ISO C90 forbids variable length array ‘new_pkts’ [-Wvla]
  357 |                 struct rte_mbuf *new_pkts[free_cnt];
      |                        ^~~~~~~~
[1603/3244] Compiling C object drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_rxtx.c.o
../drivers/net/zxdh/zxdh_rxtx.c: In function ‘zxdh_recv_pkts_packed’:
../drivers/net/zxdh/zxdh_rxtx.c:903:24: warning: ISO C90 forbids variable length array ‘new_pkts’ [-Wvla]
  903 |                 struct rte_mbuf *new_pkts[free_cnt];
      |                        ^~~~~~~~
[1616/3244] Compiling C object drivers/libtmp_rte_net_zxdh.a.p/net_zxdh_zxdh_ethdev_ops.c.o
../drivers/net/zxdh/zxdh_ethdev_ops.c: In function ‘zxdh_dev_xstats_get_names’:
../drivers/net/zxdh/zxdh_ethdev_ops.c:1983:26: warning: ‘%s’ directive output may be truncated writing up to 1359 bytes into a region of size 64 [-Wformat-truncation=]
 1983 |                         "%s", zxdh_np_stat_strings[i].name);
      |                          ^~
../drivers/net/zxdh/zxdh_ethdev_ops.c:1982:25: note: ‘snprintf’ output between 1 and 1360 bytes into a destination of size 64
 1982 |                         snprintf(xstats_names[count].name, sizeof(xstats_names[count].name),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1983 |                         "%s", zxdh_np_stat_strings[i].name);
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/zxdh/zxdh_ethdev_ops.c:1989:34: warning: ‘%s’ directive output may be truncated writing up to 2991 bytes into a region of size 64 [-Wformat-truncation=]
 1989 |                                 "%s", zxdh_mac_stat_strings[i].name);
      |                                  ^~
../drivers/net/zxdh/zxdh_ethdev_ops.c:1988:33: note: ‘snprintf’ output between 1 and 2992 bytes into a destination of size 64
 1988 |                                 snprintf(xstats_names[count].name, sizeof(xstats_names[count].name),
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1989 |                                 "%s", zxdh_mac_stat_strings[i].name);
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/zxdh/zxdh_ethdev_ops.c:1994:34: warning: ‘%s’ directive output may be truncated writing up to 271 bytes into a region of size 64 [-Wformat-truncation=]
 1994 |                                 "%s", zxdh_mac_bytes_strings[i].name);
      |                                  ^~
../drivers/net/zxdh/zxdh_ethdev_ops.c:1993:33: note: ‘snprintf’ output between 1 and 272 bytes into a destination of size 64
 1993 |                                 snprintf(xstats_names[count].name, sizeof(xstats_names[count].name),
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1994 |                                 "%s", zxdh_mac_bytes_strings[i].name);
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/zxdh/zxdh_ethdev_ops.c:2000:26: warning: ‘%s’ directive output may be truncated writing up to 339 bytes into a region of size 64 [-Wformat-truncation=]
 2000 |                         "%s", zxdh_vqm_stat_strings[i].name);
      |                          ^~
../drivers/net/zxdh/zxdh_ethdev_ops.c:1999:25: note: ‘snprintf’ output between 1 and 340 bytes into a destination of size 64
 1999 |                         snprintf(xstats_names[count].name, sizeof(xstats_names[count].name),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2000 |                         "%s", zxdh_vqm_stat_strings[i].name);
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/zxdh/zxdh_ethdev_ops.c:2010:41: warning: ‘%s’ directive output may be truncated writing up to 1359 bytes into a region of size between 54 and 58 [-Wformat-truncation=]
 2010 |                                 "rx_q%u_%s", i, zxdh_rxq_stat_strings[t].name);
      |                                         ^~
../drivers/net/zxdh/zxdh_ethdev_ops.c:2009:33: note: ‘snprintf’ output between 7 and 1370 bytes into a destination of size 64
 2009 |                                 snprintf(xstats_names[count].name, sizeof(xstats_names[count].name),
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2010 |                                 "rx_q%u_%s", i, zxdh_rxq_stat_strings[t].name);
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/zxdh/zxdh_ethdev_ops.c:2022:41: warning: ‘%s’ directive output may be truncated writing up to 1291 bytes into a region of size between 54 and 58 [-Wformat-truncation=]
 2022 |                                 "tx_q%u_%s", i, zxdh_txq_stat_strings[t].name);
      |                                         ^~
../drivers/net/zxdh/zxdh_ethdev_ops.c:2021:33: note: ‘snprintf’ output between 7 and 1302 bytes into a destination of size 64
 2021 |                                 snprintf(xstats_names[count].name, sizeof(xstats_names[count].name),
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2022 |                                 "tx_q%u_%s", i, zxdh_txq_stat_strings[t].name);
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Look for anti-patterns:
    ✔ Do not put brackets on each switch case.
    ✔ Driver must not disable warnings with compiler flags or pragma's
    ✔ Driver must not use thread and signal
    ✔ Driver should not call abort() or assert() directly
    ✔ Review exposed symbol names

    ✔ Apply coccinelle scripts; look that for example null free checks

  - Apply memcpy script for coccinelle
      Lots of fixed size memcpy's that can get fixed later (see other patch series)

    ✔ Review use of malloc
      Several places could use rte_calloc instead of rte_zmalloc (low priority)

    ✘  Review use of memset

Extra memset and cast here:
static int32_t
zxdh_fill_common_msg(struct zxdh_hw *hw, struct zxdh_pci_bar_msg *desc,
		uint8_t type, uint8_t field,
		void *buff, uint16_t buff_size)
{
	uint64_t msg_len = sizeof(struct zxdh_common_msg) + buff_size;

	desc->payload_addr = rte_zmalloc(NULL, msg_len, 0);
	if (unlikely(desc->payload_addr == NULL)) {
		PMD_DRV_LOG(ERR, "Failed to allocate msg_data");
		return -ENOMEM;
	}
	memset(desc->payload_addr, 0, msg_len); // zmalloc memory is already zero!
	desc->payload_len = msg_len;
	// payload_addr is void *, cast not needed
	struct zxdh_common_msg *msg_data = (struct zxdh_common_msg *)desc->payload_addr;