[PATCH] eal/common: fix inconsistent representation of PCI numbers

[PATCH] eal/common: fix inconsistent representation of PCI numbers

[Shani Peretz]

DPDK allows for two ways to specify PCI device numbers:
a full version ("0000:08:00.0") and a short version ("08:00.0").
The problem arises when the application uses one format (e.g., full)
when running testpmd, but then tries to use the other format (e.g., short)
in a subsequent command, leading to a failure.

The cmp_dev_name func, which is responsible for comparing PCI device names,
is not handling the inconsistent PCI number representations correctly.
The suggested fix is to use the pci_parse function, which can parse
the PCI device name and fill a struct rte_pci_addr with the standardized
representation of the PCI number.
By comparing the struct rte_pci_addr instances instead of the string
representations, the application can ensure consistent handling of
PCI device numbers, regardless of the format used.

Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
Cc: jblunck@infradead.org

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test/test_vdev.c            | 10 ++--------
 drivers/bus/pci/pci_common.c    | 10 ++++++++++
 lib/eal/common/eal_common_dev.c | 11 ++++++-----
 lib/eal/common/hotplug_mp.c     | 11 ++---------
 lib/eal/include/bus_driver.h    | 18 ++++++++++++++++++
 lib/eal/include/rte_dev.h       | 16 ++++++++++++++++
 lib/eal/linux/eal_dev.c         | 10 +---------
 lib/eal/version.map             |  3 +++
 8 files changed, 58 insertions(+), 31 deletions(-)

diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
index 3e262f30bc..860fa260af 100644
--- a/app/test/test_vdev.c
+++ b/app/test/test_vdev.c
@@ -20,12 +20,6 @@ static const char * const valid_keys[] = {
 	NULL,
 };
 
-static int
-cmp_dev_name(const struct rte_device *dev, const void *name)
-{
-	return strcmp(rte_dev_name(dev), name);
-}
-
 static int
 cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
 {
@@ -82,7 +76,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test0\n");
 		goto fail;
 	}
-	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
+	dev0 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test0");
 	if (dev0 == NULL) {
 		printf("Cannot find net_null_test0 vdev\n");
 		goto fail;
@@ -93,7 +87,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test1\n");
 		goto fail;
 	}
-	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
+	dev1 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test1");
 	if (dev1 == NULL) {
 		printf("Cannot find net_null_test1 vdev\n");
 		goto fail;
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 889a48d2af..6b3bf070ae 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -501,6 +501,15 @@ rte_pci_dump(FILE *f)
 		pci_dump_one_device(f, dev);
 	}
 }
+static int
+pci_cmp_name(const struct rte_device *dev1, const void *name2)
+{
+	const struct rte_pci_device *dev = RTE_DEV_TO_PCI_CONST(dev1);
+	char name2_addr[sizeof(struct rte_pci_addr)] = {0};
+	dev1->bus->parse(name2, (void *)&name2_addr);
+
+	return memcmp(&name2_addr, &(dev->addr), sizeof(struct rte_pci_addr));
+}
 
 static int
 pci_parse(const char *name, void *addr)
@@ -956,6 +965,7 @@ struct rte_pci_bus rte_pci_bus = {
 		.plug = pci_plug,
 		.unplug = pci_unplug,
 		.parse = pci_parse,
+		.cmp_name = pci_cmp_name,
 		.devargs_parse = rte_pci_devargs_parse,
 		.dma_map = pci_dma_map,
 		.dma_unmap = pci_dma_unmap,
diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index a99252b02f..12d68c3605 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -107,11 +107,12 @@ struct dev_next_ctx {
 #define CLSCTX(ptr) \
 	(((struct dev_next_ctx *)(intptr_t)ptr)->cls_str)
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+int rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
 {
-	const char *name = _name;
+	if (dev1->bus->cmp_name)
+		return dev1->bus->cmp_name(dev1, name2);
 
-	return strcmp(dev->name, name);
+	return strcmp(dev1->name, (const char *)name2);
 }
 
 int
@@ -197,7 +198,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	if (ret)
 		goto err_devarg;
 
-	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
+	dev = da->bus->find_device(NULL, rte_cmp_dev_name, da->name);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find device (%s)",
 			da->name);
@@ -335,7 +336,7 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -ENOENT;
 	}
 
-	dev = bus->find_device(NULL, cmp_dev_name, devname);
+	dev = bus->find_device(NULL, rte_cmp_dev_name, devname);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find plugged device (%s)", devname);
 		return -EINVAL;
diff --git a/lib/eal/common/hotplug_mp.c b/lib/eal/common/hotplug_mp.c
index 17089ca3db..a2623c96c3 100644
--- a/lib/eal/common/hotplug_mp.c
+++ b/lib/eal/common/hotplug_mp.c
@@ -21,13 +21,6 @@ struct mp_reply_bundle {
 	void *peer;
 };
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 /**
  * Secondary to primary request.
  * start from function eal_dev_hotplug_request_to_primary.
@@ -135,7 +128,7 @@ __handle_secondary_request(void *param)
 			goto finish;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da.name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da.name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da.name);
 			ret = -ENOENT;
@@ -262,7 +255,7 @@ static void __handle_primary_request(void *param)
 			goto quit;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da->name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da->name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da->name);
 			ret = -ENOENT;
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
index 7b85a17a09..eba820f36c 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -118,6 +118,23 @@ typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
  */
 typedef int (*rte_bus_parse_t)(const char *name, void *addr);
 
+/**
+ * Bus specific device name comparison function.
+ *
+ * This type of function is used to compare a bus name with an arbitrary
+ * name.
+ *
+ * @param dev
+ *	Device handle.
+ *
+ * @param name
+ *	Name to compare against.
+ *
+ * @return
+ *	0 if the device matches the name. Nonzero otherwise.
+ */
+typedef int (*rte_bus_cmp_name_t)(const struct rte_device *dev, const void *name);
+
 /**
  * Parse bus part of the device arguments.
  *
@@ -258,6 +275,7 @@ struct rte_bus {
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */
 	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 	rte_bus_parse_t parse;       /**< Parse a device name */
+	rte_bus_cmp_name_t cmp_name; /**< Compare device name */
 	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
 	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
 	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
diff --git a/lib/eal/include/rte_dev.h b/lib/eal/include/rte_dev.h
index cefa04f905..a3d666c110 100644
--- a/lib/eal/include/rte_dev.h
+++ b/lib/eal/include/rte_dev.h
@@ -21,6 +21,7 @@ extern "C" {
 
 #include <rte_config.h>
 #include <rte_common.h>
+#include <rte_compat.h>
 #include <rte_log.h>
 
 struct rte_bus;
@@ -170,6 +171,21 @@ int rte_dev_is_probed(const struct rte_device *dev);
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *drvargs);
 
+/**
+ * General device name comparison. Will compare by using the specific bus
+ * compare function or by comparing the names directly.
+ *
+ * @param dev
+ *   Device handle.
+ * @param name
+ *   Name to compare against.
+ * @return
+ *   0 if the device matches the name. Nonzero otherwise.
+ */
+__rte_experimental
+int
+rte_cmp_dev_name(const struct rte_device *dev, const void *name);
+
 /**
  * Add matching devices.
  *
diff --git a/lib/eal/linux/eal_dev.c b/lib/eal/linux/eal_dev.c
index fff4e8fa83..5c8effd7be 100644
--- a/lib/eal/linux/eal_dev.c
+++ b/lib/eal/linux/eal_dev.c
@@ -91,14 +91,6 @@ static void sigbus_handler(int signum, siginfo_t *info,
 	EAL_LOG(DEBUG, "Success to handle SIGBUS for hot-unplug!");
 }
 
-static int cmp_dev_name(const struct rte_device *dev,
-	const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 static int
 dev_uev_socket_fd_create(void)
 {
@@ -280,7 +272,7 @@ dev_uev_handler(__rte_unused void *param)
 				goto failure_handle_err;
 			}
 
-			dev = bus->find_device(NULL, cmp_dev_name,
+			dev = bus->find_device(NULL, rte_cmp_dev_name,
 					       uevent.devname);
 			if (dev == NULL) {
 				EAL_LOG(ERR, "Cannot find device (%s) on "
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 3df50c3fbb..e4355ef890 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -396,6 +396,9 @@ EXPERIMENTAL {
 
 	# added in 24.03
 	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
+
+	# added in 24.07
+	rte_cmp_dev_name;
 };
 
 INTERNAL {
-- 
2.34.1

Re: [PATCH] eal/common: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Mon, 1 Jul 2024 23:01:17 +0300
Shani Peretz <shperetz@nvidia.com> wrote:

> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -501,6 +501,15 @@ rte_pci_dump(FILE *f)
>  		pci_dump_one_device(f, dev);
>  	}
>  }
> +static int
> +pci_cmp_name(const struct rte_device *dev1, const void *name2)

Blank line between functions please

> +{
> +	const struct rte_pci_device *dev = RTE_DEV_TO_PCI_CONST(dev1);
> +	char name2_addr[sizeof(struct rte_pci_addr)] = {0};

This should just be a rte_pci_addr type, not char.
And don't need to initialize it.

> +	dev1->bus->parse(name2, (void *)&name2_addr);

If it was the correct type, cast would not be needed.

> +
> +	return memcmp(&name2_addr, &(dev->addr), sizeof(struct rte_pci_addr));

Use existing rte_pci_addr_cmp()

> +}

[PATCH v3] eal/common: fix inconsistent representation of PCI numbers

[Shani Peretz]

DPDK allows for two ways to specify PCI device numbers:
a full version ("0000:08:00.0") and a short version ("08:00.0").
The problem arises when the application uses one format (e.g., full)
when running testpmd, but then tries to use the other format (e.g., short)
in a subsequent command, leading to a failure.

The cmp_dev_name func, which is responsible for comparing PCI device names,
is not handling the inconsistent PCI number representations correctly.
The suggested fix is to use the pci_parse function, which can parse
the PCI device name and fill a struct rte_pci_addr with the standardized
representation of the PCI number.
By comparing the struct rte_pci_addr instances instead of the string
representations, the application can ensure consistent handling of
PCI device numbers, regardless of the format used.

Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
Cc: jblunck@infradead.org

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test/test_vdev.c            | 10 ++--------
 drivers/bus/pci/pci_common.c    | 11 +++++++++++
 lib/eal/common/eal_common_dev.c | 11 ++++++-----
 lib/eal/common/hotplug_mp.c     | 11 ++---------
 lib/eal/include/bus_driver.h    | 18 ++++++++++++++++++
 lib/eal/include/rte_dev.h       | 16 ++++++++++++++++
 lib/eal/linux/eal_dev.c         | 10 +---------
 lib/eal/version.map             |  3 +++
 8 files changed, 59 insertions(+), 31 deletions(-)

diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
index 3e262f30bc..860fa260af 100644
--- a/app/test/test_vdev.c
+++ b/app/test/test_vdev.c
@@ -20,12 +20,6 @@ static const char * const valid_keys[] = {
 	NULL,
 };
 
-static int
-cmp_dev_name(const struct rte_device *dev, const void *name)
-{
-	return strcmp(rte_dev_name(dev), name);
-}
-
 static int
 cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
 {
@@ -82,7 +76,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test0\n");
 		goto fail;
 	}
-	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
+	dev0 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test0");
 	if (dev0 == NULL) {
 		printf("Cannot find net_null_test0 vdev\n");
 		goto fail;
@@ -93,7 +87,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test1\n");
 		goto fail;
 	}
-	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
+	dev1 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test1");
 	if (dev1 == NULL) {
 		printf("Cannot find net_null_test1 vdev\n");
 		goto fail;
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 889a48d2af..538d491067 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -502,6 +502,16 @@ rte_pci_dump(FILE *f)
 	}
 }
 
+static int
+pci_cmp_name(const struct rte_device *dev, const void *name2)
+{
+	struct rte_pci_addr name2_addr;
+	const struct rte_pci_device *dev1 = RTE_DEV_TO_PCI_CONST(dev);
+
+	dev->bus->parse(name2, &name2_addr);
+	return rte_pci_addr_cmp(&dev1->addr, &name2_addr);
+}
+
 static int
 pci_parse(const char *name, void *addr)
 {
@@ -956,6 +966,7 @@ struct rte_pci_bus rte_pci_bus = {
 		.plug = pci_plug,
 		.unplug = pci_unplug,
 		.parse = pci_parse,
+		.cmp_name = pci_cmp_name,
 		.devargs_parse = rte_pci_devargs_parse,
 		.dma_map = pci_dma_map,
 		.dma_unmap = pci_dma_unmap,
diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index a99252b02f..12d68c3605 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -107,11 +107,12 @@ struct dev_next_ctx {
 #define CLSCTX(ptr) \
 	(((struct dev_next_ctx *)(intptr_t)ptr)->cls_str)
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+int rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
 {
-	const char *name = _name;
+	if (dev1->bus->cmp_name)
+		return dev1->bus->cmp_name(dev1, name2);
 
-	return strcmp(dev->name, name);
+	return strcmp(dev1->name, (const char *)name2);
 }
 
 int
@@ -197,7 +198,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	if (ret)
 		goto err_devarg;
 
-	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
+	dev = da->bus->find_device(NULL, rte_cmp_dev_name, da->name);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find device (%s)",
 			da->name);
@@ -335,7 +336,7 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -ENOENT;
 	}
 
-	dev = bus->find_device(NULL, cmp_dev_name, devname);
+	dev = bus->find_device(NULL, rte_cmp_dev_name, devname);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find plugged device (%s)", devname);
 		return -EINVAL;
diff --git a/lib/eal/common/hotplug_mp.c b/lib/eal/common/hotplug_mp.c
index 17089ca3db..a2623c96c3 100644
--- a/lib/eal/common/hotplug_mp.c
+++ b/lib/eal/common/hotplug_mp.c
@@ -21,13 +21,6 @@ struct mp_reply_bundle {
 	void *peer;
 };
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 /**
  * Secondary to primary request.
  * start from function eal_dev_hotplug_request_to_primary.
@@ -135,7 +128,7 @@ __handle_secondary_request(void *param)
 			goto finish;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da.name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da.name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da.name);
 			ret = -ENOENT;
@@ -262,7 +255,7 @@ static void __handle_primary_request(void *param)
 			goto quit;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da->name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da->name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da->name);
 			ret = -ENOENT;
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
index 7b85a17a09..eba820f36c 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -118,6 +118,23 @@ typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
  */
 typedef int (*rte_bus_parse_t)(const char *name, void *addr);
 
+/**
+ * Bus specific device name comparison function.
+ *
+ * This type of function is used to compare a bus name with an arbitrary
+ * name.
+ *
+ * @param dev
+ *	Device handle.
+ *
+ * @param name
+ *	Name to compare against.
+ *
+ * @return
+ *	0 if the device matches the name. Nonzero otherwise.
+ */
+typedef int (*rte_bus_cmp_name_t)(const struct rte_device *dev, const void *name);
+
 /**
  * Parse bus part of the device arguments.
  *
@@ -258,6 +275,7 @@ struct rte_bus {
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */
 	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 	rte_bus_parse_t parse;       /**< Parse a device name */
+	rte_bus_cmp_name_t cmp_name; /**< Compare device name */
 	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
 	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
 	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
diff --git a/lib/eal/include/rte_dev.h b/lib/eal/include/rte_dev.h
index cefa04f905..a3d666c110 100644
--- a/lib/eal/include/rte_dev.h
+++ b/lib/eal/include/rte_dev.h
@@ -21,6 +21,7 @@ extern "C" {
 
 #include <rte_config.h>
 #include <rte_common.h>
+#include <rte_compat.h>
 #include <rte_log.h>
 
 struct rte_bus;
@@ -170,6 +171,21 @@ int rte_dev_is_probed(const struct rte_device *dev);
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *drvargs);
 
