[dpdk-dev] [PATCH 00/13] devargs fixes

[dpdk-dev] [PATCH 00/13] devargs fixes

[Jan Blunck]

The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
API without prior notice. This series is reworking the rte_devargs changes
in a way hopefully compliant to the new failover PMD and still keeping API
compatible with earlier releases.

Jan Blunck (13):
  Revert "devargs: make device types generic"
  devargs: fix unittest
  devargs: deprecate enum rte_devtype based functions
  pci: use scan_mode configuration
  bus: add configuration interface for buses
  devargs: use bus configuration interface to set scanning mode
  devargs: add busname string field
  devargs: use busname
  devargs: parse "bus=" argument
  pci: use busname
  vdev: use busname
  devargs: remove type field
  devargs: remove bus field

 doc/guides/rel_notes/deprecation.rst            |   7 +
 drivers/net/virtio/virtio_pci.c                 |   3 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   1 +
 lib/librte_eal/common/eal_common_bus.c          |  16 ++
 lib/librte_eal/common/eal_common_devargs.c      | 248 ++++++++++++++++--------
 lib/librte_eal/common/eal_common_options.c      |   6 +-
 lib/librte_eal/common/eal_common_pci.c          |  15 +-
 lib/librte_eal/common/eal_common_vdev.c         |   3 +-
 lib/librte_eal/common/include/rte_bus.h         |   9 +
 lib/librte_eal/common/include/rte_devargs.h     |  22 ++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |   1 +
 test/test/test_devargs.c                        |  47 +++--
 12 files changed, 253 insertions(+), 125 deletions(-)

-- 
2.13.2

[dpdk-dev] [PATCH 01/13] Revert "devargs: make device types generic"

[Jan Blunck]

This (partially) reverts commit
bd279a79366f50a4893fb84db91bbf64b56f9fb1.
---
 lib/librte_eal/common/eal_common_devargs.c  |  4 ++--
 lib/librte_eal/common/eal_common_options.c  |  6 ++---
 lib/librte_eal/common/eal_common_pci.c      |  4 ++--
 lib/librte_eal/common/eal_common_vdev.c     |  1 +
 lib/librte_eal/common/include/rte_devargs.h |  6 ++---
 test/test/test_devargs.c                    | 36 ++++++++++++++---------------
 6 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a328..92c77c30e 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -154,14 +154,14 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		goto fail;
 	devargs->type = devtype;
 	bus = devargs->bus;
-	if (devargs->type == RTE_DEVTYPE_WHITELISTED) {
+	if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI) {
 		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
 			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
 		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
 			fprintf(stderr, "ERROR: incompatible device type and bus scan mode\n");
 			goto fail;
 		}
-	} else if (devargs->type == RTE_DEVTYPE_BLACKLISTED) {
+	} else if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI) {
 		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
 			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
 		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST) {
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 075b0ea9e..84a21fefe 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -798,14 +798,14 @@ eal_parse_common_option(int opt, const char *optarg,
 	switch (opt) {
 	/* blacklist */
 	case 'b':
-		if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED,
+		if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI,
 				optarg) < 0) {
 			return -1;
 		}
 		break;
 	/* whitelist */
 	case 'w':
-		if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED,
+		if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI,
 				optarg) < 0) {
 			return -1;
 		}
@@ -901,7 +901,7 @@ eal_parse_common_option(int opt, const char *optarg,
 		break;
 
 	case OPT_VDEV_NUM:
-		if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED,
+		if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
 				optarg) < 0) {
 			return -1;
 		}
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 76bbcc853..72fcc35c2 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -198,7 +198,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	/* no initialization when blacklisted, return without error */
 	if (dev->device.devargs != NULL &&
 		dev->device.devargs->type ==
-			RTE_DEVTYPE_BLACKLISTED) {
+			RTE_DEVTYPE_BLACKLISTED_PCI) {
 		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
 			" initializing\n");
 		return 1;
@@ -405,7 +405,7 @@ rte_pci_probe(void)
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
 		else if (devargs != NULL &&
-			devargs->type == RTE_DEVTYPE_WHITELISTED)
+			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
 			ret = pci_probe_all_drivers(dev);
 		if (ret < 0) {
 			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index e00dda9aa..ff6a3b571 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -141,6 +141,7 @@ alloc_devargs(const char *name, const char *args)
 	if (!devargs)
 		return NULL;
 
+	devargs->type = RTE_DEVTYPE_VIRTUAL;
 	devargs->bus = &rte_vdev_bus;
 	if (args)
 		devargs->args = strdup(args);
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd8a..62dd67bff 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -56,9 +56,9 @@ extern "C" {
  * Type of generic device
  */
 enum rte_devtype {
-	RTE_DEVTYPE_UNDEFINED,
-	RTE_DEVTYPE_WHITELISTED,
-	RTE_DEVTYPE_BLACKLISTED,
+	RTE_DEVTYPE_WHITELISTED_PCI,
+	RTE_DEVTYPE_BLACKLISTED_PCI,
+	RTE_DEVTYPE_VIRTUAL,
 };
 
 /**
diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 149c9c926..18f54edc1 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -64,32 +64,30 @@ test_devargs(void)
 	TAILQ_INIT(&devargs_list);
 
 	/* test valid cases */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "08:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:00.1") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "0000:5:00.0") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "0000:5:00.0") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "04:00.0,arg=val") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "0000:01:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1") < 0)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED) != 4)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) != 2)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED) != 0)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 2)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_UNDEFINED) != 0)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED, "net_ring0") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, "net_ring0") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED,
-				"net_ring1,key=val,k2=val2") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, "net_ring1,key=val,k2=val2") < 0)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_UNDEFINED) != 2)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 2)
 		goto fail;
 	free_devargs_list();
 
 	/* check virtual device with argument parsing */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED,
-				"net_ring1,k1=val,k2=val2") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, "net_ring1,k1=val,k2=val2") < 0)
 		goto fail;
 	devargs = TAILQ_FIRST(&devargs_list);
 	if (strncmp(devargs->name, "net_ring1",
@@ -100,7 +98,7 @@ test_devargs(void)
 	free_devargs_list();
 
 	/* check PCI device with empty argument parsing */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "04:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "04:00.1") < 0)
 		goto fail;
 	devargs = TAILQ_FIRST(&devargs_list);
 	if (strcmp(devargs->name, "04:00.1") != 0)
@@ -110,15 +108,15 @@ test_devargs(void)
 	free_devargs_list();
 
 	/* test error case: bad PCI address */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "08:1") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "00.1") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "foo") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, ",") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "000f:0:0") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
 		goto fail;
 
 	devargs_list = save_devargs_list;
-- 
2.13.2

[dpdk-dev] [PATCH 02/13] devargs: fix unittest

[Jan Blunck]

Since the scan-mode of the bus is now based on the bus configuration it
isn't possible to have blacklisted and whitelisted devices existing for
the same bus. This fixes the unittest to reflect that.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 test/test/test_devargs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 18f54edc1..02fec8b1f 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -68,13 +68,15 @@ test_devargs(void)
 		goto fail;
 	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "0000:5:00.0") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val")
+		== 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1")
+		== 0)
 		goto fail;
 	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) != 2)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 2)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 0)
 		goto fail;
 	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 0)
 		goto fail;
-- 
2.13.2

[dpdk-dev] [PATCH 03/13] devargs: deprecate enum rte_devtype based functions

[Jan Blunck]

The enum rte_devtype will need to get extended every time we add a bus.
Mark all related functions as deprecated for 17.11.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 doc/guides/rel_notes/deprecation.rst        | 7 +++++++
 lib/librte_eal/common/include/rte_devargs.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 257dcba32..0c763d522 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -64,3 +64,10 @@ Deprecation Notices
   be removed in 17.11:
 
   - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse``
