RE: [PATCH v3] raw/ifpga: remove vdev when ifpga is closed

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Tuesday, March 29, 2022 9:56 AM
> To: dev@dpdk.org; thomas@monjalon.net; Xu, Rosen
> <rosen.xu@intel.com>; nipun.gupta@nxp.com; hemant.agrawal@nxp.com
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: RE: [PATCH v3] raw/ifpga: remove vdev when ifpga is closed
> 
> Hi Thomas,
> 
> Would you please check this patch is acceptable ?

Hi Wei:
	22.07 cycle just begins, It may not necessary to push PMD merge quite early 😊
	Below is the roadmap for your reference

http://core.dpdk.org/roadmap/

Proposal deadline (RFC/v1 patches): 10 April 2022
API freeze (-rc1): 30 May 2022
PMD features freeze (-rc2): 20 June 2022
Builtin applications features freeze (-rc3): 27 June 2022
Release: 13 July 2022

Regards
Qi

> 
> > -----Original Message-----
> > From: Huang, Wei <wei.huang@intel.com>
> > Sent: Friday, March 18, 2022 16:58
> > To: dev@dpdk.org; thomas@monjalon.net; Xu, Rosen
> <rosen.xu@intel.com>;
> > nipun.gupta@nxp.com; hemant.agrawal@nxp.com
> > Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Zhang,
> > Qi Z <qi.z.zhang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Huang, Wei <wei.huang@intel.com>
> > Subject: [PATCH v3] raw/ifpga: remove vdev when ifpga is closed
> >
> > Virtual devices created on ifpga raw device are not removed when ifpga
> > is closed. To avoid such problem, ifpga virtual device remove function
> > is implemented, virtual device is removed in raw device close function.
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > Acked-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > Acked-by: Rosen Xu <rosen.xu@intel.com>
> > ---
> > v2: cleanup vdev_name no matter rte_vdev_uninit succeed or not
> > ---
> > v3: cleanup vdev_name in ifpga_cfg_remove
> > ---
> >  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];
> > +	int port;
> >  };
> >
> >  struct ifpga_rawdev *
> > --
> > 1.8.3.1
>