raw/ifpga: remove virtual device unplug operation

PATCH Applied Wei Huang 4 participants 14 messages Mar 17, 2023 – Mar 24, 2023

Share & tools

Wei Huang

author

2023-03-17 01:44 UTC #0 lore raw

PATCH 1/1 raw/ifpga: remove virtual device unplug operation

VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.

Signed-off-by Wei Huang <[email protected]>

---
 drivers/raw/ifpga/ifpga_rawdev.c | 99 ++++------------------------------------
 drivers/raw/ifpga/ifpga_rawdev.h |  2 -
 2 files changed, 9 insertions(+), 92 deletions(-)

drivers/raw/ifpga/ifpga_rawdev.c +9 ~~-90~~

index 1020adc..8e6e70f 100644
@@ -140,8 +140,6 @@ 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;
 }

@@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
 	struct ifpga_rawdev *ifpga_rdev = NULL;
 	struct opae_adapter *adapter;
 	struct opae_manager *mgr;
-	char *vdev_name = NULL;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (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;
 		}

@@ -1778,104 +1770,31 @@ static int ifpga_rawdev_get_string_arg(const char *key __rte_unused,
 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;
+	int ret = 0;
 
-	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s", vdev_name);
+	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
 	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;
-
-	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, vdev->device.devargs->args);
-	if (ret) {
-		rte_free(ifpga_dev->vdev_name[i]);
-		ifpga_dev->vdev_name[i] = NULL;
-	}
-
-	return ret;
+	return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME), dev_name,
+			vdev->device.devargs->args);
 }
 
 static int
 ifpga_cfg_remove(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;
-	char *tmp_vdev = NULL;
-	int i, ret = 0;
-
-	vdev_name = rte_vdev_device_name(vdev);
-	if (!vdev_name)
-		return -EINVAL;
+	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
-	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;
+	return 0;
 }
 
 static struct rte_vdev_driver ifpga_cfg_driver = {

drivers/raw/ifpga/ifpga_rawdev.h +0 -2

index 0fb66cb..1c128c7 100644
@@ -65,8 +65,6 @@ 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 {

Signature · 1 line

1.8.3.1

Xu, Rosen

Acked-by on v1 1/1

2023-03-20 06:52 UTC #1 lore raw

Hi,

Quoted 184 lines

-----Original Message-----
From: Huang, Wei <[email protected]>
Sent: Friday, March 17, 2023 4:45 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; Xu, Rosen <[email protected]>; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>; Huang, Wei <[email protected]>
Subject: [PATCH v1] raw/ifpga: remove virtual device unplug operation

VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.

Signed-off-by Wei Huang <[email protected]>

---
 drivers/raw/ifpga/ifpga_rawdev.c | 99 ++++------------------------------------
 drivers/raw/ifpga/ifpga_rawdev.h |  2 -
 2 files changed, 9 insertions(+), 92 deletions(-)

drivers/raw/ifpga/ifpga_rawdev.c +1 -9

index 1020adc..8e6e70f 100644
@@ -140,8 +140,6 @@ 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;
 }

@@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
 	struct ifpga_rawdev *ifpga_rdev = NULL;
 	struct opae_adapter *adapter;
 	struct opae_manager *mgr;
-	char *vdev_name = NULL;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (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;
 		}

@@ -1778,104 +1770,31 @@ static int ifpga_rawdev_get_string_arg(const

char *key __rte_unused, static int ifpga_cfg_probe(struct rte_vdev_device *vdev) {

drivers/raw/ifpga/ifpga_rawdev.c +1 -9

-	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;
+	int ret = 0;
 
-	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",

vdev_name);

drivers/raw/ifpga/ifpga_rawdev.c +2 -1

+	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
 	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);

drivers/raw/ifpga/ifpga_rawdev.c +0 ~~-20~~

-	rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
-	if (!rawdev)
-		return -ENODEV;
-	ifpga_dev = ifpga_rawdev_get(rawdev);
-	if (!ifpga_dev)
-		return -ENODEV;
-
-	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!");

drivers/raw/ifpga/ifpga_rawdev.c +1 ~~-11~~

-		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, vdev->device.devargs->args);
-	if (ret) {
-		rte_free(ifpga_dev->vdev_name[i]);
-		ifpga_dev->vdev_name[i] = NULL;
-	}
-
-	return ret;
+	return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),

