[dpdk-dev] [PATCH v2] Send updated port_id in vdev_init functions

[dpdk-dev] [PATCH v2] Send updated port_id in vdev_init functions

[Ravi Kerur]

Instead of executing following functions before and after vdev_init
   > rte_eth_dev_save and
   > rte_eth_dev_get_changed_port

update following functions to return allocated port_id.
   > rte_eal_vdev_init
   > rte_eal_probe_one

Thanks to Tetsuya for his valuable inputs.

Ravi Kerur (1):
  Change rte_eal_vdev_init to update port_id

 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(-)

-- 
1.9.1

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

[Ravi Kerur]

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);
+		}
+
 	}
 
 	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
+ */
+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
+ */
+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;
+
+} DPDK_2.1;
-- 
1.9.1

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

[Tetsuya Mukawa]

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;

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

[Ravi Kerur]

On Wed, Aug 19, 2015 at 7:07 PM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:

> 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.'
>

Hi Tetsuya,

I would like to take a step back and explain why function declarations are
in rte_dev.h and not in rte_ethdev.h

Approach 1:
Initially I thought of modifying driver init routine to return/update
port_id as the init routine is the place port_id gets allocated and it
would have been clean approach. However, it required changes to all
PMD_VDEV driver init routine to modify function signature for the changes
which I thought may be an overkill.

Approach 2:
Instead I chose to define 2 functions in librte_ether/rte_ethdev.c and make
use of it. In this approach new functions are invoked from
librte_eal/common/.c to get port_id. If I had new function declarations in
rte_ethdev.h and included that file in librte_eal/common/.c files it
creates circular dependancy and compilation fails, hence I took hybrid
approach of definitions in librte_ether and declarations in librte_eal.

Please let me know if there is a better approach to take care of your
comments. As it stands declarations cannot be moved to rte_ethdev.h for
compilation reasons.

Thanks,
Ravi


>
> 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;
>
>

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

[Tetsuya Mukawa]

