bus: attach / detach API

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 20 ++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 43 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 65 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index 2e48a73..ed09ab2 100644
@@ -162,6 +162,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +20 -0

index 8f9baf8..4619eb2 100644
@@ -145,3 +145,23 @@ rte_bus_dump(FILE *f)
 		}
 	}
 }
+
+struct rte_bus *
+rte_bus_find(rte_bus_cmp_t cmp,
+	     const void *data,
+	     const struct rte_bus *start)
+{
+	struct rte_bus *bus = NULL;
+	int started = start == NULL;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!started) {
+			if (bus == start)
+				started = 1;
+			continue;
+		}
+		if (cmp(bus, data) == 0)
+			break;
+	}
+	return bus;
+}

lib/librte_eal/common/include/rte_bus.h +43 -0

49 lines not on this page · raw message

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 670bab3..6efa517 100644
@@ -166,6 +166,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 1/12

2017-06-26 15:31 UTC #2 lore raw

Re: [PATCH v5 01/12] bus: add bus iterator to find a bus

Quoted 5 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>
This helper allows to iterate over all registered buses and find one matching data used as parameter.

Quoted 9 lines

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 20 ++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 43 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 65 insertions(+)

Two minor suggestions below. Otherwise:

Quoted 18 lines

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index 2e48a73..ed09ab2 100644
@@ -162,6 +162,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +0 -0

index 8f9baf8..4619eb2 100644
@@ -145,3 +145,23 @@ rte_bus_dump(FILE *f)
 		}

lib/librte_eal/common/eal_common_bus.c rte_bus_dump(FILE *f) · line 155

Quoted 10 lines

lib/librte_eal/common/eal_common_bus.c +8 -0

 	}
 }
+
+struct rte_bus *
+rte_bus_find(rte_bus_cmp_t cmp,
+	     const void *data,
+	     const struct rte_bus *start)
+{
+	struct rte_bus *bus = NULL;
+	int started = start == NULL;

Please put brackets around the "start == NULL" to improve readability. My first reading of this I assumed it was doing multiple assignment of both start and started to NULL. To make it extra clear, prefix the comparison with "!!".

lib/librte_eal/common/eal_common_bus.c rte_bus_dump(FILE *f) · line 167

Quoted 12 lines

lib/librte_eal/common/eal_common_bus.c +12 -0

+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!started) {
+			if (bus == start)
+				started = 1;
+			continue;
+		}
+		if (cmp(bus, data) == 0)
+			break;
+	}
+	return bus;
+}

I also think the name "started" is confusing, and might be better as the slighly longer "start_found".

Gaëtan Rivet

author

2017-06-26 20:54 UTC #3 lore raw

Re: [PATCH v5 01/12] bus: add bus iterator to find a bus

Hi Bruce,

Thanks for reading.

Quoted 54 lines · Bruce Richardson

Quoted 5 lines · Gaetan Rivet
From: Jan Blunck <[email protected]>
This helper allows to iterate over all registered buses and find one matching data used as parameter.
Quoted 9 lines
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 20 ++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 43 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 65 insertions(+)
Two minor suggestions below. Otherwise:
Acked-by Bruce Richardson <[email protected]>
Quoted 18 lines
lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0
index 2e48a73..ed09ab2 100644
@@ -162,6 +162,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;
lib/librte_eal/common/eal_common_bus.c +0 -0
index 8f9baf8..4619eb2 100644
@@ -145,3 +145,23 @@ rte_bus_dump(FILE *f)
 		}
lib/librte_eal/common/eal_common_bus.c rte_bus_dump(FILE *f) · line 155
Quoted 10 lines
lib/librte_eal/common/eal_common_bus.c +8 -0
 	}
 }
+
+struct rte_bus *
+rte_bus_find(rte_bus_cmp_t cmp,
+	     const void *data,
+	     const struct rte_bus *start)
+{
+	struct rte_bus *bus = NULL;
+	int started = start == NULL;
Please put brackets around the "start == NULL" to improve readability. My first reading of this I assumed it was doing multiple assignment of both start and started to NULL. To make it extra clear, prefix the comparison with "!!".

Quoted 16 lines

lib/librte_eal/common/eal_common_bus.c rte_bus_dump(FILE *f) · line 167
Quoted 12 lines
lib/librte_eal/common/eal_common_bus.c +12 -0
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!started) {
+			if (bus == start)
+				started = 1;
+			continue;
+		}
+		if (cmp(bus, data) == 0)
+			break;
+	}
+	return bus;
+}
I also think the name "started" is confusing, and might be better as the slighly longer "start_found".

Agreed on both account, it will be fixed in the next version.

Signature · 2 lines

Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-06-26 00:22 UTC #4 lore raw

PATCH v5 2/12 bus: add device iterator method

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/include/rte_bus.h | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

lib/librte_eal/common/include/rte_bus.h +19 -0

index ecf839b..5efb76e 100644
@@ -82,6 +82,24 @@ typedef int (*rte_bus_scan_t)(void);
 typedef int (*rte_bus_probe_t)(void);
 
 /**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to compare each device against.
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.
+ */
+typedef struct rte_device * (*rte_bus_find_device_t)(rte_dev_cmp_t cmp,
+						     const void *data);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -89,6 +107,7 @@ struct rte_bus {
 	const char *name;            /**< Name of the bus */
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
+	rte_bus_find_device_t find_device; /**< Find a device on bus */
 };
 
 /**

lib/librte_eal/common/include/rte_dev.h +21 -0

index de20c06..04d9c28 100644
@@ -191,6 +191,27 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
  */
 int rte_eal_dev_detach(const char *name);
 
+/**
+ * Device comparison function.
+ *
+ * This type of function is used to compare an rte_device with arbitrary
+ * data.
+ *
+ * @param dev
+ *   Device handle.
+ *
+ * @param data
+ *   Data to compare against. The type of this parameter is determined by
+ *   the kind of comparison performed by the function.
+ *
+ * @return
+ *   0 if the device matches the data.
+ *   !0 if the device does not match.
+ *   <0 if ordering is possible and the device is lower than the data.
+ *   >0 if ordering is possible and the device is greater than the data.
+ */
+typedef int (*rte_dev_cmp_t)(const struct rte_device *dev, const void *data);
+
 #define RTE_PMD_EXPORT_NAME_ARRAY(n, idx) n##idx[]
 
 #define RTE_PMD_EXPORT_NAME(name, idx) \

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 2/12

2017-06-26 16:20 UTC #5 lore raw

Re: [PATCH v5 02/12] bus: add device iterator method

Quoted 25 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/include/rte_bus.h | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

lib/librte_eal/common/include/rte_bus.h +8 -0

index ecf839b..5efb76e 100644
@@ -82,6 +82,24 @@ typedef int (*rte_bus_scan_t)(void);
 typedef int (*rte_bus_probe_t)(void);
 
 /**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *

lib/librte_eal/common/include/rte_bus.h typedef int (*rte_bus_scan_t)(void); · line 102

Quoted 10 lines

lib/librte_eal/common/include/rte_bus.h +10 -0

+ * @param data
+ *	Data to compare each device against.
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.
+ */
+typedef struct rte_device * (*rte_bus_find_device_t)(rte_dev_cmp_t cmp,
+						     const void *data);
+
+/**

The bus find function takes a third, start, parameter. Is it worthwhile including such a parameter here, for consistency sake if nothing else?

Otherwise:

Gaëtan Rivet

author

2017-06-26 21:13 UTC #6 lore raw

Re: [PATCH v5 02/12] bus: add device iterator method

Quoted 31 lines · Bruce Richardson

Quoted 30 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/include/rte_bus.h | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

lib/librte_eal/common/include/rte_bus.h +13 -0

index ecf839b..5efb76e 100644
@@ -82,6 +82,24 @@ typedef int (*rte_bus_scan_t)(void);
 typedef int (*rte_bus_probe_t)(void);
 
 /**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to compare each device against.
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.

Quoted 9 lines

lib/librte_eal/common/include/rte_bus.h typedef int (*rte_bus_scan_t)(void); · line 102
Quoted 5 lines
lib/librte_eal/common/include/rte_bus.h +5 -0
+ */
+typedef struct rte_device * (*rte_bus_find_device_t)(rte_dev_cmp_t cmp,
+						     const void *data);
+
+/**
The bus find function takes a third, start, parameter. Is it worthwhile including such a parameter here, for consistency sake if nothing else?

I was wondering this actually.

Doing so would deport the added complexity, to be repeated in each bus implementation. It would in turn simplify my own calls, ever so slightly. That means a few small-ish additional spots of complexity instead of a single one.

I'm also thinking that maybe a few buses could avoid having linked lists for their containers (I know it's frowned upon to roll your own, but some might have good reasons for doing so), allowing random access, bypassing cycling through each.

Well, I will do as you suggest, if anyone objects, please shout before I send the next version.

Quoted 3 lines

Otherwise:
Acked-by Bruce Richardson <[email protected]>

Signature · 2 lines

Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-06-26 00:22 UTC #7 lore raw

PATCH v5 3/12 bus: add helper to find which bus holds a device

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 25 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 32 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index ed09ab2..f1a0765 100644
@@ -163,6 +163,7 @@ DPDK_17.05 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +25 -0

index 4619eb2..d208214 100644
@@ -165,3 +165,28 @@ rte_bus_find(rte_bus_cmp_t cmp,
 	}
 	return bus;
 }
+
+static int
+cmp_rte_device(const struct rte_device *dev1, const void *_dev2)
+{
+	const struct rte_device *dev2 = _dev2;
+
+	return dev1 != dev2;
+}
+
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+	struct rte_device *dev;
+
+	if (bus->find_device == NULL)
+		return -1;
+	dev = bus->find_device(cmp_rte_device, _dev);
+	return dev == NULL;
+}
+
+struct rte_bus *
+rte_bus_find_by_device(const struct rte_device *dev)
+{
+	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
+}

lib/librte_eal/common/include/rte_bus.h +5 -0

index 5efb76e..5441af9 100644
@@ -203,6 +203,11 @@ struct rte_bus *rte_bus_find(rte_bus_cmp_t cmp,
 			     const struct rte_bus *start);
 
 /**
+ * Find the registered bus for a particular device.
+ */
+struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
+
+/**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
  */

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 6efa517..6f77222 100644
@@ -167,6 +167,7 @@ DPDK_17.05 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 3/12

2017-06-26 16:31 UTC #8 lore raw

Re: [PATCH v5 03/12] bus: add helper to find which bus holds a device

Quoted 11 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 25 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 32 insertions(+)

Would it be useful to add a unit test for this function? That would help with any future refactoring, and also indirectly test the other functions added in the previous 2 patches.

Gaëtan Rivet

author

2017-06-26 22:16 UTC #9 lore raw

Re: [PATCH v5 03/12] bus: add helper to find which bus holds a device

Quoted 13 lines · Bruce Richardson

Quoted 11 lines · Gaetan Rivet
From: Jan Blunck <[email protected]>
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 25 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 32 insertions(+)

Quoted 5 lines

Acked-by Bruce Richardson <[email protected]>
Would it be useful to add a unit test for this function? That would help with any future refactoring, and also indirectly test the other functions added in the previous 2 patches.

Yes, I think it would be better to have unit-tests for those functions. They can be a little tricky to get right, working with the layers of callbacks.

I may be short on time for the next version though. I'll see.

Signature · 2 lines

Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-06-26 00:22 UTC #10 lore raw

PATCH v5 4/12 bus: add bus iterator to find a device

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 24 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 52 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index f1a0765..21640d6 100644
@@ -164,6 +164,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +24 -0

index d208214..63fd9f1 100644
@@ -190,3 +190,27 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
 }
+
+struct rte_device *
+rte_bus_find_device(rte_dev_cmp_t cmp, const void *data,
+		    const struct rte_device *start)
+{
+	struct rte_bus *bus;
+	struct rte_device *dev = NULL;
+	int started = start == NULL;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!bus->find_device)
+			continue;
+		if (!started) {
+			dev = bus->find_device(cmp_rte_device, start);
+			if (dev)
+				started = 1;
+			continue;
+		}
+		dev = bus->find_device(cmp, data);
+		if (dev)
+			break;
+	}
+	return dev;
+}

lib/librte_eal/common/include/rte_bus.h +26 -0

index 5441af9..3e83227 100644
@@ -203,6 +203,32 @@ struct rte_bus *rte_bus_find(rte_bus_cmp_t cmp,
 			     const struct rte_bus *start);
 
 /**
+ * Bus iterator to find a particular device.
+ *
+ * This function searches each registered bus to find a device that matches
+ * the data passed as parameter.
+ *
+ * If the comparison function returns zero this function will stop iterating
+ * over any more buses and devices. To continue a search the device of
+ * a previous search can be passed via the start parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to pass to comparison function.
+ *
+ * @param start
+ *	Starting point for the iteration.
+ *
+ * @return
+ *	A pointer to an rte_bus structure or NULL in case no device matches.
+ */
+struct rte_device *rte_bus_find_device(rte_dev_cmp_t cmp,
+				       const void *data,
+				       const struct rte_device *start);
+
+/**
  * Find the registered bus for a particular device.
  */
 struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 6f77222..e0a056d 100644
@@ -168,6 +168,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 4/12

2017-06-27 10:14 UTC #11 lore raw

Re: [PATCH v5 04/12] bus: add bus iterator to find a device

Quoted 11 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 24 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 52 insertions(+)

Same comment for previous patch on naming of "started" variable.

Bruce Richardson

2017-06-27 13:54 UTC #12 lore raw

Re: [PATCH v5 04/12] bus: add bus iterator to find a device

Quoted 28 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 24 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 52 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index f1a0765..21640d6 100644
@@ -164,6 +164,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +0 -0

index d208214..63fd9f1 100644

lib/librte_eal/common/eal_common_bus.c rte_bus_find_by_device(const struct rte_device *dev) · line 196

Quoted 7 lines

lib/librte_eal/common/eal_common_bus.c +4 -0

@@ -190,3 +190,27 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
 }
+
+struct rte_device *
+rte_bus_find_device(rte_dev_cmp_t cmp, const void *data,
+		    const struct rte_device *start)

One additional suggestion: might it be worthwhile also returning the bus for the device here, in an optional 4th parameter. This is, after all, a bus API. :-)

/Bruce

Gaëtan Rivet

author

2017-06-27 15:05 UTC #13 lore raw

Re: [PATCH v5 04/12] bus: add bus iterator to find a device

Quoted 41 lines · Bruce Richardson

Quoted 28 lines · Gaetan Rivet
From: Jan Blunck <[email protected]>
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 24 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 52 insertions(+)
lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0
index f1a0765..21640d6 100644
@@ -164,6 +164,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;
lib/librte_eal/common/eal_common_bus.c +0 -0
index d208214..63fd9f1 100644
lib/librte_eal/common/eal_common_bus.c rte_bus_find_by_device(const struct rte_device *dev) · line 196
Quoted 7 lines
lib/librte_eal/common/eal_common_bus.c +4 -0
@@ -190,3 +190,27 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
 }
+
+struct rte_device *
+rte_bus_find_device(rte_dev_cmp_t cmp, const void *data,
+		    const struct rte_device *start)
One additional suggestion: might it be worthwhile also returning the bus for the device here, in an optional 4th parameter. This is, after all, a bus API. :-)

Quoted 1 line

/Bruce

I think having the bus is a good info.
However,

1. This makes this API slightly less clean I think.
A subjective matter, but I don't like adding cruft without good justification.

2. The bus will be made accessible by my future patches. The series afterward are introducing a generic device type representation, which is composed of the bus. This type info is integral to the device and thus the bus can be read from within the resulting rte_device here.

So, I won't add this parameter right now. If we have issues with the rte_devargs series, it might be interesting to come back on this and edit this API.

Signature · 2 lines

Gaëtan Rivet
6WIND

Bruce Richardson

2017-06-27 15:08 UTC #14 lore raw

Re: [PATCH v5 04/12] bus: add bus iterator to find a device

Quoted 47 lines · Gaëtan Rivet

Quoted 41 lines · Bruce Richardson
Quoted 28 lines · Gaetan Rivet
From: Jan Blunck <[email protected]>
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 24 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 52 insertions(+)
lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0
index f1a0765..21640d6 100644
@@ -164,6 +164,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;
lib/librte_eal/common/eal_common_bus.c +0 -0
index d208214..63fd9f1 100644
lib/librte_eal/common/eal_common_bus.c rte_bus_find_by_device(const struct rte_device *dev) · line 196
Quoted 7 lines
lib/librte_eal/common/eal_common_bus.c +4 -0
@@ -190,3 +190,27 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
 }
+
+struct rte_device *
+rte_bus_find_device(rte_dev_cmp_t cmp, const void *data,
+		    const struct rte_device *start)
One additional suggestion: might it be worthwhile also returning the bus for the device here, in an optional 4th parameter. This is, after all, a bus API. :-)
Quoted 1 line
/Bruce
I think having the bus is a good info.
However,

Quoted 9 lines

1. This makes this API slightly less clean I think.
A subjective matter, but I don't like adding cruft without good justification.
2. The bus will be made accessible by my future patches. The series afterward are introducing a generic device type representation, which is composed of the bus. This type info is integral to the device and thus the bus can be read from within the resulting rte_device here.

So the device will contain a pointer to it's parent bus? If so, then ok, let's leave out the extra parameter.

/Bruce

Quoted 7 lines

So, I won't add this parameter right now. If we have issues with the rte_devargs series, it might be interesting to come back on this and edit this API.
Signature · 2 lines
Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-06-26 00:22 UTC #15 lore raw

PATCH v5 5/12 bus: introduce hotplug functionality

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index 63fd9f1..0035da0 100644
@@ -50,6 +50,8 @@ rte_bus_register(struct rte_bus *bus)
 	/* A bus should mandatorily have the scan implemented */
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
+	/* Buses supporting hotplug also require unplug. */
+	RTE_VERIFY(!bus->plug || bus->unplug);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +31 -0

index 3e83227..187c37e 100644
@@ -100,6 +100,35 @@ typedef struct rte_device * (*rte_bus_find_device_t)(rte_dev_cmp_t cmp,
 						     const void *data);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *	Device declaration.
+ *
+ * @return
+ *	The pointer to a valid rte_device usable by the bus on success.
+ *	NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous device plug call.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -108,6 +137,8 @@ struct rte_bus {
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
 	rte_bus_find_device_t find_device; /**< Find a device on bus */
+	rte_bus_plug_t plug;         /**< Probe single device for drivers */
+	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 };
 
 /**

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 5/12

2017-06-27 12:23 UTC #16 lore raw

Re: [PATCH v5 05/12] bus: introduce hotplug functionality

Quoted 5 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---

Gaetan Rivet

author

2017-06-26 00:22 UTC #17 lore raw

PATCH v5 6/12 vdev: implement find_device bus operation

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_vdev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +13 -0

index 0037a64..52528ef 100644
@@ -338,9 +338,22 @@ vdev_probe(void)
 	return 0;
 }
 
+static struct rte_device *
+vdev_find_device(rte_dev_cmp_t cmp, const void *data)
+{
+	struct rte_vdev_device *dev;
+
+	TAILQ_FOREACH(dev, &vdev_device_list, next) {
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
 static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
+	.find_device = vdev_find_device,
 };
 
 RTE_INIT(rte_vdev_bus_register);

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 6/12

2017-06-27 12:25 UTC #18 lore raw

Re: [PATCH v5 06/12] vdev: implement find_device bus operation

Quoted 5 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---

Gaetan Rivet

author

2017-06-26 00:22 UTC #19 lore raw

PATCH v5 7/12 vdev: implement hotplug functionality

---
 lib/librte_eal/common/eal_common_vdev.c | 36 +++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +36 -0

index 52528ef..22e4640 100644
@@ -38,11 +38,13 @@
 #include <sys/queue.h>
 
 #include <rte_eal.h>
+#include <rte_dev.h>
 #include <rte_bus.h>
 #include <rte_vdev.h>
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_memory.h>
+#include <rte_errno.h>
 
 /** Double linked list of virtual device drivers. */
 TAILQ_HEAD(vdev_device_list, rte_vdev_device);

