Re: [dpdk-dev] [PATCH v2 01/21] net/atlantic: atlantic PMD driver skeleton

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

On 9/13/2018 12:35 PM, 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>
> ---
>  config/common_base                                |   9 +
>  drivers/net/Makefile                              |   2 +
>  drivers/net/atlantic/Makefile                     |  36 +++
>  drivers/net/atlantic/atl_common.h                 |  96 ++++++++
>  drivers/net/atlantic/atl_ethdev.c                 | 281 ++++++++++++++++++++++
>  drivers/net/atlantic/atl_ethdev.h                 |  28 +++
>  drivers/net/atlantic/meson.build                  |  19 ++
>  drivers/net/atlantic/rte_pmd_atlantic_version.map |   4 +
>  drivers/net/meson.build                           |   1 +
>  mk/rte.app.mk                                     |   1 +

<...>

> @@ -635,6 +635,15 @@ 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
> +CONFIG_RTE_LIBRTE_ATLANTIC_DEBUG=n

We are trying to have as less compile time option as possible. Instead of
CONFIG_RTE_LIBRTE_ATLANTIC_DEBUG compile time option, it is possible to use
dynamic logging, please switch to it.

> +CONFIG_RTE_LIBRTE_ATLANTIC_DEBUG_RX=n
> +CONFIG_RTE_LIBRTE_ATLANTIC_DEBUG_TX=n
> +CONFIG_RTE_LIBRTE_ATLANTIC_DEBUG_TX_FREE=n

RX, TX & TX_FREE config options are for data path, needs to be compile time
option to not affect performance. But as far as I can see they are not used at
all, please remove them and can be added back if needed.

> +
> +#
>  # Compile raw device support
>  # EXPERIMENTAL: API may change without prior notice
>  #
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 664398de9..3d4a1ae59 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -70,4 +70,6 @@ $(error "RTE_LIBRTE_CFGFILE must be enabled in configuration!")
>  endif
>  endif
>  
> +DIRS-$(CONFIG_RTE_LIBRTE_ATLANTIC_PMD) += atlantic

Please add to above block alphabetically, these are kind of peculiar ones with
extra dependencies.

<...>

> +
> +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> +LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs -lrte_hash
> +LDLIBS += -lrte_bus_pci

It is hard to really know if these dependencies exists if you add them all in
initial patch, it is good to add when real dependency occurs, so that we will
know what are not needed here.

> +
> +#
> +# Add extra flags for base driver files (also known as shared code)
> +# to disable warnings in them
> +#
> +BASE_DRIVER_OBJS=$(sort $(patsubst %.c,%.o,$(notdir $(wildcard $(SRCDIR)/base/*.c))))

You don't have "base" folder, the name of your base folder seems "hw_atl". If it
not a specific name or maintenance cost you can rename it to "base" to be
consistent with other PMDs.

<...>

> @@ -0,0 +1,281 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Aquantia Corporation
> + */
> +
> +#include "atl_ethdev.h"
> +
> +#include <sys/queue.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdarg.h>
> +#include <inttypes.h>
> +#include <netinet/in.h>
> +#include <rte_byteorder.h>
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +
> +#include <rte_interrupts.h>
> +#include <rte_log.h>
> +#include <rte_debug.h>
> +#include <rte_pci.h>
> +#include <rte_bus_pci.h>
> +#include <rte_atomic.h>
> +#include <rte_branch_prediction.h>
> +#include <rte_memory.h>
> +#include <rte_eal.h>
> +#include <rte_alarm.h>
> +#include <rte_ether.h>
> +#include <rte_ethdev_driver.h>
> +#include <rte_ethdev_pci.h>
> +#include <rte_malloc.h>
> +#include <rte_random.h>
> +#include <rte_dev.h>
> +#include <rte_hash_crc.h>
> +#include <rte_net.h>

This is a long include list, I wonder if this is copy paste or real need. If
you/we get these with extra clutter at first place, it will be harder to clean
(if somebody spends time for it) later. Please include only what you need in
_this_ patch.

> +
> +#include "atl_common.h"
> +#include "atl_hw_regs.h"
> +#include "atl_logs.h"
> +
> +static int eth_atl_dev_init(struct rte_eth_dev *eth_dev);
> +static int eth_atl_dev_uninit(struct rte_eth_dev *eth_dev);
> +
> +static int  atl_dev_configure(struct rte_eth_dev *dev);
> +static int  atl_dev_start(struct rte_eth_dev *dev);
> +static void atl_dev_stop(struct rte_eth_dev *dev);
> +static void atl_dev_close(struct rte_eth_dev *dev);
> +static int  atl_dev_reset(struct rte_eth_dev *dev);
> +
> +static int atl_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
> +					     uint16_t queue_id,
> +					     uint8_t stat_idx,
> +					     uint8_t is_rx);

Can you please add function declarations on the patch that has function
implementation.

<...>

> +static int
> +atl_get_bus_info(struct aq_hw_s *hw)
> +{
> +	hw->bus.speed = atl_bus_speed_unknown;
> +	hw->bus.width = atl_bus_width_unknown;

atl_bus_speed_unknown & atl_bus_width_unknown not defined in this patch breking
build. They look like enum items, please prefer uppercase names for enums.

> +
> +	return 0;
> +}
> +
> +static int
> +eth_atl_dev_init(struct rte_eth_dev *eth_dev)
> +{
> +	struct atl_adapter *adapter =
> +		(struct atl_adapter *)eth_dev->data->dev_private;
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> +	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
> +	struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
> +	int err = 0;
> +
> +	eth_dev->dev_ops = &atl_eth_dev_ops;
> +	eth_dev->rx_pkt_burst = &atl_recv_pkts;
> +	eth_dev->tx_pkt_burst = &atl_xmit_pkts;
> +	eth_dev->tx_pkt_prepare = &atl_prep_pkts;

These functions are not defined in this patch.

> +
> +	/* For secondary processes, the primary process has done all the work */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;

Won't you want rx_pkt_burst & tx_pkt_burst etc to be set in secondary process?

<...>

> +static int
> +atl_dev_configure(struct rte_eth_dev *dev)
> +{
> +	struct atl_interrupt *intr =
> +		ATL_DEV_PRIVATE_TO_INTR(dev->data->dev_private);

I would expect you to get compiler warning becase intr is not used, you can drop
it if you won't use.

<...>

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

This header file is *not* provided in thi patch. Please remember, each path must
be compilable. Please be sure to check patch by patch build test before sending.

<...>

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

Why hash?

<...>

> @@ -0,0 +1,4 @@
> +DPDK_18.05 {
> +
> +	local: *;
> +};

DPDK_18.11