[v3,2/5] raw/ifpga: remove vdev when ifpga is closed

  Virtual devices created on ifpga raw device will not be removed
when ifpga device has closed. To avoid resource leak problem,
this patch introduces an ifpga virtual device remove function,
virtual devices will be destroyed after the ifpga raw device closed.

Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
Cc: stable@dpdk.org

Signed-off-by: Wei Huang <wei.huang@intel.com>
---
v2: update commit log, use macro to replace immediate value.
---
 drivers/raw/ifpga/ifpga_rawdev.c | 166 ++++++++++++++++++++++++++++++---------
 drivers/raw/ifpga/ifpga_rawdev.h |   8 ++
 2 files changed, 138 insertions(+), 36 deletions(-)
  

Hi Wei,

My comments.

Thanks,
Rosen
> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Thursday, May 26, 2022 11:33
> To: dev@dpdk.org; thomas@monjalon.net; nipun.gupta@nxp.com;
> hemant.agrawal@nxp.com
> Cc: stable@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> <tianfei.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Huang, Wei
> <wei.huang@intel.com>
> Subject: [PATCH v3 2/5] raw/ifpga: remove vdev when ifpga is closed
> 
> Virtual devices created on ifpga raw device will not be removed when ifpga
> device has closed. To avoid resource leak problem, this patch introduces an
> ifpga virtual device remove function, virtual devices will be destroyed after
> the ifpga raw device closed.
> 
> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
> v2: update commit log, use macro to replace immediate value.
> ---
>  drivers/raw/ifpga/ifpga_rawdev.c | 166
> ++++++++++++++++++++++++++++++---------
>  drivers/raw/ifpga/ifpga_rawdev.h |   8 ++
>  2 files changed, 138 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index 6d4117c..fe3fc43 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -134,6 +134,8 @@ struct ifpga_rawdev *
>  	for (i = 0; i < IFPGA_MAX_IRQ; i++)
>  		dev->intr_handle[i] = NULL;
>  	dev->poll_enabled = 0;
> +	for (i = 0; i < IFPGA_MAX_VDEV; i++)
> +		dev->vdev_name[i] = NULL;
> 
>  	return dev;
>  }
> @@ -736,10 +738,22 @@ static int set_surprise_link_check_aer(  static int
> ifpga_rawdev_close(struct rte_rawdev *dev)  {
> +	struct ifpga_rawdev *ifpga_rdev = NULL;
>  	struct opae_adapter *adapter;
> +	char *vdev_name = NULL;
> +	int i = 0;
> 
>  	if (dev) {
> -		ifpga_monitor_stop_func(ifpga_rawdev_get(dev));
> +		ifpga_rdev = ifpga_rawdev_get(dev);
> +		if (ifpga_rdev) {
> +			for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> +				vdev_name = ifpga_rdev->vdev_name[i];
> +				if (vdev_name)
> +					rte_vdev_uninit(vdev_name);
> +			}
> +			ifpga_monitor_stop_func(ifpga_rdev);
> +			ifpga_rdev->rawdev = NULL;
> +		}
>  		adapter = ifpga_rawdev_get_priv(dev);
>  		if (adapter) {
>  			opae_adapter_destroy(adapter);
> @@ -1638,8 +1652,6 @@ static int fme_clean_fme_error(struct
> opae_manager *mgr)
>  		return -EINVAL;
>  	}
>  	dev = ifpga_rawdev_get(rawdev);
> -	if (dev)
> -		dev->rawdev = NULL;
> 
>  	adapter = ifpga_rawdev_get_priv(rawdev);
>  	if (!adapter)
> @@ -1714,73 +1726,118 @@ static int ifpga_rawdev_get_string_arg(const
> char *key __rte_unused,
> 
>  	return 0;
>  }
> +
>  static int
> -ifpga_cfg_probe(struct rte_vdev_device *dev)
> +ifpga_vdev_parse_devargs(struct rte_devargs *devargs,
> +	struct ifpga_vdev_args *args)
>  {
> -	struct rte_devargs *devargs;
> -	struct rte_kvargs *kvlist = NULL;
> -	struct rte_rawdev *rawdev = NULL;
> -	struct ifpga_rawdev *ifpga_dev;
> -	int port;
> +	struct rte_kvargs *kvlist;
>  	char *name = NULL;
> -	char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> -	int ret = -1;
> +	int port = 0;
> +	int ret = -EINVAL;
> 
> -	devargs = dev->device.devargs;
> +	if (!devargs || !args)
> +		return ret;
> 
>  	kvlist = rte_kvargs_parse(devargs->args, valid_args);
>  	if (!kvlist) {
> -		IFPGA_RAWDEV_PMD_LOG(ERR, "error when parsing
> param");
> -		goto end;
> +		IFPGA_RAWDEV_PMD_ERR("error when parsing devargs");
> +		return ret;
>  	}
> 
>  	if (rte_kvargs_count(kvlist, IFPGA_ARG_NAME) == 1) {
>  		if (rte_kvargs_process(kvlist, IFPGA_ARG_NAME,
> -				       &ifpga_rawdev_get_string_arg,
> -				       &name) < 0) {
> +			&ifpga_rawdev_get_string_arg, &name) < 0) {
>  			IFPGA_RAWDEV_PMD_ERR("error to parse %s",
> -				     IFPGA_ARG_NAME);
> +				IFPGA_ARG_NAME);
>  			goto end;
> +		} else {
> +			strlcpy(args->bdf, name, sizeof(args->bdf));
> +			rte_free(name);
>  		}
>  	} else {
>  		IFPGA_RAWDEV_PMD_ERR("arg %s is mandatory for ifpga
> bus",
> -			  IFPGA_ARG_NAME);
> +			IFPGA_ARG_NAME);
>  		goto end;
>  	}
> 
>  	if (rte_kvargs_count(kvlist, IFPGA_ARG_PORT) == 1) {
> -		if (rte_kvargs_process(kvlist,
> -			IFPGA_ARG_PORT,
> -			&rte_ifpga_get_integer32_arg,
> -			&port) < 0) {
> +		if (rte_kvargs_process(kvlist, IFPGA_ARG_PORT,
> +			&rte_ifpga_get_integer32_arg, &port) < 0) {
>  			IFPGA_RAWDEV_PMD_ERR("error to parse %s",
>  				IFPGA_ARG_PORT);
>  			goto end;
> +		} else {
> +			args->port = port;
>  		}
>  	} else {
>  		IFPGA_RAWDEV_PMD_ERR("arg %s is mandatory for ifpga
> bus",
> -			  IFPGA_ARG_PORT);
> +			IFPGA_ARG_PORT);
>  		goto end;
>  	}
> 
> +	ret = 0;
> +
> +end:
> +	if (kvlist)
> +		rte_kvargs_free(kvlist);
> +
> +	return ret;
> +}
> +
> +static int
> +ifpga_cfg_probe(struct rte_vdev_device *vdev) {
> +	struct rte_rawdev *rawdev = NULL;
> +	struct ifpga_rawdev *ifpga_dev;
> +	struct ifpga_vdev_args args;
> +	char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> +	const char *vdev_name = NULL;
> +	int i, n, ret = 0;
> +
> +	vdev_name = rte_vdev_device_name(vdev);
> +	if (!vdev_name)
> +		return -EINVAL;
> +
> +	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> vdev_name);
> +
> +	ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> +	if (ret)
> +		return ret;
> +
>  	memset(dev_name, 0, sizeof(dev_name));
> -	snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> name);
> +	snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> args.bdf);
>  	rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
>  	if (!rawdev)
> -		goto end;
> +		return -ENODEV;
>  	ifpga_dev = ifpga_rawdev_get(rawdev);
>  	if (!ifpga_dev)
> -		goto end;
> +		return -ENODEV;
> 
> -	memset(dev_name, 0, sizeof(dev_name));
> -	snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> -	port, name);
> +	for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> +		if (ifpga_dev->vdev_name[i] == NULL) {
> +			n = strlen(vdev_name) + 1;
> +			ifpga_dev->vdev_name[i] = rte_malloc(NULL, n, 0);
> +			if (ifpga_dev->vdev_name[i] == NULL)
> +				return -ENOMEM;
> +			strlcpy(ifpga_dev->vdev_name[i], vdev_name, n);
> +			break;
> +		}
> +	}
> 
> +	if (i >= IFPGA_MAX_VDEV) {
> +		IFPGA_RAWDEV_PMD_ERR("Can't create more virtual
> device!");
> +		return -ENOENT;
> +	}
> +
> +	snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> +		args.port, args.bdf);
>  	ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> -			dev_name, devargs->args);
> -end:
> -	rte_kvargs_free(kvlist);
> -	free(name);
> +			dev_name, vdev->device.devargs->args);
> +	if (ret) {
> +		rte_free(ifpga_dev->vdev_name[i]);
> +		ifpga_dev->vdev_name[i] = NULL;
> +	}
> 
>  	return ret;
>  }
> @@ -1788,10 +1845,47 @@ static int ifpga_rawdev_get_string_arg(const
> char *key __rte_unused,  static int  ifpga_cfg_remove(struct
> rte_vdev_device *vdev)  {
> -	IFPGA_RAWDEV_PMD_INFO("Remove ifpga_cfg %p",
> -		vdev);
> +	struct rte_rawdev *rawdev = NULL;
> +	struct ifpga_rawdev *ifpga_dev;
> +	struct ifpga_vdev_args args;
> +	char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> +	const char *vdev_name = NULL;
> +	char *tmp_vdev = NULL;
> +	int i, ret = 0;
> 
> -	return 0;
> +	vdev_name = rte_vdev_device_name(vdev);
> +	if (!vdev_name)
> +		return -EINVAL;
> +
> +	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> vdev_name);
> +
> +	ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> +	if (ret)
> +		return ret;
> +
> +	memset(dev_name, 0, sizeof(dev_name));
> +	snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> args.bdf);
> +	rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> +	if (!rawdev)
> +		return -ENODEV;
> +	ifpga_dev = ifpga_rawdev_get(rawdev);
> +	if (!ifpga_dev)
> +		return -ENODEV;
> +
> +	snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> +		args.port, args.bdf);
> +	ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
> dev_name);
> +
> +	for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> +		tmp_vdev = ifpga_dev->vdev_name[i];
> +		if (tmp_vdev && !strcmp(tmp_vdev, vdev_name)) {
> +			free(tmp_vdev);
> +			ifpga_dev->vdev_name[i] = NULL;
> +			break;
> +		}
> +	}
> +
> +	return ret;
>  }
> 
>  static struct rte_vdev_driver ifpga_cfg_driver = { diff --git
> a/drivers/raw/ifpga/ifpga_rawdev.h b/drivers/raw/ifpga/ifpga_rawdev.h
> index 857b734..eb9a9a5 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.h
> +++ b/drivers/raw/ifpga/ifpga_rawdev.h
> @@ -50,6 +50,7 @@ enum ifpga_rawdev_device_state {
> 
>  #define IFPGA_RAWDEV_MSIX_IRQ_NUM 7
>  #define IFPGA_RAWDEV_NUM 32
> +#define IFPGA_MAX_VDEV 4
>  #define IFPGA_MAX_IRQ 12
> 
>  struct ifpga_rawdev {
> @@ -64,6 +65,13 @@ struct ifpga_rawdev {
>  	void *intr_handle[IFPGA_MAX_IRQ];
>  	/* enable monitor thread poll device's sensors or not */
>  	int poll_enabled;
> +	/* name of virtual devices created on raw device */
> +	char *vdev_name[IFPGA_MAX_VDEV];
> +};
> +
> +struct ifpga_vdev_args {
> +	char bdf[8];
It seems like a PCIe bdf name, can you replace the constant with PCI_PRI_STR_SIZE?
> +	int port;
>  };
> 
>  struct ifpga_rawdev *
> --
> 1.8.3.1