[PATCH 0/9] fix hotplug API

[PATCH 0/9] fix hotplug API

[Gaetan Rivet]

Sending those fixes as separate patches as they stand on their own.
This series improves usability of the hotplug API and fixes a few issues
with existing implementations.

The hotplug API can be tested with the fail-safe PMD[1]. Its
documentation describes how to declare slaves and how to use it.

[1]: http://dpdk.org/ml/archives/dev/2017-July/070529.html

Gaetan Rivet (9):
  eal: return device handle upon plugin
  eal: fix hotplug add
  devargs: introduce removal function
  eal: release devargs on device removal
  pci: use given name as generic name
  pci: fix generic driver pointer on probe error
  pci: fix hotplug operations
  vdev: implement plug operation
  bus: remove useless plug parameter

 lib/librte_eal/bsdapp/eal/eal_pci.c             |  4 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_dev.c          | 83 +++++++++++++++++++++----
 lib/librte_eal/common/eal_common_devargs.c      | 18 ++++++
 lib/librte_eal/common/eal_common_pci.c          | 49 +++++++--------
 lib/librte_eal/common/eal_common_vdev.c         | 12 ++--
 lib/librte_eal/common/eal_private.h             |  5 ++
 lib/librte_eal/common/include/rte_bus.h         |  6 +-
 lib/librte_eal/common/include/rte_dev.h         | 10 +--
 lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++
 lib/librte_eal/common/include/rte_vdev.h        |  7 +++
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  4 +-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 13 files changed, 162 insertions(+), 56 deletions(-)

-- 
2.1.4

[PATCH 1/9] eal: return device handle upon plugin

[Gaetan Rivet]

This device handle can be useful for other systems calling hotplug
functions, to retrieve for example any associated ethdev.

This change avoids unecessary calls to rte_bus_find, as the handle is
already known and available.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c  | 40 +++++++++++++++++++++++----------
 lib/librte_eal/common/include/rte_dev.h | 10 +++++----
 2 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 32e12b5..292fefe 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -42,6 +42,7 @@
 #include <rte_devargs.h>
 #include <rte_debug.h>
 #include <rte_devargs.h>
+#include <rte_errno.h>
 #include <rte_log.h>
 
 #include "eal_private.h"
@@ -67,6 +68,7 @@ static int cmp_dev_name(const struct rte_device *dev, const void *_name)
 
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
+	struct rte_device *dev;
 	int ret;
 
 	if (name == NULL || devargs == NULL) {
@@ -74,9 +76,9 @@ int rte_eal_dev_attach(const char *name, const char *devargs)
 		return -EINVAL;
 	}
 
-	ret = rte_eal_hotplug_add("pci", name, devargs);
-	if (ret && ret != -EINVAL)
-		return ret;
+	dev = rte_eal_hotplug_add("pci", name, devargs);
+	if (dev == NULL && rte_errno != EINVAL)
+		return -rte_errno;
 
 	/*
 	 * If we haven't found a bus device the user meant to "hotplug" a
@@ -118,7 +120,8 @@ int rte_eal_dev_detach(struct rte_device *dev)
 	return ret;
 }
 
-int rte_eal_hotplug_add(const char *busname, const char *devname,
+struct rte_device *
+rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *devargs)
 {
 	struct rte_bus *bus;
@@ -128,31 +131,39 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 	bus = rte_bus_find_by_name(busname);
 	if (bus == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
-		return -ENOENT;
+		rte_errno = ENOENT;
+		return NULL;
 	}
 
 	if (bus->plug == NULL) {
 		RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
 			bus->name);
-		return -ENOTSUP;
+		rte_errno = ENOTSUP;
+		return NULL;
 	}
 
 	ret = bus->scan();
-	if (ret)
-		return ret;
+	if (ret) {
+		rte_errno = -ret;
+		return NULL;
+	}
 
 	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
-		return -EINVAL;
+		rte_errno = EINVAL;
+		return NULL;
 	}
 
 	ret = bus->plug(dev, devargs);
-	if (ret)
+	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
-	return ret;
+		rte_errno = -ret;
+		return NULL;
+	}
+	return dev;
 }
 
 int rte_eal_hotplug_remove(const char *busname, const char *devname)
@@ -164,24 +175,29 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
 	bus = rte_bus_find_by_name(busname);
 	if (bus == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
+		rte_errno = ENOENT;
 		return -ENOENT;
 	}
 
 	if (bus->unplug == NULL) {
 		RTE_LOG(ERR, EAL, "Function unplug not supported by bus (%s)\n",
 			bus->name);
+		rte_errno = ENOTSUP;
 		return -ENOTSUP;
 	}
 
 	dev = bus->find_device(NULL, cmp_dev_name, devname);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find plugged device (%s)\n", devname);
+		rte_errno = EINVAL;
 		return -EINVAL;
 	}
 
 	ret = bus->unplug(dev);
-	if (ret)
+	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
 			dev->name);
+		rte_errno = -ret;
+	}
 	return ret;
 }
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index bcd8b1e..30d1f2e 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -218,10 +218,12 @@ int rte_eal_dev_detach(struct rte_device *dev);
  * @param devargs
  *   Device arguments to be passed to the driver.
  * @return
- *   0 on success, negative on error.
+ *   The pointer to the plugged rte_device on success.
+ *   NULL on error. rte_errno is then set.
  */
-int rte_eal_hotplug_add(const char *busname, const char *devname,
-			const char *devargs);
+struct rte_device *rte_eal_hotplug_add(const char *busname,
+				       const char *devname,
+				       const char *devargs);
 
 /**
  * @warning
@@ -234,7 +236,7 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
  * @param devname
  *   The device name being removed.
  * @return
- *   0 on success, negative on error.
+ *   0 on success, negative on error. rte_errno is then set.
  */
 int rte_eal_hotplug_remove(const char *busname, const char *devname);
 
-- 
2.1.4

[PATCH 2/9] eal: fix hotplug add

[Gaetan Rivet]

New device should be represented by an rte_devargs prior to being
plugged.

Device parameters are available to rte_devices via their devargs field.
This field should be set up as soon as possible, as central information
are stored within, such as the device name which is used to search
the newly scanned device before plugging it in.

Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c | 43 +++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 292fefe..708c8e9 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -120,12 +120,32 @@ int rte_eal_dev_detach(struct rte_device *dev)
 	return ret;
 }
 
+static char *
+full_dev_name(const char *bus, const char *dev, const char *args)
+{
+	char *name;
+	size_t len;
+
+	len = strlen(bus) + 1 +
+	      strlen(dev) + 1 +
+	      strlen(args) + 1;
+	name = calloc(1, len);
+	if (name == NULL) {
+		RTE_LOG(ERR, EAL, "Could not allocate full device name\n");
+		return NULL;
+	}
+	snprintf(name, len, "%s:%s,%s", bus, dev,
+		 args ? args : "");
+	return name;
+}
+
 struct rte_device *
 rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *devargs)
 {
 	struct rte_bus *bus;
 	struct rte_device *dev;
+	char *name;
 	int ret;
 
 	bus = rte_bus_find_by_name(busname);
@@ -142,10 +162,22 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 		return NULL;
 	}
 
+	name = full_dev_name(busname, devname, devargs);
+	if (name == NULL) {
+		rte_errno = ENOMEM;
+		return NULL;
+	}
+
+	ret = rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, name);
+	if (ret) {
+		rte_errno = EINVAL;
+		goto err_name;
+	}
+
 	ret = bus->scan();
 	if (ret) {
 		rte_errno = -ret;
-		return NULL;
+		goto err_name;
 	}
 
 	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
@@ -153,7 +185,7 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
 		rte_errno = EINVAL;
-		return NULL;
+		goto err_name;
 	}
 
 	ret = bus->plug(dev, devargs);
@@ -161,9 +193,14 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
 		rte_errno = -ret;
-		return NULL;
+		goto err_name;
 	}
+	free(name);
 	return dev;
+
+err_name:
+	free(name);
+	return NULL;
 }
 
 int rte_eal_hotplug_remove(const char *busname, const char *devname)
-- 
2.1.4

[PATCH 3/9] devargs: introduce removal function

[Gaetan Rivet]

Hotplug support introduces the possibility of removing devices from the
system. Allocated resources must be freed.

Extend the rte_devargs API to allow freeing allocated resources.

This API is experimental and bound to change. It is currently designed
as a symetrical to rte_eal_devargs_add(), but the latter will evolve
shortly anyway.

Its DEVTYPE parameter is currently only used to specify scan policies,
and those will evolve in the next release. This evolution should
rationalize the rte_devargs API.

As such, the proposed API here is not the most convenient, but is
taylored to follow the current design and integrate easily with its main
use within rte_eal_hotplug_* functions.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 18 ++++++++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 38 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895..d3cf1ae 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -207,6 +207,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_rmv;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a3..4a89282 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -41,6 +41,7 @@
 #include <string.h>
 
 #include <rte_devargs.h>
+#include <rte_tailq.h>
 #include "eal_private.h"
 
 /** Global list of user devices */
@@ -182,6 +183,23 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	return -1;
 }
 
+int
+rte_eal_devargs_rmv(const char *busname, const char *devname)
+{
+	struct rte_devargs *d;
+	void *tmp;
+
+	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
+		if (strcmp(d->bus->name, busname) == 0 &&
+		    strcmp(d->name, devname) == 0) {
+			TAILQ_REMOVE(&devargs_list, d, next);
+			free(d->args);
+			free(d);
+		}
+	}
+	return 1;
+}
+
 /* count the number of devices of a specified type */
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd..89679bb 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * Remove a device from the user device list.
+ * Its resources are freed.
+ * If the devargs cannot be found, nothing happens.
+ *
+ * @param busname
+ *   bus name of the devargs to remove.
+ *
+ * @param devname
+ *   device name of the devargs to remove.
+ *
+ * @return
+ *   0 on success.
+ *   <0 on error.
+ *   >0 if the devargs was not within the user device list.
+ */
+int rte_eal_devargs_rmv(const char *busname, const char *devname);
+
+/**
  * Count the number of user devices of a specified type
  *
  * @param devtype
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009..d59308a 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -212,6 +212,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_rmv;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
-- 
2.1.4

[PATCH 4/9] eal: release devargs on device removal

[Gaetan Rivet]

Use the new rte_eal_devargs_rmv() API to free allocated resources
during device removal or when encountering errors when adding devices.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 708c8e9..143c231 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -177,7 +177,7 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 	ret = bus->scan();
 	if (ret) {
 		rte_errno = -ret;
-		goto err_name;
+		goto err_devarg;
 	}
 
 	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
@@ -185,7 +185,7 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
 		rte_errno = EINVAL;
-		goto err_name;
+		goto err_devarg;
 	}
 
 	ret = bus->plug(dev, devargs);
@@ -193,11 +193,13 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
 		rte_errno = -ret;
-		goto err_name;
+		goto err_devarg;
 	}
 	free(name);
 	return dev;
 
+err_devarg:
+	rte_eal_devargs_rmv(busname, devname);
 err_name:
 	free(name);
 	return NULL;
@@ -230,6 +232,8 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -EINVAL;
 	}
 
+	rte_eal_devargs_rmv(busname, devname);
+	dev->devargs = NULL;
 	ret = bus->unplug(dev);
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
-- 
2.1.4

[PATCH 5/9] pci: use given name as generic name

[Gaetan Rivet]

The PCI device is referenced by other DPDK systems by the name of its
parameter, not by the system name.

Moreover, this name should be set once and for all, as early as
possible. This means linking the rte_devargs associated with the PCI
device during its scan, making available other metadatas used afterward
upon probing and device search for plugging.

Fixes: beec692c5157 ("eal: add name field to generic device")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c    |  4 ++--
 lib/librte_eal/common/eal_common_pci.c | 21 ++++++++++++++++-----
 lib/librte_eal/common/eal_private.h    |  5 +++++
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  4 ++--
 4 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index e321461..97a88ec 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -282,8 +282,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 	/* FreeBSD has no NUMA support (yet) */
 	dev->device.numa_node = 0;
 
-	rte_pci_device_name(&dev->addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* FreeBSD has only one pass through driver */
 	dev->kdrv = RTE_KDRV_NIC_UIO;
@@ -334,6 +333,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource,
 					dev->mem_resource,
 					sizeof(dev->mem_resource));
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 76bbcc8..b100525 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -88,6 +88,21 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 	return NULL;
 }
 
+void
+pci_name_set(struct rte_pci_device *dev)
+{
+	struct rte_devargs *devargs;
+
+	rte_pci_device_name(&dev->addr,
+			dev->name, sizeof(dev->name));
+	devargs = pci_devargs_lookup(dev);
+	dev->device.devargs = devargs;
+	if (devargs != NULL)
+		dev->device.name = dev->device.devargs->name;
+	else
+		dev->device.name = dev->name;
+}
+
 /* map a particular resource from a file */
 void *
 pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
@@ -396,11 +411,7 @@ rte_pci_probe(void)
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
-		/* set devargs in PCI structure */
-		devargs = pci_devargs_lookup(dev);
-		if (devargs != NULL)
-			dev->device.devargs = devargs;
-
+		devargs = dev->device.devargs;
 		/* probe all or only whitelisted devices */
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 0836339..597d82e 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -113,6 +113,11 @@ struct rte_pci_driver;
 struct rte_pci_device;
 
 /**
+ * Find the name of a PCI device.
+ */
+void pci_name_set(struct rte_pci_device *dev);
+
+/**
  * Add a PCI device to the PCI Bus (append to PCI Device list). This function
  * also updates the bus references of the PCI Device (and the generic device
  * object embedded within.
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 7d9e1a9..556ae2c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -324,8 +324,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 		dev->device.numa_node = 0;
 	}
 
-	rte_pci_device_name(addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* parse resources */
 	snprintf(filename, sizeof(filename), "%s/resource", dirname);
@@ -373,6 +372,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource, dev->mem_resource,
 					sizeof(dev->mem_resource));
 				free(dev);
-- 
2.1.4

[PATCH 6/9] pci: fix generic driver pointer on probe error

[Gaetan Rivet]

The field is set but never resetted on error.
This marks the device as being attached while it is not, and forbid
further attempts to hotplug it.