dev_name,

drivers/raw/ifpga/ifpga_rawdev.c +3 ~~-12~~

+			vdev->device.devargs->args);
 }
 
 static int
 ifpga_cfg_remove(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;
-	char *tmp_vdev = NULL;
-	int i, ret = 0;
-
-	vdev_name = rte_vdev_device_name(vdev);
-	if (!vdev_name)
-		return -EINVAL;
+	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
-	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",

vdev_name);

drivers/raw/ifpga/ifpga_rawdev.c +0 -7

-
-	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);

drivers/raw/ifpga/ifpga_rawdev.c +0 ~~-10~~

-	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);

drivers/raw/ifpga/ifpga_rawdev.c +1 ~~-11~~

-
-	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;
+	return 0;
 }
 
 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 0fb66cb..1c128c7 100644

drivers/raw/ifpga/ifpga_rawdev.h +0 -3

@@ -65,8 +65,6 @@ 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 {
--

Quoted 1 line

1.8.3.1

Acked-by Rosen Xu <[email protected]>

Zhang, Qi Z

Applied

2023-03-20 12:59 UTC #2 lore raw

Quoted 197 lines

-----Original Message-----
From: Xu, Rosen <[email protected]>
Sent: Monday, March 20, 2023 2:52 PM
To: Huang, Wei <[email protected]>; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>
Subject: RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation

Hi,

Quoted 184 lines

VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.

Signed-off-by Wei Huang <[email protected]>

---
 drivers/raw/ifpga/ifpga_rawdev.c | 99
++++------------------------------------
 drivers/raw/ifpga/ifpga_rawdev.h |  2 -
 2 files changed, 9 insertions(+), 92 deletions(-)

drivers/raw/ifpga/ifpga_rawdev.c +1 -9

index 1020adc..8e6e70f 100644
@@ -140,8 +140,6 @@ 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;
 }

@@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
 	struct ifpga_rawdev *ifpga_rdev = NULL;
 	struct opae_adapter *adapter;
 	struct opae_manager *mgr;
-	char *vdev_name = NULL;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (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;
 		}

@@ -1778,104 +1770,31 @@ static int ifpga_rawdev_get_string_arg(const

char *key __rte_unused, static int ifpga_cfg_probe(struct rte_vdev_device
*vdev) {

drivers/raw/ifpga/ifpga_rawdev.c +1 -9

-	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;
+	int ret = 0;
 
-	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",

vdev_name);

drivers/raw/ifpga/ifpga_rawdev.c +2 -1

+	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
 	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);

drivers/raw/ifpga/ifpga_rawdev.c +0 ~~-20~~

-	rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
-	if (!rawdev)
-		return -ENODEV;
-	ifpga_dev = ifpga_rawdev_get(rawdev);
-	if (!ifpga_dev)
-		return -ENODEV;
-
-	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!");

drivers/raw/ifpga/ifpga_rawdev.c +1 ~~-11~~

-		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, vdev->device.devargs->args);
-	if (ret) {
-		rte_free(ifpga_dev->vdev_name[i]);
-		ifpga_dev->vdev_name[i] = NULL;
-	}
-
-	return ret;
+	return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),

dev_name,

drivers/raw/ifpga/ifpga_rawdev.c +3 ~~-12~~

+			vdev->device.devargs->args);
 }
 
 static int
 ifpga_cfg_remove(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;
-	char *tmp_vdev = NULL;
-	int i, ret = 0;
-
-	vdev_name = rte_vdev_device_name(vdev);
-	if (!vdev_name)
-		return -EINVAL;
+	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
-	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",

vdev_name);

drivers/raw/ifpga/ifpga_rawdev.c +0 -7

-
-	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);