+/**
+ * General device name comparison. Will compare by using the specific bus
+ * compare function or by comparing the names directly.
+ *
+ * @param dev
+ *   Device handle.
+ * @param name
+ *   Name to compare against.
+ * @return
+ *   0 if the device matches the name. Nonzero otherwise.
+ */
+__rte_experimental
+int
+rte_cmp_dev_name(const struct rte_device *dev, const void *name);
+
 /**
  * Add matching devices.
  *
diff --git a/lib/eal/linux/eal_dev.c b/lib/eal/linux/eal_dev.c
index fff4e8fa83..5c8effd7be 100644
--- a/lib/eal/linux/eal_dev.c
+++ b/lib/eal/linux/eal_dev.c
@@ -91,14 +91,6 @@ static void sigbus_handler(int signum, siginfo_t *info,
 	EAL_LOG(DEBUG, "Success to handle SIGBUS for hot-unplug!");
 }
 
-static int cmp_dev_name(const struct rte_device *dev,
-	const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 static int
 dev_uev_socket_fd_create(void)
 {
@@ -280,7 +272,7 @@ dev_uev_handler(__rte_unused void *param)
 				goto failure_handle_err;
 			}
 
-			dev = bus->find_device(NULL, cmp_dev_name,
+			dev = bus->find_device(NULL, rte_cmp_dev_name,
 					       uevent.devname);
 			if (dev == NULL) {
 				EAL_LOG(ERR, "Cannot find device (%s) on "
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 3df50c3fbb..e4355ef890 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -396,6 +396,9 @@ EXPERIMENTAL {
 
 	# added in 24.03
 	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
+
+	# added in 24.07
+	rte_cmp_dev_name;
 };
 
 INTERNAL {
-- 
2.34.1

Re: [PATCH v3] eal/common: fix inconsistent representation of PCI numbers

[David Marchand]

On Mon, Jul 8, 2024 at 6:52 PM Shani Peretz <shperetz@nvidia.com> wrote:
>
> DPDK allows for two ways to specify PCI device numbers:
> a full version ("0000:08:00.0") and a short version ("08:00.0").
> The problem arises when the application uses one format (e.g., full)
> when running testpmd, but then tries to use the other format (e.g., short)
> in a subsequent command, leading to a failure.
>
> The cmp_dev_name func, which is responsible for comparing PCI device names,
> is not handling the inconsistent PCI number representations correctly.
> The suggested fix is to use the pci_parse function, which can parse
> the PCI device name and fill a struct rte_pci_addr with the standardized
> representation of the PCI number.
> By comparing the struct rte_pci_addr instances instead of the string
> representations, the application can ensure consistent handling of
> PCI device numbers, regardless of the format used.
>
> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> Cc: jblunck@infradead.org
>
> Signed-off-by: Shani Peretz <shperetz@nvidia.com>
> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

I find it strange that Thomas acked this patch (for example, the
commit title prefix is wrong).

I don't understand the issue.
Please provide a reproducer.
And ideally we need a unit test to track regressions on this topic.


Thanks.

-- 
David Marchand

Re: [PATCH v3] eal/common: fix inconsistent representation of PCI numbers

[Thomas Monjalon]

12/07/2024 15:49, David Marchand:
> On Mon, Jul 8, 2024 at 6:52 PM Shani Peretz <shperetz@nvidia.com> wrote:
> >
> > DPDK allows for two ways to specify PCI device numbers:
> > a full version ("0000:08:00.0") and a short version ("08:00.0").
> > The problem arises when the application uses one format (e.g., full)
> > when running testpmd, but then tries to use the other format (e.g., short)
> > in a subsequent command, leading to a failure.
> >
> > The cmp_dev_name func, which is responsible for comparing PCI device names,
> > is not handling the inconsistent PCI number representations correctly.
> > The suggested fix is to use the pci_parse function, which can parse
> > the PCI device name and fill a struct rte_pci_addr with the standardized
> > representation of the PCI number.
> > By comparing the struct rte_pci_addr instances instead of the string
> > representations, the application can ensure consistent handling of
> > PCI device numbers, regardless of the format used.
> >
> > Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> > Cc: jblunck@infradead.org
> >
> > Signed-off-by: Shani Peretz <shperetz@nvidia.com>
> > Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> I find it strange that Thomas acked this patch (for example, the
> commit title prefix is wrong).
> 
> I don't understand the issue.
> Please provide a reproducer.
> And ideally we need a unit test to track regressions on this topic.

Indeed, Shani we shouldn't add "Acked-by" until all is reviewed in detail.

Re: [PATCH] eal/common: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Mon, 1 Jul 2024 23:01:17 +0300
Shani Peretz <shperetz@nvidia.com> wrote:

> +/**
> + * General device name comparison. Will compare by using the specific bus
> + * compare function or by comparing the names directly.
> + *
> + * @param dev
> + *   Device handle.
> + * @param name
> + *   Name to compare against.
> + * @return
> + *   0 if the device matches the name. Nonzero otherwise.
> + */
> +__rte_experimental
> +int
> +rte_cmp_dev_name(const struct rte_device *dev, const void *name);

Why is this not rte_internal instead of experimental?
Why is the name arguement void * instead of char *?

And there are other questions in the mail thread.
That is why it has not gotten accepted.

[PATCH v4] bus: fix inconsistent representation of PCI numbers

[Shani Peretz]

DPDK provides two formats for specifying PCI device numbers:
a full version ("0000:08:00.0") and a short version ("08:00.0").
Issues can occur when an application uses one format (e.g., short)
while running testpmd, then attempts to use the other format
(e.g., full) in a later command, resulting in a failure.

The issue is that find_device goes over the list of devices and
compares the user-provided string to the rte_device structure's
device->name (device->name is just the string received from devargs
(i.e "08:00.0" or "0000:08:00.0")).
Notice that there's another field that represents the device name,
but this one is in the rte_pci_bus struct. This name is actually the result
of the PCI parse function ("0000:08:00.0").
If we want to accurately compare these names, we'll need to bring both
sides to the same representation by invoking the parse function on
the user input.

To make the cmp_dev_name function applicable to all buses—not just PCI—
the proposed solution is to utilize the parse function implemented by
each bus. When comparing names, we will call parse on the supplied
string as well as on the device name itself and compare the results.
This will allow consistent comparisons between different representations
of same devices.

Also, the pci_common_set function has been modified to improve naming
consistency for PCI buses.
Now, the name stored in rte_device for PCI buses will match the parsed
name that is also stored in rte_pci_device name,
rather than using the user-provided string from devargs.
As a result, when a new PCI device is registered, the name displayed in
the device list will be the parsed version.

Added tests that compare and find devices in various forms of names
under test_devargs.

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

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 app/test/test_devargs.c                  | 123 ++++++++++++++++++++++-
 app/test/test_vdev.c                     |  10 +-
 drivers/bus/auxiliary/auxiliary_common.c |  17 +++-
 drivers/bus/cdx/cdx.c                    |  13 ++-
 drivers/bus/dpaa/dpaa_bus.c              |   7 +-
 drivers/bus/fslmc/fslmc_bus.c            |   9 +-
 drivers/bus/ifpga/ifpga_bus.c            |  14 ++-
 drivers/bus/pci/pci_common.c             |  22 ++--
 drivers/bus/platform/platform.c          |  15 ++-
 drivers/bus/uacce/uacce.c                |  14 ++-
 drivers/bus/vdev/vdev.c                  |  23 ++++-
 drivers/bus/vmbus/vmbus_common.c         |   9 +-
 drivers/dma/idxd/idxd_bus.c              |   9 +-
 drivers/raw/ifpga/ifpga_rawdev.c         |   8 +-
 lib/eal/common/eal_common_bus.c          |   2 +-
 lib/eal/common/eal_common_dev.c          |  41 +++++++-
 lib/eal/common/hotplug_mp.c              |  11 +-
 lib/eal/include/bus_driver.h             |  24 ++++-
 lib/eal/include/rte_dev.h                |  15 +++
 lib/eal/linux/eal_dev.c                  |  10 +-
 lib/eal/version.map                      |   1 +
 21 files changed, 305 insertions(+), 92 deletions(-)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 4166b2bea2..1c21e7f6fe 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -12,7 +12,9 @@
 #include <rte_kvargs.h>
 #include <bus_driver.h>
 #include <rte_class.h>
+#include <rte_bus_vdev.h>
 
+#include "virtual_pmd.h"
 #include "test.h"
 
 /* Check layer arguments. */
@@ -171,7 +173,7 @@ test_valid_devargs(void)
 	int ret;
 
 	ret = test_valid_devargs_cases(list, RTE_DIM(list));
-	if (vdev_bus != NULL && vdev_bus->parse("net_ring0", NULL) == 0)
+	if (vdev_bus != NULL && vdev_bus->parse("net_ring0", NULL, NULL) == 0)
 		/* Ring vdev driver enabled. */
 		ret |= test_valid_devargs_cases(legacy_ring_list,
 						RTE_DIM(legacy_ring_list));
@@ -302,6 +304,119 @@ test_invalid_devargs_parsing(void)
 	return fail;
 }
 
+static struct rte_device *
+create_pci_dev(const char *name)
+{
+	int port_id;
+	uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
+	struct rte_ether_addr *mac_addr = (struct rte_ether_addr *)slave_mac1;
+	char pmd_name[RTE_ETH_NAME_MAX_LEN];
+
+	mac_addr->addr_bytes[RTE_ETHER_ADDR_LEN - 1] = 0;
+	strlcpy(pmd_name, name, RTE_ETH_NAME_MAX_LEN);
+
+	port_id = virtual_ethdev_create(pmd_name,
+			mac_addr, rte_socket_id(), 1);
+
+	if (port_id < 0)
+		return NULL;
+
+	return (&rte_eth_devices[port_id])->device;
+}
+
+static int
+test_pci(struct rte_bus *pci_bus, const char *dev_name, const char *name2)
+{
+	struct rte_device *pci_dev = create_pci_dev(dev_name);
+
+	if (pci_dev == NULL)
+		return -1;
+
+	pci_dev->bus = pci_bus;
+
+	if (rte_cmp_dev_name(pci_dev, name2) != 0) {
+		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
+			       pci_dev->name, name2, pci_dev->name, name2);
+		return -1;
+	}
+	return 0;
+}
+
+static int
+test_vdev(struct rte_bus *vdev_bus, const char *dev_name, const char *name2)
+{
+	/* create vdev */
+	if (rte_vdev_init(dev_name, "") < 0) {
+		printf("Failed to create vdev %s\n", dev_name);
+		return -1;
+	}
+
+	struct rte_device *vdev_dev = vdev_bus->find_device(NULL, rte_cmp_dev_name, dev_name);
+	if (vdev_dev == NULL) {
+		printf("Cannot find %s vdev\n", dev_name);
+		rte_vdev_uninit(dev_name);
+		return -1;
+	}
+	int ret = rte_cmp_dev_name(vdev_dev, name2);
+	if (ret != 0) {
+		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
+			       vdev_dev->name, name2, vdev_dev->name, name2);
+		return -1;
+	}
+
+	if (vdev_dev != vdev_bus->find_device(NULL, rte_cmp_dev_name, name2)) {
+		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
+			       vdev_dev->name, name2, vdev_dev->name, name2);
+		return -1;
+	}
+
+	rte_vdev_uninit(dev_name);
+	return 0;
+}
+
+static int
+test_valid_cmp_dev_name(void)
+{
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+
+	if (pci_bus) {
+		if ((test_pci(pci_bus, "0000:08:11.0", "08:11.0") != 0) ||
+			(test_pci(pci_bus, "08:12.0",  "0000:08:12.0") != 0) ||
+			(test_pci(pci_bus, "08:13.0",  "08:13.0") != 0) ||
+			(test_pci(pci_bus, "0000:08:14.0",  "0000:08:14.0") != 0))
+			return -1;
+	}
+
+	if (vdev_bus) {
+		if (test_vdev(vdev_bus, "net_null_test0", "net_null_test0") != 0)
+			return -1;
+	}
+	return 0;
+}
+
+static int
+test_invalid_cmp_dev_name(void)
+{
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+
+	if (pci_bus) {
+		if ((test_pci(pci_bus, "0000:08:15.0", "08:00.0") == 0) ||
+			(test_pci(pci_bus, "08:16.0",  "0000:08:15.0") == 0) ||
+			(test_pci(pci_bus, "08:17.0",  "08:13.0") == 0) ||
+			(test_pci(pci_bus, "0000:08:18.0",  "0000:08:14.0") == 0))
+			return -1;
+	}
+
+	if (vdev_bus) {
+		if ((test_vdev(vdev_bus, "net_null_test0", "net_null_test") == 0) ||
+			(test_vdev(vdev_bus, "net_null_test1", "net_null_test2") == 0))
+			return -1;
+	}
+	return 0;
+}
+
 static int
 test_devargs(void)
 {
@@ -317,6 +432,12 @@ test_devargs(void)
 	printf("== test devargs parsing invalid case ==\n");
 	if (test_invalid_devargs_parsing() < 0)
 		return -1;
+	printf("== test find device valid case ==\n");
+	if (test_valid_cmp_dev_name() < 0)
+		return -1;
+	printf("== test find device invalid case ==\n");
+	if (test_invalid_cmp_dev_name() < 0)
+		return -1;
 	return 0;
 }
 
diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
index 3e262f30bc..860fa260af 100644
--- a/app/test/test_vdev.c
+++ b/app/test/test_vdev.c
@@ -20,12 +20,6 @@ static const char * const valid_keys[] = {
 	NULL,
 };
 
-static int
-cmp_dev_name(const struct rte_device *dev, const void *name)
-{
-	return strcmp(rte_dev_name(dev), name);
-}
-
 static int
 cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
 {
@@ -82,7 +76,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test0\n");
 		goto fail;
 	}
-	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
+	dev0 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test0");
 	if (dev0 == NULL) {
 		printf("Cannot find net_null_test0 vdev\n");
 		goto fail;
@@ -93,7 +87,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test1\n");
 		goto fail;
 	}
-	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
+	dev1 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test1");
 	if (dev1 == NULL) {
 		printf("Cannot find net_null_test1 vdev\n");
 		goto fail;
diff --git a/drivers/bus/auxiliary/auxiliary_common.c b/drivers/bus/auxiliary/auxiliary_common.c
index e6cbc4d356..ba2f69e851 100644
--- a/drivers/bus/auxiliary/auxiliary_common.c
+++ b/drivers/bus/auxiliary/auxiliary_common.c
@@ -237,10 +237,9 @@ auxiliary_probe(void)
 }
 
 static int
-auxiliary_parse(const char *name, void *addr)
+auxiliary_parse(const char *name, void *addr, int *size)
 {
 	struct rte_auxiliary_driver *drv = NULL;
-	const char **out = addr;
 
 	/* Allow empty device name "auxiliary:" to bypass entire bus scan. */
 	if (strlen(name) == 0)
@@ -250,9 +249,17 @@ auxiliary_parse(const char *name, void *addr)
 		if (drv->match(name))
 			break;
 	}
-	if (drv != NULL && addr != NULL)
-		*out = name;
-	return drv != NULL ? 0 : -1;
+
+	if (drv == NULL)
+		return -1;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
+
+	return 0;
 }
 
 /* Register a driver */
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index 62b108e082..b97f1ce1af 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -464,15 +464,20 @@ cdx_probe(void)
 }
 
 static int
-cdx_parse(const char *name, void *addr)
+cdx_parse(const char *name, void *addr, int *size)
 {
-	const char **out = addr;
 	int ret;
 
 	ret = strncmp(name, CDX_DEV_PREFIX, strlen(CDX_DEV_PREFIX));
 
-	if (ret == 0 && addr)
-		*out = name;
+	if (ret != 0)
+		return ret;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
 
 	return ret;
 }
diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
index 9ffbe07c93..f768fb9454 100644
--- a/drivers/bus/dpaa/dpaa_bus.c
+++ b/drivers/bus/dpaa/dpaa_bus.c
@@ -153,7 +153,7 @@ dpaa_devargs_lookup(struct rte_dpaa_device *dev)
 	char dev_name[32];
 
 	RTE_EAL_DEVARGS_FOREACH("dpaa_bus", devargs) {
-		devargs->bus->parse(devargs->name, &dev_name);
+		devargs->bus->parse(devargs->name, &dev_name, NULL);
 		if (strcmp(dev_name, dev->device.name) == 0) {
 			DPAA_BUS_INFO("**Devargs matched %s", dev_name);
 			return devargs;
@@ -447,7 +447,7 @@ dpaa_portal_finish(void *arg)
 }
 
 static int
-rte_dpaa_bus_parse(const char *name, void *out)
+rte_dpaa_bus_parse(const char *name, void *out, int *size)
 {
 	unsigned int i, j;
 	size_t delta, dev_delta;
@@ -494,6 +494,9 @@ rte_dpaa_bus_parse(const char *name, void *out)
 		max_name_len = sizeof("fm.-mac..") - 1;
 	}
 
+	if (size != NULL)
+		*size = max_name_len + 1;
+
 	if (out != NULL) {
 		char *out_name = out;
 
diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 68ad2b801e..a525b5e245 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -103,7 +103,7 @@ fslmc_devargs_lookup(struct rte_dpaa2_device *dev)
 	char dev_name[32];
 
 	RTE_EAL_DEVARGS_FOREACH("fslmc", devargs) {
-		devargs->bus->parse(devargs->name, &dev_name);
+		devargs->bus->parse(devargs->name, &dev_name, NULL);
 		if (strcmp(dev_name, dev->device.name) == 0) {
 			DPAA2_BUS_INFO("**Devargs matched %s", dev_name);
 			return devargs;
@@ -235,7 +235,7 @@ scan_one_fslmc_device(char *dev_name)
 }
 
 static int
-rte_fslmc_parse(const char *name, void *addr)
+rte_fslmc_parse(const char *name, void *addr, int *size)
 {
 	uint16_t dev_id;
 	char *t_ptr;
@@ -298,7 +298,10 @@ rte_fslmc_parse(const char *name, void *addr)
 		goto err_out;
 	}
 
-	if (addr)
+	if (size != NULL)
+		*size = strlen(sep) + 1;
+
+	if (addr != NULL)
 		strcpy(addr, sep);
 
 	ret = 0;
diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c
index 11b31eee4f..d5d9a86a97 100644
--- a/drivers/bus/ifpga/ifpga_bus.c
+++ b/drivers/bus/ifpga/ifpga_bus.c
@@ -459,9 +459,8 @@ ifpga_find_device(const struct rte_device *start,
 	return NULL;
 }
 static int
-ifpga_parse(const char *name, void *addr)
+ifpga_parse(const char *name, void *addr, int *size)
 {
-	int *out = addr;
 	struct rte_rawdev *rawdev = NULL;
 	char rawdev_name[RTE_RAWDEV_NAME_MAX_LEN];
 	char *c1 = NULL;
@@ -491,9 +490,14 @@ ifpga_parse(const char *name, void *addr)
 	rawdev = rte_rawdev_pmd_get_named_dev(rawdev_name);
 
 	if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
-		rawdev &&
-		(addr != NULL))
-		*out = port;
+		rawdev) {
+		if (size != NULL)
+			*size = sizeof(port);
+
+		if (addr != NULL)
+			*(int *)addr = port;
+		}
+
 
 	if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
 		rawdev)
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 1173f0887c..25e161f321 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -84,7 +84,7 @@ pci_devargs_lookup(const struct rte_pci_addr *pci_addr)
 	struct rte_pci_addr addr;
 
 	RTE_EAL_DEVARGS_FOREACH("pci", devargs) {
-		devargs->bus->parse(devargs->name, &addr);
+		devargs->bus->parse(devargs->name, &addr, NULL);
 		if (!rte_pci_addr_cmp(pci_addr, &addr))
 			return devargs;
 	}
@@ -99,21 +99,11 @@ pci_common_set(struct rte_pci_device *dev)
 	/* Each device has its internal, canonical name set. */
 	rte_pci_device_name(&dev->addr,
 			dev->name, sizeof(dev->name));
+	dev->device.name = dev->name;
+
 	devargs = pci_devargs_lookup(&dev->addr);
 	dev->device.devargs = devargs;
 
-	/* When using a blocklist, only blocked devices will have
-	 * an rte_devargs. Allowed devices won't have one.
-	 */
-	if (devargs != NULL)
-		/* If an rte_devargs exists, the generic rte_device uses the
-		 * given name as its name.
-		 */
-		dev->device.name = dev->device.devargs->name;
-	else
-		/* Otherwise, it uses the internal, canonical form. */
-		dev->device.name = dev->name;
-
 	if (dev->bus_info != NULL ||
 			asprintf(&dev->bus_info, "vendor_id=%"PRIx16", device_id=%"PRIx16,
 				dev->id.vendor_id, dev->id.device_id) != -1)
@@ -497,15 +487,19 @@ rte_pci_dump(FILE *f)
 }
 
 static int
