[PATCH 0/7] pci cleanup

[PATCH 0/7] pci cleanup

[David Marchand]

Hello all, 

Here is an attempt at having an equal implementation in bsd and linux eal_pci.c.
It results in following changes :
- checks on driver flag in bsd which were missing
- remove virtio-uio workaround in linux eal_pci.c
- remove deprecated RTE_EAL_UNBIND_PORTS option

Along the way, I discovered two small bugs: a mem leak in linux eal_pci.c and a
fd leak in both bsd and linux eal_pci.c.


-- 
David Marchand

David Marchand (7):
  pci: fix potential mem leak
  pci: align bsd implementation on linux
  pci: remove virtio-uio workaround
  pci: rework interrupt fd init and fix fd leak
  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/bsdapp/eal/eal_pci.c   |  105 ++++++------
 lib/librte_eal/linuxapp/eal/eal_pci.c |  282 +++++----------------------------
 lib/librte_pmd_virtio/virtio_ethdev.c |  133 +++++++++++++++-
 3 files changed, 218 insertions(+), 302 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] pci: fix potential mem leak

[David Marchand]

Looking at bsd implementation, we can see that there is a potential mem leak in
linux implementation. Fix this.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 9538efe..313bab7 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -678,6 +678,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 					(mapaddr = pci_map_resource(dev,
 					NULL, devname, (off_t)offset,
 					(size_t)maps[j].size)) == NULL) {
+				rte_free(uio_res);
 				return (-1);
 			}
  
-- 
1.7.10.4

[PATCH 2/7] pci: align bsd implementation on linux

[David Marchand]

bsd implementation lacks check on driver flags, fix this.
Besides, check on BAR0 is not needed and could cause trouble for devices that
have no BAR0.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c |   42 +++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index 7b04fa6..b25c0a1 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -107,8 +107,14 @@ TAILQ_HEAD(uio_res_list, uio_resource);
 
 static struct uio_res_list *uio_res_list = NULL;
 
-/* forward prototype of function called in pci_switch_module below */
-static int pci_uio_map_resource(struct rte_pci_device *dev);
+/* unbind kernel driver for this device */
+static int
+pci_unbind_kernel_driver(struct rte_pci_device *dev)
+{
+	RTE_LOG(ERR, EAL, "RTE_PCI_DRV_FORCE_UNBIND flag is not implemented "
+		"for BSD\n");
+	return -ENOTSUP;
+}
 
 /* map a particular resource from a file */
 static void *
@@ -213,6 +219,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 
 	dev->intr_handle.fd = -1;
 
+	/* secondary processes - use already recorded details */
+	if ((rte_eal_process_type() != RTE_PROC_PRIMARY) &&
+		(dev->id.vendor_id != PCI_VENDOR_ID_QUMRANET))
+		return (pci_uio_map_secondary(dev));
+
 	rte_snprintf(devname, sizeof(devname), "/dev/uio@pci:%u:%u:%u",
 			dev->addr.bus, dev->addr.devid, dev->addr.function);
 
@@ -222,11 +233,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 		return -1;
 	}
 
-	/* secondary processes - use already recorded details */
-	if ((rte_eal_process_type() != RTE_PROC_PRIMARY) &&
-		(dev->id.vendor_id != PCI_VENDOR_ID_QUMRANET))
-		return (pci_uio_map_secondary(dev));
-
 	if(dev->id.vendor_id == PCI_VENDOR_ID_QUMRANET) {
 		/* I/O port address already assigned */
 		/* rte_virtio_pmd does not need any other bar even if available */
@@ -476,19 +482,17 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 			return 0;
 		}
 
