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

[Ravi Kerur <rkerur@gmail.com>]

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