[dpdk-dev,v2,02/18] eal: remove generic devtype

  The devtype is now entirely defined by the device bus. As such, it is
already characterized by the bus identifier within an rte_devargs.

The rte_devtype enum can disappear, along with crutches added during
this transition.

rte_eal_devargs_type_count becomes useless and is removed.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/bus/pci/pci_common.c                    | 16 ++------------
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 -
 lib/librte_eal/common/eal_common_devargs.c      | 20 +----------------
 lib/librte_eal/common/eal_common_options.c      | 19 +++++-----------
 lib/librte_eal/common/include/rte_dev.h         |  8 -------
 lib/librte_eal/common/include/rte_devargs.h     | 29 +------------------------
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 -
 7 files changed, 9 insertions(+), 85 deletions(-)
  

Gaetan Rivet <gaetan.rivet@6wind.com> writes:

> The devtype is now entirely defined by the device bus. As such, it is
> already characterized by the bus identifier within an rte_devargs.
>
> The rte_devtype enum can disappear, along with crutches added during
> this transition.
>
> rte_eal_devargs_type_count becomes useless and is removed.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/bus/pci/pci_common.c                    | 16 ++------------
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 -
>  lib/librte_eal/common/eal_common_devargs.c      | 20 +----------------
>  lib/librte_eal/common/eal_common_options.c      | 19 +++++-----------
>  lib/librte_eal/common/include/rte_dev.h         |  8 -------
>  lib/librte_eal/common/include/rte_devargs.h     | 29 +------------------------
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 -
>  7 files changed, 9 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index bbe862b..5fbcf11 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -172,15 +172,6 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>  			loc->domain, loc->bus, loc->devid, loc->function,
>  			dev->device.numa_node);
>  
> -	/* no initialization when blacklisted, return without error */
> -	if (dev->device.devargs != NULL &&
> -		dev->device.devargs->policy ==
> -			RTE_DEV_BLACKLISTED) {
> -		RTE_LOG(INFO, EAL, "  Device is blacklisted, not"
> -			" initializing\n");
> -		return 1;
> -	}
> -
>  	if (dev->device.numa_node < 0) {
>  		RTE_LOG(WARNING, EAL, "  Invalid NUMA socket, default to 0\n");
>  		dev->device.numa_node = 0;
> @@ -380,11 +371,8 @@ rte_pci_probe(void)
>  		probed++;
>  
>  		devargs = dev->device.devargs;
> -		/* probe all or only whitelisted devices */
> -		if (probe_all)
> -			ret = pci_probe_all_drivers(dev);
> -		else if (devargs != NULL &&
> -			devargs->policy == RTE_DEV_WHITELISTED)
> +		/* probe all or only declared devices */
> +		if (probe_all ^ (devargs != NULL))

What is the intent of this?  If probe_all is true, and devargs != null,
I think this branch isn't taken.

Shouldn't this be ||?  Maybe I missed something?

>  			ret = pci_probe_all_drivers(dev);
>  		if (ret < 0) {
>  			RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 573869a..47416a5 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -22,7 +22,6 @@ DPDK_2.0 {
>  	rte_eal_alarm_set;
>  	rte_eal_devargs_add;
>  	rte_eal_devargs_dump;
> -	rte_eal_devargs_type_count;
>  	rte_eal_get_configuration;
>  	rte_eal_get_lcore_state;
>  	rte_eal_get_physmem_layout;
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index e371456..2fddbfa 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -153,7 +153,7 @@ rte_eal_devargs_insert(struct rte_devargs *da)
>  
>  /* store a whitelist parameter for later parsing */
>  int
> -rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> +rte_eal_devargs_add(const char *devargs_str)
>  {
>  	struct rte_devargs *devargs = NULL;
>  	const char *dev = devargs_str;
> @@ -165,9 +165,6 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>  
>  	if (rte_eal_devargs_parse(dev, devargs))
>  		goto fail;
> -	devargs->type = devtype;
> -	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
> -		devargs->policy = RTE_DEV_BLACKLISTED;
>  	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
>  	return 0;
>  
> @@ -198,21 +195,6 @@ rte_eal_devargs_remove(const char *busname, const char *devname)
>  	return 1;
>  }
>  
> -/* count the number of devices of a specified type */
> -unsigned int
> -rte_eal_devargs_type_count(enum rte_devtype devtype)
> -{
> -	struct rte_devargs *devargs;
> -	unsigned int count = 0;
> -
> -	TAILQ_FOREACH(devargs, &devargs_list, next) {
> -		if (devargs->type != devtype)
> -			continue;
> -		count++;
> -	}
> -	return count;
> -}
> -
>  /* dump the user devices on the console */
>  void
>  rte_eal_devargs_dump(FILE *f)
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index d57cb5d..603df27 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -131,7 +131,6 @@ TAILQ_HEAD(device_option_list, device_option);
>  struct device_option {
>  	TAILQ_ENTRY(device_option) next;
>  
> -	enum rte_devtype type;
>  	char arg[];
>  };
>  
> @@ -143,8 +142,7 @@ static int mem_parsed;
>  static int core_parsed;
>  
>  static int
> -eal_option_device_add(enum rte_devtype type,
> -		      const char *busname, const char *optarg)
> +eal_option_device_add(const char *busname, const char *optarg)
>  {
>  	struct device_option *devopt;
>  	size_t optlen;
> @@ -159,7 +157,6 @@ eal_option_device_add(enum rte_devtype type,
>  		return -ENOMEM;
>  	}
>  
> -	devopt->type = type;
>  	if (busname != NULL)
>  		ret = snprintf(devopt->arg, optlen, "%s:%s",
>  			       busname, optarg);
> @@ -183,7 +180,7 @@ eal_option_device_parse(void)
>  
>  	TAILQ_FOREACH_SAFE(devopt, &devopt_list, next, tmp) {
>  		if (ret == 0) {
> -			ret = rte_eal_devargs_add(devopt->type, devopt->arg);
> +			ret = rte_eal_devargs_add(devopt->arg);
>  			if (ret)
>  				RTE_LOG(ERR, EAL, "Unable to parse device '%s'\n",
>  					devopt->arg);
> @@ -1009,19 +1006,15 @@ eal_parse_common_option(int opt, const char *optarg,
>  	case 'b':
>  		if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_BLACKLIST) < 0)
>  			return -1;
> -		if (eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI,
> -				"pci", optarg) < 0) {
> +		if (eal_option_device_add("pci", optarg) < 0)
>  			return -1;
> -		}
>  		break;
>  	/* whitelist */
>  	case 'w':
>  		if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_WHITELIST) < 0)
>  			return -1;
> -		if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI,
> -				"pci", optarg) < 0) {
> +		if (eal_option_device_add("pci", optarg) < 0)
>  			return -1;
> -		}
>  		break;
>  	/* coremask */
>  	case 'c':
> @@ -1128,10 +1121,8 @@ eal_parse_common_option(int opt, const char *optarg,
>  		break;
>  
>  	case OPT_VDEV_NUM:
> -		if (eal_option_device_add(RTE_DEVTYPE_VIRTUAL,
> -				"vdev", optarg) < 0) {
> +		if (eal_option_device_add("vdev", optarg) < 0)
>  			return -1;
> -		}
>  		break;
>  
>  	case OPT_SYSLOG_NUM:
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index 4c4ac7e..5f090ed 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -127,14 +127,6 @@ enum rte_kernel_driver {
>  };
>  
>  /**
> - * Device policies.
> - */
> -enum rte_dev_policy {
> -	RTE_DEV_WHITELISTED,
> -	RTE_DEV_BLACKLISTED,
> -};
> -
> -/**
>   * A generic memory resource representation.
>   */
>  struct rte_mem_resource {
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index 58d585d..e50c166 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -53,15 +53,6 @@ extern "C" {
>  #include <rte_bus.h>
>  
>  /**
> - * Type of generic device
> - */
> -enum rte_devtype {
> -	RTE_DEVTYPE_WHITELISTED_PCI,
> -	RTE_DEVTYPE_BLACKLISTED_PCI,
> -	RTE_DEVTYPE_VIRTUAL,
> -};
> -
> -/**
>   * Structure that stores a device given by the user with its arguments
>   *
>   * A user device is a physical or a virtual device given by the user to
> @@ -74,10 +65,6 @@ enum rte_devtype {
>  struct rte_devargs {
>  	/** Next in list. */
>  	TAILQ_ENTRY(rte_devargs) next;
> -	/** Type of device. */
> -	enum rte_devtype type;
> -	/** Device policy. */
> -	enum rte_dev_policy policy;
>  	/** Bus handle for the device. */
>  	struct rte_bus *bus;
>  	/** Name of the device. */
> @@ -166,8 +153,6 @@ rte_eal_devargs_insert(struct rte_devargs *da);
>   * driver name is not checked by this function, it is done when probing
>   * the drivers.
>   *
> - * @param devtype
> - *   The type of the device.
>   * @param devargs_str
>   *   The arguments as given by the user.
>   *
> @@ -175,7 +160,7 @@ rte_eal_devargs_insert(struct rte_devargs *da);
>   *   - 0 on success
>   *   - A negative value on error
>   */
> -int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
> +int rte_eal_devargs_add(const char *devargs_str);
>  
>  /**
>   * Remove a device from the user device list.
> @@ -196,18 +181,6 @@ int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
>  int rte_eal_devargs_remove(const char *busname, const char *devname);
>  
>  /**
> - * Count the number of user devices of a specified type
> - *
> - * @param devtype
> - *   The type of the devices to counted.
> - *
> - * @return
> - *   The number of devices.
> - */
> -unsigned int
> -rte_eal_devargs_type_count(enum rte_devtype devtype);
> -
> -/**
>   * This function dumps the list of user device and their arguments.
>   *
>   * @param f
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index a2709e3..e1e2a50 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -22,7 +22,6 @@ DPDK_2.0 {
>  	rte_eal_alarm_set;
>  	rte_eal_devargs_add;
>  	rte_eal_devargs_dump;
> -	rte_eal_devargs_type_count;
>  	rte_eal_get_configuration;
>  	rte_eal_get_lcore_state;
>  	rte_eal_get_physmem_layout;
  

Hello Aaron,

On Tue, Oct 17, 2017 at 02:16:59PM -0400, Aaron Conole wrote:
> Gaetan Rivet <gaetan.rivet@6wind.com> writes:
> 
> > The devtype is now entirely defined by the device bus. As such, it is
> > already characterized by the bus identifier within an rte_devargs.
> >
> > The rte_devtype enum can disappear, along with crutches added during
> > this transition.
> >
> > rte_eal_devargs_type_count becomes useless and is removed.
> >
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
< ... >
> > @@ -380,11 +371,8 @@ rte_pci_probe(void)
> >  		probed++;
> >  
> >  		devargs = dev->device.devargs;
> > -		/* probe all or only whitelisted devices */
> > -		if (probe_all)
> > -			ret = pci_probe_all_drivers(dev);
> > -		else if (devargs != NULL &&
> > -			devargs->policy == RTE_DEV_WHITELISTED)
> > +		/* probe all or only declared devices */
> > +		if (probe_all ^ (devargs != NULL))
> 
> What is the intent of this?  If probe_all is true, and devargs != null,
> I think this branch isn't taken.
> 
> Shouldn't this be ||?  Maybe I missed something?
> 

Here are the possible choices:

probe_all \ devargs        !NULL                  NULL
--------------------------+----------------------+----------------------------+
true                      |blacklist and the     |blacklist mode and no       |
                          |devargs describes a   |devargs, meaning the device |
                          |blacklisted device    |is not blacklisted          |
                          |--> do not probe      |--> do probe                |
--------------------------+----------------------+----------------------------+
false                     |whitelist mode and    |whitelist mode and no       |
                          |whitelisted device    |devargs, device has been    |
                          |using a devargs       |scanned but not whitelisted |
                          |--> do probe          |--> do not probe            |
--------------------------+----------------------+----------------------------+

A XOR is thus the logical expression of this decision.
What I could be doubtful about here is that using a xor anywhere can be
confusing for some people and it could all be expressed in two simpler
ifs.

Also, probe_all could be renamed as "blacklist_mode" for example to be
more descriptive.

But unless I'm mistaken, the logic should be correct.

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

< ... >