Re: [PATCH v5 01/23] net/ntnic: add ethdev and makes PMD available

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

On 6/27/2024 8:38 AM, Serhii Iliushyk wrote:
> Add ntnic ethdev base  implementation
> 

Hi Serhii,

Thanks for the updated version, how driver constructed patch by patch is
easier to grasp now.

<...>

> diff --git a/doc/guides/nics/ntnic.rst b/doc/guides/nics/ntnic.rst
> new file mode 100644
> index 0000000000..68996eac35
> --- /dev/null
> +++ b/doc/guides/nics/ntnic.rst
> @@ -0,0 +1,43 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright(c) 2023 Napatech A/S
> +
> +NTNIC Poll Mode Driver
> +======================
> +
> +The NTNIC PMD provides poll mode driver support for Napatech smartNICs.
> +
> +
> +Design
> +------
> +
> +The NTNIC PMD is designed as a pure user-space driver, and requires no special
> +Napatech kernel modules.
> +
> +The Napatech smartNIC presents one control PCI device (PF0). NTNIC PMD accesses
> +smartNIC PF0 via vfio-pci kernel driver. Access to PF0 for all purposes is
> +exclusive, so only one process should access it. The physical ports are located
> +behind PF0 as DPDK port 0 and 1.
> +
> +
> +Supported NICs
> +--------------
> +
> +- NT200A02 2x100G SmartNIC
> +
> +    - FPGA ID 9563 (Inline Flow Management)
> +

Can you please provide some links for the product, so that whoever
interested can access to the devices easier.
This is also good opportunity to advertise the product.

> +
> +Features
> +--------
> +
> +- Link state information.
> +

Even this feature is not available at this stage.

As far as I can see it is missing now, but need to update this file, and
.ini file as new features enabled in the driver.

> +
> +Limitations
> +~~~~~~~~~~~
> +
> +Kernel versions before 5.7 are not supported. Kernel version 5.7 added vfio-pci
> +support for creating VFs from the PF which is required for the PMD to use
> +vfio-pci on the PF. This support has been back-ported to older Linux
> +distributions and they are also supported. If vfio-pci is not required kernel
> +version 4.18 is supported.
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index bd38b533c5..fb6d34b782 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -45,6 +45,7 @@ drivers = [
>          'nfb',
>          'nfp',
>          'ngbe',
> +        'ntnic',
>          'null',
>          'octeontx',
>          'octeon_ep',
> diff --git a/drivers/net/ntnic/meson.build b/drivers/net/ntnic/meson.build
> new file mode 100644
> index 0000000000..227949eacb
> --- /dev/null
> +++ b/drivers/net/ntnic/meson.build
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2020-2023 Napatech A/S
> +
> +if not is_linux or not dpdk_conf.has('RTE_ARCH_X86_64')
> +    build = false
> +    reason = 'only supported on x86_64 Linux'
> +    subdir_done()
> +endif
> +
> +# cflags
> +cflags += [
> +    '-std=c11',
> +]
>

Why this flag is explicitly required?


> +
> +# includes
> +includes = [
> +    include_directories('.'),
> +]
>

I think this include also not required at this stage. And I don't think
adding current folder '.' is required at all.


> +
> +# all sources
> +sources = files(
> +    'ntnic_ethdev.c',
> +)
> +# END
>

There are EOF markers in various files, if this is part of your coding
convention, it is OK to keep them in base files, but for the DPDK
specific files, no need to mark the end of file. (Unless you can justify
to have them.)

> diff --git a/drivers/net/ntnic/ntnic_ethdev.c b/drivers/net/ntnic/ntnic_ethdev.c
> new file mode 100644
> index 0000000000..9cc727ac4b
> --- /dev/null
> +++ b/drivers/net/ntnic/ntnic_ethdev.c
> @@ -0,0 +1,53 @@
> +/*
> + * SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Napatech A/S
> + */
> +
> +
> +#include <rte_kvargs.h>
>

Some of the headers are not required at this stage, can you please
include them when they are needed.

> +#include <rte_eal.h>
> +#include <rte_dev.h>
> +#include <rte_vfio.h>
> +#include <rte_ethdev.h>
> +#include <rte_bus_pci.h>
> +#include <ethdev_pci.h>
> +
> +/* Global static variables: */
>

Can drop comments not meaningful at this stage.

> +
> +static int
> +nthw_pci_dev_init(struct rte_pci_device *pci_dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +static int
> +nthw_pci_dev_deinit(struct rte_eth_dev *eth_dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +static int
> +nthw_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> +	struct rte_pci_device *pci_dev)
> +{
> +	int res;
> +	res = nthw_pci_dev_init(pci_dev);
> +	return res;
> +}
> +
> +static int
> +nthw_pci_remove(struct rte_pci_device *pci_dev)
> +{
> +	return rte_eth_dev_pci_generic_remove(pci_dev, nthw_pci_dev_deinit);
> +}
> +
> +static struct rte_pci_driver rte_nthw_pmd = {
> +	.driver = {
> +		.name = "net_ntnic",
> +	},
>

No need to set 'name', as this is already done by 'RTE_PMD_REGISTER_PCI'
macro.

> +
> +	.probe = nthw_pci_probe,
> +	.remove = nthw_pci_remove,
>

May need to add '.id_table', even it is empty (all zeros), otherwise I
expect this cause a crash during pci probe.