> -----Original Message----- > From: Shreyansh Jain [mailto:shreyansh.j...@nxp.com] > Sent: Friday, May 4, 2018 5:15 PM > To: Xu, Rosen <rosen...@intel.com>; dev@dpdk.org > Cc: Doherty, Declan <declan.dohe...@intel.com>; Richardson, Bruce > <bruce.richard...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>; > Ananyev, Konstantin <konstantin.anan...@intel.com>; Zhang, Tianfei > <tianfei.zh...@intel.com>; Wu, Hao <hao...@intel.com>; > gaetan.ri...@6wind.com; Wu, Yanglong <yanglong...@intel.com> > Subject: Re: [dpdk-dev] [PATCH v6 3/5] iFPGA: Add Intel FPGA BUS Rawdev > Driver > > 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...@intel.com> > > > > Add Intel FPGA BUS Rawdev Driver which is based on librte_rawdev > > library. > > > > Signed-off-by: Rosen Xu <rosen...@intel.com> > > Signed-off-by: Yanglong Wu <yanglong...@intel.com> > > Signed-off-by: Figo zhang <tianfei.zh...@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
Hi Shreyansh: Thank you for your review, we will fix and push at next day. Best Tianfei
