[dpdk-dev] [PATCH] Remove RTE_EAL_UNBIND_PORTS-related code

[dpdk-dev] [PATCH] Remove RTE_EAL_UNBIND_PORTS-related code

[Burakov, Anatoly]

RTE_EAL_UNBIND_PORTS was deprecated in DPDK 1.4.0 and removed in 1.6.0, but the code was not removed.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 221 ----------------------------------
 1 file changed, 221 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 5fa466e..9e5a139 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -109,199 +109,6 @@ static int pci_parse_sysfs_value(const char *filename, uint64_t *val);
 /* forward prototype of function called in pci_switch_module below */
 static int pci_uio_map_resource(struct rte_pci_device *dev);
 
-#ifdef RTE_EAL_UNBIND_PORTS
-#define PROC_MODULES "/proc/modules"
-
-#define IGB_UIO_NAME "igb_uio"
-
-#define UIO_DRV_PATH  "/sys/bus/pci/drivers/%s"
-
-/* maximum time to wait that /dev/uioX appears */
-#define UIO_DEV_WAIT_TIMEOUT 3 /* seconds */
-
-/*
- * Check that a kernel module is loaded. Returns 0 on success, or if the
- * parameter is NULL, or -1 if the module is not loaded.
- */
-static int
-pci_uio_check_module(const char *module_name)
-{
-	FILE *f;
-	unsigned i;
-	char buf[BUFSIZ];
-
-	if (module_name == NULL)
-		return 0;
-
-	f = fopen(PROC_MODULES, "r");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot open "PROC_MODULES": %s\n", 
-				strerror(errno));
-		return -1;
-	}
-
-	while(fgets(buf, sizeof(buf), f) != NULL) {
-
-		for (i = 0; i < sizeof(buf) && buf[i] != '\0'; i++) {
-			if (isspace(buf[i]))
-			    buf[i] = '\0';
-		}
-
-		if (strncmp(buf, module_name, sizeof(buf)) == 0) {
-			fclose(f);
-			return 0;
-		}
-	}
-	fclose(f);
-	return -1;
-}
-
-/* bind a PCI to the kernel module driver */
-static int
-pci_bind_device(struct rte_pci_device *dev, char dr_path[])
-{
-	FILE *f;
-	int n;
-	char buf[BUFSIZ];
-	char dev_bind[PATH_MAX];
-	struct rte_pci_addr *loc = &dev->addr;
-
-	n = rte_snprintf(dev_bind, sizeof(dev_bind), "%s/bind", dr_path);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf device bind path\n");
-		return -1;
-	}
-
-	f = fopen(dev_bind, "w");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot open %s\n", dev_bind);
-		return -1;
-	}
-	n = rte_snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
-	                 loc->domain, loc->bus, loc->devid, loc->function);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf PCI infos\n");
-		fclose(f);
-		return -1;
-	}
-	if (fwrite(buf, n, 1, f) == 0) {
-		fclose(f);
-		return -1;
-	}
-
-	fclose(f);
-	return 0;
-}
-
-static int
-pci_uio_bind_device(struct rte_pci_device *dev, const char *module_name)
-{
-	FILE *f;
-	int n;
-	char buf[BUFSIZ];
-	char uio_newid[PATH_MAX];
-	char uio_bind[PATH_MAX];
-
-	n = rte_snprintf(uio_newid, sizeof(uio_newid), UIO_DRV_PATH "/new_id", module_name);
-	if ((n < 0) || (n >= (int)sizeof(uio_newid))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf uio_newid name\n");
-		return -1;
-	}
-
-	n = rte_snprintf(uio_bind, sizeof(uio_bind), UIO_DRV_PATH, module_name);
-	if ((n < 0) || (n >= (int)sizeof(uio_bind))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf uio_bind name\n");
-		return -1;
-	}
-
-	n = rte_snprintf(buf, sizeof(buf), "%x %x\n",
-			dev->id.vendor_id, dev->id.device_id);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf vendor_id/device_id\n");
-		return -1;
-	}
-
-	f = fopen(uio_newid, "w");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot open %s\n", uio_newid);
-		return -1;
-	}
-	if (fwrite(buf, n, 1, f) == 0) {
-		fclose(f);
-		return -1;
-	}
-	fclose(f);
-
-	pci_bind_device(dev, uio_bind);
-	return 0;
-}
-
-/* unbind kernel driver for this device */
-static int
-pci_unbind_kernel_driver(struct rte_pci_device *dev)
-{
-	int n;
-	FILE *f;
-	char filename[PATH_MAX];
-	char buf[BUFSIZ];
-	struct rte_pci_addr *loc = &dev->addr;
-
-	/* open /sys/bus/pci/devices/AAAA:BB:CC.D/driver */
-	rte_snprintf(filename, sizeof(filename),
-	         SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/driver/unbind",
-	         loc->domain, loc->bus, loc->devid, loc->function);
-
-	f = fopen(filename, "w");
-	if (f == NULL) /* device was not bound */
-		return 0;
-
-	n = rte_snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
-	             loc->domain, loc->bus, loc->devid, loc->function);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "%s(): rte_snprintf failed\n", __func__);
-		goto error;
-	}
-	if (fwrite(buf, n, 1, f) == 0) {
-		RTE_LOG(ERR, EAL, "%s(): could not write to %s\n", __func__,
-				filename);
-		goto error;
-	}
-
-	fclose(f);
-	return 0;
-
-error:
-	fclose(f);
-	return -1;
-}
-
-
-static int
-pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
-		int uio_status, const char *module_name)
-{
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
-		/* check that our driver is loaded */
-		if (uio_status != 0 &&
-				(uio_status = pci_uio_check_module(module_name)) != 0)
-			rte_exit(EXIT_FAILURE, "The %s module is required by the "
-					"%s driver\n", module_name, dr->name);
-
-		/* unbind current driver, bind ours */
-		if (pci_unbind_kernel_driver(dev) < 0)
-			return -1;
-		if (pci_uio_bind_device(dev, module_name) < 0)
-			return -1;
-	}
-	/* map the NIC resources */
-	if (pci_uio_map_resource(dev) < 0)
-		return -1;
-
-	return 0;
-}
-
-#endif /* ifdef EAL_UNBIND_PORTS */
-
 /* map a particular resource from a file */
 static void *
 pci_map_resource(struct rte_pci_device *dev, void *requested_addr, 
@@ -310,25 +117,10 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	int fd;
 	void *mapaddr;
 
-#ifdef RTE_EAL_UNBIND_PORTS
-	/*
-	 * open devname, and mmap it: it can take some time to
-	 * appear, so we wait some time before returning an error
-	 */
-	unsigned n;
-	fd = dev->intr_handle.fd;
-	for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10 && fd < 0; n++) {
-		errno = 0;
-		if ((fd = open(devname, O_RDWR)) < 0 && errno != ENOENT)
-			break;
-		usleep(100000);
-	}
-#else
 	/*
 	 * open devname, to mmap it
 	 */
 	fd = open(devname, O_RDWR);