drivers/raw/ifpga/ifpga_rawdev.c +0 ~~-10~~

-	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);

drivers/raw/ifpga/ifpga_rawdev.c +1 ~~-11~~

-
-	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;
+	return 0;
 }
 
 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 0fb66cb..1c128c7 100644

drivers/raw/ifpga/ifpga_rawdev.h +0 -2

@@ -65,8 +65,6 @@ 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];
 };

Quoted 5 lines

Quoted 3 lines
drivers/raw/ifpga/ifpga_rawdev.h +0 -1
 struct ifpga_vdev_args {
--
1.8.3.1
Acked-by Rosen Xu <[email protected]>

Applied to dpdk-next-net-intel.

Thanks
Qi

Zhang, Qi Z

2023-03-20 13:00 UTC #3 lore raw

Quoted 216 lines

-----Original Message-----
From: Zhang, Qi Z
Sent: Monday, March 20, 2023 8:59 PM
To: Xu, Rosen <[email protected]>; Huang, Wei <[email protected]>; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Zhang, Tianfei <[email protected]>
Subject: RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation

Quoted 198 lines

Hi,

Quoted 186 lines

VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.

Signed-off-by Wei Huang <[email protected]>

---
 drivers/raw/ifpga/ifpga_rawdev.c | 99
++++------------------------------------
 drivers/raw/ifpga/ifpga_rawdev.h |  2 -
 2 files changed, 9 insertions(+), 92 deletions(-)

drivers/raw/ifpga/ifpga_rawdev.c +1 -9

index 1020adc..8e6e70f 100644
@@ -140,8 +140,6 @@ 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;
 }

@@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
 	struct ifpga_rawdev *ifpga_rdev = NULL;
 	struct opae_adapter *adapter;
 	struct opae_manager *mgr;
-	char *vdev_name = NULL;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (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;
 		}

@@ -1778,104 +1770,31 @@ static int

ifpga_rawdev_get_string_arg(const char *key __rte_unused, static int ifpga_cfg_probe(struct rte_vdev_device
*vdev) {

drivers/raw/ifpga/ifpga_rawdev.c +1 -9

-	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;
+	int ret = 0;
 
-	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",

vdev_name);

drivers/raw/ifpga/ifpga_rawdev.c +2 -1

+	IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
 	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);

drivers/raw/ifpga/ifpga_rawdev.c +0 ~~-20~~

-	rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
-	if (!rawdev)
-		return -ENODEV;
-	ifpga_dev = ifpga_rawdev_get(rawdev);
-	if (!ifpga_dev)
-		return -ENODEV;
-
-	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!");

drivers/raw/ifpga/ifpga_rawdev.c +1 ~~-11~~

-		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, vdev->device.devargs->args);
-	if (ret) {
-		rte_free(ifpga_dev->vdev_name[i]);
-		ifpga_dev->vdev_name[i] = NULL;
-	}
-
-	return ret;
+	return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),

dev_name,

drivers/raw/ifpga/ifpga_rawdev.c +3 ~~-12~~

+			vdev->device.devargs->args);
 }
 
 static int
 ifpga_cfg_remove(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;
-	char *tmp_vdev = NULL;
-	int i, ret = 0;
-
-	vdev_name = rte_vdev_device_name(vdev);
-	if (!vdev_name)
-		return -EINVAL;
+	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
+		rte_vdev_device_name(vdev));
 
-	IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",

vdev_name);

drivers/raw/ifpga/ifpga_rawdev.c +0 -7

-
-	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);

drivers/raw/ifpga/ifpga_rawdev.c +0 ~~-10~~

-	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);

drivers/raw/ifpga/ifpga_rawdev.c +1 ~~-11~~

-
-	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;
+	return 0;
 }
 
 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 0fb66cb..1c128c7 100644

drivers/raw/ifpga/ifpga_rawdev.h +0 -2

@@ -65,8 +65,6 @@ 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];
 };

Quoted 5 lines