@@ -350,10 +352,44 @@ vdev_find_device(rte_dev_cmp_t cmp, const void *data)
 	return NULL;
 }
 
+static struct rte_device *
+vdev_plug(struct rte_devargs *da)
+{
+	struct rte_vdev_device *dev;
+	int ret;
+
+	ret = rte_vdev_init(da->virt.drv_name, da->args);
+	if (ret) {
+		rte_errno = -ret;
+		return NULL;
+	}
+	dev = find_vdev(da->virt.drv_name);
+	return &dev->device;
+}
+
+static int
+vdev_unplug(struct rte_device *dev)
+{
+	struct rte_devargs *da;
+	int ret;
+
+	da = dev->devargs;
+	if (da == NULL) {
+		rte_errno = EINVAL;
+		return -1;
+	}
+	ret = rte_vdev_uninit(da->virt.drv_name);
+	if (ret)
+		rte_errno = -ret;
+	return ret;
+}
+
 static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
 	.find_device = vdev_find_device,
+	.plug = vdev_plug,
+	.unplug = vdev_unplug,
 };
 
 RTE_INIT(rte_vdev_bus_register);

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 7/12

2017-06-27 12:41 UTC #20 lore raw

Re: [PATCH v5 07/12] vdev: implement hotplug functionality

Quoted 5 lines · Gaetan Rivet

---
 lib/librte_eal/common/eal_common_vdev.c | 36 +++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Gaetan Rivet

author

2017-06-26 00:22 UTC #21 lore raw

PATCH v5 8/12 vdev: expose bus name

---
 lib/librte_eal/common/include/rte_vdev.h | 2 ++
 1 file changed, 2 insertions(+)

lib/librte_eal/common/include/rte_vdev.h +2 -0

index e6b678e..2d02c68 100644
@@ -41,6 +41,8 @@ extern "C" {
 #include <rte_dev.h>
 #include <rte_devargs.h>
 
+#define VIRTUAL_BUS_NAME "virtual"
+
 struct rte_vdev_device {
 	TAILQ_ENTRY(rte_vdev_device) next;      /**< Next attached vdev */
 	struct rte_device device;               /**< Inherit core device */

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 8/12

2017-06-27 12:46 UTC #22 lore raw

Re: [PATCH v5 08/12] vdev: expose bus name

Quoted 2 lines · Gaetan Rivet

Signed-off-by Gaetan Rivet <[email protected]>
---

The reason for this change is not explained, so either it explain it, or better, IMHO, just merge with the next patch, since it's a one-line change.

If kept as separate patch:

Gaetan Rivet

author

2017-06-26 00:22 UTC #23 lore raw

PATCH v5 9/12 vdev: use standard bus registration function

---
 lib/librte_eal/common/eal_common_vdev.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

lib/librte_eal/common/eal_common_vdev.c +1 ~~-17~~

index 22e4640..a654709 100644
@@ -54,14 +54,10 @@ static struct vdev_device_list vdev_device_list =
 struct vdev_driver_list vdev_driver_list =
 	TAILQ_HEAD_INITIALIZER(vdev_driver_list);
 
-static void rte_vdev_bus_register(void);
-
 /* register a driver */
 void
 rte_vdev_register(struct rte_vdev_driver *driver)
 {
-	rte_vdev_bus_register();
-
 	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
 }

@@ -392,16 +388,4 @@ static struct rte_bus rte_vdev_bus = {
 	.unplug = vdev_unplug,
 };
 
-RTE_INIT(rte_vdev_bus_register);
-
-static void rte_vdev_bus_register(void)
-{
-	static int registered;
-
-	if (registered)
-		return;
-
-	registered = 1;
-	rte_vdev_bus.name = RTE_STR(virtual);
-	rte_bus_register(&rte_vdev_bus);
-}
+RTE_REGISTER_BUS(VIRTUAL_BUS_NAME, rte_vdev_bus);

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 9/12

2017-06-27 12:59 UTC #24 lore raw

Re: [PATCH v5 09/12] vdev: use standard bus registration function

Quoted 5 lines · Gaetan Rivet

---
 lib/librte_eal/common/eal_common_vdev.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

Gaetan Rivet

author

2017-06-26 00:22 UTC #25 lore raw

PATCH v5 10/12 pci: implement find_device bus operation

---
 lib/librte_eal/common/eal_common_pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

lib/librte_eal/common/eal_common_pci.c +13 -0

index 78b097e..00d48d9 100644
@@ -488,10 +488,23 @@ rte_pci_remove_device(struct rte_pci_device *pci_dev)
 	TAILQ_REMOVE(&rte_pci_bus.device_list, pci_dev, next);
 }
 
+static struct rte_device *
+pci_find_device(rte_dev_cmp_t cmp, const void *data)
+{
+	struct rte_pci_device *dev;
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
+		.find_device = pci_find_device,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 10/12

2017-06-27 13:36 UTC #26 lore raw

Re: [PATCH v5 10/12] pci: implement find_device bus operation

Quoted 5 lines · Gaetan Rivet

Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/common/eal_common_pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Gaetan Rivet

author

2017-06-26 00:22 UTC #27 lore raw

PATCH v5 11/12 pci: implement hotplug bus operation

---
 lib/librte_eal/common/eal_common_pci.c | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

lib/librte_eal/common/eal_common_pci.c +44 -0

index 00d48d9..286357d 100644
@@ -47,6 +47,7 @@
 #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>

@@ -500,11 +501,54 @@ pci_find_device(rte_dev_cmp_t cmp, const void *data)
 	return NULL;
 }
 
+static struct rte_device *
+pci_plug(struct rte_devargs *da)
+{
+	struct rte_pci_device *pdev;
+	struct rte_pci_addr *addr;
+
+	addr = &da->pci.addr;
+	/*
+	 * Update eventual pci device in global list.
+	 * Insert it if none was found.
+	 */
+	if (pci_update_device(addr) < 0) {
+		rte_errno = EIO;
+		return NULL;
+	}
+	/* 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) {
+			if (rte_pci_probe_one(addr)) {
+				rte_errno = ENODEV;
+				return NULL;
+			}
+			break;
+		}
+	}
+	return pdev ? &pdev->device : NULL;
+}
+
+static int
+pci_unplug(struct rte_device *dev)
+{
+	struct rte_pci_device *pdev;
+
+	pdev = RTE_DEV_TO_PCI(dev);
+	if (rte_pci_detach(&pdev->addr)) {
+		rte_errno = ENODEV;
+		return -1;
+	}
+	return 0;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
 		.find_device = pci_find_device,
+		.plug = pci_plug,
+		.unplug = pci_unplug,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.1.4

Bruce Richardson

Acked-by on v5 11/12

2017-06-27 13:49 UTC #28 lore raw

Re: [PATCH v5 11/12] pci: implement hotplug bus operation

Quoted 30 lines · Gaetan Rivet

---
 lib/librte_eal/common/eal_common_pci.c | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

lib/librte_eal/common/eal_common_pci.c +10 -0

index 00d48d9..286357d 100644
@@ -47,6 +47,7 @@
 #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>

@@ -500,11 +501,54 @@ pci_find_device(rte_dev_cmp_t cmp, const void *data)
 	return NULL;
 }
 
+static struct rte_device *
+pci_plug(struct rte_devargs *da)
+{
+	struct rte_pci_device *pdev;
+	struct rte_pci_addr *addr;
+
+	addr = &da->pci.addr;
+	/*
+	 * Update eventual pci device in global list.

lib/librte_eal/common/eal_common_pci.c pci_find_device(rte_dev_cmp_t cmp, const void *data) · line 522

Quoted 10 lines

lib/librte_eal/common/eal_common_pci.c +10 -0

+	 * Insert it if none was found.
+	 */
+	if (pci_update_device(addr) < 0) {
+		rte_errno = EIO;
+		return NULL;
+	}
+	/* 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) {
+			if (rte_pci_probe_one(addr)) {

Please put the != 0, or == -1 in the condition, to make it clear it's an error leg.

lib/librte_eal/common/eal_common_pci.c pci_find_device(rte_dev_cmp_t cmp, const void *data) · line 530

Quoted 8 lines

lib/librte_eal/common/eal_common_pci.c +8 -0

+				rte_errno = ENODEV;
+				return NULL;
+			}
+			break;
+		}
+	}
+	return pdev ? &pdev->device : NULL;
+}

Please put in explicit != NULL, as per coding standards here.

lib/librte_eal/common/eal_common_pci.c pci_find_device(rte_dev_cmp_t cmp, const void *data) · line 538

Quoted 8 lines

lib/librte_eal/common/eal_common_pci.c +8 -0

+
+static int
+pci_unplug(struct rte_device *dev)
+{
+	struct rte_pci_device *pdev;
+
+	pdev = RTE_DEV_TO_PCI(dev);
+	if (rte_pci_detach(&pdev->addr)) {

As above, please check for == or != some value.

Quoted 19 lines

lib/librte_eal/common/eal_common_pci.c +8 -0

+		rte_errno = ENODEV;
+		return -1;
+	}
+	return 0;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
 		.find_device = pci_find_device,
+		.plug = pci_plug,
+		.unplug = pci_unplug,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.1.4

With above fixes,

Gaetan Rivet

author

2017-06-26 00:22 UTC #29 lore raw

PATCH v5 12/12 eal: make virtual driver probe and remove take rte_vdev_device

From: Jan Blunck <[email protected]>

This is a preparation to embed the generic rte_device into the rte_eth_dev also for virtual devices.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_dev.c | 93 ++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 22 deletions(-)

lib/librte_eal/common/eal_common_dev.c +71 ~~-22~~

index a400ddd..d83ae41 100644
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_bus.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>

@@ -45,50 +46,98 @@
 
 #include "eal_private.h"
 
+static int cmp_detached_dev_name(const struct rte_device *dev,
+	const void *_name)
+{
+	const char *name = _name;
+
+	/* skip attached devices */
+	if (dev->driver)
+		return 0;
+
+	return strcmp(dev->name, name);
+}
+
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	dev = rte_bus_find_device(cmp_detached_dev_name, name, NULL);
+	if (dev) {
+		struct rte_bus *bus;
+
+		bus = rte_bus_find_by_device(dev);
+		if (!bus) {
+			RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+				name);
+			return -EINVAL;
+		}
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
+		if (!bus->plug) {
+			RTE_LOG(ERR, EAL, "Bus function not supported\n");
+			return -ENOTSUP;
+		}
+
+		ret = (bus->plug(dev->devargs) == NULL);
+		goto out;
 	}
 
-	return 0;
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a
+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+out:
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}
+
+static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+{
+	const char *name = _name;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", name);
-	return -EINVAL;
+	return strcmp(dev->name, name);
 }
 
 int rte_eal_dev_detach(const char *name)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	struct rte_bus *bus;
+	int ret;
 
 	if (name == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device provided.\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_detach(&addr) < 0)
-			goto err;
-	} else {
-		if (rte_vdev_uninit(name))
-			goto err;
+	dev = rte_bus_find_device(cmp_dev_name, name, NULL);
+	if (!dev) {
+		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n", name);
+		return -EINVAL;
+	}
+
+	bus = rte_bus_find_by_device(dev);
+	if (!bus) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n", name);
+		return -EINVAL;
+	}
+
+	if (!bus->unplug) {
+		RTE_LOG(ERR, EAL, "Bus function not supported\n");
+		return -ENOTSUP;
 	}
-	return 0;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n", name);
-	return -EINVAL;
+	ret = bus->unplug(dev);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
+			name);
+	return ret;
 }

Signature · 1 line

2.1.4

Bruce Richardson

2017-06-27 13:58 UTC #30 lore raw

Re: [PATCH v5 12/12] eal: make virtual driver probe and remove take rte_vdev_device

Quoted 26 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>
This is a preparation to embed the generic rte_device into the rte_eth_dev also for virtual devices.
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/common/eal_common_dev.c | 93 ++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 22 deletions(-)
lib/librte_eal/common/eal_common_dev.c +1 -0
index a400ddd..d83ae41 100644
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_bus.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>
@@ -45,50 +46,98 @@
 
 #include "eal_private.h"

lib/librte_eal/common/eal_common_dev.c line 57

Quoted 10 lines

lib/librte_eal/common/eal_common_dev.c +9 -0

 
+static int cmp_detached_dev_name(const struct rte_device *dev,
+	const void *_name)
+{
+	const char *name = _name;
+
+	/* skip attached devices */
+	if (dev->driver)
+		return 0;
+

Does returning 0 from this function not mean that all already-attached devices with match? Is that really what we want, as it doesn't seem to match the logic in the function below. Please explain if I'm wrong here.

Quoted 43 lines

lib/librte_eal/common/eal_common_dev.c +24 -8

+	return strcmp(dev->name, name);
+}
+
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	dev = rte_bus_find_device(cmp_detached_dev_name, name, NULL);
+	if (dev) {
+		struct rte_bus *bus;
+
+		bus = rte_bus_find_by_device(dev);
+		if (!bus) {
+			RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+				name);
+			return -EINVAL;
+		}
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
+		if (!bus->plug) {
+			RTE_LOG(ERR, EAL, "Bus function not supported\n");
+			return -ENOTSUP;
+		}
+
+		ret = (bus->plug(dev->devargs) == NULL);
+		goto out;
 	}
 
-	return 0;
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a

lib/librte_eal/common/eal_common_dev.c line 102

Quoted 10 lines

lib/librte_eal/common/eal_common_dev.c +10 -0

+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+out:
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}
+

<snip>

Regards,
/Bruce

Gaëtan Rivet

author

2017-06-27 14:47 UTC #31 lore raw

Re: [PATCH v5 12/12] eal: make virtual driver probe and remove take rte_vdev_device

Quoted 28 lines · Bruce Richardson

Quoted 27 lines · Gaetan Rivet
From: Jan Blunck <[email protected]>
This is a preparation to embed the generic rte_device into the rte_eth_dev also for virtual devices.
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/common/eal_common_dev.c | 93 ++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 22 deletions(-)
lib/librte_eal/common/eal_common_dev.c +1 -0
index a400ddd..d83ae41 100644
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_bus.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>
@@ -45,50 +46,98 @@
 
 #include "eal_private.h"
 

Quoted 14 lines

lib/librte_eal/common/eal_common_dev.c line 57
Quoted 9 lines
lib/librte_eal/common/eal_common_dev.c +9 -0
+static int cmp_detached_dev_name(const struct rte_device *dev,
+	const void *_name)
+{
+	const char *name = _name;
+
+	/* skip attached devices */
+	if (dev->driver)
+		return 0;
+
Does returning 0 from this function not mean that all already-attached devices with match? Is that really what we want, as it doesn't seem to match the logic in the function below. Please explain if I'm wrong here.

Yes, this was a mistake, good catch.

Quoted 57 lines

Quoted 43 lines

lib/librte_eal/common/eal_common_dev.c +24 -8

+	return strcmp(dev->name, name);
+}
+
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	dev = rte_bus_find_device(cmp_detached_dev_name, name, NULL);
+	if (dev) {
+		struct rte_bus *bus;
+
+		bus = rte_bus_find_by_device(dev);
+		if (!bus) {
+			RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+				name);
+			return -EINVAL;
+		}
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
+		if (!bus->plug) {
+			RTE_LOG(ERR, EAL, "Bus function not supported\n");
+			return -ENOTSUP;
+		}
+
+		ret = (bus->plug(dev->devargs) == NULL);
+		goto out;
 	}
 
-	return 0;
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a

lib/librte_eal/common/eal_common_dev.c line 102

Quoted 10 lines

lib/librte_eal/common/eal_common_dev.c +10 -0

+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+out:
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}
+

<snip>

Regards,
/Bruce

Quoted 5 lines · Bruce Richardson

I'm also not sure about the commit title of this patch. It doesn't match
my understanding of what the patch does.
/Bruce

I have a few issues with this patch myself. However, to properly fix those, I need the rte_devargs evolutions coming afterward.

As an intermediate step, this is good enough to support the current rte_dev attach / detach API and follow the new hotplug API. It makes sense to have it as part of this series, so that the latter is self-contained and internally consistent.

But it will evolve.

I will rewrite the commit log.

Signature · 2 lines

Gaëtan Rivet
6WIND

Bruce Richardson

Acked-by

2017-06-27 15:06 UTC #32 lore raw

Re: [PATCH v5 12/12] eal: make virtual driver probe and remove take rte_vdev_device

Quoted 114 lines · Gaëtan Rivet

Quoted 28 lines · Bruce Richardson
Quoted 27 lines · Gaetan Rivet
From: Jan Blunck <[email protected]>
This is a preparation to embed the generic rte_device into the rte_eth_dev also for virtual devices.
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_eal/common/eal_common_dev.c | 93 ++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 22 deletions(-)
lib/librte_eal/common/eal_common_dev.c +1 -0
index a400ddd..d83ae41 100644
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_bus.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>
@@ -45,50 +46,98 @@
 
 #include "eal_private.h"
 
Quoted 14 lines
lib/librte_eal/common/eal_common_dev.c line 57
Quoted 9 lines
lib/librte_eal/common/eal_common_dev.c +9 -0
+static int cmp_detached_dev_name(const struct rte_device *dev,
+	const void *_name)
+{
+	const char *name = _name;
+
+	/* skip attached devices */
+	if (dev->driver)
+		return 0;
+
Does returning 0 from this function not mean that all already-attached devices with match? Is that really what we want, as it doesn't seem to match the logic in the function below. Please explain if I'm wrong here.
Yes, this was a mistake, good catch.
Quoted 57 lines
Quoted 43 lines
lib/librte_eal/common/eal_common_dev.c +24 -8
+	return strcmp(dev->name, name);
+}
+
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	dev = rte_bus_find_device(cmp_detached_dev_name, name, NULL);
+	if (dev) {
+		struct rte_bus *bus;
+
+		bus = rte_bus_find_by_device(dev);
+		if (!bus) {
+			RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+				name);
+			return -EINVAL;
+		}
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
+		if (!bus->plug) {
+			RTE_LOG(ERR, EAL, "Bus function not supported\n");
+			return -ENOTSUP;
+		}
+
+		ret = (bus->plug(dev->devargs) == NULL);
+		goto out;
 	}
 
-	return 0;
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a
lib/librte_eal/common/eal_common_dev.c line 102
Quoted 10 lines
lib/librte_eal/common/eal_common_dev.c +10 -0
+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+out:
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}
+
<snip>
Regards,
/Bruce
Quoted 5 lines · Bruce Richardson
I'm also not sure about the commit title of this patch. It doesn't match
my understanding of what the patch does.
/Bruce
I have a few issues with this patch myself. However, to properly fix those, I need the rte_devargs evolutions coming afterward.

Quoted 10 lines

As an intermediate step, this is good enough to support the current rte_dev attach / detach API and follow the new hotplug API. It makes sense to have it as part of this series, so that the latter is self-contained and internally consistent.
But it will evolve.
I will rewrite the commit log.

Ok, with bug fix and clearer commit log:

Bruce Richardson

2017-06-27 14:01 UTC #33 lore raw

Re: [PATCH v5 12/12] eal: make virtual driver probe and remove take rte_vdev_device

