* [dpdk-dev] [PATCH v2] Send updated port_id in vdev_init functions @ 2015-08-19 19:42 Ravi Kerur 2015-08-19 19:42 ` [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2015-08-19 19:42 UTC (permalink / raw) To: dev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-08-19 19:42 [dpdk-dev] [PATCH v2] Send updated port_id in vdev_init functions Ravi Kerur @ 2015-08-19 19:42 ` Ravi Kerur 2015-08-20 2:07 ` Tetsuya Mukawa 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2015-08-19 19:42 UTC (permalink / raw) To: dev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-08-19 19:42 ` [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id Ravi Kerur @ 2015-08-20 2:07 ` Tetsuya Mukawa 2015-08-20 19:16 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Tetsuya Mukawa @ 2015-08-20 2:07 UTC (permalink / raw) To: Ravi Kerur, dev 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; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-08-20 2:07 ` Tetsuya Mukawa @ 2015-08-20 19:16 ` Ravi Kerur 2015-08-21 3:33 ` Tetsuya Mukawa 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2015-08-20 19:16 UTC (permalink / raw) To: Tetsuya Mukawa; +Cc: dev 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; > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-08-20 19:16 ` Ravi Kerur @ 2015-08-21 3:33 ` Tetsuya Mukawa 2015-08-25 17:59 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Tetsuya Mukawa @ 2015-08-21 3:33 UTC (permalink / raw) To: Ravi Kerur, David Marchand; +Cc: dev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-08-21 3:33 ` Tetsuya Mukawa @ 2015-08-25 17:59 ` Ravi Kerur 2015-09-03 14:04 ` David Marchand 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2015-08-25 17:59 UTC (permalink / raw) To: Tetsuya Mukawa, Thomas Monjalon; +Cc: dev 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-08-25 17:59 ` Ravi Kerur @ 2015-09-03 14:04 ` David Marchand 2015-09-15 11:28 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: David Marchand @ 2015-09-03 14:04 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-09-03 14:04 ` David Marchand @ 2015-09-15 11:28 ` Ravi Kerur 2015-09-23 21:22 ` Ravi Kerur 0 siblings, 1 reply; 12+ messages in thread From: Ravi Kerur @ 2015-09-15 11:28 UTC (permalink / raw) To: David Marchand; +Cc: dev 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-09-15 11:28 ` Ravi Kerur @ 2015-09-23 21:22 ` Ravi Kerur 2015-09-26 11:35 ` Tetsuya Mukawa 2015-09-29 3:32 ` Tetsuya Mukawa 0 siblings, 2 replies; 12+ messages in thread From: Ravi Kerur @ 2015-09-23 21:22 UTC (permalink / raw) To: David Marchand; +Cc: dev 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 >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-09-23 21:22 ` Ravi Kerur @ 2015-09-26 11:35 ` Tetsuya Mukawa 2015-09-29 3:32 ` Tetsuya Mukawa 1 sibling, 0 replies; 12+ messages in thread From: Tetsuya Mukawa @ 2015-09-26 11:35 UTC (permalink / raw) To: Ravi Kerur, David Marchand; +Cc: dev 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 >>> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-09-23 21:22 ` Ravi Kerur 2015-09-26 11:35 ` Tetsuya Mukawa @ 2015-09-29 3:32 ` Tetsuya Mukawa 2015-09-30 19:14 ` Ravi Kerur 1 sibling, 1 reply; 12+ messages in thread From: Tetsuya Mukawa @ 2015-09-29 3:32 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev 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 >>> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id 2015-09-29 3:32 ` Tetsuya Mukawa @ 2015-09-30 19:14 ` Ravi Kerur 0 siblings, 0 replies; 12+ messages in thread From: Ravi Kerur @ 2015-09-30 19:14 UTC (permalink / raw) To: Tetsuya Mukawa; +Cc: dev 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 > >>> > >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-09-30 19:14 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-19 19:42 [dpdk-dev] [PATCH v2] Send updated port_id in vdev_init functions Ravi Kerur 2015-08-19 19:42 ` [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id Ravi Kerur 2015-08-20 2:07 ` Tetsuya Mukawa 2015-08-20 19:16 ` Ravi Kerur 2015-08-21 3:33 ` Tetsuya Mukawa 2015-08-25 17:59 ` Ravi Kerur 2015-09-03 14:04 ` David Marchand 2015-09-15 11:28 ` Ravi Kerur 2015-09-23 21:22 ` Ravi Kerur 2015-09-26 11:35 ` Tetsuya Mukawa 2015-09-29 3:32 ` Tetsuya Mukawa 2015-09-30 19:14 ` Ravi Kerur
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).