On 2015/08/21 4:16, Ravi Kerur wrote:
>
>     >  /**
>     >   * 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.'
>
>
> Hi Tetsuya,
>
> I would like to take a step back and explain why function declarations
> are in rte_dev.h and not in rte_ethdev.h
>
> Approach 1:
> Initially I thought of modifying driver init routine to return/update
> port_id as the init routine is the place port_id gets allocated and it
> would have been clean approach. However, it required changes to all
> PMD_VDEV driver init routine to modify function signature for the
> changes which I thought may be an overkill.
>
> Approach 2:
> Instead I chose to define 2 functions in librte_ether/rte_ethdev.c and
> make use of it. In this approach new functions are invoked from
> librte_eal/common/.c to get port_id. If I had new function
> declarations in rte_ethdev.h and included that file in
> librte_eal/common/.c files it creates circular dependancy and
> compilation fails, hence I took hybrid approach of definitions in
> librte_ether and declarations in librte_eal. 
>
> Please let me know if there is a better approach to take care of your
> comments. As it stands declarations cannot be moved to rte_ethdev.h
> for compilation reasons.
>
> Thanks,
> Ravi
>

Hi Ravi,
(Adding David)

I appreciate your description. I understand why you define the functions
in rte_dev.h.

About Approach2, I don't know a way to implement cleanly.
I guess if we define the functions in rte_dev.h, the developers who want
to use the functions will be confused because the functions are
implemented in ethdev.c, but it is needed to include rte_dev.h.

To avoid such a confusion, following implementation might be worked, but
I am not sure this cording style is allowed in eal library.

----------------------------
Define the functions in rte_ethdev.h, then fix librte_eal/common/.c
files like below

ex) lib/librte_eal/common/eal_common_dev.c
----------------------------
+#include <rte_pci.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>

 #include "eal_private.h"

+extern int rte_eth_dev_get_port_by_name(const char *name, uint8_t
*port_id);
+extern int rte_eth_dev_get_port_by_addr(const struct rte_pci_addr
*addr, uint8_t *port_id);
----------------------------

In this case, the developer might be able to notice that above usage in
eal library is some kind of exception. But I guess the DPDK code won't
be clean if we start having a exception.
So it might be good to choose Approach1, because apparently it is
straight forward.
Anyone won't be confused and complained about coding style.


Hi David,

Could you please let us know what you think?
Do you have a good approach for this?

Thanks,
Tetsuya

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

[Ravi Kerur]

Hi Thomas, David

Let us know how you want us to fix this? To fix rte_eal_vdev_init and
rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
mentioned in earlier discussion. In addition to those we have another
approach with changes isolated only to rte_ether component. I am attaching
diffs (preliminary) with this email. Please let us know your inputs since
it involves EAL component.

Thanks,
Ravi


On Thu, Aug 20, 2015 at 8:33 PM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:

> On 2015/08/21 4:16, Ravi Kerur wrote:
> >
> >     >  /**
> >     >   * 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.'
> >
> >
> > Hi Tetsuya,
> >
> > I would like to take a step back and explain why function declarations
> > are in rte_dev.h and not in rte_ethdev.h
> >
> > Approach 1:
> > Initially I thought of modifying driver init routine to return/update
> > port_id as the init routine is the place port_id gets allocated and it
> > would have been clean approach. However, it required changes to all
> > PMD_VDEV driver init routine to modify function signature for the
> > changes which I thought may be an overkill.
> >
> > Approach 2:
> > Instead I chose to define 2 functions in librte_ether/rte_ethdev.c and
> > make use of it. In this approach new functions are invoked from
> > librte_eal/common/.c to get port_id. If I had new function
> > declarations in rte_ethdev.h and included that file in
> > librte_eal/common/.c files it creates circular dependancy and
> > compilation fails, hence I took hybrid approach of definitions in
> > librte_ether and declarations in librte_eal.
> >
> > Please let me know if there is a better approach to take care of your
> > comments. As it stands declarations cannot be moved to rte_ethdev.h
> > for compilation reasons.
> >
> > Thanks,
> > Ravi
> >
>
> Hi Ravi,
> (Adding David)
>
> I appreciate your description. I understand why you define the functions
> in rte_dev.h.
>
> About Approach2, I don't know a way to implement cleanly.
> I guess if we define the functions in rte_dev.h, the developers who want
> to use the functions will be confused because the functions are
> implemented in ethdev.c, but it is needed to include rte_dev.h.
>
> To avoid such a confusion, following implementation might be worked, but
> I am not sure this cording style is allowed in eal library.
>
> ----------------------------
> Define the functions in rte_ethdev.h, then fix librte_eal/common/.c
> files like below
>
> ex) lib/librte_eal/common/eal_common_dev.c
> ----------------------------
> +#include <rte_pci.h>
>  #include <rte_dev.h>
>  #include <rte_devargs.h>
>  #include <rte_debug.h>
>
>  #include "eal_private.h"
>
> +extern int rte_eth_dev_get_port_by_name(const char *name, uint8_t
> *port_id);
> +extern int rte_eth_dev_get_port_by_addr(const struct rte_pci_addr
> *addr, uint8_t *port_id);
> ----------------------------
>
> In this case, the developer might be able to notice that above usage in
> eal library is some kind of exception. But I guess the DPDK code won't
> be clean if we start having a exception.
> So it might be good to choose Approach1, because apparently it is
> straight forward.
> Anyone won't be confused and complained about coding style.
>
>
> Hi David,
>
> Could you please let us know what you think?
> Do you have a good approach for this?
>
> Thanks,
> Tetsuya
>
>

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

[David Marchand]

Hello Ravi, Tetsuya,

On Tue, Aug 25, 2015 at 7:59 PM, Ravi Kerur <rkerur@gmail.com> wrote:

> Let us know how you want us to fix this? To fix rte_eal_vdev_init and
> rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
> mentioned in earlier discussion. In addition to those we have another
> approach with changes isolated only to rte_ether component. I am attaching
> diffs (preliminary) with this email. Please let us know your inputs since
> it involves EAL component.
>

- This patch looks like a good ethdev cleanup (even if it really lacks some
context / commit log).

I wonder just why you only take the first part of the name in
rte_eth_dev_get_port_by_name().
Would not this match, let's say, both toto and toto0 vdevs ?
Is this intended ?


- In the end, with this patch, do we still need to update eal ?
Looking at the code, I am not sure anymore.


-- 
David Marchand

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

[Ravi Kerur]

Hi David,


On Thu, Sep 3, 2015 at 7:04 AM, David Marchand <david.marchand@6wind.com>
wrote:

> Hello Ravi, Tetsuya,
>
> On Tue, Aug 25, 2015 at 7:59 PM, Ravi Kerur <rkerur@gmail.com> wrote:
>
>> Let us know how you want us to fix this? To fix rte_eal_vdev_init and
>> rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
>> mentioned in earlier discussion. In addition to those we have another
>> approach with changes isolated only to rte_ether component. I am attaching
>> diffs (preliminary) with this email. Please let us know your inputs since
>> it involves EAL component.
>>
>
> - This patch looks like a good ethdev cleanup (even if it really lacks
> some context / commit log).
>
> I wonder just why you only take the first part of the name in
> rte_eth_dev_get_port_by_name().
> Would not this match, let's say, both toto and toto0 vdevs ?
> Is this intended ?
>
> It was not intended, i will look into it.

>
> - In the end, with this patch, do we still need to update eal ?
> Looking at the code, I am not sure anymore.
>

Approach 3 (preliminary diffs sent as an attachment) doesn't involve EAL
but the other two solutions do. So please let us know which one you prefer.
I will send updated patch.

Thanks,
Ravi


>
>
>
> --
> David Marchand
>

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

[Ravi Kerur]

Hi David, Tetsuya,

I have sent V3 (changes isolated to rte_ether component) for formal review.
Please look into it and let me know your inputs.

@David: I looked at "rte_eth_dev_get_port_by_name()", this function is
similar to "rte_eth_dev_get_name_by_port" and I have used same logic. Let
me know if this not correct I can fix both.

Thanks,
Ravi


On Tue, Sep 15, 2015 at 4:28 AM, Ravi Kerur <rkerur@gmail.com> wrote:

> Hi David,
>
>
> On Thu, Sep 3, 2015 at 7:04 AM, David Marchand <david.marchand@6wind.com>
> wrote:
>
>> Hello Ravi, Tetsuya,
>>
>> On Tue, Aug 25, 2015 at 7:59 PM, Ravi Kerur <rkerur@gmail.com> wrote:
>>
>>> Let us know how you want us to fix this? To fix rte_eal_vdev_init and
>>> rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
>>> mentioned in earlier discussion. In addition to those we have another
>>> approach with changes isolated only to rte_ether component. I am attaching
>>> diffs (preliminary) with this email. Please let us know your inputs since
>>> it involves EAL component.
>>>
>>
>> - This patch looks like a good ethdev cleanup (even if it really lacks
>> some context / commit log).
>>
>> I wonder just why you only take the first part of the name in
>> rte_eth_dev_get_port_by_name().
>> Would not this match, let's say, both toto and toto0 vdevs ?
>> Is this intended ?
>>
>> It was not intended, i will look into it.
>
>>
>> - In the end, with this patch, do we still need to update eal ?
>> Looking at the code, I am not sure anymore.
>>
>
> Approach 3 (preliminary diffs sent as an attachment) doesn't involve EAL
> but the other two solutions do. So please let us know which one you prefer.
> I will send updated patch.
>
> Thanks,
> Ravi
>
>
>>
>>
>>
>> --
>> David Marchand
>>
>
>

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

[Tetsuya Mukawa]

Hi Ravi,

I am out of office now, and will be back 29th.
After backing, I will check it and send reply.

Tetsuya

On 2015/09/24 6:22, Ravi Kerur wrote:
> Hi David, Tetsuya,
>
> I have sent V3 (changes isolated to rte_ether component) for formal review.
> Please look into it and let me know your inputs.
>
> @David: I looked at "rte_eth_dev_get_port_by_name()", this function is
> similar to "rte_eth_dev_get_name_by_port" and I have used same logic. Let
> me know if this not correct I can fix both.
>
> Thanks,
> Ravi
>
>
> On Tue, Sep 15, 2015 at 4:28 AM, Ravi Kerur <rkerur@gmail.com> wrote:
>
>> Hi David,
>>
>>
>> On Thu, Sep 3, 2015 at 7:04 AM, David Marchand <david.marchand@6wind.com>
>> wrote:
>>
>>> Hello Ravi, Tetsuya,
>>>
>>> On Tue, Aug 25, 2015 at 7:59 PM, Ravi Kerur <rkerur@gmail.com> wrote:
>>>
>>>> Let us know how you want us to fix this? To fix rte_eal_vdev_init and
>>>> rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
>>>> mentioned in earlier discussion. In addition to those we have another
>>>> approach with changes isolated only to rte_ether component. I am attaching
>>>> diffs (preliminary) with this email. Please let us know your inputs since
>>>> it involves EAL component.
>>>>
>>> - This patch looks like a good ethdev cleanup (even if it really lacks
>>> some context / commit log).
>>>
>>> I wonder just why you only take the first part of the name in
>>> rte_eth_dev_get_port_by_name().
>>> Would not this match, let's say, both toto and toto0 vdevs ?
>>> Is this intended ?
>>>
>>> It was not intended, i will look into it.
>>> - In the end, with this patch, do we still need to update eal ?
>>> Looking at the code, I am not sure anymore.
>>>
>> Approach 3 (preliminary diffs sent as an attachment) doesn't involve EAL
>> but the other two solutions do. So please let us know which one you prefer.
>> I will send updated patch.
>>
>> Thanks,
>> Ravi
>>
>>
>>>
>>>
>>> --
>>> David Marchand
>>>
>>

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

[Tetsuya Mukawa]

On 2015/09/24 6:22, Ravi Kerur wrote:
> Hi David, Tetsuya,
>
> I have sent V3 (changes isolated to rte_ether component) for formal review.
> Please look into it and let me know your inputs.

Hi Ravi,

I've checked the patch.
I guess your patch is good.

>
> @David: I looked at "rte_eth_dev_get_port_by_name()", this function is
> similar to "rte_eth_dev_get_name_by_port" and I have used same logic. Let
> me know if this not correct I can fix both.

Do you comment about rte_eth_dev_get_port_by_name and
rte_eth_dev_get_port_by_addr?
If so, I guess we don't need to merge.

> Thanks,
> Ravi
>
>
> On Tue, Sep 15, 2015 at 4:28 AM, Ravi Kerur <rkerur@gmail.com> wrote:
>
>> Hi David,
>>
>>
>> On Thu, Sep 3, 2015 at 7:04 AM, David Marchand <david.marchand@6wind.com>
>> wrote:
>>
>>> Hello Ravi, Tetsuya,
>>>
>>> On Tue, Aug 25, 2015 at 7:59 PM, Ravi Kerur <rkerur@gmail.com> wrote:
>>>
>>>> Let us know how you want us to fix this? To fix rte_eal_vdev_init and
>>>> rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
>>>> mentioned in earlier discussion. In addition to those we have another
>>>> approach with changes isolated only to rte_ether component. I am attaching
>>>> diffs (preliminary) with this email. Please let us know your inputs since
>>>> it involves EAL component.
>>>>
>>> - This patch looks like a good ethdev cleanup (even if it really lacks
>>> some context / commit log).
>>>
>>> I wonder just why you only take the first part of the name in
>>> rte_eth_dev_get_port_by_name().
>>> Would not this match, let's say, both toto and toto0 vdevs ?
>>> Is this intended ?
>>>
>>> It was not intended, i will look into it.
>>> - In the end, with this patch, do we still need to update eal ?
>>> Looking at the code, I am not sure anymore.
>>>
>> Approach 3 (preliminary diffs sent as an attachment) doesn't involve EAL
>> but the other two solutions do. So please let us know which one you prefer.
>> I will send updated patch.
>>
>> Thanks,
>> Ravi
>>
>>
>>>
>>>
>>> --
>>> David Marchand
>>>
>>

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

[Ravi Kerur]

Hi Tetsuya,


On Mon, Sep 28, 2015 at 8:32 PM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:

> On 2015/09/24 6:22, Ravi Kerur wrote:
> > Hi David, Tetsuya,
> >
> > I have sent V3 (changes isolated to rte_ether component) for formal
> review.
> > Please look into it and let me know your inputs.
>
> Hi Ravi,
>
> I've checked the patch.
> I guess your patch is good.
>
> >
> > @David: I looked at "rte_eth_dev_get_port_by_name()", this function is
> > similar to "rte_eth_dev_get_name_by_port" and I have used same logic. Let
> > me know if this not correct I can fix both.
>
> Do you comment about rte_eth_dev_get_port_by_name and
> rte_eth_dev_get_port_by_addr?
> If so, I guess we don't need to merge.
>
>
I just mentioned that new functions are using same logic as existing
function.

Thanks,
Ravi


> > Thanks,
> > Ravi
> >
> >
> > On Tue, Sep 15, 2015 at 4:28 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> >
> >> Hi David,
> >>
> >>
> >> On Thu, Sep 3, 2015 at 7:04 AM, David Marchand <
> david.marchand@6wind.com>
> >> wrote:
> >>
> >>> Hello Ravi, Tetsuya,
> >>>
> >>> On Tue, Aug 25, 2015 at 7:59 PM, Ravi Kerur <rkerur@gmail.com> wrote:
> >>>
> >>>> Let us know how you want us to fix this? To fix rte_eal_vdev_init and
> >>>> rte_eal_pci_probe_one to return allocated port_id we had 2 approaches
> >>>> mentioned in earlier discussion. In addition to those we have another
> >>>> approach with changes isolated only to rte_ether component. I am
> attaching
> >>>> diffs (preliminary) with this email. Please let us know your inputs
> since
> >>>> it involves EAL component.
> >>>>
> >>> - This patch looks like a good ethdev cleanup (even if it really lacks
> >>> some context / commit log).
> >>>
> >>> I wonder just why you only take the first part of the name in
> >>> rte_eth_dev_get_port_by_name().
> >>> Would not this match, let's say, both toto and toto0 vdevs ?
> >>> Is this intended ?
> >>>
> >>> It was not intended, i will look into it.
> >>> - In the end, with this patch, do we still need to update eal ?
> >>> Looking at the code, I am not sure anymore.
> >>>
> >> Approach 3 (preliminary diffs sent as an attachment) doesn't involve EAL
> >> but the other two solutions do. So please let us know which one you
> prefer.
> >> I will send updated patch.
> >>
> >> Thanks,
> >> Ravi
> >>
> >>
> >>>
> >>>
> >>> --
> >>> David Marchand
> >>>
> >>
>
>