+
+* devargs: An API/ABI change is planed for 17.11 for ``struct rte_devargs`` to
+  remove ``enum rte_devtype`` so that starting from 17.08 the following
+  functions are deprecated:
+
+  - ``rte_eal_devargs_add``
+  - ``rte_eal_devargs_type_count``
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 62dd67bff..41db817cc 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -53,6 +53,7 @@ extern "C" {
 #include <rte_bus.h>
 
 /**
+ * @deprecated
  * Type of generic device
  */
 enum rte_devtype {
@@ -139,6 +140,7 @@ rte_eal_devargs_parse(const char *dev,
 		      struct rte_devargs *da);
 
 /**
+ * @deprecated
  * Add a device to the user device list
  *
  * For PCI devices, the format of arguments string is "PCI_ADDR" or
@@ -163,6 +165,7 @@ rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * @deprecated
  * Count the number of user devices of a specified type
  *
  * @param devtype
-- 
2.13.2

Re: [dpdk-dev] [PATCH 03/13] devargs: deprecate enum rte_devtype based functions

[Thomas Monjalon]

12/07/2017 01:25, Jan Blunck:
> The enum rte_devtype will need to get extended every time we add a bus.
> Mark all related functions as deprecated for 17.11.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> +* devargs: An API/ABI change is planed for 17.11 for ``struct rte_devargs`` to
> +  remove ``enum rte_devtype`` so that starting from 17.08 the following
> +  functions are deprecated:
> +
> +  - ``rte_eal_devargs_add``
> +  - ``rte_eal_devargs_type_count``

This deprecation notice is superseded by:
	http://dpdk.org/dev/patchwork/patch/27443/

[dpdk-dev] [PATCH 04/13] pci: use scan_mode configuration

[Jan Blunck]

When scanning/probing devices the bus should use its configuration instead
of looking at the devargs->type field.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_pci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 72fcc35c2..fb0e29ac4 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -197,8 +197,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 
 	/* no initialization when blacklisted, return without error */
 	if (dev->device.devargs != NULL &&
-		dev->device.devargs->type ==
-			RTE_DEVTYPE_BLACKLISTED_PCI) {
+		rte_pci_bus.bus.conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
 		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
 			" initializing\n");
 		return 1;
@@ -404,8 +403,7 @@ rte_pci_probe(void)
 		/* probe all or only whitelisted devices */
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
-		else if (devargs != NULL &&
-			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
+		else if (devargs != NULL)
 			ret = pci_probe_all_drivers(dev);
 		if (ret < 0) {
 			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
-- 
2.13.2

Re: [dpdk-dev] [PATCH 04/13] pci: use scan_mode configuration

[Gaëtan Rivet]

On Tue, Jul 11, 2017 at 07:25:03PM -0400, Jan Blunck wrote:
> When scanning/probing devices the bus should use its configuration instead
> of looking at the devargs->type field.
> 

With this patch, how do you probe a device that was previously
blacklisted?

The answers I see to this question are pretty bad, maybe you have a good
solution.

On the other hand, can you explain why you want this limitation? What
problem does this solve? You have one view of the hotplug API, I would
like to understand why you hold this view.

Regarding the rte_devargs API, it can be fixed without making the
hotplug needlessly complicated I think.

I must point out that the scan_mode (incorrectly named) is something
that will be removed next release. The probe policies will be reworked
and I don't think that the solution should be proposed as a fix a few
days before the RC2.

> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/common/eal_common_pci.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index 72fcc35c2..fb0e29ac4 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -197,8 +197,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>  
>  	/* no initialization when blacklisted, return without error */
>  	if (dev->device.devargs != NULL &&
> -		dev->device.devargs->type ==
> -			RTE_DEVTYPE_BLACKLISTED_PCI) {
> +		rte_pci_bus.bus.conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
>  		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
>  			" initializing\n");
>  		return 1;
> @@ -404,8 +403,7 @@ rte_pci_probe(void)
>  		/* probe all or only whitelisted devices */
>  		if (probe_all)
>  			ret = pci_probe_all_drivers(dev);
> -		else if (devargs != NULL &&
> -			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
> +		else if (devargs != NULL)
>  			ret = pci_probe_all_drivers(dev);
>  		if (ret < 0) {
>  			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
> -- 
> 2.13.2
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH 04/13] pci: use scan_mode configuration

[Jan Blunck]

On Thu, Jul 13, 2017 at 1:59 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> On Tue, Jul 11, 2017 at 07:25:03PM -0400, Jan Blunck wrote:
>> When scanning/probing devices the bus should use its configuration instead
>> of looking at the devargs->type field.
>>
>
> With this patch, how do you probe a device that was previously
> blacklisted?
>
> The answers I see to this question are pretty bad, maybe you have a good
> solution.
>
> On the other hand, can you explain why you want this limitation? What
> problem does this solve? You have one view of the hotplug API, I would
> like to understand why you hold this view.
>
> Regarding the rte_devargs API, it can be fixed without making the
> hotplug needlessly complicated I think.
>
> I must point out that the scan_mode (incorrectly named) is something
> that will be removed next release. The probe policies will be reworked
> and I don't think that the solution should be proposed as a fix a few
> days before the RC2.
>

You just introduced them in 17.08-rc1 and you want to remove them
again for 17.11?! Please revert these changes in this case for
17.08-rc2.

Thomas, what is your take on this?


>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>> ---
>>  lib/librte_eal/common/eal_common_pci.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
>> index 72fcc35c2..fb0e29ac4 100644
>> --- a/lib/librte_eal/common/eal_common_pci.c
>> +++ b/lib/librte_eal/common/eal_common_pci.c
>> @@ -197,8 +197,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>>
>>       /* no initialization when blacklisted, return without error */
>>       if (dev->device.devargs != NULL &&
>> -             dev->device.devargs->type ==
>> -                     RTE_DEVTYPE_BLACKLISTED_PCI) {
>> +             rte_pci_bus.bus.conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
>>               RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
>>                       " initializing\n");
>>               return 1;
>> @@ -404,8 +403,7 @@ rte_pci_probe(void)
>>               /* probe all or only whitelisted devices */
>>               if (probe_all)
>>                       ret = pci_probe_all_drivers(dev);
>> -             else if (devargs != NULL &&
>> -                     devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
>> +             else if (devargs != NULL)
>>                       ret = pci_probe_all_drivers(dev);
>>               if (ret < 0) {
>>                       RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
>> --
>> 2.13.2
>>
>
> --
> Gaėtan Rivet
> 6WIND

Re: [dpdk-dev] [PATCH 04/13] pci: use scan_mode configuration

[Thomas Monjalon]

13/07/2017 21:42, Jan Blunck:
> On Thu, Jul 13, 2017 at 1:59 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> > On Tue, Jul 11, 2017 at 07:25:03PM -0400, Jan Blunck wrote:
> >> When scanning/probing devices the bus should use its configuration instead
> >> of looking at the devargs->type field.
> >>
> >
> > With this patch, how do you probe a device that was previously
> > blacklisted?
> >
> > The answers I see to this question are pretty bad, maybe you have a good
> > solution.
> >
> > On the other hand, can you explain why you want this limitation? What
> > problem does this solve? You have one view of the hotplug API, I would
> > like to understand why you hold this view.
> >
> > Regarding the rte_devargs API, it can be fixed without making the
> > hotplug needlessly complicated I think.
> >
> > I must point out that the scan_mode (incorrectly named) is something
> > that will be removed next release. The probe policies will be reworked
> > and I don't think that the solution should be proposed as a fix a few
> > days before the RC2.
> 
> You just introduced them in 17.08-rc1 and you want to remove them
> again for 17.11?! Please revert these changes in this case for
> 17.08-rc2.

Please remember that it has been introduced to prepare the move of
the bus drivers and allow some kind of hotplug as used in failsafe,
without breaking the devargs syntax.
This is a step in an incremental process, and experimental functions
and deprecated API can be dropped for a better replacement in 17.11.

> Thomas, what is your take on this?

I did not change my mind:
We must deprecate the syntax in devargs for 17.11.
Then, with a new syntax, it will be possible to simplify a lot of things,
including the probe policies.

For now, we must work on 17.08-rc2 with two goals:
	- fix the API break introduced in devargs API
	- fix the reworked hotplug to make it work in basic PCI cases
	- make sure the new failsafe PMD can be integrated

I will integrate only the patches which clearly fix something.
Patches with justification "it would be better" will wait for 17.11.

Are these rules clear enough to let us progress together in
the 17.08 timeframe, and 17.11 cycle?
Thanks for your efforts and making 17.08 release possible.

[dpdk-dev] [PATCH 05/13] bus: add configuration interface for buses

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 16 ++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  9 +++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 27 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895cd..82f64c386 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -206,6 +206,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_bus_configure;
 	rte_eal_devargs_parse;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 08bec2d93..f5368a486 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -64,6 +64,22 @@ rte_bus_unregister(struct rte_bus *bus)
 	RTE_LOG(DEBUG, EAL, "Unregistered [%s] bus.\n", bus->name);
 }
 
+int rte_bus_configure(struct rte_bus *bus, const struct rte_bus_conf *conf)
+{
+	if (bus == NULL)
+		return -1;
+
+	/* only set bus scan policy if it was unset before */
+	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+		RTE_LOG(DEBUG, EAL, "Bus [%s] scan_mode=%d\n", bus->name,
+			conf->scan_mode);
+		bus->conf.scan_mode = conf->scan_mode;
+	} else if (bus->conf.scan_mode != conf->scan_mode)
+		return -1;
+
+	return 0;
+}
+
 /* Scan all the buses for registered devices */
 int
 rte_bus_scan(void)
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index af9f0e13f..ff67e6e93 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -206,6 +206,15 @@ void rte_bus_register(struct rte_bus *bus);
 void rte_bus_unregister(struct rte_bus *bus);
 
 /**
+ * Configure a Bus instance.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be configured.
+ */
+int rte_bus_configure(struct rte_bus *bus, const struct rte_bus_conf *conf);
+
+/**
  * Scan all the buses.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009b6..8b8c382a7 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -211,6 +211,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_bus_configure;
 	rte_eal_devargs_parse;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
-- 
2.13.2

[dpdk-dev] [PATCH 06/13] devargs: use bus configuration interface to set scanning mode

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 92c77c30e..691538095 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -137,6 +137,14 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 	return 0;
 }
 
+static const struct rte_bus_conf BUS_CONF_WHITELIST = {
+	.scan_mode = RTE_BUS_SCAN_WHITELIST,
+};
+
+static const struct rte_bus_conf BUS_CONF_BLACKLIST = {
+	.scan_mode = RTE_BUS_SCAN_BLACKLIST,
+};
+
 /* store a whitelist parameter for later parsing */
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
@@ -144,6 +152,7 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	struct rte_devargs *devargs = NULL;
 	struct rte_bus *bus = NULL;
 	const char *dev = devargs_str;
+	int ret;
 
 	/* use calloc instead of rte_zmalloc as it's called early at init */
 	devargs = calloc(1, sizeof(*devargs));
@@ -154,20 +163,13 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		goto fail;
 	devargs->type = devtype;
 	bus = devargs->bus;
-	if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
-			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
-			fprintf(stderr, "ERROR: incompatible device type and bus scan mode\n");
-			goto fail;
-		}
-	} else if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
-			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST) {
-			fprintf(stderr, "ERROR: incompatible device type and bus scan mode\n");
-			goto fail;
-		}
+	ret = rte_bus_configure(bus,
+		devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
+		&BUS_CONF_WHITELIST : &BUS_CONF_WHITELIST);
+	if (ret != 0) {
+		RTE_LOG(ERR, EAL,
+			"incompatible device type and bus scan mode\n");
+		goto fail;
 	}
 
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
-- 
2.13.2

[dpdk-dev] [PATCH 07/13] devargs: add busname string field

[Jan Blunck]

This adds the busname as a string to struct rte_devargs. The function
rte_eal_devargs_add() is adding the busname based on the devtype which
is ok for now since the function is deprecated anyway.

As a side-effect this is also no longer validating the PCI device name.
This was done only for PCI devices anyway but didn't guarantee that this
device exists.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c  | 89 +++++++++++++++++++++--------
 lib/librte_eal/common/include/rte_devargs.h |  9 ++-
 test/test/test_devargs.c                    | 12 ----
 3 files changed, 71 insertions(+), 39 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 691538095..f9f23f5fd 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -77,6 +77,42 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 	return 0;
 }
 
+static struct rte_devargs *
+devargs_alloc(const char *busname, const char *name, const char *args)
+{
+	struct rte_devargs *devargs;
+	int ret;
+
+	if (busname == NULL || name == NULL || args == NULL)
+		return NULL;
+
+	/* use calloc instead of rte_zmalloc as it's called early at init */
+	devargs = calloc(1, sizeof(*devargs));
+	if (devargs == NULL)
+		goto fail;
+
+	ret = snprintf(devargs->busname, sizeof(devargs->busname), "%s",
+		busname);
+	if (ret < 0 || ret >= (int)sizeof(devargs->busname))
+		goto fail;
+
+	ret = snprintf(devargs->name, sizeof(devargs->name), "%s", name);
+	if (ret < 0 || ret >= (int)sizeof(devargs->name))
+		goto fail;
+
+	devargs->args = strdup(args);
+	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
+	return devargs;
+
+fail:
+	if (devargs != NULL) {
+		free(devargs->args);
+		free(devargs);
+	}
+
+	return NULL;
+}
+
 static int
 bus_name_cmp(const struct rte_bus *bus, const void *name)
 {
@@ -150,38 +186,43 @@ int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
 	struct rte_devargs *devargs = NULL;
-	struct rte_bus *bus = NULL;
-	const char *dev = devargs_str;
+	const char *busname = NULL;
+	char *name = NULL;
+	char *args = NULL;
 	int ret;
 
-	/* use calloc instead of rte_zmalloc as it's called early at init */
-	devargs = calloc(1, sizeof(*devargs));
-	if (devargs == NULL)
+	ret = rte_eal_parse_devargs_str(devargs_str, &name, &args);
+	if (ret != 0)
 		goto fail;
 
-	if (rte_eal_devargs_parse(dev, devargs))
-		goto fail;
-	devargs->type = devtype;
-	bus = devargs->bus;
-	ret = rte_bus_configure(bus,
-		devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
-		&BUS_CONF_WHITELIST : &BUS_CONF_WHITELIST);
-	if (ret != 0) {
-		RTE_LOG(ERR, EAL,
-			"incompatible device type and bus scan mode\n");
-		goto fail;
+	switch (devtype) {
+	case RTE_DEVTYPE_WHITELISTED_PCI:
+	case RTE_DEVTYPE_BLACKLISTED_PCI:
+		busname = "pci";
+		ret = rte_bus_configure(rte_bus_find_by_name(busname),
+			devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
+			&BUS_CONF_WHITELIST : &BUS_CONF_BLACKLIST);
+		if (ret != 0) {
+			RTE_LOG(ERR, EAL,
+				"Bus [%s] scan_mode conflicts with devtype: "
+				"%s\n", busname, name);
+			goto fail;
+		}
+		break;
+	case RTE_DEVTYPE_VIRTUAL:
+		busname = "vdev";
+		break;
 	}
 
-	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
-	return 0;
+	devargs = devargs_alloc(busname, name, args);
+	if (devargs != NULL)
+		devargs->type = devtype;
+	ret = devargs == NULL ? -1 : 0;
 
 fail:
-	if (devargs) {
-		free(devargs->args);
-		free(devargs);
-	}
-
-	return -1;
+	free(name);
+	free(args);
+	return ret;
 }
 
 /* count the number of devices of a specified type */
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 41db817cc..29631cbaa 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -79,6 +79,8 @@ struct rte_devargs {
 	enum rte_devtype type;
 	/** Bus handle for the device. */
 	struct rte_bus *bus;
+	/** Name of the bus the device belongs to. */
+	char busname[RTE_DEV_NAME_MAX_LEN];
 	/** Name of the device. */
 	char name[RTE_DEV_NAME_MAX_LEN];
 	/** Arguments string as given by user or "" for no argument. */
@@ -149,9 +151,10 @@ rte_eal_devargs_parse(const char *dev,
  *
  * For virtual devices, the format of arguments string is "DRIVER_NAME*"
  * or "DRIVER_NAME*,key=val,key2=val2,...". Examples: "net_ring",
- * "net_ring0", "net_pmdAnything,arg=0:arg2=1". The validity of the
- * driver name is not checked by this function, it is done when probing
- * the drivers.
+ * "net_ring0", "net_pmdAnything,arg=0:arg2=1".
+ *
+ * The validity of the device name is not checked by this function, it is
+ * done when probing the drivers.
  *
  * @param devtype
  *   The type of the device.
diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 02fec8b1f..048b3d00b 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -109,18 +109,6 @@ test_devargs(void)
 		goto fail;
 	free_devargs_list();
 
-	/* test error case: bad PCI address */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
-		goto fail;
-
 	devargs_list = save_devargs_list;
 	return 0;
 
-- 
2.13.2

Re: [dpdk-dev] [PATCH 07/13] devargs: add busname string field

[Gaëtan Rivet]

On Tue, Jul 11, 2017 at 07:25:06PM -0400, Jan Blunck wrote:
> This adds the busname as a string to struct rte_devargs. The function
> rte_eal_devargs_add() is adding the busname based on the devtype which
> is ok for now since the function is deprecated anyway.
> 
> As a side-effect this is also no longer validating the PCI device name.
> This was done only for PCI devices anyway but didn't guarantee that this
> device exists.
> 

Why add the bus name as a string instead of using the bus handle already
present within the rte_devargs?

I don't understand the need for this commit, can you explain the problem
you are solving?

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH 08/13] devargs: use busname

[Jan Blunck]

This makes the devargs code itself require the rte_devargs type field for
properly functioning.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 42 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index f9f23f5fd..2bdee9a30 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -77,14 +77,14 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 	return 0;
 }
 
-static struct rte_devargs *
-devargs_alloc(const char *busname, const char *name, const char *args)
+static int
+devargs_add(const char *busname, const char *name, const char *args)
 {
 	struct rte_devargs *devargs;
 	int ret;
 
 	if (busname == NULL || name == NULL || args == NULL)
-		return NULL;
+		return -1;
 
 	/* use calloc instead of rte_zmalloc as it's called early at init */
 	devargs = calloc(1, sizeof(*devargs));
@@ -102,7 +102,7 @@ devargs_alloc(const char *busname, const char *name, const char *args)
 
 	devargs->args = strdup(args);
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
-	return devargs;
+	return 0;
 
 fail:
 	if (devargs != NULL) {
@@ -110,7 +110,7 @@ devargs_alloc(const char *busname, const char *name, const char *args)
 		free(devargs);
 	}
 
-	return NULL;
+	return -1;
 }
 
 static int
@@ -185,7 +185,6 @@ static const struct rte_bus_conf BUS_CONF_BLACKLIST = {
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
-	struct rte_devargs *devargs = NULL;
 	const char *busname = NULL;
 	char *name = NULL;
 	char *args = NULL;
@@ -214,10 +213,7 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		break;
 	}
 
-	devargs = devargs_alloc(busname, name, args);
-	if (devargs != NULL)
-		devargs->type = devtype;
-	ret = devargs == NULL ? -1 : 0;
+	ret = devargs_add(busname, name, args);
 
 fail:
 	free(name);
@@ -229,13 +225,28 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
 {
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	const char *busname = "";
 	struct rte_devargs *devargs;
 	unsigned int count = 0;
 
+	switch (devtype) {
+	case RTE_DEVTYPE_WHITELISTED_PCI:
+		if (pci_bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST)
+			busname = "pci";
+		break;
+	case RTE_DEVTYPE_BLACKLISTED_PCI:
+		if (pci_bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST)
+			busname = "pci";
+		break;
+	case RTE_DEVTYPE_VIRTUAL:
+		busname = "vdev";
+		break;
+	}
+
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
-		if (devargs->type != devtype)
-			continue;
-		count++;
+		if (strcmp(busname, devargs->busname) == 0)
+			count++;
 	}
 	return count;
 }
@@ -248,8 +259,7 @@ rte_eal_devargs_dump(FILE *f)
 
 	fprintf(f, "User device list:\n");
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
-		fprintf(f, "  [%s]: %s %s\n",
-			(devargs->bus ? devargs->bus->name : "??"),
-			devargs->name, devargs->args);
+		fprintf(f, "  [%s]: %s %s\n", devargs->busname,	devargs->name,
+			devargs->args);
 	}
 }
