[dpdk-dev] [PATCH v2] Change rte_eal_vdev_init to update port_id
Ravi Kerur
rkerur at gmail.com
Thu Aug 20 21:16:03 CEST 2015
On Wed, Aug 19, 2015 at 7:07 PM, Tetsuya Mukawa <mukawa at 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 at 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;
>
>
More information about the dev
mailing list