Re: [dpdk-dev] [PATCH v6 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver

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

Hello Rosen,

Sorry for multiple review iterations, but, in the v7, can you take care 
of these as well:

On Thursday 26 April 2018 03:13 PM, Xu, Rosen wrote:
> From: Rosen Xu <rosen.xu@intel.com>
> 
> Add Intel FPGA BUS Rawdev Driver which is based on
> librte_rawdev library.
> 
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> Signed-off-by: Yanglong Wu <yanglong.wu@intel.com>
> Signed-off-by: Figo zhang <tianfei.zhang@intel.com>
> ---
>   config/common_base                                 |   1 +
>   drivers/raw/Makefile                               |   1 +
>   drivers/raw/ifpga_rawdev/Makefile                  |  36 ++
>   drivers/raw/ifpga_rawdev/ifpga_rawdev.c            | 597 +++++++++++++++++++++
>   drivers/raw/ifpga_rawdev/ifpga_rawdev.h            |  37 ++
>   .../raw/ifpga_rawdev/rte_ifpga_rawdev_version.map  |   4 +
>   mk/rte.app.mk                                      |   1 +
>   7 files changed, 677 insertions(+)
>   create mode 100644 drivers/raw/ifpga_rawdev/Makefile
>   create mode 100644 drivers/raw/ifpga_rawdev/ifpga_rawdev.c
>   create mode 100644 drivers/raw/ifpga_rawdev/ifpga_rawdev.h
>   create mode 100644 drivers/raw/ifpga_rawdev/rte_ifpga_rawdev_version.map
> 

[...]

> +static int
> +fpga_pr(struct rte_rawdev *raw_dev, u32 port_id, u64 *buffer, u32 size,
> +			u64 *status)
> +{
> +
> +	struct opae_adapter *adapter;
> +	struct opae_manager *mgr;
> +	struct opae_accelerator *acc;
> +	struct opae_bridge *br;
> +	int ret;
> +
> +	adapter = ifpga_rawdev_get_priv(raw_dev);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	mgr = opae_adapter_get_mgr(adapter);
> +	if (!mgr)
> +		return -ENODEV;
> +
> +	acc = opae_adapter_get_acc(adapter, port_id);
> +	if (!acc)
> +		return -ENODEV;
> +
> +	br = opae_acc_get_br(acc);
> +	if (!br)
> +		return -ENODEV;
> +
> +	ret = opae_manager_flash(mgr, port_id, buffer, size, status);
> +	if (ret) {
> +		printf("%s pr error %d\n", __func__, ret);

Please use the debugging or error macros.

> +		return ret;
> +	}
> +
> +	usleep(100);

Hmm, is that some caveat or a stray line left from some debugging?

> +	ret = opae_bridge_reset(br);
> +	if (ret) {
> +		printf("%s reset port:%d error %d\n", __func__, port_id, ret);

Same - debugging/error macros.

> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int rte_fpga_do_pr(struct rte_rawdev *rawdev, int port_id,
> +		const char *file_name)
> +{
> +	struct stat file_stat;
> +	int file_fd;
> +	int ret = 0;
> +	u32 buffer_size;
> +	void *buffer;
> +	u64 pr_error;
> +
> +	if (!file_name)
> +		return -EINVAL;
> +
> +	file_fd = open(file_name, O_RDONLY);
> +	if (file_fd < 0) {
> +		printf("%s: open file error: %s\n", __func__, file_name);
> +		printf("Message : %s\n", strerror(errno));

Same - debug/error macros

> +		return -EINVAL;
> +	}
> +	ret = stat(file_name, &file_stat);
> +	if (ret) {
> +		printf("stat on bitstream file failed: %s\n", file_name);

One more (same is applicable across the file).

> +		return -EINVAL;
> +	}
> +	buffer_size = file_stat.st_size;
> +	printf("bitstream file size: %u\n", buffer_size);
> +	buffer = rte_malloc(NULL, buffer_size, 0);
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto close_fd;
> +	}
> +
> +	/*read the raw data*/
> +	if (buffer_size != read(file_fd, (void *)buffer, buffer_size)) {
> +		ret = -EINVAL;
> +		goto free_buffer;
> +	}
> +
> +	/*do PR now*/
> +	ret = fpga_pr(rawdev, port_id, buffer, buffer_size, &pr_error);
> +	printf("downloading to device port %d....%s.\n", port_id,
> +		ret ? "failed" : "success");
> +	if (ret) {
> +		ret = -EINVAL;
> +		//raw_dev->ops->show_pr_error(pr_error);

I am guessing this is stray from some debugging attempt. Same for the 
printf above.

> +		goto free_buffer;
> +	}
> +
> +free_buffer:
> +	if (buffer)
> +		rte_free(buffer);
> +close_fd:
> +	close(file_fd);
> +	file_fd = 0;
> +	return ret;
> +}
> +
> +static int ifpga_rawdev_pr(struct rte_rawdev *dev,
> +	rte_rawdev_obj_t pr_conf)
> +{
> +	struct opae_adapter *adapter;
> +	struct rte_afu_pr_conf *afu_pr_conf;
> +	int ret;
> +	struct uuid uuid;
> +	struct opae_accelerator *acc;
> +
> +	IFPGA_RAWDEV_PMD_FUNC_TRACE();
> +
> +	adapter = ifpga_rawdev_get_priv(dev);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	if (!pr_conf)
> +		return -EINVAL;
> +
> +	afu_pr_conf = pr_conf;
> +
> +	if (afu_pr_conf->pr_enable) {
> +		ret = rte_fpga_do_pr(dev,
> +				afu_pr_conf->afu_id.port,
> +				afu_pr_conf->bs_path);
> +		if (ret) {
> +			printf("do pr error %d\n", ret);

One more

> +			return ret;
> +		}
> +	}
> +
> +	acc = opae_adapter_get_acc(adapter, afu_pr_conf->afu_id.port);
> +	if (!acc)
> +		return -ENODEV;
> +
> +	ret = opae_acc_get_uuid(acc, &uuid);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(&afu_pr_conf->afu_id.uuid_low, uuid.b, sizeof(u64));
> +	memcpy(&afu_pr_conf->afu_id.uuid_high, uuid.b + 8, sizeof(u64));
> +
> +	printf("%s: uuid_l=0x%lx, uuid_h=0x%lx\n", __func__,
> +		(u64)afu_pr_conf->afu_id.uuid_low,
> +		(u64)afu_pr_conf->afu_id.uuid_high);
debug/error macros

> +
> +	return 0;
> +}
> +
> +static const struct rte_rawdev_ops ifpga_rawdev_ops = {
> +	.dev_info_get = ifpga_rawdev_info_get,
> +	.dev_configure = NULL,
> +	.dev_start = ifpga_rawdev_start,
> +	.dev_stop = ifpga_rawdev_stop,
> +	.dev_close = ifpga_rawdev_close,
> +	.dev_reset = ifpga_rawdev_reset,
> +
> +	.queue_def_conf = NULL,
> +	.queue_setup = NULL,
> +	.queue_release = NULL,
> +
> +	.attr_get = NULL,
> +	.attr_set = NULL,
> +
> +	.enqueue_bufs = NULL,
> +	.dequeue_bufs = NULL,
> +
> +	.dump = NULL,
> +
> +	.xstats_get = NULL,
> +	.xstats_get_names = NULL,
> +	.xstats_get_by_name = NULL,
> +	.xstats_reset = NULL,
> +
> +	.firmware_status_get = NULL,
> +	.firmware_version_get = NULL,
> +	.firmware_load = ifpga_rawdev_pr,
> +	.firmware_unload = NULL,
> +
> +	.dev_selftest = NULL,
> +};
> +
> +static int
> +ifpga_rawdev_create(struct rte_pci_device *pci_dev,
> +			int socket_id)
> +{
> +	int ret = 0;
> +	struct rte_rawdev *rawdev = NULL;
> +	struct opae_adapter *adapter;
> +	struct opae_manager *mgr;
> +	struct opae_adapter_data_pci *data;
> +	char name[RTE_RAWDEV_NAME_MAX_LEN];
> +	int i;
> +
> +	if (!pci_dev) {
> +		IFPGA_RAWDEV_PMD_ERR("Invalid pci_dev of the device!");
> +		ret = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	memset(name, 0, sizeof(name));
> +	snprintf(name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%x:%02x.%x",
> +	pci_dev->addr.bus, pci_dev->addr.devid, pci_dev->addr.function);

Alignment issue... 'pci_dev->addr.bus ... ' would start from underneath 
arguments of snprintf.

> +
> +	IFPGA_RAWDEV_PMD_INFO("Init %s on NUMA node %d", name, rte_socket_id());
> +
> +	/* Allocate device structure */
> +	rawdev = rte_rawdev_pmd_allocate(name, sizeof(struct opae_adapter),
> +					 socket_id);
> +	if (rawdev == NULL) {
> +		IFPGA_RAWDEV_PMD_ERR("Unable to allocate rawdevice");
> +		ret = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	/* alloc OPAE_FPGA_PCI data to register to OPAE hardware level API */
> +	data = opae_adapter_data_alloc(OPAE_FPGA_PCI);
> +	if (!data)
> +		return -ENOMEM;

What about jumping to cleanup here? (pmd_release())

> +
> +	/* init opae_adapter_data_pci for device specific information */
> +	for (i = 0; i < PCI_MAX_RESOURCE; i++) {
> +		data->region[i].phys_addr = pci_dev->mem_resource[i].phys_addr;
> +		data->region[i].len = pci_dev->mem_resource[i].len;
> +		data->region[i].addr = pci_dev->mem_resource[i].addr;
> +	}
> +	data->device_id = pci_dev->id.device_id;
> +	data->vendor_id = pci_dev->id.vendor_id;
> +
> +	/* create a opae_adapter based on above device data */
> +	adapter = opae_adapter_alloc(pci_dev->device.name, data);
> +	if (!adapter) {
> +		opae_adapter_data_free(data);
> +		return -ENOMEM;

Same - you are not performing cleanup here.

> +	}
> +
> +	rawdev->dev_ops = &ifpga_rawdev_ops;
> +	rawdev->device = &pci_dev->device;
> +	rawdev->driver_name = pci_dev->device.driver->name;
> +
> +	rawdev->dev_private = adapter;
> +
> +	/* must enumerate the adapter before use it */
> +	ret = opae_adapter_enumerate(adapter);
> +	if (ret)
> +		return ret;

Cleanup to be performed here.

> +
> +	/* set opae_manager to rawdev */
> +	mgr = opae_adapter_get_mgr(adapter);
> +	if (mgr) {
> +		/*PF function*/

/*<space>PF function<space>*/

> +		IFPGA_RAWDEV_PMD_INFO("this is a PF function");
> +	}
> +
> +	return ret;
> +
> +cleanup:
> +	if (rawdev)
> +		rte_rawdev_pmd_release(rawdev);
> +
> +	return ret;
> +}
> +
> +static int
> +ifpga_rawdev_destroy(struct rte_pci_device *pci_dev)
> +{
> +	int ret;
> +	struct rte_rawdev *rawdev;
> +	char name[RTE_RAWDEV_NAME_MAX_LEN];
> +	struct opae_adapter *adapter;
> +
> +	if (!pci_dev) {
> +		IFPGA_RAWDEV_PMD_ERR("Invalid pci_dev of the device!");
> +		ret = -EINVAL;
> +		return ret;
> +	}
> +
> +	memset(name, 0, sizeof(name));
> +	snprintf(name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%x:%02x.%x",
> +		pci_dev->addr.bus, pci_dev->addr.devid, pci_dev->addr.function);
> +
> +	IFPGA_RAWDEV_PMD_INFO("Closing %s on NUMA node %d",
> +		name, rte_socket_id());
> +
> +	rawdev = rte_rawdev_pmd_get_named_dev(name);
> +	if (!rawdev) {
> +		IFPGA_RAWDEV_PMD_ERR("Invalid device name (%s)", name);
> +		return -EINVAL;
> +	}
> +
> +	adapter = ifpga_rawdev_get_priv(rawdev);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	opae_adapter_data_free(adapter->data);
> +	opae_adapter_free(adapter);
> +
> +	/* rte_rawdev_close is called by pmd_release */
> +	ret = rte_rawdev_pmd_release(rawdev);
> +	if (ret)
> +		IFPGA_RAWDEV_PMD_DEBUG("Device cleanup failed");
> +
> +	return 0;
> +}
> +
> +static int
> +ifpga_rawdev_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> +	struct rte_pci_device *pci_dev)
> +{
> +
> +	printf("## %s\n", __func__);

One more.

> +	return ifpga_rawdev_create(pci_dev, rte_socket_id());
> +}
> +
> +static int ifpga_rawdev_pci_remove(struct rte_pci_device *pci_dev)
> +{
> +	return ifpga_rawdev_destroy(pci_dev);
> +}
> +
> +static struct rte_pci_driver rte_ifpga_rawdev_pmd = {
> +	.id_table  = pci_ifpga_map,
> +	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> +	.probe     = ifpga_rawdev_pci_probe,
> +	.remove    = ifpga_rawdev_pci_remove,
> +};
> +
> +RTE_PMD_REGISTER_PCI(ifpga_rawdev_pci_driver, rte_ifpga_rawdev_pmd);
> +RTE_PMD_REGISTER_PCI_TABLE(ifpga_rawdev_pci_driver, rte_ifpga_rawdev_pmd);
> +RTE_PMD_REGISTER_KMOD_DEP(ifpga_rawdev_pci_driver, "* igb_uio | uio_pci_generic | vfio-pci");
> +
> +RTE_INIT(ifpga_rawdev_init_log);
> +static void
> +ifpga_rawdev_init_log(void)
> +{
> +	ifpga_rawdev_logtype = rte_log_register("driver.raw.init");
> +	if (ifpga_rawdev_logtype >= 0)
> +		rte_log_set_level(ifpga_rawdev_logtype, RTE_LOG_NOTICE);
> +}
> +
> +static const char * const valid_args[] = {
> +#define IFPGA_ARG_NAME         "ifpga"
> +	IFPGA_ARG_NAME,
> +#define IFPGA_ARG_PORT         "port"
> +	IFPGA_ARG_PORT,
> +#define IFPGA_AFU_BTS          "afu_bts"
> +	IFPGA_AFU_BTS,
> +	NULL
> +};
> +
> +static int
> +ifpga_cfg_probe(struct rte_vdev_device *dev)
> +{
> +	struct rte_devargs *devargs;
> +	struct rte_kvargs *kvlist = NULL;
> +	int      port;
> +	char *name = NULL;
> +	char dev_name[RTE_RAWDEV_NAME_MAX_LEN+8];
> +
> +	devargs = dev->device.devargs;
> +
> +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
> +	if (!kvlist) {
> +		IFPGA_RAWDEV_PMD_LOG(ERR, "error when parsing param");
> +		goto end;
> +	}
> +
> +	if (rte_kvargs_count(kvlist, IFPGA_ARG_NAME) == 1) {
> +		if (rte_kvargs_process(kvlist, IFPGA_ARG_NAME,
> +				       &ifpga_get_string_arg, &name) < 0) {
> +			IFPGA_RAWDEV_PMD_ERR("error to parse %s",
> +				     IFPGA_ARG_NAME);
> +			goto end;
> +		}
> +	} else {
> +		IFPGA_RAWDEV_PMD_ERR("arg %s is mandatory for ifpga bus",
> +			  IFPGA_ARG_NAME);
> +		goto end;
> +	}
> +
> +	if (rte_kvargs_count(kvlist, IFPGA_ARG_PORT) == 1) {
> +		if (rte_kvargs_process(kvlist,
> +			IFPGA_ARG_PORT,
> +			&ifpga_get_integer32_arg,
> +			&port) < 0) {
> +			IFPGA_RAWDEV_PMD_ERR("error to parse %s",
> +				IFPGA_ARG_PORT);
> +			goto end;
> +		}
> +	} else {
> +		IFPGA_RAWDEV_PMD_ERR("arg %s is mandatory for ifpga bus",
> +			  IFPGA_ARG_PORT);
> +		goto end;
> +	}
> +
> +	memset(dev_name, 0, sizeof(dev_name));
> +	snprintf(dev_name, (RTE_RAWDEV_NAME_MAX_LEN+8), "%d|%s",

Why did you do this? (adding 8 to RTE_RAWDEV_NAME_MAX_LEN)?
Just add a patch to increase the value of RTE_RAWDEV_NAME_MAX_LEN.

> +	port, name);
> +
> +	rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> +							dev_name,
> +							devargs->args);

Seems to be some alignment issue here...

> +end:
> +	if (kvlist)
> +		rte_kvargs_free(kvlist);
> +	if (name)
> +		free(name);
> +
> +	return 0;
> +}
> +
> +static int
> +ifpga_cfg_remove(struct rte_vdev_device *vdev)
> +{
> +	IFPGA_RAWDEV_PMD_INFO("Remove ifpga_cfg %p",
> +		vdev);
> +
> +	return 0;
> +}
> +
> +static struct rte_vdev_driver ifpga_cfg_driver = {
> +	.probe = ifpga_cfg_probe,
> +	.remove = ifpga_cfg_remove,
> +};
> +
> +RTE_PMD_REGISTER_VDEV(net_ifpga_cfg, ifpga_cfg_driver);
> +RTE_PMD_REGISTER_ALIAS(net_ifpga_cfg, ifpga_cfg);
> +RTE_PMD_REGISTER_PARAM_STRING(net_ifpga_cfg,
> +	"bdf=<string> "
> +	"port=<int> "
> +	"afu_bts=<path>");
> +
> diff --git a/drivers/raw/ifpga_rawdev/ifpga_rawdev.h b/drivers/raw/ifpga_rawdev/ifpga_rawdev.h
> new file mode 100644
> index 0000000..169dc1d
> --- /dev/null
> +++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +
> +#ifndef _IFPGA_RAWDEV_H_
> +#define _IFPGA_RAWDEV_H_
> +
> +extern int ifpga_rawdev_logtype;
> +
> +#define IFPGA_RAWDEV_PMD_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, ifpga_rawdev_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)

Just a trivial comment - you want all your logs, whether INFO, ERROR etc 
to be appended with function name? Its your choice, but ideally you 
should separate.

> +
> +#define IFPGA_RAWDEV_PMD_FUNC_TRACE() IFPGA_RAWDEV_PMD_LOG(DEBUG, ">>")
> +
> +#define IFPGA_RAWDEV_PMD_DEBUG(fmt, args...) \
> +	IFPGA_RAWDEV_PMD_LOG(DEBUG, fmt, ## args)
> +#define IFPGA_RAWDEV_PMD_INFO(fmt, args...) \
> +	IFPGA_RAWDEV_PMD_LOG(INFO, fmt, ## args)
> +#define IFPGA_RAWDEV_PMD_ERR(fmt, args...) \
> +	IFPGA_RAWDEV_PMD_LOG(ERR, fmt, ## args)
> +#define IFPGA_RAWDEV_PMD_WARN(fmt, args...) \
> +	IFPGA_RAWDEV_PMD_LOG(WARNING, fmt, ## args)
> +
> +enum ifpga_rawdev_device_state {
> +	IFPGA_IDLE,
> +	IFPGA_READY,
> +	IFPGA_ERROR
> +};
> +
> +static inline struct opae_adapter *
> +ifpga_rawdev_get_priv(const struct rte_rawdev *rawdev)
> +{
> +	return rawdev->dev_private;
> +}
> +
> +#endif /* _IFPGA_RAWDEV_H_ */
> diff --git a/drivers/raw/ifpga_rawdev/rte_ifpga_rawdev_version.map b/drivers/raw/ifpga_rawdev/rte_ifpga_rawdev_version.map
> new file mode 100644
> index 0000000..9b9ab1a
> --- /dev/null
> +++ b/drivers/raw/ifpga_rawdev/rte_ifpga_rawdev_version.map
> @@ -0,0 +1,4 @@
> +DPDK_18.05 {
> +
> +	local: *;
> +};
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index 9de2edb..4a76890 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -249,6 +249,7 @@ endif # CONFIG_RTE_LIBRTE_EVENTDEV
>   
>   ifeq ($(CONFIG_RTE_LIBRTE_RAWDEV),y)
>   _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_SKELETON_RAWDEV) += -lrte_pmd_skeleton_rawdev
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_IFPGA_RAWDEV)   += -lrte_ifpga_rawdev
>   endif # CONFIG_RTE_LIBRTE_RAWDEV
>   
>   
> 

How fast can you fix and push? I can spare time tomorrow (5/May) to 
re-review, if you can push by then. It might help in merging next week.

-
Shreyansh