Re: [PATCH v4] net/zxdh: Provided zxdh basic init

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

On 9/10/2024 1:00 PM, Junlong Wang wrote:
> provided zxdh initialization of zxdh PMD driver.
> include msg channel, np init and etc.
> 

Hi Junlong,

It is very hard to review the driver as a single patch, it helps to
split driver into multiple patches, please check the suggestion on it:
https://patches.dpdk.org/project/dpdk/patch/20240916162856.11566-1-stephen@networkplumber.org/

Also there are errors reported by scripts, please fix them:
./devtools/checkpatches.sh -n1
./devtools/check-meson.py


> Signed-off-by: Junlong Wang <wang.junlong1@zte.com.cn>
> ---
> V4: Resolve compilation issues
> V3: Resolve compilation issues
> V2: Resolve compilation issues and modify doc(zxdh.ini zdh.rst)
> V1: Provide zxdh basic init and open source NPSDK lib
> ---
>  doc/guides/nics/features/zxdh.ini |   10 +
>  doc/guides/nics/index.rst         |    1 +
>  doc/guides/nics/zxdh.rst          |   34 +
>  drivers/net/meson.build           |    1 +
>  drivers/net/zxdh/meson.build      |   23 +
>  drivers/net/zxdh/zxdh_common.c    |   59 ++
>  drivers/net/zxdh/zxdh_common.h    |   32 +
>  drivers/net/zxdh/zxdh_ethdev.c    | 1328 +++++++++++++++++++++++++++++
>  drivers/net/zxdh/zxdh_ethdev.h    |  202 +++++
>  drivers/net/zxdh/zxdh_logs.h      |   38 +
>  drivers/net/zxdh/zxdh_msg.c       | 1177 +++++++++++++++++++++++++
>  drivers/net/zxdh/zxdh_msg.h       |  408 +++++++++
>  drivers/net/zxdh/zxdh_npsdk.c     |  158 ++++
>  drivers/net/zxdh/zxdh_npsdk.h     |  216 +++++
>  drivers/net/zxdh/zxdh_pci.c       |  462 ++++++++++
>  drivers/net/zxdh/zxdh_pci.h       |  259 ++++++
>  drivers/net/zxdh/zxdh_queue.c     |  138 +++
>  drivers/net/zxdh/zxdh_queue.h     |   85 ++
>  drivers/net/zxdh/zxdh_ring.h      |   87 ++
>  drivers/net/zxdh/zxdh_rxtx.h      |   48 ++
>  20 files changed, 4766 insertions(+)
>  create mode 100644 doc/guides/nics/features/zxdh.ini
>  create mode 100644 doc/guides/nics/zxdh.rst
>  create mode 100644 drivers/net/zxdh/meson.build
>  create mode 100644 drivers/net/zxdh/zxdh_common.c
>  create mode 100644 drivers/net/zxdh/zxdh_common.h
>  create mode 100644 drivers/net/zxdh/zxdh_ethdev.c
>  create mode 100644 drivers/net/zxdh/zxdh_ethdev.h
>  create mode 100644 drivers/net/zxdh/zxdh_logs.h
>  create mode 100644 drivers/net/zxdh/zxdh_msg.c
>  create mode 100644 drivers/net/zxdh/zxdh_msg.h
>  create mode 100644 drivers/net/zxdh/zxdh_npsdk.c
>  create mode 100644 drivers/net/zxdh/zxdh_npsdk.h
>  create mode 100644 drivers/net/zxdh/zxdh_pci.c
>  create mode 100644 drivers/net/zxdh/zxdh_pci.h
>  create mode 100644 drivers/net/zxdh/zxdh_queue.c
>  create mode 100644 drivers/net/zxdh/zxdh_queue.h
>  create mode 100644 drivers/net/zxdh/zxdh_ring.h
>  create mode 100644 drivers/net/zxdh/zxdh_rxtx.h
> 
> diff --git a/doc/guides/nics/features/zxdh.ini b/doc/guides/nics/
> features/zxdh.ini
> new file mode 100644
> index 0000000000..083c75511b
> --- /dev/null
> +++ b/doc/guides/nics/features/zxdh.ini
> @@ -0,0 +1,10 @@
> +;
> +; Supported features of the 'zxdh' network poll mode driver.
> +;
> +; Refer to default.ini for the full list of available PMD features.
> +;
> +[Features]
> +Linux                = Y
> +x86-64               = Y
> +ARMv8                = Y
> +
> diff --git a/doc/guides/nics/index.rst b/doc/guides/nics/index.rst
> index c14bc7988a..8e371ac4a5 100644
> --- a/doc/guides/nics/index.rst
> +++ b/doc/guides/nics/index.rst
> @@ -69,3 +69,4 @@ Network Interface Controller Drivers
>      vhost
>      virtio
>      vmxnet3
> +    zxdh
> diff --git a/doc/guides/nics/zxdh.rst b/doc/guides/nics/zxdh.rst
> new file mode 100644
> index 0000000000..e878058b7b
> --- /dev/null
> +++ b/doc/guides/nics/zxdh.rst
> @@ -0,0 +1,34 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright(c) 2023 ZTE Corporation.
> +
> +ZXDH Poll Mode Driver
> +======================
> +
> +The ZXDH PMD (**librte_net_zxdh**) provides poll mode driver support
> +for 25/100 Gbps ZXDH NX Series Ethernet Controller based on
> +the ZTE Ethernet Controller E310/E312.
> +
>