-pci_parse(const char *name, void *addr)
+pci_parse(const char *name, void *addr, int *size)
 {
 	struct rte_pci_addr *out = addr;
 	struct rte_pci_addr pci_addr;
 	bool parse;
 
+	if (size != NULL)
+		*size = sizeof(struct rte_pci_addr);
+
 	parse = (rte_pci_addr_parse(name, &pci_addr) == 0);
 	if (parse && addr != NULL)
 		*out = pci_addr;
+
 	return parse == false;
 }
 
diff --git a/drivers/bus/platform/platform.c b/drivers/bus/platform/platform.c
index 11892caa24..87ed7404e1 100644
--- a/drivers/bus/platform/platform.c
+++ b/drivers/bus/platform/platform.c
@@ -542,11 +542,10 @@ platform_bus_unplug(struct rte_device *dev)
 }
 
 static int
-platform_bus_parse(const char *name, void *addr)
+platform_bus_parse(const char *name, void *addr, int *size)
 {
 	struct rte_platform_device pdev = { };
 	struct rte_platform_driver *pdrv;
-	const char **out = addr;
 
 	rte_strscpy(pdev.name, name, sizeof(pdev.name));
 
@@ -555,10 +554,16 @@ platform_bus_parse(const char *name, void *addr)
 			break;
 	}
 
-	if (pdrv != NULL && addr != NULL)
-		*out = name;
+	if (pdrv == NULL)
+		return -ENODEV;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
 
-	return pdrv != NULL ? 0 : -ENODEV;
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
+
+	return 0;
 }
 
 static int
diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
index 9ca048122d..b18fbe1103 100644
--- a/drivers/bus/uacce/uacce.c
+++ b/drivers/bus/uacce/uacce.c
@@ -20,6 +20,7 @@
 #include <rte_errno.h>
 #include <rte_log.h>
 #include <rte_kvargs.h>
+#include <rte_string_fns.h>
 #include <bus_driver.h>
 
 #include "bus_uacce_driver.h"
@@ -529,15 +530,20 @@ uacce_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,  const void
 }
 
 static int
-uacce_parse(const char *name, void *addr)
+uacce_parse(const char *name, void *addr, int *size)
 {
-	const char **out = addr;
 	int ret;
 
 	ret = strncmp(name, UACCE_DEV_PREFIX, strlen(UACCE_DEV_PREFIX));
 
-	if (ret == 0 && addr)
-		*out = name;
+	if (ret != 0)
+		return ret;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
 
 	return ret;
 }
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index ec7abe7cda..cc87461f27 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -113,7 +113,7 @@ rte_vdev_remove_custom_scan(rte_vdev_scan_callback callback, void *user_arg)
 }
 
 static int
-vdev_parse(const char *name, void *addr)
+vdev_find_driver(const char *name, void *addr)
 {
 	struct rte_vdev_driver **out = addr;
 	struct rte_vdev_driver *driver = NULL;
@@ -197,7 +197,7 @@ vdev_probe_all_drivers(struct rte_vdev_device *dev)
 	name = rte_vdev_device_name(dev);
 	VDEV_LOG(DEBUG, "Search driver to probe device %s", name);
 
-	if (vdev_parse(name, &driver))
+	if (vdev_find_driver(name, &driver))
 		return -1;
 
 	iova_mode = rte_eal_iova_mode();
@@ -232,6 +232,23 @@ find_vdev(const char *name)
 	return NULL;
 }
 
+static int
+vdev_parse(const char *name, void *addr, int *size)
+{
+	struct rte_vdev_device *dev = find_vdev(name);
+
+	if (dev == NULL)
+		return 1;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
+
+	return 0;
+}
+
 static struct rte_devargs *
 alloc_devargs(const char *name, const char *args)
 {
@@ -647,7 +664,7 @@ vdev_get_iommu_class(void)
 
 	TAILQ_FOREACH(dev, &vdev_device_list, next) {
 		name = rte_vdev_device_name(dev);
-		if (vdev_parse(name, &driver))
+		if (vdev_find_driver(name, &driver))
 			continue;
 
 		if (driver->drv_flags & RTE_VDEV_DRV_NEED_IOVA_AS_VA)
diff --git a/drivers/bus/vmbus/vmbus_common.c b/drivers/bus/vmbus/vmbus_common.c
index 8a965d10d9..18805aee0d 100644
--- a/drivers/bus/vmbus/vmbus_common.c
+++ b/drivers/bus/vmbus/vmbus_common.c
@@ -245,13 +245,16 @@ rte_vmbus_cleanup(void)
 }
 
 static int
-vmbus_parse(const char *name, void *addr)
+vmbus_parse(const char *name, void *addr, int *size)
 {
 	rte_uuid_t guid;
 	int ret;
 
+	if (size != NULL)
+		*size = sizeof(guid);
+
 	ret = rte_uuid_parse(name, guid);
-	if (ret == 0 && addr)
+	if (ret == 0 && addr != NULL)
 		memcpy(addr, &guid, sizeof(guid));
 
 	return ret;
@@ -269,7 +272,7 @@ vmbus_devargs_lookup(struct rte_vmbus_device *dev)
 	rte_uuid_t addr;
 
 	RTE_EAL_DEVARGS_FOREACH("vmbus", devargs) {
-		vmbus_parse(devargs->name, &addr);
+		vmbus_parse(devargs->name, &addr, NULL);
 
 		if (rte_uuid_compare(dev->device_id, addr) == 0)
 			return devargs;
diff --git a/drivers/dma/idxd/idxd_bus.c b/drivers/dma/idxd/idxd_bus.c
index ba8076715d..d68e8b630e 100644
--- a/drivers/dma/idxd/idxd_bus.c
+++ b/drivers/dma/idxd/idxd_bus.c
@@ -47,7 +47,7 @@ static int dsa_probe(void);
 static struct rte_device *dsa_find_device(const struct rte_device *start,
 		rte_dev_cmp_t cmp,  const void *data);
 static enum rte_iova_mode dsa_get_iommu_class(void);
-static int dsa_addr_parse(const char *name, void *addr);
+static int dsa_addr_parse(const char *name, void *addr, int *size);
 
 /** List of devices */
 TAILQ_HEAD(dsa_device_list, rte_dsa_device);
@@ -345,7 +345,7 @@ dsa_scan(void)
 			closedir(dev_dir);
 			return -ENOMEM;
 		}
-		if (dsa_addr_parse(wq->d_name, &dev->addr) < 0) {
+		if (dsa_addr_parse(wq->d_name, &dev->addr, NULL) < 0) {
 			IDXD_PMD_ERR("Error parsing WQ name: %s", wq->d_name);
 			free(dev);
 			continue;
@@ -391,11 +391,14 @@ dsa_get_iommu_class(void)
 }
 
 static int
-dsa_addr_parse(const char *name, void *addr)
+dsa_addr_parse(const char *name, void *addr, int *size)
 {
 	struct dsa_wq_addr *wq = addr;
 	unsigned int device_id, wq_id;
 
+	if (size != NULL)
+		*size = sizeof(struct dsa_wq_addr);
+
 	if (sscanf(name, "wq%u.%u", &device_id, &wq_id) != 2) {
 		IDXD_PMD_DEBUG("Parsing WQ name failed: %s", name);
 		return -1;
diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index 5b9b596435..7238246e1c 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -1833,12 +1833,6 @@ ifpga_cfg_probe(struct rte_vdev_device *vdev)
 	return ret;
 }
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
-{
-	const char *name = _name;
-	return strcmp(dev->name, name);
-}
-
 static int
 ifpga_cfg_remove(struct rte_vdev_device *vdev)
 {
@@ -1874,7 +1868,7 @@ ifpga_cfg_remove(struct rte_vdev_device *vdev)
 		args.port, args.bdf);
 	bus = rte_bus_find_by_name(RTE_STR(IFPGA_BUS_NAME));
 	if (bus) {
-		if (bus->find_device(NULL, cmp_dev_name, dev_name)) {
+		if (bus->find_device(NULL, rte_cmp_dev_name, dev_name)) {
 			ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
 				dev_name);
 		}
diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
index 7cbd09c421..b1bb5df5b7 100644
--- a/lib/eal/common/eal_common_bus.c
+++ b/lib/eal/common/eal_common_bus.c
@@ -200,7 +200,7 @@ bus_can_parse(const struct rte_bus *bus, const void *_name)
 {
 	const char *name = _name;
 
-	return !(bus->parse && bus->parse(name, NULL) == 0);
+	return !(bus->parse && bus->parse(name, NULL, NULL) == 0);
 }
 
 struct rte_bus *
diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index 70aa04dcd9..c0e2202482 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -107,11 +107,42 @@ struct dev_next_ctx {
 #define CLSCTX(ptr) \
 	(((struct dev_next_ctx *)(intptr_t)ptr)->cls_str)
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+int
+rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
 {
-	const char *name = _name;
+	void *parsed_name1;
+	void *parsed_name2;
+	int size1 = 0;
+	int size2 = 0;
+	int ret;
+
+	if (dev1->bus->parse(dev1->name, NULL, &size1) != 0 ||
+		dev1->bus->parse(name2, NULL, &size2) != 0)
+		return 1;
+
+	if (size1 != size2)
+		return 1;
+
+	parsed_name1 = malloc(size1);
+	if (parsed_name1 == NULL)
+		return 1;
+
+	parsed_name2 = malloc(size2);
+	if (parsed_name2 == NULL) {
+		free(parsed_name1);
+		return 1;
+	}
 
-	return strcmp(dev->name, name);
+	memset(parsed_name1, 0, size1);
+	memset(parsed_name2, 0, size2);
+
+	dev1->bus->parse(dev1->name, parsed_name1, NULL);
+	dev1->bus->parse(name2, parsed_name2, NULL);
+
+	ret = memcmp(parsed_name1, parsed_name2, size1);
+	free(parsed_name1);
+	free(parsed_name2);
+	return ret;
 }
 
 int
@@ -197,7 +228,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	if (ret)
 		goto err_devarg;
 
-	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
+	dev = da->bus->find_device(NULL, rte_cmp_dev_name, da->name);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find device (%s)",
 			da->name);
@@ -335,7 +366,7 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -ENOENT;
 	}
 
-	dev = bus->find_device(NULL, cmp_dev_name, devname);
+	dev = bus->find_device(NULL, rte_cmp_dev_name, devname);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find plugged device (%s)", devname);
 		return -EINVAL;
diff --git a/lib/eal/common/hotplug_mp.c b/lib/eal/common/hotplug_mp.c
index 17089ca3db..a2623c96c3 100644
--- a/lib/eal/common/hotplug_mp.c
+++ b/lib/eal/common/hotplug_mp.c
@@ -21,13 +21,6 @@ struct mp_reply_bundle {
 	void *peer;
 };
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 /**
  * Secondary to primary request.
  * start from function eal_dev_hotplug_request_to_primary.
@@ -135,7 +128,7 @@ __handle_secondary_request(void *param)
 			goto finish;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da.name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da.name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da.name);
 			ret = -ENOENT;
@@ -262,7 +255,7 @@ static void __handle_primary_request(void *param)
 			goto quit;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da->name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da->name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da->name);
 			ret = -ENOENT;
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
index 60527b75b6..01447670de 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -112,11 +112,32 @@ typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
  *	should be written. If NULL, nothing should be written, which
  *	is not an error.
  *
+ * @param[out] size
+ *	device information size. If NULL, nothing should
+ *	be written, which is not an error.
+ *
  * @return
  *	0 if parsing was successful.
  *	!0 for any error.
  */
-typedef int (*rte_bus_parse_t)(const char *name, void *addr);
+typedef int (*rte_bus_parse_t)(const char *name, void *addr, int *size);
+
+/**
+ * Bus specific device name comparison function.
+ *
+ * This type of function is used to compare a bus name with an arbitrary
+ * name.
+ *
+ * @param dev
+ *	Device handle.
+ *
+ * @param name
+ *	Name to compare against.
+ *
+ * @return
+ *	0 if the device matches the name. Nonzero otherwise.
+ */
+typedef int (*rte_bus_cmp_name_t)(const struct rte_device *dev, const void *name);
 
 /**
  * Parse bus part of the device arguments.
@@ -258,6 +279,7 @@ struct rte_bus {
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */
 	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 	rte_bus_parse_t parse;       /**< Parse a device name */
+	rte_bus_cmp_name_t cmp_name; /**< Compare device name */
 	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
 	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
 	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
diff --git a/lib/eal/include/rte_dev.h b/lib/eal/include/rte_dev.h
index 738400e8d1..2df8143af1 100644
--- a/lib/eal/include/rte_dev.h
+++ b/lib/eal/include/rte_dev.h
@@ -17,6 +17,7 @@
 
 #include <rte_config.h>
 #include <rte_common.h>
+#include <rte_compat.h>
 #include <rte_log.h>
 
 #ifdef __cplusplus
@@ -170,6 +171,20 @@ int rte_dev_is_probed(const struct rte_device *dev);
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *drvargs);
 
+/**
+ * General device name comparison. Will compare by using the specific bus
+ * compare function or by comparing the names directly.
+ *
+ * @param dev
+ *   Device handle.
+ * @param name
+ *   Name to compare against.
+ * @return
+ *   0 if the device matches the name. Nonzero otherwise.
+ */
+__rte_internal
+int rte_cmp_dev_name(const struct rte_device *dev, const void *name);
+
 /**
  * Add matching devices.
  *
diff --git a/lib/eal/linux/eal_dev.c b/lib/eal/linux/eal_dev.c
index e63f24d108..3b68cda87f 100644
--- a/lib/eal/linux/eal_dev.c
+++ b/lib/eal/linux/eal_dev.c
@@ -91,14 +91,6 @@ static void sigbus_handler(int signum, siginfo_t *info,
 	EAL_LOG(DEBUG, "Success to handle SIGBUS for hot-unplug!");
 }
 
-static int cmp_dev_name(const struct rte_device *dev,
-	const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 static int
 dev_uev_socket_fd_create(void)
 {
@@ -280,7 +272,7 @@ dev_uev_handler(__rte_unused void *param)
 				goto failure_handle_err;
 			}
 
-			dev = bus->find_device(NULL, cmp_dev_name,
+			dev = bus->find_device(NULL, rte_cmp_dev_name,
 					       uevent.devname);
 			if (dev == NULL) {
 				EAL_LOG(ERR, "Cannot find device (%s) on "
diff --git a/lib/eal/version.map b/lib/eal/version.map
index a20c713eb1..e50a95fd31 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -448,4 +448,5 @@ INTERNAL {
 	rte_mem_unmap;
 	rte_thread_create_internal_control;
 	rte_thread_set_prefixed_name;
+	rte_cmp_dev_name;
 };
-- 
2.34.1

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Bruce Richardson]

On Wed, Jan 29, 2025 at 10:54:16AM +0200, Shani Peretz wrote:
> DPDK provides two formats for specifying PCI device numbers:
> a full version ("0000:08:00.0") and a short version ("08:00.0").
> Issues can occur when an application uses one format (e.g., short)
> while running testpmd, then attempts to use the other format
> (e.g., full) in a later command, resulting in a failure.
> 
> The issue is that find_device goes over the list of devices and
> compares the user-provided string to the rte_device structure's
> device->name (device->name is just the string received from devargs
> (i.e "08:00.0" or "0000:08:00.0")).
> Notice that there's another field that represents the device name,
> but this one is in the rte_pci_bus struct. This name is actually the result
> of the PCI parse function ("0000:08:00.0").
> If we want to accurately compare these names, we'll need to bring both
> sides to the same representation by invoking the parse function on
> the user input.
> 
> To make the cmp_dev_name function applicable to all buses—not just PCI—
> the proposed solution is to utilize the parse function implemented by
> each bus. When comparing names, we will call parse on the supplied
> string as well as on the device name itself and compare the results.
> This will allow consistent comparisons between different representations
> of same devices.
> 
> Also, the pci_common_set function has been modified to improve naming
> consistency for PCI buses.
> Now, the name stored in rte_device for PCI buses will match the parsed
> name that is also stored in rte_pci_device name,
> rather than using the user-provided string from devargs.
> As a result, when a new PCI device is registered, the name displayed in
> the device list will be the parsed version.
> 
> Added tests that compare and find devices in various forms of names
> under test_devargs.
> 
> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> 
> Signed-off-by: Shani Peretz <shperetz@nvidia.com>
> ---

Thanks, I believe this fixes issues that I previously encountered.

However, there are a lot of changes in this one patch, so review would be
easier if it could be split a bit. Could you please look to split it up.
For example, one possible split might be:

* have one patch to adjust the bus parse function to add the extra
  parameter there. That should clear most/all of the non-pci driver changes.
* have a separate patch at the end for testing code.
* the middle patch can therefore be focused on rest of the problem
  statement and especially on the PCI bus and EAL changes.

Would such a split work?

Thanks,
/Bruce

>  app/test/test_devargs.c                  | 123 ++++++++++++++++++++++-
>  app/test/test_vdev.c                     |  10 +-
>  drivers/bus/auxiliary/auxiliary_common.c |  17 +++-
>  drivers/bus/cdx/cdx.c                    |  13 ++-
>  drivers/bus/dpaa/dpaa_bus.c              |   7 +-
>  drivers/bus/fslmc/fslmc_bus.c            |   9 +-
>  drivers/bus/ifpga/ifpga_bus.c            |  14 ++-
>  drivers/bus/pci/pci_common.c             |  22 ++--
>  drivers/bus/platform/platform.c          |  15 ++-
>  drivers/bus/uacce/uacce.c                |  14 ++-
>  drivers/bus/vdev/vdev.c                  |  23 ++++-
>  drivers/bus/vmbus/vmbus_common.c         |   9 +-
>  drivers/dma/idxd/idxd_bus.c              |   9 +-
>  drivers/raw/ifpga/ifpga_rawdev.c         |   8 +-
>  lib/eal/common/eal_common_bus.c          |   2 +-
>  lib/eal/common/eal_common_dev.c          |  41 +++++++-
>  lib/eal/common/hotplug_mp.c              |  11 +-
>  lib/eal/include/bus_driver.h             |  24 ++++-
>  lib/eal/include/rte_dev.h                |  15 +++
>  lib/eal/linux/eal_dev.c                  |  10 +-
>  lib/eal/version.map                      |   1 +
>  21 files changed, 305 insertions(+), 92 deletions(-)
> 

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Wed, 29 Jan 2025 10:54:16 +0200
Shani Peretz <shperetz@nvidia.com> wrote:

> +create_pci_dev(const char *name)
> +{
> +	int port_id;
> +	uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> +	struct rte_ether_addr *mac_addr = (struct rte_ether_addr *)slave_mac1;

Use different initializer and you can avoid the need for cast here.


>  
> +/**
> + * General device name comparison. Will compare by using the specific bus
> + * compare function or by comparing the names directly.
> + *
> + * @param dev
> + *   Device handle.
> + * @param name
> + *   Name to compare against.
> + * @return
> + *   0 if the device matches the name. Nonzero otherwise.
> + */
> +__rte_internal
> +int rte_cmp_dev_name(const struct rte_device *dev, const void *name);

It would make more sense to me if name was a character not void pointer.

The design might be clearer if bus address was more of an typedef with
a pointer and size together. Treat it more like an object.

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Wed, 29 Jan 2025 10:54:16 +0200
Shani Peretz <shperetz@nvidia.com> wrote:

> DPDK provides two formats for specifying PCI device numbers:
> a full version ("0000:08:00.0") and a short version ("08:00.0").
> Issues can occur when an application uses one format (e.g., short)
> while running testpmd, then attempts to use the other format
> (e.g., full) in a later command, resulting in a failure.
> 
> The issue is that find_device goes over the list of devices and
> compares the user-provided string to the rte_device structure's
> device->name (device->name is just the string received from devargs
> (i.e "08:00.0" or "0000:08:00.0")).
> Notice that there's another field that represents the device name,
> but this one is in the rte_pci_bus struct. This name is actually the result
> of the PCI parse function ("0000:08:00.0").
> If we want to accurately compare these names, we'll need to bring both
> sides to the same representation by invoking the parse function on
> the user input.
> 
> To make the cmp_dev_name function applicable to all buses—not just PCI—
> the proposed solution is to utilize the parse function implemented by
> each bus. When comparing names, we will call parse on the supplied
> string as well as on the device name itself and compare the results.
> This will allow consistent comparisons between different representations
> of same devices.
> 
> Also, the pci_common_set function has been modified to improve naming
> consistency for PCI buses.
> Now, the name stored in rte_device for PCI buses will match the parsed
> name that is also stored in rte_pci_device name,
> rather than using the user-provided string from devargs.
> As a result, when a new PCI device is registered, the name displayed in
> the device list will be the parsed version.
> 
> Added tests that compare and find devices in various forms of names
> under test_devargs.
> 
> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")

Maybe just fix for now by normalizing the PCI device string when before
storing and after parsing?  That would allow for simple fix that can be backported.
The more complex generalization of bus address is too much to go to stable branch.

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Bruce Richardson]

On Wed, Jan 29, 2025 at 09:17:38AM -0800, Stephen Hemminger wrote:
> On Wed, 29 Jan 2025 10:54:16 +0200
> Shani Peretz <shperetz@nvidia.com> wrote:
> 
> > DPDK provides two formats for specifying PCI device numbers:
> > a full version ("0000:08:00.0") and a short version ("08:00.0").
> > Issues can occur when an application uses one format (e.g., short)
> > while running testpmd, then attempts to use the other format
> > (e.g., full) in a later command, resulting in a failure.
> > 
> > The issue is that find_device goes over the list of devices and
> > compares the user-provided string to the rte_device structure's
> > device->name (device->name is just the string received from devargs
> > (i.e "08:00.0" or "0000:08:00.0")).
> > Notice that there's another field that represents the device name,
> > but this one is in the rte_pci_bus struct. This name is actually the result
> > of the PCI parse function ("0000:08:00.0").
> > If we want to accurately compare these names, we'll need to bring both
> > sides to the same representation by invoking the parse function on
> > the user input.
> > 
> > To make the cmp_dev_name function applicable to all buses—not just PCI—
> > the proposed solution is to utilize the parse function implemented by
> > each bus. When comparing names, we will call parse on the supplied
> > string as well as on the device name itself and compare the results.
> > This will allow consistent comparisons between different representations
> > of same devices.
> > 
> > Also, the pci_common_set function has been modified to improve naming
> > consistency for PCI buses.
> > Now, the name stored in rte_device for PCI buses will match the parsed
> > name that is also stored in rte_pci_device name,
> > rather than using the user-provided string from devargs.
> > As a result, when a new PCI device is registered, the name displayed in
> > the device list will be the parsed version.
> > 
> > Added tests that compare and find devices in various forms of names
> > under test_devargs.
> > 
> > Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> 
> Maybe just fix for now by normalizing the PCI device string when before
> storing and after parsing?  That would allow for simple fix that can be backported.
> The more complex generalization of bus address is too much to go to stable branch.

One idea for backport, perhaps?

https://patches.dpdk.org/project/dpdk/patch/20241119155723.2307189-1-bruce.richardson@intel.com/

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[fengchengwen]

On 2025/1/30 2:06, Bruce Richardson wrote:
> On Wed, Jan 29, 2025 at 09:17:38AM -0800, Stephen Hemminger wrote:
>> On Wed, 29 Jan 2025 10:54:16 +0200
>> Shani Peretz <shperetz@nvidia.com> wrote:
>>
>>> DPDK provides two formats for specifying PCI device numbers:
>>> a full version ("0000:08:00.0") and a short version ("08:00.0").
>>> Issues can occur when an application uses one format (e.g., short)
>>> while running testpmd, then attempts to use the other format
>>> (e.g., full) in a later command, resulting in a failure.
>>>
>>> The issue is that find_device goes over the list of devices and
>>> compares the user-provided string to the rte_device structure's
>>> device->name (device->name is just the string received from devargs
>>> (i.e "08:00.0" or "0000:08:00.0")).
>>> Notice that there's another field that represents the device name,
>>> but this one is in the rte_pci_bus struct. This name is actually the result
>>> of the PCI parse function ("0000:08:00.0").
>>> If we want to accurately compare these names, we'll need to bring both
>>> sides to the same representation by invoking the parse function on
>>> the user input.
>>>
>>> To make the cmp_dev_name function applicable to all buses—not just PCI—
>>> the proposed solution is to utilize the parse function implemented by
>>> each bus. When comparing names, we will call parse on the supplied
>>> string as well as on the device name itself and compare the results.
>>> This will allow consistent comparisons between different representations
>>> of same devices.
>>>
>>> Also, the pci_common_set function has been modified to improve naming
>>> consistency for PCI buses.
>>> Now, the name stored in rte_device for PCI buses will match the parsed
>>> name that is also stored in rte_pci_device name,
>>> rather than using the user-provided string from devargs.
>>> As a result, when a new PCI device is registered, the name displayed in
>>> the device list will be the parsed version.
>>>
>>> Added tests that compare and find devices in various forms of names
>>> under test_devargs.
>>>
>>> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
>>
>> Maybe just fix for now by normalizing the PCI device string when before
>> storing and after parsing?  That would allow for simple fix that can be backported.
>> The more complex generalization of bus address is too much to go to stable branch.

+1

> 
> One idea for backport, perhaps?
> 
> https://patches.dpdk.org/project/dpdk/patch/20241119155723.2307189-1-bruce.richardson@intel.com/

ack

> 

RE: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Shani Peretz]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, 29 January 2025 18:25
> To: Shani Peretz <shperetz@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> Richardson <bruce.richardson@intel.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 29 Jan 2025 10:54:16 +0200
> Shani Peretz <shperetz@nvidia.com> wrote:
> 
> > +create_pci_dev(const char *name)
> > +{
> > +     int port_id;
> > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > +*)slave_mac1;
> 
> Use different initializer and you can avoid the need for cast here.
> 
> 
> >
> > +/**
> > + * General device name comparison. Will compare by using the specific
> > +bus
> > + * compare function or by comparing the names directly.
> > + *
> > + * @param dev
> > + *   Device handle.
> > + * @param name
> > + *   Name to compare against.
> > + * @return
> > + *   0 if the device matches the name. Nonzero otherwise.
> > + */
> > +__rte_internal
> > +int rte_cmp_dev_name(const struct rte_device *dev, const void *name);
> 
> It would make more sense to me if name was a character not void pointer.
> 
> The design might be clearer if bus address was more of an typedef with a
> pointer and size together. Treat it more like an object.


Okay so just to understand,
this is the struct I am adding:

	struct rte_bus_address {
	       const void *addr;
	       size_t size;
	};

This is how I pass it to find_device:
	
	struct rte_bus_address dev_addr = {
	          .addr = da->name,
	          .size = RTE_DEV_NAME_MAX_LEN
	};
	dev = da->bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);