Quoted 3 lines
drivers/raw/ifpga/ifpga_rawdev.h +0 -1
 struct ifpga_vdev_args {
--
1.8.3.1
Acked-by Rosen Xu <[email protected]>

Quoted 4 lines

Applied to dpdk-next-net-intel.
Thanks
Qi

Sorry, this didnt happen, I replied on the wrong email.

Thomas Monjalon

2023-03-20 16:06 UTC #4 lore raw

In reply to Wei Huang

16/03/2023 21:44, Wei Huang:

Quoted 3 lines

VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.

Why no need?
If the application wants to explicitly remove a device, what happens?

Huang, Wei

author

2023-03-21 00:11 UTC #5 lore raw

Hi Tomas,

Quoted 9 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Tuesday, March 21, 2023 00:06
To: Huang, Wei <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Xu, Rosen <[email protected]>; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>
Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation

Quoted 9 lines

16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.
Why no need?
If the application wants to explicitly remove a device, what happens?

EAL will output an error information "Cannot find plugged device (%s)".

Thomas Monjalon

2023-03-21 08:14 UTC #6 lore raw

21/03/2023 01:11, Huang, Wei:

Quoted 11 lines

From: Thomas Monjalon <[email protected]>
Quoted 9 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.
Why no need?
If the application wants to explicitly remove a device, what happens?
EAL will output an error information "Cannot find plugged device (%s)".

It does not look what we expect.

Huang, Wei

author

2023-03-21 08:41 UTC #7 lore raw

Hi Thomas,

Quoted 16 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Tuesday, March 21, 2023 16:14
To: Huang, Wei <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Xu, Rosen <[email protected]>; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>
Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
21/03/2023 01:11, Huang, Wei:
Quoted 6 lines
From: Thomas Monjalon <[email protected]>
Quoted 5 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
Why no need?
If the application wants to explicitly remove a device, what happens?
EAL will output an error information "Cannot find plugged device (%s)".
It does not look what we expect.

Let me clear it.
With this patch, no error information will be outputted.
Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup, bus->find_device() returns NULL, EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c

Thomas Monjalon

2023-03-21 10:30 UTC #8 lore raw

21/03/2023 09:41, Huang, Wei:

Quoted 14 lines

From: Thomas Monjalon <[email protected]>
Quoted 13 lines
21/03/2023 01:11, Huang, Wei:
Quoted 11 lines
From: Thomas Monjalon <[email protected]>
Quoted 9 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.
Why no need?
If the application wants to explicitly remove a device, what happens?
EAL will output an error information "Cannot find plugged device (%s)".

Quoted 7 lines

Quoted 2 lines
It does not look what we expect.
Let me clear it.
With this patch, no error information will be outputted. Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup,

Why ipfga unplug the device after the bus cleanup? I'm not following.

Quoted 2 lines

bus->find_device() returns NULL,
EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c

Anyway, the good answer is not to completely remove the "remove" operation.

Huang, Wei

author

2023-03-22 01:27 UTC #9 lore raw

Quoted 28 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Tuesday, March 21, 2023 18:30
To: Huang, Wei <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Xu, Rosen <[email protected]>; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>
Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
21/03/2023 09:41, Huang, Wei:
Quoted 16 lines
From: Thomas Monjalon <[email protected]>
Quoted 15 lines
21/03/2023 01:11, Huang, Wei:
Quoted 11 lines
From: Thomas Monjalon <[email protected]>
Quoted 9 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.
Why no need?
If the application wants to explicitly remove a device, what happens?
EAL will output an error information "Cannot find plugged device (%s)".
It does not look what we expect.

Quoted 9 lines

Quoted 5 lines
Let me clear it.
With this patch, no error information will be outputted. Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup,
Why ipfga unplug the device after the bus cleanup? I'm not following.

The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup, ifpga has the responsibility to unplug these virtual devices.

Quoted 7 lines

Quoted 3 lines
bus->find_device() returns NULL,
EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c
Anyway, the good answer is not to completely remove the "remove" operation.

If not to completely remove the "remove", the same virtual device will be unplug twice, is it reasonable?

Thomas Monjalon

2023-03-22 11:54 UTC #10 lore raw

22/03/2023 02:26, Huang, Wei:

Quoted 24 lines

From: Thomas Monjalon <[email protected]>
Quoted 23 lines
21/03/2023 09:41, Huang, Wei:
Quoted 21 lines
From: Thomas Monjalon <[email protected]>
Quoted 7 lines
21/03/2023 01:11, Huang, Wei:
Quoted 6 lines
From: Thomas Monjalon <[email protected]>
Quoted 5 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and unplug them.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Why no need?
If the application wants to explicitly remove a device, what happens?
EAL will output an error information "Cannot find plugged device (%s)".
It does not look what we expect.
Let me clear it.
With this patch, no error information will be outputted. Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup,

Quoted 5 lines

Quoted 3 lines
Why ipfga unplug the device after the bus cleanup? I'm not following.
The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup, ifpga has the responsibility to unplug these virtual devices.

Really I don't understand the flow.
Are you talking about EAL cleanup case?
What happens first? Do you need ifpga to be called first? I think you need the correct checks to allow any order of cleanup.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
bus->find_device() returns NULL,
EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c
Anyway, the good answer is not to completely remove the "remove" operation.
If not to completely remove the "remove", the same virtual device will be unplug twice, is it reasonable?

You need to add a check to not unplug something already unplugged. But you must allow the user calling "remove" directly.

Huang, Wei

author

2023-03-23 03:26 UTC #11 lore raw

Quoted 39 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Wednesday, March 22, 2023 19:54
To: Huang, Wei <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Xu, Rosen <[email protected]>; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>
Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
22/03/2023 02:26, Huang, Wei:
Quoted 9 lines
From: Thomas Monjalon <[email protected]>
Quoted 8 lines
21/03/2023 09:41, Huang, Wei:
Quoted 7 lines
From: Thomas Monjalon <[email protected]>
Quoted 6 lines
21/03/2023 01:11, Huang, Wei:
Quoted 5 lines
From: Thomas Monjalon <[email protected]>
Quoted 4 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and
unplug them.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Why no need?
If the application wants to explicitly remove a device, what
happens?
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
EAL will output an error information "Cannot find plugged device
(%s)".
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
It does not look what we expect.
Let me clear it.
With this patch, no error information will be outputted. Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup,

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Why ipfga unplug the device after the bus cleanup? I'm not following.
The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup, ifpga has the responsibility to unplug these virtual devices.
Really I don't understand the flow.
Are you talking about EAL cleanup case?

Yes, it's about EAL cleanup.

Quoted 1 line

What happens first? Do you need ifpga to be called first?

The cleanup flow is rte_eal_cleanup() -> eal_bus_cleanup()
eal_bus_cleanup() will call each bus's cleanup method to complete cleanup work.
There are three types of device related to ifpga PMD: PCI device, VDEV device and AFU device.
VDEV device is created on PCI device, it's a mediate device which purpose is to plug a AFU device on IFPGA bus.
Before eal_bus_cleanup() is implemented, application will call rte_pmd_ifpga_cleanup() to remove PCI device, VDEV device will be removed when PCI device is removed, AFU device will be removed when VDEV device is removed, it works fine. Now eal_bus_cleanup() takes the job, application has no need to call rte_pmd_ifpga_cleanup(), ifpga PCI device has no need to remove ifpga VDEV device and ifpga VDEV device has no need to remove ifpga AFU device.

Quoted 1 line

I think you need the correct checks to allow any order of cleanup.

When this patch is committed, no order dependent on cleanup.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
bus->find_device() returns NULL,
EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c

Quoted 9 lines

Quoted 4 lines
Quoted 3 lines
Anyway, the good answer is not to completely remove the "remove" operation.
If not to completely remove the "remove", the same virtual device will be
unplug twice, is it reasonable?
You need to add a check to not unplug something already unplugged. But you must allow the user calling "remove" directly.

rte_pmd_ifpga_cleanup() is the only one interface for user to calling "remove" directly, when this patch is committed, VDEV and AFU device will not be unplugged twice. For PCI device, the implementation of rte_pmd_ifpga_cleanup() is like below

	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
		dev = &ifpga_rawdevices[i];
		if (dev->rawdev) {
			rte_rawdev_pmd_release(dev->rawdev);
			dev->rawdev = NULL;
		}
	}