Can you please provide a link for the product?
There is a link in the prerequisetes section below, if that link is for
product please move it here.

> +
> +Features
> +--------
> +
> +Features of the zxdh PMD are:
> +
> +- Multi arch support: x86_64, ARMv8.
> +
> +Prerequisites
> +-------------
> +
> +- Learning about ZXDH NX Series Ethernet Controller NICs using
> +  `<https://enterprise.zte.com.cn/sup-detail.html?id=271&suptype=1>`_.
> +
> +Driver compilation and testing
> +------------------------------
> +
> +Refer to the document :ref:`compiling and testing a PMD for a NIC <pmd_build_and_test>`
> +for details.
> +
> +Limitations or Known issues
> +---------------------------
> +X86-32, Power8, ARMv7 and BSD are not supported yet.
> +
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index fb6d34b782..1a3db8a04d 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -62,6 +62,7 @@ drivers = [
>          'vhost',
>          'virtio',
>          'vmxnet3',
> +    'zxdh',
>  ]
>  std_deps = ['ethdev', 'kvargs'] # 'ethdev' also pulls in mbuf, net, eal etc
>  std_deps += ['bus_pci']         # very many PMDs depend on PCI, so make std
> diff --git a/drivers/net/zxdh/meson.build b/drivers/net/zxdh/meson.build
> new file mode 100644
> index 0000000000..593e3c5933
> --- /dev/null
> +++ b/drivers/net/zxdh/meson.build
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 ZTE Corporation
> +
> +if not is_linux
> +    build = false
> +    reason = 'only supported on Linux'
> +    subdir_done()
> +endif
> +
> +if arch_subdir != 'x86' and arch_subdir !
> = 'arm' or not dpdk_conf.get('RTE_ARCH_64')
>

Why not check 'RTE_ARCH_X86_64' and 'RTE_ARCH_ARM64'?

<...>

> +/* dev_ops for zxdh, bare necessities for basic operation */
> +static const struct eth_dev_ops zxdh_eth_dev_ops = {
> +    .dev_configure             = NULL,
> +    .dev_start                 = NULL,
> +    .dev_stop                 = NULL,
> +    .dev_close                 = NULL,
> +
> +    .rx_queue_setup             = NULL,
> +    .rx_queue_intr_enable     = NULL,
> +    .rx_queue_intr_disable     = NULL,
> +
> +    .tx_queue_setup             = NULL,
> +};
>

No ops is implemented, so when you run dpdk application when your device
exist, what happens, does application crash?

<...>

> +static int32_t zxdh_eth_dev_uninit(struct rte_eth_dev *eth_dev __rte_unused)
> +{
> +    if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> +        return 0;
> +    /** todo later
> +     * zxdh_dev_close(eth_dev);
>

Please either remove or implements todo comments in upstream driver.

> +     */
> +    return 0;
> +}
> +
> +int32_t zxdh_eth_pci_remove(struct rte_pci_device *pci_dev)
> +{
> +    int32_t ret = rte_eth_dev_pci_generic_remove(pci_dev, zxdh_eth_dev_uninit);
> +
> +    if (ret == -ENODEV) { /* Port has already been released by close. */
> +        ret = 0;
> +    }
> +    return ret;
> +}
> +
> +static const struct rte_pci_id pci_id_zxdh_map[] = {
> +    {RTE_PCI_DEVICE(PCI_VENDOR_ID_ZTE, ZXDH_E310_PF_DEVICEID)},
> +    {RTE_PCI_DEVICE(PCI_VENDOR_ID_ZTE, ZXDH_E310_VF_DEVICEID)},
> +    {RTE_PCI_DEVICE(PCI_VENDOR_ID_ZTE, ZXDH_E312_PF_DEVICEID)},
> +    {RTE_PCI_DEVICE(PCI_VENDOR_ID_ZTE, ZXDH_E312_VF_DEVICEID)},
> +    {.vendor_id = 0, /* sentinel */ },
> +};
> +static struct rte_pci_driver zxdh_pmd = {
> +    .driver = {.name = "net_zxdh", },
>

'driver.name' is already set by 'RTE_PMD_REGISTER_PCI' macro, no need to
set above.

> +    .id_table = pci_id_zxdh_map,
> +    .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> +    .probe = zxdh_eth_pci_probe,
> +    .remove = zxdh_eth_pci_remove,
> +};
> +
> +RTE_PMD_REGISTER_PCI(net_zxdh, zxdh_pmd);
> +RTE_PMD_REGISTER_PCI_TABLE(net_zxdh, pci_id_zxdh_map);
> +RTE_PMD_REGISTER_KMOD_DEP(net_zxdh, "* vfio-pci");
> +RTE_LOG_REGISTER_SUFFIX(zxdh_logtype_init, init, NOTICE);
> +RTE_LOG_REGISTER_SUFFIX(zxdh_logtype_driver, driver, NOTICE);
> +RTE_LOG_REGISTER_SUFFIX(zxdh_logtype_rx, rx, DEBUG);
> +RTE_LOG_REGISTER_SUFFIX(zxdh_logtype_tx, tx, DEBUG);
> +
> +RTE_LOG_REGISTER_SUFFIX(zxdh_logtype_msg, msg, DEBUG);
>

Default 'DEBUG' log level is too verbose, mostly default is NOTICE.

> +RTE_PMD_REGISTER_PARAM_STRING(net_zxdh,
> +    "q_depth=<int>");
>

Please document device arguments in the driver documentation.

<...>