[dpdk-dev] [PATCH v14 12/13] eal/pci: Add rte_eal_dev_attach/detach() functions

[Tetsuya Mukawa]

On 2015/02/25 23:00, Thomas Monjalon wrote:
> 2015-02-25 21:32, Tetsuya Mukawa:
>> 2015-02-25 20:21 GMT+09:00 Thomas Monjalon <thomas.monjalon at 6wind.com>:
>>> 2015-02-25 13:04, Tetsuya Mukawa:
>>>> --- a/lib/librte_eal/common/eal_common_dev.c
>>>> +++ b/lib/librte_eal/common/eal_common_dev.c
>>>> @@ -32,10 +32,13 @@
>>>>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>>>   */
>>>>
>>>> +#include <stdio.h>
>>>> +#include <limits.h>
>>>>  #include <string.h>
>>>>  #include <inttypes.h>
>>>>  #include <sys/queue.h>
>>>>
>>>> +#include <rte_ethdev.h>
>>>>  #include <rte_dev.h>
>>>>  #include <rte_devargs.h>
>>> No, you must not include ethdev in EAL.
>>> The ethdev layer is by design on top of EAL.
>>> Maxime already asked why you did it. He was implicitly asking to remove it.
>>> You said that you are calling ethdev_is_detachable() but you should
>>> call a function eal_is_detachable() or something like that.
>>> The detachable state must be only device-related, i.e. in EAL.
>>> The ethdev API is only a wrapper (with port id) in such case.
>>>
>> Hi Thomas,
>>
>> If ethdev library is on top of EAL, hotplug functions like
>> rte_eal_dev_attach/detach should be implemented in ethdev library.
>> Is it right?
> Yes you're right.
>
>> If so, I will move rte_eal_dev_attach/detach to ethdev library.
>> And I will change names like rte_eth_dev_attach/detach.
> It seems to be the right thing to do.
>
>> Also, I will add "rte_dev.h" and "rte_pci.h" in rte_ethdev.h, and call
>> below EAL functions from ethdev library.
>>
>> - For virtual device initialization and finalization
>> -- rte_eth_vdev_init
>> -- rte_eth_vdev_uninit()
>> - For physical NIC initialization and finalization
>> -- rte_eal_pci_probe_one()
>> -- rte_eal_pci_close_one()
>>
>> I guess this will fix this design violation.
>> Is this ok?
> I think yes.
> If needed, we could do some cleanup after RC1.
> I'm just waiting for you fixing this, to avoid introducing
> a layering violation.
> Would you able to do it today?

Hi Thomas,

I appreciate for your reply.
I start trying it.

Thanks,
Tetsuya

> Thanks
>
>>>> --- a/lib/librte_eal/linuxapp/eal/Makefile
>>>> +++ b/lib/librte_eal/linuxapp/eal/Makefile
>>>> @@ -45,6 +45,7 @@ CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
>>>>  CFLAGS += -I$(RTE_SDK)/lib/librte_ring
>>>>  CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
>>>>  CFLAGS += -I$(RTE_SDK)/lib/librte_malloc
>>>> +CFLAGS += -I$(RTE_SDK)/lib/librte_mbuf
>>> By removing ethdev dependency, you can remove this ugly mbuf dependency.
>>>
>>> Thanks Tetsuya
>>>
>