-- 
2.13.2

[dpdk-dev] [PATCH 09/13] devargs: parse "bus=" argument

[Jan Blunck]

Let the rte_eal_devargs_parse() function parse the "bus=" argument.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
 test/test/test_devargs.c                   |  19 +++++
 2 files changed, 102 insertions(+), 48 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 2bdee9a30..a0d0e6e91 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 	return 0;
 }
 
+/*
+ * Parse "bus=" argument without adding a dependency on rte_kvargs.h
+ */
+static char *parse_bus_arg(const char *args)
+{
+	const char *c;
+	char *str = NULL;
+
+	if (!args || args[0] == '\0')
+		return NULL;
+
+	c = args;
+
+	do {
+		if (strncmp(c, "bus=", 4) == 0) {
+			c += 4;
+			break;
+		}
+
+		c = strchr(c, ',');
+		if (c)
+			c++;
+	} while (c);
+
+	if (c) {
+		str = strdup(c);
+		c = strchr(str, ',');
+		if (c)
+			c = '\0';
+	}
+
+	return str;
+}
+
 static int
 devargs_add(const char *busname, const char *name, const char *args)
 {
@@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
 	return -1;
 }
 
-static int
-bus_name_cmp(const struct rte_bus *bus, const void *name)
-{
-	return strncmp(bus->name, name, strlen(bus->name));
-}
-
 int
 rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 {
-	struct rte_bus *bus = NULL;
-	const char *devname;
-	const size_t maxlen = sizeof(da->name);
-	size_t i;
+	char *busname;
+	char *devname;
+	char *drvargs;
+	int ret;
 
 	if (dev == NULL || da == NULL)
 		return -EINVAL;
-	/* Retrieve eventual bus info */
-	do {
-		devname = dev;
-		bus = rte_bus_find(bus, bus_name_cmp, dev);
-		if (bus == NULL)
-			break;
-		devname = dev + strlen(bus->name) + 1;
-		if (rte_bus_find_by_device_name(devname) == bus)
-			break;
-	} while (1);
-	/* Store device name */
-	i = 0;
-	while (devname[i] != '\0' && devname[i] != ',') {
-		da->name[i] = devname[i];
-		i++;
-		if (i == maxlen) {
-			fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
-				dev, maxlen);
-			da->name[i - 1] = '\0';
-			return -EINVAL;
-		}
-	}
-	da->name[i] = '\0';
-	if (bus == NULL) {
-		bus = rte_bus_find_by_device_name(da->name);
+
+	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
+	if (ret != 0)
+		return ret;
+
+	RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
+		__FUNCTION__, devname, drvargs);
+
+	/* Retrieve eventual bus info (...,bus=...)*/
+	busname = parse_bus_arg(drvargs);
+	if (busname == NULL) {
+		struct rte_bus *bus;
+
+		bus = rte_bus_find_by_device_name(devname);
 		if (bus == NULL) {
-			fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
-				da->name);
-			return -EFAULT;
+			RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
+				devname);
+			ret = -EINVAL;
+			goto fail;
 		}
+
+		busname = strdup(bus->name);
 	}
-	da->bus = bus;
-	/* Parse eventual device arguments */
-	if (devname[i] == ',')
-		da->args = strdup(&devname[i + 1]);
-	else
-		da->args = strdup("");
-	if (da->args == NULL) {
-		fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
-		return -ENOMEM;
+
+	ret = devargs_add(busname, devname, drvargs);
+	if (ret != 0) {
+		RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
+			devname);
+		goto fail;
 	}
-	return 0;
+
+	ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
+	if (ret < 0 || ret >= (int)sizeof(da->busname))
+		goto fail;
+
+	ret = snprintf(da->name, sizeof(da->name), "%s", devname);
+	if (ret < 0 || ret >= (int)sizeof(da->name))
+		goto fail;
+
+	da->args = strdup(drvargs);
+	if (da->args == NULL)
+		ret = -ENOMEM;
+	ret = 0;
+
+fail:
+	free(busname);
+	free(devname);
+	free(drvargs);
+	return ret;
 }
 
 static const struct rte_bus_conf BUS_CONF_WHITELIST = {
diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 048b3d00b..3a1033ca3 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -58,6 +58,7 @@ test_devargs(void)
 {
 	struct rte_devargs_list save_devargs_list;
 	struct rte_devargs *devargs;
+	struct rte_devargs devargs_stack;
 
 	/* save the real devargs_list, it is restored at the end of the test */
 	save_devargs_list = devargs_list;
@@ -109,6 +110,24 @@ test_devargs(void)
 		goto fail;
 	free_devargs_list();
 
+	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
+		printf("Error in rte_eal_devargs_parse\n");
+		goto fail;
+	}
+
+	if (rte_eal_devargs_parse("08:00.1,foo=bar,bus=pci", &devargs_stack) != 0) {
+		printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
+		goto fail;
+	}
+	devargs = TAILQ_FIRST(&devargs_list);
+	if (strcmp(devargs->name, "08:00.1") != 0)
+		goto fail;
+	if (strcmp(devargs->busname, "pci") != 0)
+		goto fail;
+	if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
+		goto fail;
+
+	free_devargs_list();
 	devargs_list = save_devargs_list;
 	return 0;
 
-- 
2.13.2

Re: [dpdk-dev] [PATCH 09/13] devargs: parse "bus=" argument

[Gaëtan Rivet]

I still disagree with parsing the body of devargs to retrieve bus
information. They should not be mixed.

This conceptual discrepancy can be seen plainly in the code structure:
you emulate librte_kvargs because you do not want to depend on it within
the EAL, but copy its behavior and are forced to rewrite it partially.

Can you justify this? The commitlog does not describe the problem being
solved.

I think there should be a discussion about the future format of device
declarations. One version has been integrated in RC1 but it will be reworked
anyway. I'd like to hear more opinions.

On Tue, Jul 11, 2017 at 07:25:08PM -0400, Jan Blunck wrote:
> Let the rte_eal_devargs_parse() function parse the "bus=" argument.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
>  test/test/test_devargs.c                   |  19 +++++
>  2 files changed, 102 insertions(+), 48 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 2bdee9a30..a0d0e6e91 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>  	return 0;
>  }
>  
> +/*
> + * Parse "bus=" argument without adding a dependency on rte_kvargs.h
> + */
> +static char *parse_bus_arg(const char *args)
> +{
> +	const char *c;
> +	char *str = NULL;
> +
> +	if (!args || args[0] == '\0')
> +		return NULL;
> +
> +	c = args;
> +
> +	do {
> +		if (strncmp(c, "bus=", 4) == 0) {
> +			c += 4;
> +			break;
> +		}
> +
> +		c = strchr(c, ',');
> +		if (c)
> +			c++;
> +	} while (c);
> +
> +	if (c) {
> +		str = strdup(c);
> +		c = strchr(str, ',');
> +		if (c)
> +			c = '\0';
> +	}
> +
> +	return str;
> +}
> +
>  static int
>  devargs_add(const char *busname, const char *name, const char *args)
>  {
> @@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
>  	return -1;
>  }
>  
> -static int
> -bus_name_cmp(const struct rte_bus *bus, const void *name)
> -{
> -	return strncmp(bus->name, name, strlen(bus->name));
> -}
> -
>  int
>  rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>  {
> -	struct rte_bus *bus = NULL;
> -	const char *devname;
> -	const size_t maxlen = sizeof(da->name);
> -	size_t i;
> +	char *busname;
> +	char *devname;
> +	char *drvargs;
> +	int ret;
>  
>  	if (dev == NULL || da == NULL)
>  		return -EINVAL;
> -	/* Retrieve eventual bus info */
> -	do {
> -		devname = dev;
> -		bus = rte_bus_find(bus, bus_name_cmp, dev);
> -		if (bus == NULL)
> -			break;
> -		devname = dev + strlen(bus->name) + 1;
> -		if (rte_bus_find_by_device_name(devname) == bus)
> -			break;
> -	} while (1);
> -	/* Store device name */
> -	i = 0;
> -	while (devname[i] != '\0' && devname[i] != ',') {
> -		da->name[i] = devname[i];
> -		i++;
> -		if (i == maxlen) {
> -			fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
> -				dev, maxlen);
> -			da->name[i - 1] = '\0';
> -			return -EINVAL;
> -		}
> -	}
> -	da->name[i] = '\0';
> -	if (bus == NULL) {
> -		bus = rte_bus_find_by_device_name(da->name);
> +
> +	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
> +	if (ret != 0)
> +		return ret;
> +
> +	RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
> +		__FUNCTION__, devname, drvargs);
> +
> +	/* Retrieve eventual bus info (...,bus=...)*/
> +	busname = parse_bus_arg(drvargs);
> +	if (busname == NULL) {
> +		struct rte_bus *bus;
> +
> +		bus = rte_bus_find_by_device_name(devname);
>  		if (bus == NULL) {
> -			fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
> -				da->name);
> -			return -EFAULT;
> +			RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
> +				devname);
> +			ret = -EINVAL;
> +			goto fail;
>  		}
> +
> +		busname = strdup(bus->name);
>  	}
> -	da->bus = bus;
> -	/* Parse eventual device arguments */
> -	if (devname[i] == ',')
> -		da->args = strdup(&devname[i + 1]);
> -	else
> -		da->args = strdup("");
> -	if (da->args == NULL) {
> -		fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
> -		return -ENOMEM;
> +
> +	ret = devargs_add(busname, devname, drvargs);
> +	if (ret != 0) {
> +		RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
> +			devname);
> +		goto fail;
>  	}
> -	return 0;
> +
> +	ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
> +	if (ret < 0 || ret >= (int)sizeof(da->busname))
> +		goto fail;
> +
> +	ret = snprintf(da->name, sizeof(da->name), "%s", devname);
> +	if (ret < 0 || ret >= (int)sizeof(da->name))
> +		goto fail;
> +
> +	da->args = strdup(drvargs);
> +	if (da->args == NULL)
> +		ret = -ENOMEM;
> +	ret = 0;
> +
> +fail:
> +	free(busname);
> +	free(devname);
> +	free(drvargs);
> +	return ret;
>  }
>  
>  static const struct rte_bus_conf BUS_CONF_WHITELIST = {
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 048b3d00b..3a1033ca3 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -58,6 +58,7 @@ test_devargs(void)
>  {
>  	struct rte_devargs_list save_devargs_list;
>  	struct rte_devargs *devargs;
> +	struct rte_devargs devargs_stack;
>  
>  	/* save the real devargs_list, it is restored at the end of the test */
>  	save_devargs_list = devargs_list;
> @@ -109,6 +110,24 @@ test_devargs(void)
>  		goto fail;
>  	free_devargs_list();
>  
> +	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
> +		printf("Error in rte_eal_devargs_parse\n");
> +		goto fail;
> +	}
> +
> +	if (rte_eal_devargs_parse("08:00.1,foo=bar,bus=pci", &devargs_stack) != 0) {
> +		printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
> +		goto fail;
> +	}
> +	devargs = TAILQ_FIRST(&devargs_list);
> +	if (strcmp(devargs->name, "08:00.1") != 0)
> +		goto fail;
> +	if (strcmp(devargs->busname, "pci") != 0)
> +		goto fail;
> +	if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
> +		goto fail;
> +
> +	free_devargs_list();
>  	devargs_list = save_devargs_list;
>  	return 0;
>  
> -- 
> 2.13.2
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH 09/13] devargs: parse "bus=" argument

[Jan Blunck]

On Thu, Jul 13, 2017 at 9:40 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> I still disagree with parsing the body of devargs to retrieve bus
> information. They should not be mixed.
>

I agree. I always said that it should be passed explicitly. If we
agree on this I'll fix the signature of rte_eal_devargs_parse()
accordingly.

