Re: [PATCH v6 01/26] drivers/net: introduce a new PMD driver

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/30/2021 6:08 AM, Yanling Song wrote:
> Introduce a new PMD driver which names spnic.

PMD stands for "Poll mode driver", so "PMD driver" usage is wrong,
can you please update it in the patch title too?

Also domain for patch title can be "net/spnic: ".

> Now, this driver only implements module entry
> without doing anything else.
> 
> Signed-off-by: Yanling Song <songyl@ramaxel.com>
> ---
>   drivers/net/meson.build               |   1 +
>   drivers/net/spnic/base/meson.build    |  26 ++++
>   drivers/net/spnic/base/spnic_compat.h | 184 ++++++++++++++++++++++++++
>   drivers/net/spnic/meson.build         |  17 +++
>   drivers/net/spnic/spnic_ethdev.c      | 107 +++++++++++++++
>   drivers/net/spnic/spnic_ethdev.h      |  28 ++++
>   drivers/net/spnic/version.map         |   3 +
>   7 files changed, 366 insertions(+)
>   create mode 100644 drivers/net/spnic/base/meson.build
>   create mode 100644 drivers/net/spnic/base/spnic_compat.h
>   create mode 100644 drivers/net/spnic/meson.build
>   create mode 100644 drivers/net/spnic/spnic_ethdev.c
>   create mode 100644 drivers/net/spnic/spnic_ethdev.h
>   create mode 100644 drivers/net/spnic/version.map


Can you please add/update following files in this first patch:
- MAINTAINERS (sorted by vendor name)
- doc/guides/nics/spnic.rst
- doc/guides/nics/features/spnic.ini
- doc/guides/nics/index.rst
- doc/guides/rel_notes/release_22_03.rst

Most of them are already in the patch 25/26, so please squash it in this patch,
and I put some comment on the documentation.