I'm also not sure about the commit title of this patch. It doesn't match my understanding of what the patch does.

/Bruce

Quoted 8 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>
This is a preparation to embed the generic rte_device into the rte_eth_dev also for virtual devices.
Signed-off-by Jan Blunck <[email protected]>
Signed-off-by Gaetan Rivet <[email protected]>
---

Bruce Richardson

2017-06-27 14:08 UTC #34 lore raw

Quoted 9 lines · Gaetan Rivet

Following the work from Jan:
This patchset introduces the attach / detach API to rte_bus. The rte_device structure is used as the generic device representation.
This API is implemented for the virtual bus.
The functions rte_eal_dev_attach and rte_eal_dev_detach are updated to use this new interface.

Other than my opens on patch 12, this set all looks good to me. The other comments I made are mostly just suggestions.

Hopefully next version can be merged.

Regards,
/Bruce

Gaëtan Rivet

author

2017-06-27 14:48 UTC #35 lore raw

Quoted 6 lines · Bruce Richardson

Quoted 5 lines · Gaetan Rivet
Following the work from Jan:
This patchset introduces the attach / detach API to rte_bus. The rte_device structure is used as the generic device representation.

Quoted 8 lines

Quoted 4 lines
This API is implemented for the virtual bus.
The functions rte_eal_dev_attach and rte_eal_dev_detach are updated to use this new interface.
Other than my opens on patch 12, this set all looks good to me. The other comments I made are mostly just suggestions.
Hopefully next version can be merged.

Thanks for the review, your remarks were useful.
I will send the new version shortly.

Quoted 3 lines

Regards,
/Bruce

Signature · 2 lines

Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-06-27 16:11 UTC #36 lore raw

v6 cover bus: attach / detach API

Following the work from Jan:

This patchset introduces the attach / detach API to rte_bus. The rte_device structure is used as the generic device representation.

This API is implemented for the virtual bus.
The functions rte_eal_dev_attach and rte_eal_dev_detach are updated to use this new interface.

-- v2

0. API rework
-------------

I would like to propose an evolution on the API developed by Jan.

The attach / detach rte_bus API is necessary to support the attach/detach rte_dev API. Those are two different levels for one similar functionality.

This should help maintainers understanding the aim of this API and the differences with the APIs higher-up, clarify the field and allow a new functionality to be proposed.

One remaining problem with the vdev bus is the rte_dev attach implementation, which needs the rte_devargs rework to be properly fixed.

1. Additional evolutions in the patchset
----------------------------------------

The bus devices iterator has been fixed.

The internal rte_device handle was not properly setup within the net_ring PMD.

-- v3

The new API is now

typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da); typedef int (*rte_bus_unplug_t)(struct rte_device *dev);

Additionally, bus and device comparators now returns 0 on match, following strcmp-like behavior.

-- v4

* rte_bus_find now takes a *start* parameter, that can be null. The bus search starts from this element if set.

* A few doc fixes.

* The rte_device field was fixed within the rte_ring PMD in a previous patch. This fix has been integrated by other means, it is not necessary anymore.

-- v5

* The commit
ethdev: use embedded rte_device to detach driver
has been removed from this series to be sent separately.

* The PCI support for device access and hotplug is merged in this series instead of being proposed as a separate patchset.

* A few nitpicks to the code itself have been fixed.

* Some documentation has been reworked.

-- v6

* Fixed a bug in rte_eal_dev_attach with a wrong comparison callback used to find detached devices.

* Fixed coding style errors in PCI hotplug implementation.

* rte_bus find_device() method now takes a *start* parameter.

Gaetan Rivet (4):
vdev: implement hotplug bus operation
vdev: use standard bus registration function
pci: implement find_device bus operation
pci: implement hotplug bus operation

 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   3 +
 lib/librte_eal/common/eal_common_bus.c          |  64 ++++++++++++
 lib/librte_eal/common/eal_common_dev.c          |  92 ++++++++++++----
 lib/librte_eal/common/eal_common_pci.c          |  65 ++++++++++++
 lib/librte_eal/common/eal_common_vdev.c         |  73 ++++++++++---
 lib/librte_eal/common/include/rte_bus.h         | 133 ++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         |  21 ++++
 lib/librte_eal/common/include/rte_vdev.h        |   2 +
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |   3 +
 9 files changed, 418 insertions(+), 38 deletions(-)

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #37 lore raw

PATCH v6 1/11 bus: add bus iterator to find a bus

From: Jan Blunck <[email protected]>

This helper allows to iterate over all registered buses and find one matching data used as parameter.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 20 ++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 43 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 65 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index 2e48a73..ed09ab2 100644
@@ -162,6 +162,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +20 -0

index 8f9baf8..86a54d1 100644
@@ -145,3 +145,23 @@ rte_bus_dump(FILE *f)
 		}
 	}
 }
+
+struct rte_bus *
+rte_bus_find(rte_bus_cmp_t cmp,
+	     const void *data,
+	     const struct rte_bus *start)
+{
+	struct rte_bus *bus = NULL;
+	int start_found = !!(start == NULL);
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!start_found) {
+			if (bus == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(bus, data) == 0)
+			break;
+	}
+	return bus;
+}

lib/librte_eal/common/include/rte_bus.h +43 -0

49 lines not on this page · raw message

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 670bab3..6efa517 100644
@@ -166,6 +166,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #38 lore raw

PATCH v6 2/11 bus: add device iterator method

From: Jan Blunck <[email protected]>

This new method allows buses to expose their devices in a controlled manner. A comparison function is provided by the user to discriminate between devices, using arbitrary data as identifier.

It is possible to start an iteration from a specific point, in order to continue a search.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/include/rte_bus.h | 28 ++++++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 2 files changed, 49 insertions(+)

lib/librte_eal/common/include/rte_bus.h +28 -0

index ecf839b..51f41d9 100644
@@ -82,6 +82,33 @@ typedef int (*rte_bus_scan_t)(void);
 typedef int (*rte_bus_probe_t)(void);
 
 /**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * If the comparison function returns zero this function should stop iterating
+ * over any more devices. To continue a search the device of a previous search
+ * can be passed via the start parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to compare each device against.
+ *
+ * @param start
+ *	starting point for the iteration
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.
+ */
+typedef struct rte_device *
+(*rte_bus_find_device_t)(rte_dev_cmp_t cmp,
+			 const void *data,
+			 const struct rte_device *start);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -89,6 +116,7 @@ struct rte_bus {
 	const char *name;            /**< Name of the bus */
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
+	rte_bus_find_device_t find_device; /**< Find a device on the bus */
 };
 
 /**

lib/librte_eal/common/include/rte_dev.h +21 -0

index de20c06..04d9c28 100644
@@ -191,6 +191,27 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
  */
 int rte_eal_dev_detach(const char *name);
 
+/**
+ * Device comparison function.
+ *
+ * This type of function is used to compare an rte_device with arbitrary
+ * data.
+ *
+ * @param dev
+ *   Device handle.
+ *
+ * @param data
+ *   Data to compare against. The type of this parameter is determined by
+ *   the kind of comparison performed by the function.
+ *
+ * @return
+ *   0 if the device matches the data.
+ *   !0 if the device does not match.
+ *   <0 if ordering is possible and the device is lower than the data.
+ *   >0 if ordering is possible and the device is greater than the data.
+ */
+typedef int (*rte_dev_cmp_t)(const struct rte_device *dev, const void *data);
+
 #define RTE_PMD_EXPORT_NAME_ARRAY(n, idx) n##idx[]
 
 #define RTE_PMD_EXPORT_NAME(name, idx) \

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #39 lore raw

PATCH v6 3/11 bus: add helper to find which bus holds a device

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 25 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 32 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index ed09ab2..f1a0765 100644
@@ -163,6 +163,7 @@ DPDK_17.05 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +25 -0

index 86a54d1..e12dfec 100644
@@ -165,3 +165,28 @@ rte_bus_find(rte_bus_cmp_t cmp,
 	}
 	return bus;
 }
+
+static int
+cmp_rte_device(const struct rte_device *dev1, const void *_dev2)
+{
+	const struct rte_device *dev2 = _dev2;
+
+	return dev1 != dev2;
+}
+
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+	struct rte_device *dev;
+
+	if (bus->find_device == NULL)
+		return -1;
+	dev = bus->find_device(cmp_rte_device, _dev, NULL);
+	return dev == NULL;
+}
+
+struct rte_bus *
+rte_bus_find_by_device(const struct rte_device *dev)
+{
+	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
+}

lib/librte_eal/common/include/rte_bus.h +5 -0

index 51f41d9..ee7b8f0 100644
@@ -212,6 +212,11 @@ struct rte_bus *rte_bus_find(rte_bus_cmp_t cmp,
 			     const struct rte_bus *start);
 
 /**
+ * Find the registered bus for a particular device.
+ */
+struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
+
+/**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
  */

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 6efa517..6f77222 100644
@@ -167,6 +167,7 @@ DPDK_17.05 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #40 lore raw

PATCH v6 4/11 bus: add bus iterator to find a device

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 17 ++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 45 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index f1a0765..21640d6 100644
@@ -164,6 +164,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +17 -0

index e12dfec..dde6c83 100644
@@ -190,3 +190,20 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(bus_find_device, (const void *)dev, NULL);
 }
+
+struct rte_device *
+rte_bus_find_device(rte_dev_cmp_t cmp, const void *data,
+		    const struct rte_device *start)
+{
+	struct rte_bus *bus;
+	struct rte_device *dev = NULL;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (bus->find_device == NULL)
+			continue;
+		dev = bus->find_device(cmp, data, start);
+		if (dev)
+			break;
+	}
+	return dev;
+}

lib/librte_eal/common/include/rte_bus.h +26 -0

index ee7b8f0..9aa047d 100644
@@ -212,6 +212,32 @@ struct rte_bus *rte_bus_find(rte_bus_cmp_t cmp,
 			     const struct rte_bus *start);
 
 /**
+ * Bus iterator to find a particular device.
+ *
+ * This function searches each registered bus to find a device that matches
+ * the data passed as parameter.
+ *
+ * If the comparison function returns zero this function will stop iterating
+ * over any more buses and devices. To continue a search the device of
+ * a previous search can be passed via the start parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to pass to comparison function.
+ *
+ * @param start
+ *	Starting point for the iteration.
+ *
+ * @return
+ *	A pointer to an rte_bus structure or NULL in case no device matches.
+ */
+struct rte_device *rte_bus_find_device(rte_dev_cmp_t cmp,
+				       const void *data,
+				       const struct rte_device *start);
+
+/**
  * Find the registered bus for a particular device.
  */
 struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 6f77222..e0a056d 100644
@@ -168,6 +168,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #41 lore raw

PATCH v6 5/11 bus: introduce hotplug functionality

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index dde6c83..83d9c07 100644
@@ -50,6 +50,8 @@ rte_bus_register(struct rte_bus *bus)
 	/* A bus should mandatorily have the scan implemented */
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
+	/* Buses supporting hotplug also require unplug. */
+	RTE_VERIFY(!bus->plug || bus->unplug);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +31 -0

index 9aa047d..8c2d19c 100644
@@ -109,6 +109,35 @@ typedef struct rte_device *
 			 const struct rte_device *start);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *	Device declaration.
+ *
+ * @return
+ *	The pointer to a valid rte_device usable by the bus on success.
+ *	NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous device plug call.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -117,6 +146,8 @@ struct rte_bus {
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
 	rte_bus_find_device_t find_device; /**< Find a device on the bus */
+	rte_bus_plug_t plug;         /**< Probe single device for drivers */
+	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 };
 
 /**

Signature · 1 line

2.1.4

Jan Blunck

2017-06-27 19:03 UTC #42 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 34 lines · Gaetan Rivet

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

[1]: http://dpdk.org/ml/archives/dev/2017-June/069057.html

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index dde6c83..83d9c07 100644
@@ -50,6 +50,8 @@ rte_bus_register(struct rte_bus *bus)
        /* A bus should mandatorily have the scan implemented */
        RTE_VERIFY(bus->scan);
        RTE_VERIFY(bus->probe);
+       /* Buses supporting hotplug also require unplug. */
+       RTE_VERIFY(!bus->plug || bus->unplug);
 
        TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
        RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +4 -0

index 9aa047d..8c2d19c 100644
@@ -109,6 +109,35 @@ typedef struct rte_device *
                         const struct rte_device *start);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant

lib/librte_eal/common/include/rte_bus.h typedef struct rte_device * · line 125

Quoted 10 lines

lib/librte_eal/common/include/rte_bus.h +10 -0

+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
+ *     The pointer to a valid rte_device usable by the bus on success.
+ *     NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);

Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.

Jan

Quoted 30 lines

lib/librte_eal/common/include/rte_bus.h +17 -1

+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *     Device pointer that was returned by a previous device plug call.
+ *
+ * @return
+ *     0 on success.
+ *     !0 on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -117,6 +146,8 @@ struct rte_bus {
        rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
        rte_bus_probe_t probe;       /**< Probe devices on bus */
        rte_bus_find_device_t find_device; /**< Find a device on the bus */
+       rte_bus_plug_t plug;         /**< Probe single device for drivers */
+       rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 };
 
 /**
--

2.1.4

Thomas Monjalon

2017-06-28 11:44 UTC #43 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

27/06/2017 21:03, Jan Blunck:

Quoted 14 lines · Gaetan Rivet

Quoted 13 lines

lib/librte_eal/common/include/rte_bus.h +10 -0

 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return

Quoted 8 lines

Quoted 4 lines
+ * The pointer to a valid rte_device usable by the bus on success.
+ * NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.

Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.

Jan Blunck

2017-06-28 11:58 UTC #44 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 24 lines · Thomas Monjalon

27/06/2017 21:03, Jan Blunck:
Quoted 14 lines · Gaetan Rivet
Quoted 13 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
Quoted 8 lines
Quoted 4 lines
+ * The pointer to a valid rte_device usable by the bus on success.
+ * NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.

Quoted 4 lines

Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.

What do you mean by "true hotplug"?

The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.

When we get notified about a hotplug event we already know which bus this event belongs to:

1. scan the bus for incoming devices
2. plug single device with devargs and probe for drivers

Makes sense?

Thomas Monjalon

2017-06-28 12:11 UTC #45 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

28/06/2017 13:58, Jan Blunck:

Quoted 25 lines · Thomas Monjalon

Quoted 24 lines
27/06/2017 21:03, Jan Blunck:
Quoted 22 lines · Gaetan Rivet
Quoted 7 lines
lib/librte_eal/common/include/rte_bus.h +4 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
lib/librte_eal/common/include/rte_bus.h typedef struct rte_device * · line 125
Quoted 10 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
+ *     The pointer to a valid rte_device usable by the bus on success.
+ *     NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.

Quoted 6 lines

Quoted 4 lines
Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.
What do you mean by "true hotplug"?

I mean a kernel notification of a new device.

Quoted 5 lines

The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.

Just after the notification, the rte_device does not exist yet. So the plug function could share the same code as the scan function to get the metadata and create the device instance.

Quoted 4 lines

When we get notified about a hotplug event we already know which bus this event belongs to:
1. scan the bus for incoming devices

No need to scan every devices here.

Quoted 3 lines

2. plug single device with devargs and probe for drivers
Makes sense?

I want to make sure there is no misunderstanding first :)

Jan Blunck

2017-06-28 13:09 UTC #46 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 26 lines · Thomas Monjalon

28/06/2017 13:58, Jan Blunck:
Quoted 25 lines · Thomas Monjalon
Quoted 24 lines
27/06/2017 21:03, Jan Blunck:
Quoted 22 lines · Gaetan Rivet
Quoted 7 lines
lib/librte_eal/common/include/rte_bus.h +4 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
lib/librte_eal/common/include/rte_bus.h typedef struct rte_device * · line 125
Quoted 10 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
+ *     The pointer to a valid rte_device usable by the bus on success.
+ *     NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.
What do you mean by "true hotplug"?
I mean a kernel notification of a new device.

Does a "false hotplug" exist, too? :)

Quoted 10 lines

Quoted 5 lines
The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.
Just after the notification, the rte_device does not exist yet. So the plug function could share the same code as the scan function to get the metadata and create the device instance.

Exactly this is what I want to avoid. The plug() function would become a "scan-one and probe". From my point of view plug() and unplug() should be orthogonal. The plug() and unplug() should only be responsible for adding drivers with optional arguments. The EAL should allow the drivers to get unplugged/re-plugged at run-time. I want to be able to change arguments ... or even drivers :)

Quoted 7 lines

Quoted 4 lines
When we get notified about a hotplug event we already know which bus this event belongs to:
1. scan the bus for incoming devices
No need to scan every devices here.

This is a readdir followed by open+read+close for any new device. This code belongs here anyway. Its lightweight if nothing changed. The scan itself should be idempotent anyway.

Quoted 5 lines

Quoted 3 lines
2. plug single device with devargs and probe for drivers
Makes sense?
I want to make sure there is no misunderstanding first :)

Which makes sense. That is probably my fault due to being too distracted with other things and not communicating well enough while Gaetan consumed my code.

Thomas Monjalon

2017-06-28 13:30 UTC #47 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

28/06/2017 15:09, Jan Blunck:

Quoted 32 lines · Thomas Monjalon

Quoted 31 lines
28/06/2017 13:58, Jan Blunck:
Quoted 30 lines · Thomas Monjalon
Quoted 28 lines
27/06/2017 21:03, Jan Blunck:
Quoted 14 lines · Gaetan Rivet
Quoted 13 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
Quoted 8 lines
Quoted 4 lines
+ * The pointer to a valid rte_device usable by the bus on success.
+ * NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.
Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
What do you mean by "true hotplug"?
I mean a kernel notification of a new device.
Does a "false hotplug" exist, too? :)

The false hotplug was the original attach function which was just adding a new ethdev interface.

Quoted 11 lines

Quoted 9 lines
Quoted 5 lines
The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.
Just after the notification, the rte_device does not exist yet. So the plug function could share the same code as the scan function to get the metadata and create the device instance.
Exactly this is what I want to avoid.

Why do you want to avoid that?
I think you mean it is not what you had in mind.

Quoted 1 line

The plug() function would become a "scan-one and probe".

Yes

Quoted 5 lines

From my point of view plug() and unplug() should be orthogonal. The plug() and unplug() should only be responsible for adding drivers with optional arguments. The EAL should allow the drivers to get unplugged/re-plugged at run-time. I want to be able to change arguments ... or even drivers :)

It is a totally different thing.
We are talking about hotplug of a device,
and you are talking about changing drivers dynamically.

So I still don't understand what is the issue with the plug/unplug functions proposed here.

Quoted 13 lines

Quoted 6 lines
Quoted 4 lines
When we get notified about a hotplug event we already know which bus this event belongs to:
1. scan the bus for incoming devices
No need to scan every devices here.
This is a readdir followed by open+read+close for any new device. This code belongs here anyway. Its lightweight if nothing changed. The scan itself should be idempotent anyway.
Quoted 2 lines
Quoted 2 lines
2. plug single device with devargs and probe for drivers

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Makes sense?
I want to make sure there is no misunderstanding first :)
Which makes sense. That is probably my fault due to being too distracted with other things and not communicating well enough while Gaetan consumed my code.

Your ideas are probably interesting, and I want to understand them. In the meantime, we need to progress on 17.08-rc1 which must be done in following days. Please let's separate the ideas which are not yet implemented from what we are already able to deliver.

Jan Blunck

2017-06-28 15:11 UTC #48 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 67 lines · Thomas Monjalon

28/06/2017 15:09, Jan Blunck:
Quoted 32 lines · Thomas Monjalon
Quoted 31 lines
28/06/2017 13:58, Jan Blunck:
Quoted 30 lines · Thomas Monjalon
Quoted 28 lines
27/06/2017 21:03, Jan Blunck:
Quoted 14 lines · Gaetan Rivet
Quoted 13 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
Quoted 8 lines
Quoted 4 lines
+ * The pointer to a valid rte_device usable by the bus on success.
+ * NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.
Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
What do you mean by "true hotplug"?
I mean a kernel notification of a new device.
Does a "false hotplug" exist, too? :)
The false hotplug was the original attach function which was just adding a new ethdev interface.
Quoted 11 lines
Quoted 9 lines
Quoted 5 lines
The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.
Just after the notification, the rte_device does not exist yet. So the plug function could share the same code as the scan function to get the metadata and create the device instance.
Exactly this is what I want to avoid.
Why do you want to avoid that?
I think you mean it is not what you had in mind.
Quoted 1 line
The plug() function would become a "scan-one and probe".
Yes
Quoted 5 lines
From my point of view plug() and unplug() should be orthogonal. The plug() and unplug() should only be responsible for adding drivers with optional arguments. The EAL should allow the drivers to get unplugged/re-plugged at run-time. I want to be able to change arguments ... or even drivers :)

Quoted 7 lines

It is a totally different thing.
We are talking about hotplug of a device,
and you are talking about changing drivers dynamically.
So I still don't understand what is the issue with the plug/unplug functions proposed here.

I don't agree with the notion that plug() means "scan-one and probe".

Quoted 17 lines

Quoted 17 lines
Quoted 6 lines
Quoted 4 lines
When we get notified about a hotplug event we already know which bus this event belongs to:
1. scan the bus for incoming devices
No need to scan every devices here.
This is a readdir followed by open+read+close for any new device. This code belongs here anyway. Its lightweight if nothing changed. The scan itself should be idempotent anyway.
Quoted 5 lines
Quoted 3 lines
2. plug single device with devargs and probe for drivers
Makes sense?
I want to make sure there is no misunderstanding first :)

Quoted 8 lines

Quoted 3 lines
Which makes sense. That is probably my fault due to being too distracted with other things and not communicating well enough while Gaetan consumed my code.
Your ideas are probably interesting, and I want to understand them. In the meantime, we need to progress on 17.08-rc1 which must be done in following days. Please let's separate the ideas which are not yet implemented from what we are already able to deliver.

Sorry, if you got the impression that this is taken from thin air but this is how my original patch series looked like and I'm just providing the rational behind it.

Bruce Richardson

2017-06-28 15:33 UTC #49 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 72 lines · Jan Blunck

Quoted 71 lines · Thomas Monjalon
28/06/2017 15:09, Jan Blunck:
Quoted 32 lines · Thomas Monjalon
Quoted 31 lines
28/06/2017 13:58, Jan Blunck:
Quoted 30 lines · Thomas Monjalon
Quoted 28 lines
27/06/2017 21:03, Jan Blunck:
Quoted 14 lines · Gaetan Rivet
Quoted 13 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
Quoted 8 lines
Quoted 4 lines
+ * The pointer to a valid rte_device usable by the bus on success.
+ * NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.
Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
What do you mean by "true hotplug"?
I mean a kernel notification of a new device.
Does a "false hotplug" exist, too? :)
The false hotplug was the original attach function which was just adding a new ethdev interface.
Quoted 11 lines
Quoted 9 lines
Quoted 5 lines
The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.
Just after the notification, the rte_device does not exist yet. So the plug function could share the same code as the scan function to get the metadata and create the device instance.
Exactly this is what I want to avoid.
Why do you want to avoid that?
I think you mean it is not what you had in mind.
Quoted 1 line
The plug() function would become a "scan-one and probe".
Yes
Quoted 5 lines
From my point of view plug() and unplug() should be orthogonal. The plug() and unplug() should only be responsible for adding drivers with optional arguments. The EAL should allow the drivers to get unplugged/re-plugged at run-time. I want to be able to change arguments ... or even drivers :)
It is a totally different thing.
We are talking about hotplug of a device,
and you are talking about changing drivers dynamically.

Quoted 5 lines

Quoted 3 lines
So I still don't understand what is the issue with the plug/unplug functions proposed here.
I don't agree with the notion that plug() means "scan-one and probe".

What do you see as plug doing then? What do you see as the use-case and then logic flow for hotplug?

Gaëtan Rivet

author

2017-06-29 12:59 UTC #50 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 73 lines · Bruce Richardson

Quoted 72 lines · Jan Blunck
Quoted 71 lines · Thomas Monjalon
28/06/2017 15:09, Jan Blunck:
Quoted 32 lines · Thomas Monjalon
Quoted 31 lines
28/06/2017 13:58, Jan Blunck:
Quoted 30 lines · Thomas Monjalon
Quoted 28 lines
27/06/2017 21:03, Jan Blunck:
Quoted 14 lines · Gaetan Rivet
Quoted 13 lines
lib/librte_eal/common/include/rte_bus.h +10 -0
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ * The plugged device might already have been used previously by the bus,
+ * in which case some buses might prefer to detect and re-use the relevant
+ * information pertaining to this device.
+ *
+ * @param da
+ *     Device declaration.
+ *
+ * @return
Quoted 8 lines
Quoted 4 lines
+ * The pointer to a valid rte_device usable by the bus on success.
+ * NULL on error. rte_errno is then set.
+ */
+typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
Shouldn't this be orthogonal to unplug() and take a rte_device. You should only be able to plug devices that have been found by scan before.
Plugging a device that has been scanned before is a special case. In a true hotplug scenario, we could use this plug function passing a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
What do you mean by "true hotplug"?
I mean a kernel notification of a new device.
Does a "false hotplug" exist, too? :)
The false hotplug was the original attach function which was just adding a new ethdev interface.
Quoted 11 lines
Quoted 9 lines
Quoted 5 lines
The problem with this is that passing just rte_devargs to plug() requires the bus to parse and enrich the rte_devargs with bus specifics. From there it gets folded into the to-be-created bus specific rte_XXX_device. This makes it unnecessarily complicated and even worse it adds a second code path how devices come alive.
Just after the notification, the rte_device does not exist yet. So the plug function could share the same code as the scan function to get the metadata and create the device instance.
Exactly this is what I want to avoid.
Why do you want to avoid that?
I think you mean it is not what you had in mind.
Quoted 1 line
The plug() function would become a "scan-one and probe".
Yes
Quoted 5 lines
From my point of view plug() and unplug() should be orthogonal. The plug() and unplug() should only be responsible for adding drivers with optional arguments. The EAL should allow the drivers to get unplugged/re-plugged at run-time. I want to be able to change arguments ... or even drivers :)
It is a totally different thing.
We are talking about hotplug of a device,
and you are talking about changing drivers dynamically.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
So I still don't understand what is the issue with the plug/unplug functions proposed here.
I don't agree with the notion that plug() means "scan-one and probe".
What do you see as plug doing then? What do you see as the use-case and then logic flow for hotplug?

