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

[Tetsuya Mukawa <mukawa@igel.co.jp>]

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