-#endif
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", 
 			devname, strerror(errno));
@@ -1036,23 +828,10 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 			return 0;
 		}
 
-#ifdef RTE_EAL_UNBIND_PORTS
-		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
-			/* unbind driver and load uio resources for Intel NICs */
-			if (pci_switch_module(dr, dev, 1, IGB_UIO_NAME) < 0)
-				return -1;
-		} else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND &&
-		           rte_eal_process_type() == RTE_PROC_PRIMARY) {
-			/* unbind current driver */
-			if (pci_unbind_kernel_driver(dev) < 0)
-				return -1;
-		}
-#else
 		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO)
 			/* just map resources for Intel NICs */
 			if (pci_uio_map_resource(dev) < 0)
 				return -1;
-#endif
 
 		/* reference driver structure */
 		dev->driver = dr;
-- 
1.8.1.4

[dpdk-dev] [PATCH 0/3] remove RTE_EAL_UNBIND_PORTS related code

[David Marchand]

Hello Anatoly, 

I think we might want to start a discussion on what should be done by the eal
and what should be done by the drivers themselves. But this can be done later.

For now, I have a problem with your patch as it breaks drivers that use
RTE_PCI_DRV_FORCE_UNBIND flag, since it removes the part where this flag is
handled. Besides, I can see some remaining parts mentioning pci_switch_module()
in comments.

So, here is a patchset that should do the trick.
It does some cleanup before removing the RTE_EAL_UNBIND_PORTS part.


Regards, 
-- 
David Marchand

David Marchand (3):
  pci: pci_switch_module cleanup
  pci: move RTE_PCI_DRV_FORCE_UNBIND handling out of #ifdef
  pci: remove deprecated RTE_EAL_UNBIND_PORTS option

 lib/librte_eal/linuxapp/eal/eal_pci.c |  183 +--------------------------------
 1 file changed, 2 insertions(+), 181 deletions(-)

-- 
1.7.10.4

[dpdk-dev] [PATCH 1/3] pci: pci_switch_module cleanup

[David Marchand]

The pci_switch_module() function should only do what its name tells: unbind pci
devices and rebind them on the specified kernel driver.
Hence, it can not call pci_uio_map_resource().

Call to pci_uio_map_resource() should be moved to rte_eal_pci_probe_one_driver()
so that we can factorize code.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |   21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 9538efe..8e0922d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -107,9 +107,6 @@ TAILQ_HEAD(uio_res_list, uio_resource);
 static struct uio_res_list *uio_res_list = NULL;
 static int pci_parse_sysfs_value(const char *filename, uint64_t *val);
 
-/* forward prototype of function called in pci_switch_module below */
-static int pci_uio_map_resource(struct rte_pci_device *dev);
-
 #ifdef RTE_EAL_UNBIND_PORTS
 #define PROC_MODULES "/proc/modules"
 