And this is how I use it in rte_cmp_dev_name:

	rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
	 {
	       const struct rte_bus_address *dev2_addr = addr;
	…
	}

Is that what you meant? 
Also, I'm not sure if the size is really needed, because we check the size of the parsed name, which may be different than the size of the original name

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Wed, 5 Feb 2025 16:36:11 +0000
Shani Peretz <shperetz@nvidia.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, 29 January 2025 18:25
> > To: Shani Peretz <shperetz@nvidia.com>
> > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> > Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> > <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> > Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> > <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> > Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, 29 Jan 2025 10:54:16 +0200
> > Shani Peretz <shperetz@nvidia.com> wrote:
> >   
> > > +create_pci_dev(const char *name)
> > > +{
> > > +     int port_id;
> > > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > > +*)slave_mac1;  
> > 
> > Use different initializer and you can avoid the need for cast here.
> > 
> >   
> > >
> > > +/**
> > > + * General device name comparison. Will compare by using the specific
> > > +bus
> > > + * compare function or by comparing the names directly.
> > > + *
> > > + * @param dev
> > > + *   Device handle.
> > > + * @param name
> > > + *   Name to compare against.
> > > + * @return
> > > + *   0 if the device matches the name. Nonzero otherwise.
> > > + */
> > > +__rte_internal
> > > +int rte_cmp_dev_name(const struct rte_device *dev, const void *name);  
> > 
> > It would make more sense to me if name was a character not void pointer.
> > 
> > The design might be clearer if bus address was more of an typedef with a
> > pointer and size together. Treat it more like an object.  
> 
> 
> Okay so just to understand,
> this is the struct I am adding:
> 
> 	struct rte_bus_address {
> 	       const void *addr;
> 	       size_t size;
> 	};
> 
> This is how I pass it to find_device:
> 	
> 	struct rte_bus_address dev_addr = {
> 	          .addr = da->name,
> 	          .size = RTE_DEV_NAME_MAX_LEN
> 	};
> 	dev = da->bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
> 
> And this is how I use it in rte_cmp_dev_name:
> 
> 	rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
> 	 {
> 	       const struct rte_bus_address *dev2_addr = addr;
> 	…
> 	}
> 
> Is that what you meant? 
> Also, I'm not sure if the size is really needed, because we check the size of the parsed name, which may be different than the size of the original name
> 

It would be best to pass rte_bus_address to rte_cmp_dev_name rather than having implied cast.
Not sure if that is possible without breaking API though.

RE: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Shani Peretz]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, 5 February 2025 18:42
> To: Shani Peretz <shperetz@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> Richardson <bruce.richardson@intel.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 5 Feb 2025 16:36:11 +0000
> Shani Peretz <shperetz@nvidia.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Wednesday, 29 January 2025 18:25
> > > To: Shani Peretz <shperetz@nvidia.com>
> > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > <thomas@monjalon.net>; Tyler Retzlaff
> > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> <nipun.gupta@amd.com>;
> > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI
> > > numbers
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Wed, 29 Jan 2025 10:54:16 +0200
> > > Shani Peretz <shperetz@nvidia.com> wrote:
> > >
> > > > +create_pci_dev(const char *name)
> > > > +{
> > > > +     int port_id;
> > > > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > > > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > > > +*)slave_mac1;
> > >
> > > Use different initializer and you can avoid the need for cast here.
> > >
> > >
> > > >
> > > > +/**
> > > > + * General device name comparison. Will compare by using the
> > > > +specific bus
> > > > + * compare function or by comparing the names directly.
> > > > + *
> > > > + * @param dev
> > > > + *   Device handle.
> > > > + * @param name
> > > > + *   Name to compare against.
> > > > + * @return
> > > > + *   0 if the device matches the name. Nonzero otherwise.
> > > > + */
> > > > +__rte_internal
> > > > +int rte_cmp_dev_name(const struct rte_device *dev, const void
> > > > +*name);
> > >
> > > It would make more sense to me if name was a character not void pointer.
> > >
> > > The design might be clearer if bus address was more of an typedef
> > > with a pointer and size together. Treat it more like an object.
> >
> >
> > Okay so just to understand,
> > this is the struct I am adding:
> >
> >       struct rte_bus_address {
> >              const void *addr;
> >              size_t size;
> >       };
> >
> > This is how I pass it to find_device:
> >
> >       struct rte_bus_address dev_addr = {
> >                 .addr = da->name,
> >                 .size = RTE_DEV_NAME_MAX_LEN
> >       };
> >       dev = da->bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
> >
> > And this is how I use it in rte_cmp_dev_name:
> >
> >       rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
> >        {
> >              const struct rte_bus_address *dev2_addr = addr;
> >       …
> >       }
> >
> > Is that what you meant?
> > Also, I'm not sure if the size is really needed, because we check the
> > size of the parsed name, which may be different than the size of the
> > original name
> >
> 
> It would be best to pass rte_bus_address to rte_cmp_dev_name rather than
> having implied cast.
> Not sure if that is possible without breaking API though.

I think you are right, also it will require to change the signature of find_device which will make it even bigger change

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Wed, 5 Feb 2025 17:37:54 +0000
Shani Peretz <shperetz@nvidia.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, 5 February 2025 18:42
> > To: Shani Peretz <shperetz@nvidia.com>
> > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> > Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> > <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> > Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> > <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> > Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, 5 Feb 2025 16:36:11 +0000
> > Shani Peretz <shperetz@nvidia.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > Sent: Wednesday, 29 January 2025 18:25
> > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > > <thomas@monjalon.net>; Tyler Retzlaff
> > > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta  
> > <nipun.gupta@amd.com>;  
> > > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > > > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI
> > > > numbers
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Wed, 29 Jan 2025 10:54:16 +0200
> > > > Shani Peretz <shperetz@nvidia.com> wrote:
> > > >  
> > > > > +create_pci_dev(const char *name)
> > > > > +{
> > > > > +     int port_id;
> > > > > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > > > > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > > > > +*)slave_mac1;  
> > > >
> > > > Use different initializer and you can avoid the need for cast here.
> > > >
> > > >  
> > > > >
> > > > > +/**
> > > > > + * General device name comparison. Will compare by using the
> > > > > +specific bus
> > > > > + * compare function or by comparing the names directly.
> > > > > + *
> > > > > + * @param dev
> > > > > + *   Device handle.
> > > > > + * @param name
> > > > > + *   Name to compare against.
> > > > > + * @return
> > > > > + *   0 if the device matches the name. Nonzero otherwise.
> > > > > + */
> > > > > +__rte_internal
> > > > > +int rte_cmp_dev_name(const struct rte_device *dev, const void
> > > > > +*name);  
> > > >
> > > > It would make more sense to me if name was a character not void pointer.
> > > >
> > > > The design might be clearer if bus address was more of an typedef
> > > > with a pointer and size together. Treat it more like an object.  
> > >
> > >
> > > Okay so just to understand,
> > > this is the struct I am adding:
> > >
> > >       struct rte_bus_address {
> > >              const void *addr;
> > >              size_t size;
> > >       };
> > >
> > > This is how I pass it to find_device:
> > >
> > >       struct rte_bus_address dev_addr = {
> > >                 .addr = da->name,
> > >                 .size = RTE_DEV_NAME_MAX_LEN
> > >       };
> > >       dev = da->bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
> > >
> > > And this is how I use it in rte_cmp_dev_name:
> > >
> > >       rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
> > >        {
> > >              const struct rte_bus_address *dev2_addr = addr;
> > >       …
> > >       }
> > >
> > > Is that what you meant?
> > > Also, I'm not sure if the size is really needed, because we check the
> > > size of the parsed name, which may be different than the size of the
> > > original name
> > >  
> > 
> > It would be best to pass rte_bus_address to rte_cmp_dev_name rather than
> > having implied cast.
> > Not sure if that is possible without breaking API though.  
> 
> I think you are right, also it will require to change the signature of find_device which will make it even bigger change