> This conceptual discrepancy can be seen plainly in the code structure:
> you emulate librte_kvargs because you do not want to depend on it within
> the EAL, but copy its behavior and are forced to rewrite it partially.
>
> Can you justify this? The commitlog does not describe the problem being
> solved.
>
> I think there should be a discussion about the future format of device
> declarations. One version has been integrated in RC1 but it will be reworked
> anyway. I'd like to hear more opinions.
>
> On Tue, Jul 11, 2017 at 07:25:08PM -0400, Jan Blunck wrote:
>> Let the rte_eal_devargs_parse() function parse the "bus=" argument.
>>
>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>> ---
>>  lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
>>  test/test/test_devargs.c                   |  19 +++++
>>  2 files changed, 102 insertions(+), 48 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
>> index 2bdee9a30..a0d0e6e91 100644
>> --- a/lib/librte_eal/common/eal_common_devargs.c
>> +++ b/lib/librte_eal/common/eal_common_devargs.c
>> @@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>>       return 0;
>>  }
>>
>> +/*
>> + * Parse "bus=" argument without adding a dependency on rte_kvargs.h
>> + */
>> +static char *parse_bus_arg(const char *args)
>> +{
>> +     const char *c;
>> +     char *str = NULL;
>> +
>> +     if (!args || args[0] == '\0')
>> +             return NULL;
>> +
>> +     c = args;
>> +
>> +     do {
>> +             if (strncmp(c, "bus=", 4) == 0) {
>> +                     c += 4;
>> +                     break;
>> +             }
>> +
>> +             c = strchr(c, ',');
>> +             if (c)
>> +                     c++;
>> +     } while (c);
>> +
>> +     if (c) {
>> +             str = strdup(c);
>> +             c = strchr(str, ',');
>> +             if (c)
>> +                     c = '\0';
>> +     }
>> +
>> +     return str;
>> +}
>> +
>>  static int
>>  devargs_add(const char *busname, const char *name, const char *args)
>>  {
>> @@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
>>       return -1;
>>  }
>>
>> -static int
>> -bus_name_cmp(const struct rte_bus *bus, const void *name)
>> -{
>> -     return strncmp(bus->name, name, strlen(bus->name));
>> -}
>> -
>>  int
>>  rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>>  {
>> -     struct rte_bus *bus = NULL;
>> -     const char *devname;
>> -     const size_t maxlen = sizeof(da->name);
>> -     size_t i;
>> +     char *busname;
>> +     char *devname;
>> +     char *drvargs;
>> +     int ret;
>>
>>       if (dev == NULL || da == NULL)
>>               return -EINVAL;
>> -     /* Retrieve eventual bus info */
>> -     do {
>> -             devname = dev;
>> -             bus = rte_bus_find(bus, bus_name_cmp, dev);
>> -             if (bus == NULL)
>> -                     break;
>> -             devname = dev + strlen(bus->name) + 1;
>> -             if (rte_bus_find_by_device_name(devname) == bus)
>> -                     break;
>> -     } while (1);
>> -     /* Store device name */
>> -     i = 0;
>> -     while (devname[i] != '\0' && devname[i] != ',') {
>> -             da->name[i] = devname[i];
>> -             i++;
>> -             if (i == maxlen) {
>> -                     fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
>> -                             dev, maxlen);
>> -                     da->name[i - 1] = '\0';
>> -                     return -EINVAL;
>> -             }
>> -     }
>> -     da->name[i] = '\0';
>> -     if (bus == NULL) {
>> -             bus = rte_bus_find_by_device_name(da->name);
>> +
>> +     ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
>> +             __FUNCTION__, devname, drvargs);
>> +
>> +     /* Retrieve eventual bus info (...,bus=...)*/
>> +     busname = parse_bus_arg(drvargs);
>> +     if (busname == NULL) {
>> +             struct rte_bus *bus;
>> +
>> +             bus = rte_bus_find_by_device_name(devname);
>>               if (bus == NULL) {
>> -                     fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
>> -                             da->name);
>> -                     return -EFAULT;
>> +                     RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
>> +                             devname);
>> +                     ret = -EINVAL;
>> +                     goto fail;
>>               }
>> +
>> +             busname = strdup(bus->name);
>>       }
>> -     da->bus = bus;
>> -     /* Parse eventual device arguments */
>> -     if (devname[i] == ',')
>> -             da->args = strdup(&devname[i + 1]);
>> -     else
>> -             da->args = strdup("");
>> -     if (da->args == NULL) {
>> -             fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
>> -             return -ENOMEM;
>> +
>> +     ret = devargs_add(busname, devname, drvargs);
>> +     if (ret != 0) {
>> +             RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
>> +                     devname);
>> +             goto fail;
>>       }
>> -     return 0;
>> +
>> +     ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
>> +     if (ret < 0 || ret >= (int)sizeof(da->busname))
>> +             goto fail;
>> +
>> +     ret = snprintf(da->name, sizeof(da->name), "%s", devname);
>> +     if (ret < 0 || ret >= (int)sizeof(da->name))
>> +             goto fail;
>> +
>> +     da->args = strdup(drvargs);
>> +     if (da->args == NULL)
>> +             ret = -ENOMEM;
>> +     ret = 0;
>> +
>> +fail:
>> +     free(busname);
>> +     free(devname);
>> +     free(drvargs);
>> +     return ret;
>>  }
>>
>>  static const struct rte_bus_conf BUS_CONF_WHITELIST = {
>> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
>> index 048b3d00b..3a1033ca3 100644
>> --- a/test/test/test_devargs.c
>> +++ b/test/test/test_devargs.c
>> @@ -58,6 +58,7 @@ test_devargs(void)
>>  {
>>       struct rte_devargs_list save_devargs_list;
>>       struct rte_devargs *devargs;
>> +     struct rte_devargs devargs_stack;
>>
>>       /* save the real devargs_list, it is restored at the end of the test */
>>       save_devargs_list = devargs_list;
>> @@ -109,6 +110,24 @@ test_devargs(void)
>>               goto fail;
>>       free_devargs_list();
>>
>> +     if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
>> +             printf("Error in rte_eal_devargs_parse\n");
>> +             goto fail;
>> +     }
>> +
>> +     if (rte_eal_devargs_parse("08:00.1,foo=bar,bus=pci", &devargs_stack) != 0) {
>> +             printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
>> +             goto fail;
>> +     }
>> +     devargs = TAILQ_FIRST(&devargs_list);
>> +     if (strcmp(devargs->name, "08:00.1") != 0)
>> +             goto fail;
>> +     if (strcmp(devargs->busname, "pci") != 0)
>> +             goto fail;
>> +     if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
>> +             goto fail;
>> +
>> +     free_devargs_list();
>>       devargs_list = save_devargs_list;
>>       return 0;
>>
>> --
>> 2.13.2
>>
>
> --
> Gaėtan Rivet
> 6WIND

[dpdk-dev] [PATCH 10/13] pci: use busname

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/virtio/virtio_pci.c        | 3 +--
 lib/librte_eal/common/eal_common_pci.c | 9 +++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index e6eda75b6..a81322969 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -685,8 +685,7 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 	if (rte_pci_ioport_map(dev, 0, VTPCI_IO(hw)) < 0) {
 		if (dev->kdrv == RTE_KDRV_UNKNOWN &&
 		    (!dev->device.devargs ||
-		     dev->device.devargs->bus !=
-		     rte_bus_find_by_name("pci"))) {
+                     strcmp("pci", dev->device.devargs->busname) != 0)) {
 			PMD_INIT_LOG(INFO,
 				"skip kernel managed virtio device.");
 			return 1;
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index fb0e29ac4..834db50de 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -71,17 +71,18 @@ const char *pci_get_sysfs_path(void)
 	return path;
 }
 
+static int pci_parse(const char *name, void *addr);
+
 static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 {
 	struct rte_devargs *devargs;
 	struct rte_pci_addr addr;
-	struct rte_bus *pbus;
 
-	pbus = rte_bus_find_by_name("pci");
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
-		if (devargs->bus != pbus)
+		if (strcmp(devargs->busname, rte_pci_bus.bus.name) != 0)
+			continue;
+		if (pci_parse(devargs->name, &addr) != 0)
 			continue;
-		devargs->bus->parse(devargs->name, &addr);
 		if (!rte_eal_compare_pci_addr(&dev->addr, &addr))
 			return devargs;
 	}
-- 
2.13.2

[dpdk-dev] [PATCH 11/13] vdev: use busname

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_vdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index ff6a3b571..d04015582 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -260,7 +260,7 @@ vdev_scan(void)
 	/* for virtual devices we scan the devargs_list populated via cmdline */
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
 
-		if (devargs->bus != &rte_vdev_bus)
+		if (strcmp(devargs->busname, rte_vdev_bus.name) != 0)
 			continue;
 
 		dev = find_vdev(devargs->name);
-- 
2.13.2

[dpdk-dev] [PATCH 12/13] devargs: remove type field

[Jan Blunck]

This removes the enum rte_devtype field ``type`` from struct rte_devargs.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_vdev.c     | 1 -
 lib/librte_eal/common/include/rte_devargs.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index d04015582..c785fcc79 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -141,7 +141,6 @@ alloc_devargs(const char *name, const char *args)
 	if (!devargs)
 		return NULL;
 
-	devargs->type = RTE_DEVTYPE_VIRTUAL;
 	devargs->bus = &rte_vdev_bus;
 	if (args)
 		devargs->args = strdup(args);
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 29631cbaa..6977c4fc4 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -75,8 +75,6 @@ enum rte_devtype {
 struct rte_devargs {
 	/** Next in list. */
 	TAILQ_ENTRY(rte_devargs) next;
-	/** Type of device. */
-	enum rte_devtype type;
 	/** Bus handle for the device. */
 	struct rte_bus *bus;
 	/** Name of the bus the device belongs to. */
-- 
2.13.2

[dpdk-dev] [PATCH 13/13] devargs: remove bus field

[Jan Blunck]

This removes the enum rte_bus field ``bus`` from struct rte_devargs.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_vdev.c     | 1 -
 lib/librte_eal/common/include/rte_devargs.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index c785fcc79..b33f15b03 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -141,7 +141,6 @@ alloc_devargs(const char *name, const char *args)
 	if (!devargs)
 		return NULL;
 
-	devargs->bus = &rte_vdev_bus;
 	if (args)
 		devargs->args = strdup(args);
 	else
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 6977c4fc4..db9b0eb74 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -75,8 +75,6 @@ enum rte_devtype {
 struct rte_devargs {
 	/** Next in list. */
 	TAILQ_ENTRY(rte_devargs) next;
-	/** Bus handle for the device. */
-	struct rte_bus *bus;
 	/** Name of the bus the device belongs to. */
 	char busname[RTE_DEV_NAME_MAX_LEN];
 	/** Name of the device. */
-- 
2.13.2

Re: [dpdk-dev] [PATCH 00/13] devargs fixes

[Thomas Monjalon]

12/07/2017 01:24, Jan Blunck:
> The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
> API without prior notice. This series is reworking the rte_devargs changes
> in a way hopefully compliant to the new failover PMD and still keeping API
> compatible with earlier releases.

I have not reviewed this series yet.
I have just seen that your are introducing "bus=" in devargs.
Do you keep compatibility with old scheme without "bus=" option?
We need to keep this compat for 17.08 and deprecate it for 17.11.

Please, could you also review the fixes from Gaetan and ack those
which are relevant from your point of view?
We cannot progress if we don't apply the fixes we agree on.

Last note: we should make RC2 in few days (hopefully on Sunday),
so please reply quickly.
Thanks

Re: [dpdk-dev] [PATCH 00/13] devargs fixes

[Jan Blunck]

On Wed, Jul 12, 2017 at 3:29 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 12/07/2017 01:24, Jan Blunck:
>> The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
>> API without prior notice. This series is reworking the rte_devargs changes
>> in a way hopefully compliant to the new failover PMD and still keeping API
>> compatible with earlier releases.
>
> I have not reviewed this series yet.
> I have just seen that your are introducing "bus=" in devargs.
> Do you keep compatibility with old scheme without "bus=" option?
> We need to keep this compat for 17.08 and deprecate it for 17.11.

The old scheme which got introduced with 17.08-rc1 is using the
concatenation of bus name, a one character separator and the device
name:

rte_eal_devargs_parse( "busname*devname,args" )

The new scheme I propose is changing this to take an explicit 'bus='
argument into account. In case the 'bus=' argument isn't given but the
device is found by iterating the devices on the buses this will also
work.

> Please, could you also review the fixes from Gaetan and ack those
> which are relevant from your point of view?
> We cannot progress if we don't apply the fixes we agree on.

Yes, will do today.

> Last note: we should make RC2 in few days (hopefully on Sunday),
> so please reply quickly.
> Thanks
>

Re: [dpdk-dev] [PATCH 00/13] devargs fixes

[Thomas Monjalon]

12/07/2017 10:09, Jan Blunck:
> On Wed, Jul 12, 2017 at 3:29 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > 12/07/2017 01:24, Jan Blunck:
> >> The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
> >> API without prior notice. This series is reworking the rte_devargs changes
> >> in a way hopefully compliant to the new failover PMD and still keeping API
> >> compatible with earlier releases.
> >
> > I have not reviewed this series yet.
> > I have just seen that your are introducing "bus=" in devargs.
> > Do you keep compatibility with old scheme without "bus=" option?
> > We need to keep this compat for 17.08 and deprecate it for 17.11.
> 
> The old scheme which got introduced with 17.08-rc1 is using the
> concatenation of bus name, a one character separator and the device
> name:
> 
> rte_eal_devargs_parse( "busname*devname,args" )

No I was talking about the real old scheme without any bus name :)

> The new scheme I propose is changing this to take an explicit 'bus='
> argument into account. In case the 'bus=' argument isn't given but the
> device is found by iterating the devices on the buses this will also
> work.

OK

> > Please, could you also review the fixes from Gaetan and ack those
> > which are relevant from your point of view?
> > We cannot progress if we don't apply the fixes we agree on.
> 
> Yes, will do today.

Thanks

> > Last note: we should make RC2 in few days (hopefully on Sunday),
> > so please reply quickly.
> > Thanks

Re: [dpdk-dev] [PATCH 00/13] devargs fixes

[Jan Blunck]

On Wed, Jul 12, 2017 at 4:50 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 12/07/2017 10:09, Jan Blunck:
>> On Wed, Jul 12, 2017 at 3:29 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
>> > 12/07/2017 01:24, Jan Blunck:
>> >> The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
>> >> API without prior notice. This series is reworking the rte_devargs changes
>> >> in a way hopefully compliant to the new failover PMD and still keeping API
>> >> compatible with earlier releases.
>> >
>> > I have not reviewed this series yet.
>> > I have just seen that your are introducing "bus=" in devargs.
>> > Do you keep compatibility with old scheme without "bus=" option?
>> > We need to keep this compat for 17.08 and deprecate it for 17.11.
>>
>> The old scheme which got introduced with 17.08-rc1 is using the
>> concatenation of bus name, a one character separator and the device
>> name:
>>
>> rte_eal_devargs_parse( "busname*devname,args" )
>
> No I was talking about the real old scheme without any bus name :)
>

Yes, the first patches of the series are fixing the API breakage
introduced by renaming the enum rte_devtype enumerators in 17.08-rc1.
>From what I can tell the rte_eal_devargs_add() function should be API
compatible with 17.05 again after applying this.

[dpdk-dev] [PATCH v2 00/15] devargs fixes

[Jan Blunck]

The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
API without prior notice. This series is reworking the rte_devargs changes
in a way hopefully compliant to the new failover PMD and still keeping API
compatible with earlier releases.

The introduced changes to 17.08-rc1 are trading the tightly coupling of
struct rte_devargs to the PCI and vdev bus against the struct rte_bus.
The changes proposed in this series decouple struct rte_devargs from
the new dependencies.

Changes since v1:
- explicitly pass busname to rte_eal_devargs_parse() and validate it
- better explain why changes are done

Jan Blunck (15):
  Revert "devargs: make device types generic"
  devargs: fix unittest
  devargs: extend unittest
  devargs: deprecate enum rte_devtype based functions
  pci: use scan_mode configuration
  bus: add configuration interface for buses
  devargs: use bus configuration interface to set scanning mode
  devargs: use existing functions in rte_eal_devargs_parse()
  devargs: add busname string field
  devargs: use busname
  pci: use busname
  vdev: use busname
  devargs: pass busname argument when parsing
  devargs: remove type field
  devargs: remove bus field

 doc/guides/rel_notes/deprecation.rst            |   7 +
 drivers/net/virtio/virtio_pci.c                 |   3 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   1 +
 lib/librte_eal/common/eal_common_bus.c          |  16 +++
 lib/librte_eal/common/eal_common_devargs.c      | 168 +++++++++++++-----------
 lib/librte_eal/common/eal_common_options.c      |   6 +-
 lib/librte_eal/common/eal_common_pci.c          |  15 +--
 lib/librte_eal/common/eal_common_vdev.c         |   3 +-
 lib/librte_eal/common/include/rte_bus.h         |   9 ++
 lib/librte_eal/common/include/rte_devargs.h     |  32 +++--
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |   1 +
 test/test/test_devargs.c                        |  55 +++++---
 12 files changed, 188 insertions(+), 128 deletions(-)

-- 
2.13.2

[dpdk-dev] [PATCH v2 01/15] Revert "devargs: make device types generic"

[Jan Blunck]

This (partially) reverts commit
bd279a79366f50a4893fb84db91bbf64b56f9fb1.
---
 lib/librte_eal/common/eal_common_devargs.c  |  4 ++--
 lib/librte_eal/common/eal_common_options.c  |  6 ++---
 lib/librte_eal/common/eal_common_pci.c      |  4 ++--
 lib/librte_eal/common/eal_common_vdev.c     |  1 +
 lib/librte_eal/common/include/rte_devargs.h |  6 ++---
 test/test/test_devargs.c                    | 36 ++++++++++++++---------------
 6 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a328..92c77c30e 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -154,14 +154,14 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		goto fail;
 	devargs->type = devtype;
 	bus = devargs->bus;
-	if (devargs->type == RTE_DEVTYPE_WHITELISTED) {
+	if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI) {
 		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
 			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
 		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
 			fprintf(stderr, "ERROR: incompatible device type and bus scan mode\n");
 			goto fail;
 		}
-	} else if (devargs->type == RTE_DEVTYPE_BLACKLISTED) {
+	} else if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI) {
 		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
 			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
 		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST) {
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 075b0ea9e..84a21fefe 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -798,14 +798,14 @@ eal_parse_common_option(int opt, const char *optarg,
 	switch (opt) {
 	/* blacklist */
 	case 'b':
-		if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED,
+		if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI,
 				optarg) < 0) {
 			return -1;
 		}
 		break;
 	/* whitelist */
 	case 'w':
-		if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED,
+		if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI,
 				optarg) < 0) {
 			return -1;
 		}
