Re: [PATCH v3 01/22] net/atlantic: atlantic PMD driver skeleton

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

On 9/29/2018 11:30 AM, Igor Russkikh wrote:
> From: Pavel Belous <Pavel.Belous@aquantia.com>
> 
> Makefile/meson build infrastructure, atl_ethdev minimal skeleton,
> header with aquantia aQtion NIC device and vendor IDs.
> 
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> ---
>  config/common_base                                |   5 +
>  drivers/net/Makefile                              |   1 +
>  drivers/net/atlantic/Makefile                     |  27 ++++
>  drivers/net/atlantic/atl_common.h                 |  96 ++++++++++++++
>  drivers/net/atlantic/atl_ethdev.c                 | 145 ++++++++++++++++++++++
>  drivers/net/atlantic/atl_ethdev.h                 |  15 +++
>  drivers/net/atlantic/meson.build                  |  10 ++
>  drivers/net/atlantic/rte_pmd_atlantic_version.map |   4 +
>  drivers/net/meson.build                           |   1 +
>  mk/rte.app.mk                                     |   1 +
>  10 files changed, 305 insertions(+)
>  create mode 100644 drivers/net/atlantic/Makefile
>  create mode 100644 drivers/net/atlantic/atl_common.h
>  create mode 100644 drivers/net/atlantic/atl_ethdev.c
>  create mode 100644 drivers/net/atlantic/atl_ethdev.h
>  create mode 100644 drivers/net/atlantic/meson.build
>  create mode 100644 drivers/net/atlantic/rte_pmd_atlantic_version.map
> 
> diff --git a/config/common_base b/config/common_base
> index 155c7d40ea31..ba51ffbd43ce 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -635,6 +635,11 @@ CONFIG_RTE_LIBRTE_PMD_DPAA_EVENTDEV=n
>  CONFIG_RTE_LIBRTE_PMD_DPAA2_EVENTDEV=n
>  
>  #
> +# Compile Aquantia Atlantic PMD driver
> +#
> +CONFIG_RTE_LIBRTE_ATLANTIC_PMD=y

Just to check, default enable in "common_base" means PMD supports all
architectures (x86_64, i686, arm, ppc) and both Linux and FreeBSD.
If this is not the case, please disable in common_base and enable in whatever
supported.

<...>

> +int atl_logtype_init;
> +int atl_logtype_driver;

These are not yet used in this PMD, please add when used.

<...>

> +static struct rte_pci_driver rte_atl_pmd = {
> +	.id_table = pci_id_atl_map,
> +	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |

RTE_PCI_DRV_INTR_LSC is way of saying LSC interrupts are supported, please add
the flag when driver supports it.

> +		     RTE_PCI_DRV_IOVA_AS_VA,
> +	.probe = eth_atl_pci_probe,
> +	.remove = eth_atl_pci_remove,
> +};
> +
> +static const struct eth_dev_ops atl_eth_dev_ops = {
> +	.dev_configure	      = atl_dev_configure,
> +	.dev_start	      = atl_dev_start,
> +	.dev_stop	      = atl_dev_stop,
> +	.dev_close	      = atl_dev_close,
> +	.dev_reset	      = atl_dev_reset,

I am not sure but it may fail to run without .dev_infos_get, can you please check.

<...>

> +static int
> +atl_dev_reset(struct rte_eth_dev *dev)
> +{
> +	int ret;
> +
> +	ret = eth_atl_dev_uninit(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_atl_dev_init(dev);
> +
> +	return ret;
> +}

Up to you, but dev_reset may be left out from initial skeleton patch and moved
into later patches.

<...>

> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Aquantia Corporation
> + */
> +
> +#ifndef _ATLANTIC_ETHDEV_H_
> +#define _ATLANTIC_ETHDEV_H_
> +#include <rte_tm_driver.h>

Why include "rte_tm_driver.h" at this stage?

<...>

> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2018 Aquantia Corporation
> +
> +sources = files(
> +	'atl_ethdev.c',
> +)
> +
> +deps += ['eal']

Makefile has more dependency, although I am not sure what to provide in meson,
just can you please double check if this mismatch is ok?

> +
> +allow_experimental_apis = true

Do you need this, at least at this stage?

<...>

> @@ -157,6 +157,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD)       += -lrte_pmd_mlx5 -ldl -lmnl
>  else
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD)       += -lrte_pmd_mlx5 -libverbs -lmlx5 -lmnl
>  endif
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ATLANTIC_PMD)   += -lrte_pmd_atlantic

Add in alphabetical order please.