Fixes: 7917d5f5ea46 ("pci: initialize generic driver pointer")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index b100525..9cc4148 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -237,6 +237,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		dev->driver = NULL;
+		dev->device.driver = NULL;
 		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
 			/* Don't unmap if device is unsupported and
 			 * driver needs mapped resources.
-- 
2.1.4

[PATCH 7/9] pci: fix hotplug operations

[Gaetan Rivet]

The device handle is already known and does not have to be infered from
the PCI address. The relevant helpers are already available within the
PCI bus to avoid searching for a handle already known.

Additionally, rte_memcpy.h was erroneously included.

Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 9cc4148..7e82a31 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -47,7 +47,6 @@
 #include <rte_pci.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
-#include <rte_memcpy.h>
 #include <rte_memzone.h>
 #include <rte_eal.h>
 #include <rte_string_fns.h>
@@ -536,32 +535,20 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 static int
 pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
 {
-	struct rte_pci_device *pdev;
-	struct rte_pci_addr *addr;
-
-	addr = &RTE_DEV_TO_PCI(dev)->addr;
-
-	/* Find the current device holding this address in the bus. */
-	FOREACH_DEVICE_ON_PCIBUS(pdev) {
-		if (rte_eal_compare_pci_addr(&pdev->addr, addr) == 0)
-			return rte_pci_probe_one(addr);
-	}
-
-	rte_errno = ENODEV;
-	return -1;
+	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
 
 static int
 pci_unplug(struct rte_device *dev)
 {
 	struct rte_pci_device *pdev;
+	int ret;
 
 	pdev = RTE_DEV_TO_PCI(dev);
-	if (rte_pci_detach(&pdev->addr) != 0) {
-		rte_errno = ENODEV;
-		return -1;
-	}
-	return 0;
+	ret = rte_pci_detach_dev(pdev);
+	rte_pci_remove_device(pdev);
+	free(pdev);
+	return ret;
 }
 
 struct rte_pci_bus rte_pci_bus = {
-- 
2.1.4

[PATCH 8/9] vdev: implement plug operation

[Gaetan Rivet]

This method must be implemented to allow using a unified, generic API to
hotplug devices, including virtual ones.

VDEV devices actually exist unattached after performing a scan on the
rte_devargs list. As such it makes sense to be able to perform a device
hotplug afterward.

Finally, missing this generic interface forces the EAL to be dependent
on vdev-specific API, which hinders the plan of moving the vdev bus to
drivers/bus.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_vdev.c  | 12 +++++++-----
 lib/librte_eal/common/include/rte_vdev.h |  7 +++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index 5d81ca8..f8b48b5 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -354,12 +354,14 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
+vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+{
+	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
+}
+
+static int
 vdev_unplug(struct rte_device *dev)
 {
-	/*
-	 * The virtual bus doesn't support 'unattached' devices so this is
-	 * actually equal to hotplugging removal of it.
-	 */
 	return rte_vdev_uninit(dev->name);
 }
 
@@ -367,7 +369,7 @@ static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
 	.find_device = vdev_find_device,
-	/* .plug = NULL, see comment on vdev_unplug */
+	.plug = vdev_plug,
 	.unplug = vdev_unplug,
 	.parse = vdev_parse,
 };
diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
index 3c07b76..639e6d6 100644
--- a/lib/librte_eal/common/include/rte_vdev.h
+++ b/lib/librte_eal/common/include/rte_vdev.h
@@ -46,6 +46,13 @@ struct rte_vdev_device {
 	struct rte_device device;               /**< Inherit core device */
 };
 
+/**
+ * @internal
+ * Helper macro for drivers that need to convert to struct rte_vdev_device.
+ */
+#define RTE_DEV_TO_VDEV(ptr) \
+	container_of(ptr, struct rte_vdev_device, device)
+
 static inline const char *
 rte_vdev_device_name(const struct rte_vdev_device *dev)
 {
-- 
2.1.4

[PATCH 9/9] bus: remove useless plug parameter

[Gaetan Rivet]

The prior scan should link the relevant rte_devargs to the newly
allocated rte_device. As such, it is useless to pass device arguments to
the plug callback. Those arguments are available within the devargs
field of the rte_device structure.

Fixes: 7c8810f43f6e ("bus: introduce device plug/unplug")
Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")
Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c  | 2 +-
 lib/librte_eal/common/eal_common_pci.c  | 2 +-
 lib/librte_eal/common/eal_common_vdev.c | 2 +-
 lib/librte_eal/common/include/rte_bus.h | 6 +-----
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 143c231..42e91db 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -188,7 +188,7 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 		goto err_devarg;
 	}
 
-	ret = bus->plug(dev, devargs);
+	ret = bus->plug(dev);
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 7e82a31..a64d76c 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -533,7 +533,7 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
+pci_plug(struct rte_device *dev)
 {
 	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index f8b48b5..a2cdfa4 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -354,7 +354,7 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+vdev_plug(struct rte_device *dev)
 {
 	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
 }
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index 37cc230..167635a 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -114,15 +114,11 @@ typedef struct rte_device *
  * @param dev
  *	Device pointer that was returned by a previous call to find_device.
  *
- * @param devargs
- *	Device declaration.
- *
  * @return
  *	0 on success.
  *	!0 on error.
  */
-typedef int (*rte_bus_plug_t)(struct rte_device *dev,
-			      const char *devargs);
+typedef int (*rte_bus_plug_t)(struct rte_device *dev);
 
 /**
  * Implementation specific remove function which is responsible for unlinking
-- 
2.1.4

Re: [PATCH 0/9] fix hotplug API

[Jan Blunck]

On Sat, Jul 8, 2017 at 9:45 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> Sending those fixes as separate patches as they stand on their own.
> This series improves usability of the hotplug API and fixes a few issues
> with existing implementations.
>

Interesting that you send this series as fixes. From what I can tell
this is a collection of changes that you have proposed before and I
have commented on and requested changes for. But I don't see that they
have been addressed at all:

- you still concatenate the bus and device name just to pass it down
to the buses and parse it into its components again
- you still delegate the devargs parsing to the buses

Please fix this issues. If you really want to return a device handle
from the hotplug API please present the use-case and in which cases
this solves a real-world problem.


> The hotplug API can be tested with the fail-safe PMD[1]. Its
> documentation describes how to declare slaves and how to use it.
>
> [1]: http://dpdk.org/ml/archives/dev/2017-July/070529.html
>
> Gaetan Rivet (9):
>   eal: return device handle upon plugin
>   eal: fix hotplug add
>   devargs: introduce removal function
>   eal: release devargs on device removal
>   pci: use given name as generic name
>   pci: fix generic driver pointer on probe error
>   pci: fix hotplug operations
>   vdev: implement plug operation
>   bus: remove useless plug parameter
>
>  lib/librte_eal/bsdapp/eal/eal_pci.c             |  4 +-
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/eal_common_dev.c          | 83 +++++++++++++++++++++----
>  lib/librte_eal/common/eal_common_devargs.c      | 18 ++++++
>  lib/librte_eal/common/eal_common_pci.c          | 49 +++++++--------
>  lib/librte_eal/common/eal_common_vdev.c         | 12 ++--
>  lib/librte_eal/common/eal_private.h             |  5 ++
>  lib/librte_eal/common/include/rte_bus.h         |  6 +-
>  lib/librte_eal/common/include/rte_dev.h         | 10 +--
>  lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++
>  lib/librte_eal/common/include/rte_vdev.h        |  7 +++
>  lib/librte_eal/linuxapp/eal/eal_pci.c           |  4 +-
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  13 files changed, 162 insertions(+), 56 deletions(-)
>
> --
> 2.1.4
>

Re: [PATCH 0/9] fix hotplug API

[Gaëtan Rivet]

On Sun, Jul 09, 2017 at 06:38:52AM -0400, Jan Blunck wrote:
> On Sat, Jul 8, 2017 at 9:45 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > Sending those fixes as separate patches as they stand on their own.
> > This series improves usability of the hotplug API and fixes a few issues
> > with existing implementations.
> >
> 
> Interesting that you send this series as fixes. From what I can tell
> this is a collection of changes that you have proposed before and I
> have commented on and requested changes for. But I don't see that they
> have been addressed at all:
> 

The API changes are limited to two elements to make using this API
easier. The rest are fixes: inserting relevant rte_devargs, fixing PCI
hotplug, supporting the hotplug interface in vdev, releasing the
relevant resources on remove. Those are bugs which make hotplug
impossible to use right now, and this series aims to fix those.

> - you still concatenate the bus and device name just to pass it down
> to the buses and parse it into its components again

I use the current API. This API has been discussed in other places and
the reason to keep the current behavior, and how it should evolve
shortly, has already been exposed. In the meantime, even if planified to
evolve, the API should still be respected.

I know you disagreed in the relevant thread[2], as you repeat it here.
However, you received a response both from me and Bruce regarding this
issue and did not press the matter further, which led to an implicit approval.

> - you still delegate the devargs parsing to the buses
> 

I agree that this should be avoided. The relevant API is clearly marked
as going out shortly[3].

But this is not relevant to fixing the hotplug API.

> Please fix this issues. If you really want to return a device handle
> from the hotplug API please present the use-case and in which cases
> this solves a real-world problem.
> 
> 

Other solutions can be used instead, always. It just seems simpler to
work with. I can do without it if that's a hangup.

[2]: http://dpdk.org/ml/archives/dev/2017-June/069256.html
[3]: http://dpdk.org/ml/archives/dev/2017-July/070505.html

> > The hotplug API can be tested with the fail-safe PMD[1]. Its
> > documentation describes how to declare slaves and how to use it.
> >
> > [1]: http://dpdk.org/ml/archives/dev/2017-July/070529.html
> >
> > Gaetan Rivet (9):
> >   eal: return device handle upon plugin
> >   eal: fix hotplug add
> >   devargs: introduce removal function
> >   eal: release devargs on device removal
> >   pci: use given name as generic name
> >   pci: fix generic driver pointer on probe error
> >   pci: fix hotplug operations
> >   vdev: implement plug operation
> >   bus: remove useless plug parameter
> >
> >  lib/librte_eal/bsdapp/eal/eal_pci.c             |  4 +-
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> >  lib/librte_eal/common/eal_common_dev.c          | 83 +++++++++++++++++++++----
> >  lib/librte_eal/common/eal_common_devargs.c      | 18 ++++++
> >  lib/librte_eal/common/eal_common_pci.c          | 49 +++++++--------
> >  lib/librte_eal/common/eal_common_vdev.c         | 12 ++--
> >  lib/librte_eal/common/eal_private.h             |  5 ++
> >  lib/librte_eal/common/include/rte_bus.h         |  6 +-
> >  lib/librte_eal/common/include/rte_dev.h         | 10 +--
> >  lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++
> >  lib/librte_eal/common/include/rte_vdev.h        |  7 +++
> >  lib/librte_eal/linuxapp/eal/eal_pci.c           |  4 +-
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >  13 files changed, 162 insertions(+), 56 deletions(-)
> >
> > --
> > 2.1.4
> >

-- 
Gaëtan Rivet
6WIND

Re: [PATCH 0/9] fix hotplug API

[Thomas Monjalon]

09/07/2017 12:38, Jan Blunck:
> On Sat, Jul 8, 2017 at 9:45 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > Sending those fixes as separate patches as they stand on their own.
> > This series improves usability of the hotplug API and fixes a few issues
> > with existing implementations.
> >
> 
> Interesting that you send this series as fixes. From what I can tell
> this is a collection of changes that you have proposed before and I
> have commented on and requested changes for. But I don't see that they
> have been addressed at all:
> 
> - you still concatenate the bus and device name just to pass it down
> to the buses and parse it into its components again
> - you still delegate the devargs parsing to the buses
> 
> Please fix this issues. If you really want to return a device handle
> from the hotplug API please present the use-case and in which cases
> this solves a real-world problem.

Please Jan, could you check in the 9 patches of this series
which ones are OK for you?
Thank you

[PATCH v2 0/8] fix hotplug API

[Gaetan Rivet]

Sending those fixes as separate patches as they stand on their own.
This series improves usability of the hotplug API and fixes a few issues
with existing implementations.

The hotplug API can be tested with the fail-safe PMD[1]. Its
documentation describes how to declare slaves and how to use it.

[1]: http://dpdk.org/ml/archives/dev/2017-July/070529.html


v2:

  - Add a new rte_devargs function taylored for hotplug operations.
    Upon device hotplug, the device is implicitly whitelisted.
    This configuration should supersede any previous existing bus
    configuration.
  - Merge hotplug add and remove fixes as they are now tied up.
  - Do not return an rte_device handle back from hotplug_add.

Gaetan Rivet (8):
  vdev: implement plug operation
  devargs: introduce removal function
  devargs: introduce insert function
  eal: fix hotplug add / remove
  pci: use given name as generic name
  pci: fix generic driver pointer on probe error
  pci: fix hotplug operations
  bus: remove useless plug parameter

 lib/librte_eal/bsdapp/eal/eal_pci.c             |  4 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  2 +
 lib/librte_eal/common/eal_common_dev.c          | 59 +++++++++++++++++++++++--
 lib/librte_eal/common/eal_common_devargs.c      | 31 +++++++++++++
 lib/librte_eal/common/eal_common_pci.c          | 49 ++++++++++----------
 lib/librte_eal/common/eal_common_vdev.c         | 12 ++---
 lib/librte_eal/common/eal_private.h             |  5 +++
 lib/librte_eal/common/include/rte_bus.h         |  6 +--
 lib/librte_eal/common/include/rte_devargs.h     | 31 +++++++++++++
 lib/librte_eal/common/include/rte_vdev.h        |  7 +++
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  4 +-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  2 +
 12 files changed, 169 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH v2 1/8] vdev: implement plug operation

[Gaetan Rivet]

This method must be implemented to allow using a unified, generic API to
hotplug devices, including virtual ones.

VDEV devices actually exist unattached after performing a scan on the
rte_devargs list. As such it makes sense to be able to perform a device
hotplug afterward.

Finally, missing this generic interface forces the EAL to be dependent
on vdev-specific API, which hinders the plan of moving the vdev bus to
drivers/bus.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_vdev.c  | 12 +++++++-----
 lib/librte_eal/common/include/rte_vdev.h |  7 +++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index 2ca0cdb..79bb795 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -316,12 +316,14 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
+vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+{
+	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
+}
+
+static int
 vdev_unplug(struct rte_device *dev)
 {
-	/*
-	 * The virtual bus doesn't support 'unattached' devices so this is
-	 * actually equal to hotplugging removal of it.
-	 */
 	return rte_vdev_uninit(dev->name);
 }
 
