Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/08/20 4:42, Ravi Kerur wrote:
> v2:
>    > Remove rte_pmd_mpipe_devinit changes
>    > Use rte_eal_compare_pci_addr for address comparison
>    > Use dpdk_2.2 in version map file for new functions
>
> v1:
> Changes include
>    > Modify rte_eal_vdev_init to return allocated port_id
>    > Modify rte_eal_probe_one to return allocated port_id
>
> 2. Removed following functions
>    > rte_eth_dev_save and
>    > rte_eth_dev_get_changed_port
>
> 3. Added 2 new functions
>    > rte_eth_dev_get_port_by_name
>    > rte_eth_dev_get_port_by_addr
>
> 4. Fix return error(ENOMEM) in function rte_pmd_mpipe_devinit
>
> Compiled on Linux for following targets
>    > x86_64-native-linuxapp-gcc
>    > x86_64-native-linuxapp-clang
>    > x86_x32-native-linuxapp-gcc
>
> Compiled on FreeBSD for following targets
>    > x86_64-native-bsdapp-clang
>    > x86_64-native-bsdapp-gcc
>
> Tested on Linux/FreeBSD:
>    > port attach eth_null
>    > port start all
>    > port stop all
>    > port close all
>    > port detach 0
>    > port attach eth_null
>    > port start all
>    > port stop all
>    > port close all
>    > port detach 0
>
> Successful run of checkpatch.pl on the diffs
>
> Successful validate_abi on Linux for following targets
>
>    > x86_64-native-linuxapp-gcc
>    > x86_64-native-linuxapp-clang
>
> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> ---
>  drivers/net/enic/enic_ethdev.c          |   2 +-
>  lib/librte_eal/common/eal_common_dev.c  |  13 ++--
>  lib/librte_eal/common/eal_common_pci.c  |   6 +-
>  lib/librte_eal/common/include/rte_dev.h |  36 +++++++++-
>  lib/librte_eal/common/include/rte_pci.h |   4 +-
>  lib/librte_ether/rte_ethdev.c           | 122 +++++++++++++++++---------------
>  lib/librte_ether/rte_ether_version.map  |   8 +++
>  7 files changed, 125 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index 8280cea..472ef5a 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -36,8 +36,8 @@
>  #include <stdio.h>
>  #include <stdint.h>
>  
> -#include <rte_dev.h>
>  #include <rte_pci.h>
> +#include <rte_dev.h>
>  #include <rte_ethdev.h>
>  #include <rte_string_fns.h>
>  
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 4089d66..ffdb3b5 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -37,6 +37,7 @@
>  #include <inttypes.h>
>  #include <sys/queue.h>
>  
> +#include <rte_pci.h>
>  #include <rte_dev.h>
>  #include <rte_devargs.h>
>  #include <rte_debug.h>
> @@ -64,7 +65,7 @@ rte_eal_driver_unregister(struct rte_driver *driver)
>  }
>  
>  int
> -rte_eal_vdev_init(const char *name, const char *args)
> +rte_eal_vdev_init(const char *name, const char *args, uint8_t *port_id)
>  {
>  	struct rte_driver *driver;
>  
> @@ -81,8 +82,12 @@ rte_eal_vdev_init(const char *name, const char *args)
>  		 * will be "eth_pcap", but "name" will be "eth_pcapN".
>  		 * So use strncmp to compare.
>  		 */
> -		if (!strncmp(driver->name, name, strlen(driver->name)))
> -			return driver->init(name, args);
> +		if (!strncmp(driver->name, name, strlen(driver->name))) {
> +			if (!driver->init(name, args))
> +				return rte_eth_dev_get_port_by_name(
> +						name, port_id);
> +		}
> +

Please remove needless line.

>  	}
>  
>  	RTE_LOG(ERR, EAL, "no driver found for %s\n", name);
> @@ -108,7 +113,7 @@ rte_eal_dev_init(void)
>  			continue;
>  
>  		if (rte_eal_vdev_init(devargs->virtual.drv_name,
> -					devargs->args)) {
> +					devargs->args, NULL)) {
>  			RTE_LOG(ERR, EAL, "failed to initialize %s device\n",
>  					devargs->virtual.drv_name);
>  			return -1;
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index 16e8629..3d97892 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -79,6 +79,7 @@
>  #include <rte_string_fns.h>
>  #include <rte_common.h>
>  #include <rte_devargs.h>
> +#include <rte_dev.h>
>  
>  #include "eal_private.h"
>  
> @@ -322,7 +323,7 @@ pci_detach_all_drivers(struct rte_pci_device *dev)
>   * the driver of the devive.
>   */
>  int
> -rte_eal_pci_probe_one(const struct rte_pci_addr *addr)
> +rte_eal_pci_probe_one(const struct rte_pci_addr *addr, uint8_t *port_id)
>  {
>  	struct rte_pci_device *dev = NULL;
>  	int ret = 0;
> @@ -337,7 +338,8 @@ rte_eal_pci_probe_one(const struct rte_pci_addr *addr)
>  		ret = pci_probe_all_drivers(dev);
>  		if (ret < 0)
>  			goto err_return;
> -		return 0;
> +
> +		return rte_eth_dev_get_port_by_addr(addr, port_id);
>  	}
>  	return -1;
>  
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index f601d21..564cdf3 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -110,10 +110,12 @@ int rte_eal_dev_init(void);
>   *   The pointer to a driver name to be initialized.
>   * @param args
>   *   The pointer to arguments used by driver initialization.
> + * @param port_id
> + *   The port_id associated with the driver after initialization.
>   * @return
>   *  0 on success, negative on error
>   */
> -int rte_eal_vdev_init(const char *name, const char *args);
> +int rte_eal_vdev_init(const char *name, const char *args, uint8_t *port_id);
>  
>  /**
>   * Uninitalize a driver specified by name.
> @@ -125,6 +127,38 @@ int rte_eal_vdev_init(const char *name, const char *args);
>   */
>  int rte_eal_vdev_uninit(const char *name);
>  
> +/**
> + * Given name, return port_id associated with the device.
> + *
> + * @param name
> + *   Name associated with device.
> + * @param port_id
> + *   The port identifier of the device.
> + *
> + * @return
> + *   - 0: Success.
> + *   - -EINVAL: NULL string (name)
> + *   - -ENODEV failure

Please define above in 'rte_ethdev.h.'

Also Above description uses different format.
The other description in DPDK headers is like below.
"0 on success, negative on error"
So probably it'll be nice to follow it.
At least, it will be nice to choose whether ':' is involved or not.

> + */
> +extern int
> +rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id);
> +
> +/**
> + * Given pci_addr, return port_id associated with the device.
> + *
> + * @param addr
> + *   PCI addr associated with device.
> + * @param port_id
> + *   The port identifier of the device.
> + *
> + * @return
> + *   - 0: Success.
> + *   - -EINVAL: NULL pointer
> + *   - -ENODEV failure

Same above.
(Please define it in 'rte_ethdev.h', also use same style)

> + */
> +extern int
> +rte_eth_dev_get_port_by_addr(const struct rte_pci_addr *addr, uint8_t *port_id);
> +
>  #define PMD_REGISTER_DRIVER(d)\
>  void devinitfn_ ##d(void);\
>  void __attribute__((constructor, used)) devinitfn_ ##d(void)\
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 3fb2d3a..91ad23f 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -406,11 +406,13 @@ void pci_unmap_resource(void *requested_addr, size_t size);
>   *
>   * @param addr
>   *	The PCI Bus-Device-Function address to probe.
> + * @param port_id
> + *	Port-id associated with PCI device address
>   * @return
>   *   - 0 on success.
>   *   - Negative on error.
>   */
> -int rte_eal_pci_probe_one(const struct rte_pci_addr *addr);
> +int rte_eal_pci_probe_one(const struct rte_pci_addr *addr, uint8_t *port_id);
>  
>  /**
>   * Close the single PCI device.
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 6b2400c..1b0c6fa 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -442,32 +442,6 @@ rte_eth_dev_get_device_type(uint8_t port_id)
>  }
>  
>  static int
> -rte_eth_dev_save(struct rte_eth_dev *devs, size_t size)
> -{
> -	if ((devs == NULL) ||
> -	    (size != sizeof(struct rte_eth_dev) * RTE_MAX_ETHPORTS))
> -		return -EINVAL;
> -
> -	/* save current rte_eth_devices */
> -	memcpy(devs, rte_eth_devices, size);
> -	return 0;
> -}
> -
> -static int
> -rte_eth_dev_get_changed_port(struct rte_eth_dev *devs, uint8_t *port_id)
> -{
> -	if ((devs == NULL) || (port_id == NULL))
> -		return -EINVAL;
> -
> -	/* check which port was attached or detached */
> -	for (*port_id = 0; *port_id < RTE_MAX_ETHPORTS; (*port_id)++, devs++) {
> -		if (rte_eth_devices[*port_id].attached ^ devs->attached)
> -			return 0;
> -	}
> -	return -ENODEV;
> -}
> -
> -static int
>  rte_eth_dev_get_addr_by_port(uint8_t port_id, struct rte_pci_addr *addr)
>  {
>  	VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> @@ -530,30 +504,16 @@ rte_eth_dev_is_detachable(uint8_t port_id)
>  static int
>  rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
>  {
> -	uint8_t new_port_id;
> -	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> -
>  	if ((addr == NULL) || (port_id == NULL))
>  		goto err;
>  
> -	/* save current port status */
> -	if (rte_eth_dev_save(devs, sizeof(devs)))
> -		goto err;
>  	/* re-construct pci_device_list */
>  	if (rte_eal_pci_scan())
>  		goto err;
> -	/* invoke probe func of the driver can handle the new device.
> -	 * TODO:
> -	 * rte_eal_pci_probe_one() should return port_id.
> -	 * And rte_eth_dev_save() and rte_eth_dev_get_changed_port()
> -	 * should be removed. */
> -	if (rte_eal_pci_probe_one(addr))
> -		goto err;
> -	/* get port_id enabled by above procedures */
> -	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> +	/* Invoke probe func of the driver can handle the new device. */
> +	if (rte_eal_pci_probe_one(addr, port_id))
>  		goto err;
>  
> -	*port_id = new_port_id;
>  	return 0;
>  err:
>  	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> @@ -600,8 +560,6 @@ static int
>  rte_eth_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
>  {
>  	char *name = NULL, *args = NULL;
> -	uint8_t new_port_id;
> -	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
>  	int ret = -1;
>  
>  	if ((vdevargs == NULL) || (port_id == NULL))
> @@ -611,22 +569,15 @@ rte_eth_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
>  	if (rte_eal_parse_devargs_str(vdevargs, &name, &args))
>  		goto end;
>  
> -	/* save current port status */
> -	if (rte_eth_dev_save(devs, sizeof(devs)))
> -		goto end;
>  	/* walk around dev_driver_list to find the driver of the device,
> -	 * then invoke probe function o the driver.
> -	 * TODO:
> -	 * rte_eal_vdev_init() should return port_id,
> -	 * And rte_eth_dev_save() and rte_eth_dev_get_changed_port()
> -	 * should be removed. */
> -	if (rte_eal_vdev_init(name, args))
> -		goto end;
> -	/* get port_id enabled by above procedures */
> -	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> +	 * then invoke probe function of the driver.
> +	 * rte_eal_vdev_init() updates port_id allocated after
> +	 * initialization.
> +	 */
> +	if (rte_eal_vdev_init(name, args, port_id))
>  		goto end;
> +
>  	ret = 0;
> -	*port_id = new_port_id;
>  end:
>  	if (name)
>  		free(name);
> @@ -3610,3 +3561,60 @@ rte_eth_dev_set_eeprom(uint8_t port_id, struct rte_dev_eeprom_info *info)
>  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_eeprom, -ENOTSUP);
>  	return (*dev->dev_ops->set_eeprom)(dev, info);
>  }
> +
> +int
> +rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
> +{
> +	int i;
> +
> +	if (name == NULL) {
> +		PMD_DEBUG_TRACE("Null pointer is specified\n");
> +		return -EINVAL;
> +	}
> +
> +	if (port_id)
> +		*port_id = RTE_MAX_ETHPORTS;
> +
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +
> +		if (!strncmp(name,
> +			rte_eth_dev_data[i].name, strlen(name))) {
> +
> +			if (port_id)
> +				*port_id = i;
> +
> +			return 0;
> +		}
> +	}
> +	return -ENODEV;
> +}
> +
> +int
> +rte_eth_dev_get_port_by_addr(const struct rte_pci_addr *addr, uint8_t *port_id)
> +{
> +	int i;
> +	struct rte_pci_device *pci_dev = NULL;
> +
> +	if (addr == NULL) {
> +		PMD_DEBUG_TRACE("Null pointer is specified\n");
> +		return -EINVAL;
> +	}
> +
> +	if (port_id)
> +		*port_id = RTE_MAX_ETHPORTS;
> +
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +
> +		pci_dev = rte_eth_devices[i].pci_dev;
> +
> +		if (pci_dev &&
> +			!rte_eal_compare_pci_addr(&pci_dev->addr, addr)) {
> +
> +			if (port_id)
> +				*port_id = i;
> +
> +			return 0;
> +		}
> +	}
> +	return -ENODEV;
> +}
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index 8345a6c..9ab14da 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -127,3 +127,11 @@ DPDK_2.1 {
>  	rte_eth_timesync_read_tx_timestamp;
>  
>  } DPDK_2.0;
> +
> +DPDK_2.2 {
> +	global:
> +
> +	rte_eth_dev_get_port_by_name;
> +	rte_eth_dev_get_port_by_addr;

Please use alphabetical order.

> +
> +} DPDK_2.1;