Could you post patch with something "good enough" that builds and passes tests?

RE: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Shani Peretz]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, 5 February 2025 20:46
> To: Shani Peretz <shperetz@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> Richardson <bruce.richardson@intel.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 5 Feb 2025 17:37:54 +0000
> Shani Peretz <shperetz@nvidia.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Wednesday, 5 February 2025 18:42
> > > To: Shani Peretz <shperetz@nvidia.com>
> > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > <thomas@monjalon.net>; Tyler Retzlaff
> > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> <nipun.gupta@amd.com>;
> > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI
> > > numbers
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Wed, 5 Feb 2025 16:36:11 +0000
> > > Shani Peretz <shperetz@nvidia.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > > Sent: Wednesday, 29 January 2025 18:25
> > > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > > > <thomas@monjalon.net>; Tyler Retzlaff
> > > > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> > > <nipun.gupta@amd.com>;
> > > > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>;
> > > > > Bruce Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of
> > > > > PCI numbers
> > > > >
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > On Wed, 29 Jan 2025 10:54:16 +0200 Shani Peretz
> > > > > <shperetz@nvidia.com> wrote:
> > > > >
> > > > > > +create_pci_dev(const char *name) {
> > > > > > +     int port_id;
> > > > > > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > > > > > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > > > > > +*)slave_mac1;
> > > > >
> > > > > Use different initializer and you can avoid the need for cast here.
> > > > >
> > > > >
> > > > > >
> > > > > > +/**
> > > > > > + * General device name comparison. Will compare by using the
> > > > > > +specific bus
> > > > > > + * compare function or by comparing the names directly.
> > > > > > + *
> > > > > > + * @param dev
> > > > > > + *   Device handle.
> > > > > > + * @param name
> > > > > > + *   Name to compare against.
> > > > > > + * @return
> > > > > > + *   0 if the device matches the name. Nonzero otherwise.
> > > > > > + */
> > > > > > +__rte_internal
> > > > > > +int rte_cmp_dev_name(const struct rte_device *dev, const void
> > > > > > +*name);
> > > > >
> > > > > It would make more sense to me if name was a character not void
> pointer.
> > > > >
> > > > > The design might be clearer if bus address was more of an
> > > > > typedef with a pointer and size together. Treat it more like an object.
> > > >
> > > >
> > > > Okay so just to understand,
> > > > this is the struct I am adding:
> > > >
> > > >       struct rte_bus_address {
> > > >              const void *addr;
> > > >              size_t size;
> > > >       };
> > > >
> > > > This is how I pass it to find_device:
> > > >
> > > >       struct rte_bus_address dev_addr = {
> > > >                 .addr = da->name,
> > > >                 .size = RTE_DEV_NAME_MAX_LEN
> > > >       };
> > > >       dev = da->bus->find_device(NULL, rte_cmp_dev_name,
> > > > &dev_addr);
> > > >
> > > > And this is how I use it in rte_cmp_dev_name:
> > > >
> > > >       rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
> > > >        {
> > > >              const struct rte_bus_address *dev2_addr = addr;
> > > >       …
> > > >       }
> > > >
> > > > Is that what you meant?
> > > > Also, I'm not sure if the size is really needed, because we check
> > > > the size of the parsed name, which may be different than the size
> > > > of the original name
> > > >
> > >
> > > It would be best to pass rte_bus_address to rte_cmp_dev_name rather
> > > than having implied cast.
> > > Not sure if that is possible without breaking API though.
> >
> > I think you are right, also it will require to change the signature of
> > find_device which will make it even bigger change
> 
> Could you post patch with something "good enough" that builds and passes
> tests?

You mean with the rte_bus_address change or without?
Without it it was tested and passed. With it I only ran few tests locally and they passed

[PATCH v5 0/4] fix comparison between devices

[Shani Peretz]

This series fixes two issues:
1. saving PCI name as the parsed one instead of user-provided string when registering a new device 
2. fixes rte_cmp_dev_name function that currently compares the names by compering 2 strings

Also added test to check the functionality of rte_cmp_dev_name.


Shani Peretz (4):
  bus/pci: fix registration of PCI device
  lib: fix comparison between devices
  app/test: add tests to find devices
  lib: change find device and cmp dev name functions

 app/test/test_devargs.c                  | 140 ++++++++++++++++++++++-
 app/test/test_vdev.c                     |  29 +++--
 drivers/bus/auxiliary/auxiliary_common.c |  19 ++-
 drivers/bus/auxiliary/auxiliary_params.c |  10 +-
 drivers/bus/cdx/cdx.c                    |  25 ++--
 drivers/bus/dpaa/dpaa_bus.c              |   9 +-
 drivers/bus/fslmc/fslmc_bus.c            |  11 +-
 drivers/bus/ifpga/ifpga_bus.c            |  16 ++-
 drivers/bus/pci/pci_common.c             |  23 ++--
 drivers/bus/pci/pci_params.c             |  10 +-
 drivers/bus/platform/platform.c          |  18 ++-
 drivers/bus/platform/platform_params.c   |  10 +-
 drivers/bus/uacce/uacce.c                |  27 +++--
 drivers/bus/vdev/vdev.c                  |  25 +++-
 drivers/bus/vdev/vdev_params.c           |  10 +-
 drivers/bus/vdev/vdev_private.h          |   2 +-
 drivers/bus/vmbus/vmbus_common.c         |  11 +-
 drivers/dma/idxd/idxd_bus.c              |  13 ++-
 drivers/net/bonding/rte_eth_bond_args.c  |  15 +--
 drivers/net/vdev_netvsc/vdev_netvsc.c    |   8 +-
 drivers/raw/ifpga/ifpga_rawdev.c         |  12 +-
 lib/eal/common/eal_common_bus.c          |  12 +-
 lib/eal/common/eal_common_dev.c          |  49 +++++++-
 lib/eal/common/hotplug_mp.c              |  19 +--
 lib/eal/include/bus_driver.h             |  26 ++++-
 lib/eal/include/rte_dev.h                |  22 +++-
 lib/eal/linux/eal_dev.c                  |  16 +--
 lib/eal/version.map                      |   1 +
 28 files changed, 444 insertions(+), 144 deletions(-)

-- 
2.34.1

[PATCH v5 1/4] bus/pci: fix registration of PCI device

[Shani Peretz]

When registering a new PCI device, the device->name field stored
the user-provided string from devargs (e.g., "08:00.0" or "0000:08:00.0").
This approach led to inconsistencies when registering new devices.

This patch fix this issue by saving the parsed PCI in device->name,
so when a new PCI device is registering the name displayed in the device
list will be the parsed version.

Fixes: 23eaa9059ec2 ("bus/pci: use given name as generic name")

Cc: stable@dpdk.org
Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 drivers/bus/pci/pci_common.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 1173f0887c..70faae4e44 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -99,21 +99,11 @@ pci_common_set(struct rte_pci_device *dev)
 	/* Each device has its internal, canonical name set. */
 	rte_pci_device_name(&dev->addr,
 			dev->name, sizeof(dev->name));
+	dev->device.name = dev->name;
+
 	devargs = pci_devargs_lookup(&dev->addr);
 	dev->device.devargs = devargs;
 
-	/* When using a blocklist, only blocked devices will have
-	 * an rte_devargs. Allowed devices won't have one.
-	 */
-	if (devargs != NULL)
-		/* If an rte_devargs exists, the generic rte_device uses the
-		 * given name as its name.
-		 */
-		dev->device.name = dev->device.devargs->name;
-	else
-		/* Otherwise, it uses the internal, canonical form. */
-		dev->device.name = dev->name;
-
 	if (dev->bus_info != NULL ||
 			asprintf(&dev->bus_info, "vendor_id=%"PRIx16", device_id=%"PRIx16,
 				dev->id.vendor_id, dev->id.device_id) != -1)
-- 
2.34.1

[PATCH v5 2/4] lib: fix comparison between devices

[Shani Peretz]

DPDK supports multiple formats for specifying buses,
(such as "0000:08:00.0" and "08:00.0" for PCI).
This flexibility can lead to inconsistencies when using one
format while running testpmd, then attempts to use the other
format in a later command, resulting in a failure.

The issue arises from the find_device function, which compares
the user-provided string directly with the device->name in
the rte_device structure.
If we want to accurately compare these names, we'll need to bring both
sides to the same representation by invoking the parse function
on the user input.

The proposed solution is to utilize the parse function implemented
by each bus. When comparing names, we will call parse on the supplied
string as well as on the device name itself and compare the results.
As part of the change the parse function will now return the size of the
parsed address.

This will allow consistent comparisons between different representations
of same devices.

In addition, fixed vdev test to use the rte_cmp_dev_name function
instead of the custom one.

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

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 app/test/test_vdev.c                     | 10 ++----
 drivers/bus/auxiliary/auxiliary_common.c | 17 +++++++---
 drivers/bus/cdx/cdx.c                    | 13 +++++---
 drivers/bus/dpaa/dpaa_bus.c              |  7 ++--
 drivers/bus/fslmc/fslmc_bus.c            |  9 ++++--
 drivers/bus/ifpga/ifpga_bus.c            | 14 +++++---
 drivers/bus/pci/pci_common.c             |  7 ++--
 drivers/bus/platform/platform.c          | 15 ++++++---
 drivers/bus/uacce/uacce.c                | 14 +++++---
 drivers/bus/vdev/vdev.c                  | 23 +++++++++++--
 drivers/bus/vmbus/vmbus_common.c         |  9 ++++--
 drivers/dma/idxd/idxd_bus.c              |  9 ++++--
 drivers/raw/ifpga/ifpga_rawdev.c         |  8 +----
 lib/eal/common/eal_common_bus.c          |  2 +-
 lib/eal/common/eal_common_dev.c          | 41 +++++++++++++++++++++---
 lib/eal/common/hotplug_mp.c              | 11 ++-----
 lib/eal/include/bus_driver.h             | 24 +++++++++++++-
 lib/eal/include/rte_dev.h                | 15 +++++++++
 lib/eal/linux/eal_dev.c                  | 10 +-----
 lib/eal/version.map                      |  1 +
 20 files changed, 180 insertions(+), 79 deletions(-)

diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
index 3e262f30bc..860fa260af 100644
--- a/app/test/test_vdev.c
+++ b/app/test/test_vdev.c
@@ -20,12 +20,6 @@ static const char * const valid_keys[] = {
 	NULL,
 };
 
-static int
-cmp_dev_name(const struct rte_device *dev, const void *name)
-{
-	return strcmp(rte_dev_name(dev), name);
-}
-
 static int
 cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
 {
@@ -82,7 +76,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test0\n");
 		goto fail;
 	}
-	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
+	dev0 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test0");
 	if (dev0 == NULL) {
 		printf("Cannot find net_null_test0 vdev\n");
 		goto fail;
@@ -93,7 +87,7 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test1\n");
 		goto fail;
 	}
-	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
+	dev1 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test1");
 	if (dev1 == NULL) {
 		printf("Cannot find net_null_test1 vdev\n");
 		goto fail;
diff --git a/drivers/bus/auxiliary/auxiliary_common.c b/drivers/bus/auxiliary/auxiliary_common.c
index e6cbc4d356..ba2f69e851 100644
--- a/drivers/bus/auxiliary/auxiliary_common.c
+++ b/drivers/bus/auxiliary/auxiliary_common.c
@@ -237,10 +237,9 @@ auxiliary_probe(void)
 }
 
 static int
-auxiliary_parse(const char *name, void *addr)
+auxiliary_parse(const char *name, void *addr, int *size)
 {
 	struct rte_auxiliary_driver *drv = NULL;
-	const char **out = addr;
 
 	/* Allow empty device name "auxiliary:" to bypass entire bus scan. */
 	if (strlen(name) == 0)
@@ -250,9 +249,17 @@ auxiliary_parse(const char *name, void *addr)
 		if (drv->match(name))
 			break;
 	}
-	if (drv != NULL && addr != NULL)
-		*out = name;
-	return drv != NULL ? 0 : -1;
+
+	if (drv == NULL)
+		return -1;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
+
+	return 0;
 }
 
 /* Register a driver */
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index 62b108e082..b97f1ce1af 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -464,15 +464,20 @@ cdx_probe(void)
 }
 
 static int
-cdx_parse(const char *name, void *addr)
+cdx_parse(const char *name, void *addr, int *size)
 {
-	const char **out = addr;
 	int ret;
 
 	ret = strncmp(name, CDX_DEV_PREFIX, strlen(CDX_DEV_PREFIX));
 
-	if (ret == 0 && addr)
-		*out = name;
+	if (ret != 0)
+		return ret;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
 
 	return ret;
 }
diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
index 9ffbe07c93..f768fb9454 100644
--- a/drivers/bus/dpaa/dpaa_bus.c
+++ b/drivers/bus/dpaa/dpaa_bus.c
@@ -153,7 +153,7 @@ dpaa_devargs_lookup(struct rte_dpaa_device *dev)
 	char dev_name[32];
 
 	RTE_EAL_DEVARGS_FOREACH("dpaa_bus", devargs) {
-		devargs->bus->parse(devargs->name, &dev_name);
+		devargs->bus->parse(devargs->name, &dev_name, NULL);
 		if (strcmp(dev_name, dev->device.name) == 0) {
 			DPAA_BUS_INFO("**Devargs matched %s", dev_name);
 			return devargs;
@@ -447,7 +447,7 @@ dpaa_portal_finish(void *arg)
 }
 
 static int
-rte_dpaa_bus_parse(const char *name, void *out)
+rte_dpaa_bus_parse(const char *name, void *out, int *size)
 {
 	unsigned int i, j;
 	size_t delta, dev_delta;
@@ -494,6 +494,9 @@ rte_dpaa_bus_parse(const char *name, void *out)
 		max_name_len = sizeof("fm.-mac..") - 1;
 	}
 
+	if (size != NULL)
+		*size = max_name_len + 1;
+
 	if (out != NULL) {
 		char *out_name = out;
 
diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 68ad2b801e..a525b5e245 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -103,7 +103,7 @@ fslmc_devargs_lookup(struct rte_dpaa2_device *dev)
 	char dev_name[32];
 
 	RTE_EAL_DEVARGS_FOREACH("fslmc", devargs) {
-		devargs->bus->parse(devargs->name, &dev_name);
+		devargs->bus->parse(devargs->name, &dev_name, NULL);
 		if (strcmp(dev_name, dev->device.name) == 0) {
 			DPAA2_BUS_INFO("**Devargs matched %s", dev_name);
 			return devargs;
@@ -235,7 +235,7 @@ scan_one_fslmc_device(char *dev_name)
 }
 
 static int
-rte_fslmc_parse(const char *name, void *addr)
+rte_fslmc_parse(const char *name, void *addr, int *size)
 {
 	uint16_t dev_id;
 	char *t_ptr;
@@ -298,7 +298,10 @@ rte_fslmc_parse(const char *name, void *addr)
 		goto err_out;
 	}
 
-	if (addr)
+	if (size != NULL)
+		*size = strlen(sep) + 1;
+
+	if (addr != NULL)
 		strcpy(addr, sep);
 
 	ret = 0;
diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c
index 11b31eee4f..d5d9a86a97 100644
--- a/drivers/bus/ifpga/ifpga_bus.c
+++ b/drivers/bus/ifpga/ifpga_bus.c
@@ -459,9 +459,8 @@ ifpga_find_device(const struct rte_device *start,
 	return NULL;
 }
 static int
-ifpga_parse(const char *name, void *addr)
+ifpga_parse(const char *name, void *addr, int *size)
 {
-	int *out = addr;
 	struct rte_rawdev *rawdev = NULL;
 	char rawdev_name[RTE_RAWDEV_NAME_MAX_LEN];
 	char *c1 = NULL;
@@ -491,9 +490,14 @@ ifpga_parse(const char *name, void *addr)
 	rawdev = rte_rawdev_pmd_get_named_dev(rawdev_name);
 
 	if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
-		rawdev &&
-		(addr != NULL))
-		*out = port;
+		rawdev) {
+		if (size != NULL)
+			*size = sizeof(port);
+
+		if (addr != NULL)
+			*(int *)addr = port;
+		}
+
 
 	if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
 		rawdev)
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 70faae4e44..82632bc473 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -84,7 +84,7 @@ pci_devargs_lookup(const struct rte_pci_addr *pci_addr)
 	struct rte_pci_addr addr;
 
 	RTE_EAL_DEVARGS_FOREACH("pci", devargs) {
-		devargs->bus->parse(devargs->name, &addr);
+		devargs->bus->parse(devargs->name, &addr, NULL);
 		if (!rte_pci_addr_cmp(pci_addr, &addr))
 			return devargs;
 	}
@@ -487,12 +487,15 @@ rte_pci_dump(FILE *f)
 }
 
 static int