@@ -329,7 +331,7 @@ static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
 	.find_device = vdev_find_device,
-	/* .plug = NULL, see comment on vdev_unplug */
+	.plug = vdev_plug,
 	.unplug = vdev_unplug,
 	.parse = vdev_parse,
 };
diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
index 3c07b76..639e6d6 100644
--- a/lib/librte_eal/common/include/rte_vdev.h
+++ b/lib/librte_eal/common/include/rte_vdev.h
@@ -46,6 +46,13 @@ struct rte_vdev_device {
 	struct rte_device device;               /**< Inherit core device */
 };
 
+/**
+ * @internal
+ * Helper macro for drivers that need to convert to struct rte_vdev_device.
+ */
+#define RTE_DEV_TO_VDEV(ptr) \
+	container_of(ptr, struct rte_vdev_device, device)
+
 static inline const char *
 rte_vdev_device_name(const struct rte_vdev_device *dev)
 {
-- 
2.1.4

[PATCH v2 2/8] devargs: introduce removal function

[Gaetan Rivet]

Hotplug support introduces the possibility of removing devices from the
system. Allocated resources must be freed.

Extend the rte_devargs API to allow freeing allocated resources.

This API is experimental and bound to change. It is currently designed
as a symetrical to rte_eal_devargs_add(), but the latter will evolve
shortly anyway.

Its DEVTYPE parameter is currently only used to specify scan policies,
and those will evolve in the next release. This evolution should
rationalize the rte_devargs API.

As such, the proposed API here is not the most convenient, but is
taylored to follow the current design and integrate easily with its main
use within rte_eal_hotplug_* functions.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 39 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895..40cd523 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -207,6 +207,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a3..bcdee13 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -41,6 +41,7 @@
 #include <string.h>
 
 #include <rte_devargs.h>
+#include <rte_tailq.h>
 #include "eal_private.h"
 
 /** Global list of user devices */
@@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	return -1;
 }
 
+int
+rte_eal_devargs_remove(const char *busname, const char *devname)
+{
+	struct rte_devargs *d;
+	void *tmp;
+
+	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
+		if (strcmp(d->bus->name, busname) == 0 &&
+		    strcmp(d->name, devname) == 0) {
+			TAILQ_REMOVE(&devargs_list, d, next);
+			free(d->args);
+			free(d);
+			return 0;
+		}
+	}
+	return 1;
+}
+
 /* count the number of devices of a specified type */
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd..36453b6 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * Remove a device from the user device list.
+ * Its resources are freed.
+ * If the devargs cannot be found, nothing happens.
+ *
+ * @param busname
+ *   bus name of the devargs to remove.
+ *
+ * @param devname
+ *   device name of the devargs to remove.
+ *
+ * @return
+ *   0 on success.
+ *   <0 on error.
+ *   >0 if the devargs was not within the user device list.
+ */
+int rte_eal_devargs_remove(const char *busname, const char *devname);
+
+/**
  * Count the number of user devices of a specified type
  *
  * @param devtype
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009..a8ee349 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -212,6 +212,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
-- 
2.1.4

[PATCH v2 3/8] devargs: introduce insert function

[Gaetan Rivet]

This new function expects a fully-formed rte_devargs, previously parsed
and allocated.

It does not check whether the new rte_devargs is compatible with current
bus configuration, but will replace any eventual existing one for the same
device.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 27 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 40cd523..8b24309 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -206,6 +206,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_devargs_insert;
 	rte_eal_devargs_parse;
 	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index bcdee13..ff6c2a8 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 	return 0;
 }
 
+int
+rte_eal_devargs_insert(struct rte_devargs *da)
+{
+	int ret;
+
+	ret = rte_eal_devargs_remove(da->bus->name, da->name);
+	if (ret < 0)
+		return ret;
+	TAILQ_INSERT_TAIL(&devargs_list, da, next);
+	return 0;
+}
+
 /* store a whitelist parameter for later parsing */
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 36453b6..7b63fa3 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
 		      struct rte_devargs *da);
 
 /**
+ * Insert an rte_devargs in the global list.
+ *
+ * @param da
+ *  The devargs structure to insert.
+ *
+ * @return
+ *   - 0 on success
+ *   - Negative on error.
+ */
+int
+rte_eal_devargs_insert(struct rte_devargs *da);
+
+/**
  * Add a device to the user device list
  *
  * For PCI devices, the format of arguments string is "PCI_ADDR" or
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index a8ee349..81f6af3 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -211,6 +211,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_devargs_insert;
 	rte_eal_devargs_parse;
 	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
-- 
2.1.4

[PATCH v2 4/8] eal: fix hotplug add / remove

[Gaetan Rivet]

New device should be represented by an rte_devargs prior to being
plugged.

Device parameters are available to rte_devices via their devargs field.
This field should be set up as soon as possible, as central information
are stored within, such as the device name which is used to search
the newly scanned device before plugging it in.

When a device is introduced using the hotplug API, it is implicitly
whitelisted. As such, it can conflict with existing bus configuration.
The new rte_devargs uses the new rte_eal_devargs_insert function that
supersedes previous rte_devargs, allowing to force the insertion of new
devices.

Upon device removal, the newly allocated rte_devargs are freed.

Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c | 57 ++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 32e12b5..f5566a6 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -118,11 +118,32 @@ int rte_eal_dev_detach(struct rte_device *dev)
 	return ret;
 }
 
+static char *
+full_dev_name(const char *bus, const char *dev, const char *args)
+{
+	char *name;
+	size_t len;
+
+	len = strlen(bus) + 1 +
+	      strlen(dev) + 1 +
+	      strlen(args) + 1;
+	name = calloc(1, len);
+	if (name == NULL) {
+		RTE_LOG(ERR, EAL, "Could not allocate full device name\n");
+		return NULL;
+	}
+	snprintf(name, len, "%s:%s,%s", bus, dev,
+		 args ? args : "");
+	return name;
+}
+
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *devargs)
 {
 	struct rte_bus *bus;
 	struct rte_device *dev;
+	struct rte_devargs *da;
+	char *name;
 	int ret;
 
 	bus = rte_bus_find_by_name(busname);
@@ -137,21 +158,49 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 		return -ENOTSUP;
 	}
 
+	name = full_dev_name(busname, devname, devargs);
+	if (name == NULL)
+		return -ENOMEM;
+
+	da = calloc(1, sizeof(*da));
+	if (da == NULL) {
+		ret = -ENOMEM;
+		goto err_name;
+	}
+
+	ret = rte_eal_devargs_parse(name, da);
+	if (ret)
+		goto err_devarg;
+
+	ret = rte_eal_devargs_insert(da);
+	if (ret)
+		goto err_devarg;
+
 	ret = bus->scan();
 	if (ret)
-		return ret;
+		goto err_devarg;
 
 	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
-		return -EINVAL;
+		ret = -ENODEV;
+		goto err_devarg;
 	}
 
 	ret = bus->plug(dev, devargs);
-	if (ret)
+	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
+		goto err_devarg;
+	}
+	free(name);
+	return 0;
+
+err_devarg:
+	rte_eal_devargs_remove(busname, devname);
+err_name:
+	free(name);
 	return ret;
 }
 
@@ -179,6 +228,8 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -EINVAL;
 	}
 
+	rte_eal_devargs_remove(busname, devname);
+	dev->devargs = NULL;
 	ret = bus->unplug(dev);
 	if (ret)
 		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
-- 
2.1.4

[PATCH v2 5/8] pci: use given name as generic name

[Gaetan Rivet]

The PCI device is referenced by other DPDK systems by the name of its
parameter, not by the system name.

Moreover, this name should be set once and for all, as early as
possible. This means linking the rte_devargs associated with the PCI
device during its scan, making available other metadatas used afterward
upon probing and device search for plugging.

Fixes: beec692c5157 ("eal: add name field to generic device")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c    |  4 ++--
 lib/librte_eal/common/eal_common_pci.c | 21 ++++++++++++++++-----
 lib/librte_eal/common/eal_private.h    |  5 +++++
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  4 ++--
 4 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index e321461..97a88ec 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -282,8 +282,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 	/* FreeBSD has no NUMA support (yet) */
 	dev->device.numa_node = 0;
 
-	rte_pci_device_name(&dev->addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* FreeBSD has only one pass through driver */
 	dev->kdrv = RTE_KDRV_NIC_UIO;
@@ -334,6 +333,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource,
 					dev->mem_resource,
 					sizeof(dev->mem_resource));
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 76bbcc8..b100525 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -88,6 +88,21 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 	return NULL;
 }
 
+void
+pci_name_set(struct rte_pci_device *dev)
+{
+	struct rte_devargs *devargs;
+
+	rte_pci_device_name(&dev->addr,
+			dev->name, sizeof(dev->name));
+	devargs = pci_devargs_lookup(dev);
+	dev->device.devargs = devargs;
+	if (devargs != NULL)
+		dev->device.name = dev->device.devargs->name;
+	else
+		dev->device.name = dev->name;
+}
+
 /* map a particular resource from a file */
 void *
 pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
@@ -396,11 +411,7 @@ rte_pci_probe(void)
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
-		/* set devargs in PCI structure */
-		devargs = pci_devargs_lookup(dev);
-		if (devargs != NULL)
-			dev->device.devargs = devargs;
-
+		devargs = dev->device.devargs;
 		/* probe all or only whitelisted devices */
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 0836339..597d82e 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -113,6 +113,11 @@ struct rte_pci_driver;
 struct rte_pci_device;
 
 /**
+ * Find the name of a PCI device.
+ */
+void pci_name_set(struct rte_pci_device *dev);
+
+/**
  * Add a PCI device to the PCI Bus (append to PCI Device list). This function
  * also updates the bus references of the PCI Device (and the generic device
  * object embedded within.
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 7d9e1a9..556ae2c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -324,8 +324,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 		dev->device.numa_node = 0;
 	}
 
-	rte_pci_device_name(addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* parse resources */
 	snprintf(filename, sizeof(filename), "%s/resource", dirname);
@@ -373,6 +372,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource, dev->mem_resource,
 					sizeof(dev->mem_resource));
 				free(dev);
-- 
2.1.4

[PATCH v2 6/8] pci: fix generic driver pointer on probe error

[Gaetan Rivet]

The field is set but never resetted on error.
This marks the device as being attached while it is not, and forbid
further attempts to hotplug it.

Fixes: 7917d5f5ea46 ("pci: initialize generic driver pointer")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index b100525..9cc4148 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -237,6 +237,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		dev->driver = NULL;
+		dev->device.driver = NULL;
 		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
 			/* Don't unmap if device is unsupported and
 			 * driver needs mapped resources.
-- 
2.1.4

[PATCH v2 7/8] pci: fix hotplug operations

[Gaetan Rivet]

The device handle is already known and does not have to be infered from
the PCI address. The relevant helpers are already available within the
PCI bus to avoid searching for a handle already known.

Additionally, rte_memcpy.h was erroneously included.

Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 9cc4148..7e82a31 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -47,7 +47,6 @@
 #include <rte_pci.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
-#include <rte_memcpy.h>
 #include <rte_memzone.h>
 #include <rte_eal.h>
 #include <rte_string_fns.h>
@@ -536,32 +535,20 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 static int
 pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
 {
-	struct rte_pci_device *pdev;
-	struct rte_pci_addr *addr;
-
-	addr = &RTE_DEV_TO_PCI(dev)->addr;
-
-	/* Find the current device holding this address in the bus. */
-	FOREACH_DEVICE_ON_PCIBUS(pdev) {
-		if (rte_eal_compare_pci_addr(&pdev->addr, addr) == 0)
-			return rte_pci_probe_one(addr);
-	}
-
-	rte_errno = ENODEV;
-	return -1;
+	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
 
 static int
 pci_unplug(struct rte_device *dev)
 {
 	struct rte_pci_device *pdev;
+	int ret;
 
 	pdev = RTE_DEV_TO_PCI(dev);
-	if (rte_pci_detach(&pdev->addr) != 0) {
-		rte_errno = ENODEV;
-		return -1;
-	}
-	return 0;
+	ret = rte_pci_detach_dev(pdev);
+	rte_pci_remove_device(pdev);
+	free(pdev);
+	return ret;
 }
 
 struct rte_pci_bus rte_pci_bus = {
-- 
2.1.4

[PATCH v2 8/8] bus: remove useless plug parameter

[Gaetan Rivet]

The prior scan should link the relevant rte_devargs to the newly
allocated rte_device. As such, it is useless to pass device arguments to
the plug callback. Those arguments are available within the devargs
field of the rte_device structure.

Fixes: 7c8810f43f6e ("bus: introduce device plug/unplug")
Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")
Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c  | 2 +-
 lib/librte_eal/common/eal_common_pci.c  | 2 +-
 lib/librte_eal/common/eal_common_vdev.c | 2 +-
 lib/librte_eal/common/include/rte_bus.h | 6 +-----
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index f5566a6..6fcbea4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -188,7 +188,7 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 		goto err_devarg;
 	}
 