Hi all,

We are all for having "true" hotplug support in DPDK.
By true hotplug, it means that at some point, a new device exists on the system, while the DPDK bus on which it should be probed does not yet have its metadata. Something needs to be done to retrieve these metadata, one way or another.

What I see as a solution to this:

- An interrupt framework integrated to rte_bus, allowing drivers to describe interrupt sources (Kernel UEVENT, custom FDs, ...), to their buses.

- Applications should be able to pilot these interrupts for rte_bus (either in describing expected devices, or allowing actions to be taken upon device removal).

- Buses could take the responsibility of plugging in and out their own devices, once properly configured.

In this context, it is possible to imagine triggering a bus-rescan upon device ADD, iff we explicitly define scan() as being idempotent (a thing that is not part of its API yet and that we cannot expect from buses at this point).

Then, we can limit bus->plug() to a probe, once we are sure that metadatas for the bus are (almost) always in sync with the system.

Where we are:

- Intel is proposing a UEVENT API[1]. It might be interesting to help them make it respect a generic framework.

- A first plug / unplug implementation is being proposed. Plug() is currently effectively defined as (scan-one + probe).

What can be done to go forward:

- Define the API for rte_bus interrupt sources, configuration and triggers to allow the development of the needed subsystems.

- Each device event sources should offer an event parser to transform them in device descriptions. Depending on the specifics of the source, different levels of info are available.

- Redefine the requirements for bus->scan() to allow hotplugging.

- Reduce the scope of plug() from (scan-one + probe) to (probe), as everything is now in place.

- Further hotplugging developments are then possible: add INTR_ADD support, with flexible device definition for example... A thing that is not yet possible with the current architecture but seemed to interest a few people.

If we can agree that this is a way forward, do you think Jan that having the current plug() API hinders this plan? We can reduce its scope later, without changing current implementations as, as you said, a proper scan should be idempotent. The future API as envisionned is already respected, but right now the hotplug support in buses is a little more involved. Applications that will start using plug() right now would not have to be rewritten, as the requirements for plugging would still be fullfilled.

We can support already hotplugging in DPDK. We can refine this support later to make it more generic and easier to implement for PMD maintainers. But I do not see this as a reason to block this support from being integrated right now.

Signature · 2 lines

Gaëtan Rivet
6WIND

Jan Blunck

2017-06-29 19:20 UTC #51 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 15 lines · Gaëtan Rivet

Hi all,
We are all for having "true" hotplug support in DPDK.
By true hotplug, it means that at some point, a new device exists on the system, while the DPDK bus on which it should be probed does not yet have its metadata. Something needs to be done to retrieve these metadata, one way or another.
What I see as a solution to this:
- An interrupt framework integrated to rte_bus, allowing drivers to describe interrupt sources (Kernel UEVENT, custom FDs, ...), to their buses.

Quoted 7 lines

- Applications should be able to pilot these interrupts for rte_bus (either in describing expected devices, or allowing actions to be taken upon device removal).
- Buses could take the responsibility of plugging in and out their own devices, once properly configured.

This is highly application dependent and it is up to the application developer to decide when such events are getting processed. There is a major difference between the data path functionality that support interrupts and the processing of hotplug events. So from my perspective it needs to be left as an exercise to the programmer to add the polling of the /sysfs files into the event loop. We might offer an example of how to do this though.

Quoted 4 lines

In this context, it is possible to imagine triggering a bus-rescan upon device ADD, iff we explicitly define scan() as being idempotent (a thing that is not part of its API yet and that we cannot expect from buses at this point).

Hmm, so from what I can tell the PCI bus offers an idempotent scan() and if I haven't added any bugs this is true for the virtual bus too.

Quoted 7 lines

Then, we can limit bus->plug() to a probe, once we are sure that metadatas for the bus are (almost) always in sync with the system.
Where we are:
- Intel is proposing a UEVENT API[1]. It might be interesting to help them make it respect a generic framework.

Just because you can do it and someone invested time doesn't mean its a good idea. What problem is this solving? You now have a DPDK native library that reads uevents ... Where is the benefit for the application developer?

Although we replicate some components of an operating system in userspace it doesn't mean that we are building one. If we don't push back on things that don't belong here we will have a big pile of average code soon instead of focusing on the technical problems that need to get addressed.

Quoted 8 lines

- A first plug / unplug implementation is being proposed. Plug() is currently effectively defined as (scan-one + probe).
What can be done to go forward:
- Define the API for rte_bus interrupt sources, configuration and triggers to allow the development of the needed subsystems.

Quoted 9 lines

- Each device event sources should offer an event parser to transform them in device descriptions. Depending on the specifics of the source, different levels of info are available.
- Redefine the requirements for bus->scan() to allow hotplugging.
- Reduce the scope of plug() from (scan-one + probe) to (probe), as everything is now in place.

Also see the series that I send out today. From my point of view we are here already.

Quoted 14 lines

- Further hotplugging developments are then possible: add INTR_ADD support, with flexible device definition for example... A thing that is not yet possible with the current architecture but seemed to interest a few people.
If we can agree that this is a way forward, do you think Jan that having the current plug() API hinders this plan? We can reduce its scope later, without changing current implementations as, as you said, a proper scan should be idempotent. The future API as envisionned is already respected, but right now the hotplug support in buses is a little more involved. Applications that will start using plug() right now would not have to be rewritten, as the requirements for plugging would still be fullfilled.

Quoted 4 lines

We can support already hotplugging in DPDK. We can refine this support later to make it more generic and easier to implement for PMD maintainers. But I do not see this as a reason to block this support from being integrated right now.

Indeed. I had hotplug support in the version of DPDK that I'm working with for the last years. Don't get me wrong: I'm not arguing against the inclusion of hotplug code. I just don't understand the reasoning behind the implementation you proposed (with my original SOB) especially since some of the things that you listed as going forward are already there, are easy to fix or don't necessarily need to be part of the DPDK EAL.

So lets try to get this right from the beginning. What is missing:
1. document and verify that existing scan() implementations are idempotent
2. example app with udev based hotplug
3. check that the symbols are in the correct libraries (bus, pci, vdev)

What am I missing?

Quoted 5 lines

[1]: http://dpdk.org/ml/archives/dev/2017-June/069057.html
--
Gaėtan Rivet
6WIND

Gaëtan Rivet

author

2017-06-30 11:33 UTC #52 lore raw

Re: [PATCH v6 05/11] bus: introduce hotplug functionality

Quoted 10 lines · Jan Blunck

Quoted 9 lines · Gaëtan Rivet
What can be done to go forward:
- Define the API for rte_bus interrupt sources, configuration and triggers to allow the development of the needed subsystems.
- Each device event sources should offer an event parser to transform them in device descriptions. Depending on the specifics of the source, different levels of info are available.

Quoted 9 lines

Quoted 5 lines
- Redefine the requirements for bus->scan() to allow hotplugging.
- Reduce the scope of plug() from (scan-one + probe) to (probe), as everything is now in place.
Also see the series that I send out today. From my point of view we are here already.

Yes, your series is only a superficial departure from the v6. In the end, I see that your critics were pretty much only on interfaces and that you simply wanted it to be your way.

You did not expose your reason for disagreeing, thus I threw a few ideas to at least make you either explicit your view or accept the current proposal.

Quoted 19 lines

Quoted 14 lines
- Further hotplugging developments are then possible: add INTR_ADD support, with flexible device definition for example... A thing that is not yet possible with the current architecture but seemed to interest a few people.
If we can agree that this is a way forward, do you think Jan that having the current plug() API hinders this plan? We can reduce its scope later, without changing current implementations as, as you said, a proper scan should be idempotent. The future API as envisionned is already respected, but right now the hotplug support in buses is a little more involved. Applications that will start using plug() right now would not have to be rewritten, as the requirements for plugging would still be fullfilled.
Quoted 4 lines
We can support already hotplugging in DPDK. We can refine this support later to make it more generic and easier to implement for PMD maintainers. But I do not see this as a reason to block this support from being integrated right now.

Quoted 8 lines

Indeed. I had hotplug support in the version of DPDK that I'm working with for the last years. Don't get me wrong: I'm not arguing against the inclusion of hotplug code. I just don't understand the reasoning behind the implementation you proposed (with my original SOB) especially since some of the things that you listed as going forward are already there, are easy to fix or don't necessarily need to be part of the DPDK EAL.

Don't get me wrong, I am not personally a proponent of these changes, but I wanted the current implementation to at least leave things open as I think a few people will push for this in a near future.

But as far as I'm concerned, the hotplug support in DPDK will be sufficient in v17.08.

Quoted 7 lines

So lets try to get this right from the beginning. What is missing:
1. document and verify that existing scan() implementations are idempotent
2. example app with udev based hotplug
3. check that the symbols are in the correct libraries (bus, pci, vdev)
What am I missing?

Nothing on the technical side.

Signature · 2 lines

Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-06-27 16:11 UTC #53 lore raw

PATCH v6 6/11 vdev: implement find_device bus operation

From: Jan Blunck <[email protected]>

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_vdev.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +21 -0

index 0037a64..625a8e2 100644
@@ -338,9 +338,30 @@ vdev_probe(void)
 	return 0;
 }
 