@@ -279,12 +276,11 @@ error:
 
 static int
 pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
-		int uio_status, const char *module_name)
+                  const char *module_name)
 {
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		/* check that our driver is loaded */
-		if (uio_status != 0 &&
-				(uio_status = pci_uio_check_module(module_name)) != 0)
+		if (pci_uio_check_module(module_name) != 0)
 			rte_exit(EXIT_FAILURE, "The %s module is required by the "
 					"%s driver\n", module_name, dr->name);
 
@@ -294,9 +290,6 @@ pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
 		if (pci_uio_bind_device(dev, module_name) < 0)
 			return -1;
 	}
-	/* map the NIC resources */
-	if (pci_uio_map_resource(dev) < 0)
-		return -1;
 
 	return 0;
 }
@@ -1040,8 +1033,8 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 
 #ifdef RTE_EAL_UNBIND_PORTS
 		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
-			/* unbind driver and load uio resources for Intel NICs */
-			if (pci_switch_module(dr, dev, 1, IGB_UIO_NAME) < 0)
+			/* unbind current driver and bind on igb_uio */
+			if (pci_switch_module(dr, dev, IGB_UIO_NAME) < 0)
 				return -1;
 		} else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND &&
 		           rte_eal_process_type() == RTE_PROC_PRIMARY) {
@@ -1049,12 +1042,12 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 			if (pci_unbind_kernel_driver(dev) < 0)
 				return -1;
 		}
-#else
+#endif
+
 		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO)
-			/* just map resources for Intel NICs */
+			/* map resources for devices that use igb_uio */
 			if (pci_uio_map_resource(dev) < 0)
 				return -1;
-#endif
 
 		/* reference driver structure */
 		dev->driver = dr;
-- 
1.7.10.4

[dpdk-dev] [PATCH 2/3] pci: move RTE_PCI_DRV_FORCE_UNBIND handling out of #ifdef

[David Marchand]

Move RTE_PCI_DRV_FORCE_UNBIND flag handling out of RTE_EAL_UNBIND_PORTS section.
This had nothing to do with RTE_EAL_UNBIND_PORTS anyway.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |   92 ++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 8e0922d..6b57a9f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -107,6 +107,45 @@ TAILQ_HEAD(uio_res_list, uio_resource);
 static struct uio_res_list *uio_res_list = NULL;
 static int pci_parse_sysfs_value(const char *filename, uint64_t *val);
 
+/* unbind kernel driver for this device */
+static int
+pci_unbind_kernel_driver(struct rte_pci_device *dev)
+{
+	int n;
+	FILE *f;
+	char filename[PATH_MAX];
+	char buf[BUFSIZ];
+	struct rte_pci_addr *loc = &dev->addr;
+
+	/* open /sys/bus/pci/devices/AAAA:BB:CC.D/driver */
+	rte_snprintf(filename, sizeof(filename),
+	         SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/driver/unbind",
+	         loc->domain, loc->bus, loc->devid, loc->function);
+
+	f = fopen(filename, "w");
+	if (f == NULL) /* device was not bound */
+		return 0;
+
+	n = rte_snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
+	             loc->domain, loc->bus, loc->devid, loc->function);
+	if ((n < 0) || (n >= (int)sizeof(buf))) {
+		RTE_LOG(ERR, EAL, "%s(): rte_snprintf failed\n", __func__);
+		goto error;
+	}
+	if (fwrite(buf, n, 1, f) == 0) {
+		RTE_LOG(ERR, EAL, "%s(): could not write to %s\n", __func__,
+				filename);
+		goto error;
+	}
+
+	fclose(f);
+	return 0;
+
+error:
+	fclose(f);
+	return -1;
+}
+
 #ifdef RTE_EAL_UNBIND_PORTS
 #define PROC_MODULES "/proc/modules"
 
@@ -234,46 +273,6 @@ pci_uio_bind_device(struct rte_pci_device *dev, const char *module_name)
 	return 0;
 }
 
-/* unbind kernel driver for this device */
-static int
-pci_unbind_kernel_driver(struct rte_pci_device *dev)
-{
-	int n;
-	FILE *f;
-	char filename[PATH_MAX];
-	char buf[BUFSIZ];
-	struct rte_pci_addr *loc = &dev->addr;
-
-	/* open /sys/bus/pci/devices/AAAA:BB:CC.D/driver */
-	rte_snprintf(filename, sizeof(filename),
-	         SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/driver/unbind",
-	         loc->domain, loc->bus, loc->devid, loc->function);
-
-	f = fopen(filename, "w");
-	if (f == NULL) /* device was not bound */
-		return 0;
-
-	n = rte_snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
-	             loc->domain, loc->bus, loc->devid, loc->function);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "%s(): rte_snprintf failed\n", __func__);
-		goto error;
-	}
-	if (fwrite(buf, n, 1, f) == 0) {
-		RTE_LOG(ERR, EAL, "%s(): could not write to %s\n", __func__,
-				filename);
-		goto error;
-	}
-
-	fclose(f);
-	return 0;
-
-error:
-	fclose(f);
-	return -1;
-}
-
-
 static int
 pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
                   const char *module_name)