-	ret = bus->plug(dev, devargs);
+	ret = bus->plug(dev);
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 7e82a31..a64d76c 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -533,7 +533,7 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
+pci_plug(struct rte_device *dev)
 {
 	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index 79bb795..0817573 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -316,7 +316,7 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+vdev_plug(struct rte_device *dev)
 {
 	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
 }
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index af9f0e1..c79368d 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -113,15 +113,11 @@ typedef struct rte_device *
  * @param dev
  *	Device pointer that was returned by a previous call to find_device.
  *
- * @param devargs
- *	Device declaration.
- *
  * @return
  *	0 on success.
  *	!0 on error.
  */
-typedef int (*rte_bus_plug_t)(struct rte_device *dev,
-			      const char *devargs);
+typedef int (*rte_bus_plug_t)(struct rte_device *dev);
 
 /**
  * Implementation specific remove function which is responsible for unlinking
-- 
2.1.4

Re: [PATCH 0/9] fix hotplug API

[Gaëtan Rivet]

On Tue, Jul 11, 2017 at 03:07:02PM -0400, Jan Blunck wrote:
> On Tue, Jul 11, 2017 at 12:02 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> > On Sun, Jul 09, 2017 at 04:19:05PM +0200, Thomas Monjalon wrote:
> >> 09/07/2017 12:38, Jan Blunck:
> >> > On Sat, Jul 8, 2017 at 9:45 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> > > Sending those fixes as separate patches as they stand on their own.
> >> > > This series improves usability of the hotplug API and fixes a few issues
> >> > > with existing implementations.
> >> > >
> >> >
> >> > Interesting that you send this series as fixes. From what I can tell
> >> > this is a collection of changes that you have proposed before and I
> >> > have commented on and requested changes for. But I don't see that they
> >> > have been addressed at all:
> >> >
> >> > - you still concatenate the bus and device name just to pass it down
> >> > to the buses and parse it into its components again
> >> > - you still delegate the devargs parsing to the buses
> >> >
> >> > Please fix this issues. If you really want to return a device handle
> >> > from the hotplug API please present the use-case and in which cases
> >> > this solves a real-world problem.
> >>
> >> Please Jan, could you check in the 9 patches of this series
> >> which ones are OK for you?
> >> Thank you
> >>
> >
> > Hi Jan,
> >
> > I sent a new version, consisting mostly in fixes, apart from removing
> > the devarg argument from plug / unplug (which is closely tied to the
> > fixes I proposed). I removed the changes to rte_eal_hotplug_add return
> > value.
> >
> 
> Thanks for the notice. I'll have a look.
> 
> > What do you think of this version? The hotplug must be fixed before
> > the release, this way or another.
> >
> 
> Do you mean the hotplug of real devices or of virtual devices?

Well, both ideally. Virtual devices are not so different from real
devices in the end.

The vdev scan allocates rte_vdev_devices, which are linked to their
rte_devargs. after this scan, it is possible to use vdev->find_device() to get
back the handle to use during the (hypothetical) vdev->plug().

So it is not far from a regular bus. This proximity is a good thing IMO,
as it allows genericity in its use. No need to define an exception for
the vdev bus, it can be used with the others.

Which is useful, if we consider decoupling the EAL from the vdev bus.
Given that this change will not happen during this release it
is not strictly necessary, but it seems clean enough and should simplify
future work anyway.

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [PATCH v2 5/8] pci: use given name as generic name

[Thomas Monjalon]

11/07/2017 01:19, Gaetan Rivet:
> The PCI device is referenced by other DPDK systems by the name of its
> parameter, not by the system name.

I don't understand this sentence. Please give an example.

> Moreover, this name should be set once and for all, as early as
> possible. This means linking the rte_devargs associated with the PCI
> device during its scan, making available other metadatas used afterward
> upon probing and device search for plugging.

Which other metadatas? Please give an example.

[...]
> +void
> +pci_name_set(struct rte_pci_device *dev)
> +{
> +	struct rte_devargs *devargs;
> +
> +	rte_pci_device_name(&dev->addr,
> +			dev->name, sizeof(dev->name));
> +	devargs = pci_devargs_lookup(dev);
> +	dev->device.devargs = devargs;
> +	if (devargs != NULL)
> +		dev->device.name = dev->device.devargs->name;
> +	else
> +		dev->device.name = dev->name;
> +}

I think this function deserves some comments.
The relation between rte_pci_device_name and pci_devargs_lookup
is not obvious.
Moreover, this function is not just setting the name:
it is also setting the devargs in the rte_device.
How dev->device.devargs->name is different from dev->name?

Re: [PATCH v2 3/8] devargs: introduce insert function

[Thomas Monjalon]

11/07/2017 01:19, Gaetan Rivet:
> This new function expects a fully-formed rte_devargs, previously parsed
> and allocated.
> 
> It does not check whether the new rte_devargs is compatible with current
> bus configuration, but will replace any eventual existing one for the same
> device.

Please explain why this function is needed.

Re: [PATCH v2 4/8] eal: fix hotplug add / remove

[Thomas Monjalon]

11/07/2017 01:19, Gaetan Rivet:
> New device should be represented by an rte_devargs prior to being
> plugged.

Why this assumption?
Please start by stated the issue to solve.

> Device parameters are available to rte_devices via their devargs field.
> This field should be set up as soon as possible, as central information
> are stored within, such as the device name which is used to search
> the newly scanned device before plugging it in.
> 
> When a device is introduced using the hotplug API, it is implicitly
> whitelisted. As such, it can conflict with existing bus configuration.
> The new rte_devargs uses the new rte_eal_devargs_insert function that
> supersedes previous rte_devargs, allowing to force the insertion of new
> devices.

I cannot parse this sentence. I probably need to sleep and read it again ;)

[PATCH v3 0/8] fix hotplug API

[Gaetan Rivet]

Sending those fixes as separate patches as they stand on their own.
This series improves usability of the hotplug API and fixes a few issues
with existing implementations.

The hotplug API can be tested with the fail-safe PMD[1]. Its
documentation describes how to declare slaves and how to use it.

[1]: http://dpdk.org/ml/archives/dev/2017-July/070529.html


v2:

  - Add a new rte_devargs function taylored for hotplug operations.
    Upon device hotplug, the device is implicitly whitelisted.
    This configuration should supersede any previous existing bus
    configuration.
  - Merge hotplug add and remove fixes as they are now tied up.
  - Do not return an rte_device handle back from hotplug_add.

v3:

  - Further explain
      - the new rte_eal_devargs_insert function.
      - the hotplug add / remove fix
      - the PCI device name fix
  - Add comments to pci_name_set

Gaetan Rivet (8):
  vdev: implement plug operation
  devargs: introduce removal function
  devargs: introduce insert function
  eal: fix hotplug add / remove
  pci: use given name as generic name
  pci: fix generic driver pointer on probe error
  pci: fix hotplug operations
  bus: remove useless plug parameter

 lib/librte_eal/bsdapp/eal/eal_pci.c             |  4 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  2 +
 lib/librte_eal/common/eal_common_dev.c          | 59 +++++++++++++++++++++++--
 lib/librte_eal/common/eal_common_devargs.c      | 31 +++++++++++++
 lib/librte_eal/common/eal_common_pci.c          | 57 +++++++++++++-----------
 lib/librte_eal/common/eal_common_vdev.c         | 12 ++---
 lib/librte_eal/common/eal_private.h             |  5 +++
 lib/librte_eal/common/include/rte_bus.h         |  6 +--
 lib/librte_eal/common/include/rte_devargs.h     | 31 +++++++++++++
 lib/librte_eal/common/include/rte_vdev.h        |  7 +++
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  4 +-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  2 +
 12 files changed, 177 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH v3 1/8] vdev: implement plug operation

[Gaetan Rivet]

This method must be implemented to allow using a unified, generic API to
hotplug devices, including virtual ones.

VDEV devices actually exist unattached after performing a scan on the
rte_devargs list. As such it makes sense to be able to perform a device
hotplug afterward.

Finally, missing this generic interface forces the EAL to be dependent
on vdev-specific API, which hinders the plan of moving the vdev bus to
drivers/bus.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_vdev.c  | 12 +++++++-----
 lib/librte_eal/common/include/rte_vdev.h |  7 +++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index 2ca0cdb..79bb795 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -316,12 +316,14 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
+vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+{
+	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
+}
+
+static int
 vdev_unplug(struct rte_device *dev)
 {
-	/*
-	 * The virtual bus doesn't support 'unattached' devices so this is
-	 * actually equal to hotplugging removal of it.
-	 */
 	return rte_vdev_uninit(dev->name);
 }
 
@@ -329,7 +331,7 @@ static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
 	.find_device = vdev_find_device,
-	/* .plug = NULL, see comment on vdev_unplug */
+	.plug = vdev_plug,
 	.unplug = vdev_unplug,
 	.parse = vdev_parse,
 };
diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
index 3c07b76..639e6d6 100644
--- a/lib/librte_eal/common/include/rte_vdev.h
+++ b/lib/librte_eal/common/include/rte_vdev.h
@@ -46,6 +46,13 @@ struct rte_vdev_device {
 	struct rte_device device;               /**< Inherit core device */
 };
 
+/**
+ * @internal
+ * Helper macro for drivers that need to convert to struct rte_vdev_device.
+ */
+#define RTE_DEV_TO_VDEV(ptr) \
+	container_of(ptr, struct rte_vdev_device, device)
+
 static inline const char *
 rte_vdev_device_name(const struct rte_vdev_device *dev)
 {
-- 
2.1.4

[PATCH v3 2/8] devargs: introduce removal function

[Gaetan Rivet]

Hotplug support introduces the possibility of removing devices from the
system. Allocated resources must be freed.

Extend the rte_devargs API to allow freeing allocated resources.

This API is experimental and bound to change. It is currently designed
as a symetrical to rte_eal_devargs_add(), but the latter will evolve
shortly anyway.

Its DEVTYPE parameter is currently only used to specify scan policies,
and those will evolve in the next release. This evolution should
rationalize the rte_devargs API.

As such, the proposed API here is not the most convenient, but is
taylored to follow the current design and integrate easily with its main
use within rte_eal_hotplug_* functions.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 39 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895..40cd523 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -207,6 +207,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a3..bcdee13 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -41,6 +41,7 @@
 #include <string.h>
 
 #include <rte_devargs.h>
+#include <rte_tailq.h>
 #include "eal_private.h"
 
 /** Global list of user devices */
@@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	return -1;
 }
 
+int
+rte_eal_devargs_remove(const char *busname, const char *devname)
+{
+	struct rte_devargs *d;
+	void *tmp;
+
+	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
+		if (strcmp(d->bus->name, busname) == 0 &&
+		    strcmp(d->name, devname) == 0) {
+			TAILQ_REMOVE(&devargs_list, d, next);
+			free(d->args);
+			free(d);
+			return 0;
+		}
+	}
+	return 1;
+}
+
 /* count the number of devices of a specified type */
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd..36453b6 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * Remove a device from the user device list.
+ * Its resources are freed.
+ * If the devargs cannot be found, nothing happens.
+ *
+ * @param busname
+ *   bus name of the devargs to remove.
+ *
+ * @param devname
+ *   device name of the devargs to remove.
+ *
+ * @return
+ *   0 on success.
+ *   <0 on error.
+ *   >0 if the devargs was not within the user device list.
+ */
+int rte_eal_devargs_remove(const char *busname, const char *devname);
+
+/**
  * Count the number of user devices of a specified type
  *
  * @param devtype
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009..a8ee349 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -212,6 +212,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
-- 
2.1.4

[PATCH v3 3/8] devargs: introduce insert function

[Gaetan Rivet]

Some buses will operate either in whitelist or blacklist mode.
This mode is currently passed down by the rte_eal_devargs_add function
with the devtype argument.

When inserting devices using the hotplug API, the implicit assumption is
that this device is being whitelisted, meaning that it is explicitly
requested by the application to be used. This can conflict with the
initial bus configuration.

While the rte_eal_devargs_add API is being deprecated soon, it cannot
be modified at the moment to accomodate this situation.
As such, this new experimental API offers a bare interface for inserting
rte_devargs without directly manipulating the global rte_devargs list.

This new function expects a fully-formed rte_devargs, previously parsed
and allocated.

It does not check whether the new rte_devargs is compatible with current
bus configuration, but will replace any eventual existing one for the same
device, allowing the hotplug operation to proceed. i.e. a previously
blacklisted device can be redefined as being whitelisted.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 27 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 40cd523..8b24309 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -206,6 +206,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_devargs_insert;
 	rte_eal_devargs_parse;
 	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index bcdee13..ff6c2a8 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 	return 0;
 }
 
+int
+rte_eal_devargs_insert(struct rte_devargs *da)
+{
+	int ret;
+
+	ret = rte_eal_devargs_remove(da->bus->name, da->name);
+	if (ret < 0)
+		return ret;
+	TAILQ_INSERT_TAIL(&devargs_list, da, next);
+	return 0;
+}
+
 /* store a whitelist parameter for later parsing */
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 36453b6..7b63fa3 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
 		      struct rte_devargs *da);
 
 /**
+ * Insert an rte_devargs in the global list.
+ *
+ * @param da
+ *  The devargs structure to insert.
+ *
+ * @return
+ *   - 0 on success
+ *   - Negative on error.
+ */
+int
+rte_eal_devargs_insert(struct rte_devargs *da);
+
+/**
  * Add a device to the user device list
  *
  * For PCI devices, the format of arguments string is "PCI_ADDR" or
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index a8ee349..81f6af3 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -211,6 +211,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_devargs_insert;
 	rte_eal_devargs_parse;
 	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
-- 
2.1.4

[PATCH v3 4/8] eal: fix hotplug add / remove

[Gaetan Rivet]

The hotplug API requires a few properties that were not previously
explicitly enforced:

  - Idempotency, two consecutive scans should result in the same state.
  - Upon returning, internal devices are now allocated and available
    through the new `find_device` operator, meaning that they should be
    identifiable.

The current rte_eal_hotplug_add implementation identifies devices by
their names, as it is readily available and easy to define.

The device name must be passed to the internal rte_device handle in
order to be available during scan, when it is then assigned to the
device. The current way of passing down this information from the device
declaration is through the global rte_devargs list.

Furthermore, the rte_device cannot take a bus-specific generated name,
as it is then not identifiable by the `find_device` operator. The device
must take the user-defined name. Ideally, an rte_device name should not
change during its existence.

This commit generates a new rte_devargs associated with the plugged
device and inserts it in the global rte_devargs list. It consequently
releases it upon device removal.

Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c | 57 ++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 32e12b5..f5566a6 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -118,11 +118,32 @@ int rte_eal_dev_detach(struct rte_device *dev)
 	return ret;
 }
 
+static char *
+full_dev_name(const char *bus, const char *dev, const char *args)
+{
+	char *name;
+	size_t len;
+
+	len = strlen(bus) + 1 +
+	      strlen(dev) + 1 +
+	      strlen(args) + 1;
+	name = calloc(1, len);
+	if (name == NULL) {
+		RTE_LOG(ERR, EAL, "Could not allocate full device name\n");
+		return NULL;
+	}
+	snprintf(name, len, "%s:%s,%s", bus, dev,
+		 args ? args : "");
+	return name;
+}
+
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *devargs)
 {
 	struct rte_bus *bus;
 	struct rte_device *dev;
+	struct rte_devargs *da;
+	char *name;
 	int ret;
 
 	bus = rte_bus_find_by_name(busname);
@@ -137,21 +158,49 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 		return -ENOTSUP;
 	}
 
+	name = full_dev_name(busname, devname, devargs);
+	if (name == NULL)
+		return -ENOMEM;
+
+	da = calloc(1, sizeof(*da));
+	if (da == NULL) {
+		ret = -ENOMEM;
+		goto err_name;
+	}
+
+	ret = rte_eal_devargs_parse(name, da);
+	if (ret)
+		goto err_devarg;
+
+	ret = rte_eal_devargs_insert(da);
+	if (ret)
+		goto err_devarg;
+
 	ret = bus->scan();
 	if (ret)
-		return ret;
+		goto err_devarg;
 
 	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
-		return -EINVAL;
+		ret = -ENODEV;
+		goto err_devarg;
 	}
 
 	ret = bus->plug(dev, devargs);
-	if (ret)
+	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
+		goto err_devarg;
+	}
+	free(name);
+	return 0;
+
+err_devarg:
+	rte_eal_devargs_remove(busname, devname);
+err_name:
+	free(name);
 	return ret;
 }
 
@@ -179,6 +228,8 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -EINVAL;
 	}
 
+	rte_eal_devargs_remove(busname, devname);
+	dev->devargs = NULL;
 	ret = bus->unplug(dev);
 	if (ret)
 		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
-- 
2.1.4

[PATCH v3 5/8] pci: use given name as generic name

[Gaetan Rivet]

When an application requests the use of a PCI device, it can currently
interchangeably use either the longform DomBDF format (0000:00:00.0) or
the shorter BDF format (00:00.0).

When a device is inserted via the hotplug API, it must first be scanned
and then will be identified by its name using `find_device`. The name of
the device must match the name given by the user to be found and then
probed.

A new function sets the expected name for a scanned PCI device. It was
previously generated from parsing the PCI address. This canonical name
is superseded when an rte_devargs exists describing the device. In such
case, the device takes the given name found within the rte_devargs.

As the rte_devargs is linked to the rte_pci_device during scanning, it
can be avoided during the probe. Additionally, this fixes the issue of
the rte_devargs lookup not being done within rte_pci_probe_one.

Fixes: beec692c5157 ("eal: add name field to generic device")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c    |  4 ++--
 lib/librte_eal/common/eal_common_pci.c | 29 ++++++++++++++++++++++++-----
 lib/librte_eal/common/eal_private.h    |  5 +++++
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  4 ++--
 4 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index e321461..97a88ec 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -282,8 +282,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 	/* FreeBSD has no NUMA support (yet) */
 	dev->device.numa_node = 0;
 