If ifpga PCI device is already removed, dev->rawdev is NULL, it will not be unplugged again.

Thomas Monjalon

2023-03-23 08:52 UTC #12 lore raw

23/03/2023 04:26, Huang, Wei:

Quoted 37 lines

From: Thomas Monjalon <[email protected]>
Quoted 28 lines
22/03/2023 02:26, Huang, Wei:
Quoted 9 lines
From: Thomas Monjalon <[email protected]>
Quoted 8 lines
21/03/2023 09:41, Huang, Wei:
Quoted 7 lines
From: Thomas Monjalon <[email protected]>
Quoted 6 lines
21/03/2023 01:11, Huang, Wei:
Quoted 5 lines
From: Thomas Monjalon <[email protected]>
Quoted 4 lines
16/03/2023 21:44, Wei Huang:
Quoted 3 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and
unplug them.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Why no need?
If the application wants to explicitly remove a device, what
happens?
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
EAL will output an error information "Cannot find plugged device
(%s)".
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
It does not look what we expect.
Let me clear it.
With this patch, no error information will be outputted. Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup,
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Why ipfga unplug the device after the bus cleanup? I'm not following.
The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup, ifpga has the responsibility to unplug these virtual devices.
Really I don't understand the flow.
Are you talking about EAL cleanup case?