@@ -1036,18 +1035,19 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 			/* unbind current driver and bind on igb_uio */
 			if (pci_switch_module(dr, dev, IGB_UIO_NAME) < 0)
 				return -1;
-		} else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND &&
-		           rte_eal_process_type() == RTE_PROC_PRIMARY) {
-			/* unbind current driver */
-			if (pci_unbind_kernel_driver(dev) < 0)
-				return -1;
 		}
 #endif
 
-		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO)
+		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
 			/* map resources for devices that use igb_uio */
 			if (pci_uio_map_resource(dev) < 0)
 				return -1;
+		} else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND &&
+		           rte_eal_process_type() == RTE_PROC_PRIMARY) {
+			/* unbind current driver */
+			if (pci_unbind_kernel_driver(dev) < 0)
+				return -1;
+		}
 
 		/* reference driver structure */
 		dev->driver = dr;
-- 
1.7.10.4

[dpdk-dev] [PATCH 3/3] pci: remove deprecated RTE_EAL_UNBIND_PORTS option

[David Marchand]

RTE_EAL_UNBIND_PORTS was deprecated in DPDK 1.4.0 and removed in 1.6.0, but the
code was not removed.

The bind/unbind operations should not be handled by the eal.
These operations should be either done outside of dpdk or inside the PMDs
themselves as these are their problems.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |  172 ---------------------------------
 1 file changed, 172 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 6b57a9f..f4f99ab 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -146,155 +146,6 @@ error:
 	return -1;
 }
 
-#ifdef RTE_EAL_UNBIND_PORTS
-#define PROC_MODULES "/proc/modules"
-
-#define IGB_UIO_NAME "igb_uio"
-
-#define UIO_DRV_PATH  "/sys/bus/pci/drivers/%s"
-
-/* maximum time to wait that /dev/uioX appears */
-#define UIO_DEV_WAIT_TIMEOUT 3 /* seconds */
-
-/*
- * Check that a kernel module is loaded. Returns 0 on success, or if the
- * parameter is NULL, or -1 if the module is not loaded.
- */
-static int
-pci_uio_check_module(const char *module_name)
-{
-	FILE *f;
-	unsigned i;
-	char buf[BUFSIZ];
-
-	if (module_name == NULL)
-		return 0;
-
-	f = fopen(PROC_MODULES, "r");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot open "PROC_MODULES": %s\n", 
-				strerror(errno));
-		return -1;
-	}
-
-	while(fgets(buf, sizeof(buf), f) != NULL) {
-
-		for (i = 0; i < sizeof(buf) && buf[i] != '\0'; i++) {
-			if (isspace(buf[i]))
-			    buf[i] = '\0';
-		}
-
-		if (strncmp(buf, module_name, sizeof(buf)) == 0) {
-			fclose(f);
-			return 0;
-		}
-	}
-	fclose(f);
-	return -1;
-}
-
-/* bind a PCI to the kernel module driver */
-static int
-pci_bind_device(struct rte_pci_device *dev, char dr_path[])
-{
-	FILE *f;
-	int n;
-	char buf[BUFSIZ];
-	char dev_bind[PATH_MAX];
-	struct rte_pci_addr *loc = &dev->addr;
-
-	n = rte_snprintf(dev_bind, sizeof(dev_bind), "%s/bind", dr_path);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf device bind path\n");
-		return -1;
-	}
-
-	f = fopen(dev_bind, "w");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot open %s\n", dev_bind);
-		return -1;
-	}
-	n = rte_snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
-	                 loc->domain, loc->bus, loc->devid, loc->function);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf PCI infos\n");
-		fclose(f);
-		return -1;
-	}
-	if (fwrite(buf, n, 1, f) == 0) {
-		fclose(f);
-		return -1;
-	}
-
-	fclose(f);
-	return 0;
-}
-
-static int
-pci_uio_bind_device(struct rte_pci_device *dev, const char *module_name)
-{
-	FILE *f;
-	int n;
-	char buf[BUFSIZ];
-	char uio_newid[PATH_MAX];
-	char uio_bind[PATH_MAX];
-
-	n = rte_snprintf(uio_newid, sizeof(uio_newid), UIO_DRV_PATH "/new_id", module_name);
-	if ((n < 0) || (n >= (int)sizeof(uio_newid))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf uio_newid name\n");
-		return -1;
-	}
-
-	n = rte_snprintf(uio_bind, sizeof(uio_bind), UIO_DRV_PATH, module_name);
-	if ((n < 0) || (n >= (int)sizeof(uio_bind))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf uio_bind name\n");
-		return -1;
-	}
-
-	n = rte_snprintf(buf, sizeof(buf), "%x %x\n",
-			dev->id.vendor_id, dev->id.device_id);
-	if ((n < 0) || (n >= (int)sizeof(buf))) {
-		RTE_LOG(ERR, EAL, "Cannot rte_snprintf vendor_id/device_id\n");
-		return -1;
-	}
-
-	f = fopen(uio_newid, "w");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot open %s\n", uio_newid);
-		return -1;
-	}
-	if (fwrite(buf, n, 1, f) == 0) {
-		fclose(f);
-		return -1;
-	}
-	fclose(f);
-
-	pci_bind_device(dev, uio_bind);
-	return 0;
-}
-
-static int
-pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
-                  const char *module_name)
-{
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
-		/* check that our driver is loaded */
-		if (pci_uio_check_module(module_name) != 0)
-			rte_exit(EXIT_FAILURE, "The %s module is required by the "
-					"%s driver\n", module_name, dr->name);
-
-		/* unbind current driver, bind ours */
-		if (pci_unbind_kernel_driver(dev) < 0)
-			return -1;
-		if (pci_uio_bind_device(dev, module_name) < 0)
-			return -1;
-	}
-
-	return 0;
-}
-
-#endif /* ifdef EAL_UNBIND_PORTS */
-
 /* map a particular resource from a file */
 static void *
 pci_map_resource(struct rte_pci_device *dev, void *requested_addr, 
@@ -303,25 +154,10 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	int fd;
 	void *mapaddr;
 
-#ifdef RTE_EAL_UNBIND_PORTS
-	/*
-	 * open devname, and mmap it: it can take some time to
-	 * appear, so we wait some time before returning an error
-	 */
-	unsigned n;
-	fd = dev->intr_handle.fd;
-	for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10 && fd < 0; n++) {
-		errno = 0;
-		if ((fd = open(devname, O_RDWR)) < 0 && errno != ENOENT)
-			break;
-		usleep(100000);
-	}
-#else
 	/*
 	 * open devname, to mmap it
 	 */
 	fd = open(devname, O_RDWR);