@@ -901,7 +901,7 @@ eal_parse_common_option(int opt, const char *optarg,
 		break;
 
 	case OPT_VDEV_NUM:
-		if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED,
+		if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
 				optarg) < 0) {
 			return -1;
 		}
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 76bbcc853..72fcc35c2 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -198,7 +198,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	/* no initialization when blacklisted, return without error */
 	if (dev->device.devargs != NULL &&
 		dev->device.devargs->type ==
-			RTE_DEVTYPE_BLACKLISTED) {
+			RTE_DEVTYPE_BLACKLISTED_PCI) {
 		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
 			" initializing\n");
 		return 1;
@@ -405,7 +405,7 @@ rte_pci_probe(void)
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
 		else if (devargs != NULL &&
-			devargs->type == RTE_DEVTYPE_WHITELISTED)
+			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
 			ret = pci_probe_all_drivers(dev);
 		if (ret < 0) {
 			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index e00dda9aa..ff6a3b571 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -141,6 +141,7 @@ alloc_devargs(const char *name, const char *args)
 	if (!devargs)
 		return NULL;
 
+	devargs->type = RTE_DEVTYPE_VIRTUAL;
 	devargs->bus = &rte_vdev_bus;
 	if (args)
 		devargs->args = strdup(args);
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd8a..62dd67bff 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -56,9 +56,9 @@ extern "C" {
  * Type of generic device
  */
 enum rte_devtype {
-	RTE_DEVTYPE_UNDEFINED,
-	RTE_DEVTYPE_WHITELISTED,
-	RTE_DEVTYPE_BLACKLISTED,
+	RTE_DEVTYPE_WHITELISTED_PCI,
+	RTE_DEVTYPE_BLACKLISTED_PCI,
+	RTE_DEVTYPE_VIRTUAL,
 };
 
 /**
diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 149c9c926..18f54edc1 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -64,32 +64,30 @@ test_devargs(void)
 	TAILQ_INIT(&devargs_list);
 
 	/* test valid cases */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "08:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:00.1") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "0000:5:00.0") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "0000:5:00.0") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "04:00.0,arg=val") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "0000:01:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1") < 0)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED) != 4)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) != 2)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED) != 0)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 2)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_UNDEFINED) != 0)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED, "net_ring0") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, "net_ring0") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED,
-				"net_ring1,key=val,k2=val2") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, "net_ring1,key=val,k2=val2") < 0)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_UNDEFINED) != 2)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 2)
 		goto fail;
 	free_devargs_list();
 
 	/* check virtual device with argument parsing */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_UNDEFINED,
-				"net_ring1,k1=val,k2=val2") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, "net_ring1,k1=val,k2=val2") < 0)
 		goto fail;
 	devargs = TAILQ_FIRST(&devargs_list);
 	if (strncmp(devargs->name, "net_ring1",
@@ -100,7 +98,7 @@ test_devargs(void)
 	free_devargs_list();
 
 	/* check PCI device with empty argument parsing */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "04:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "04:00.1") < 0)
 		goto fail;
 	devargs = TAILQ_FIRST(&devargs_list);
 	if (strcmp(devargs->name, "04:00.1") != 0)
@@ -110,15 +108,15 @@ test_devargs(void)
 	free_devargs_list();
 
 	/* test error case: bad PCI address */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "08:1") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "00.1") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "foo") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, ",") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED, "000f:0:0") == 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
 		goto fail;
 
 	devargs_list = save_devargs_list;
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 01/15] Revert "devargs: make device types generic"

[Ferruh Yigit]

On 7/14/2017 10:11 PM, Jan Blunck wrote:
> This (partially) reverts commit
> bd279a79366f50a4893fb84db91bbf64b56f9fb1.

This seems already applied to 17.08:

http://dpdk.org/commit/bd279a79366f

[dpdk-dev] [PATCH v2 02/15] devargs: fix unittest

[Jan Blunck]

Since the scan-mode of the bus is now based on the bus configuration it
isn't possible to have blacklisted and whitelisted devices existing for
the same bus. This fixes the unittest to reflect that.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 test/test/test_devargs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 18f54edc1..02fec8b1f 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -68,13 +68,15 @@ test_devargs(void)
 		goto fail;
 	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "0000:5:00.0") < 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val")
+		== 0)
 		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1") < 0)
+	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1")
+		== 0)
 		goto fail;
 	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) != 2)
 		goto fail;
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 2)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 0)
 		goto fail;
 	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 0)
 		goto fail;
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 02/15] devargs: fix unittest

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> Since the scan-mode of the bus is now based on the bus configuration it
> isn't possible to have blacklisted and whitelisted devices existing for
> the same bus. This fixes the unittest to reflect that.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  test/test/test_devargs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 18f54edc1..02fec8b1f 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -68,13 +68,15 @@ test_devargs(void)
>  		goto fail;
>  	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "0000:5:00.0") < 0)

[1] see below.

>  		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val") < 0)
> +	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "04:00.0,arg=val")
> +		== 0)

Although as comment said, bus scan policy only can be whitelist or
blacklist, and previous call sets it to whitelist [1], this API call
doesn't return error. So changing its return type will fail the unittest.

Fix can be changing type to whitelist, and update count to 4 in below
check [2]. And keep blacklist count 0 [3].

>  		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1") < 0)
> +	if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI, "0000:01:00.1")
> +		== 0)
>  		goto fail;
>  	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) != 2)
>  		goto fail;

[2]

> -	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 2)
> +	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 0)
>  		goto fail;

[3]

>  	if (rte_eal_devargs_type_count(RTE_DEVTYPE_VIRTUAL) != 0)
>  		goto fail;
> 

[dpdk-dev] [PATCH v2 03/15] devargs: extend unittest

[Jan Blunck]

This is extending the existing unittest to also cover corner cases of
rte_eal_devargs_parse().

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 test/test/test_devargs.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 02fec8b1f..178c3b243 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -58,6 +58,7 @@ test_devargs(void)
 {
 	struct rte_devargs_list save_devargs_list;
 	struct rte_devargs *devargs;
+	struct rte_devargs devargs_stack;
 
 	/* save the real devargs_list, it is restored at the end of the test */
 	save_devargs_list = devargs_list;
@@ -121,6 +122,25 @@ test_devargs(void)
 	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
 		goto fail;
 
+	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
+		printf("Error in rte_eal_devargs_parse()\n");
+		goto fail;
+	}
+
+	if (rte_eal_devargs_parse("08:00.1,foo=bar", &devargs_stack) != 0) {
+		printf("Error in rte_eal_devargs_parse(08:00.1,foo=bar)\n");
+		goto fail;
+	}
+	devargs = TAILQ_FIRST(&devargs_list);
+	if (devargs != NULL)
+		goto fail;
+	devargs = &devargs_stack;
+	if (strcmp(devargs->name, "08:00.1") != 0)
+		goto fail;
+	if (!devargs->args || strcmp(devargs->args, "foo=bar") != 0)
+		goto fail;
+
+	free_devargs_list();
 	devargs_list = save_devargs_list;
 	return 0;
 
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 03/15] devargs: extend unittest

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> This is extending the existing unittest to also cover corner cases of
> rte_eal_devargs_parse().
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  test/test/test_devargs.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 02fec8b1f..178c3b243 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -58,6 +58,7 @@ test_devargs(void)
>  {
>  	struct rte_devargs_list save_devargs_list;
>  	struct rte_devargs *devargs;
> +	struct rte_devargs devargs_stack;
>  
>  	/* save the real devargs_list, it is restored at the end of the test */
>  	save_devargs_list = devargs_list;
> @@ -121,6 +122,25 @@ test_devargs(void)
>  	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
>  		goto fail;
>  
> +	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
> +		printf("Error in rte_eal_devargs_parse()\n");
> +		goto fail;
> +	}
> +
> +	if (rte_eal_devargs_parse("08:00.1,foo=bar", &devargs_stack) != 0) {
> +		printf("Error in rte_eal_devargs_parse(08:00.1,foo=bar)\n");
> +		goto fail;
> +	}
> +	devargs = TAILQ_FIRST(&devargs_list);
> +	if (devargs != NULL)
> +		goto fail;
> +	devargs = &devargs_stack;
> +	if (strcmp(devargs->name, "08:00.1") != 0)

rte_eal_devargs_parse() does not insert into devargs_list, so this check
will fail.

rte_eal_devargs_add() both calls rte_eal_devargs_parse() and adds into
the list.

> +		goto fail;
> +	if (!devargs->args || strcmp(devargs->args, "foo=bar") != 0)
> +		goto fail;
> +
> +	free_devargs_list();
>  	devargs_list = save_devargs_list;
>  	return 0;
>  
> 

[dpdk-dev] [PATCH v2 04/15] devargs: deprecate enum rte_devtype based functions

[Jan Blunck]

The enum rte_devtype will need to get extended every time we add a bus.
Mark all related functions as deprecated for 17.11.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 doc/guides/rel_notes/deprecation.rst        | 7 +++++++
 lib/librte_eal/common/include/rte_devargs.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 257dcba32..0c763d522 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -64,3 +64,10 @@ Deprecation Notices
   be removed in 17.11:
 
   - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse``