+static struct rte_device *
+vdev_find_device(rte_dev_cmp_t cmp,
+		 const void *data,
+		 const struct rte_device *start)
+{
+	struct rte_vdev_device *dev;
+	int start_found = !!(start == NULL);
+
+	TAILQ_FOREACH(dev, &vdev_device_list, next) {
+		if (start_found == 0) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
 static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
+	.find_device = vdev_find_device,
 };
 
 RTE_INIT(rte_vdev_bus_register);

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #54 lore raw

PATCH v6 7/11 vdev: implement hotplug bus operation

---
 lib/librte_eal/common/eal_common_vdev.c | 36 +++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +36 -0

index 625a8e2..f5c557f 100644
@@ -38,11 +38,13 @@
 #include <sys/queue.h>
 
 #include <rte_eal.h>
+#include <rte_dev.h>
 #include <rte_bus.h>
 #include <rte_vdev.h>
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_memory.h>
+#include <rte_errno.h>
 
 /** Double linked list of virtual device drivers. */
 TAILQ_HEAD(vdev_device_list, rte_vdev_device);

@@ -358,10 +360,44 @@ vdev_find_device(rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+static struct rte_device *
+vdev_plug(struct rte_devargs *da)
+{
+	struct rte_vdev_device *dev;
+	int ret;
+
+	ret = rte_vdev_init(da->virt.drv_name, da->args);
+	if (ret) {
+		rte_errno = -ret;
+		return NULL;
+	}
+	dev = find_vdev(da->virt.drv_name);
+	return &dev->device;
+}
+
+static int
+vdev_unplug(struct rte_device *dev)
+{
+	struct rte_devargs *da;
+	int ret;
+
+	da = dev->devargs;
+	if (da == NULL) {
+		rte_errno = EINVAL;
+		return -1;
+	}
+	ret = rte_vdev_uninit(da->virt.drv_name);
+	if (ret)
+		rte_errno = -ret;
+	return ret;
+}
+
 static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
 	.find_device = vdev_find_device,
+	.plug = vdev_plug,
+	.unplug = vdev_unplug,
 };
 
 RTE_INIT(rte_vdev_bus_register);

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #55 lore raw

PATCH v6 8/11 vdev: use standard bus registration function

---
 lib/librte_eal/common/eal_common_vdev.c  | 18 +-----------------
 lib/librte_eal/common/include/rte_vdev.h |  2 ++
 2 files changed, 3 insertions(+), 17 deletions(-)

lib/librte_eal/common/eal_common_vdev.c +1 ~~-17~~

index f5c557f..93a0ebe 100644
@@ -54,14 +54,10 @@ static struct vdev_device_list vdev_device_list =
 struct vdev_driver_list vdev_driver_list =
 	TAILQ_HEAD_INITIALIZER(vdev_driver_list);
 
-static void rte_vdev_bus_register(void);
-
 /* register a driver */
 void
 rte_vdev_register(struct rte_vdev_driver *driver)
 {
-	rte_vdev_bus_register();
-
 	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
 }

@@ -400,16 +396,4 @@ static struct rte_bus rte_vdev_bus = {
 	.unplug = vdev_unplug,
 };
 
-RTE_INIT(rte_vdev_bus_register);
-
-static void rte_vdev_bus_register(void)
-{
-	static int registered;
-
-	if (registered)
-		return;
-
-	registered = 1;
-	rte_vdev_bus.name = RTE_STR(virtual);
-	rte_bus_register(&rte_vdev_bus);
-}
+RTE_REGISTER_BUS(VIRTUAL_BUS_NAME, rte_vdev_bus);

lib/librte_eal/common/include/rte_vdev.h +2 -0

index e6b678e..2d02c68 100644
@@ -41,6 +41,8 @@ extern "C" {
 #include <rte_dev.h>
 #include <rte_devargs.h>
 
+#define VIRTUAL_BUS_NAME "virtual"
+
 struct rte_vdev_device {
 	TAILQ_ENTRY(rte_vdev_device) next;      /**< Next attached vdev */
 	struct rte_device device;               /**< Inherit core device */

Signature · 1 line

2.1.4

Thomas Monjalon

Applied

2017-07-03 22:16 UTC #56 lore raw

Re: [PATCH v6 08/11] vdev: use standard bus registration function

27/06/2017 18:11, Gaetan Rivet:

Quoted 1 line

Signed-off-by Gaetan Rivet <[email protected]>

Applied as a prerequisite for v8, thanks

Gaetan Rivet

author

2017-06-27 16:11 UTC #57 lore raw

PATCH v6 9/11 pci: implement find_device bus operation

---
 lib/librte_eal/common/eal_common_pci.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

lib/librte_eal/common/eal_common_pci.c +21 -0

index 78b097e..ab55041 100644
@@ -488,10 +488,31 @@ rte_pci_remove_device(struct rte_pci_device *pci_dev)
 	TAILQ_REMOVE(&rte_pci_bus.device_list, pci_dev, next);
 }
 
+static struct rte_device *
+pci_find_device(rte_dev_cmp_t cmp,
+		const void *data,
+		const struct rte_device *start)
+{
+	struct rte_pci_device *dev;
+	int start_found = !!(start == NULL);
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (!start_found) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
+		.find_device = pci_find_device,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.1.4

Stephen Hemminger

2017-06-27 23:35 UTC #58 lore raw

Re: [PATCH v6 09/11] pci: implement find_device bus operation

Quoted 4 lines · On Tue, 27 Jun 2017 18:11:16 +0200 Gaetan Rivet <[email protected]> wrote:

lib/librte_eal/common/eal_common_pci.c +4 -0

+	int start_found = !!(start == NULL);
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (!start_found) {

lib/librte_eal/common/eal_common_pci.c rte_pci_remove_device(struct rte_pci_device *pci_dev) · line 510

Quoted 10 lines

lib/librte_eal/common/eal_common_pci.c +10 -0

+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+

Why is start_found not a boolean?

Do you really need start_found at all? Why not just reuse existing pointer?

	FOREACH_DEVICE_ON_PCIBUS(dev) {
		if (start) {
			if (&dev->device == start)
				start = NULL
			continue;
		}
		if (cmp(&dev->device, data) == 0)
			return &dev->device;
	}
	return NULL;
}

Gaëtan Rivet

author

2017-06-28 09:17 UTC #59 lore raw

Re: [PATCH v6 09/11] pci: implement find_device bus operation

Quoted 20 lines · Stephen Hemminger

Quoted 4 lines · On Tue, 27 Jun 2017 18:11:16 +0200 Gaetan Rivet <[email protected]> wrote:
lib/librte_eal/common/eal_common_pci.c +4 -0
+	int start_found = !!(start == NULL);
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (!start_found) {
lib/librte_eal/common/eal_common_pci.c rte_pci_remove_device(struct rte_pci_device *pci_dev) · line 510
Quoted 10 lines
lib/librte_eal/common/eal_common_pci.c +10 -0
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
Why is start_found not a boolean?

Ah, yes, I wrote this a few times over in rte_bus and rte_vdev, and mostly used the same scheme in the PCI implementation, without checking for the use of stdbool in the vincinity otherwise.

I would not be opposed to using a bool if I was rewriting it, but I don't feel this change warrants a new version.

Quoted 3 lines

Do you really need start_found at all? Why not just reuse existing pointer?

You are right, it could be reduced. But I find it a little too "clever" in a sense, and I prefer usually to avoid rewriting input parameters. If this function had to be refactored later, the writer would need to be careful about start having changed. The advantage of using one variable less does not outweight this drawback I think.

Quoted 11 lines

	FOREACH_DEVICE_ON_PCIBUS(dev) {
		if (start) {
			if (&dev->device == start)
				start = NULL
			continue;
		}
		if (cmp(&dev->device, data) == 0)
			return &dev->device;
	}
	return NULL;
}

Signature · 2 lines

Gaëtan Rivet
6WIND

Richardson, Bruce

2017-06-28 09:53 UTC #60 lore raw

Re: [PATCH v6 09/11] pci: implement find_device bus operation

Quoted 43 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Gaëtan Rivet
Sent: Wednesday, June 28, 2017 10:18 AM
To: Stephen Hemminger <[email protected]>
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH v6 09/11] pci: implement find_device bus operation
Quoted 20 lines · Stephen Hemminger
Quoted 4 lines · On Tue, 27 Jun 2017 18:11:16 +0200 Gaetan Rivet <[email protected]> wrote:
lib/librte_eal/common/eal_common_pci.c +4 -0
+	int start_found = !!(start == NULL);
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (!start_found) {
lib/librte_eal/common/eal_common_pci.c rte_pci_remove_device(struct rte_pci_device *pci_dev) · line 510
Quoted 10 lines
lib/librte_eal/common/eal_common_pci.c +10 -0
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
Why is start_found not a boolean?
Ah, yes, I wrote this a few times over in rte_bus and rte_vdev, and mostly used the same scheme in the PCI implementation, without checking for the use of stdbool in the vincinity otherwise.
I would not be opposed to using a bool if I was rewriting it, but I don't feel this change warrants a new version.
Quoted 3 lines
Do you really need start_found at all? Why not just reuse existing pointer?

Quoted 6 lines

You are right, it could be reduced. But I find it a little too "clever" in a sense, and I prefer usually to avoid rewriting input parameters. If this function had to be refactored later, the writer would need to be careful about start having changed. The advantage of using one variable less does not outweight this drawback I think.

+1 for having an extra variable for clarity.

Gaetan Rivet

author

2017-06-27 16:11 UTC #61 lore raw

PATCH v6 10/11 pci: implement hotplug bus operation

---
 lib/librte_eal/common/eal_common_pci.c | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

lib/librte_eal/common/eal_common_pci.c +44 -0

index ab55041..e30d2d1 100644
@@ -47,6 +47,7 @@
 #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>

@@ -508,11 +509,54 @@ pci_find_device(rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+static struct rte_device *
+pci_plug(struct rte_devargs *da)
+{
+	struct rte_pci_device *pdev;
+	struct rte_pci_addr *addr;
+
+	addr = &da->pci.addr;
+	/*
+	 * Update eventual pci device in global list.
+	 * Insert it if none was found.
+	 */
+	if (pci_update_device(addr) < 0) {
+		rte_errno = EIO;
+		return NULL;
+	}
+	/* 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) {
+			if (rte_pci_probe_one(addr) != 0) {
+				rte_errno = ENODEV;
+				return NULL;
+			}
+			break;
+		}
+	}
+	return (pdev != NULL) ? &pdev->device : NULL;
+}
+
+static int
+pci_unplug(struct rte_device *dev)
+{
+	struct rte_pci_device *pdev;
+
+	pdev = RTE_DEV_TO_PCI(dev);
+	if (rte_pci_detach(&pdev->addr) != 0) {
+		rte_errno = ENODEV;
+		return -1;
+	}
+	return 0;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
 		.find_device = pci_find_device,
+		.plug = pci_plug,
+		.unplug = pci_unplug,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-06-27 16:11 UTC #62 lore raw

PATCH v6 11/11 dev: use new hotplug API in attach / detach

From: Jan Blunck <[email protected]>

This new generic API allows attach and detach to be compatible with all buses supporting hotplug.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_dev.c | 92 ++++++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 22 deletions(-)

lib/librte_eal/common/eal_common_dev.c +70 ~~-22~~

index a400ddd..19508e6 100644
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_bus.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>

@@ -45,50 +46,97 @@
 
 #include "eal_private.h"
 
+static int cmp_detached_dev_name(const struct rte_device *dev,
+	const void *_name)
+{
+	const char *name = _name;
+
+	/* skip attached devices */
+	if (dev->driver)
+		return 1;
+	return strcmp(dev->name, name);
+}
+
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	dev = rte_bus_find_device(cmp_detached_dev_name, name, NULL);
+	if (dev) {
+		struct rte_bus *bus;
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
+		bus = rte_bus_find_by_device(dev);
+		if (!bus) {
+			RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+				name);
+			return -EINVAL;
+		}
+
+		if (!bus->plug) {
+			RTE_LOG(ERR, EAL, "Bus function not supported\n");
+			return -ENOTSUP;
+		}
+
+		ret = (bus->plug(dev->devargs) == NULL);
+		goto out;
 	}
 
-	return 0;
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a
+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+out:
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", name);
-	return -EINVAL;
+static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+{
+	const char *name = _name;
+
+	return strcmp(dev->name, name);
 }
 
 int rte_eal_dev_detach(const char *name)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	struct rte_bus *bus;
+	int ret;
 
 	if (name == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device provided.\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_detach(&addr) < 0)
-			goto err;
-	} else {
-		if (rte_vdev_uninit(name))
-			goto err;
+	dev = rte_bus_find_device(cmp_dev_name, name, NULL);
+	if (!dev) {
+		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n", name);
+		return -EINVAL;
+	}
+
+	bus = rte_bus_find_by_device(dev);
+	if (!bus) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n", name);
+		return -EINVAL;
+	}
+
+	if (!bus->unplug) {
+		RTE_LOG(ERR, EAL, "Bus function not supported\n");
+		return -ENOTSUP;
 	}
-	return 0;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n", name);
-	return -EINVAL;
+	ret = bus->unplug(dev);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
+			name);
+	return ret;
 }

Signature · 1 line

2.1.4

Jan Blunck

2017-06-29 18:22 UTC #63 lore raw

cover bus attach/detach API and hotplug rework

Following the work from Gaetan, who based his work on my original series.

-- v7

* Revert removal of RTE_VERIFY on find_device operation since a bus that isn't able to even iterate its devices is pointless

* Revert plug() changes that have been introduced in v3:
int (*rte_bus_plug_t)(struct rte_device *dev, const char *devargs);

* Revert pointless reordering of start argument for bus iterator functions

* Add fslmc bus find_device operation

* Add support to find a bus by its name

* Add EAL hotplug API:
int rte_eal_hotplug_add(const char *busname, const char *devname,

			  const char *devargs);

int rte_eal_hotplug_remove(const char *busname, const char *devname);

* Keep rte_eal_dev_attach() backwards compatible and support hotplug of PCI and virtual devices only

* Add back patch "ethdev: use embedded rte_device to detach driver" which is critical for correctly detaching the correct low-level device

Gaetan Rivet (1):
pci: implement hotplug bus operation

Jan Blunck (14):
bus: add bus iterator to find a bus
bus: add find_device bus operation
vdev: implement find_device bus operation
pci: implement find_device bus operation bus/fslmc: implement find_device bus operation
bus: add helper to find which bus holds a device
bus: add bus iterator to find a device
bus: require buses to implement find_device operation
bus: add rte_bus_find_by_name
bus: introduce device plug/unplug functionality
vdev: implement unplug bus operation
eal: add hotplug add/remove functions
dev: use new hotplug API in attach / detach
ethdev: Use embedded rte_device to detach driver

 drivers/bus/fslmc/fslmc_bus.c                   |  23 ++++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   6 +
 lib/librte_eal/common/eal_common_bus.c          |  74 ++++++++++++
 lib/librte_eal/common/eal_common_dev.c          | 149 ++++++++++++++++++++----
 lib/librte_eal/common/eal_common_pci.c          |  55 +++++++++
 lib/librte_eal/common/eal_common_vdev.c         |  35 ++++++
 lib/librte_eal/common/include/rte_bus.h         | 137 ++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         |  57 +++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |   5 +
 lib/librte_ether/rte_ethdev.c                   |   3 +-
 10 files changed, 521 insertions(+), 23 deletions(-)

--
2.9.4

Jan Blunck

2017-06-29 18:22 UTC #64 lore raw

PATCH v7 1/15 bus: add bus iterator to find a bus

This helper allows to iterate over all registered buses and find one matching data used as parameter.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 19 +++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 42 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 63 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index 2e48a73..ed09ab2 100644
@@ -162,6 +162,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +19 -0

index 8f9baf8..3094daa 100644
@@ -145,3 +145,22 @@ rte_bus_dump(FILE *f)
 		}
 	}
 }
+
+struct rte_bus *
+rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
+	     const void *data)
+{
+	struct rte_bus *bus = NULL;
+	bool start_found = !start;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!start_found) {
+			if (bus == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(bus, data) == 0)
+			break;
+	}
+	return bus;
+}

lib/librte_eal/common/include/rte_bus.h +42 -0

48 lines not on this page · raw message

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 670bab3..6efa517 100644
@@ -166,6 +166,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_bus_find;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #65 lore raw

PATCH v7 2/15 bus: add find_device bus operation

This new method allows buses to expose their devices in a controlled manner. A comparison function is provided by the user to discriminate between devices, using arbitrary data as identifier.

It is possible to start an iteration from a specific point, in order to continue a search.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/include/rte_bus.h | 27 +++++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 2 files changed, 48 insertions(+)

lib/librte_eal/common/include/rte_bus.h +27 -0

index c52b65b..052ac8d 100644
@@ -82,6 +82,32 @@ typedef int (*rte_bus_scan_t)(void);
 typedef int (*rte_bus_probe_t)(void);
 
 /**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * If the comparison function returns zero this function should stop iterating
+ * over any more devices. To continue a search the device of a previous search
+ * can be passed via the start parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to compare each device against.
+ *
+ * @param start
+ *	starting point for the iteration
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.
+ */
+typedef struct rte_device *
+(*rte_bus_find_device_t)(const struct rte_device *start, rte_dev_cmp_t cmp,
+			 const void *data);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -89,6 +115,7 @@ struct rte_bus {
 	const char *name;            /**< Name of the bus */
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
+	rte_bus_find_device_t find_device; /**< Find a device on the bus */
 };
 
 /**

lib/librte_eal/common/include/rte_dev.h +21 -0

index de20c06..04d9c28 100644
@@ -191,6 +191,27 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
  */
 int rte_eal_dev_detach(const char *name);
 
+/**
+ * Device comparison function.
+ *
+ * This type of function is used to compare an rte_device with arbitrary
+ * data.
+ *
+ * @param dev
+ *   Device handle.
+ *
+ * @param data
+ *   Data to compare against. The type of this parameter is determined by
+ *   the kind of comparison performed by the function.
+ *
+ * @return
+ *   0 if the device matches the data.
+ *   !0 if the device does not match.
+ *   <0 if ordering is possible and the device is lower than the data.
+ *   >0 if ordering is possible and the device is greater than the data.
+ */
+typedef int (*rte_dev_cmp_t)(const struct rte_device *dev, const void *data);
+
 #define RTE_PMD_EXPORT_NAME_ARRAY(n, idx) n##idx[]
 
 #define RTE_PMD_EXPORT_NAME(name, idx) \

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #66 lore raw

PATCH v7 3/15 vdev: implement find_device bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_vdev.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +21 -0

index b4db2be..9bb7427 100644
@@ -35,6 +35,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <sys/queue.h>
 
 #include <rte_eal.h>

@@ -334,9 +335,29 @@ vdev_probe(void)
 	return 0;
 }
 
+static struct rte_device *
+vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		 const void *data)
+{
+	struct rte_vdev_device *dev;
+	bool start_found = !start;
+
+	TAILQ_FOREACH(dev, &vdev_device_list, next) {
+		if (start_found == 0) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
 static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
+	.find_device = vdev_find_device,
 };
 
 RTE_REGISTER_BUS(VIRTUAL_BUS_NAME, rte_vdev_bus);

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #67 lore raw

PATCH v7 4/15 pci: implement find_device bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_pci.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

lib/librte_eal/common/eal_common_pci.c +21 -0

index 479c62d..e449758 100644
@@ -485,10 +485,31 @@ rte_pci_remove_device(struct rte_pci_device *pci_dev)
 	TAILQ_REMOVE(&rte_pci_bus.device_list, pci_dev, next);
 }
 
+static struct rte_device *
+pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		const void *data)
+{
+	struct rte_pci_device *dev;
+	bool start_found = !start;
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (!start_found) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+
+	return NULL;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
+		.find_device = pci_find_device,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #68 lore raw

PATCH v7 5/15 bus/fslmc: implement find_device bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 drivers/bus/fslmc/fslmc_bus.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

drivers/bus/fslmc/fslmc_bus.c +23 -0

index b24642d..1e3bbee 100644
@@ -32,6 +32,7 @@
 
 #include <string.h>
 #include <dirent.h>
+#include <stdbool.h>
 
 #include <rte_log.h>
 #include <rte_bus.h>

@@ -105,6 +106,27 @@ rte_fslmc_probe(void)
 	return ret;
 }
 
+static struct rte_device *
+rte_fslmc_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		      const void *data)
+{
+	struct rte_dpaa2_device *dev;
+	bool start_found = !start;
+
+	TAILQ_FOREACH(dev, &rte_fslmc_bus.device_list, next) {
+		if (!start_found) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+
+	return NULL;
+}
+
 /*register a fslmc bus based dpaa2 driver */
 void
 rte_fslmc_driver_register(struct rte_dpaa2_driver *driver)

@@ -133,6 +155,7 @@ struct rte_fslmc_bus rte_fslmc_bus = {
 	.bus = {
 		.scan = rte_fslmc_scan,
 		.probe = rte_fslmc_probe,
+		.find_device = rte_fslmc_find_device,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_fslmc_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_fslmc_bus.driver_list),

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #69 lore raw

PATCH v7 6/15 bus: add helper to find which bus holds a device

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 23 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 30 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index ed09ab2..f1a0765 100644
@@ -163,6 +163,7 @@ DPDK_17.05 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +23 -0

index 3094daa..276cce6 100644
@@ -164,3 +164,26 @@ rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 	}
 	return bus;
 }
+
+static int
+cmp_rte_device(const struct rte_device *dev1, const void *_dev2)
+{
+	const struct rte_device *dev2 = _dev2;
+
+	return dev1 != dev2;
+}
+
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+	struct rte_device *dev;
+
+	dev = bus->find_device(NULL, cmp_rte_device, _dev);
+	return !dev;
+}
+
+struct rte_bus *
+rte_bus_find_by_device(const struct rte_device *dev)
+{
+	return rte_bus_find(NULL, bus_find_device, (const void *)dev);
+}

lib/librte_eal/common/include/rte_bus.h +5 -0

index 052ac8d..f8b3215 100644
@@ -210,6 +210,11 @@ struct rte_bus *rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 			     const void *data);
 
 /**
+ * Find the registered bus for a particular device.
+ */
+struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
+
+/**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
  */

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 6efa517..6f77222 100644
@@ -167,6 +167,7 @@ DPDK_17.05 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.9.4

Thomas Monjalon

2017-06-30 09:16 UTC #70 lore raw

Re: [PATCH v7 06/15] bus: add helper to find which bus holds a device

29/06/2017 20:21, Jan Blunck:

lib/librte_eal/common/eal_common_bus.c rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp, · line 183

Quoted 8 lines

lib/librte_eal/common/eal_common_bus.c +8 -0

+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+	struct rte_device *dev;
+
+	dev = bus->find_device(NULL, cmp_rte_device, _dev);
+	return !dev;
+}

The preferred code style is to make explicit the NULL comparisons:

	return dev == NULL;

Jan Blunck

2017-06-30 16:46 UTC #71 lore raw

Re: [PATCH v7 06/15] bus: add helper to find which bus holds a device

Quoted 11 lines · Thomas Monjalon