Quoted 8 lines

Yes, it's about EAL cleanup.
Quoted 1 line
What happens first? Do you need ifpga to be called first?
The cleanup flow is rte_eal_cleanup() -> eal_bus_cleanup()
eal_bus_cleanup() will call each bus's cleanup method to complete cleanup work.
There are three types of device related to ifpga PMD: PCI device, VDEV device and AFU device.
VDEV device is created on PCI device, it's a mediate device which purpose is to plug a AFU device on IFPGA bus.
Before eal_bus_cleanup() is implemented, application will call rte_pmd_ifpga_cleanup() to remove PCI device, VDEV device will be removed when PCI device is removed, AFU device will be removed when VDEV device is removed, it works fine. Now eal_bus_cleanup() takes the job, application has no need to call rte_pmd_ifpga_cleanup(), ifpga PCI device has no need to remove ifpga VDEV device and ifpga VDEV device has no need to remove ifpga AFU device.

That's the problem in your approach.
You need to keep the code removing children devices.
And if all children are removed, then the parent should remove itself. If you implement these 2 conditions, the cleanup can happen in any order.

Quoted 10 lines

Quoted 1 line
I think you need the correct checks to allow any order of cleanup.
When this patch is committed, no order dependent on cleanup.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
bus->find_device() returns NULL,
EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c
Anyway, the good answer is not to completely remove the "remove" operation.

Quoted 7 lines

Quoted 6 lines
Quoted 1 line
If not to completely remove the "remove", the same virtual device will be
unplug twice, is it reasonable?
You need to add a check to not unplug something already unplugged. But you must allow the user calling "remove" directly.
rte_pmd_ifpga_cleanup() is the only one interface for user to calling "remove" directly,

No, there are functions in EAL to "remove" devices, like rte_dev_remove(), and we must make sure it works effectively.

Quoted 10 lines

when this patch is committed, VDEV and AFU device will not be unplugged twice. For PCI device, the implementation of rte_pmd_ifpga_cleanup() is like below
	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
		dev = &ifpga_rawdevices[i];
		if (dev->rawdev) {
			rte_rawdev_pmd_release(dev->rawdev);
			dev->rawdev = NULL;
		}
	}
If ifpga PCI device is already removed, dev->rawdev is NULL, it will not be unplugged again.

Huang, Wei

author

2023-03-24 01:27 UTC #13 lore raw