> 
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index 2355d1cde8..a5c715f59c 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -53,6 +53,7 @@ drivers = [
>           'ring',
>           'sfc',
>           'softnic',
> +	'spnic',

The indentation in the meson files needs to be fixed, please run 'check-meson.py'
Comment is valid for all meson file updates in other patches.

$ ./devtools/check-meson.py
Error parsing drivers/net/meson.build:54, got some tabulation
Error: Incorrect indent at drivers/net/meson.build:55
Error parsing drivers/net/spnic/meson.build:13, got some tabulation
Error: Incorrect indent at drivers/net/spnic/meson.build:14
Error parsing drivers/net/spnic/meson.build:14, got some tabulation
Error parsing drivers/net/spnic/base/meson.build:23, got some tabulation
Error parsing drivers/net/spnic/base/meson.build:24, got some tabulation


>           'tap',
>           'thunderx',
>           'txgbe',
> diff --git a/drivers/net/spnic/base/meson.build b/drivers/net/spnic/base/meson.build
> new file mode 100644
> index 0000000000..e83a473881
> --- /dev/null
> +++ b/drivers/net/spnic/base/meson.build
> @@ -0,0 +1,26 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2021 Ramaxel Memory Technology, Ltd
> +
> +sources = [
> +]
> +
> +extra_flags = []
> +# The driver runs only on arch64 machine, remove 32bit warnings
> +if not dpdk_conf.get('RTE_ARCH_64')
> +        extra_flags += ['-Wno-int-to-pointer-cast', '-Wno-pointer-to-int-cast']
> +endif

Why disabling compiler warning even without having any source file.
Please start with empty meson file, and develop it as needed.

> +
> +foreach flag: extra_flags
> +        if cc.has_argument(flag)
> +                cflags += flag
> +        endif
> +endforeach
> +
> +deps += ['hash']
> +cflags += ['-DHW_CONVERT_ENDIAN']
> +c_args = cflags

ditto

> +
> +base_lib = static_library('spnic_base', sources,
> +	dependencies: [static_rte_eal, static_rte_ethdev, static_rte_bus_pci, static_rte_hash],
> +	c_args: c_args)
> +base_objs = base_lib.extract_all_objects()
> diff --git a/drivers/net/spnic/base/spnic_compat.h b/drivers/net/spnic/base/spnic_compat.h
> new file mode 100644
> index 0000000000..97f817cba9
> --- /dev/null
> +++ b/drivers/net/spnic/base/spnic_compat.h
> @@ -0,0 +1,184 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Ramaxel Memory Technology, Ltd
> + */
> +
> +#ifndef _SPNIC_COMPAT_H_
> +#define _SPNIC_COMPAT_H_
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <sys/time.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +#include <rte_common.h>
> +#include <rte_byteorder.h>
> +#include <rte_memzone.h>
> +#include <rte_memcpy.h>
> +#include <rte_malloc.h>
> +#include <rte_atomic.h>
> +#include <rte_spinlock.h>
> +#include <rte_cycles.h>
> +#include <rte_log.h>
> +#include <rte_config.h>
> +#include <rte_io.h>
> +

Do you need to include all these headers at this stage?
Can you please add them as needed?

> +typedef uint8_t   u8;
> +typedef int8_t    s8;
> +typedef uint16_t  u16;
> +typedef uint32_t  u32;
> +typedef int32_t   s32;
> +typedef uint64_t  u64;
> +
> +#ifndef BIT
> +#define BIT(n) (1 << (n))
> +#endif
> +

There is already RTE_BIT64 / RTE_BIT32 in DPDK, those can be used.


> +#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
> +#define lower_32_bits(n) ((u32)(n))
> +
> +#define SPNIC_MEM_ALLOC_ALIGN_MIN	1
> +
> +#define SPNIC_DRIVER_NAME "spnic"
> +
> +extern int spnic_logtype;
> +
> +#define PMD_DRV_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, spnic_logtype, \
> +		SPNIC_DRIVER_NAME ": " fmt "\n", ##args)
> +
> +/* Bit order interface */
> +#define cpu_to_be16(o) rte_cpu_to_be_16(o)
> +#define cpu_to_be32(o) rte_cpu_to_be_32(o)
> +#define cpu_to_be64(o) rte_cpu_to_be_64(o)
> +#define cpu_to_le32(o) rte_cpu_to_le_32(o)
> +#define be16_to_cpu(o) rte_be_to_cpu_16(o)
> +#define be32_to_cpu(o) rte_be_to_cpu_32(o)
> +#define be64_to_cpu(o) rte_be_to_cpu_64(o)
> +#define le32_to_cpu(o) rte_le_to_cpu_32(o)
> +
> +#define ARRAY_LEN(arr) ((sizeof(arr) / sizeof((arr)[0])))
> +

There is DPDK 'RTE_DIM' for it.

> +#define SPNIC_MUTEX_TIMEOUT	10
> +#define SPNIC_S_TO_MS_UNIT	1000
> +#define SPNIC_S_TO_NS_UNIT	1000000
> +
> +static inline unsigned long clock_gettime_ms(void)
> +{
> +	struct timespec tv;
> +
> +	(void)clock_gettime(CLOCK_MONOTONIC_COARSE, &tv);
> +
> +	return (unsigned long)tv.tv_sec * SPNIC_S_TO_MS_UNIT +
> +	       (unsigned long)tv.tv_nsec / SPNIC_S_TO_NS_UNIT;
> +}
> +
> +#define jiffies	clock_gettime_ms()

Is 'jiffies' used at all?

<...>

> +
> +RTE_INIT(spnic_init_log)
> +{
> +	spnic_logtype = rte_log_register("pmd.net.spnic");
> +	if (spnic_logtype >= 0)
> +		rte_log_set_level(spnic_logtype, RTE_LOG_INFO);
> +}

Can you please use 'RTE_LOG_REGISTER_DEFAULT' macro instead?

<...>