-pci_parse(const char *name, void *addr)
+pci_parse(const char *name, void *addr, int *size)
 {
 	struct rte_pci_addr *out = addr;
 	struct rte_pci_addr pci_addr;
 	bool parse;
 
+	if (size != NULL)
+		*size = sizeof(struct rte_pci_addr);
+
 	parse = (rte_pci_addr_parse(name, &pci_addr) == 0);
 	if (parse && addr != NULL)
 		*out = pci_addr;
diff --git a/drivers/bus/platform/platform.c b/drivers/bus/platform/platform.c
index 11892caa24..87ed7404e1 100644
--- a/drivers/bus/platform/platform.c
+++ b/drivers/bus/platform/platform.c
@@ -542,11 +542,10 @@ platform_bus_unplug(struct rte_device *dev)
 }
 
 static int
-platform_bus_parse(const char *name, void *addr)
+platform_bus_parse(const char *name, void *addr, int *size)
 {
 	struct rte_platform_device pdev = { };
 	struct rte_platform_driver *pdrv;
-	const char **out = addr;
 
 	rte_strscpy(pdev.name, name, sizeof(pdev.name));
 
@@ -555,10 +554,16 @@ platform_bus_parse(const char *name, void *addr)
 			break;
 	}
 
-	if (pdrv != NULL && addr != NULL)
-		*out = name;
+	if (pdrv == NULL)
+		return -ENODEV;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
 
-	return pdrv != NULL ? 0 : -ENODEV;
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
+
+	return 0;
 }
 
 static int
diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
index 9ca048122d..b18fbe1103 100644
--- a/drivers/bus/uacce/uacce.c
+++ b/drivers/bus/uacce/uacce.c
@@ -20,6 +20,7 @@
 #include <rte_errno.h>
 #include <rte_log.h>
 #include <rte_kvargs.h>
+#include <rte_string_fns.h>
 #include <bus_driver.h>
 
 #include "bus_uacce_driver.h"
@@ -529,15 +530,20 @@ uacce_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,  const void
 }
 
 static int
-uacce_parse(const char *name, void *addr)
+uacce_parse(const char *name, void *addr, int *size)
 {
-	const char **out = addr;
 	int ret;
 
 	ret = strncmp(name, UACCE_DEV_PREFIX, strlen(UACCE_DEV_PREFIX));
 
-	if (ret == 0 && addr)
-		*out = name;
+	if (ret != 0)
+		return ret;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
 
 	return ret;
 }
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index ec7abe7cda..0d3c8ca3c5 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -113,7 +113,7 @@ rte_vdev_remove_custom_scan(rte_vdev_scan_callback callback, void *user_arg)
 }
 
 static int
-vdev_parse(const char *name, void *addr)
+vdev_find_driver(const char *name, void *addr)
 {
 	struct rte_vdev_driver **out = addr;
 	struct rte_vdev_driver *driver = NULL;
@@ -197,7 +197,7 @@ vdev_probe_all_drivers(struct rte_vdev_device *dev)
 	name = rte_vdev_device_name(dev);
 	VDEV_LOG(DEBUG, "Search driver to probe device %s", name);
 
-	if (vdev_parse(name, &driver))
+	if (vdev_find_driver(name, &driver))
 		return -1;
 
 	iova_mode = rte_eal_iova_mode();
@@ -232,6 +232,23 @@ find_vdev(const char *name)
 	return NULL;
 }
 
+static int
+vdev_parse(const char *name, void *addr, int *size)
+{
+	struct rte_vdev_driver *driver;
+
+	if (vdev_find_driver(name, &driver))
+		return 1;
+
+	if (size != NULL)
+		*size = strlen(name) + 1;
+
+	if (addr != NULL)
+		rte_strscpy(addr, name, strlen(name) + 1);
+
+	return 0;
+}
+
 static struct rte_devargs *
 alloc_devargs(const char *name, const char *args)
 {
@@ -647,7 +664,7 @@ vdev_get_iommu_class(void)
 
 	TAILQ_FOREACH(dev, &vdev_device_list, next) {
 		name = rte_vdev_device_name(dev);
-		if (vdev_parse(name, &driver))
+		if (vdev_find_driver(name, &driver))
 			continue;
 
 		if (driver->drv_flags & RTE_VDEV_DRV_NEED_IOVA_AS_VA)
diff --git a/drivers/bus/vmbus/vmbus_common.c b/drivers/bus/vmbus/vmbus_common.c
index 8a965d10d9..18805aee0d 100644
--- a/drivers/bus/vmbus/vmbus_common.c
+++ b/drivers/bus/vmbus/vmbus_common.c
@@ -245,13 +245,16 @@ rte_vmbus_cleanup(void)
 }
 
 static int
-vmbus_parse(const char *name, void *addr)
+vmbus_parse(const char *name, void *addr, int *size)
 {
 	rte_uuid_t guid;
 	int ret;
 
+	if (size != NULL)
+		*size = sizeof(guid);
+
 	ret = rte_uuid_parse(name, guid);
-	if (ret == 0 && addr)
+	if (ret == 0 && addr != NULL)
 		memcpy(addr, &guid, sizeof(guid));
 
 	return ret;
@@ -269,7 +272,7 @@ vmbus_devargs_lookup(struct rte_vmbus_device *dev)
 	rte_uuid_t addr;
 
 	RTE_EAL_DEVARGS_FOREACH("vmbus", devargs) {
-		vmbus_parse(devargs->name, &addr);
+		vmbus_parse(devargs->name, &addr, NULL);
 
 		if (rte_uuid_compare(dev->device_id, addr) == 0)
 			return devargs;
diff --git a/drivers/dma/idxd/idxd_bus.c b/drivers/dma/idxd/idxd_bus.c
index ba8076715d..d68e8b630e 100644
--- a/drivers/dma/idxd/idxd_bus.c
+++ b/drivers/dma/idxd/idxd_bus.c
@@ -47,7 +47,7 @@ static int dsa_probe(void);
 static struct rte_device *dsa_find_device(const struct rte_device *start,
 		rte_dev_cmp_t cmp,  const void *data);
 static enum rte_iova_mode dsa_get_iommu_class(void);
-static int dsa_addr_parse(const char *name, void *addr);
+static int dsa_addr_parse(const char *name, void *addr, int *size);
 
 /** List of devices */
 TAILQ_HEAD(dsa_device_list, rte_dsa_device);
@@ -345,7 +345,7 @@ dsa_scan(void)
 			closedir(dev_dir);
 			return -ENOMEM;
 		}
-		if (dsa_addr_parse(wq->d_name, &dev->addr) < 0) {
+		if (dsa_addr_parse(wq->d_name, &dev->addr, NULL) < 0) {
 			IDXD_PMD_ERR("Error parsing WQ name: %s", wq->d_name);
 			free(dev);
 			continue;
@@ -391,11 +391,14 @@ dsa_get_iommu_class(void)
 }
 
 static int
-dsa_addr_parse(const char *name, void *addr)
+dsa_addr_parse(const char *name, void *addr, int *size)
 {
 	struct dsa_wq_addr *wq = addr;
 	unsigned int device_id, wq_id;
 
+	if (size != NULL)
+		*size = sizeof(struct dsa_wq_addr);
+
 	if (sscanf(name, "wq%u.%u", &device_id, &wq_id) != 2) {
 		IDXD_PMD_DEBUG("Parsing WQ name failed: %s", name);
 		return -1;
diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index 5b9b596435..7238246e1c 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -1833,12 +1833,6 @@ ifpga_cfg_probe(struct rte_vdev_device *vdev)
 	return ret;
 }
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
-{
-	const char *name = _name;
-	return strcmp(dev->name, name);
-}
-
 static int
 ifpga_cfg_remove(struct rte_vdev_device *vdev)
 {
@@ -1874,7 +1868,7 @@ ifpga_cfg_remove(struct rte_vdev_device *vdev)
 		args.port, args.bdf);
 	bus = rte_bus_find_by_name(RTE_STR(IFPGA_BUS_NAME));
 	if (bus) {
-		if (bus->find_device(NULL, cmp_dev_name, dev_name)) {
+		if (bus->find_device(NULL, rte_cmp_dev_name, dev_name)) {
 			ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
 				dev_name);
 		}
diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
index 7cbd09c421..b1bb5df5b7 100644
--- a/lib/eal/common/eal_common_bus.c
+++ b/lib/eal/common/eal_common_bus.c
@@ -200,7 +200,7 @@ bus_can_parse(const struct rte_bus *bus, const void *_name)
 {
 	const char *name = _name;
 
-	return !(bus->parse && bus->parse(name, NULL) == 0);
+	return !(bus->parse && bus->parse(name, NULL, NULL) == 0);
 }
 
 struct rte_bus *
diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index 70aa04dcd9..c0e2202482 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -107,11 +107,42 @@ struct dev_next_ctx {
 #define CLSCTX(ptr) \
 	(((struct dev_next_ctx *)(intptr_t)ptr)->cls_str)
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
+int
+rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
 {
-	const char *name = _name;
+	void *parsed_name1;
+	void *parsed_name2;
+	int size1 = 0;
+	int size2 = 0;
+	int ret;
+
+	if (dev1->bus->parse(dev1->name, NULL, &size1) != 0 ||
+		dev1->bus->parse(name2, NULL, &size2) != 0)
+		return 1;
+
+	if (size1 != size2)
+		return 1;
+
+	parsed_name1 = malloc(size1);
+	if (parsed_name1 == NULL)
+		return 1;
+
+	parsed_name2 = malloc(size2);
+	if (parsed_name2 == NULL) {
+		free(parsed_name1);
+		return 1;
+	}
 
-	return strcmp(dev->name, name);
+	memset(parsed_name1, 0, size1);
+	memset(parsed_name2, 0, size2);
+
+	dev1->bus->parse(dev1->name, parsed_name1, NULL);
+	dev1->bus->parse(name2, parsed_name2, NULL);
+
+	ret = memcmp(parsed_name1, parsed_name2, size1);
+	free(parsed_name1);
+	free(parsed_name2);
+	return ret;
 }
 
 int
@@ -197,7 +228,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	if (ret)
 		goto err_devarg;
 
-	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
+	dev = da->bus->find_device(NULL, rte_cmp_dev_name, da->name);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find device (%s)",
 			da->name);
@@ -335,7 +366,7 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -ENOENT;
 	}
 
-	dev = bus->find_device(NULL, cmp_dev_name, devname);
+	dev = bus->find_device(NULL, rte_cmp_dev_name, devname);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find plugged device (%s)", devname);
 		return -EINVAL;
diff --git a/lib/eal/common/hotplug_mp.c b/lib/eal/common/hotplug_mp.c
index 17089ca3db..a2623c96c3 100644
--- a/lib/eal/common/hotplug_mp.c
+++ b/lib/eal/common/hotplug_mp.c
@@ -21,13 +21,6 @@ struct mp_reply_bundle {
 	void *peer;
 };
 
-static int cmp_dev_name(const struct rte_device *dev, const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 /**
  * Secondary to primary request.
  * start from function eal_dev_hotplug_request_to_primary.
@@ -135,7 +128,7 @@ __handle_secondary_request(void *param)
 			goto finish;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da.name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da.name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da.name);
 			ret = -ENOENT;
@@ -262,7 +255,7 @@ static void __handle_primary_request(void *param)
 			goto quit;
 		}
 
-		dev = bus->find_device(NULL, cmp_dev_name, da->name);
+		dev = bus->find_device(NULL, rte_cmp_dev_name, da->name);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da->name);
 			ret = -ENOENT;
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
index 60527b75b6..01447670de 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -112,11 +112,32 @@ typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
  *	should be written. If NULL, nothing should be written, which
  *	is not an error.
  *
+ * @param[out] size
+ *	device information size. If NULL, nothing should
+ *	be written, which is not an error.
+ *
  * @return
  *	0 if parsing was successful.
  *	!0 for any error.
  */
-typedef int (*rte_bus_parse_t)(const char *name, void *addr);
+typedef int (*rte_bus_parse_t)(const char *name, void *addr, int *size);
+
+/**
+ * Bus specific device name comparison function.
+ *
+ * This type of function is used to compare a bus name with an arbitrary
+ * name.
+ *
+ * @param dev
+ *	Device handle.
+ *
+ * @param name
+ *	Name to compare against.
+ *
+ * @return
+ *	0 if the device matches the name. Nonzero otherwise.
+ */
+typedef int (*rte_bus_cmp_name_t)(const struct rte_device *dev, const void *name);
 
 /**
  * Parse bus part of the device arguments.
@@ -258,6 +279,7 @@ struct rte_bus {
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */
 	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 	rte_bus_parse_t parse;       /**< Parse a device name */
+	rte_bus_cmp_name_t cmp_name; /**< Compare device name */
 	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
 	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
 	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
diff --git a/lib/eal/include/rte_dev.h b/lib/eal/include/rte_dev.h
index 738400e8d1..2df8143af1 100644
--- a/lib/eal/include/rte_dev.h
+++ b/lib/eal/include/rte_dev.h
@@ -17,6 +17,7 @@
 
 #include <rte_config.h>
 #include <rte_common.h>
+#include <rte_compat.h>
 #include <rte_log.h>
 
 #ifdef __cplusplus
@@ -170,6 +171,20 @@ int rte_dev_is_probed(const struct rte_device *dev);
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *drvargs);
 
+/**
+ * General device name comparison. Will compare by using the specific bus
+ * compare function or by comparing the names directly.
+ *
+ * @param dev
+ *   Device handle.
+ * @param name
+ *   Name to compare against.
+ * @return
+ *   0 if the device matches the name. Nonzero otherwise.
+ */
+__rte_internal
+int rte_cmp_dev_name(const struct rte_device *dev, const void *name);
+
 /**
  * Add matching devices.
  *
diff --git a/lib/eal/linux/eal_dev.c b/lib/eal/linux/eal_dev.c
index e63f24d108..3b68cda87f 100644
--- a/lib/eal/linux/eal_dev.c
+++ b/lib/eal/linux/eal_dev.c
@@ -91,14 +91,6 @@ static void sigbus_handler(int signum, siginfo_t *info,
 	EAL_LOG(DEBUG, "Success to handle SIGBUS for hot-unplug!");
 }
 
-static int cmp_dev_name(const struct rte_device *dev,
-	const void *_name)
-{
-	const char *name = _name;
-
-	return strcmp(dev->name, name);
-}
-
 static int
 dev_uev_socket_fd_create(void)
 {
@@ -280,7 +272,7 @@ dev_uev_handler(__rte_unused void *param)
 				goto failure_handle_err;
 			}
 
-			dev = bus->find_device(NULL, cmp_dev_name,
+			dev = bus->find_device(NULL, rte_cmp_dev_name,
 					       uevent.devname);
 			if (dev == NULL) {
 				EAL_LOG(ERR, "Cannot find device (%s) on "
diff --git a/lib/eal/version.map b/lib/eal/version.map
index a20c713eb1..e50a95fd31 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -448,4 +448,5 @@ INTERNAL {
 	rte_mem_unmap;
 	rte_thread_create_internal_control;
 	rte_thread_set_prefixed_name;
+	rte_cmp_dev_name;
 };
-- 
2.34.1

[PATCH v5 3/4] app/test: add tests to find devices

[Shani Peretz]

Added tests to PCI and vdev bus that compare and find devices
in various forms of names under test_devargs.
The tests check the functionality of the rte_cmp_dev_name function by
calling it with different variations of names, like short and full,
full and full, etc.

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 app/test/test_devargs.c | 125 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 124 insertions(+), 1 deletion(-)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 4166b2bea2..2d973a643a 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -12,7 +12,9 @@
 #include <rte_kvargs.h>
 #include <bus_driver.h>
 #include <rte_class.h>
+#include <rte_bus_vdev.h>
 
+#include "virtual_pmd.h"
 #include "test.h"
 
 /* Check layer arguments. */
@@ -171,7 +173,7 @@ test_valid_devargs(void)
 	int ret;
 
 	ret = test_valid_devargs_cases(list, RTE_DIM(list));
-	if (vdev_bus != NULL && vdev_bus->parse("net_ring0", NULL) == 0)
+	if (vdev_bus != NULL && vdev_bus->parse("net_ring0", NULL, NULL) == 0)
 		/* Ring vdev driver enabled. */
 		ret |= test_valid_devargs_cases(legacy_ring_list,
 						RTE_DIM(legacy_ring_list));
@@ -302,6 +304,121 @@ test_invalid_devargs_parsing(void)
 	return fail;
 }
 
+static struct rte_device *
+create_pci_dev(const char *name)
+{
+	int port_id;
+	struct rte_ether_addr mac_addr = {
+		.addr_bytes = {
+				0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00
+			}
+		};
+
+	char pmd_name[RTE_ETH_NAME_MAX_LEN];
+	strlcpy(pmd_name, name, RTE_ETH_NAME_MAX_LEN);
+
+	port_id = virtual_ethdev_create(pmd_name,
+			&mac_addr, rte_socket_id(), 1);
+
+	if (port_id < 0)
+		return NULL;
+
+	return (&rte_eth_devices[port_id])->device;
+}
+
+static int
+test_pci(struct rte_bus *pci_bus, const char *dev_name, const char *name2)
+{
+	struct rte_device *pci_dev = create_pci_dev(dev_name);
+
+	if (pci_dev == NULL)
+		return -1;
+
+	pci_dev->bus = pci_bus;
+
+	if (rte_cmp_dev_name(pci_dev, name2) != 0) {
+		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
+			       pci_dev->name, name2, pci_dev->name, name2);
+		return -1;
+	}
+	return 0;
+}
+
+static int
+test_vdev(struct rte_bus *vdev_bus, const char *dev_name, const char *name2)
+{
+	/* create vdev */
+	if (rte_vdev_init(dev_name, "") < 0) {
+		printf("Failed to create vdev %s\n", dev_name);
+		return -1;
+	}
+
+	struct rte_device *vdev_dev = vdev_bus->find_device(NULL, rte_cmp_dev_name, dev_name);
+	if (vdev_dev == NULL) {
+		printf("Cannot find %s vdev\n", dev_name);
+		rte_vdev_uninit(dev_name);
+		return -1;
+	}
+	int ret = rte_cmp_dev_name(vdev_dev, name2);
+	if (ret != 0) {
+		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
+			       vdev_dev->name, name2, vdev_dev->name, name2);
+		return -1;
+	}
+
+	if (vdev_dev != vdev_bus->find_device(NULL, rte_cmp_dev_name, name2)) {
+		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
+			       vdev_dev->name, name2, vdev_dev->name, name2);
+		return -1;
+	}
+
+	rte_vdev_uninit(dev_name);
+	return 0;
+}
+
+static int
+test_valid_cmp_dev_name(void)
+{
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+
+	if (pci_bus) {
+		if ((test_pci(pci_bus, "0000:08:11.0", "08:11.0") != 0) ||
+			(test_pci(pci_bus, "08:12.0",  "0000:08:12.0") != 0) ||
+			(test_pci(pci_bus, "08:13.0",  "08:13.0") != 0) ||
+			(test_pci(pci_bus, "0000:08:14.0",  "0000:08:14.0") != 0))
+			return -1;
+	}
+
+	if (vdev_bus) {
+		if (test_vdev(vdev_bus, "net_null_test0", "net_null_test0") != 0)
+			return -1;
+	}
+	return 0;
+}
+
+static int
+test_invalid_cmp_dev_name(void)
+{
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+
+	if (pci_bus) {
+		if ((test_pci(pci_bus, "0000:08:15.0", "08:00.0") == 0) ||
+			(test_pci(pci_bus, "08:16.0",  "0000:08:15.0") == 0) ||
+			(test_pci(pci_bus, "08:17.0",  "08:13.0") == 0) ||
+			(test_pci(pci_bus, "0000:08:18.0",  "0000:08:14.0") == 0))
+			return -1;
+	}
+
+	if (vdev_bus) {
+		if ((test_vdev(vdev_bus, "net_null_test0", "net_null_test") == 0) ||
+			(test_vdev(vdev_bus, "net_null_test1", "net_null_test2") == 0))
+			return -1;
+	}
+	return 0;
+}
+
 static int
 test_devargs(void)
 {
@@ -317,6 +434,12 @@ test_devargs(void)
 	printf("== test devargs parsing invalid case ==\n");
 	if (test_invalid_devargs_parsing() < 0)
 		return -1;
+	printf("== test find device valid case ==\n");
+	if (test_valid_cmp_dev_name() < 0)
+		return -1;
+	printf("== test find device invalid case ==\n");
+	if (test_invalid_cmp_dev_name() < 0)
+		return -1;
 	return 0;
 }
 