-		/* just map the NIC resources */
-		if (pci_uio_map_resource(dev) < 0)
-			return -1;
-
-		/* We always should have BAR0 mapped */
-		if (rte_eal_process_type() == RTE_PROC_PRIMARY && 
-			dev->mem_resource[0].addr == NULL) {
-			RTE_LOG(ERR, EAL,
-				"%s(): BAR0 is not mapped\n",
-				__func__);
-			return (-1);
+		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

[PATCH 3/7] pci: remove virtio-uio workaround

[David Marchand]

virtio-uio does not need eal to map bars from uio device, so remove flag
RTE_PCI_DRV_NEED_IGB_UIO.
Then, move virtio-uio workaround out of generic eal_pci.c for linux
implementation.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c   |    9 +--
 lib/librte_eal/linuxapp/eal/eal_pci.c |   30 +-------
 lib/librte_pmd_virtio/virtio_ethdev.c |  133 ++++++++++++++++++++++++++++++++-
 3 files changed, 134 insertions(+), 38 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index b25c0a1..fbb281f 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -220,8 +220,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	dev->intr_handle.fd = -1;
 
 	/* secondary processes - use already recorded details */
-	if ((rte_eal_process_type() != RTE_PROC_PRIMARY) &&
-		(dev->id.vendor_id != PCI_VENDOR_ID_QUMRANET))
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return (pci_uio_map_secondary(dev));
 
 	rte_snprintf(devname, sizeof(devname), "/dev/uio@pci:%u:%u:%u",
@@ -233,12 +232,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 		return -1;
 	}
 
-	if(dev->id.vendor_id == PCI_VENDOR_ID_QUMRANET) {
-		/* I/O port address already assigned */
-		/* rte_virtio_pmd does not need any other bar even if available */
-		return (0);
-	}
-	
 	/* allocate the mapping details for secondary processes*/
 	if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) {
 		RTE_LOG(ERR, EAL,
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 313bab7..64c2130 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -584,11 +584,9 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 {
 	int i, j;
 	char dirname[PATH_MAX];
-	char filename[PATH_MAX];
 	char devname[PATH_MAX]; /* contains the /dev/uioX */
 	void *mapaddr;
 	int uio_num;
-	unsigned long start,size;
 	uint64_t phaddr;
 	uint64_t offset;
 	uint64_t pagesz;
@@ -600,8 +598,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	dev->intr_handle.fd = -1;
 
 	/* secondary processes - use already recorded details */
-	if ((rte_eal_process_type() != RTE_PROC_PRIMARY) &&
-	    (dev->id.vendor_id != PCI_VENDOR_ID_QUMRANET))
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return (pci_uio_map_secondary(dev));
 
 	/* find uio resource */
@@ -612,31 +609,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 		return -1;
 	}
 
-	if(dev->id.vendor_id == PCI_VENDOR_ID_QUMRANET) {
-		/* get portio size */
-		rte_snprintf(filename, sizeof(filename),
-			 "%s/portio/port0/size", dirname);
-		if (eal_parse_sysfs_value(filename, &size) < 0) {
-			RTE_LOG(ERR, EAL, "%s(): cannot parse size\n",
-				__func__);
-			return -1;
-		}
-
-		/* get portio start */
-		rte_snprintf(filename, sizeof(filename),
-			 "%s/portio/port0/start", dirname);
-		if (eal_parse_sysfs_value(filename, &start) < 0) {
-			RTE_LOG(ERR, EAL, "%s(): cannot parse portio start\n",
-				__func__);
-			return -1;
-		}
-		dev->mem_resource[0].addr = (void *)(uintptr_t)start;
-		dev->mem_resource[0].len =  (uint64_t)size;
-		RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%lx with size=0x%lx\n", start, size);
-		/* rte_virtio_pmd does not need any other bar even if available */
-		return (0);
-	}
-	
 	/* allocate the mapping details for secondary processes*/
 	if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) {
 		RTE_LOG(ERR, EAL,
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index f107161..c6a1df5 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -36,6 +36,9 @@
 #include <stdio.h>
 #include <errno.h>
 #include <unistd.h>
+#ifdef RTE_EXEC_ENV_LINUXAPP
+#include <dirent.h>
+#endif
 
 #include <rte_ethdev.h>
 #include <rte_memcpy.h>
@@ -392,6 +395,103 @@ virtio_negotiate_features(struct virtio_hw *hw)
 	hw->guest_features = vtpci_negotiate_features(hw, guest_features);
 }
 
+#ifdef RTE_EXEC_ENV_LINUXAPP
+static int
+parse_sysfs_value(const char *filename, unsigned long *val)
+{
+	FILE *f;
+	char buf[BUFSIZ];
+	char *end = NULL;
+
+	if ((f = fopen(filename, "r")) == NULL) {
+		PMD_INIT_LOG(ERR, "%s(): cannot open sysfs value %s\n",
+			     __func__, filename);
+		return -1;
+	}
+
+	if (fgets(buf, sizeof(buf), f) == NULL) {
+		PMD_INIT_LOG(ERR, "%s(): cannot read sysfs value %s\n",
+			     __func__, filename);
+		fclose(f);
+		return -1;
+	}
+	*val = strtoul(buf, &end, 0);
+	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
+		PMD_INIT_LOG(ERR, "%s(): cannot parse sysfs value %s\n",
+			     __func__, filename);
+		fclose(f);
+		return -1;
+	}
+	fclose(f);
+	return 0;
+}
+
+static int get_uio_dev(struct rte_pci_addr *loc, char *buf, unsigned int buflen)
+{
+	unsigned int uio_num;
+	struct dirent *e;
+	DIR *dir;
+	char dirname[PATH_MAX];
+
+	/* depending on kernel version, uio can be located in uio/uioX
+	 * or uio:uioX */
+	rte_snprintf(dirname, sizeof(dirname),
+	         SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/uio",
+	         loc->domain, loc->bus, loc->devid, loc->function);
+	dir = opendir(dirname);
+	if (dir == NULL) {
+		/* retry with the parent directory */
+		rte_snprintf(dirname, sizeof(dirname),
+		         SYSFS_PCI_DEVICES "/" PCI_PRI_FMT,
+		         loc->domain, loc->bus, loc->devid, loc->function);
+		dir = opendir(dirname);
+
+		if (dir == NULL) {
+			PMD_INIT_LOG(ERR, "Cannot opendir %s\n", dirname);
+			return -1;
+		}
+	}
+
+	/* take the first file starting with "uio" */
+	while ((e = readdir(dir)) != NULL) {
+		/* format could be uio%d ...*/
+		int shortprefix_len = sizeof("uio") - 1;
+		/* ... or uio:uio%d */
+		int longprefix_len = sizeof("uio:uio") - 1;
+		char *endptr;
+
+		if (strncmp(e->d_name, "uio", 3) != 0)
+			continue;
+
+		/* first try uio%d */
+		errno = 0;
+		uio_num = strtoull(e->d_name + shortprefix_len, &endptr, 10);
+		if (errno == 0 && endptr != (e->d_name + shortprefix_len)) {
+			rte_snprintf(buf, buflen, "%s/uio%u", dirname, uio_num);
+			break;
+		}
+
+		/* then try uio:uio%d */
+		errno = 0;
+		uio_num = strtoull(e->d_name + longprefix_len, &endptr, 10);
+		if (errno == 0 && endptr != (e->d_name + longprefix_len)) {
+			rte_snprintf(buf, buflen, "%s/uio:uio%u", dirname,
+				     uio_num);
+			break;
+		}
+	}
+	closedir(dir);
+
+	/* No uio resource found */
+	if (e == NULL) {
+		PMD_INIT_LOG(ERR, "Could not find uio resource\n");
+		return -1;
+	}
+
+	return 0;
+}
+#endif
+
 /*
  * This function is based on probe() function in virtio_pci.c
  * It returns 0 on success.
@@ -426,6 +526,38 @@ eth_virtio_dev_init(__rte_unused struct eth_driver *eth_drv,
 
 	hw->device_id = pci_dev->id.device_id;
 	hw->vendor_id = pci_dev->id.vendor_id;
+#ifdef RTE_EXEC_ENV_LINUXAPP
+	{
+		char dirname[PATH_MAX];
+		char filename[PATH_MAX];
+		unsigned long start,size;
+
+		if (get_uio_dev(&pci_dev->addr, dirname, sizeof(dirname)) < 0)
+			return -1;
+
+		/* get portio size */
+		rte_snprintf(filename, sizeof(filename),
+			     "%s/portio/port0/size", dirname);
+		if (parse_sysfs_value(filename, &size) < 0) {
+			PMD_INIT_LOG(ERR, "%s(): cannot parse size\n",
+				     __func__);
+			return -1;
+		}
+
+		/* get portio start */
+		rte_snprintf(filename, sizeof(filename),
+			     "%s/portio/port0/start", dirname);
+		if (parse_sysfs_value(filename, &start) < 0) {
+			PMD_INIT_LOG(ERR, "%s(): cannot parse portio start\n",
+				     __func__);
+			return -1;
+		}
+		pci_dev->mem_resource[0].addr = (void *)(uintptr_t)start;
+		pci_dev->mem_resource[0].len =  (uint64_t)size;
+		PMD_INIT_LOG(DEBUG, "PCI Port IO found start=0x%lx with "
+			     "size=0x%lx\n", start, size);
+	}
+#endif
 	hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr;
 
 	hw->max_rx_queues = VIRTIO_MAX_RX_QUEUES;
