Hello Rosen, inlined are some comments from a quick look...
On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu <rosen...@intel.com> wrote: > Signed-off-by: Rosen Xu <rosen...@intel.com> > --- > 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 <string.h> > +#include <inttypes.h> > +#include <stdint.h> > +#include <stdlib.h> > +#include <stdio.h> > +#include <sys/queue.h> > +#include <sys/mman.h> > +#include <sys/types.h> > +#include <unistd.h> > +#include <fcntl.h> > + > +#include <rte_errno.h> > +#include <rte_bus.h> > +#include <rte_per_lcore.h> > +#include <rte_memory.h> > +#include <rte_memzone.h> > +#include <rte_eal.h> > +#include <rte_common.h> > + > +#include <rte_devargs.h> > +#include <rte_pci.h> > +#include <rte_bus_pci.h> > +#include <rte_kvargs.h> > +#include <rte_alarm.h> > + > +#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 ' ' <space> 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 <rte_log.h> > + > +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 >