-	rte_pci_device_name(&dev->addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* FreeBSD has only one pass through driver */
 	dev->kdrv = RTE_KDRV_NIC_UIO;
@@ -334,6 +333,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource,
 					dev->mem_resource,
 					sizeof(dev->mem_resource));
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 76bbcc8..45e697e 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -88,6 +88,29 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 	return NULL;
 }
 
+void
+pci_name_set(struct rte_pci_device *dev)
+{
+	struct rte_devargs *devargs;
+
+	/* Each device has its internal, canonical name set. */
+	rte_pci_device_name(&dev->addr,
+			dev->name, sizeof(dev->name));
+	devargs = pci_devargs_lookup(dev);
+	dev->device.devargs = devargs;
+	/* In blacklist mode, if the device is not blacklisted, no
+	 * rte_devargs exists for it.
+	 */
+	if (devargs != NULL)
+		/* If an rte_devargs exists, the generic rte_device uses the
+		 * given name as its namea
+		 */
+		dev->device.name = dev->device.devargs->name;
+	else
+		/* Otherwise, it uses the internal, canonical form. */
+		dev->device.name = dev->name;
+}
+
 /* map a particular resource from a file */
 void *
 pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
@@ -396,11 +419,7 @@ rte_pci_probe(void)
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
-		/* set devargs in PCI structure */
-		devargs = pci_devargs_lookup(dev);
-		if (devargs != NULL)
-			dev->device.devargs = devargs;
-
+		devargs = dev->device.devargs;
 		/* probe all or only whitelisted devices */
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 0836339..597d82e 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -113,6 +113,11 @@ struct rte_pci_driver;
 struct rte_pci_device;
 
 /**
+ * Find the name of a PCI device.
+ */
+void pci_name_set(struct rte_pci_device *dev);
+
+/**
  * Add a PCI device to the PCI Bus (append to PCI Device list). This function
  * also updates the bus references of the PCI Device (and the generic device
  * object embedded within.
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 7d9e1a9..556ae2c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -324,8 +324,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 		dev->device.numa_node = 0;
 	}
 
-	rte_pci_device_name(addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* parse resources */
 	snprintf(filename, sizeof(filename), "%s/resource", dirname);
@@ -373,6 +372,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource, dev->mem_resource,
 					sizeof(dev->mem_resource));
 				free(dev);
-- 
2.1.4

[PATCH v3 6/8] pci: fix generic driver pointer on probe error

[Gaetan Rivet]

The field is set but never resetted on error.
This marks the device as being attached while it is not, and forbid
further attempts to hotplug it.

Fixes: 7917d5f5ea46 ("pci: initialize generic driver pointer")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 45e697e..0a4062c 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -245,6 +245,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		dev->driver = NULL;
+		dev->device.driver = NULL;
 		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
 			/* Don't unmap if device is unsupported and
 			 * driver needs mapped resources.
-- 
2.1.4

[PATCH v3 7/8] pci: fix hotplug operations

[Gaetan Rivet]

The device handle is already known and does not have to be infered from
the PCI address. The relevant helpers are already available within the
PCI bus to avoid searching for a handle already known.

Additionally, rte_memcpy.h was erroneously included.

Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 0a4062c..cdd197a 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -47,7 +47,6 @@
 #include <rte_pci.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
-#include <rte_memcpy.h>
 #include <rte_memzone.h>
 #include <rte_eal.h>
 #include <rte_string_fns.h>
@@ -544,32 +543,20 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 static int
 pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
 {
-	struct rte_pci_device *pdev;
-	struct rte_pci_addr *addr;
-
-	addr = &RTE_DEV_TO_PCI(dev)->addr;
-
-	/* Find the current device holding this address in the bus. */
-	FOREACH_DEVICE_ON_PCIBUS(pdev) {
-		if (rte_eal_compare_pci_addr(&pdev->addr, addr) == 0)
-			return rte_pci_probe_one(addr);
-	}
-
-	rte_errno = ENODEV;
-	return -1;
+	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
 
 static int
 pci_unplug(struct rte_device *dev)
 {
 	struct rte_pci_device *pdev;
+	int ret;
 
 	pdev = RTE_DEV_TO_PCI(dev);
-	if (rte_pci_detach(&pdev->addr) != 0) {
-		rte_errno = ENODEV;
-		return -1;
-	}
-	return 0;
+	ret = rte_pci_detach_dev(pdev);
+	rte_pci_remove_device(pdev);
+	free(pdev);
+	return ret;
 }
 
 struct rte_pci_bus rte_pci_bus = {
-- 
2.1.4

[PATCH v3 8/8] bus: remove useless plug parameter

[Gaetan Rivet]

The prior scan should link the relevant rte_devargs to the newly
allocated rte_device. As such, it is useless to pass device arguments to
the plug callback. Those arguments are available within the devargs
field of the rte_device structure.

Fixes: 7c8810f43f6e ("bus: introduce device plug/unplug")
Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")
Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c  | 2 +-
 lib/librte_eal/common/eal_common_pci.c  | 2 +-
 lib/librte_eal/common/eal_common_vdev.c | 2 +-
 lib/librte_eal/common/include/rte_bus.h | 6 +-----
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index f5566a6..6fcbea4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -188,7 +188,7 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 		goto err_devarg;
 	}
 
