From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 755AB4C9D for ; Fri, 21 Sep 2018 16:21:08 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Sep 2018 07:21:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,285,1534834800"; d="scan'208";a="71844257" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.39]) ([10.237.221.39]) by fmsmga007.fm.intel.com with ESMTP; 21 Sep 2018 07:21:06 -0700 To: Igor Russkikh , dev@dpdk.org Cc: pavel.belous@aquantia.com References: <1536838528-11800-1-git-send-email-igor.russkikh@aquantia.com> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: <5db0866a-66f8-9547-244e-7697eb13c12d@intel.com> Date: Fri, 21 Sep 2018 15:21:05 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1536838528-11800-1-git-send-email-igor.russkikh@aquantia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 01/21] net/atlantic: atlantic PMD driver skeleton X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Sep 2018 14:21:09 -0000 On 9/13/2018 12:35 PM, Igor Russkikh wrote: > From: Pavel Belous > > Makefile/meson build infrastructure, atl_ethdev minimal skeleton, > header with aquantia aQtion NIC device and vendor IDs. > > Signed-off-by: Igor Russkikh > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include 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 > +#include > +#include > +#include > +#include > + > +#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