+
+* devargs: An API/ABI change is planed for 17.11 for ``struct rte_devargs`` to
+  remove ``enum rte_devtype`` so that starting from 17.08 the following
+  functions are deprecated:
+
+  - ``rte_eal_devargs_add``
+  - ``rte_eal_devargs_type_count``
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 62dd67bff..41db817cc 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -53,6 +53,7 @@ extern "C" {
 #include <rte_bus.h>
 
 /**
+ * @deprecated
  * Type of generic device
  */
 enum rte_devtype {
@@ -139,6 +140,7 @@ rte_eal_devargs_parse(const char *dev,
 		      struct rte_devargs *da);
 
 /**
+ * @deprecated
  * Add a device to the user device list
  *
  * For PCI devices, the format of arguments string is "PCI_ADDR" or
@@ -163,6 +165,7 @@ rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * @deprecated
  * Count the number of user devices of a specified type
  *
  * @param devtype
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 04/15] devargs: deprecate enum rte_devtype based functions

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> The enum rte_devtype will need to get extended every time we add a bus.
> Mark all related functions as deprecated for 17.11.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  doc/guides/rel_notes/deprecation.rst        | 7 +++++++
>  lib/librte_eal/common/include/rte_devargs.h | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 257dcba32..0c763d522 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -64,3 +64,10 @@ Deprecation Notices
>    be removed in 17.11:
>  
>    - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse``
> +
> +* devargs: An API/ABI change is planed for 17.11 for ``struct rte_devargs`` to
> +  remove ``enum rte_devtype`` so that starting from 17.08 the following
> +  functions are deprecated:
> +
> +  - ``rte_eal_devargs_add``
> +  - ``rte_eal_devargs_type_count``

This also seems already applied.

> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index 62dd67bff..41db817cc 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -53,6 +53,7 @@ extern "C" {
>  #include <rte_bus.h>
>  
>  /**
> + * @deprecated

Although these "@deprecated" notes not applied.

>   * Type of generic device
>   */
>  enum rte_devtype {
> @@ -139,6 +140,7 @@ rte_eal_devargs_parse(const char *dev,
>  		      struct rte_devargs *da);
>  
>  /**
> + * @deprecated
>   * Add a device to the user device list
>   *
>   * For PCI devices, the format of arguments string is "PCI_ADDR" or
> @@ -163,6 +165,7 @@ rte_eal_devargs_parse(const char *dev,
>  int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
>  
>  /**
> + * @deprecated
>   * Count the number of user devices of a specified type
>   *
>   * @param devtype
> 

[dpdk-dev] [PATCH v2 05/15] pci: use scan_mode configuration

[Jan Blunck]

When scanning/probing devices the bus doesn't need to look at the
devargs->type field: if the bus is in blacklist probing mode and there is
no devargs found for the device it is white-listed. Therefore it is enough
to let the bus check for the scan_mode.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_pci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 72fcc35c2..fb0e29ac4 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -197,8 +197,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 
 	/* no initialization when blacklisted, return without error */
 	if (dev->device.devargs != NULL &&
-		dev->device.devargs->type ==
-			RTE_DEVTYPE_BLACKLISTED_PCI) {
+		rte_pci_bus.bus.conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
 		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
 			" initializing\n");
 		return 1;
@@ -404,8 +403,7 @@ rte_pci_probe(void)
 		/* probe all or only whitelisted devices */
 		if (probe_all)
 			ret = pci_probe_all_drivers(dev);
-		else if (devargs != NULL &&
-			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
+		else if (devargs != NULL)
 			ret = pci_probe_all_drivers(dev);
 		if (ret < 0) {
 			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 05/15] pci: use scan_mode configuration

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> When scanning/probing devices the bus doesn't need to look at the
> devargs->type field: if the bus is in blacklist probing mode and there is
> no devargs found for the device it is white-listed. Therefore it is enough
> to let the bus check for the scan_mode.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/common/eal_common_pci.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index 72fcc35c2..fb0e29ac4 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -197,8 +197,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>  
>  	/* no initialization when blacklisted, return without error */
>  	if (dev->device.devargs != NULL &&
> -		dev->device.devargs->type ==
> -			RTE_DEVTYPE_BLACKLISTED_PCI) {
> +		rte_pci_bus.bus.conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {

If eal blacklist command issued (-b), even for one device, bus scan_mode
set to RTE_BUS_SCAN_BLACKLIST. Only devices provided with -b should be
blacklisted, all other devices in the bus should be probed.

If check done based on "scan_mode", this will blacklist all devices in
the bus, unless I am missing something.

I see this is to remove "devargs->type" but I don't think so this can be
replaced with "conf.scan_mode" check.

And while thinking about this, I wonder what "scan_mode" really mean?
and where it is used/useful?

>  		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
>  			" initializing\n");
>  		return 1;
> @@ -404,8 +403,7 @@ rte_pci_probe(void)
>  		/* probe all or only whitelisted devices */
>  		if (probe_all)
>  			ret = pci_probe_all_drivers(dev);
> -		else if (devargs != NULL &&
> -			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
> +		else if (devargs != NULL)

if "probe_all" is not set, this means "scan_mode" is WHITELIST. And only
physical devices white-listed will have devargs, so this check looks OK,
but I believe this requires some comment, otherwise this is not clear.

Also why not check blacklist here?

>  			ret = pci_probe_all_drivers(dev);
>  		if (ret < 0) {
>  			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
> 

[dpdk-dev] [PATCH v2 06/15] bus: add configuration interface for buses

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_bus.c          | 16 ++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         |  9 +++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 27 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895cd..82f64c386 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -206,6 +206,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_bus_configure;
 	rte_eal_devargs_parse;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 08bec2d93..f5368a486 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -64,6 +64,22 @@ rte_bus_unregister(struct rte_bus *bus)
 	RTE_LOG(DEBUG, EAL, "Unregistered [%s] bus.\n", bus->name);
 }
 
+int rte_bus_configure(struct rte_bus *bus, const struct rte_bus_conf *conf)
+{
+	if (bus == NULL)
+		return -1;
+
+	/* only set bus scan policy if it was unset before */
+	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+		RTE_LOG(DEBUG, EAL, "Bus [%s] scan_mode=%d\n", bus->name,
+			conf->scan_mode);
+		bus->conf.scan_mode = conf->scan_mode;
+	} else if (bus->conf.scan_mode != conf->scan_mode)
+		return -1;
+
+	return 0;
+}
+
 /* Scan all the buses for registered devices */
 int
 rte_bus_scan(void)
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index af9f0e13f..ff67e6e93 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -206,6 +206,15 @@ void rte_bus_register(struct rte_bus *bus);
 void rte_bus_unregister(struct rte_bus *bus);
 
 /**
+ * Configure a Bus instance.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be configured.
+ */
+int rte_bus_configure(struct rte_bus *bus, const struct rte_bus_conf *conf);
+
+/**
  * Scan all the buses.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009b6..8b8c382a7 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -211,6 +211,7 @@ DPDK_17.08 {
 EXPERIMENTAL {
 	global:
 
+	rte_eal_bus_configure;
 	rte_eal_devargs_parse;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 06/15] bus: add configuration interface for buses

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/eal_common_bus.c          | 16 ++++++++++++++++
>  lib/librte_eal/common/include/rte_bus.h         |  9 +++++++++
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  4 files changed, 27 insertions(+)
> 

<...>

>  
> +int rte_bus_configure(struct rte_bus *bus, const struct rte_bus_conf *conf)
> +{
> +	if (bus == NULL)
> +		return -1;
> +
> +	/* only set bus scan policy if it was unset before */
> +	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
> +		RTE_LOG(DEBUG, EAL, "Bus [%s] scan_mode=%d\n", bus->name,
> +			conf->scan_mode);
> +		bus->conf.scan_mode = conf->scan_mode;
> +	} else if (bus->conf.scan_mode != conf->scan_mode)
> +		return -1;

Right now "struct rte_bus_conf" has only field "scan_mode", so this
function implemented as set scan_mode is no issue.

But if in the future, "struct rte_bus_conf" extended to have another
field, this same function will be used and this will be confusing.

What do you think make this function rte_bus_configure_scan_mode(), is
it overkill?

> +
> +	return 0;
> +}
> +

<...>

[dpdk-dev] [PATCH v2 07/15] devargs: use bus configuration interface to set scanning mode

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 92c77c30e..205fabb95 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -137,6 +137,14 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 	return 0;
 }
 
+static const struct rte_bus_conf BUS_CONF_WHITELIST = {
+	.scan_mode = RTE_BUS_SCAN_WHITELIST,
+};
+
+static const struct rte_bus_conf BUS_CONF_BLACKLIST = {
+	.scan_mode = RTE_BUS_SCAN_BLACKLIST,
+};
+
 /* store a whitelist parameter for later parsing */
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
@@ -144,6 +152,7 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	struct rte_devargs *devargs = NULL;
 	struct rte_bus *bus = NULL;
 	const char *dev = devargs_str;
+	int ret;
 
 	/* use calloc instead of rte_zmalloc as it's called early at init */
 	devargs = calloc(1, sizeof(*devargs));
@@ -154,18 +163,14 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		goto fail;
 	devargs->type = devtype;
 	bus = devargs->bus;
-	if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
-			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
-			fprintf(stderr, "ERROR: incompatible device type and bus scan mode\n");
-			goto fail;
-		}
-	} else if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
-			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST) {
-			fprintf(stderr, "ERROR: incompatible device type and bus scan mode\n");
+	if (devtype != RTE_DEVTYPE_VIRTUAL) {
+		ret = rte_bus_configure(bus,
+			devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
+			&BUS_CONF_WHITELIST : &BUS_CONF_BLACKLIST);
+		if (ret != 0) {
+			RTE_LOG(ERR, EAL,
+				"Bus [%s] scan_mode conflicts with devtype: "
+				"%s\n", bus->name, devargs_str);
 			goto fail;
 		}
 	}
-- 
2.13.2

[dpdk-dev] [PATCH v2 08/15] devargs: use existing functions in rte_eal_devargs_parse()

[Jan Blunck]

This fixes the newly introduces rte_eal_devargs_parse() to make use of:
- snprintf() instead of open coding a while() loop
- rte_eal_parse_devargs_str() instead of duplicating parsing code
- RTE_LOG() instead of direct output to stderr

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 57 +++++++++++++++---------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 205fabb95..b5273287e 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -87,54 +87,53 @@ int
 rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 {
 	struct rte_bus *bus = NULL;
-	const char *devname;
-	const size_t maxlen = sizeof(da->name);
-	size_t i;
+	char *devname = NULL, *drvargs = NULL;
+	int ret;
 
 	if (dev == NULL || da == NULL)
 		return -EINVAL;
 	/* Retrieve eventual bus info */
 	do {
-		devname = dev;
 		bus = rte_bus_find(bus, bus_name_cmp, dev);
 		if (bus == NULL)
 			break;
-		devname = dev + strlen(bus->name) + 1;
-		if (rte_bus_find_by_device_name(devname) == bus)
+		dev += strlen(bus->name) + 1;
+		if (rte_bus_find_by_device_name(dev) == bus)
 			break;
 	} while (1);
+
+	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
+	if (ret != 0)
+		return ret;
+
 	/* Store device name */
-	i = 0;
-	while (devname[i] != '\0' && devname[i] != ',') {
-		da->name[i] = devname[i];
-		i++;
-		if (i == maxlen) {
-			fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
-				dev, maxlen);
-			da->name[i - 1] = '\0';
-			return -EINVAL;
-		}
+	ret = snprintf(da->name, sizeof(da->name), "%s", devname);
+	if (ret < 0 || ret >= (int)sizeof(da->name)) {
+		RTE_LOG(ERR, EAL, "Invalid device name: \"%s\"\n", devname);
+		ret = -EINVAL;
+		goto fail;
 	}
-	da->name[i] = '\0';
+
+	/* Store drivers arguments */
+	da->args = drvargs;
+	drvargs = NULL;
+
 	if (bus == NULL) {
 		bus = rte_bus_find_by_device_name(da->name);
 		if (bus == NULL) {
-			fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
+			RTE_LOG(ERR, EAL, "Failed to parse device: \"%s\"\n",
 				da->name);
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 	}
 	da->bus = bus;
-	/* Parse eventual device arguments */
-	if (devname[i] == ',')
-		da->args = strdup(&devname[i + 1]);
-	else
-		da->args = strdup("");
-	if (da->args == NULL) {
-		fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
-		return -ENOMEM;
-	}
-	return 0;
+	ret = 0;
+
+fail:
+	free(devname);
+	free(drvargs);
+	return ret;
 }
 
 static const struct rte_bus_conf BUS_CONF_WHITELIST = {
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 08/15] devargs: use existing functions in rte_eal_devargs_parse()

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> This fixes the newly introduces rte_eal_devargs_parse() to make use of:
> - snprintf() instead of open coding a while() loop
> - rte_eal_parse_devargs_str() instead of duplicating parsing code
> - RTE_LOG() instead of direct output to stderr
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/common/eal_common_devargs.c | 57 +++++++++++++++---------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 205fabb95..b5273287e 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -87,54 +87,53 @@ int
>  rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)

here "const char *dev" is devarg_str, it can be something like:
"00:01.0,somekey=somevalue,keymore=valuemore"

Calling this "dev" is confusing I think, since you are touching to the
function does it make sense to rename this?

>  {
>  	struct rte_bus *bus = NULL;
> -	const char *devname;
> -	const size_t maxlen = sizeof(da->name);
> -	size_t i;
> +	char *devname = NULL, *drvargs = NULL;
> +	int ret;
>  
>  	if (dev == NULL || da == NULL)
>  		return -EINVAL;
>  	/* Retrieve eventual bus info */
>  	do {
> -		devname = dev;
>  		bus = rte_bus_find(bus, bus_name_cmp, dev);
>  		if (bus == NULL)
>  			break;
> -		devname = dev + strlen(bus->name) + 1;
> -		if (rte_bus_find_by_device_name(devname) == bus)
> +		dev += strlen(bus->name) + 1;

This deserves a comment I believe, what is done here?

> +		if (rte_bus_find_by_device_name(dev) == bus)
>  			break;
>  	} while (1);

<...>

[dpdk-dev] [PATCH v2 09/15] devargs: add busname string field

[Jan Blunck]

This adds the busname as a string to struct rte_devargs. This is a generic
replacement for enum rte_devtype without tightly coupling rte_devargs to
the rte_bus structure.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c  | 9 +++++++++
 lib/librte_eal/common/include/rte_devargs.h | 2 ++
 test/test/test_devargs.c                    | 2 ++
 3 files changed, 13 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index b5273287e..588df1410 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -128,6 +128,15 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 		}
 	}
 	da->bus = bus;
+
+	/* Store bus name */
+	ret = snprintf(da->busname, sizeof(da->busname), "%s", bus->name);
+	if (ret < 0 || ret >= (int)sizeof(da->busname)) {
+		RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", bus->name);
+		ret = -EINVAL;
+		goto fail;
+	}
+
 	ret = 0;
 
 fail:
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 41db817cc..8dafb167e 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -79,6 +79,8 @@ struct rte_devargs {
 	enum rte_devtype type;
 	/** Bus handle for the device. */
 	struct rte_bus *bus;
+	/** Name of the bus the device belongs to. */
+	char busname[RTE_DEV_NAME_MAX_LEN];
 	/** Name of the device. */
 	char name[RTE_DEV_NAME_MAX_LEN];
 	/** Arguments string as given by user or "" for no argument. */
diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 178c3b243..9e0ff5995 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -135,6 +135,8 @@ test_devargs(void)
 	if (devargs != NULL)
 		goto fail;
 	devargs = &devargs_stack;
+	if (strcmp(devargs->busname, "pci") != 0)
+		goto fail;
 	if (strcmp(devargs->name, "08:00.1") != 0)
 		goto fail;
 	if (!devargs->args || strcmp(devargs->args, "foo=bar") != 0)
-- 
2.13.2

[dpdk-dev] [PATCH v2 10/15] devargs: use busname

[Jan Blunck]

This makes the devargs code itself require the rte_devargs type field for
properly functioning.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 588df1410..38ba64567 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -199,13 +199,28 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
 {
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	const char *busname = "";
 	struct rte_devargs *devargs;
 	unsigned int count = 0;
 
+	switch (devtype) {
+	case RTE_DEVTYPE_WHITELISTED_PCI:
+		if (pci_bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST)
+			busname = "pci";
+		break;
+	case RTE_DEVTYPE_BLACKLISTED_PCI:
+		if (pci_bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST)
+			busname = "pci";
+		break;
+	case RTE_DEVTYPE_VIRTUAL:
+		busname = "vdev";
+		break;
+	}
+
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
-		if (devargs->type != devtype)
-			continue;
-		count++;
+		if (strcmp(busname, devargs->busname) == 0)
+			count++;
 	}
 	return count;
 }