-	ret = bus->plug(dev, devargs);
+	ret = bus->plug(dev);
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index cdd197a..9ad1bf1 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -541,7 +541,7 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
+pci_plug(struct rte_device *dev)
 {
 	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index 79bb795..0817573 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -316,7 +316,7 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+vdev_plug(struct rte_device *dev)
 {
 	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
 }
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index af9f0e1..c79368d 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -113,15 +113,11 @@ typedef struct rte_device *
  * @param dev
  *	Device pointer that was returned by a previous call to find_device.
  *
- * @param devargs
- *	Device declaration.
- *
  * @return
  *	0 on success.
  *	!0 on error.
  */
-typedef int (*rte_bus_plug_t)(struct rte_device *dev,
-			      const char *devargs);
+typedef int (*rte_bus_plug_t)(struct rte_device *dev);
 
 /**
  * Implementation specific remove function which is responsible for unlinking
-- 
2.1.4

Re: [PATCH v3 3/8] devargs: introduce insert function

[Jan Blunck]

On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> Some buses will operate either in whitelist or blacklist mode.
> This mode is currently passed down by the rte_eal_devargs_add function
> with the devtype argument.
>
> When inserting devices using the hotplug API, the implicit assumption is
> that this device is being whitelisted, meaning that it is explicitly
> requested by the application to be used. This can conflict with the
> initial bus configuration.

Actually I don't think that this is correct. If I blacklist a device
via devargs I don't want this to be probed in case my udev helper is
calling hotplug add.

Maybe it is better to just update the args field in case the devargs
instance is already found.

>
> While the rte_eal_devargs_add API is being deprecated soon, it cannot
> be modified at the moment to accomodate this situation.
> As such, this new experimental API offers a bare interface for inserting
> rte_devargs without directly manipulating the global rte_devargs list.
>
> This new function expects a fully-formed rte_devargs, previously parsed
> and allocated.
>
> It does not check whether the new rte_devargs is compatible with current
> bus configuration, but will replace any eventual existing one for the same
> device, allowing the hotplug operation to proceed. i.e. a previously
> blacklisted device can be redefined as being whitelisted.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
>  lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  4 files changed, 27 insertions(+)
>
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 40cd523..8b24309 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -206,6 +206,7 @@ DPDK_17.08 {
>  EXPERIMENTAL {
>         global:
>
> +       rte_eal_devargs_insert;
>         rte_eal_devargs_parse;
>         rte_eal_devargs_remove;
>         rte_eal_hotplug_add;
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index bcdee13..ff6c2a8 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>         return 0;
>  }
>
> +int
> +rte_eal_devargs_insert(struct rte_devargs *da)
> +{
> +       int ret;
> +
> +       ret = rte_eal_devargs_remove(da->bus->name, da->name);
> +       if (ret < 0)
> +               return ret;
> +       TAILQ_INSERT_TAIL(&devargs_list, da, next);
> +       return 0;
> +}
> +
>  /* store a whitelist parameter for later parsing */
>  int
>  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index 36453b6..7b63fa3 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
>                       struct rte_devargs *da);
>
>  /**
> + * Insert an rte_devargs in the global list.
> + *
> + * @param da
> + *  The devargs structure to insert.
> + *
> + * @return
> + *   - 0 on success
> + *   - Negative on error.
> + */
> +int
> +rte_eal_devargs_insert(struct rte_devargs *da);
> +
> +/**
>   * Add a device to the user device list
>   *
>   * For PCI devices, the format of arguments string is "PCI_ADDR" or
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index a8ee349..81f6af3 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -211,6 +211,7 @@ DPDK_17.08 {
>  EXPERIMENTAL {
>         global:
>
> +       rte_eal_devargs_insert;
>         rte_eal_devargs_parse;
>         rte_eal_devargs_remove;
>         rte_eal_hotplug_add;
> --
> 2.1.4
>

Re: [PATCH v3 2/8] devargs: introduce removal function

[Jan Blunck]

On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> Hotplug support introduces the possibility of removing devices from the
> system. Allocated resources must be freed.
>
> Extend the rte_devargs API to allow freeing allocated resources.
>
> This API is experimental and bound to change. It is currently designed
> as a symetrical to rte_eal_devargs_add(), but the latter will evolve
> shortly anyway.
>
> Its DEVTYPE parameter is currently only used to specify scan policies,
> and those will evolve in the next release. This evolution should
> rationalize the rte_devargs API.
>
> As such, the proposed API here is not the most convenient, but is
> taylored to follow the current design and integrate easily with its main
> use within rte_eal_hotplug_* functions.

I don't think that this is safe to do. The rte_device is using a
reference to the allocated rte_devargs so before we remove it we need
to make sure that the device is gone.

Besides that the comment says that its current user is inside the EAL
so I wonder why it is exported.

> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
>  lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  4 files changed, 39 insertions(+)
>
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 381f895..40cd523 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -207,6 +207,7 @@ EXPERIMENTAL {
>         global:
>
>         rte_eal_devargs_parse;
> +       rte_eal_devargs_remove;
>         rte_eal_hotplug_add;
>         rte_eal_hotplug_remove;
>
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 49d43a3..bcdee13 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -41,6 +41,7 @@
>  #include <string.h>
>
>  #include <rte_devargs.h>
> +#include <rte_tailq.h>
>  #include "eal_private.h"
>
>  /** Global list of user devices */
> @@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>         return -1;
>  }
>
> +int
> +rte_eal_devargs_remove(const char *busname, const char *devname)
> +{
> +       struct rte_devargs *d;
> +       void *tmp;
> +
> +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> +               if (strcmp(d->bus->name, busname) == 0 &&
> +                   strcmp(d->name, devname) == 0) {
> +                       TAILQ_REMOVE(&devargs_list, d, next);
> +                       free(d->args);
> +                       free(d);
> +                       return 0;
> +               }
> +       }
> +       return 1;
> +}
> +
>  /* count the number of devices of a specified type */
>  unsigned int
>  rte_eal_devargs_type_count(enum rte_devtype devtype)
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index a0427cd..36453b6 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
>  int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
>
>  /**
> + * Remove a device from the user device list.
> + * Its resources are freed.
> + * If the devargs cannot be found, nothing happens.
> + *
> + * @param busname
> + *   bus name of the devargs to remove.
> + *
> + * @param devname
> + *   device name of the devargs to remove.
> + *
> + * @return
> + *   0 on success.
> + *   <0 on error.
> + *   >0 if the devargs was not within the user device list.
> + */
> +int rte_eal_devargs_remove(const char *busname, const char *devname);
> +
> +/**
>   * Count the number of user devices of a specified type
>   *
>   * @param devtype
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 0f9e009..a8ee349 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -212,6 +212,7 @@ EXPERIMENTAL {
>         global:
>
>         rte_eal_devargs_parse;
> +       rte_eal_devargs_remove;
>         rte_eal_hotplug_add;
>         rte_eal_hotplug_remove;
>
> --
> 2.1.4
>

Re: [PATCH v3 4/8] eal: fix hotplug add / remove

[Jan Blunck]

On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> The hotplug API requires a few properties that were not previously
> explicitly enforced:
>
>   - Idempotency, two consecutive scans should result in the same state.
>   - Upon returning, internal devices are now allocated and available
>     through the new `find_device` operator, meaning that they should be
>     identifiable.
>
> The current rte_eal_hotplug_add implementation identifies devices by
> their names, as it is readily available and easy to define.
>
> The device name must be passed to the internal rte_device handle in
> order to be available during scan, when it is then assigned to the
> device. The current way of passing down this information from the device
> declaration is through the global rte_devargs list.

This only true for the virtual bus (vdev).

>
> Furthermore, the rte_device cannot take a bus-specific generated name,
> as it is then not identifiable by the `find_device` operator. The device
> must take the user-defined name. Ideally, an rte_device name should not
> change during its existence.
>
> This commit generates a new rte_devargs associated with the plugged
> device and inserts it in the global rte_devargs list. It consequently
> releases it upon device removal.

So unplugging a device which devargs have been passed via the command
line is implicitly removing the command line parameter? This is a
surprising side-effect and it sound wrong to do. What happens if the
device is getting replugged?

It seems to me that you want to make the hotplug add functionality
behave like a force attach. You can achieve this by calling
rte_eal_devargs_add/parse before calling into hotplug add.


> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/common/eal_common_dev.c | 57 ++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 32e12b5..f5566a6 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -118,11 +118,32 @@ int rte_eal_dev_detach(struct rte_device *dev)
>         return ret;
>  }
>
> +static char *
> +full_dev_name(const char *bus, const char *dev, const char *args)
> +{
> +       char *name;
> +       size_t len;
> +
> +       len = strlen(bus) + 1 +
> +             strlen(dev) + 1 +
> +             strlen(args) + 1;
> +       name = calloc(1, len);
> +       if (name == NULL) {
> +               RTE_LOG(ERR, EAL, "Could not allocate full device name\n");
> +               return NULL;
> +       }
> +       snprintf(name, len, "%s:%s,%s", bus, dev,
> +                args ? args : "");
> +       return name;
> +}
> +
>  int rte_eal_hotplug_add(const char *busname, const char *devname,
>                         const char *devargs)
>  {
>         struct rte_bus *bus;
>         struct rte_device *dev;
> +       struct rte_devargs *da;
> +       char *name;
>         int ret;
>
>         bus = rte_bus_find_by_name(busname);
> @@ -137,21 +158,49 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
>                 return -ENOTSUP;
>         }
>
> +       name = full_dev_name(busname, devname, devargs);
> +       if (name == NULL)
> +               return -ENOMEM;
> +
> +       da = calloc(1, sizeof(*da));
> +       if (da == NULL) {
> +               ret = -ENOMEM;
> +               goto err_name;
> +       }
> +
> +       ret = rte_eal_devargs_parse(name, da);
> +       if (ret)
> +               goto err_devarg;
> +
> +       ret = rte_eal_devargs_insert(da);
> +       if (ret)
> +               goto err_devarg;
> +
>         ret = bus->scan();
>         if (ret)
> -               return ret;
> +               goto err_devarg;
>
>         dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
>         if (dev == NULL) {
>                 RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
>                         devname);
> -               return -EINVAL;
> +               ret = -ENODEV;
> +               goto err_devarg;
>         }
>
>         ret = bus->plug(dev, devargs);
> -       if (ret)
> +       if (ret) {
>                 RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
>                         dev->name);
> +               goto err_devarg;
> +       }
> +       free(name);
> +       return 0;
> +
> +err_devarg:
> +       rte_eal_devargs_remove(busname, devname);
> +err_name:
> +       free(name);
>         return ret;
>  }
>
> @@ -179,6 +228,8 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
>                 return -EINVAL;
>         }
>
> +       rte_eal_devargs_remove(busname, devname);
> +       dev->devargs = NULL;
>         ret = bus->unplug(dev);
>         if (ret)
>                 RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
> --
> 2.1.4
>

Re: [PATCH v3 3/8] devargs: introduce insert function

[Gaëtan Rivet]

On Wed, Jul 12, 2017 at 04:20:48AM -0400, Jan Blunck wrote:
> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > Some buses will operate either in whitelist or blacklist mode.
> > This mode is currently passed down by the rte_eal_devargs_add function
> > with the devtype argument.
> >
> > When inserting devices using the hotplug API, the implicit assumption is
> > that this device is being whitelisted, meaning that it is explicitly
> > requested by the application to be used. This can conflict with the
> > initial bus configuration.
> 
> Actually I don't think that this is correct. If I blacklist a device
> via devargs I don't want this to be probed in case my udev helper is
> calling hotplug add.
> 

This would be good for a udev handler yes. But should we expect this
behavior from all potential hotplug initiators?

To put it another way: should this rule be enforced at the EAL level or
from those using it? And is it impossible to imagine a system that would
actually need to be able to update a device, changing it from
blacklisted to whitelisted on some specific condition?

The fail-safe at least makes use of this ability. This is at the moment
the only hotplug user.

> Maybe it is better to just update the args field in case the devargs
> instance is already found.
> 
> >
> > While the rte_eal_devargs_add API is being deprecated soon, it cannot
> > be modified at the moment to accomodate this situation.
> > As such, this new experimental API offers a bare interface for inserting
> > rte_devargs without directly manipulating the global rte_devargs list.
> >
> > This new function expects a fully-formed rte_devargs, previously parsed
> > and allocated.
> >
> > It does not check whether the new rte_devargs is compatible with current
> > bus configuration, but will replace any eventual existing one for the same
> > device, allowing the hotplug operation to proceed. i.e. a previously
> > blacklisted device can be redefined as being whitelisted.
> >
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> >  lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
> >  lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >  4 files changed, 27 insertions(+)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > index 40cd523..8b24309 100644
> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > @@ -206,6 +206,7 @@ DPDK_17.08 {
> >  EXPERIMENTAL {
> >         global:
> >
> > +       rte_eal_devargs_insert;
> >         rte_eal_devargs_parse;
> >         rte_eal_devargs_remove;
> >         rte_eal_hotplug_add;
> > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> > index bcdee13..ff6c2a8 100644
> > --- a/lib/librte_eal/common/eal_common_devargs.c
> > +++ b/lib/librte_eal/common/eal_common_devargs.c
> > @@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
> >         return 0;
> >  }
> >
> > +int
> > +rte_eal_devargs_insert(struct rte_devargs *da)
> > +{
> > +       int ret;
> > +
> > +       ret = rte_eal_devargs_remove(da->bus->name, da->name);
> > +       if (ret < 0)
> > +               return ret;
> > +       TAILQ_INSERT_TAIL(&devargs_list, da, next);
> > +       return 0;
> > +}
> > +
> >  /* store a whitelist parameter for later parsing */
> >  int
> >  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> > index 36453b6..7b63fa3 100644
> > --- a/lib/librte_eal/common/include/rte_devargs.h
> > +++ b/lib/librte_eal/common/include/rte_devargs.h
> > @@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
> >                       struct rte_devargs *da);
> >
> >  /**
> > + * Insert an rte_devargs in the global list.
> > + *
> > + * @param da
> > + *  The devargs structure to insert.
> > + *
> > + * @return
> > + *   - 0 on success
> > + *   - Negative on error.
> > + */
> > +int
> > +rte_eal_devargs_insert(struct rte_devargs *da);
> > +
> > +/**
> >   * Add a device to the user device list
> >   *
> >   * For PCI devices, the format of arguments string is "PCI_ADDR" or
> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > index a8ee349..81f6af3 100644
> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > @@ -211,6 +211,7 @@ DPDK_17.08 {
> >  EXPERIMENTAL {
> >         global:
> >
> > +       rte_eal_devargs_insert;
> >         rte_eal_devargs_parse;
> >         rte_eal_devargs_remove;
> >         rte_eal_hotplug_add;
> > --
> > 2.1.4
> >

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v3 2/8] devargs: introduce removal function

[Gaëtan Rivet]

On Wed, Jul 12, 2017 at 04:27:34AM -0400, Jan Blunck wrote:
> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > Hotplug support introduces the possibility of removing devices from the
> > system. Allocated resources must be freed.
> >
> > Extend the rte_devargs API to allow freeing allocated resources.
> >
> > This API is experimental and bound to change. It is currently designed
> > as a symetrical to rte_eal_devargs_add(), but the latter will evolve
> > shortly anyway.
> >
> > Its DEVTYPE parameter is currently only used to specify scan policies,
> > and those will evolve in the next release. This evolution should
> > rationalize the rte_devargs API.
> >
> > As such, the proposed API here is not the most convenient, but is
> > taylored to follow the current design and integrate easily with its main
> > use within rte_eal_hotplug_* functions.
> 
> I don't think that this is safe to do. The rte_device is using a
> reference to the allocated rte_devargs so before we remove it we need
> to make sure that the device is gone.
> 

I think you're right. No bus should need to use the rte_devargs to
remove a device, but we cannot be sure.

> Besides that the comment says that its current user is inside the EAL
> so I wonder why it is exported.
> 

I agree that there isn't much use outside. However, the converse
rte_eal_devargs_add is exposed so it makes sense to also offer the
possibility of removing one.

But it will all change with the deprecation of rte_eal_devargs_add
anyway, so let's not expose it if there is no need.

> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> >  lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
> >  lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >  4 files changed, 39 insertions(+)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > index 381f895..40cd523 100644
> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > @@ -207,6 +207,7 @@ EXPERIMENTAL {
> >         global:
> >
> >         rte_eal_devargs_parse;
> > +       rte_eal_devargs_remove;
> >         rte_eal_hotplug_add;
> >         rte_eal_hotplug_remove;
> >
> > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> > index 49d43a3..bcdee13 100644
> > --- a/lib/librte_eal/common/eal_common_devargs.c
> > +++ b/lib/librte_eal/common/eal_common_devargs.c
> > @@ -41,6 +41,7 @@
> >  #include <string.h>
> >
> >  #include <rte_devargs.h>
> > +#include <rte_tailq.h>
> >  #include "eal_private.h"
> >
> >  /** Global list of user devices */
> > @@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> >         return -1;
> >  }
> >
> > +int
> > +rte_eal_devargs_remove(const char *busname, const char *devname)
> > +{
> > +       struct rte_devargs *d;
> > +       void *tmp;
> > +
> > +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> > +               if (strcmp(d->bus->name, busname) == 0 &&
> > +                   strcmp(d->name, devname) == 0) {
> > +                       TAILQ_REMOVE(&devargs_list, d, next);
> > +                       free(d->args);
> > +                       free(d);
> > +                       return 0;
> > +               }
> > +       }
> > +       return 1;
> > +}
> > +
> >  /* count the number of devices of a specified type */
> >  unsigned int
> >  rte_eal_devargs_type_count(enum rte_devtype devtype)
> > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> > index a0427cd..36453b6 100644
> > --- a/lib/librte_eal/common/include/rte_devargs.h
> > +++ b/lib/librte_eal/common/include/rte_devargs.h
> > @@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
> >  int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
> >
> >  /**
> > + * Remove a device from the user device list.
> > + * Its resources are freed.
> > + * If the devargs cannot be found, nothing happens.
> > + *
> > + * @param busname
> > + *   bus name of the devargs to remove.
> > + *
> > + * @param devname
> > + *   device name of the devargs to remove.
> > + *
> > + * @return
> > + *   0 on success.
> > + *   <0 on error.
> > + *   >0 if the devargs was not within the user device list.
> > + */
> > +int rte_eal_devargs_remove(const char *busname, const char *devname);
> > +
> > +/**
> >   * Count the number of user devices of a specified type
> >   *
> >   * @param devtype
> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > index 0f9e009..a8ee349 100644
> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > @@ -212,6 +212,7 @@ EXPERIMENTAL {
> >         global:
> >
> >         rte_eal_devargs_parse;
> > +       rte_eal_devargs_remove;
> >         rte_eal_hotplug_add;
> >         rte_eal_hotplug_remove;
> >
> > --
> > 2.1.4
> >

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v3 4/8] eal: fix hotplug add / remove

[Gaëtan Rivet]

On Wed, Jul 12, 2017 at 04:44:28AM -0400, Jan Blunck wrote:
> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > The hotplug API requires a few properties that were not previously
> > explicitly enforced:
> >
> >   - Idempotency, two consecutive scans should result in the same state.
> >   - Upon returning, internal devices are now allocated and available
> >     through the new `find_device` operator, meaning that they should be
> >     identifiable.
> >
> > The current rte_eal_hotplug_add implementation identifies devices by
> > their names, as it is readily available and easy to define.
> >
> > The device name must be passed to the internal rte_device handle in
> > order to be available during scan, when it is then assigned to the
> > device. The current way of passing down this information from the device
> > declaration is through the global rte_devargs list.
> 
> This only true for the virtual bus (vdev).
> 

The way the hotplug API is designed at the moment, the devname passed to
it must match the name of the scanned device. Without this match, the
hotplug function cannot work, so while this was only true for the vdev
bus, this becomes a requirement to support hotplug.

The alternative is to be able to transform a "user-centric" name to a
canonical name by a bus, which would then allow to search for the
canonical name to find the newly scanned device.

> >
> > Furthermore, the rte_device cannot take a bus-specific generated name,
> > as it is then not identifiable by the `find_device` operator. The device
> > must take the user-defined name. Ideally, an rte_device name should not
> > change during its existence.
> >
> > This commit generates a new rte_devargs associated with the plugged
> > device and inserts it in the global rte_devargs list. It consequently
> > releases it upon device removal.
> 
> So unplugging a device which devargs have been passed via the command
> line is implicitly removing the command line parameter? This is a
> surprising side-effect and it sound wrong to do. What happens if the
> device is getting replugged?
> 

If the device is replugged, then the only way to use is to call
hotplug_add, meaning that an rte_devargs is recreated with the devname
and devargs strings.

There is no way for an application currently to be sure that the new
device is actually the same as the one that was previously removed. It
may share the same PCI address, but nothing tells us that this is
actually the same one and that it can take the same arguments. Managing
this discrepancy means some involvement anyway, memoizing the previous
devargs if necessary is nothing in comparison.

> It seems to me that you want to make the hotplug add functionality
> behave like a force attach. You can achieve this by calling
> rte_eal_devargs_add/parse before calling into hotplug add.
> 
> 

What do you mean by force attach? Sorry if it's obvious.
To me, the way the hotplug is designed, every user would need to use
rte_eal_devargs_add/parse beforehand, so it makes sense to put it within
the function and spare the code lines.

> > Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> >
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  lib/librte_eal/common/eal_common_dev.c | 57 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 54 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> > index 32e12b5..f5566a6 100644
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -118,11 +118,32 @@ int rte_eal_dev_detach(struct rte_device *dev)
> >         return ret;
> >  }
> >
> > +static char *
> > +full_dev_name(const char *bus, const char *dev, const char *args)
> > +{
> > +       char *name;
> > +       size_t len;
> > +
> > +       len = strlen(bus) + 1 +
> > +             strlen(dev) + 1 +
> > +             strlen(args) + 1;
> > +       name = calloc(1, len);
> > +       if (name == NULL) {
> > +               RTE_LOG(ERR, EAL, "Could not allocate full device name\n");
> > +               return NULL;
> > +       }
> > +       snprintf(name, len, "%s:%s,%s", bus, dev,
> > +                args ? args : "");
> > +       return name;
> > +}
> > +
> >  int rte_eal_hotplug_add(const char *busname, const char *devname,
> >                         const char *devargs)
> >  {
> >         struct rte_bus *bus;
> >         struct rte_device *dev;
> > +       struct rte_devargs *da;
> > +       char *name;
> >         int ret;
> >
> >         bus = rte_bus_find_by_name(busname);
> > @@ -137,21 +158,49 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
> >                 return -ENOTSUP;
> >         }
> >
> > +       name = full_dev_name(busname, devname, devargs);
> > +       if (name == NULL)
> > +               return -ENOMEM;
> > +
> > +       da = calloc(1, sizeof(*da));
> > +       if (da == NULL) {
> > +               ret = -ENOMEM;
> > +               goto err_name;
> > +       }
> > +
> > +       ret = rte_eal_devargs_parse(name, da);
> > +       if (ret)
> > +               goto err_devarg;
> > +
> > +       ret = rte_eal_devargs_insert(da);
> > +       if (ret)
> > +               goto err_devarg;
> > +
> >         ret = bus->scan();
> >         if (ret)
> > -               return ret;
> > +               goto err_devarg;
> >
> >         dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
> >         if (dev == NULL) {
> >                 RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
> >                         devname);
> > -               return -EINVAL;
> > +               ret = -ENODEV;
> > +               goto err_devarg;
> >         }
> >
> >         ret = bus->plug(dev, devargs);
> > -       if (ret)
> > +       if (ret) {
> >                 RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
> >                         dev->name);
> > +               goto err_devarg;
> > +       }
> > +       free(name);
> > +       return 0;
> > +
> > +err_devarg:
> > +       rte_eal_devargs_remove(busname, devname);
> > +err_name:
> > +       free(name);
> >         return ret;
> >  }
> >
> > @@ -179,6 +228,8 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
> >                 return -EINVAL;
> >         }
> >
> > +       rte_eal_devargs_remove(busname, devname);
> > +       dev->devargs = NULL;
> >         ret = bus->unplug(dev);
> >         if (ret)
> >                 RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
> > --
> > 2.1.4
> >

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v3 3/8] devargs: introduce insert function

[Jan Blunck]

On Wed, Jul 12, 2017 at 1:20 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> On Wed, Jul 12, 2017 at 04:20:48AM -0400, Jan Blunck wrote:
>> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> > Some buses will operate either in whitelist or blacklist mode.
>> > This mode is currently passed down by the rte_eal_devargs_add function
>> > with the devtype argument.
>> >
>> > When inserting devices using the hotplug API, the implicit assumption is
>> > that this device is being whitelisted, meaning that it is explicitly
>> > requested by the application to be used. This can conflict with the
>> > initial bus configuration.
>>
>> Actually I don't think that this is correct. If I blacklist a device
>> via devargs I don't want this to be probed in case my udev helper is
>> calling hotplug add.
>>
>
> This would be good for a udev handler yes. But should we expect this
> behavior from all potential hotplug initiators?
>
> To put it another way: should this rule be enforced at the EAL level or
> from those using it? And is it impossible to imagine a system that would
> actually need to be able to update a device, changing it from
> blacklisted to whitelisted on some specific condition?
>
> The fail-safe at least makes use of this ability. This is at the moment
> the only hotplug user.
>

In tree .... I have hotplug functionality available for quite some time already.


>> Maybe it is better to just update the args field in case the devargs
>> instance is already found.
>>
>> >
>> > While the rte_eal_devargs_add API is being deprecated soon, it cannot
>> > be modified at the moment to accomodate this situation.
>> > As such, this new experimental API offers a bare interface for inserting
>> > rte_devargs without directly manipulating the global rte_devargs list.
>> >
>> > This new function expects a fully-formed rte_devargs, previously parsed
>> > and allocated.
>> >
>> > It does not check whether the new rte_devargs is compatible with current
>> > bus configuration, but will replace any eventual existing one for the same
>> > device, allowing the hotplug operation to proceed. i.e. a previously
>> > blacklisted device can be redefined as being whitelisted.
>> >
>> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> > ---
>> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>> >  lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
>> >  lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
>> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>> >  4 files changed, 27 insertions(+)
>> >
>> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > index 40cd523..8b24309 100644
>> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > @@ -206,6 +206,7 @@ DPDK_17.08 {
>> >  EXPERIMENTAL {
>> >         global:
>> >
>> > +       rte_eal_devargs_insert;
>> >         rte_eal_devargs_parse;
>> >         rte_eal_devargs_remove;
>> >         rte_eal_hotplug_add;
>> > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
>> > index bcdee13..ff6c2a8 100644
>> > --- a/lib/librte_eal/common/eal_common_devargs.c
>> > +++ b/lib/librte_eal/common/eal_common_devargs.c
>> > @@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>> >         return 0;
>> >  }
>> >
>> > +int
>> > +rte_eal_devargs_insert(struct rte_devargs *da)
>> > +{
>> > +       int ret;
>> > +
>> > +       ret = rte_eal_devargs_remove(da->bus->name, da->name);
>> > +       if (ret < 0)
>> > +               return ret;
>> > +       TAILQ_INSERT_TAIL(&devargs_list, da, next);
>> > +       return 0;
>> > +}
>> > +
>> >  /* store a whitelist parameter for later parsing */
>> >  int
>> >  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>> > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
>> > index 36453b6..7b63fa3 100644
>> > --- a/lib/librte_eal/common/include/rte_devargs.h
>> > +++ b/lib/librte_eal/common/include/rte_devargs.h
>> > @@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
>> >                       struct rte_devargs *da);
>> >
>> >  /**
>> > + * Insert an rte_devargs in the global list.
>> > + *
>> > + * @param da
>> > + *  The devargs structure to insert.
>> > + *
>> > + * @return
>> > + *   - 0 on success
>> > + *   - Negative on error.
>> > + */
>> > +int
>> > +rte_eal_devargs_insert(struct rte_devargs *da);
>> > +
>> > +/**
>> >   * Add a device to the user device list
>> >   *
>> >   * For PCI devices, the format of arguments string is "PCI_ADDR" or
>> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > index a8ee349..81f6af3 100644
>> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > @@ -211,6 +211,7 @@ DPDK_17.08 {
>> >  EXPERIMENTAL {
>> >         global:
>> >
>> > +       rte_eal_devargs_insert;
>> >         rte_eal_devargs_parse;
>> >         rte_eal_devargs_remove;
>> >         rte_eal_hotplug_add;
>> > --
>> > 2.1.4
>> >
>
> --
> Gaėtan Rivet
> 6WIND

[PATCH v4 0/8] fix hotplug API

[Gaetan Rivet]

Sending those fixes as separate patches as they stand on their own.
This series improves usability of the hotplug API and fixes a few issues
with existing implementations.

The hotplug API can be tested with the fail-safe PMD[1]. Its
documentation describes how to declare slaves and how to use it.

[1]: http://dpdk.org/ml/archives/dev/2017-July/070529.html


v2:

  - Add a new rte_devargs function taylored for hotplug operations.
    Upon device hotplug, the device is implicitly whitelisted.
    This configuration should supersede any previous existing bus
    configuration.
  - Merge hotplug add and remove fixes as they are now tied up.
  - Do not return an rte_device handle back from hotplug_add.

v3:

  - Further explain
      - the new rte_eal_devargs_insert function.
      - the hotplug add / remove fix
      - the PCI device name fix
  - Add comments to pci_name_set

v4:

  - Free rte_devargs after the device is being removed,
    to avoid risks of buses relying on the rte_devargs during the
    process.

Gaetan Rivet (8):
  vdev: implement plug operation
  devargs: introduce removal function
  devargs: introduce insert function
  eal: fix hotplug add / remove
  pci: use given name as generic name
  pci: fix generic driver pointer on probe error
  pci: fix hotplug operations
  bus: remove useless plug parameter

 lib/librte_eal/bsdapp/eal/eal_pci.c             |  4 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  2 +
 lib/librte_eal/common/eal_common_dev.c          | 58 +++++++++++++++++++++++--
 lib/librte_eal/common/eal_common_devargs.c      | 31 +++++++++++++
 lib/librte_eal/common/eal_common_pci.c          | 57 +++++++++++++-----------
 lib/librte_eal/common/eal_common_vdev.c         | 12 ++---
 lib/librte_eal/common/eal_private.h             |  5 +++
 lib/librte_eal/common/include/rte_bus.h         |  6 +--
 lib/librte_eal/common/include/rte_devargs.h     | 31 +++++++++++++
 lib/librte_eal/common/include/rte_vdev.h        |  7 +++
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  4 +-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  2 +
 12 files changed, 176 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH v4 1/8] vdev: implement plug operation

[Gaetan Rivet]

This method must be implemented to allow using a unified, generic API to
hotplug devices, including virtual ones.

VDEV devices actually exist unattached after performing a scan on the
rte_devargs list. As such it makes sense to be able to perform a device
hotplug afterward.

Finally, missing this generic interface forces the EAL to be dependent
on vdev-specific API, which hinders the plan of moving the vdev bus to
drivers/bus.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_vdev.c  | 12 +++++++-----
 lib/librte_eal/common/include/rte_vdev.h |  7 +++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index e00dda9..85cf3fc 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -319,12 +319,14 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
+vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+{
+	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
+}
+
+static int
 vdev_unplug(struct rte_device *dev)
 {
-	/*
-	 * The virtual bus doesn't support 'unattached' devices so this is
-	 * actually equal to hotplugging removal of it.
-	 */
 	return rte_vdev_uninit(dev->name);
 }
 