@@ -474,7 +606,6 @@ static struct eth_driver rte_virtio_pmd = {
 	{
 		.name = "rte_virtio_pmd",
 		.id_table = pci_id_virtio_map,
-		.drv_flags = RTE_PCI_DRV_NEED_IGB_UIO,
 	},
 	.eth_dev_init = eth_virtio_dev_init,
 	.dev_private_size = sizeof(struct virtio_adapter),
-- 
1.7.10.4

[PATCH 4/7] pci: rework interrupt fd init and fix fd leak

[David Marchand]

A fd leak happens in pci_map_resource when multiple bars are mapped.
Fix this by closing fd unconditionnally in this function and open the
intr_handle fd in pci_uio_map_resource instead.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c   |   60 +++++++++++++--------------
 lib/librte_eal/linuxapp/eal/eal_pci.c |   73 ++++++++++++++++-----------------
 2 files changed, 63 insertions(+), 70 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index fbb281f..5e53e07 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -118,8 +118,8 @@ pci_unbind_kernel_driver(struct rte_pci_device *dev)
 
 /* map a particular resource from a file */
 static void *
-pci_map_resource(struct rte_pci_device *dev, void *requested_addr, 
-		const char *devname, off_t offset, size_t size)
+pci_map_resource(void *requested_addr, const char *devname, off_t offset,
+		 size_t size)
 {
 	int fd;
 	void *mapaddr;
@@ -129,7 +129,7 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	 */
 	fd = open(devname, O_RDWR);
 	if (fd < 0) {
-		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", 
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 			devname, strerror(errno));
 		goto fail;
 	}
@@ -137,35 +137,21 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	/* Map the PCI memory resource of device */
 	mapaddr = mmap(requested_addr, size, PROT_READ | PROT_WRITE,
 			MAP_SHARED, fd, offset);
+	close(fd);
 	if (mapaddr == MAP_FAILED ||
 			(requested_addr != NULL && mapaddr != requested_addr)) {
 		RTE_LOG(ERR, EAL, "%s(): cannot mmap(%s(%d), %p, 0x%lx, 0x%lx):"
-			" %s (%p)\n", __func__, devname, fd, requested_addr, 
+			" %s (%p)\n", __func__, devname, fd, requested_addr,
 			(unsigned long)size, (unsigned long)offset,
 			strerror(errno), mapaddr);
-		close(fd);
 		goto fail;
 	}
 
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
-		/* save fd if in primary process */
-		dev->intr_handle.fd = fd;
-		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
-	} else {
-		/* fd is not needed in slave process, close it */
-		dev->intr_handle.fd = -1;
-		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
-		close(fd);
-	}
-
 	RTE_LOG(DEBUG, EAL, "  PCI memory mapped at %p\n", mapaddr);
 
 	return mapaddr;
 
 fail:
-	dev->intr_handle.fd = -1;
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
-
 	return NULL;
 }
 
@@ -178,19 +164,19 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 {
         size_t i;
         struct uio_resource *uio_res;
- 
+
 	TAILQ_FOREACH(uio_res, uio_res_list, next) {
- 
+
 		/* skip this element if it doesn't match our PCI address */
 		if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
 			continue;
-                
+
 		for (i = 0; i != uio_res->nb_maps; i++) {
-			if (pci_map_resource(dev, uio_res->maps[i].addr,
-					uio_res->path,
-					(off_t)uio_res->maps[i].offset,
-					(size_t)uio_res->maps[i].size) != 
-					uio_res->maps[i].addr) {
+			if (pci_map_resource(uio_res->maps[i].addr,
+					     uio_res->path,
+					     (off_t)uio_res->maps[i].offset,
+					     (size_t)uio_res->maps[i].size)
+			    != uio_res->maps[i].addr) {
 				RTE_LOG(ERR, EAL,
 					"Cannot mmap device resource\n");
 				return (-1);
@@ -218,6 +204,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	struct uio_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* secondary processes - use already recorded details */
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
@@ -232,6 +219,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 		return -1;
 	}
 
+	/* save fd if in primary process */
+	dev->intr_handle.fd = open(devname, O_RDWR);
+	if (dev->intr_handle.fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+			devname, strerror(errno));
+		return -1;
+	}
+	dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
+
 	/* allocate the mapping details for secondary processes*/
 	if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) {
 		RTE_LOG(ERR, EAL,
@@ -245,7 +241,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 
 	/* Map all BARs */
 	pagesz = sysconf(_SC_PAGESIZE);
- 
+
 	maps = uio_res->maps;
 	for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
 
@@ -253,16 +249,16 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 		/* skip empty BAR */
 		if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
 			continue;
- 
+
 		/* if matching map is found, then use it */
 		offset = i * pagesz;
 		maps[j].offset = offset;
 		maps[j].phaddr = dev->mem_resource[i].phys_addr;
 		maps[j].size = dev->mem_resource[i].len;
 		if (maps[j].addr != NULL ||
-				(mapaddr = pci_map_resource(dev,
-				NULL, devname, (off_t)offset,
-				(size_t)maps[j].size)) == NULL) {
+		    (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
+						(size_t)maps[j].size)
+		    ) == NULL) {
 			rte_free(uio_res);
 			return (-1);
 		}
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 64c2130..2c3b914 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -305,8 +305,8 @@ pci_switch_module(struct rte_pci_driver *dr, struct rte_pci_device *dev,
 
 /* map a particular resource from a file */
 static void *
-pci_map_resource(struct rte_pci_device *dev, void *requested_addr, 
-		const char *devname, off_t offset, size_t size)
+pci_map_resource(void *requested_addr, const char *devname, off_t offset,
+		 size_t size)
 {
 	int fd;
 	void *mapaddr;
@@ -317,8 +317,7 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	 * 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++) {
+	for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10; n++) {
 		errno = 0;
 		if ((fd = open(devname, O_RDWR)) < 0 && errno != ENOENT)
 			break;
@@ -331,7 +330,7 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	fd = open(devname, O_RDWR);
 #endif
 	if (fd < 0) {
-		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", 
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 			devname, strerror(errno));
 		goto fail;
 	}
@@ -339,34 +338,21 @@ pci_map_resource(struct rte_pci_device *dev, void *requested_addr,
 	/* Map the PCI memory resource of device */
 	mapaddr = mmap(requested_addr, size, PROT_READ | PROT_WRITE,
 			MAP_SHARED, fd, offset);
+	close(fd);
 	if (mapaddr == MAP_FAILED ||
 			(requested_addr != NULL && mapaddr != requested_addr)) {
 		RTE_LOG(ERR, EAL, "%s(): cannot mmap(%s(%d), %p, 0x%lx, 0x%lx):"
-			" %s (%p)\n", __func__, devname, fd, requested_addr, 
+			" %s (%p)\n", __func__, devname, fd, requested_addr,
 			(unsigned long)size, (unsigned long)offset,
 			strerror(errno), mapaddr);
-		close(fd);
 		goto fail;
 	}
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
-		/* save fd if in primary process */
-		dev->intr_handle.fd = fd;
-		dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
-	} else {
-		/* fd is not needed in slave process, close it */
-		dev->intr_handle.fd = -1;
-		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
-		close(fd);
-	}
 
 	RTE_LOG(DEBUG, EAL, "  PCI memory mapped at %p\n", mapaddr);
 
 	return mapaddr;
 
 fail:
-	dev->intr_handle.fd = -1;
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
-
 	return NULL;
 }
 
@@ -436,19 +422,19 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 {
         size_t i;
         struct uio_resource *uio_res;
- 
+
 	TAILQ_FOREACH(uio_res, uio_res_list, next) {
- 
+
 		/* skip this element if it doesn't match our PCI address */
 		if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
 			continue;
-                
+
 		for (i = 0; i != uio_res->nb_maps; i++) {
-			if (pci_map_resource(dev, uio_res->maps[i].addr,
-					uio_res->path,
-					(off_t)uio_res->maps[i].offset,
-					(size_t)uio_res->maps[i].size) != 
-					uio_res->maps[i].addr) {
+			if (pci_map_resource(uio_res->maps[i].addr,
+					     uio_res->path,
+					     (off_t)uio_res->maps[i].offset,
+					     (size_t)uio_res->maps[i].size)
+			    != uio_res->maps[i].addr) {
 				RTE_LOG(ERR, EAL,
 					"Cannot mmap device resource\n");
 				return (-1);
@@ -596,6 +582,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 	struct uio_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* secondary processes - use already recorded details */
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
@@ -608,6 +595,16 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 				"skipping\n", loc->domain, loc->bus, loc->devid, loc->function);
 		return -1;
 	}
+	rte_snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
+
+	/* save fd if in primary process */
+	dev->intr_handle.fd = open(devname, O_RDWR);
+	if (dev->intr_handle.fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+			devname, strerror(errno));
+		return -1;
+	}
+	dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 
 	/* allocate the mapping details for secondary processes*/
 	if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) {
@@ -616,7 +613,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 		return (-1);
 	}
 
-	rte_snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
 	rte_snprintf(uio_res->path, sizeof(uio_res->path), "%s", devname);
 	memcpy(&uio_res->pci_addr, &dev->addr, sizeof(uio_res->pci_addr));
 
@@ -625,35 +621,36 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 			sizeof (uio_res->maps) / sizeof (uio_res->maps[0])))
 			< 0)
 		return (nb_maps);
- 
+
 	uio_res->nb_maps = nb_maps;
 
 	/* Map all BARs */
 	pagesz = sysconf(_SC_PAGESIZE);
- 
+
 	maps = uio_res->maps;
 	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
-    
+
 		/* skip empty BAR */
 		if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
 			continue;
- 
+
 		for (j = 0; j != nb_maps && (phaddr != maps[j].phaddr ||
 				dev->mem_resource[i].len != maps[j].size);
 				j++)
 			;
- 
+
 		/* if matching map is found, then use it */
 		if (j != nb_maps) {
 			offset = j * pagesz;
 			if (maps[j].addr != NULL ||
-					(mapaddr = pci_map_resource(dev,
-					NULL, devname, (off_t)offset,
-					(size_t)maps[j].size)) == NULL) {
+			    (mapaddr = pci_map_resource(NULL, devname,
+							(off_t)offset,
+							(size_t)maps[j].size)
+			    ) == NULL) {
 				rte_free(uio_res);
 				return (-1);
 			}
- 
+
 			maps[j].addr = mapaddr;
 			maps[j].offset = offset;
 			dev->mem_resource[i].addr = mapaddr;
-- 
1.7.10.4

[PATCH 5/7] 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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 2c3b914..db1fb3f 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;
 }
@@ -1010,8 +1003,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) {
@@ -1019,12 +1012,13 @@ 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
-		if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO)
-			/* just map resources for Intel NICs */
+#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)
 				return -1;