-#endif
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", 
 			devname, strerror(errno));
@@ -1030,14 +866,6 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 			return 0;
 		}
 
-#ifdef RTE_EAL_UNBIND_PORTS
-		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
-			/* unbind current driver and bind on igb_uio */
-			if (pci_switch_module(dr, dev, IGB_UIO_NAME) < 0)
-				return -1;
-		}
-#endif
-
 		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
 			/* map resources for devices that use igb_uio */
 			if (pci_uio_map_resource(dev) < 0)
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH 0/3] remove RTE_EAL_UNBIND_PORTS related code

[Burakov, Anatoly]

Hi David,

> For now, I have a problem with your patch as it breaks drivers that use
> RTE_PCI_DRV_FORCE_UNBIND flag, since it removes the part where this flag
> is handled. Besides, I can see some remaining parts mentioning
> pci_switch_module() in comments.

I wonder if this functionality is needed at all. I couldn't find any drivers using that flag, and the unbinding can be done using the included Python script as well. Given that we're getting rid of unbinding and binding in the EAL, it's only logical that we get rid of the force-unbinding as well, don't you think?

Best regards,
Anatoly Burakov
DPDK SW Engineer

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

Re: [dpdk-dev] [PATCH 0/3] remove RTE_EAL_UNBIND_PORTS related code

[David Marchand]

On 04/14/2014 03:12 PM, Burakov, Anatoly wrote:
>> For now, I have a problem with your patch as it breaks drivers that use
>> RTE_PCI_DRV_FORCE_UNBIND flag, since it removes the part where this flag
>> is handled. Besides, I can see some remaining parts mentioning
>> pci_switch_module() in comments.
>
> I wonder if this functionality is needed at all. I couldn't find any drivers using that flag, and the unbinding can be done using the included Python script as well. Given that we're getting rid of unbinding and binding in the EAL, it's only logical that we get rid of the force-unbinding as well, don't you think?

This functionality is at least used by virtio-net-pmd.
So we cannot remove this without a fix for virtio-net-pmd.
Besides, if we remove this functionality, then headers and documentation 
should be updated accordingly.

Yet, even if we remove this from the eal, I think we should provide a 
proper api to bind/unbind devices to kernel drivers.
This way PMDs can use this api and forget about OS dependencies (linux/bsd).


By the way, there is also a cleanup to be done inside bsd implementation 
as I found references to linux code inside it.

$ grep -rl pci_switch_module lib/
lib/librte_eal/bsdapp/eal/eal_pci.c

I am not sure BSD has a /sys filesystem.

$ grep -rl eal_parse_sysfs_value lib/librte_eal/bsdapp/
lib/librte_eal/bsdapp/eal/include/eal_filesystem.h
lib/librte_eal/bsdapp/eal/eal.c



As a conclusion, as far as all these cleanups are concerned, I would say 
we should target the next major release.