29/06/2017 20:21, Jan Blunck:
lib/librte_eal/common/eal_common_bus.c rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp, · line 183
Quoted 8 lines
lib/librte_eal/common/eal_common_bus.c +8 -0
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+     struct rte_device *dev;
+
+     dev = bus->find_device(NULL, cmp_rte_device, _dev);
+     return !dev;
+}
The preferred code style is to make explicit the NULL comparisons:

Quoted 1 line

        return dev == NULL;

Oh, interesting ... not a lot of C++ programmers around here I guess.

Does this mean you also want me to make integer tests explicit again 0?

Thomas Monjalon

2017-06-30 18:29 UTC #72 lore raw

Re: [PATCH v7 06/15] bus: add helper to find which bus holds a device

30/06/2017 18:46, Jan Blunck:

Quoted 14 lines · Thomas Monjalon

Quoted 11 lines
29/06/2017 20:21, Jan Blunck:
lib/librte_eal/common/eal_common_bus.c rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp, · line 183
Quoted 8 lines
lib/librte_eal/common/eal_common_bus.c +8 -0
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+     struct rte_device *dev;
+
+     dev = bus->find_device(NULL, cmp_rte_device, _dev);
+     return !dev;
+}
The preferred code style is to make explicit the NULL comparisons:
Quoted 1 line
        return dev == NULL;

Quoted 3 lines

Oh, interesting ... not a lot of C++ programmers around here I guess.
Does this mean you also want me to make integer tests explicit again 0?

Good question, I don't know.
I know only this part of the coding rules:
http://dpdk.org/doc/guides/contributing/coding_style.html#null-pointers

Bruce Richardson

2017-06-30 21:24 UTC #73 lore raw

Re: [PATCH v7 06/15] bus: add helper to find which bus holds a device

Quoted 15 lines · Thomas Monjalon

30/06/2017 18:46, Jan Blunck:
Quoted 14 lines · Thomas Monjalon
Quoted 12 lines
29/06/2017 20:21, Jan Blunck:
lib/librte_eal/common/eal_common_bus.c rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp, · line 183
Quoted 8 lines
lib/librte_eal/common/eal_common_bus.c +8 -0
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+     struct rte_device *dev;
+
+     dev = bus->find_device(NULL, cmp_rte_device, _dev);
+     return !dev;
+}
The preferred code style is to make explicit the NULL comparisons:
        return dev == NULL;

Quoted 8 lines

Quoted 3 lines
Oh, interesting ... not a lot of C++ programmers around here I guess.
Does this mean you also want me to make integer tests explicit again 0?
Good question, I don't know.
I know only this part of the coding rules:
http://dpdk.org/doc/guides/contributing/coding_style.html#null-pointers

Yes, I noticed that gap the other day. IMHO for consistency the integers should similarly be compared to 0/non-zero explicitly rather than using "!" operator. The exception I would allow is where a function is named in such a way that is clearly returns a boolean value as int e.g. a function "int is_computer_on()".

/Bruce

Bruce Richardson

2017-06-30 21:25 UTC #74 lore raw

Re: [PATCH v7 06/15] bus: add helper to find which bus holds a device

Quoted 14 lines · Jan Blunck

Quoted 11 lines · Thomas Monjalon
29/06/2017 20:21, Jan Blunck:
lib/librte_eal/common/eal_common_bus.c rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp, · line 183
Quoted 8 lines
lib/librte_eal/common/eal_common_bus.c +8 -0
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+     struct rte_device *dev;
+
+     dev = bus->find_device(NULL, cmp_rte_device, _dev);
+     return !dev;
+}
The preferred code style is to make explicit the NULL comparisons:
Quoted 1 line
        return dev == NULL;

Quoted 3 lines

Oh, interesting ... not a lot of C++ programmers around here I guess.
Does this mean you also want me to make integer tests explicit again 0?

Please do.

Jan Blunck

2017-06-29 18:22 UTC #75 lore raw

PATCH v7 7/15 bus: add bus iterator to find a device

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 15 ++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 26 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 43 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index f1a0765..21640d6 100644
@@ -164,6 +164,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_log_dump;
 	rte_log_register;

lib/librte_eal/common/eal_common_bus.c +15 -0

index 276cce6..61aa947 100644
@@ -187,3 +187,18 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(NULL, bus_find_device, (const void *)dev);
 }
+
+struct rte_device *
+rte_bus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		    const void *data)
+{
+	struct rte_bus *bus;
+	struct rte_device *dev = NULL;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		dev = bus->find_device(start, cmp, data);
+		if (dev)
+			break;
+	}
+	return dev;
+}

lib/librte_eal/common/include/rte_bus.h +26 -0

index f8b3215..fea0f39 100644
@@ -210,6 +210,32 @@ struct rte_bus *rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 			     const void *data);
 
 /**
+ * Bus iterator to find a particular device.
+ *
+ * This function searches each registered bus to find a device that matches
+ * the data passed as parameter.
+ *
+ * If the comparison function returns zero this function will stop iterating
+ * over any more buses and devices. To continue a search the device of
+ * a previous search can be passed via the start parameter.
+ *
+ * @param start
+ *	Starting point for the iteration.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to pass to comparison function.
+ *
+ * @return
+ *	A pointer to a rte_bus structure or NULL in case no device matches.
+ */
+struct rte_device *rte_bus_find_device(const struct rte_device *start,
+				       rte_dev_cmp_t cmp,
+				       const void *data);
+
+/**
  * Find the registered bus for a particular device.
  */
 struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 6f77222..e0a056d 100644
@@ -168,6 +168,7 @@ DPDK_17.05 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_device;
 	rte_cpu_is_supported;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;

Signature · 1 line

2.9.4

Thomas Monjalon

2017-06-30 09:17 UTC #76 lore raw

Re: [PATCH v7 07/15] bus: add bus iterator to find a device

29/06/2017 20:21, Jan Blunck:

lib/librte_eal/common/eal_common_bus.c rte_bus_find_by_device(const struct rte_device *dev) · line 200

Quoted 10 lines

lib/librte_eal/common/eal_common_bus.c +10 -0

+struct rte_device *
+rte_bus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+                   const void *data)
+{
+       struct rte_bus *bus;
+       struct rte_device *dev = NULL;
+
+       TAILQ_FOREACH(bus, &rte_bus_list, next) {
+               dev = bus->find_device(start, cmp, data);
+               if (dev)

Same nit as previous patch,
better to make explicit dev != NULL.

Quoted 4 lines

lib/librte_eal/common/eal_common_bus.c +4 -0

+                       break;
+       }
+       return dev;
+}

Jan Blunck

2017-06-29 18:22 UTC #77 lore raw

PATCH v7 8/15 bus: require buses to implement find_device operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c | 1 +
 1 file changed, 1 insertion(+)

lib/librte_eal/common/eal_common_bus.c +1 -0

index 61aa947..d0e652e 100644
@@ -50,6 +50,7 @@ rte_bus_register(struct rte_bus *bus)
 	/* A bus should mandatorily have the scan implemented */
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
+	RTE_VERIFY(bus->find_device);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #78 lore raw

PATCH v7 9/15 bus: add rte_bus_find_by_name

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  | 14 ++++++++++++++
 lib/librte_eal/common/include/rte_bus.h |  5 +++++
 2 files changed, 19 insertions(+)

lib/librte_eal/common/eal_common_bus.c +14 -0

index d0e652e..418804a 100644
@@ -203,3 +203,17 @@ rte_bus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 	}
 	return dev;
 }
+
+static int
+cmp_bus_name(const struct rte_bus *bus, const void *_name)
+{
+	const char *name = _name;
+
+	return strcmp(bus->name, name);
+}
+
+struct rte_bus *
+rte_bus_find_by_name(const char *busname)
+{
+	return rte_bus_find(NULL, cmp_bus_name, (const void *)busname);
+}

lib/librte_eal/common/include/rte_bus.h +5 -0

index fea0f39..08f8d46 100644
@@ -241,6 +241,11 @@ struct rte_device *rte_bus_find_device(const struct rte_device *start,
 struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
 
 /**
+ * Find the registered bus for a given name.
+ */
+struct rte_bus *rte_bus_find_by_name(const char *busname);
+
+/**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
  */

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #79 lore raw

PATCH v7 10/15 bus: introduce device plug/unplug functionality

This allows the buses to plug and probe specific devices. This is meant to be a building block for hotplug support.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index 418804a..22ff0f6 100644
@@ -51,6 +51,8 @@ rte_bus_register(struct rte_bus *bus)
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
 	RTE_VERIFY(bus->find_device);
+	/* Buses supporting driver plug also require unplug. */
+	RTE_VERIFY(!bus->plug || bus->unplug);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +32 -0

index 08f8d46..4b6c73b 100644
@@ -108,6 +108,36 @@ typedef struct rte_device *
 			 const void *data);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @param devargs
+ *	Device declaration.
+ *
+ * @return
+ *	The pointer to a valid rte_device usable by the bus on success.
+ *	NULL on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_plug_t)(struct rte_device *dev,
+			      const char *devargs);
+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -116,6 +146,8 @@ struct rte_bus {
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
 	rte_bus_find_device_t find_device; /**< Find a device on the bus */
+	rte_bus_plug_t plug;         /**< Probe single device for drivers */
+	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 };
 
 /**

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #80 lore raw

PATCH v7 11/15 vdev: implement unplug bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_vdev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +14 -0

index 9bb7427..baf3c5b 100644
@@ -39,11 +39,13 @@
 #include <sys/queue.h>
 
 #include <rte_eal.h>
+#include <rte_dev.h>
 #include <rte_bus.h>
 #include <rte_vdev.h>
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_memory.h>
+#include <rte_errno.h>
 
 /** Double linked list of virtual device drivers. */
 TAILQ_HEAD(vdev_device_list, rte_vdev_device);

@@ -354,10 +356,22 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+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);
+}
+
 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 */
+	.unplug = vdev_unplug,
 };
 
 RTE_REGISTER_BUS(VIRTUAL_BUS_NAME, rte_vdev_bus);

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #81 lore raw

PATCH v7 12/15 pci: implement hotplug bus operation

From: Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/eal_common_pci.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

lib/librte_eal/common/eal_common_pci.c +34 -0

index e449758..5ee100e 100644
@@ -47,6 +47,7 @@
 #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>

@@ -505,11 +506,44 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+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;
+}
+
+static int
+pci_unplug(struct rte_device *dev)
+{
+	struct rte_pci_device *pdev;
+
+	pdev = RTE_DEV_TO_PCI(dev);
+	if (rte_pci_detach(&pdev->addr) != 0) {
+		rte_errno = ENODEV;
+		return -1;
+	}
+	return 0;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
 		.find_device = pci_find_device,
+		.plug = pci_plug,
+		.unplug = pci_unplug,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.9.4

Jan Blunck

2017-06-29 18:22 UTC #82 lore raw

PATCH v7 13/15 eal: add hotplug add/remove functions

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  2 +
 lib/librte_eal/common/eal_common_dev.c          | 68 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         | 28 ++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  2 +
 4 files changed, 100 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +2 -0

index 21640d6..b7d26b2 100644
@@ -166,6 +166,8 @@ DPDK_17.05 {
 	rte_bus_find_by_device;
 	rte_bus_find_device;
 	rte_cpu_is_supported;
+	rte_eal_hotplug_add;
+	rte_eal_hotplug_remove;
 	rte_log_dump;
 	rte_log_register;
 	rte_log_get_global_level;

lib/librte_eal/common/eal_common_dev.c +68 -0

71 lines not on this page · raw message

lib/librte_eal/common/include/rte_dev.h +28 -0

index 04d9c28..95440eb 100644
@@ -192,6 +192,34 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
 int rte_eal_dev_detach(const char *name);
 
 /**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ *   The bus name the device is added to.
+ * @param devname
+ *   The device name. Based on this device name, eal will identify a driver
+ *   capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ *   Device arguments to be passed to the driver.
+ * @return
+ *   0 on success, negative on error.
+ */
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+			const char *devargs);
+
+/**
+ * Hotplug remove a given device from a specific bus.
+ *
+ * @param busname
+ *   The bus name the device is removed from.
+ * @param devname
+ *   The device name being removed.
+ * @return
+ *   0 on success, negative on error.
+ */
+int rte_eal_hotplug_remove(const char *busname, const char *devname);
+
+/**
  * Device comparison function.
  *
  * This type of function is used to compare an rte_device with arbitrary

lib/librte_eal/linuxapp/eal/rte_eal_version.map +2 -0

index e0a056d..a94cb7a 100644
@@ -170,6 +170,8 @@ DPDK_17.05 {
 	rte_bus_find_by_device;
 	rte_bus_find_device;
 	rte_cpu_is_supported;
+	rte_eal_hotplug_add;
+	rte_eal_hotplug_remove;
 	rte_intr_free_epoll_fd;
 	rte_log_dump;
 	rte_log_get_global_level;

Signature · 1 line

2.9.4

Thomas Monjalon

2017-06-30 09:06 UTC #83 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

29/06/2017 20:22, Jan Blunck:

Quoted 13 lines

/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */

lib/librte_eal/common/include/rte_dev.h int rte_eal_dev_attach(const char *name, const char *devargs); · line 208

Quoted 2 lines

lib/librte_eal/common/include/rte_dev.h +2 -0

+int rte_eal_hotplug_add(const char *busname, const char *devname,
+			const char *devargs);

After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?

Gaëtan Rivet

author

2017-06-30 09:11 UTC #84 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

Quoted 13 lines · Thomas Monjalon

29/06/2017 20:22, Jan Blunck:
Quoted 12 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.

Quoted 8 lines

lib/librte_eal/common/include/rte_dev.h int rte_eal_dev_attach(const char *name, const char *devargs); · line 208
Quoted 3 lines
+ */
lib/librte_eal/common/include/rte_dev.h +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+			const char *devargs);
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?

Also possible: return a struct *rte_device and set rte_errno on error.

Signature · 2 lines

Gaëtan Rivet
6WIND

Bruce Richardson

2017-06-30 09:20 UTC #85 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

Quoted 23 lines · Gaëtan Rivet

Quoted 13 lines · Thomas Monjalon
29/06/2017 20:22, Jan Blunck:
Quoted 12 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
Quoted 8 lines
lib/librte_eal/common/include/rte_dev.h int rte_eal_dev_attach(const char *name, const char *devargs); · line 208
Quoted 3 lines
+ */
lib/librte_eal/common/include/rte_dev.h +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+			const char *devargs);
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?

Quoted 2 lines

Also possible: return a struct *rte_device and set rte_errno on error.

+1 for this option.

Jan Blunck

2017-06-30 15:44 UTC #86 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

Quoted 19 lines · Bruce Richardson

Quoted 18 lines · Gaėtan Rivet
Quoted 17 lines · Thomas Monjalon
29/06/2017 20:22, Jan Blunck:
Quoted 15 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */
lib/librte_eal/common/include/rte_dev.h +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+                 const char *devargs);

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?
Also possible: return a struct *rte_device and set rte_errno on error.
+1 for this option.

Given that the caller of this is usually something that injects events from the system I wonder what it is going to do with a rte_device reference. Additionally to what the caller knows anyway (name, numa_node, devargs) it can check if a driver got assigned. Sure the caller could upcast conditionally based on the busname ...

At this point I guess the control plane would anyway want to get access to a high-level object, e.g. the rte_ethdev. I believe it is better to decouple this through callbacks that can get registered with individual buses.

Thomas Monjalon

2017-06-30 16:03 UTC #87 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

30/06/2017 17:44, Jan Blunck:

Quoted 36 lines

Quoted 19 lines · Bruce Richardson
Quoted 18 lines · Gaėtan Rivet
Quoted 17 lines · Thomas Monjalon
29/06/2017 20:22, Jan Blunck:
Quoted 15 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */
lib/librte_eal/common/include/rte_dev.h +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+                 const char *devargs);
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?
Also possible: return a struct *rte_device and set rte_errno on error.
+1 for this option.
Given that the caller of this is usually something that injects events from the system I wonder what it is going to do with a rte_device reference. Additionally to what the caller knows anyway (name, numa_node, devargs) it can check if a driver got assigned. Sure the caller could upcast conditionally based on the busname ...

Quoted 4 lines

At this point I guess the control plane would anyway want to get access to a high-level object, e.g. the rte_ethdev. I believe it is better to decouple this through callbacks that can get registered with individual buses.

I think Gaetan has an example of use of rte_device after plugging with the failsafe PMD (managing slaves).
Anyway, it can be discussed later with a real example of use if needed. We have a couple of weeks before freezing the API.

Gaëtan Rivet

author

2017-06-30 16:14 UTC #88 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

Quoted 42 lines · Thomas Monjalon

30/06/2017 17:44, Jan Blunck:
Quoted 36 lines
Quoted 19 lines · Bruce Richardson
Quoted 18 lines · Gaėtan Rivet
Quoted 17 lines · Thomas Monjalon
29/06/2017 20:22, Jan Blunck:
Quoted 15 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */
lib/librte_eal/common/include/rte_dev.h +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+                 const char *devargs);
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?
Also possible: return a struct *rte_device and set rte_errno on error.
+1 for this option.
Given that the caller of this is usually something that injects events from the system I wonder what it is going to do with a rte_device reference. Additionally to what the caller knows anyway (name, numa_node, devargs) it can check if a driver got assigned. Sure the caller could upcast conditionally based on the busname ...
Quoted 4 lines
At this point I guess the control plane would anyway want to get access to a high-level object, e.g. the rte_ethdev. I believe it is better to decouple this through callbacks that can get registered with individual buses.

Quoted 4 lines

I think Gaetan has an example of use of rte_device after plugging with the failsafe PMD (managing slaves).
Anyway, it can be discussed later with a real example of use if needed. We have a couple of weeks before freezing the API.

The rte_device should be accessible from the rte_eth_dev anyway so it does not make much difference. As long as a handle on the device is available. It is of course possible to add yet another callback to search the device just plugged, but I don't see a reason here not to do it in one pass.

Signature · 2 lines

Gaëtan Rivet
6WIND

Bruce Richardson

2017-06-30 16:25 UTC #89 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

Quoted 48 lines · Gaëtan Rivet

Quoted 42 lines · Thomas Monjalon
30/06/2017 17:44, Jan Blunck:
Quoted 36 lines
Quoted 19 lines · Bruce Richardson
Quoted 18 lines · Gaėtan Rivet
Quoted 17 lines · Thomas Monjalon
29/06/2017 20:22, Jan Blunck:
Quoted 15 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */
(quoted diff) +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+                 const char *devargs);
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?
Also possible: return a struct *rte_device and set rte_errno on error.
+1 for this option.
Given that the caller of this is usually something that injects events from the system I wonder what it is going to do with a rte_device reference. Additionally to what the caller knows anyway (name, numa_node, devargs) it can check if a driver got assigned. Sure the caller could upcast conditionally based on the busname ...
Quoted 4 lines
At this point I guess the control plane would anyway want to get access to a high-level object, e.g. the rte_ethdev. I believe it is better to decouple this through callbacks that can get registered with individual buses.
Quoted 4 lines
I think Gaetan has an example of use of rte_device after plugging with the failsafe PMD (managing slaves).
Anyway, it can be discussed later with a real example of use if needed. We have a couple of weeks before freezing the API.

Quoted 6 lines

The rte_device should be accessible from the rte_eth_dev anyway so it does not make much difference. As long as a handle on the device is available. It is of course possible to add yet another callback to search the device just plugged, but I don't see a reason here not to do it in one pass.

At this point in the process we just need to get in what we can.