@@ -332,7 +334,7 @@ static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
 	.find_device = vdev_find_device,
-	/* .plug = NULL, see comment on vdev_unplug */
+	.plug = vdev_plug,
 	.unplug = vdev_unplug,
 	.parse = vdev_parse,
 };
diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
index fc24298..29f5a52 100644
--- a/lib/librte_eal/common/include/rte_vdev.h
+++ b/lib/librte_eal/common/include/rte_vdev.h
@@ -46,6 +46,13 @@ struct rte_vdev_device {
 	struct rte_device device;               /**< Inherit core device */
 };
 
+/**
+ * @internal
+ * Helper macro for drivers that need to convert to struct rte_vdev_device.
+ */
+#define RTE_DEV_TO_VDEV(ptr) \
+	container_of(ptr, struct rte_vdev_device, device)
+
 static inline const char *
 rte_vdev_device_name(const struct rte_vdev_device *dev)
 {
-- 
2.1.4

[PATCH v4 2/8] devargs: introduce removal function

[Gaetan Rivet]

Hotplug support introduces the possibility of removing devices from the
system. Allocated resources must be freed.

Extend the rte_devargs API to allow freeing allocated resources.

This API is experimental and bound to change. It is currently designed
as a symetrical to rte_eal_devargs_add(), but the latter will evolve
shortly anyway.

Its DEVTYPE parameter is currently only used to specify scan policies,
and those will evolve in the next release. This evolution should
rationalize the rte_devargs API.

As such, the proposed API here is not the most convenient, but is
taylored to follow the current design and integrate easily with its main
use within rte_eal_hotplug_* functions.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 39 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895..40cd523 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -207,6 +207,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a3..bcdee13 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -41,6 +41,7 @@
 #include <string.h>
 
 #include <rte_devargs.h>
+#include <rte_tailq.h>
 #include "eal_private.h"
 
 /** Global list of user devices */
@@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	return -1;
 }
 
