[v2,2/4] devargs: simplify parameters of removal function

  The function rte_devargs_remove(), which is intended to be internal,
can take a devargs structure as argument.
The matching is still using string comparison of bus name and
device name.
It is simpler and may allow a different devargs matching in future.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/bus/ifpga/ifpga_bus.c               |  5 +----
 drivers/bus/vdev/vdev.c                     |  8 ++------
 lib/librte_eal/common/eal_common_dev.c      |  4 ++--
 lib/librte_eal/common/eal_common_devargs.c  |  8 ++++----
 lib/librte_eal/common/include/rte_devargs.h | 10 +++-------
 5 files changed, 12 insertions(+), 23 deletions(-)
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, September 27, 2018 12:48 AM
> To: dev@dpdk.org
> Cc: gaetan.rivet@6wind.com; Ophir Munk <ophirmu@mellanox.com>;
> qi.z.zhang@intel.com; ferruh.yigit@intel.com; ktraynor@redhat.com
> Subject: [PATCH v2 2/4] devargs: simplify parameters of removal function
> 
> The function rte_devargs_remove(), which is intended to be internal, can
> take a devargs structure as argument.
> The matching is still using string comparison of bus name and device name.
> It is simpler and may allow a different devargs matching in future.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  drivers/bus/ifpga/ifpga_bus.c               |  5 +----
>  drivers/bus/vdev/vdev.c                     |  8 ++------
>  lib/librte_eal/common/eal_common_dev.c      |  4 ++--
>  lib/librte_eal/common/eal_common_devargs.c  |  8 ++++----
> lib/librte_eal/common/include/rte_devargs.h | 10 +++-------
>  5 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c
> index b324872ee..0e17ea9a3 100644
> --- a/drivers/bus/ifpga/ifpga_bus.c
> +++ b/drivers/bus/ifpga/ifpga_bus.c
> @@ -363,7 +363,6 @@ static int
>  ifpga_unplug(struct rte_device *dev)
>  {
>  	struct rte_afu_device *afu_dev = NULL;
> -	struct rte_devargs *devargs = NULL;
>  	int ret;
> 
>  	if (dev == NULL)
> @@ -373,15 +372,13 @@ ifpga_unplug(struct rte_device *dev)
>  	if (!afu_dev)
>  		return -ENOENT;
> 
> -	devargs = dev->devargs;
> -
>  	ret = ifpga_remove_driver(afu_dev);
>  	if (ret)
>  		return ret;
> 
>  	TAILQ_REMOVE(&ifpga_afu_dev_list, afu_dev, next);
> 
> -	rte_devargs_remove(devargs->bus->name, devargs->name);
> +	rte_devargs_remove(dev->devargs);
>  	free(afu_dev);
>  	return 0;
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index
> 69dee89a8..390c2ce70 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -248,7 +248,6 @@ int
>  rte_vdev_init(const char *name, const char *args)  {
>  	struct rte_vdev_device *dev;
> -	struct rte_devargs *devargs;
>  	int ret;
> 
>  	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> @@ -259,9 +258,8 @@ rte_vdev_init(const char *name, const char *args)
>  			if (ret > 0)
>  				VDEV_LOG(ERR, "no driver found for %s",
> name);
>  			/* If fails, remove it from vdev list */
> -			devargs = dev->device.devargs;
>  			TAILQ_REMOVE(&vdev_device_list, dev, next);
> -			rte_devargs_remove(devargs->bus->name, devargs-
> >name);
> +			rte_devargs_remove(dev->device.devargs);
>  			free(dev);
>  		}
>  	}
> @@ -289,7 +287,6 @@ int
>  rte_vdev_uninit(const char *name)
>  {
>  	struct rte_vdev_device *dev;
> -	struct rte_devargs *devargs;
>  	int ret;
> 
>  	if (name == NULL)
> @@ -308,8 +305,7 @@ rte_vdev_uninit(const char *name)
>  		goto unlock;
> 
>  	TAILQ_REMOVE(&vdev_device_list, dev, next);
> -	devargs = dev->device.devargs;
> -	rte_devargs_remove(devargs->bus->name, devargs->name);
> +	rte_devargs_remove(dev->device.devargs);
>  	free(dev);
> 
>  unlock:
> diff --git a/lib/librte_eal/common/eal_common_dev.c
> b/lib/librte_eal/common/eal_common_dev.c
> index 678dbcac7..e81ff4258 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -186,7 +186,7 @@ int __rte_experimental rte_eal_hotplug_add(const
> char *busname, const char *devn
>  	return 0;
> 
>  err_devarg:
> -	if (rte_devargs_remove(busname, devname)) {
> +	if (rte_devargs_remove(da)) {

Can you please explain why calling with 'da' parameter (which may have different internal fields of busname and devname than the explicit busname and devname parameters) - is correct?

>  		free(da->args);
>  		free(da);
>  	}
> @@ -227,7 +227,7 @@ rte_eal_hotplug_remove(const char *busname,
> const char *devname)
>  	if (ret)
>  		RTE_LOG(ERR, EAL, "Driver cannot detach the device
> (%s)\n",
>  			dev->name);
> -	rte_devargs_remove(busname, devname);
> +	rte_devargs_remove(dev->devargs);

Can you please explain why calling with 'dev->devargs' parameter (which may have different internal fields of busname and devname than the explicit busname and devname parameters) - is correct?

>  	return ret;
>  }
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c
> b/lib/librte_eal/common/eal_common_devargs.c
> index f63d2c663..2f2bb4d90 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -266,7 +266,7 @@ rte_devargs_insert(struct rte_devargs *da)  {
>  	int ret;
> 
> -	ret = rte_devargs_remove(da->bus->name, da->name);
> +	ret = rte_devargs_remove(da);
>  	if (ret < 0)
>  		return ret;
>  	TAILQ_INSERT_TAIL(&devargs_list, da, next); @@ -312,14 +312,14
> @@ rte_devargs_add(enum rte_devtype devtype, const char *devargs_str)  }
> 
>  int __rte_experimental
> -rte_devargs_remove(const char *busname, const char *devname)
> +rte_devargs_remove(struct rte_devargs *devargs)
>  {
>  	struct rte_devargs *d;
>  	void *tmp;
> 
>  	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> -		if (strcmp(d->bus->name, busname) == 0 &&
> -		    strcmp(d->name, devname) == 0) {
> +		if (strcmp(d->bus->name, devargs->bus->name) == 0 &&
> +		    strcmp(d->name, devargs->name) == 0) {
>  			TAILQ_REMOVE(&devargs_list, d, next);
>  			free(d->args);
>  			free(d);
> diff --git a/lib/librte_eal/common/include/rte_devargs.h
> b/lib/librte_eal/common/include/rte_devargs.h
> index 0eef6e9c4..b1f121f83 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -176,11 +176,8 @@ int rte_devargs_add(enum rte_devtype devtype,
> const char *devargs_str);
>   * Its resources are freed.
>   * If the devargs cannot be found, nothing happens.
>   *
> - * @param busname
> - *   bus name of the devargs to remove.
> - *
> - * @param devname
> - *   device name of the devargs to remove.
> + * @param devargs
> + *   The instance or a copy of devargs to remove.
>   *
>   * @return
>   *   0 on success.
> @@ -188,8 +185,7 @@ int rte_devargs_add(enum rte_devtype devtype,
> const char *devargs_str);
>   *   >0 if the devargs was not within the user device list.
>   */
>  __rte_experimental
> -int rte_devargs_remove(const char *busname,
> -		       const char *devname);
> +int rte_devargs_remove(struct rte_devargs *devargs);
> 
>  /**
>   * Count the number of user devices of a specified type
> --
> 2.19.0
  

Hi,

27/09/2018 10:24, Ophir Munk:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > The function rte_devargs_remove(), which is intended to be internal, can
> > take a devargs structure as argument.
> > The matching is still using string comparison of bus name and device name.
> > It is simpler and may allow a different devargs matching in future.
[...]
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -186,7 +186,7 @@ int __rte_experimental rte_eal_hotplug_add(const
> > char *busname, const char *devn
> >  	return 0;
> > 
> >  err_devarg:
> > -	if (rte_devargs_remove(busname, devname)) {
> > +	if (rte_devargs_remove(da)) {
> 
> Can you please explain why calling with 'da' parameter
> (which may have different internal fields of busname and devname
> than the explicit busname and devname parameters) - is correct?

Before devargs is parsed and inserted in the global list,
devargs values are NULL, so nothing is found (as before this patch).
rte_devargs_remove(da) will have no effect and will return 1,
so the devargs will be freed by code in the if block.

When devargs is inserted in the list, it will have the values
of busname and devname, so it is the same as before this patch.

However, there may be an issue dereferencing devargs->bus->name
when devargs is not parsed. I must add this code in rte_devargs_remove:
    if (devargs->bus == NULL)
        return 1;

[...]
> > @@ -227,7 +227,7 @@ rte_eal_hotplug_remove(const char *busname,
> > const char *devname)
> >  	if (ret)
> >  		RTE_LOG(ERR, EAL, "Driver cannot detach the device
> > (%s)\n",
> >  			dev->name);
> > -	rte_devargs_remove(busname, devname);
> > +	rte_devargs_remove(dev->devargs);
> 
> Can you please explain why calling with 'dev->devargs' parameter
> (which may have different internal fields of busname and devname
> than the explicit busname and devname parameters) - is correct?

The device is found by checking the busname and devname.
I don't see how the devargs values may be different.