Regards,
-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/3] remove RTE_EAL_UNBIND_PORTS related code

[Burakov, Anatoly]

Hi David,

> This functionality is at least used by virtio-net-pmd.
> So we cannot remove this without a fix for virtio-net-pmd.

It appears that virtio-net-pmd hasn't been merged in yet? At least I can't see it in the git tree for 1.6 release.

> Yet, even if we remove this from the eal, I think we should provide a proper
> api to bind/unbind devices to kernel drivers.
> This way PMDs can use this api and forget about OS dependencies
> (linux/bsd).

Please forgive me for repeating myself, but I'm still not convinced that binding/unbinding the kernel drivers has to be the responsibility of the application in the first place. As far as I know, the reasons why the binding/unbinding code was there are purely historical and have no rationale behind them other than "this is how it was first implemented". Plus, correct me if I'm wrong, but FreeBSD has some issues with port binding/unbinding - i.e. it can't do that at all, at least during runtime, so on BSD it's even less of an issue.

Anyway, the reason I'm bringing this subject up in the first place is I'm preparing a patch to support VFIO driver alongside igb_uio. Among the changes I'm preparing to make is getting rid of the device ID list in igb_uio, so that any driver could be bound to it (via writing to new_id) - i.e. making it similar to how pci_stub, vfio-pci  et al work. A consequence of that will be that igb_uio won't try to bind any devices unless the user explicitly asks for it, meaning that the virtio device will not be bound to igb_uio by default. I think this will remedy the issues with virtio-net-pmd without the need for force-unbind logic in the EAL.

Best regards,
Anatoly Burakov
DPDK SW Engineer

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare


 

Re: [dpdk-dev] [PATCH 0/3] remove RTE_EAL_UNBIND_PORTS related code

[Thomas Monjalon]

Hi Anatoly,

2014-04-14 14:39, Burakov, Anatoly:
> > This functionality is at least used by virtio-net-pmd.
> > So we cannot remove this without a fix for virtio-net-pmd.
> 
> It appears that virtio-net-pmd hasn't been merged in yet? At least I can't
> see it in the git tree for 1.6 release.

Maybe you don't know the history of the project. virtio-net-pmd was the first 
PMD for virtio. It has been released out of tree. Then Intel designed another 
one based on uio and released it in the DPDK tree.
On this topic, you should know that some parts of virtio-uio are (wrongly) 
integrated in EAL. So it seems to be a bad idea to integrate PMDs in DPDK 
tree.

virtio-net-pmd is hosted outside of the git tree:
	http://dpdk.org/browse/virtio-net-pmd

> > Yet, even if we remove this from the eal, I think we should provide a
> > proper api to bind/unbind devices to kernel drivers.
> > This way PMDs can use this api and forget about OS dependencies
> > (linux/bsd).
> 
> Please forgive me for repeating myself, but I'm still not convinced that
> binding/unbinding the kernel drivers has to be the responsibility of the
> application in the first place. As far as I know, the reasons why the
> binding/unbinding code was there are purely historical and have no
> rationale behind them other than "this is how it was first implemented".
> Plus, correct me if I'm wrong, but FreeBSD has some issues with port
> binding/unbinding - i.e. it can't do that at all, at least during runtime,
> so on BSD it's even less of an issue.

I understand your point of view.
I think everybody agree that EAL should not force binding/unbinding 
operations. But it could be very convenient to allow PMDs to do it by 
themselves.

Do you agree that the David's version is a good step in the right direction as 
it allows to remove RTE_EAL_UNBIND_PORTS?
If so, could you ack these patches, please?
Then we could discuss what are the next steps.

> Anyway, the reason I'm bringing this subject up in the first place is I'm
> preparing a patch to support VFIO driver alongside igb_uio. Among the
> changes I'm preparing to make is getting rid of the device ID list in
> igb_uio, so that any driver could be bound to it (via writing to new_id) -
> i.e. making it similar to how pci_stub, vfio-pci  et al work. A consequence
> of that will be that igb_uio won't try to bind any devices unless the user
> explicitly asks for it, meaning that the virtio device will not be bound to
> igb_uio by default. I think this will remedy the issues with virtio-net-pmd
> without the need for force-unbind logic in the EAL.

virtio-net-pmd doesn't need any kernel driver. Moreover, the virtio-net kernel 
driver must be unbound. It can be done before running DPDK but it's simpler to 
do it in the PMD initialization.
By the way, we should remove this flag FORCE_UNBIND. But please, let's do it 
in another step.

-- 
Thomas

Re: [dpdk-dev] [PATCH 0/3] remove RTE_EAL_UNBIND_PORTS related code

[Burakov, Anatoly]

Hi Thomas, David,

Thanks for your detailed explanations and insights, I now have a better picture. I still have to do a lot of catch up on dpdk.org tree and ecosystem, so please forgive my ignorance about some dpdk.org-related subjects at times :-)