-#endif
+		}
 
 		/* reference driver structure */
 		dev->driver = dr;
-- 
1.7.10.4

[PATCH 6/7] 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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |   89 ++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 45 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index db1fb3f..c21b270 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)
@@ -1006,11 +1005,6 @@ 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
 
@@ -1018,6 +1012,11 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
 			/* 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 */
-- 
1.7.10.4

[PATCH 7/7] 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c |  171 ---------------------------------
 1 file changed, 171 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index c21b270..538d2d0 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(void *requested_addr, const char *devname, off_t offset,
@@ -303,24 +154,10 @@ pci_map_resource(void *requested_addr, const char *devname, off_t offset,
 	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;
-	for (n = 0; n < UIO_DEV_WAIT_TIMEOUT*10; 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));
@@ -1000,14 +837,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: [PATCH 0/7] pci cleanup

[Burakov, Anatoly]

> Here is an attempt at having an equal implementation in bsd and linux
> eal_pci.c.
> It results in following changes :
> - checks on driver flag in bsd which were missing
> - remove virtio-uio workaround in linux eal_pci.c
> - remove deprecated RTE_EAL_UNBIND_PORTS option
> 
> Along the way, I discovered two small bugs: a mem leak in linux eal_pci.c and
> a
> fd leak in both bsd and linux eal_pci.c.
> 
> 

Acked-by: Anatoly Burakov <anatoly.burakov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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: [PATCH 0/7] pci cleanup

[Neil Horman]

On Mon, Apr 28, 2014 at 03:19:40PM +0200, David Marchand wrote:
> Hello all, 
> 
> Here is an attempt at having an equal implementation in bsd and linux eal_pci.c.
> It results in following changes :
> - checks on driver flag in bsd which were missing
> - remove virtio-uio workaround in linux eal_pci.c
> - remove deprecated RTE_EAL_UNBIND_PORTS option
> 
> Along the way, I discovered two small bugs: a mem leak in linux eal_pci.c and a
> fd leak in both bsd and linux eal_pci.c.
> 
Series 
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

> 
> -- 
> David Marchand
> 
> David Marchand (7):
>   pci: fix potential mem leak
>   pci: align bsd implementation on linux
>   pci: remove virtio-uio workaround
>   pci: rework interrupt fd init and fix fd leak
>   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/bsdapp/eal/eal_pci.c   |  105 ++++++------
>  lib/librte_eal/linuxapp/eal/eal_pci.c |  282 +++++----------------------------
>  lib/librte_pmd_virtio/virtio_ethdev.c |  133 +++++++++++++++-
>  3 files changed, 218 insertions(+), 302 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> 

Re: [PATCH 1/7] pci: fix potential mem leak

[Burakov, Anatoly]

Hi David,

> Looking at bsd implementation, we can see that there is a potential mem
> leak in linux implementation. Fix this.
> 
> Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 9538efe..313bab7 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -678,6 +678,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  					(mapaddr = pci_map_resource(dev,
>  					NULL, devname, (off_t)offset,
>  					(size_t)maps[j].size)) == NULL) {
> +				rte_free(uio_res);
>  				return (-1);
>  			}
> 
> --
> 1.7.10.4

