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

[Ravi Kerur <rkerur@gmail.com>]

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