Quoted 67 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Thursday, March 23, 2023 16:52
To: Huang, Wei <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Xu, Rosen <[email protected]>; Zhang, Tianfei <[email protected]>; Zhang, Qi Z <[email protected]>
Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
23/03/2023 04:26, Huang, Wei:
Quoted 38 lines
From: Thomas Monjalon <[email protected]>
Quoted 37 lines
22/03/2023 02:26, Huang, Wei:
Quoted 10 lines
From: Thomas Monjalon <[email protected]>
Quoted 9 lines
21/03/2023 09:41, Huang, Wei:
Quoted 8 lines
From: Thomas Monjalon <[email protected]>
Quoted 7 lines
21/03/2023 01:11, Huang, Wei:
Quoted 6 lines
From: Thomas Monjalon <[email protected]>
Quoted 5 lines
16/03/2023 21:44, Wei Huang:
Quoted 4 lines
VDEV bus has implemented cleanup() function to perform cleanup for devices on the bus during eal_cleanup(), so there is no need for ifpga driver to record virtual devices and
unplug them.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Why no need?
If the application wants to explicitly remove a device, what
happens?
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
EAL will output an error information "Cannot find plugged device
(%s)".
Quoted 15 lines
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
It does not look what we expect.
Let me clear it.
With this patch, no error information will be outputted. Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup,
Quoted 3 lines
Why ipfga unplug the device after the bus cleanup? I'm not following.
The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup, ifpga has the responsibility to unplug these virtual
devices.
Quoted 7 lines
Quoted 3 lines
Really I don't understand the flow.
Are you talking about EAL cleanup case?
Yes, it's about EAL cleanup.
Quoted 1 line
What happens first? Do you need ifpga to be called first?
The cleanup flow is rte_eal_cleanup() -> eal_bus_cleanup() eal_bus_cleanup() will call each bus's cleanup method to complete cleanup
work.
Quoted 1 line
There are three types of device related to ifpga PMD: PCI device, VDEV
device and AFU device.
Quoted 1 line
VDEV device is created on PCI device, it's a mediate device which purpose
is to plug a AFU device on IFPGA bus.
Quoted 1 line
Before eal_bus_cleanup() is implemented, application will call
rte_pmd_ifpga_cleanup() to remove PCI device, VDEV device will be removed when PCI device is removed, AFU device will be removed when VDEV device is removed, it works fine.

Quoted 9 lines

Quoted 1 line
Now eal_bus_cleanup() takes the job, application has no need to call
rte_pmd_ifpga_cleanup(), ifpga PCI device has no need to remove ifpga VDEV device and ifpga VDEV device has no need to remove ifpga AFU device.
That's the problem in your approach.
You need to keep the code removing children devices.
And if all children are removed, then the parent should remove itself. If you implement these 2 conditions, the cleanup can happen in any order.

OK, this patch should be rejected, I will commit another patch according to your comments.

Quoted 39 lines

Quoted 12 lines
Quoted 1 line
I think you need the correct checks to allow any order of cleanup.
When this patch is committed, no order dependent on cleanup.
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
bus->find_device() returns NULL,
EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c
Anyway, the good answer is not to completely remove the "remove" operation.
If not to completely remove the "remove", the same virtual device will be
Quoted 7 lines
Quoted 5 lines
unplug twice, is it reasonable?
You need to add a check to not unplug something already unplugged. But you must allow the user calling "remove" directly.
rte_pmd_ifpga_cleanup() is the only one interface for user to calling "remove" directly,
No, there are functions in EAL to "remove" devices, like rte_dev_remove(), and we must make sure it works effectively.
Quoted 1 line
when this patch is committed, VDEV and AFU device will not be unplugged
twice.
Quoted 1 line
For PCI device, the implementation of rte_pmd_ifpga_cleanup() is like
below
Quoted 8 lines
	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
		dev = &ifpga_rawdevices[i];
		if (dev->rawdev) {
			rte_rawdev_pmd_release(dev->rawdev);
			dev->rawdev = NULL;
		}
	}
If ifpga PCI device is already removed, dev->rawdev is NULL, it will not be
unplugged again.