Given there is so much discussion I would suggest we apply what we have now, but mark the new APIs as "experimental" at this point. That should allow us to test them, and build upon them without holding us back if we do need to change them in the next release. Once everyone is happy with the final result, we can lock them down then. It seems premature to do so now, with the current discussion, but on the other hand it seems foolish to not put what work has been done thus far into a release as a starting point.

my 2c.
/Bruce

Thomas Monjalon

2017-06-30 16:29 UTC #90 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

30/06/2017 18:25, Bruce Richardson:

Quoted 57 lines · Gaëtan Rivet

Quoted 48 lines · Thomas Monjalon
Quoted 42 lines
30/06/2017 17:44, Jan Blunck:
Quoted 36 lines
Quoted 19 lines · Bruce Richardson
Quoted 18 lines · Gaėtan Rivet
Quoted 17 lines · Thomas Monjalon
29/06/2017 20:22, Jan Blunck:
Quoted 15 lines
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */
(quoted diff) +2 -0
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+                 const char *devargs);
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
After the hotplug, we may need to get the rte_device. Should we add a struct **rte_device as parameter, or should we add a helper function to get the rte_device from busname and devname?
Also possible: return a struct *rte_device and set rte_errno on error.
+1 for this option.
Given that the caller of this is usually something that injects events from the system I wonder what it is going to do with a rte_device reference. Additionally to what the caller knows anyway (name, numa_node, devargs) it can check if a driver got assigned. Sure the caller could upcast conditionally based on the busname ...
Quoted 4 lines
At this point I guess the control plane would anyway want to get access to a high-level object, e.g. the rte_ethdev. I believe it is better to decouple this through callbacks that can get registered with individual buses.
Quoted 4 lines
I think Gaetan has an example of use of rte_device after plugging with the failsafe PMD (managing slaves).
Anyway, it can be discussed later with a real example of use if needed. We have a couple of weeks before freezing the API.
Quoted 6 lines
The rte_device should be accessible from the rte_eth_dev anyway so it does not make much difference. As long as a handle on the device is available. It is of course possible to add yet another callback to search the device just plugged, but I don't see a reason here not to do it in one pass.
At this point in the process we just need to get in what we can.

Quoted 8 lines

Given there is so much discussion I would suggest we apply what we have now, but mark the new APIs as "experimental" at this point. That should allow us to test them, and build upon them without holding us back if we do need to change them in the next release. Once everyone is happy with the final result, we can lock them down then. It seems premature to do so now, with the current discussion, but on the other hand it seems foolish to not put what work has been done thus far into a release as a starting point.

I agree.
Please Jan, add EXPERIMENTAL in the doxygen of the new hotplug API for your next version. Thanks

Thomas Monjalon

2017-06-30 12:54 UTC #91 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

It seems a function is missing in this patch:

29/06/2017 20:22, Jan Blunck:

Quoted 14 lines

lib/librte_eal/common/eal_common_dev.c +14 -0

+int rte_eal_hotplug_add(const char *busname, const char *devname,
+			const char *devargs)
+{
+	struct rte_bus *bus;
+	struct rte_device *dev;
+	int ret;
+
+	bus = rte_bus_find_by_name(busname);
+	if (!bus) {
+		RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
+		return -ENOENT;
+	}
+
+	if (!bus->plug) {

lib/librte_eal/common/eal_common_dev.c int rte_eal_dev_detach(const char *name) · line 119

Quoted 10 lines

lib/librte_eal/common/eal_common_dev.c +10 -0

+		RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
+			bus->name);
+		return -ENOTSUP;
+	}
+
+	ret = bus->scan();
+	if (ret)
+		return ret;
+
+	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);

fatal error: use of undeclared identifier 'cmp_detached_dev_name'

Jan Blunck

2017-06-30 15:12 UTC #92 lore raw

Re: [PATCH v7 13/15] eal: add hotplug add/remove functions

Quoted 21 lines · Thomas Monjalon

It seems a function is missing in this patch:

29/06/2017 20:22, Jan Blunck:

Quoted 18 lines

lib/librte_eal/common/eal_common_dev.c +18 -0

+int rte_eal_hotplug_add(const char *busname, const char *devname,
+                     const char *devargs)
+{
+     struct rte_bus *bus;
+     struct rte_device *dev;
+     int ret;
+
+     bus = rte_bus_find_by_name(busname);
+     if (!bus) {
+             RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
+             return -ENOENT;
+     }
+
+     if (!bus->plug) {
+             RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
+                     bus->name);
+             return -ENOTSUP;
+     }

Quoted 9 lines

lib/librte_eal/common/eal_common_dev.c int rte_eal_dev_detach(const char *name) · line 119
Quoted 6 lines
lib/librte_eal/common/eal_common_dev.c +6 -0
+
+     ret = bus->scan();
+     if (ret)
+             return ret;
+
+     dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
fatal error: use of undeclared identifier 'cmp_detached_dev_name'

Sorry. I'll make them bisectable with the next drop today.

Jan Blunck

2017-06-29 18:22 UTC #93 lore raw

PATCH v7 14/15 dev: use new hotplug API in attach / detach

Using the new generic API allows attach and detach to be backwards compatible while decoupling from the concrete bus implementations.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_dev.c | 76 ++++++++++++++++++++++++----------
 1 file changed, 53 insertions(+), 23 deletions(-)

lib/librte_eal/common/eal_common_dev.c +53 ~~-23~~

index 477b4cf..68c6b45 100644
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_bus.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>

@@ -45,52 +46,81 @@
 
 #include "eal_private.h"
 
+static int cmp_detached_dev_name(const struct rte_device *dev,
+	const void *_name)
+{
+	const char *name = _name;
+
+	/* skip attached devices */
+	if (dev->driver)
+		return 1;
+	return strcmp(dev->name, name);
+}
+
 int rte_eal_dev_attach(const char *name, const char *devargs)
 {
-	struct rte_pci_addr addr;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	ret = rte_eal_hotplug_add("PCI", name, devargs);
+	if (ret && ret != -EINVAL)
+		return ret;
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
-	}
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a
+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}
 
-	return 0;
+static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+{
+	const char *name = _name;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", name);
-	return -EINVAL;
+	return strcmp(dev->name, name);
 }
 
 int rte_eal_dev_detach(const char *name)
 {
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	struct rte_bus *bus;
+	int ret;
 
 	if (name == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device provided.\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_detach(&addr) < 0)
-			goto err;
-	} else {
-		if (rte_vdev_uninit(name))
-			goto err;
+	dev = rte_bus_find_device(NULL, cmp_dev_name, name);
+	if (!dev) {
+		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n", name);
+		return -EINVAL;
+	}
+
+	bus = rte_bus_find_by_device(dev);
+	if (!bus) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n", name);
+		return -EINVAL;
+	}
+
+	if (!bus->unplug) {
+		RTE_LOG(ERR, EAL, "Bus function not supported\n");
+		return -ENOTSUP;
 	}
-	return 0;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n", name);
-	return -EINVAL;
+	ret = bus->unplug(dev);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
+			name);
+	return ret;
 }
 
 int rte_eal_hotplug_add(const char *busname, const char *devname,

Signature · 1 line

2.9.4

Thomas Monjalon

2017-06-29 21:05 UTC #94 lore raw

Re: [PATCH v7 14/15] dev: use new hotplug API in attach / detach

29/06/2017 20:22, Jan Blunck:

Quoted 26 lines

Using the new generic API allows attach and detach to be backwards compatible while decoupling from the concrete bus implementations.
Signed-off-by Jan Blunck <[email protected]>
---
int rte_eal_dev_attach(const char *name, const char *devargs) {
lib/librte_eal/common/eal_common_dev.c +4 -8
-	struct rte_pci_addr addr;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
+	ret = rte_eal_hotplug_add("PCI", name, devargs);
+	if (ret && ret != -EINVAL)
+		return ret;
 
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
-	}

lib/librte_eal/common/eal_common_dev.c line 82

Quoted 10 lines

lib/librte_eal/common/eal_common_dev.c +10 -0

+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a
+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
+}

I really don't like this function.
It is not really decoupled from the buses (cf "PCI" and rte_vdev_init). However I think it is acceptable if it is explictly deprecated and kept only for the legacy rte_eth_dev_attach() function.

Jan Blunck

2017-06-29 18:22 UTC #95 lore raw

PATCH v7 15/15 ethdev: Use embedded rte_device to detach driver

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map |  1 +
 lib/librte_eal/common/eal_common_dev.c        | 43 ++++++++++++++++-----------
 lib/librte_eal/common/include/rte_dev.h       |  8 +++++
 lib/librte_ether/rte_ethdev.c                 |  3 +-
 4 files changed, 36 insertions(+), 19 deletions(-)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index b7d26b2..df7abc5 100644
@@ -162,6 +162,7 @@ DPDK_17.02 {
 DPDK_17.05 {
 	global:
 
+	rte_eal_device_detach;
 	rte_bus_find;
 	rte_bus_find_by_device;
 	rte_bus_find_device;

lib/librte_eal/common/eal_common_dev.c +25 ~~-18~~

61 lines not on this page · raw message

lib/librte_eal/common/include/rte_dev.h +8 -0

index 95440eb..a3e6d7a 100644
@@ -192,6 +192,14 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
 int rte_eal_dev_detach(const char *name);
 
 /**
+ * Unplug the device from the device driver.
+ *
+ * @param dev
+ *   A pointer to a rte_device structure.
+ */
+int rte_eal_device_detach(struct rte_device *dev);
+
+/**
  * Hotplug add a given device to a specific bus.
  *
  * @param busname

lib/librte_ether/rte_ethdev.c +2 -1

index 81a45c0..89addc6 100644
@@ -439,7 +439,8 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 
 	snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
 		 "%s", rte_eth_devices[port_id].data->name);
-	ret = rte_eal_dev_detach(name);
+
+	ret = rte_eal_device_detach(rte_eth_devices[0].device);
 	if (ret < 0)
 		goto err;

Signature · 1 line

2.9.4

Thomas Monjalon

2017-06-29 20:58 UTC #96 lore raw

Re: [PATCH v7 15/15] ethdev: Use embedded rte_device to detach driver

29/06/2017 20:22, Jan Blunck:

Quoted 3 lines

lib/librte_eal/common/eal_common_dev.c +1 -0
+int rte_eal_device_detach(struct rte_device *dev)

[...]

Quoted 1 line

int rte_eal_dev_detach(const char *name)

I would be in favor of breaking the API without notice,
and keep the name rte_eal_dev_detach for the function
taking a rte_device as parameter.
This function was introduced in 16.11 and has probably never been used, except in ethdev and testpmd. Legacy apps probably use rte_eth_dev_detach.

Thomas Monjalon

2017-06-30 09:59 UTC #97 lore raw

Re: [PATCH 00/15] bus attach/detach API and hotplug rework

29/06/2017 20:21, Jan Blunck:

Quoted 18 lines

Gaetan Rivet (1):
pci: implement hotplug bus operation
Jan Blunck (14):
bus: add bus iterator to find a bus
bus: add find_device bus operation
vdev: implement find_device bus operation
pci: implement find_device bus operation bus/fslmc: implement find_device bus operation
bus: add helper to find which bus holds a device
bus: add bus iterator to find a device
bus: require buses to implement find_device operation
bus: add rte_bus_find_by_name
bus: introduce device plug/unplug functionality
vdev: implement unplug bus operation
eal: add hotplug add/remove functions
dev: use new hotplug API in attach / detach
ethdev: Use embedded rte_device to detach driver

It seems you forgot this patch in the series:
[PATCH v6 08/11] vdev: use standard bus registration function

Jan Blunck

2017-06-30 18:20 UTC #98 lore raw

v8 cover bus attach/detach API and hotplug rework

-- v7

* Revert removal of RTE_VERIFY on find_device operation since a bus that isn't able to even iterate its devices is pointless

* Revert plug() changes that have been introduced in v3:
int (*rte_bus_plug_t)(struct rte_device *dev, const char *devargs);

* Revert pointless reordering of start argument for bus iterator functions

* Add fslmc bus find_device operation

* Add support to find a bus by its name

* Add EAL hotplug API:
int rte_eal_hotplug_add(const char *busname, const char *devname,

                          const char *devargs);

int rte_eal_hotplug_remove(const char *busname, const char *devname);

* Keep rte_eal_dev_attach() backwards compatible and support hotplug of PCI and virtual devices only

* Add back patch "ethdev: use embedded rte_device to detach driver" which is critical for correctly detaching the correct low-level device

-- v8

* Rework rte_eal_dev_detach to break API and remove rte_eal_device_detach

* Drop "bus: add bus iterator to find a device" since its not actively used

* Adjust coding style to explicitly check against NULL pointer

* Mark hotplug API EXPERIMENTAL

* Fix exporting of new bus function symbols

Gaetan Rivet (1):
pci: implement hotplug bus operation

Jan Blunck (13):
bus: add bus iterator to find a bus
bus: add find_device bus operation
vdev: implement find_device bus operation
pci: implement find_device bus operation bus/fslmc: implement find_device bus operation
bus: add helper to find which bus holds a device
bus: require buses to implement find_device operation
bus: add rte_bus_find_by_name
bus: introduce device plug/unplug functionality
vdev: implement unplug bus operation
eal: add hotplug add/remove functions
ethdev: Use embedded rte_device to detach driver
dev: use new hotplug API in attach

 app/test-pmd/testpmd.c                          |   2 +-
 drivers/bus/fslmc/fslmc_bus.c                   |  23 ++++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  17 +++
 lib/librte_eal/common/eal_common_bus.c          |  59 ++++++++++
 lib/librte_eal/common/eal_common_dev.c          | 143 +++++++++++++++++++-----
 lib/librte_eal/common/eal_common_pci.c          |  55 +++++++++
 lib/librte_eal/common/eal_common_vdev.c         |  35 ++++++
 lib/librte_eal/common/include/rte_bus.h         | 111 ++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         |  62 +++++++++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  17 +++
 lib/librte_ether/rte_ethdev.c                   |   3 +-
 11 files changed, 496 insertions(+), 31 deletions(-)

--
2.9.4

Jan Blunck

2017-06-30 18:20 UTC #99 lore raw

PATCH v8 1/14 bus: add bus iterator to find a bus

This helper allows to iterate over all registered buses and find one matching data used as parameter.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  7 +++++
 lib/librte_eal/common/eal_common_bus.c          | 19 +++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 42 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  7 +++++
 4 files changed, 75 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +7 -0

index 2e48a73..edf9d7d 100644
@@ -193,3 +193,10 @@ DPDK_17.05 {
 	vfio_get_group_no;
 
 } DPDK_17.02;
+
+DPDK_17.08 {
+	global:
+
+	rte_bus_find;
+
+} DPDK_17.05;

lib/librte_eal/common/eal_common_bus.c +19 -0

index 8f9baf8..3094daa 100644
@@ -145,3 +145,22 @@ rte_bus_dump(FILE *f)
 		}
 	}
 }
+
+struct rte_bus *
+rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
+	     const void *data)
+{
+	struct rte_bus *bus = NULL;
+	bool start_found = !start;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!start_found) {
+			if (bus == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(bus, data) == 0)
+			break;
+	}
+	return bus;
+}

lib/librte_eal/common/include/rte_bus.h +42 -0

48 lines not on this page · raw message

lib/librte_eal/linuxapp/eal/rte_eal_version.map +7 -0

index 670bab3..944416e 100644
@@ -198,3 +198,10 @@ DPDK_17.05 {
 	vfio_get_group_no;
 
 } DPDK_17.02;
+
+DPDK_17.08 {
+	global:
+
+	rte_bus_find;
+
+} DPDK_17.05;

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #100 lore raw

PATCH v8 2/14 bus: add find_device bus operation

This new method allows buses to expose their devices in a controlled manner. A comparison function is provided by the user to discriminate between devices, using arbitrary data as identifier.

It is possible to start an iteration from a specific point, in order to continue a search.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/include/rte_bus.h | 27 +++++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 2 files changed, 48 insertions(+)

lib/librte_eal/common/include/rte_bus.h +27 -0

index c52b65b..052ac8d 100644
@@ -82,6 +82,32 @@ typedef int (*rte_bus_scan_t)(void);
 typedef int (*rte_bus_probe_t)(void);
 
 /**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * If the comparison function returns zero this function should stop iterating
+ * over any more devices. To continue a search the device of a previous search
+ * can be passed via the start parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to compare each device against.
+ *
+ * @param start
+ *	starting point for the iteration
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.
+ */
+typedef struct rte_device *
+(*rte_bus_find_device_t)(const struct rte_device *start, rte_dev_cmp_t cmp,
+			 const void *data);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -89,6 +115,7 @@ struct rte_bus {
 	const char *name;            /**< Name of the bus */
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
+	rte_bus_find_device_t find_device; /**< Find a device on the bus */
 };
 
 /**

lib/librte_eal/common/include/rte_dev.h +21 -0

index de20c06..04d9c28 100644
@@ -191,6 +191,27 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
  */
 int rte_eal_dev_detach(const char *name);
 
+/**
+ * Device comparison function.
+ *
+ * This type of function is used to compare an rte_device with arbitrary
+ * data.
+ *
+ * @param dev
+ *   Device handle.
+ *
+ * @param data
+ *   Data to compare against. The type of this parameter is determined by
+ *   the kind of comparison performed by the function.
+ *
+ * @return
+ *   0 if the device matches the data.
+ *   !0 if the device does not match.
+ *   <0 if ordering is possible and the device is lower than the data.
+ *   >0 if ordering is possible and the device is greater than the data.
+ */
+typedef int (*rte_dev_cmp_t)(const struct rte_device *dev, const void *data);
+
 #define RTE_PMD_EXPORT_NAME_ARRAY(n, idx) n##idx[]
 
 #define RTE_PMD_EXPORT_NAME(name, idx) \

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #101 lore raw

PATCH v8 3/14 vdev: implement find_device bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_vdev.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +21 -0

index b4db2be..9bb7427 100644
@@ -35,6 +35,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <sys/queue.h>
 
 #include <rte_eal.h>

@@ -334,9 +335,29 @@ vdev_probe(void)
 	return 0;
 }
 
+static struct rte_device *
+vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		 const void *data)
+{
+	struct rte_vdev_device *dev;
+	bool start_found = !start;
+
+	TAILQ_FOREACH(dev, &vdev_device_list, next) {
+		if (start_found == 0) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+	return NULL;
+}
+
 static struct rte_bus rte_vdev_bus = {
 	.scan = vdev_scan,
 	.probe = vdev_probe,
+	.find_device = vdev_find_device,
 };
 
 RTE_REGISTER_BUS(VIRTUAL_BUS_NAME, rte_vdev_bus);

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #102 lore raw

PATCH v8 4/14 pci: implement find_device bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_pci.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

lib/librte_eal/common/eal_common_pci.c +21 -0

index 479c62d..e449758 100644
@@ -485,10 +485,31 @@ rte_pci_remove_device(struct rte_pci_device *pci_dev)
 	TAILQ_REMOVE(&rte_pci_bus.device_list, pci_dev, next);
 }
 