-- 
2.34.1

[PATCH v5 4/4] lib: change find device and cmp dev name functions

[Shani Peretz]

Change find_device and rte_cmp_dev_name to accept a struct
with pointer and size instead of a void* pointer.

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 app/test/test_devargs.c                  | 23 +++++++++++++++++++----
 app/test/test_vdev.c                     | 23 ++++++++++++++++++-----
 drivers/bus/auxiliary/auxiliary_common.c |  2 +-
 drivers/bus/auxiliary/auxiliary_params.c | 10 +++++++---
 drivers/bus/cdx/cdx.c                    | 12 ++++++++----
 drivers/bus/dpaa/dpaa_bus.c              |  2 +-
 drivers/bus/fslmc/fslmc_bus.c            |  2 +-
 drivers/bus/ifpga/ifpga_bus.c            |  2 +-
 drivers/bus/pci/pci_common.c             |  2 +-
 drivers/bus/pci/pci_params.c             | 10 +++++++---
 drivers/bus/platform/platform.c          |  3 ++-
 drivers/bus/platform/platform_params.c   | 10 +++++++---
 drivers/bus/uacce/uacce.c                | 13 +++++++++----
 drivers/bus/vdev/vdev.c                  |  2 +-
 drivers/bus/vdev/vdev_params.c           | 10 +++++++---
 drivers/bus/vdev/vdev_private.h          |  2 +-
 drivers/bus/vmbus/vmbus_common.c         |  2 +-
 drivers/dma/idxd/idxd_bus.c              |  4 ++--
 drivers/net/bonding/rte_eth_bond_args.c  | 15 +++++----------
 drivers/net/vdev_netvsc/vdev_netvsc.c    |  8 ++++++--
 drivers/raw/ifpga/ifpga_rawdev.c         |  6 +++++-
 lib/eal/common/eal_common_bus.c          | 10 +++++++---
 lib/eal/common/eal_common_dev.c          | 18 +++++++++++++-----
 lib/eal/common/hotplug_mp.c              | 12 ++++++++++--
 lib/eal/include/bus_driver.h             |  2 +-
 lib/eal/include/rte_dev.h                | 15 ++++++++++-----
 lib/eal/linux/eal_dev.c                  |  6 +++++-
 27 files changed, 156 insertions(+), 70 deletions(-)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 2d973a643a..e7b73f2ff1 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -336,7 +336,12 @@ test_pci(struct rte_bus *pci_bus, const char *dev_name, const char *name2)
 
 	pci_dev->bus = pci_bus;
 
-	if (rte_cmp_dev_name(pci_dev, name2) != 0) {
+	struct rte_bus_address addr = {
+		.addr = name2,
+		.size = strlen(name2)
+	};
+
+	if (rte_cmp_dev_name(pci_dev, &addr) != 0) {
 		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
 			       pci_dev->name, name2, pci_dev->name, name2);
 		return -1;
@@ -353,20 +358,30 @@ test_vdev(struct rte_bus *vdev_bus, const char *dev_name, const char *name2)
 		return -1;
 	}
 
-	struct rte_device *vdev_dev = vdev_bus->find_device(NULL, rte_cmp_dev_name, dev_name);
+	struct rte_bus_address dev_addr = {
+		.addr = dev_name,
+		.size = strlen(dev_name)
+	};
+
+	struct rte_device *vdev_dev = vdev_bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
 	if (vdev_dev == NULL) {
 		printf("Cannot find %s vdev\n", dev_name);
 		rte_vdev_uninit(dev_name);
 		return -1;
 	}
-	int ret = rte_cmp_dev_name(vdev_dev, name2);
+
+	struct rte_bus_address dev_addr2 = {
+		.addr = name2,
+		.size = strlen(name2)
+	};
+	int ret = rte_cmp_dev_name(vdev_dev, &dev_addr2);
 	if (ret != 0) {
 		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
 			       vdev_dev->name, name2, vdev_dev->name, name2);
 		return -1;
 	}
 
-	if (vdev_dev != vdev_bus->find_device(NULL, rte_cmp_dev_name, name2)) {
+	if (vdev_dev != vdev_bus->find_device(NULL, rte_cmp_dev_name, &dev_addr2)) {
 		printf("rte_cmp_dev_name(%s, %s) device name (%s) not expected (%s)\n",
 			       vdev_dev->name, name2, vdev_dev->name, name2);
 		return -1;
diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
index 860fa260af..568c16593e 100644
--- a/app/test/test_vdev.c
+++ b/app/test/test_vdev.c
@@ -21,9 +21,9 @@ static const char * const valid_keys[] = {
 };
 
 static int
-cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
+cmp_dev_match(const struct rte_device *dev, const struct rte_bus_address *_kvlist)
 {
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const char *key = TEST_VDEV_KEY_NAME;
 	const char *name;
 
@@ -54,7 +54,11 @@ get_matching_vdev(const char *match_str)
 		}
 	}
 
-	dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = vdev_bus->find_device(NULL, cmp_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 
 	return dev;
@@ -76,7 +80,12 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test0\n");
 		goto fail;
 	}
-	dev0 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test0");
+
+	struct rte_bus_address dev0_addr = {
+		.addr = "net_null_test0",
+		.size = strlen("net_null_test0")
+	};
+	dev0 = vdev_bus->find_device(NULL, rte_cmp_dev_name, &dev0_addr);
 	if (dev0 == NULL) {
 		printf("Cannot find net_null_test0 vdev\n");
 		goto fail;
@@ -87,7 +96,11 @@ test_vdev_bus(void)
 		printf("Failed to create vdev net_null_test1\n");
 		goto fail;
 	}
-	dev1 = vdev_bus->find_device(NULL, rte_cmp_dev_name, "net_null_test1");
+	struct rte_bus_address dev1_addr = {
+		.addr = "net_null_test1",
+		.size = strlen("net_null_test1")
+	};
+	dev1 = vdev_bus->find_device(NULL, rte_cmp_dev_name, &dev1_addr);
 	if (dev1 == NULL) {
 		printf("Cannot find net_null_test1 vdev\n");
 		goto fail;
diff --git a/drivers/bus/auxiliary/auxiliary_common.c b/drivers/bus/auxiliary/auxiliary_common.c
index ba2f69e851..091e559c60 100644
--- a/drivers/bus/auxiliary/auxiliary_common.c
+++ b/drivers/bus/auxiliary/auxiliary_common.c
@@ -300,7 +300,7 @@ rte_auxiliary_remove_device(struct rte_auxiliary_device *auxiliary_dev)
 
 static struct rte_device *
 auxiliary_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		      const void *data)
+		      const struct rte_bus_address *data)
 {
 	const struct rte_auxiliary_device *pstart;
 	struct rte_auxiliary_device *adev;
diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
index e4c7ee0c3b..93f0bd46be 100644
--- a/drivers/bus/auxiliary/auxiliary_params.c
+++ b/drivers/bus/auxiliary/auxiliary_params.c
@@ -22,9 +22,9 @@ static const char * const auxiliary_params_keys[] = {
 
 static int
 auxiliary_dev_match(const struct rte_device *dev,
-	      const void *_kvlist)
+	      const struct rte_bus_address *_kvlist)
 {
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
 	const char *name;
 
@@ -59,7 +59,11 @@ auxiliary_dev_iterate(const void *start,
 		}
 	}
 	find_device = auxiliary_bus.bus.find_device;
-	dev = find_device(start, auxiliary_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = find_device(start, auxiliary_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 	return dev;
 }
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index b97f1ce1af..58596d1f71 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -500,7 +500,7 @@ rte_cdx_unregister(struct rte_cdx_driver *driver)
 
 static struct rte_device *
 cdx_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		const void *data)
+		const struct rte_bus_address *data)
 {
 	const struct rte_cdx_device *cdx_start;
 	struct rte_cdx_device *cdx_dev;
@@ -613,9 +613,9 @@ cdx_get_iommu_class(void)
 
 static int
 cdx_dev_match(const struct rte_device *dev,
-		const void *_kvlist)
+		const struct rte_bus_address *_kvlist)
 {
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const char *key = cdx_params_keys[RTE_CDX_PARAM_NAME];
 	const char *name;
 
@@ -649,7 +649,11 @@ cdx_dev_iterate(const void *start,
 		}
 	}
 	find_device = rte_cdx_bus.bus.find_device;
-	dev = find_device(start, cdx_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = find_device(start, cdx_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 	return dev;
 }
diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
index f768fb9454..5a24fc2f47 100644
--- a/drivers/bus/dpaa/dpaa_bus.c
+++ b/drivers/bus/dpaa/dpaa_bus.c
@@ -745,7 +745,7 @@ rte_dpaa_bus_probe(void)
 
 static struct rte_device *
 rte_dpaa_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		     const void *data)
+		     const struct rte_bus_address *data)
 {
 	struct rte_dpaa_device *dev;
 	const struct rte_dpaa_device *dstart;
diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index a525b5e245..7a80cb41f1 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -497,7 +497,7 @@ rte_fslmc_probe(void)
 
 static struct rte_device *
 rte_fslmc_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		      const void *data)
+		      const struct rte_bus_address *data)
 {
 	const struct rte_dpaa2_device *dstart;
 	struct rte_dpaa2_device *dev;
diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c
index d5d9a86a97..195429c1d4 100644
--- a/drivers/bus/ifpga/ifpga_bus.c
+++ b/drivers/bus/ifpga/ifpga_bus.c
@@ -443,7 +443,7 @@ ifpga_unplug(struct rte_device *dev)
 
 static struct rte_device *
 ifpga_find_device(const struct rte_device *start,
-	rte_dev_cmp_t cmp, const void *data)
+	rte_dev_cmp_t cmp, const struct rte_bus_address *data)
 {
 	struct rte_afu_device *afu_dev;
 
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 82632bc473..29cec5f588 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -540,7 +540,7 @@ rte_pci_remove_device(struct rte_pci_device *pci_dev)
 
 static struct rte_device *
 pci_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		const void *data)
+		const struct rte_bus_address *data)
 {
 	const struct rte_pci_device *pstart;
 	struct rte_pci_device *pdev;
diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 087ec38bb9..de7ea8f2f8 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -41,9 +41,9 @@ pci_addr_kv_cmp(const char *key __rte_unused,
 
 static int
 pci_dev_match(const struct rte_device *dev,
-	      const void *_kvlist)
+	      const struct rte_bus_address *_kvlist)
 {
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const struct rte_pci_device *pdev;
 
 	if (kvlist == NULL)
@@ -76,7 +76,11 @@ rte_pci_dev_iterate(const void *start,
 		}
 	}
 	find_device = rte_pci_bus.bus.find_device;
-	dev = find_device(start, pci_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = find_device(start, pci_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 	return dev;
 }
diff --git a/drivers/bus/platform/platform.c b/drivers/bus/platform/platform.c
index 87ed7404e1..5b72418ad0 100644
--- a/drivers/bus/platform/platform.c
+++ b/drivers/bus/platform/platform.c
@@ -473,7 +473,8 @@ platform_bus_probe(void)
 }
 
 static struct rte_device *
-platform_bus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp, const void *data)
+platform_bus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		const struct rte_bus_address *data)
 {
 	struct rte_platform_device *pdev;
 
diff --git a/drivers/bus/platform/platform_params.c b/drivers/bus/platform/platform_params.c
index 8a6214b97c..00f6774c8c 100644
--- a/drivers/bus/platform/platform_params.c
+++ b/drivers/bus/platform/platform_params.c
@@ -27,10 +27,10 @@ static const char * const platform_params_keys[] = {
 };
 
 static int
-platform_dev_match(const struct rte_device *dev, const void *_kvlist)
+platform_dev_match(const struct rte_device *dev, const struct rte_bus_address *_kvlist)
 {
 	const char *key = platform_params_keys[RTE_PLATFORM_PARAM_NAME];
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const char *name;
 
 	/* no kvlist arg, all devices match */
@@ -68,7 +68,11 @@ platform_bus_dev_iterate(const void *start, const char *str,
 		return NULL;
 	}
 
-	dev = platform_bus.bus.find_device(start, platform_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = platform_bus.bus.find_device(start, platform_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 
 	return dev;
diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
index b18fbe1103..76d4c3f467 100644
--- a/drivers/bus/uacce/uacce.c
+++ b/drivers/bus/uacce/uacce.c
@@ -508,7 +508,8 @@ uacce_unplug(struct rte_device *dev)
 }
 
 static struct rte_device *
-uacce_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,  const void *data)
+uacce_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
+		const struct rte_bus_address *data)
 {
 	const struct rte_uacce_device *uacce_start;
 	struct rte_uacce_device *uacce_dev;
@@ -549,10 +550,10 @@ uacce_parse(const char *name, void *addr, int *size)
 }
 
 static int
-uacce_dev_match(const struct rte_device *dev, const void *_kvlist)
+uacce_dev_match(const struct rte_device *dev, const struct rte_bus_address *_kvlist)
 {
 	const char *key = uacce_params_keys[RTE_UACCE_PARAM_NAME];
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const char *name;
 
 	/* no kvlist arg, all devices match. */
@@ -583,7 +584,11 @@ uacce_dev_iterate(const void *start, const char *str,
 		}
 	}
 	find_device = uacce_bus.bus.find_device;
-	dev = find_device(start, uacce_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = find_device(start, uacce_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 	return dev;
 }
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 0d3c8ca3c5..e287c8cf64 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -621,7 +621,7 @@ vdev_cleanup(void)
 
 struct rte_device *
 rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		     const void *data)
+		     const struct rte_bus_address *data)
 {
 	const struct rte_vdev_device *vstart;
 	struct rte_vdev_device *dev;
diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
index 68ae09e2e9..7197c0f350 100644
--- a/drivers/bus/vdev/vdev_params.c
+++ b/drivers/bus/vdev/vdev_params.c
@@ -24,9 +24,9 @@ static const char * const vdev_params_keys[] = {
 
 static int
 vdev_dev_match(const struct rte_device *dev,
-	       const void *_kvlist)
+	       const struct rte_bus_address *_kvlist)
 {
-	const struct rte_kvargs *kvlist = _kvlist;
+	const struct rte_kvargs *kvlist = _kvlist->addr;
 	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
 	const char *name;
 
@@ -58,7 +58,11 @@ rte_vdev_dev_iterate(const void *start,
 			return NULL;
 		}
 	}
-	dev = rte_vdev_find_device(start, vdev_dev_match, kvargs);
+	struct rte_bus_address dev_addr = {
+		.addr = kvargs,
+		.size = sizeof(struct rte_kvargs),
+	};
+	dev = rte_vdev_find_device(start, vdev_dev_match, &dev_addr);
 	rte_kvargs_free(kvargs);
 	return dev;
 }