+int
+rte_eal_devargs_remove(const char *busname, const char *devname)
+{
+	struct rte_devargs *d;
+	void *tmp;
+
+	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
+		if (strcmp(d->bus->name, busname) == 0 &&
+		    strcmp(d->name, devname) == 0) {
+			TAILQ_REMOVE(&devargs_list, d, next);
+			free(d->args);
+			free(d);
+			return 0;
+		}
+	}
+	return 1;
+}
+
 /* count the number of devices of a specified type */
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd..36453b6 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * Remove a device from the user device list.
+ * Its resources are freed.
+ * If the devargs cannot be found, nothing happens.
+ *
+ * @param busname
+ *   bus name of the devargs to remove.
+ *
+ * @param devname
+ *   device name of the devargs to remove.
+ *
+ * @return
+ *   0 on success.
+ *   <0 on error.
+ *   >0 if the devargs was not within the user device list.
+ */
+int rte_eal_devargs_remove(const char *busname, const char *devname);
+
+/**
  * Count the number of user devices of a specified type
  *
  * @param devtype
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009..a8ee349 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -212,6 +212,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
-- 
2.1.4

[PATCH v4 3/8] devargs: introduce insert function

[Gaetan Rivet]

Some buses will operate either in whitelist or blacklist mode.
This mode is currently passed down by the rte_eal_devargs_add function
with the devtype argument.

When inserting devices using the hotplug API, the implicit assumption is
that this device is being whitelisted, meaning that it is explicitly
requested by the application to be used. This can conflict with the
initial bus configuration.

While the rte_eal_devargs_add API is being deprecated soon, it cannot
be modified at the moment to accomodate this situation.
As such, this new experimental API offers a bare interface for inserting
rte_devargs without directly manipulating the global rte_devargs list.

This new function expects a fully-formed rte_devargs, previously parsed
and allocated.

It does not check whether the new rte_devargs is compatible with current
bus configuration, but will replace any eventual existing one for the same
device, allowing the hotplug operation to proceed. i.e. a previously
blacklisted device can be redefined as being whitelisted.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 27 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 40cd523..8b24309 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -206,6 +206,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_devargs_insert;
 	rte_eal_devargs_parse;
 	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index bcdee13..ff6c2a8 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 	return 0;
 }
 
+int
+rte_eal_devargs_insert(struct rte_devargs *da)
+{
+	int ret;
+
+	ret = rte_eal_devargs_remove(da->bus->name, da->name);
+	if (ret < 0)
+		return ret;
+	TAILQ_INSERT_TAIL(&devargs_list, da, next);
+	return 0;
+}
+
 /* store a whitelist parameter for later parsing */
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 36453b6..7b63fa3 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
 		      struct rte_devargs *da);
 
 /**
+ * Insert an rte_devargs in the global list.
+ *
+ * @param da
+ *  The devargs structure to insert.
+ *
+ * @return
+ *   - 0 on success
+ *   - Negative on error.
+ */
+int
+rte_eal_devargs_insert(struct rte_devargs *da);
+
+/**
  * Add a device to the user device list
  *
  * For PCI devices, the format of arguments string is "PCI_ADDR" or
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index a8ee349..81f6af3 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -211,6 +211,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_devargs_insert;
 	rte_eal_devargs_parse;
 	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
-- 
2.1.4

[PATCH v4 4/8] eal: fix hotplug add / remove

[Gaetan Rivet]

The hotplug API requires a few properties that were not previously
explicitly enforced:

  - Idempotency, two consecutive scans should result in the same state.
  - Upon returning, internal devices are now allocated and available
    through the new `find_device` operator, meaning that they should be
    identifiable.

The current rte_eal_hotplug_add implementation identifies devices by
their names, as it is readily available and easy to define.

The device name must be passed to the internal rte_device handle in
order to be available during scan, when it is then assigned to the
device. The current way of passing down this information from the device
declaration is through the global rte_devargs list.

Furthermore, the rte_device cannot take a bus-specific generated name,
as it is then not identifiable by the `find_device` operator. The device
must take the user-defined name. Ideally, an rte_device name should not
change during its existence.

This commit generates a new rte_devargs associated with the plugged
device and inserts it in the global rte_devargs list. It consequently
releases it upon device removal.

Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c | 56 ++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 32e12b5..2540fb3 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -118,11 +118,32 @@ int rte_eal_dev_detach(struct rte_device *dev)
 	return ret;
 }
 
+static char *
+full_dev_name(const char *bus, const char *dev, const char *args)
+{
+	char *name;
+	size_t len;
+
+	len = strlen(bus) + 1 +
+	      strlen(dev) + 1 +
+	      strlen(args) + 1;
+	name = calloc(1, len);
+	if (name == NULL) {
+		RTE_LOG(ERR, EAL, "Could not allocate full device name\n");
+		return NULL;
+	}
+	snprintf(name, len, "%s:%s,%s", bus, dev,
+		 args ? args : "");
+	return name;
+}
+
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *devargs)
 {
 	struct rte_bus *bus;
 	struct rte_device *dev;
+	struct rte_devargs *da;
+	char *name;
 	int ret;
 
 	bus = rte_bus_find_by_name(busname);
@@ -137,21 +158,49 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 		return -ENOTSUP;
 	}
 
+	name = full_dev_name(busname, devname, devargs);
+	if (name == NULL)
+		return -ENOMEM;
+
+	da = calloc(1, sizeof(*da));
+	if (da == NULL) {
+		ret = -ENOMEM;
+		goto err_name;
+	}
+
+	ret = rte_eal_devargs_parse(name, da);
+	if (ret)
+		goto err_devarg;
+
+	ret = rte_eal_devargs_insert(da);
+	if (ret)
+		goto err_devarg;
+
 	ret = bus->scan();
 	if (ret)
-		return ret;
+		goto err_devarg;
 
 	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
-		return -EINVAL;
+		ret = -ENODEV;
+		goto err_devarg;
 	}
 
 	ret = bus->plug(dev, devargs);
-	if (ret)
+	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
+		goto err_devarg;
+	}
+	free(name);
+	return 0;
+
+err_devarg:
+	rte_eal_devargs_remove(busname, devname);
+err_name:
+	free(name);
 	return ret;
 }
 
@@ -183,5 +232,6 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname)
 	if (ret)
 		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
 			dev->name);
+	rte_eal_devargs_remove(busname, devname);
 	return ret;
 }
-- 
2.1.4

[PATCH v4 5/8] pci: use given name as generic name

[Gaetan Rivet]

When an application requests the use of a PCI device, it can currently
interchangeably use either the longform DomBDF format (0000:00:00.0) or
the shorter BDF format (00:00.0).

When a device is inserted via the hotplug API, it must first be scanned
and then will be identified by its name using `find_device`. The name of
the device must match the name given by the user to be found and then
probed.

A new function sets the expected name for a scanned PCI device. It was
previously generated from parsing the PCI address. This canonical name
is superseded when an rte_devargs exists describing the device. In such
case, the device takes the given name found within the rte_devargs.

As the rte_devargs is linked to the rte_pci_device during scanning, it
can be avoided during the probe. Additionally, this fixes the issue of
the rte_devargs lookup not being done within rte_pci_probe_one.

Fixes: beec692c5157 ("eal: add name field to generic device")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/eal_pci.c    |  4 ++--
 lib/librte_eal/common/eal_common_pci.c | 29 ++++++++++++++++++++++++-----
 lib/librte_eal/common/eal_private.h    |  5 +++++
 lib/librte_eal/linuxapp/eal/eal_pci.c  |  4 ++--
 4 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index e321461..97a88ec 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -282,8 +282,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 	/* FreeBSD has no NUMA support (yet) */
 	dev->device.numa_node = 0;
 
-	rte_pci_device_name(&dev->addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* FreeBSD has only one pass through driver */
 	dev->kdrv = RTE_KDRV_NIC_UIO;
@@ -334,6 +333,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource,
 					dev->mem_resource,
 					sizeof(dev->mem_resource));
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 76bbcc8..45e697e 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -88,6 +88,29 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 	return NULL;
 }
 
+void
+pci_name_set(struct rte_pci_device *dev)
+{
+	struct rte_devargs *devargs;
+
+	/* Each device has its internal, canonical name set. */
+	rte_pci_device_name(&dev->addr,
+			dev->name, sizeof(dev->name));
+	devargs = pci_devargs_lookup(dev);
+	dev->device.devargs = devargs;
+	/* In blacklist mode, if the device is not blacklisted, no
+	 * rte_devargs exists for it.
+	 */
+	if (devargs != NULL)
+		/* If an rte_devargs exists, the generic rte_device uses the
+		 * given name as its namea
+		 */
+		dev->device.name = dev->device.devargs->name;
+	else
+		/* Otherwise, it uses the internal, canonical form. */
+		dev->device.name = dev->name;
+}
+
 /* map a particular resource from a file */
 void *
 pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
@@ -396,11 +419,7 @@ rte_pci_probe(void)
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
-		/* set devargs in PCI structure */
-		devargs = pci_devargs_lookup(dev);
-		if (devargs != NULL)
-			dev->device.devargs = devargs;
-
+		devargs = dev->device.devargs;
 		/* probe all or only whitelisted devices */
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 0836339..597d82e 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -113,6 +113,11 @@ struct rte_pci_driver;
 struct rte_pci_device;
 
 /**
+ * Find the name of a PCI device.
+ */
+void pci_name_set(struct rte_pci_device *dev);
+
+/**
  * Add a PCI device to the PCI Bus (append to PCI Device list). This function
  * also updates the bus references of the PCI Device (and the generic device
  * object embedded within.
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 7d9e1a9..556ae2c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -324,8 +324,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 		dev->device.numa_node = 0;
 	}
 
-	rte_pci_device_name(addr, dev->name, sizeof(dev->name));
-	dev->device.name = dev->name;
+	pci_name_set(dev);
 
 	/* parse resources */
 	snprintf(filename, sizeof(filename), "%s/resource", dirname);
@@ -373,6 +372,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
+				pci_name_set(dev2);
 				memmove(dev2->mem_resource, dev->mem_resource,
 					sizeof(dev->mem_resource));
 				free(dev);
-- 
2.1.4

[PATCH v4 6/8] pci: fix generic driver pointer on probe error

[Gaetan Rivet]

The field is set but never resetted on error.
This marks the device as being attached while it is not, and forbid
further attempts to hotplug it.

Fixes: 7917d5f5ea46 ("pci: initialize generic driver pointer")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 45e697e..0a4062c 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -245,6 +245,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		dev->driver = NULL;
+		dev->device.driver = NULL;
 		if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
 			/* Don't unmap if device is unsupported and
 			 * driver needs mapped resources.
-- 
2.1.4

[PATCH v4 7/8] pci: fix hotplug operations

[Gaetan Rivet]

The device handle is already known and does not have to be infered from
the PCI address. The relevant helpers are already available within the
PCI bus to avoid searching for a handle already known.

Additionally, rte_memcpy.h was erroneously included.

Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 0a4062c..cdd197a 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -47,7 +47,6 @@
 #include <rte_pci.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
-#include <rte_memcpy.h>
 #include <rte_memzone.h>
 #include <rte_eal.h>
 #include <rte_string_fns.h>
@@ -544,32 +543,20 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 static int
 pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
 {
-	struct rte_pci_device *pdev;
-	struct rte_pci_addr *addr;
-
-	addr = &RTE_DEV_TO_PCI(dev)->addr;
-
-	/* Find the current device holding this address in the bus. */
-	FOREACH_DEVICE_ON_PCIBUS(pdev) {
-		if (rte_eal_compare_pci_addr(&pdev->addr, addr) == 0)
-			return rte_pci_probe_one(addr);
-	}
-
-	rte_errno = ENODEV;
-	return -1;
+	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
 
 static int
 pci_unplug(struct rte_device *dev)
 {
 	struct rte_pci_device *pdev;
+	int ret;
 
 	pdev = RTE_DEV_TO_PCI(dev);
-	if (rte_pci_detach(&pdev->addr) != 0) {
-		rte_errno = ENODEV;
-		return -1;
-	}
-	return 0;
+	ret = rte_pci_detach_dev(pdev);
+	rte_pci_remove_device(pdev);
+	free(pdev);
+	return ret;
 }
 
 struct rte_pci_bus rte_pci_bus = {
-- 
2.1.4

[PATCH v4 8/8] bus: remove useless plug parameter

[Gaetan Rivet]

The prior scan should link the relevant rte_devargs to the newly
allocated rte_device. As such, it is useless to pass device arguments to
the plug callback. Those arguments are available within the devargs
field of the rte_device structure.

Fixes: 7c8810f43f6e ("bus: introduce device plug/unplug")
Fixes: 00e62aae69c0 ("bus/pci: implement plug/unplug operations")
Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_dev.c  | 2 +-
 lib/librte_eal/common/eal_common_pci.c  | 2 +-
 lib/librte_eal/common/eal_common_vdev.c | 2 +-
 lib/librte_eal/common/include/rte_bus.h | 6 +-----
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 2540fb3..2579159 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -188,7 +188,7 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
 		goto err_devarg;
 	}
 
-	ret = bus->plug(dev, devargs);
+	ret = bus->plug(dev);
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
 			dev->name);
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index cdd197a..9ad1bf1 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -541,7 +541,7 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-pci_plug(struct rte_device *dev, const char *devargs __rte_unused)
+pci_plug(struct rte_device *dev)
 {
 	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
 }
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index 85cf3fc..f7e547a 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -319,7 +319,7 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 }
 
 static int
-vdev_plug(struct rte_device *dev, const char *args __rte_unused)
+vdev_plug(struct rte_device *dev)
 {
 	return vdev_probe_all_drivers(RTE_DEV_TO_VDEV(dev));
 }
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index af9f0e1..c79368d 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -113,15 +113,11 @@ typedef struct rte_device *
  * @param dev
  *	Device pointer that was returned by a previous call to find_device.
  *
- * @param devargs
- *	Device declaration.
- *
  * @return
  *	0 on success.
  *	!0 on error.
  */
-typedef int (*rte_bus_plug_t)(struct rte_device *dev,
-			      const char *devargs);
+typedef int (*rte_bus_plug_t)(struct rte_device *dev);
 
 /**
  * Implementation specific remove function which is responsible for unlinking
-- 
2.1.4

Re: [PATCH v4 0/8] fix hotplug API

[Thomas Monjalon]

15/07/2017 20:56, Gaetan Rivet:
> Sending those fixes as separate patches as they stand on their own.
> This series improves usability of the hotplug API and fixes a few issues
> with existing implementations.
> 
> The hotplug API can be tested with the fail-safe PMD[1]. Its
> documentation describes how to declare slaves and how to use it.
[...]
> Gaetan Rivet (8):
>   vdev: implement plug operation
>   devargs: introduce removal function
>   devargs: introduce insert function
>   eal: fix hotplug add / remove
>   pci: use given name as generic name
>   pci: fix generic driver pointer on probe error
>   pci: fix hotplug operations
>   bus: remove useless plug parameter

Applied to fix the current bus state, thanks.
It is now clear that it can be done differently in 17.11 after
the deprecation of the device specification syntax.
Jan already sent some patches in this direction.