Re: [PATCH V2 3/5] Add Intel FPGA BUS Lib Code

[Shreyansh Jain <shreyansh.jain@nxp.com>]

Hello Rosen,

inlined are some comments from a quick look...

On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu <rosen.xu@intel.com> wrote:
> Signed-off-by: Rosen Xu <rosen.xu@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
>