From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f65.google.com (mail-oi0-f65.google.com [209.85.218.65]) by dpdk.org (Postfix) with ESMTP id 0F84C8DB2 for ; Thu, 20 Aug 2015 21:16:04 +0200 (CEST) Received: by oieu201 with SMTP id u201so2163014oie.2 for ; Thu, 20 Aug 2015 12:16:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=YLAz6g+lCTDGIswsGNtfCdDO9/9G1z62y169aTLW34M=; b=G8/GiyJ6DCrwaZMJj1JAO3FRVg5jyScLZcebcA92K9Kavix6ZPRNK6R2qlW7SHZnNw 2aMOBQFDFMUsbIltEby8N8roxWFwo1Oo8xVDwilNZYiaN1DsIjfvHEvkw3Cc9lgab6pF ggLXgLo3vemtP1+ie08nHXy92P0sjY+6iKzZ+qZm5MRN5XJsCwamwIL3q3c6GGlupgfz +0NGRvtf0AVb62rTSAlBAE08T2C0Her+V6BH9FTr29+i7lgtZPAbz1+Aw2zrW06AHnqn G/YAg+qLgs8enw53EMGEU3FMthWAxRSo9H1bQ9y8jkNt2UeKPdzyT5RxBIerj14YGRqh eVMQ== MIME-Version: 1.0 X-Received: by 10.202.228.72 with SMTP id b69mr4020994oih.96.1440098163379; Thu, 20 Aug 2015 12:16:03 -0700 (PDT) Received: by 10.202.65.197 with HTTP; Thu, 20 Aug 2015 12:16:03 -0700 (PDT) In-Reply-To: <55D53655.1040808@igel.co.jp> References: <1440013341-29659-1-git-send-email-rkerur@gmail.com> <1440013376-29715-1-git-send-email-rkerur@gmail.com> <55D53655.1040808@igel.co.jp> Date: Thu, 20 Aug 2015 12:16:03 -0700 Message-ID: From: Ravi Kerur To: Tetsuya Mukawa Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Aug 2015 19:16:04 -0000 On Wed, Aug 19, 2015 at 7:07 PM, Tetsuya Mukawa 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 > > --- > > 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 > > #include > > > > -#include > > #include > > +#include > > #include > > #include > > > > 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 > > #include > > > > +#include > > #include > > #include > > #include > > @@ -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 > > #include > > #include > > +#include > > > > #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; > >