diff --git a/drivers/bus/vdev/vdev_private.h b/drivers/bus/vdev/vdev_private.h
index e683f5f133..ef15266b16 100644
--- a/drivers/bus/vdev/vdev_private.h
+++ b/drivers/bus/vdev/vdev_private.h
@@ -14,7 +14,7 @@ extern "C" {
 struct rte_device *
 rte_vdev_find_device(const struct rte_device *start,
 		     rte_dev_cmp_t cmp,
-		     const void *data);
+		     const struct rte_bus_address *data);
 
 void *
 rte_vdev_dev_iterate(const void *start,
diff --git a/drivers/bus/vmbus/vmbus_common.c b/drivers/bus/vmbus/vmbus_common.c
index 18805aee0d..06aa64c20d 100644
--- a/drivers/bus/vmbus/vmbus_common.c
+++ b/drivers/bus/vmbus/vmbus_common.c
@@ -323,7 +323,7 @@ vmbus_remove_device(struct rte_vmbus_device *vmbus_dev)
 /* VMBUS doesn't support hotplug */
 static struct rte_device *
 vmbus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-		  const void *data)
+		  const struct rte_bus_address *data)
 {
 	struct rte_vmbus_device *dev;
 
diff --git a/drivers/dma/idxd/idxd_bus.c b/drivers/dma/idxd/idxd_bus.c
index d68e8b630e..ee33ddbfb4 100644
--- a/drivers/dma/idxd/idxd_bus.c
+++ b/drivers/dma/idxd/idxd_bus.c
@@ -45,7 +45,7 @@ struct dsa_bus;
 static int dsa_scan(void);
 static int dsa_probe(void);
 static struct rte_device *dsa_find_device(const struct rte_device *start,
-		rte_dev_cmp_t cmp,  const void *data);
+		rte_dev_cmp_t cmp,  const struct rte_bus_address *data);
 static enum rte_iova_mode dsa_get_iommu_class(void);
 static int dsa_addr_parse(const char *name, void *addr, int *size);
 
@@ -366,7 +366,7 @@ dsa_scan(void)
 
 static struct rte_device *
 dsa_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
-			 const void *data)
+			 const struct rte_bus_address *data)
 {
 	struct rte_dsa_device *dev = TAILQ_FIRST(&dsa_bus.device_list);
 
diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index bdec5d61d4..2ef42db796 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -23,15 +23,6 @@ const char *pmd_bond_init_valid_arguments[] = {
 	NULL
 };
 
-static inline int
-bond_pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr)
-{
-	const struct rte_pci_device *pdev = RTE_DEV_TO_PCI_CONST(dev);
-	const struct rte_pci_addr *paddr = _pci_addr;
-
-	return rte_pci_addr_cmp(&pdev->addr, paddr);
-}
-
 static inline int
 find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
 {
@@ -45,7 +36,11 @@ find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
 		return -1;
 	}
 
-	dev = pci_bus->find_device(NULL, bond_pci_addr_cmp, pci_addr);
+	struct rte_bus_address dev_addr = {
+		.addr = pci_addr,
+		.size = PCI_PRI_STR_SIZE
+	};
+	dev = pci_bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
 	if (dev == NULL) {
 		RTE_BOND_LOG(ERR, "unable to find PCI device");
 		return -1;
diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index f4a84783ce..020b703906 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -771,7 +771,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_vdev_netvsc,
 /** Compare function for vdev find device operation. */
 static int
 vdev_netvsc_cmp_rte_device(const struct rte_device *dev1,
-			   __rte_unused const void *_dev2)
+			   __rte_unused const struct rte_bus_address *_dev2)
 {
 	return strncmp(dev1->devargs->name, VDEV_NETVSC_DRIVER_NAME,
 		       VDEV_NETVSC_DRIVER_NAME_LEN);
@@ -794,8 +794,12 @@ vdev_netvsc_scan_callback(__rte_unused void *arg)
 			     VDEV_NETVSC_DRIVER_NAME_LEN))
 			return;
 
+	struct rte_bus_address dev_addr = {
+		.addr = VDEV_NETVSC_DRIVER_NAME,
+		.size = VDEV_NETVSC_DRIVER_NAME_LEN,
+	};
 	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
-				VDEV_NETVSC_DRIVER_NAME);
+				&dev_addr);
 	if (dev)
 		return;
 	if (rte_devargs_add(RTE_DEVTYPE_VIRTUAL, VDEV_NETVSC_DRIVER_NAME))
diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index 7238246e1c..9a081fd5d8 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -1868,7 +1868,11 @@ ifpga_cfg_remove(struct rte_vdev_device *vdev)
 		args.port, args.bdf);
 	bus = rte_bus_find_by_name(RTE_STR(IFPGA_BUS_NAME));
 	if (bus) {
-		if (bus->find_device(NULL, rte_cmp_dev_name, dev_name)) {
+		struct rte_bus_address dev_addr = {
+			.addr = dev_name,
+			.size = RTE_RAWDEV_NAME_MAX_LEN
+		};
+		if (bus->find_device(NULL, rte_cmp_dev_name, &dev_addr)) {
 			ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
 				dev_name);
 		}
diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
index b1bb5df5b7..15d3cfb29b 100644
--- a/lib/eal/common/eal_common_bus.c
+++ b/lib/eal/common/eal_common_bus.c
@@ -159,9 +159,9 @@ rte_bus_find(const struct rte_bus *start, rte_bus_cmp_t cmp,
 }
 
 static int
-cmp_rte_device(const struct rte_device *dev1, const void *_dev2)
+cmp_rte_device(const struct rte_device *dev1, const struct rte_bus_address *_dev2)
 {
-	const struct rte_device *dev2 = _dev2;
+	const struct rte_device *dev2 = _dev2->addr;
 
 	return dev1 != dev2;
 }
@@ -171,7 +171,11 @@ bus_find_device(const struct rte_bus *bus, const void *_dev)
 {
 	struct rte_device *dev;
 
-	dev = bus->find_device(NULL, cmp_rte_device, _dev);
+	struct rte_bus_address dev_addr = {
+		.addr = _dev,
+		.size = 0,
+	};
+	dev = bus->find_device(NULL, cmp_rte_device, &dev_addr);
 	return dev == NULL;
 }
 
diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index c0e2202482..4e4951a73f 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -108,7 +108,7 @@ struct dev_next_ctx {
 	(((struct dev_next_ctx *)(intptr_t)ptr)->cls_str)
 
 int
-rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
+rte_cmp_dev_name(const struct rte_device *dev1, const struct rte_bus_address *dev2_addr)
 {
 	void *parsed_name1;
 	void *parsed_name2;
@@ -117,7 +117,7 @@ rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
 	int ret;
 
 	if (dev1->bus->parse(dev1->name, NULL, &size1) != 0 ||
-		dev1->bus->parse(name2, NULL, &size2) != 0)
+		dev1->bus->parse(dev2_addr->addr, NULL, &size2) != 0)
 		return 1;
 
 	if (size1 != size2)
@@ -137,7 +137,7 @@ rte_cmp_dev_name(const struct rte_device *dev1, const void *name2)
 	memset(parsed_name2, 0, size2);
 
 	dev1->bus->parse(dev1->name, parsed_name1, NULL);
-	dev1->bus->parse(name2, parsed_name2, NULL);
+	dev1->bus->parse(dev2_addr->addr, parsed_name2, NULL);
 
 	ret = memcmp(parsed_name1, parsed_name2, size1);
 	free(parsed_name1);
@@ -228,7 +228,11 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	if (ret)
 		goto err_devarg;
 
-	dev = da->bus->find_device(NULL, rte_cmp_dev_name, da->name);
+	struct rte_bus_address dev_addr = {
+		.addr = da->name,
+		.size = RTE_DEV_NAME_MAX_LEN
+	};
+	dev = da->bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find device (%s)",
 			da->name);
@@ -366,7 +370,11 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -ENOENT;
 	}
 
-	dev = bus->find_device(NULL, rte_cmp_dev_name, devname);
+	struct rte_bus_address dev_addr = {
+		.addr = devname,
+		.size = strlen(devname)
+	};
+	dev = bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
 	if (dev == NULL) {
 		EAL_LOG(ERR, "Cannot find plugged device (%s)", devname);
 		return -EINVAL;
diff --git a/lib/eal/common/hotplug_mp.c b/lib/eal/common/hotplug_mp.c
index a2623c96c3..f4cce7c2e5 100644
--- a/lib/eal/common/hotplug_mp.c
+++ b/lib/eal/common/hotplug_mp.c
@@ -128,7 +128,11 @@ __handle_secondary_request(void *param)
 			goto finish;
 		}
 
-		dev = bus->find_device(NULL, rte_cmp_dev_name, da.name);
+		struct rte_bus_address dev_addr = {
+			.addr = da.name,
+			.size = RTE_DEV_NAME_MAX_LEN
+		};
+		dev = bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da.name);
 			ret = -ENOENT;
@@ -255,7 +259,11 @@ static void __handle_primary_request(void *param)
 			goto quit;
 		}
 
-		dev = bus->find_device(NULL, rte_cmp_dev_name, da->name);
+		struct rte_bus_address dev_addr = {
+			.addr = da->name,
+			.size = RTE_DEV_NAME_MAX_LEN
+		};
+		dev = bus->find_device(NULL, rte_cmp_dev_name, &dev_addr);
 		if (dev == NULL) {
 			EAL_LOG(ERR, "Cannot find plugged device (%s)", da->name);
 			ret = -ENOENT;
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
index 01447670de..16e8838868 100644
--- a/lib/eal/include/bus_driver.h
+++ b/lib/eal/include/bus_driver.h
@@ -70,7 +70,7 @@ typedef int (*rte_bus_probe_t)(void);
  */
 typedef struct rte_device *
 (*rte_bus_find_device_t)(const struct rte_device *start, rte_dev_cmp_t cmp,
-			 const void *data);
+			 const struct rte_bus_address *data);
 
 /**
  * Implementation specific probe function which is responsible for linking
diff --git a/lib/eal/include/rte_dev.h b/lib/eal/include/rte_dev.h
index 2df8143af1..47a6c22b53 100644
--- a/lib/eal/include/rte_dev.h
+++ b/lib/eal/include/rte_dev.h
@@ -171,19 +171,24 @@ int rte_dev_is_probed(const struct rte_device *dev);
 int rte_eal_hotplug_add(const char *busname, const char *devname,
 			const char *drvargs);
 
+struct rte_bus_address {
+	const void *addr;
+	size_t size;
+};
+
 /**
  * General device name comparison. Will compare by using the specific bus
  * compare function or by comparing the names directly.
  *
  * @param dev
  *   Device handle.
- * @param name
- *   Name to compare against.
+ * @param addr
+ *   Address to compare against.
  * @return
- *   0 if the device matches the name. Nonzero otherwise.
+ *   0 if the device matches the address. Nonzero otherwise.
  */
 __rte_internal
-int rte_cmp_dev_name(const struct rte_device *dev, const void *name);
+int rte_cmp_dev_name(const struct rte_device *dev, const struct rte_bus_address *addr);
 
 /**
  * Add matching devices.
@@ -245,7 +250,7 @@ int rte_dev_remove(struct rte_device *dev);
  *   <0 if ordering is possible and the device is lower than the data.
  *   >0 if ordering is possible and the device is greater than the data.
  */
-typedef int (*rte_dev_cmp_t)(const struct rte_device *dev, const void *data);
+typedef int (*rte_dev_cmp_t)(const struct rte_device *dev, const struct rte_bus_address *data);
 
 #define RTE_PMD_EXPORT_NAME_ARRAY(n, idx) n##idx[]
 
diff --git a/lib/eal/linux/eal_dev.c b/lib/eal/linux/eal_dev.c
index 3b68cda87f..22c126278b 100644
--- a/lib/eal/linux/eal_dev.c
+++ b/lib/eal/linux/eal_dev.c
@@ -272,8 +272,12 @@ dev_uev_handler(__rte_unused void *param)
 				goto failure_handle_err;
 			}
 
+			const struct rte_bus_address dev_addr = {
+				.addr = uevent.devname,
+				.size = strlen(uevent.devname)
+			};
 			dev = bus->find_device(NULL, rte_cmp_dev_name,
-					       uevent.devname);
+					       &dev_addr);
 			if (dev == NULL) {
 				EAL_LOG(ERR, "Cannot find device (%s) on "
 					"bus (%s)", uevent.devname, busname);
-- 
2.34.1

Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Stephen Hemminger]

On Wed, 5 Feb 2025 20:16:48 +0000
Shani Peretz <shperetz@nvidia.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, 5 February 2025 20:46
> > To: Shani Peretz <shperetz@nvidia.com>
> > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> > Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> > <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> > Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> > <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> > Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, 5 Feb 2025 17:37:54 +0000
> > Shani Peretz <shperetz@nvidia.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > Sent: Wednesday, 5 February 2025 18:42
> > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > > <thomas@monjalon.net>; Tyler Retzlaff
> > > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta  
> > <nipun.gupta@amd.com>;  
> > > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > > > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI
> > > > numbers
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Wed, 5 Feb 2025 16:36:11 +0000
> > > > Shani Peretz <shperetz@nvidia.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > > > Sent: Wednesday, 29 January 2025 18:25
> > > > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > > > > <thomas@monjalon.net>; Tyler Retzlaff
> > > > > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > > > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta  
> > > > <nipun.gupta@amd.com>;  
> > > > > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>;
> > > > > > Bruce Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of
> > > > > > PCI numbers
> > > > > >
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > On Wed, 29 Jan 2025 10:54:16 +0200 Shani Peretz
> > > > > > <shperetz@nvidia.com> wrote:
> > > > > >  
> > > > > > > +create_pci_dev(const char *name) {
> > > > > > > +     int port_id;
> > > > > > > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > > > > > > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > > > > > > +*)slave_mac1;  
> > > > > >
> > > > > > Use different initializer and you can avoid the need for cast here.
> > > > > >
> > > > > >  
> > > > > > >
> > > > > > > +/**
> > > > > > > + * General device name comparison. Will compare by using the
> > > > > > > +specific bus
> > > > > > > + * compare function or by comparing the names directly.
> > > > > > > + *
> > > > > > > + * @param dev
> > > > > > > + *   Device handle.
> > > > > > > + * @param name
> > > > > > > + *   Name to compare against.
> > > > > > > + * @return
> > > > > > > + *   0 if the device matches the name. Nonzero otherwise.
> > > > > > > + */
> > > > > > > +__rte_internal
> > > > > > > +int rte_cmp_dev_name(const struct rte_device *dev, const void
> > > > > > > +*name);  
> > > > > >
> > > > > > It would make more sense to me if name was a character not void  
> > pointer.  
> > > > > >
> > > > > > The design might be clearer if bus address was more of an
> > > > > > typedef with a pointer and size together. Treat it more like an object.  
> > > > >
> > > > >
> > > > > Okay so just to understand,
> > > > > this is the struct I am adding:
> > > > >
> > > > >       struct rte_bus_address {
> > > > >              const void *addr;
> > > > >              size_t size;
> > > > >       };
> > > > >
> > > > > This is how I pass it to find_device:
> > > > >
> > > > >       struct rte_bus_address dev_addr = {
> > > > >                 .addr = da->name,
> > > > >                 .size = RTE_DEV_NAME_MAX_LEN
> > > > >       };
> > > > >       dev = da->bus->find_device(NULL, rte_cmp_dev_name,
> > > > > &dev_addr);
> > > > >
> > > > > And this is how I use it in rte_cmp_dev_name:
> > > > >
> > > > >       rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
> > > > >        {
> > > > >              const struct rte_bus_address *dev2_addr = addr;
> > > > >       …
> > > > >       }
> > > > >
> > > > > Is that what you meant?
> > > > > Also, I'm not sure if the size is really needed, because we check
> > > > > the size of the parsed name, which may be different than the size
> > > > > of the original name
> > > > >  
> > > >
> > > > It would be best to pass rte_bus_address to rte_cmp_dev_name rather
> > > > than having implied cast.
> > > > Not sure if that is possible without breaking API though.  
> > >
> > > I think you are right, also it will require to change the signature of
> > > find_device which will make it even bigger change  
> > 
> > Could you post patch with something "good enough" that builds and passes
> > tests?  
> 
> You mean with the rte_bus_address change or without?
> Without it it was tested and passed. With it I only ran few tests locally and they passed

With the bus address change how much impact? Not sure it really matter much though
both solutions are ok, it is more about dealing with other buses in future.

Re: [PATCH v5 3/4] app/test: add tests to find devices

[Stephen Hemminger]

On Thu, 6 Feb 2025 02:08:37 +0200
Shani Peretz <shperetz@nvidia.com> wrote:

> +static struct rte_device *
> +create_pci_dev(const char *name)
> +{
> +	int port_id;
> +	struct rte_ether_addr mac_addr = {
> +		.addr_bytes = {
> +				0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00
> +			}
> +		};
> +
> +	char pmd_name[RTE_ETH_NAME_MAX_LEN];

Why hardcode a MAC address, which could lead to future issues.
Instead use rte_eth_random_addr().