@@ -218,8 +233,7 @@ rte_eal_devargs_dump(FILE *f)
 
 	fprintf(f, "User device list:\n");
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
-		fprintf(f, "  [%s]: %s %s\n",
-			(devargs->bus ? devargs->bus->name : "??"),
-			devargs->name, devargs->args);
+		fprintf(f, "  [%s]: %s %s\n", devargs->busname,	devargs->name,
+			devargs->args);
 	}
 }
-- 
2.13.2

[dpdk-dev] [PATCH v2 11/15] pci: use busname

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/virtio/virtio_pci.c        | 3 +--
 lib/librte_eal/common/eal_common_pci.c | 9 +++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index e6eda75b6..dfc6edac2 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -685,8 +685,7 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 	if (rte_pci_ioport_map(dev, 0, VTPCI_IO(hw)) < 0) {
 		if (dev->kdrv == RTE_KDRV_UNKNOWN &&
 		    (!dev->device.devargs ||
-		     dev->device.devargs->bus !=
-		     rte_bus_find_by_name("pci"))) {
+		     strcmp("pci", dev->device.devargs->busname) != 0)) {
 			PMD_INIT_LOG(INFO,
 				"skip kernel managed virtio device.");
 			return 1;
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index fb0e29ac4..834db50de 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -71,17 +71,18 @@ const char *pci_get_sysfs_path(void)
 	return path;
 }
 
+static int pci_parse(const char *name, void *addr);
+
 static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 {
 	struct rte_devargs *devargs;
 	struct rte_pci_addr addr;
-	struct rte_bus *pbus;
 
-	pbus = rte_bus_find_by_name("pci");
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
-		if (devargs->bus != pbus)
+		if (strcmp(devargs->busname, rte_pci_bus.bus.name) != 0)
+			continue;
+		if (pci_parse(devargs->name, &addr) != 0)
 			continue;
-		devargs->bus->parse(devargs->name, &addr);
 		if (!rte_eal_compare_pci_addr(&dev->addr, &addr))
 			return devargs;
 	}
-- 
2.13.2

[dpdk-dev] [PATCH v2 12/15] vdev: use busname

[Jan Blunck]

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_vdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index ff6a3b571..d04015582 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -260,7 +260,7 @@ vdev_scan(void)
 	/* for virtual devices we scan the devargs_list populated via cmdline */
 	TAILQ_FOREACH(devargs, &devargs_list, next) {
 
-		if (devargs->bus != &rte_vdev_bus)
+		if (strcmp(devargs->busname, rte_vdev_bus.name) != 0)
 			continue;
 
 		dev = find_vdev(devargs->name);
-- 
2.13.2

[dpdk-dev] [PATCH v2 13/15] devargs: pass busname argument when parsing

[Jan Blunck]

Let the rte_eal_devargs_parse() function explicitly take a "busname"
argument that is validated.

Now that the busname is known and validated at parse time the validity of
the device name is checked for all device types when they get probed.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c  | 72 +++++++++++------------------
 lib/librte_eal/common/include/rte_devargs.h | 17 ++++---
 test/test/test_devargs.c                    | 21 +++------
 3 files changed, 45 insertions(+), 65 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 38ba64567..44a8d8562 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -77,30 +77,21 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 	return 0;
 }
 
-static int
-bus_name_cmp(const struct rte_bus *bus, const void *name)
-{
-	return strncmp(bus->name, name, strlen(bus->name));
-}
-
 int
-rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
+rte_eal_devargs_parse(const char *busname, const char *dev,
+	struct rte_devargs *da)
 {
-	struct rte_bus *bus = NULL;
 	char *devname = NULL, *drvargs = NULL;
 	int ret;
 
-	if (dev == NULL || da == NULL)
+	if (busname == NULL || dev == NULL || da == NULL)
 		return -EINVAL;
 	/* Retrieve eventual bus info */
-	do {
-		bus = rte_bus_find(bus, bus_name_cmp, dev);
-		if (bus == NULL)
-			break;
-		dev += strlen(bus->name) + 1;
-		if (rte_bus_find_by_device_name(dev) == bus)
-			break;
-	} while (1);
+	da->bus = rte_bus_find_by_name(busname);
+	if (da->bus == NULL) {
+		RTE_LOG(ERR, EAL, "Bus not found: \"%s\"\n", busname);
+		return -EFAULT;
+	}
 
 	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
 	if (ret != 0)
@@ -118,21 +109,10 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 	da->args = drvargs;
 	drvargs = NULL;
 
-	if (bus == NULL) {
-		bus = rte_bus_find_by_device_name(da->name);
-		if (bus == NULL) {
-			RTE_LOG(ERR, EAL, "Failed to parse device: \"%s\"\n",
-				da->name);
-			ret = -EFAULT;
-			goto fail;
-		}
-	}
-	da->bus = bus;
-
 	/* Store bus name */
-	ret = snprintf(da->busname, sizeof(da->busname), "%s", bus->name);
+	ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
 	if (ret < 0 || ret >= (int)sizeof(da->busname)) {
-		RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", bus->name);
+		RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", busname);
 		ret = -EINVAL;
 		goto fail;
 	}
@@ -158,8 +138,7 @@ int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
 	struct rte_devargs *devargs = NULL;
-	struct rte_bus *bus = NULL;
-	const char *dev = devargs_str;
+	const char *busname = NULL;
 	int ret;
 
 	/* use calloc instead of rte_zmalloc as it's called early at init */
@@ -167,31 +146,36 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	if (devargs == NULL)
 		goto fail;
 
-	if (rte_eal_devargs_parse(dev, devargs))
-		goto fail;
-	devargs->type = devtype;
-	bus = devargs->bus;
-	if (devtype != RTE_DEVTYPE_VIRTUAL) {
-		ret = rte_bus_configure(bus,
+	switch (devtype) {
+	case RTE_DEVTYPE_WHITELISTED_PCI:
+	case RTE_DEVTYPE_BLACKLISTED_PCI:
+		busname = "pci";
+		ret = rte_bus_configure(rte_bus_find_by_name(busname),
 			devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
 			&BUS_CONF_WHITELIST : &BUS_CONF_BLACKLIST);
 		if (ret != 0) {
 			RTE_LOG(ERR, EAL,
 				"Bus [%s] scan_mode conflicts with devtype: "
-				"%s\n", bus->name, devargs_str);
+				"%s\n", busname, devargs_str);
 			goto fail;
 		}
+		break;
+	case RTE_DEVTYPE_VIRTUAL:
+		busname = "vdev";
+		break;
 	}
 
+	if (rte_eal_devargs_parse(busname, devargs_str, devargs))
+		goto fail;
+
+	RTE_LOG(DEBUG, EAL, "Adding devargs for device [%s] on bus [%s]: %s\n",
+		devargs->name, busname, devargs->args);
+
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
 
 fail:
-	if (devargs) {
-		free(devargs->args);
-		free(devargs);
-	}
-
+	free(devargs);
 	return -1;
 }
 
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 8dafb167e..10953327f 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -124,10 +124,12 @@ int rte_eal_parse_devargs_str(const char *devargs_str,
 /**
  * Parse a device string.
  *
- * Verify that a bus is capable of handling the device passed
- * in argument. Store which bus will handle the device, its name
- * and the eventual device parameters.
+ * The function parses the arguments string to get driver name and driver
+ * arguments and stores together with the busname that will handle the device,
+ * its name and the eventual device parameters.
  *
+ * @param busname
+ *   The bus name the device declaration string applies to.
  * @param dev
  *   The device declaration string.
  * @param da
@@ -138,7 +140,7 @@ int rte_eal_parse_devargs_str(const char *devargs_str,
  *   - Negative errno on error.
  */
 int
-rte_eal_devargs_parse(const char *dev,
+rte_eal_devargs_parse(const char *busname, const char *dev,
 		      struct rte_devargs *da);
 
 /**
@@ -151,9 +153,10 @@ rte_eal_devargs_parse(const char *dev,
  *
  * For virtual devices, the format of arguments string is "DRIVER_NAME*"
  * or "DRIVER_NAME*,key=val,key2=val2,...". Examples: "net_ring",
- * "net_ring0", "net_pmdAnything,arg=0:arg2=1". The validity of the
- * driver name is not checked by this function, it is done when probing
- * the drivers.
+ * "net_ring0", "net_pmdAnything,arg=0:arg2=1".
+ *
+ * The validity of the device name is not checked by this function, it is done
+ * when probing the drivers.
  *
  * @param devtype
  *   The type of the device.
diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
index 9e0ff5995..84803cf5a 100644
--- a/test/test/test_devargs.c
+++ b/test/test/test_devargs.c
@@ -110,24 +110,17 @@ test_devargs(void)
 		goto fail;
 	free_devargs_list();
 
-	/* test error case: bad PCI address */
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
-		goto fail;
-	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
-		goto fail;
-
-	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
+	if (rte_eal_devargs_parse("", "", &devargs_stack) == 0) {
 		printf("Error in rte_eal_devargs_parse()\n");
 		goto fail;
 	}
 
-	if (rte_eal_devargs_parse("08:00.1,foo=bar", &devargs_stack) != 0) {
+	if (rte_eal_devargs_parse("vdev", "not_found", &devargs_stack) != 0) {
+		printf("Error in rte_eal_devargs_parse(vdev:not_found)\n");
+		goto fail;
+	}
+	if (rte_eal_devargs_parse("pci", "08:00.1,foo=bar",
+		&devargs_stack) != 0) {
 		printf("Error in rte_eal_devargs_parse(08:00.1,foo=bar)\n");
 		goto fail;
 	}
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 13/15] devargs: pass busname argument when parsing

[Gaëtan Rivet]

On Fri, Jul 14, 2017 at 05:12:11PM -0400, Jan Blunck wrote:
> Let the rte_eal_devargs_parse() function explicitly take a "busname"
> argument that is validated.
> 
> Now that the busname is known and validated at parse time the validity of
> the device name is checked for all device types when they get probed.
> 

What use is there for a parsing API if the fields have already been
recognized? Why would someone need to parse an rte_devargs if they already
know the busname from the devname? This function becomes useless.

You are applying here the same logic you used with the rte_dev API,
where you divided the fields because it made no sense to have them
merged.

However, the rte_dev API is not in contact with users / applications, it
is not the interface with the outside world where it must take in external
inputs and format them for subsequent systems to use.

> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/common/eal_common_devargs.c  | 72 +++++++++++------------------
>  lib/librte_eal/common/include/rte_devargs.h | 17 ++++---
>  test/test/test_devargs.c                    | 21 +++------
>  3 files changed, 45 insertions(+), 65 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 38ba64567..44a8d8562 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -77,30 +77,21 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>  	return 0;
>  }
>  
> -static int
> -bus_name_cmp(const struct rte_bus *bus, const void *name)
> -{
> -	return strncmp(bus->name, name, strlen(bus->name));
> -}
> -
>  int
> -rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
> +rte_eal_devargs_parse(const char *busname, const char *dev,
> +	struct rte_devargs *da)
>  {
> -	struct rte_bus *bus = NULL;
>  	char *devname = NULL, *drvargs = NULL;
>  	int ret;
>  
> -	if (dev == NULL || da == NULL)
> +	if (busname == NULL || dev == NULL || da == NULL)
>  		return -EINVAL;
>  	/* Retrieve eventual bus info */
> -	do {
> -		bus = rte_bus_find(bus, bus_name_cmp, dev);
> -		if (bus == NULL)
> -			break;
> -		dev += strlen(bus->name) + 1;
> -		if (rte_bus_find_by_device_name(dev) == bus)
> -			break;
> -	} while (1);
> +	da->bus = rte_bus_find_by_name(busname);
> +	if (da->bus == NULL) {
> +		RTE_LOG(ERR, EAL, "Bus not found: \"%s\"\n", busname);
> +		return -EFAULT;
> +	}
>  
>  	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
>  	if (ret != 0)
> @@ -118,21 +109,10 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>  	da->args = drvargs;
>  	drvargs = NULL;
>  
> -	if (bus == NULL) {
> -		bus = rte_bus_find_by_device_name(da->name);
> -		if (bus == NULL) {
> -			RTE_LOG(ERR, EAL, "Failed to parse device: \"%s\"\n",
> -				da->name);
> -			ret = -EFAULT;
> -			goto fail;
> -		}
> -	}
> -	da->bus = bus;
> -
>  	/* Store bus name */
> -	ret = snprintf(da->busname, sizeof(da->busname), "%s", bus->name);
> +	ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
>  	if (ret < 0 || ret >= (int)sizeof(da->busname)) {
> -		RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", bus->name);
> +		RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", busname);
>  		ret = -EINVAL;
>  		goto fail;
>  	}
> @@ -158,8 +138,7 @@ int
>  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>  {
>  	struct rte_devargs *devargs = NULL;
> -	struct rte_bus *bus = NULL;
> -	const char *dev = devargs_str;
> +	const char *busname = NULL;
>  	int ret;
>  
>  	/* use calloc instead of rte_zmalloc as it's called early at init */
> @@ -167,31 +146,36 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>  	if (devargs == NULL)
>  		goto fail;
>  
> -	if (rte_eal_devargs_parse(dev, devargs))
> -		goto fail;
> -	devargs->type = devtype;
> -	bus = devargs->bus;
> -	if (devtype != RTE_DEVTYPE_VIRTUAL) {
> -		ret = rte_bus_configure(bus,
> +	switch (devtype) {
> +	case RTE_DEVTYPE_WHITELISTED_PCI:
> +	case RTE_DEVTYPE_BLACKLISTED_PCI:
> +		busname = "pci";
> +		ret = rte_bus_configure(rte_bus_find_by_name(busname),
>  			devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
>  			&BUS_CONF_WHITELIST : &BUS_CONF_BLACKLIST);
>  		if (ret != 0) {
>  			RTE_LOG(ERR, EAL,
>  				"Bus [%s] scan_mode conflicts with devtype: "
> -				"%s\n", bus->name, devargs_str);
> +				"%s\n", busname, devargs_str);
>  			goto fail;
>  		}
> +		break;
> +	case RTE_DEVTYPE_VIRTUAL:
> +		busname = "vdev";
> +		break;
>  	}
>  
> +	if (rte_eal_devargs_parse(busname, devargs_str, devargs))
> +		goto fail;
> +
> +	RTE_LOG(DEBUG, EAL, "Adding devargs for device [%s] on bus [%s]: %s\n",
> +		devargs->name, busname, devargs->args);
> +
>  	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
>  	return 0;
>  
>  fail:
> -	if (devargs) {
> -		free(devargs->args);
> -		free(devargs);
> -	}
> -
> +	free(devargs);
>  	return -1;
>  }
>  
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index 8dafb167e..10953327f 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -124,10 +124,12 @@ int rte_eal_parse_devargs_str(const char *devargs_str,
>  /**
>   * Parse a device string.
>   *
> - * Verify that a bus is capable of handling the device passed
> - * in argument. Store which bus will handle the device, its name
> - * and the eventual device parameters.
> + * The function parses the arguments string to get driver name and driver
> + * arguments and stores together with the busname that will handle the device,
> + * its name and the eventual device parameters.
>   *
> + * @param busname
> + *   The bus name the device declaration string applies to.
>   * @param dev
>   *   The device declaration string.
>   * @param da
> @@ -138,7 +140,7 @@ int rte_eal_parse_devargs_str(const char *devargs_str,
>   *   - Negative errno on error.
>   */
>  int
> -rte_eal_devargs_parse(const char *dev,
> +rte_eal_devargs_parse(const char *busname, const char *dev,
>  		      struct rte_devargs *da);
>  
>  /**
> @@ -151,9 +153,10 @@ rte_eal_devargs_parse(const char *dev,
>   *
>   * For virtual devices, the format of arguments string is "DRIVER_NAME*"
>   * or "DRIVER_NAME*,key=val,key2=val2,...". Examples: "net_ring",
> - * "net_ring0", "net_pmdAnything,arg=0:arg2=1". The validity of the
> - * driver name is not checked by this function, it is done when probing
> - * the drivers.
> + * "net_ring0", "net_pmdAnything,arg=0:arg2=1".
> + *
> + * The validity of the device name is not checked by this function, it is done
> + * when probing the drivers.
>   *
>   * @param devtype
>   *   The type of the device.
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 9e0ff5995..84803cf5a 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -110,24 +110,17 @@ test_devargs(void)
>  		goto fail;
>  	free_devargs_list();
>  
> -	/* test error case: bad PCI address */
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
> -		goto fail;
> -
> -	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
> +	if (rte_eal_devargs_parse("", "", &devargs_stack) == 0) {
>  		printf("Error in rte_eal_devargs_parse()\n");
>  		goto fail;
>  	}
>  
> -	if (rte_eal_devargs_parse("08:00.1,foo=bar", &devargs_stack) != 0) {
> +	if (rte_eal_devargs_parse("vdev", "not_found", &devargs_stack) != 0) {
> +		printf("Error in rte_eal_devargs_parse(vdev:not_found)\n");
> +		goto fail;
> +	}
> +	if (rte_eal_devargs_parse("pci", "08:00.1,foo=bar",
> +		&devargs_stack) != 0) {
>  		printf("Error in rte_eal_devargs_parse(08:00.1,foo=bar)\n");
>  		goto fail;
>  	}
> -- 
> 2.13.2
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2 13/15] devargs: pass busname argument when parsing

[Ferruh Yigit]

On 7/15/2017 3:48 PM, Gaëtan Rivet wrote:
> On Fri, Jul 14, 2017 at 05:12:11PM -0400, Jan Blunck wrote:
>> Let the rte_eal_devargs_parse() function explicitly take a "busname"
>> argument that is validated.
>>
>> Now that the busname is known and validated at parse time the validity of
>> the device name is checked for all device types when they get probed.
>>
> 
> What use is there for a parsing API if the fields have already been
> recognized? Why would someone need to parse an rte_devargs if they already
> know the busname from the devname? This function becomes useless.

rte_eal_devargs_parse() converts devargs_string to the devargs struct,
this usage is still valid.

Getting busname as argument to this function looks good simplification
to me, I didn't get the problem here.

> 
> You are applying here the same logic you used with the rte_dev API,
> where you divided the fields because it made no sense to have them
> merged.
> 
> However, the rte_dev API is not in contact with users / applications, it
> is not the interface with the outside world where it must take in external
> inputs and format them for subsequent systems to use.
> 
>> Signed-off-by: Jan Blunck <jblunck@infradead.org>

<...>

Re: [dpdk-dev] [PATCH v2 13/15] devargs: pass busname argument when parsing

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> Let the rte_eal_devargs_parse() function explicitly take a "busname"
> argument that is validated.
> 
> Now that the busname is known and validated at parse time the validity of
> the device name is checked for all device types when they get probed.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>

<...>

> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -110,24 +110,17 @@ test_devargs(void)
>  		goto fail;
>  	free_devargs_list();
>  
> -	/* test error case: bad PCI address */
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
> -		goto fail;
> -	if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
> -		goto fail;

Why removed these cases?

<...>

[dpdk-dev] [PATCH v2 14/15] devargs: remove type field

[Jan Blunck]

This removes the enum rte_devtype field ``type`` from struct rte_devargs.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_vdev.c     | 1 -
 lib/librte_eal/common/include/rte_devargs.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index d04015582..c785fcc79 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -141,7 +141,6 @@ alloc_devargs(const char *name, const char *args)
 	if (!devargs)
 		return NULL;
 
-	devargs->type = RTE_DEVTYPE_VIRTUAL;
 	devargs->bus = &rte_vdev_bus;
 	if (args)
 		devargs->args = strdup(args);
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 10953327f..e3722ca28 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -75,8 +75,6 @@ enum rte_devtype {
 struct rte_devargs {
 	/** Next in list. */
 	TAILQ_ENTRY(rte_devargs) next;
-	/** Type of device. */
-	enum rte_devtype type;
 	/** Bus handle for the device. */
 	struct rte_bus *bus;
 	/** Name of the bus the device belongs to. */
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 14/15] devargs: remove type field

[Ferruh Yigit]

On 7/14/2017 10:12 PM, Jan Blunck wrote:
> This removes the enum rte_devtype field ``type`` from struct rte_devargs.
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>

<...>

> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -75,8 +75,6 @@ enum rte_devtype {
>  struct rte_devargs {
>  	/** Next in list. */
>  	TAILQ_ENTRY(rte_devargs) next;
> -	/** Type of device. */
> -	enum rte_devtype type;

I am not sure if type can be removed, please check the comment on patch
5/15.

>  	/** Bus handle for the device. */
>  	struct rte_bus *bus;
>  	/** Name of the bus the device belongs to. */
> 

[dpdk-dev] [PATCH v2 15/15] devargs: remove bus field

[Jan Blunck]

This removes the enum rte_bus field ``bus`` from struct rte_devargs.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c  | 3 +--
 lib/librte_eal/common/eal_common_vdev.c     | 1 -
 lib/librte_eal/common/include/rte_devargs.h | 2 --
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 44a8d8562..191f53607 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -87,8 +87,7 @@ rte_eal_devargs_parse(const char *busname, const char *dev,
 	if (busname == NULL || dev == NULL || da == NULL)
 		return -EINVAL;
 	/* Retrieve eventual bus info */
-	da->bus = rte_bus_find_by_name(busname);
-	if (da->bus == NULL) {
+	if (rte_bus_find_by_name(busname) == NULL) {
 		RTE_LOG(ERR, EAL, "Bus not found: \"%s\"\n", busname);
 		return -EFAULT;
 	}
diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
index c785fcc79..b33f15b03 100644
--- a/lib/librte_eal/common/eal_common_vdev.c
+++ b/lib/librte_eal/common/eal_common_vdev.c
@@ -141,7 +141,6 @@ alloc_devargs(const char *name, const char *args)
 	if (!devargs)
 		return NULL;
 
-	devargs->bus = &rte_vdev_bus;
 	if (args)
 		devargs->args = strdup(args);
 	else
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index e3722ca28..943a4ee98 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -75,8 +75,6 @@ enum rte_devtype {
 struct rte_devargs {
 	/** Next in list. */
 	TAILQ_ENTRY(rte_devargs) next;
-	/** Bus handle for the device. */
-	struct rte_bus *bus;
 	/** Name of the bus the device belongs to. */
 	char busname[RTE_DEV_NAME_MAX_LEN];
 	/** Name of the device. */
-- 
2.13.2

Re: [dpdk-dev] [PATCH v2 00/15] devargs fixes

[Thomas Monjalon]

14/07/2017 23:11, Jan Blunck:
> The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
> API without prior notice. This series is reworking the rte_devargs changes
> in a way hopefully compliant to the new failover PMD and still keeping API
> compatible with earlier releases.
> 
> The introduced changes to 17.08-rc1 are trading the tightly coupling of
> struct rte_devargs to the PCI and vdev bus against the struct rte_bus.
> The changes proposed in this series decouple struct rte_devargs from
> the new dependencies.
> 
> Changes since v1:
> - explicitly pass busname to rte_eal_devargs_parse() and validate it
> - better explain why changes are done

Most of these changes does not seem to fix a bug or an API break.
Before -rc1, we can take them for the sake of cleaning.
But after -rc1, we must take only fixes.

If I understand well, the urgent fix to make in -rc2 is to revert
an API break without prior notice. Which change is it? rte_devtype?

The plan is to fix hotplug/devargs while integrating failsafe in -rc2.
After releasing -rc2, we must agree on the planned clean-up and be sure
that the right deprecation notices are in 17.08.
Then we will integrate all these patches in August for 17.11.

Re: [dpdk-dev] [PATCH v2 00/15] devargs fixes

[Ferruh Yigit]

On 7/14/2017 10:11 PM, Jan Blunck wrote:
> The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
> API without prior notice. This series is reworking the rte_devargs changes
> in a way hopefully compliant to the new failover PMD and still keeping API
> compatible with earlier releases.

This patchset seems target 17.08, but 17.08 already released and some of
the patches in this patchset seems included in the release.

Patchset needs to be rebased on top of latest HEAD.

> 
> The introduced changes to 17.08-rc1 are trading the tightly coupling of
> struct rte_devargs to the PCI and vdev bus against the struct rte_bus.
> The changes proposed in this series decouple struct rte_devargs from
> the new dependencies.
> 
> Changes since v1:
> - explicitly pass busname to rte_eal_devargs_parse() and validate it
> - better explain why changes are done
> 
> Jan Blunck (15):
>   Revert "devargs: make device types generic"
>   devargs: fix unittest
>   devargs: extend unittest
>   devargs: deprecate enum rte_devtype based functions
>   pci: use scan_mode configuration
>   bus: add configuration interface for buses
>   devargs: use bus configuration interface to set scanning mode
>   devargs: use existing functions in rte_eal_devargs_parse()
>   devargs: add busname string field
>   devargs: use busname
>   pci: use busname
>   vdev: use busname
>   devargs: pass busname argument when parsing
>   devargs: remove type field
>   devargs: remove bus field

<...>

Re: [dpdk-dev] [PATCH v2 00/15] devargs fixes

[Gaëtan Rivet]

Hi Ferruh,

On Mon, Sep 04, 2017 at 05:04:57PM +0100, Ferruh Yigit wrote:
> On 7/14/2017 10:11 PM, Jan Blunck wrote:
> > The changes to enum rte_devtype that got merged into 17.08-rc1 are breaking
> > API without prior notice. This series is reworking the rte_devargs changes
> > in a way hopefully compliant to the new failover PMD and still keeping API
> > compatible with earlier releases.
> 
> This patchset seems target 17.08, but 17.08 already released and some of
> the patches in this patchset seems included in the release.
> 
> Patchset needs to be rebased on top of latest HEAD.
> 

The relevant fixes in this patchset were included. Other "fixes" that
tried to impose a certain API and one way of doing things were not.

These evolutions should have been proposed within the proposal window.
I am currently working on a series addressing a few of those elements.

> > 
> > The introduced changes to 17.08-rc1 are trading the tightly coupling of
> > struct rte_devargs to the PCI and vdev bus against the struct rte_bus.
> > The changes proposed in this series decouple struct rte_devargs from
> > the new dependencies.
> > 
> > Changes since v1:
> > - explicitly pass busname to rte_eal_devargs_parse() and validate it
> > - better explain why changes are done
> > 
> > Jan Blunck (15):
> >   Revert "devargs: make device types generic"
> >   devargs: fix unittest
> >   devargs: extend unittest
> >   devargs: deprecate enum rte_devtype based functions
> >   pci: use scan_mode configuration
> >   bus: add configuration interface for buses
> >   devargs: use bus configuration interface to set scanning mode
> >   devargs: use existing functions in rte_eal_devargs_parse()
> >   devargs: add busname string field
> >   devargs: use busname
> >   pci: use busname
> >   vdev: use busname
> >   devargs: pass busname argument when parsing
> >   devargs: remove type field
> >   devargs: remove bus field
> 
> <...>
> 

-- 
Gaëtan Rivet
6WIND