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

[Hemant Agrawal <hemant.agrawal@oss.nxp.com>]

On 06-02-2025 05:38, Shani Peretz wrote:
> 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);
>   };

for DPAA/FSLMC:

Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>