Actually, there's another possible mem leak, right after uio_res = rte_zmalloc():

	/* collect info about device mappings */
	if ((nb_maps = pci_uio_get_mappings(dirname, uio_res->maps,
			sizeof (uio_res->maps) / sizeof (uio_res->maps[0])))
			< 0)
		return (nb_maps);

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: [PATCH 1/7] pci: fix potential mem leak

[David Marchand]

Hello Anatoly,

Oh yes, missed this one.
Ok, I will resubmit an updated patch.

Thanks.

-- 
David Marchand



On Thu, May 1, 2014 at 5:00 PM, Burakov, Anatoly
<anatoly.burakov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>wrote:

> Hi David,
>
> > Looking at bsd implementation, we can see that there is a potential mem
> > leak in linux implementation. Fix this.
> >
> > Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal_pci.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c
> > b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > index 9538efe..313bab7 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > @@ -678,6 +678,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
> >                                       (mapaddr = pci_map_resource(dev,
> >                                       NULL, devname, (off_t)offset,
> >                                       (size_t)maps[j].size)) == NULL) {
> > +                             rte_free(uio_res);
> >                               return (-1);
> >                       }
> >
> > --
> > 1.7.10.4
>
> Actually, there's another possible mem leak, right after uio_res =
> rte_zmalloc():
>
>         /* collect info about device mappings */
>         if ((nb_maps = pci_uio_get_mappings(dirname, uio_res->maps,
>                         sizeof (uio_res->maps) / sizeof
> (uio_res->maps[0])))
>                         < 0)
>                 return (nb_maps);
>
> 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
>
>
>