[dpdk-dev] [PATCH V2 3/5] Add Intel FPGA BUS Lib Code
Shreyansh Jain
shreyansh.jain at nxp.com
Wed Mar 21 10:28:02 CET 2018
Hello Rosen,
inlined are some comments from a quick look...
On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu <rosen.xu at intel.com> wrote:
> Signed-off-by: Rosen Xu <rosen.xu at 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
>
More information about the dev
mailing list