From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH V2 3/5] Add Intel FPGA BUS Lib Code Date: Wed, 21 Mar 2018 14:58:02 +0530 Message-ID: References: <1521618694-140757-1-git-send-email-rosen.xu@intel.com> <1521618694-140757-4-git-send-email-rosen.xu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: dev@dpdk.org, declan.doherty@intel.com, Bruce Richardson , tianfei.zhang@intel.com, hao.wu@intel.com, gaetan.rivet@6wind.com To: Rosen Xu Return-path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0079.outbound.protection.outlook.com [104.47.1.79]) by dpdk.org (Postfix) with ESMTP id 6CE8B2C58 for ; Wed, 21 Mar 2018 10:28:36 +0100 (CET) Received: by mail-wm0-f50.google.com with SMTP id x82so8446502wmg.1 for ; Wed, 21 Mar 2018 02:28:34 -0700 (PDT) In-Reply-To: <1521618694-140757-4-git-send-email-rosen.xu@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Rosen, inlined are some comments from a quick look... On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu wrote: > Signed-off-by: Rosen Xu > --- > drivers/bus/ifpga/Makefile | 64 ++++ > drivers/bus/ifpga/ifpga_bus.c | 573 ++++++++++++++++++++++++++++ > drivers/bus/ifpga/ifpga_common.c | 154 ++++++++ > drivers/bus/ifpga/ifpga_common.h | 25 ++ > drivers/bus/ifpga/ifpga_logs.h | 32 ++ > drivers/bus/ifpga/rte_bus_ifpga.h | 141 +++++++ > drivers/bus/ifpga/rte_bus_ifpga_version.map | 8 + > 7 files changed, 997 insertions(+) > create mode 100644 drivers/bus/ifpga/Makefile > create mode 100644 drivers/bus/ifpga/ifpga_bus.c > create mode 100644 drivers/bus/ifpga/ifpga_common.c > create mode 100644 drivers/bus/ifpga/ifpga_common.h > create mode 100644 drivers/bus/ifpga/ifpga_logs.h > create mode 100644 drivers/bus/ifpga/rte_bus_ifpga.h > create mode 100644 drivers/bus/ifpga/rte_bus_ifpga_version.map > > diff --git a/drivers/bus/ifpga/Makefile b/drivers/bus/ifpga/Makefile > new file mode 100644 > index 0000000..c71f186 > --- /dev/null > +++ b/drivers/bus/ifpga/Makefile > @@ -0,0 +1,64 @@ > +# BSD LICENSE > +# > +# Copyright(c) 2010-2017 Intel Corporation. All rights reserved. > +# All rights reserved. > +# > +# Redistribution and use in source and binary forms, with or without > +# modification, are permitted provided that the following conditions > +# are met: > +# > +# * Redistributions of source code must retain the above copyright > +# notice, this list of conditions and the following disclaimer. > +# * Redistributions in binary form must reproduce the above copyright > +# notice, this list of conditions and the following disclaimer in > +# the documentation and/or other materials provided with the > +# distribution. > +# * Neither the name of Intel Corporation nor the names of its > +# contributors may be used to endorse or promote products derived > +# from this software without specific prior written permission. > +# > +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Did you get a chance to go through the comment in RFC? I think you should replace the boilerplate with SPDX tags. > + > +include $(RTE_SDK)/mk/rte.vars.mk > + > +# > +# library name > +# > +LIB = librte_bus_ifpga.a > +LIBABIVER := 1 > +EXPORT_MAP := rte_bus_ifpga_version.map > + > +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_DEBUG_INIT),y) I think there was a similar comment on RFC - "...DPAA2..." macro is a copy-paste error. > +CFLAGS += -O0 -g > +CFLAGS += "-Wno-error" > +else > +CFLAGS += -O3 > +CFLAGS += $(WERROR_FLAGS) > +endif > + > +CFLAGS += -I$(RTE_SDK)/drivers/bus/ifpga > +CFLAGS += -I$(RTE_SDK)/drivers/bus/pci > +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal > +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common > +#CFLAGS += -I$(RTE_SDK)/lib/librte_rawdev > +#LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring -lrte_rawdev If you don't need these lines, don't keep them. That is ok until RFC, but not in formal patch. > +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring > +#LDLIBS += -lrte_ethdev > + > +VPATH += $(SRCDIR)/base > + > +SRCS-y += \ > + ifpga_bus.c \ > + ifpga_common.c > + > +include $(RTE_SDK)/mk/rte.lib.mk > diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c > new file mode 100644 > index 0000000..ff72b74 > --- /dev/null > +++ b/drivers/bus/ifpga/ifpga_bus.c > @@ -0,0 +1,573 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2014 Intel Corporation. > + * Copyright 2013-2014 6WIND S.A. Are you sure of the above copyright? I think this is a new file. Maybe your internal HW routines can have old copyrights. Just a trivial comment, though. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#include "rte_rawdev.h" > +#include "rte_rawdev_pmd.h" > +#include "rte_bus_ifpga.h" > +#include "ifpga_logs.h" > +#include "ifpga_common.h" > + > +int ifpga_bus_logtype; > + > +/*register a ifpga bus based driver */ Comments have ' ' after the '/*'. Maybe you can refer [1] once. [1] https://dpdk.org/doc/guides/contributing/coding_style.html#coding-style > +void rte_ifpga_driver_register(struct rte_afu_driver *driver) > +{ > + RTE_VERIFY(driver); > + > + TAILQ_INSERT_TAIL(&rte_ifpga_bus.driver_list, driver, next); > +} > + [..snip..] > diff --git a/drivers/bus/ifpga/ifpga_logs.h b/drivers/bus/ifpga/ifpga_logs.h > new file mode 100644 > index 0000000..36b9b3f > --- /dev/null > +++ b/drivers/bus/ifpga/ifpga_logs.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2015 Intel Corporation. > + * Copyright 2013-2014 6WIND S.A. > + */ > + > +#ifndef _IFPGA_BUS_LOGS_H_ > +#define _IFPGA_BUS_LOGS_H_ Ideally this is name of the file, which is IFPFA_LOGS. But, technically this is not an issue. > + > +#include > + > +extern int ifpga_bus_logtype; > + > +#define IFPGA_LOG(level, fmt, args...) \ > + rte_log(RTE_LOG_ ## level, ifpga_bus_logtype, "%s(): " fmt "\n", \ > + __func__, ##args) > + > +#define IFPGA_BUS_LOG(level, fmt, args...) \ > + rte_log(RTE_LOG_ ## level, ifpga_bus_logtype, "%s(): " fmt "\n", \ > + __func__, ##args) > + I noticed that at some places where you have used the above macros, you have added '\n' in the call. It would lead to double '\n' as your IFPGA_LOG and IFPGA_BUS_LOG already have one '\n'. > +#define IFPGA_BUS_FUNC_TRACE() IFPGA_BUS_LOG(DEBUG, ">>") > + > +#define IFPGA_BUS_DEBUG(fmt, args...) \ > + IFPGA_BUS_LOG(DEBUG, fmt, ## args) > +#define IFPGA_BUS_INFO(fmt, args...) \ > + IFPGA_BUS_LOG(INFO, fmt, ## args) > +#define IFPGA_BUS_ERR(fmt, args...) \ > + IFPGA_BUS_LOG(ERR, fmt, ## args) > +#define IFPGA_BUS_WARN(fmt, args...) \ > + IFPGA_BUS_LOG(WARNING, fmt, ## args) > + > +#endif /* _IFPGA_BUS_LOGS_H_ */ [..snip..] > +#endif /* _RTE_BUS_IFPGA_H_ */ > diff --git a/drivers/bus/ifpga/rte_bus_ifpga_version.map b/drivers/bus/ifpga/rte_bus_ifpga_version.map > new file mode 100644 > index 0000000..e2aa7da > --- /dev/null > +++ b/drivers/bus/ifpga/rte_bus_ifpga_version.map > @@ -0,0 +1,8 @@ > +DPDK_17.11 { Should be DPDK 18.05 > + global: > + > + rte_ifpga_driver_register; > + rte_ifpga_driver_unregister; And indentation is incorrect. > + > + local: *; > +}; > -- > 1.8.3.1 >