+static struct rte_device *
+pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		const void *data)
+{
+	struct rte_pci_device *dev;
+	bool start_found = !start;
+
+	FOREACH_DEVICE_ON_PCIBUS(dev) {
+		if (!start_found) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+
+	return NULL;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
+		.find_device = pci_find_device,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #103 lore raw

PATCH v8 5/14 bus/fslmc: implement find_device bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 drivers/bus/fslmc/fslmc_bus.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

drivers/bus/fslmc/fslmc_bus.c +23 -0

index b24642d..1e3bbee 100644
@@ -32,6 +32,7 @@
 
 #include <string.h>
 #include <dirent.h>
+#include <stdbool.h>
 
 #include <rte_log.h>
 #include <rte_bus.h>

@@ -105,6 +106,27 @@ rte_fslmc_probe(void)
 	return ret;
 }
 
+static struct rte_device *
+rte_fslmc_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		      const void *data)
+{
+	struct rte_dpaa2_device *dev;
+	bool start_found = !start;
+
+	TAILQ_FOREACH(dev, &rte_fslmc_bus.device_list, next) {
+		if (!start_found) {
+			if (&dev->device == start)
+				start_found = 1;
+			continue;
+		}
+
+		if (cmp(&dev->device, data) == 0)
+			return &dev->device;
+	}
+
+	return NULL;
+}
+
 /*register a fslmc bus based dpaa2 driver */
 void
 rte_fslmc_driver_register(struct rte_dpaa2_driver *driver)

@@ -133,6 +155,7 @@ struct rte_fslmc_bus rte_fslmc_bus = {
 	.bus = {
 		.scan = rte_fslmc_scan,
 		.probe = rte_fslmc_probe,
+		.find_device = rte_fslmc_find_device,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_fslmc_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_fslmc_bus.driver_list),

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #104 lore raw

PATCH v8 6/14 bus: add helper to find which bus holds a device

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 23 +++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 30 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index edf9d7d..458e3a6 100644
@@ -198,5 +198,6 @@ DPDK_17.08 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 
 } DPDK_17.05;

lib/librte_eal/common/eal_common_bus.c +23 -0

index 3094daa..5d06f5d 100644
@@ -164,3 +164,26 @@ rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 	}
 	return bus;
 }
+
+static int
+cmp_rte_device(const struct rte_device *dev1, const void *_dev2)
+{
+	const struct rte_device *dev2 = _dev2;
+
+	return dev1 != dev2;
+}
+
+static int
+bus_find_device(const struct rte_bus *bus, const void *_dev)
+{
+	struct rte_device *dev;
+
+	dev = bus->find_device(NULL, cmp_rte_device, _dev);
+	return dev == NULL;
+}
+
+struct rte_bus *
+rte_bus_find_by_device(const struct rte_device *dev)
+{
+	return rte_bus_find(NULL, bus_find_device, (const void *)dev);
+}

lib/librte_eal/common/include/rte_bus.h +5 -0

index 052ac8d..f8b3215 100644
@@ -210,6 +210,11 @@ struct rte_bus *rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 			     const void *data);
 
 /**
+ * Find the registered bus for a particular device.
+ */
+struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
+
+/**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
  */

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 944416e..3c193e4 100644
@@ -203,5 +203,6 @@ DPDK_17.08 {
 	global:
 
 	rte_bus_find;
+	rte_bus_find_by_device;
 
 } DPDK_17.05;

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #105 lore raw

PATCH v8 7/14 bus: require buses to implement find_device operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c | 1 +
 1 file changed, 1 insertion(+)

lib/librte_eal/common/eal_common_bus.c +1 -0

index 5d06f5d..b8a9e30 100644
@@ -50,6 +50,7 @@ rte_bus_register(struct rte_bus *bus)
 	/* A bus should mandatorily have the scan implemented */
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
+	RTE_VERIFY(bus->find_device);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #106 lore raw

PATCH v8 8/14 bus: add rte_bus_find_by_name

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 14 ++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  5 +++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 21 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +1 -0

index 458e3a6..d138a96 100644
@@ -199,5 +199,6 @@ DPDK_17.08 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_by_name;
 
 } DPDK_17.05;

lib/librte_eal/common/eal_common_bus.c +14 -0

index b8a9e30..bf2b138 100644
@@ -188,3 +188,17 @@ rte_bus_find_by_device(const struct rte_device *dev)
 {
 	return rte_bus_find(NULL, bus_find_device, (const void *)dev);
 }
+
+static int
+cmp_bus_name(const struct rte_bus *bus, const void *_name)
+{
+	const char *name = _name;
+
+	return strcmp(bus->name, name);
+}
+
+struct rte_bus *
+rte_bus_find_by_name(const char *busname)
+{
+	return rte_bus_find(NULL, cmp_bus_name, (const void *)busname);
+}

lib/librte_eal/common/include/rte_bus.h +5 -0

index f8b3215..509292d 100644
@@ -215,6 +215,11 @@ struct rte_bus *rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
 
 /**
+ * Find the registered bus for a given name.
+ */
+struct rte_bus *rte_bus_find_by_name(const char *busname);
+
+/**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
  */

lib/librte_eal/linuxapp/eal/rte_eal_version.map +1 -0

index 3c193e4..643ec1c 100644
@@ -204,5 +204,6 @@ DPDK_17.08 {
 
 	rte_bus_find;
 	rte_bus_find_by_device;
+	rte_bus_find_by_name;
 
 } DPDK_17.05;

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #107 lore raw

PATCH v8 9/14 bus: introduce device plug/unplug functionality

This allows the buses to plug and probe specific devices. This is meant to be a building block for hotplug support.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index bf2b138..87b0c6e 100644
@@ -51,6 +51,8 @@ rte_bus_register(struct rte_bus *bus)
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
 	RTE_VERIFY(bus->find_device);
+	/* Buses supporting driver plug also require unplug. */
+	RTE_VERIFY(!bus->plug || bus->unplug);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +32 -0

index 509292d..34ea9d5 100644
@@ -108,6 +108,36 @@ typedef struct rte_device *
 			 const void *data);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @param devargs
+ *	Device declaration.
+ *
+ * @return
+ *	The pointer to a valid rte_device usable by the bus on success.
+ *	NULL on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_plug_t)(struct rte_device *dev,
+			      const char *devargs);
+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -116,6 +146,8 @@ struct rte_bus {
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
 	rte_bus_find_device_t find_device; /**< Find a device on the bus */
+	rte_bus_plug_t plug;         /**< Probe single device for drivers */
+	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 };
 
 /**

Signature · 1 line

2.9.4

Gaëtan Rivet

author

2017-06-30 18:38 UTC #108 lore raw

Re: [PATCH v8 09/14] bus: introduce device plug/unplug functionality

Quoted 34 lines · Jan Blunck

This allows the buses to plug and probe specific devices. This is meant to be a building block for hotplug support.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index bf2b138..87b0c6e 100644
@@ -51,6 +51,8 @@ rte_bus_register(struct rte_bus *bus)
 	RTE_VERIFY(bus->scan);
 	RTE_VERIFY(bus->probe);
 	RTE_VERIFY(bus->find_device);
+	/* Buses supporting driver plug also require unplug. */
+	RTE_VERIFY(!bus->plug || bus->unplug);
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +4 -0

index 509292d..34ea9d5 100644
@@ -108,6 +108,36 @@ typedef struct rte_device *
 			 const void *data);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ *
+ * @param dev

lib/librte_eal/common/include/rte_bus.h typedef struct rte_device * · line 124

Quoted 10 lines

lib/librte_eal/common/include/rte_bus.h +10 -0

+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @param devargs
+ *	Device declaration.
+ *
+ * @return
+ *	The pointer to a valid rte_device usable by the bus on success.
+ *	NULL on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_plug_t)(struct rte_device *dev,

This typedef does not match its doc.
If it is the doc that is right, the PCI and vdev implementation should be fixed as well.

Quoted 31 lines

lib/librte_eal/common/include/rte_bus.h +18 -0

+			      const char *devargs);
+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
  * A structure describing a generic bus.
  */
 struct rte_bus {

@@ -116,6 +146,8 @@ struct rte_bus {
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
 	rte_bus_find_device_t find_device; /**< Find a device on the bus */
+	rte_bus_plug_t plug;         /**< Probe single device for drivers */
+	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 };
 
 /**

Signature · 1 line

2.9.4

Signature · 2 lines

Gaëtan Rivet
6WIND

Jan Blunck

2017-06-30 18:52 UTC #109 lore raw

Re: [PATCH v8 09/14] bus: introduce device plug/unplug functionality

Quoted 41 lines · Gaëtan Rivet

Quoted 40 lines · Jan Blunck

This allows the buses to plug and probe specific devices. This is meant to be a building block for hotplug support.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

lib/librte_eal/common/eal_common_bus.c +2 -0

index bf2b138..87b0c6e 100644
@@ -51,6 +51,8 @@ rte_bus_register(struct rte_bus *bus)
      RTE_VERIFY(bus->scan);
      RTE_VERIFY(bus->probe);
      RTE_VERIFY(bus->find_device);
+     /* Buses supporting driver plug also require unplug. */
+     RTE_VERIFY(!bus->plug || bus->unplug);
 
      TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
      RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);

lib/librte_eal/common/include/rte_bus.h +10 -0

index 509292d..34ea9d5 100644
@@ -108,6 +108,36 @@ typedef struct rte_device *
                       const void *data);
 
 /**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ *
+ * @param dev
+ *   Device pointer that was returned by a previous call to find_device.
+ *
+ * @param devargs
+ *   Device declaration.
+ *
+ * @return

Quoted 9 lines

lib/librte_eal/common/include/rte_bus.h typedef struct rte_device * · line 124
Quoted 4 lines
lib/librte_eal/common/include/rte_bus.h +4 -0
+ *   The pointer to a valid rte_device usable by the bus on success.
+ *   NULL on error. rte_errno is then set.
+ */
+typedef int (*rte_bus_plug_t)(struct rte_device *dev,
This typedef does not match its doc.
If it is the doc that is right, the PCI and vdev implementation should be fixed as well.

No, its the doc that needs fixing.

lib/librte_eal/common/inc renamed from lib/librte_eal/common/include/rte_bus.h +0 -0

lude/rte_bus.h
index 34ea9d5..2f1c911 100644

lib/librte_eal/common/include/rte_bus.h +3 -3

@@ -118,8 +118,8 @@ typedef struct rte_device *
  *     Device declaration.
  *
  * @return
- *     The pointer to a valid rte_device usable by the bus on success.
- *     NULL on error. rte_errno is then set.
+ *     0 on success.
+ *     !0 on error.
  */
 typedef int (*rte_bus_plug_t)(struct rte_device *dev,
                              const char *devargs);

@@ -133,7 +133,7 @@ typedef int (*rte_bus_plug_t)(struct rte_device *dev,
  *
  * @return
  *     0 on success.
- *     !0 on error. rte_errno is then set.
+ *     !0 on error.
  */
 typedef int (*rte_bus_unplug_t)(struct rte_device *dev);

Jan Blunck

2017-06-30 18:20 UTC #110 lore raw

PATCH v8 10/14 vdev: implement unplug bus operation

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_vdev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

lib/librte_eal/common/eal_common_vdev.c +14 -0

index 9bb7427..baf3c5b 100644
@@ -39,11 +39,13 @@
 #include <sys/queue.h>
 
 #include <rte_eal.h>
+#include <rte_dev.h>
 #include <rte_bus.h>
 #include <rte_vdev.h>
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_memory.h>
+#include <rte_errno.h>
 
 /** Double linked list of virtual device drivers. */
 TAILQ_HEAD(vdev_device_list, rte_vdev_device);

@@ -354,10 +356,22 @@ vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+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);
+}
+
 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 */
+	.unplug = vdev_unplug,
 };
 
 RTE_REGISTER_BUS(VIRTUAL_BUS_NAME, rte_vdev_bus);

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #111 lore raw

PATCH v8 11/14 pci: implement hotplug bus operation

From: Gaetan Rivet <[email protected]>

---
 lib/librte_eal/common/eal_common_pci.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

lib/librte_eal/common/eal_common_pci.c +34 -0

index e449758..5ee100e 100644
@@ -47,6 +47,7 @@
 #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>

@@ -505,11 +506,44 @@ pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+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;
+}
+
+static int
+pci_unplug(struct rte_device *dev)
+{
+	struct rte_pci_device *pdev;
+
+	pdev = RTE_DEV_TO_PCI(dev);
+	if (rte_pci_detach(&pdev->addr) != 0) {
+		rte_errno = ENODEV;
+		return -1;
+	}
+	return 0;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
 		.probe = rte_pci_probe,
 		.find_device = pci_find_device,
+		.plug = pci_plug,
+		.unplug = pci_unplug,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #112 lore raw

PATCH v8 12/14 eal: add hotplug add/remove functions

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  8 +++
 lib/librte_eal/common/eal_common_dev.c          | 88 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         | 34 ++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  8 +++
 4 files changed, 138 insertions(+)

lib/librte_eal/bsdapp/eal/rte_eal_version.map +8 -0

index d138a96..0295ea9 100644
@@ -202,3 +202,11 @@ DPDK_17.08 {
 	rte_bus_find_by_name;
 
 } DPDK_17.05;
+
+EXPERIMENTAL {
+	global:
+
+	rte_eal_hotplug_add;
+	rte_eal_hotplug_remove;
+
+} DPDK_17.08;

lib/librte_eal/common/eal_common_dev.c +88 -0

103 lines not on this page · raw message

lib/librte_eal/common/include/rte_dev.h +34 -0

index 04d9c28..a0d67d0 100644
@@ -192,6 +192,40 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
 int rte_eal_dev_detach(const char *name);
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ *   The bus name the device is added to.
+ * @param devname
+ *   The device name. Based on this device name, eal will identify a driver
+ *   capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ *   Device arguments to be passed to the driver.
+ * @return
+ *   0 on success, negative on error.
+ */
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+			const char *devargs);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Hotplug remove a given device from a specific bus.
+ *
+ * @param busname
+ *   The bus name the device is removed from.
+ * @param devname
+ *   The device name being removed.
+ * @return
+ *   0 on success, negative on error.
+ */
+int rte_eal_hotplug_remove(const char *busname, const char *devname);
+
+/**
  * Device comparison function.
  *
  * This type of function is used to compare an rte_device with arbitrary

lib/librte_eal/linuxapp/eal/rte_eal_version.map +8 -0

index 643ec1c..a118fb1 100644
@@ -207,3 +207,11 @@ DPDK_17.08 {
 	rte_bus_find_by_name;
 
 } DPDK_17.05;
+
+EXPERIMENTAL {
+	global:
+
+	rte_eal_hotplug_add;
+	rte_eal_hotplug_remove;
+
+} DPDK_17.08;

Signature · 1 line

2.9.4

Jan Blunck

2017-06-30 18:20 UTC #113 lore raw

PATCH v8 13/14 ethdev: Use embedded rte_device to detach driver

This is changing the API of rte_eal_dev_detach().

Signed-off-by Jan Blunck <[email protected]>

---
 app/test-pmd/testpmd.c                  |  2 +-
 lib/librte_eal/common/eal_common_dev.c  | 32 +++++++++++++++++++-------------
 lib/librte_eal/common/include/rte_dev.h |  7 +++----
 lib/librte_ether/rte_ethdev.c           |  3 ++-
 4 files changed, 25 insertions(+), 19 deletions(-)

app/test-pmd/testpmd.c +1 -1

index b29328a..86d494b 100644
@@ -1824,7 +1824,7 @@ rmv_event_callback(void *arg)
 	else if (da->type == RTE_DEVTYPE_WHITELISTED_PCI)
 		rte_pci_device_name(&da->pci.addr, name, sizeof(name));
 	printf("removing device %s\n", name);
-	rte_eal_dev_detach(name);
+	rte_eal_dev_detach(dev->device);
 	dev->state = RTE_ETH_DEV_UNUSED;
 }

lib/librte_eal/common/eal_common_dev.c +19 ~~-13~~

index 3606332..ede68e4 100644
@@ -90,27 +90,33 @@ int rte_eal_dev_attach(const char *name, const char *devargs)
 	return -EINVAL;
 }
 
-int rte_eal_dev_detach(const char *name)
+int rte_eal_dev_detach(struct rte_device *dev)
 {
-	struct rte_pci_addr addr;
+	struct rte_bus *bus;
+	int ret;
 
-	if (name == NULL) {
+	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device provided.\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_detach(&addr) < 0)
-			goto err;
-	} else {
-		if (rte_vdev_uninit(name))
-			goto err;
+	bus = rte_bus_find_by_device(dev);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			dev->name);
+		return -EINVAL;
 	}
-	return 0;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n", name);
-	return -EINVAL;
+	if (bus->unplug == NULL) {
+		RTE_LOG(ERR, EAL, "Bus function not supported\n");
+		return -ENOTSUP;
+	}
+
+	ret = bus->unplug(dev);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
+			dev->name);
+	return ret;
 }
 
 int rte_eal_hotplug_add(const char *busname, const char *devname,

lib/librte_eal/common/include/rte_dev.h +3 -4

index a0d67d0..9f2765d 100644
@@ -183,13 +183,12 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
 /**
  * Detach a device from its driver.
  *
- * @param name
- *   Same description as for rte_eal_dev_attach().
- *   Here, eal will call the driver detaching function.
+ * @param dev
+ *   A pointer to a rte_device structure.
  * @return
  *   0 on success, negative on error.
  */
-int rte_eal_dev_detach(const char *name);
+int rte_eal_dev_detach(struct rte_device *dev);
 
 /**
  * @warning

lib/librte_ether/rte_ethdev.c +2 -1

index 81a45c0..6554378 100644
@@ -439,7 +439,8 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 
 	snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
 		 "%s", rte_eth_devices[port_id].data->name);
-	ret = rte_eal_dev_detach(name);
+
+	ret = rte_eal_dev_detach(rte_eth_devices[0].device);
 	if (ret < 0)
 		goto err;

Signature · 1 line

2.9.4

Thomas Monjalon

2017-07-03 23:17 UTC #114 lore raw

Re: [PATCH v8 13/14] ethdev: Use embedded rte_device to detach driver

30/06/2017 20:19, Jan Blunck:

lib/librte_ether/rte_ethdev.c rte_eth_dev_detach(uint8_t port_id, char *name) · line 443

Quoted 9 lines

lib/librte_ether/rte_ethdev.c +2 -1

@@ -439,7 +439,8 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 
        snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
                 "%s", rte_eth_devices[port_id].data->name);
-       ret = rte_eal_dev_detach(name);
+
+       ret = rte_eal_dev_detach(rte_eth_devices[0].device);

Obviously, I change rte_eth_devices[0] to rte_eth_devices[port_id].

Jan Blunck

2017-06-30 18:20 UTC #115 lore raw

PATCH v8 14/14 dev: use new hotplug API in attach

Using the new hotplug API allows attach to be backwards compatible while decoupling it from the concrete bus implementations.

Signed-off-by Jan Blunck <[email protected]>

---
 lib/librte_eal/common/eal_common_dev.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

lib/librte_eal/common/eal_common_dev.c +13 ~~-14~~

index ede68e4..4ee52fd 100644
@@ -67,27 +67,26 @@ 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_pci_addr addr;
+	int ret;
 
 	if (name == NULL || devargs == NULL) {
 		RTE_LOG(ERR, EAL, "Invalid device or arguments provided\n");
 		return -EINVAL;
 	}
 
-	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
-		if (rte_pci_probe_one(&addr) < 0)
-			goto err;
-
-	} else {
-		if (rte_vdev_init(name, devargs))
-			goto err;
-	}
-
-	return 0;
+	ret = rte_eal_hotplug_add("PCI", name, devargs);
+	if (ret && ret != -EINVAL)
+		return ret;
 
-err:
-	RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", name);
-	return -EINVAL;
+	/*
+	 * If we haven't found a bus device the user meant to "hotplug" a
+	 * virtual device instead.
+	 */
+	ret = rte_vdev_init(name, devargs);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+			name);
+	return ret;
 }
 
 int rte_eal_dev_detach(struct rte_device *dev)

Signature · 1 line

2.9.4

Thomas Monjalon

Applied

2017-07-03 22:59 UTC #116 lore raw

Re: [PATCH v8 00/14] bus attach/detach API and hotplug rework