> Do you agree that the David's version is a good step in the right direction as it
> allows to remove RTE_EAL_UNBIND_PORTS?
> If so, could you ack these patches, please?
> Then we could discuss what are the next steps.

Yes. I'll ack and rebase my patch once David's patch makes it into the tree then.

Best regards,
Anatoly Burakov
DPDK SW Engineer

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

Re: [dpdk-dev] [PATCH 1/3] pci: pci_switch_module cleanup

[Burakov, Anatoly]

> The pci_switch_module() function should only do what its name tells: unbind
> pci devices and rebind them on the specified kernel driver.
> Hence, it can not call pci_uio_map_resource().
> 
> Call to pci_uio_map_resource() should be moved to
> rte_eal_pci_probe_one_driver() so that we can factorize code.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Best regards,
Anatoly Burakov
DPDK SW Engineer

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

Re: [dpdk-dev] [PATCH 2/3] pci: move RTE_PCI_DRV_FORCE_UNBIND handling out of #ifdef

[Burakov, Anatoly]

> Move RTE_PCI_DRV_FORCE_UNBIND flag handling out of
> RTE_EAL_UNBIND_PORTS section.
> This had nothing to do with RTE_EAL_UNBIND_PORTS anyway.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Best regards,
Anatoly Burakov
DPDK SW Engineer

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

Re: [dpdk-dev] [PATCH 3/3] pci: remove deprecated RTE_EAL_UNBIND_PORTS option

[Burakov, Anatoly]

> RTE_EAL_UNBIND_PORTS was deprecated in DPDK 1.4.0 and removed in
> 1.6.0, but the code was not removed.
> 
> The bind/unbind operations should not be handled by the eal.
> These operations should be either done outside of dpdk or inside the PMDs
> themselves as these are their problems.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: David Marchand <david.marchand@6wind.com>

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Best regards,
Anatoly Burakov
DPDK SW Engineer

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

Re: [dpdk-dev] [PATCH 3/3] pci: remove deprecated RTE_EAL_UNBIND_PORTS option

[David Marchand]

Hello all,

I have found some problems with these patches.
So NAK.

A fd and mem leaks have been revealed and other places needed some cleanup,
so I will send new patches that supersede these 3.


-- 
David Marchand


On Mon, Apr 14, 2014 at 2:51 PM, David Marchand <david.marchand@6wind.com>wrote:

> RTE_EAL_UNBIND_PORTS was deprecated in DPDK 1.4.0 and removed in 1.6.0,
> but the
> code was not removed.
>
> The bind/unbind operations should not be handled by the eal.
> These operations should be either done outside of dpdk or inside the PMDs
> themselves as these are their problems.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c |  172
> ---------------------------------
>  1 file changed, 172 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 6b57a9f..f4f99ab 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -146,155 +146,6 @@ error:
>         return -1;
>  }
>
> -#ifdef RTE_EAL_UNBIND_PORTS
> -#define PROC_MODULES "/proc/modules"
> -
> -#define IGB_UIO_NAME "igb_uio"
> -
> -#define UIO_DRV_PATH  "/sys/bus/pci/drivers/%s"
> -
> -/* maximum time to wait that /dev/uioX appears */
> -#define UIO_DEV_WAIT_TIMEOUT 3 /* seconds */
> -
> -/*
> - * Check that a kernel module is loaded. Returns 0 on success, or if the
> - * parameter is NULL, or -1 if the module is not loaded.
> - */
> -static int
> -pci_uio_check_module(const char *module_name)
> -{
> -       FILE *f;
> -       unsigned i;
> -       char buf[BUFSIZ];
> -
> -       if (module_name == NULL)
> -               return 0;
> -
> -       f = fopen(PROC_MODULES, "r");
> -       if (f == NULL) {
> -               RTE_LOG(ERR, EAL, "Cannot open "PROC_MODULES": %s\n",
> -                               strerror(errno));
> -               return -1;
> -       }
> -
> -       while(fgets(buf, sizeof(buf), f) != NULL) {
> -
> -               for (i = 0; i < sizeof(buf) && buf[i] != '\0'; i++) {
> -                       if (isspace(buf[i]))
> -                           buf[i] = '\0';
> -               }
> -
> -               if (strncmp(buf, module_name, sizeof(buf)) == 0) {
> -                       fclose(f);
> -                       return 0;
> -               }
> -       }
> -       fclose(f);
> -       return -1;
> -}
> -
> -/* bind a PCI to the kernel module driver */
> -static int
> -pci_bind_device(struct rte_pci_device *dev, char dr_path[])
> -{
> -       FILE *f;
> -       int n;
> -       char buf[BUFSIZ];
> -       char dev_bind[PATH_MAX];
> -       struct rte_pci_addr *loc = &dev->addr;
> -
> -       n = rte_snprintf(dev_bind, sizeof(dev_bind), "%s/bind", dr_path);
> -       if ((n < 0) || (n >= (int)sizeof(buf))) {
> -               RTE_LOG(ERR, EAL, "Cannot rte_snprintf device bind
> path\n");
> -               return -1;
> -       }
> -
> -       f = fopen(dev_bind, "w");
> -       if (f == NULL) {
> -               RTE_LOG(ERR, EAL, "Cannot open %s\n", dev_bind);
> -               return -1;
> -       }
> -       n = rte_snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
> -                        loc->domain, loc->bus, loc->devid, loc->function);
> -       if ((n < 0) || (n >= (int)sizeof(buf))) {
> -               RTE_LOG(ERR, EAL, "Cannot rte_snprintf PCI infos\n");
> -               fclose(f);
> -               return -1;
> -       }
> -       if (fwrite(buf, n, 1, f) == 0) {
> -               fclose(f);
> -               return -1;
> -       }
> -
> -       fclose(f);
> -       return 0;
> -}
> -
> -static int
> -pci_uio_bind_device(struct rte_pci_device *dev, const char *module_name)
> -{
> -       FILE *f;
> -       int n;
> -       char buf[BUFSIZ];
> -       char uio_newid[PATH_MAX];
> -       char uio_bind[PATH_MAX];
> -
> -       n = rte_snprintf(uio_newid, sizeof(uio_newid), UIO_DRV_PATH
> "/new_id", module_name);
> -       if ((n < 0) || (n >= (int)sizeof(uio_newid))) {
> -               RTE_LOG(ERR, EAL, "Cannot rte_snprintf uio_newid name\n");
> -               return -1;
> -       }
> -
> -       n = rte_snprintf(uio_bind, sizeof(uio_bind), UIO_DRV_PATH,
> module_name);
> -       if ((n < 0) || (n >= (int)sizeof(uio_bind))) {
> -               RTE_LOG(ERR, EAL, "Cannot rte_snprintf uio_bind name\n");
> -               return -1;
> -       }
> -
> -       n = rte_snprintf(buf, sizeof(buf), "%x %x\n",
> -                       dev->id.vendor_id, dev->id.device_id);
> -       if ((n < 0) || (n >= (int)sizeof(buf))) {
> -               RTE_LOG(ERR, EAL, "Cannot rte_snprintf
> vendor_id/device_id\n");
> -               return -1;
> -       }
> -
> -       f = fopen(uio_newid, "w");
> -       if (f == NULL) {
> -               RTE_LOG(ERR, EAL, "Cannot open %s\n", uio_newid);
> -               return -1;
> -       }
> -       if (fwrite(buf, n, 1, f) == 0) {
> -               fclose(f);
> -               return -1;
> -       }
> -       fclose(f);
> -
> -       pci_bind_device(dev, uio_bind);
> -       return 0;
> -}
> -
> -static int
> -pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
> -                  const char *module_name)
> -{
> -       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> -               /* check that our driver is loaded */
> -               if (pci_uio_check_module(module_name) != 0)
> -                       rte_exit(EXIT_FAILURE, "The %s module is required
> by the "
> -                                       "%s driver\n", module_name,
> dr->name);
> -
> -               /* unbind current driver, bind ours */
> -               if (pci_unbind_kernel_driver(dev) < 0)
> -                       return -1;
> -               if (pci_uio_bind_device(dev, module_name) < 0)
> -                       return -1;
> -       }
> -
> -       return 0;
> -}
> -
> -#endif /* ifdef EAL_UNBIND_PORTS */
> -
>  /* map a particular resource from a file */
>  static void *
>  pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
> @@ -303,25 +154,10 @@ pci_map_resource(struct rte_pci_device *dev, void
> *requested_addr,
>         int fd;
>         void *mapaddr;
>
> -#ifdef RTE_EAL_UNBIND_PORTS
> -       /*
> -        * open devname, and mmap it: it can take some time to
> -        * appear, so we wait some time before returning an error
> -        */
> -       unsigned n;
> -       fd = dev->intr_handle.fd;
> -       for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10 && fd < 0; n++) {
> -               errno = 0;
> -               if ((fd = open(devname, O_RDWR)) < 0 && errno != ENOENT)
> -                       break;
> -               usleep(100000);
> -       }
> -#else
>         /*
>          * open devname, to mmap it
>          */
>         fd = open(devname, O_RDWR);
> -#endif
>         if (fd < 0) {
>                 RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>                         devname, strerror(errno));
> @@ -1030,14 +866,6 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver
> *dr, struct rte_pci_device *d
>                         return 0;
>                 }
>
> -#ifdef RTE_EAL_UNBIND_PORTS
> -               if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
> -                       /* unbind current driver and bind on igb_uio */
> -                       if (pci_switch_module(dr, dev, IGB_UIO_NAME) < 0)
> -                               return -1;
> -               }
> -#endif
> -
>                 if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO) {
>                         /* map resources for devices that use igb_uio */
>                         if (pci_uio_map_resource(dev) < 0)
> --
> 1.7.10.4
>
>