From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tetsuya Mukawa Subject: Re: [PATCH v9 13/14] eal/pci: Add rte_eal_dev_attach/detach() functions Date: Thu, 19 Feb 2015 22:30:07 +0900 Message-ID: <54E5E55F.7020905@igel.co.jp> References: <1424060073-23484-12-git-send-email-mukawa@igel.co.jp> <1424314187-25177-1-git-send-email-mukawa@igel.co.jp> <1424314187-25177-14-git-send-email-mukawa@igel.co.jp> <10399957.vsF1Tt8fsr@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: dev-VfR2kkLFssw@public.gmane.org To: Thomas Monjalon Return-path: In-Reply-To: <10399957.vsF1Tt8fsr@xps13> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On 2015/02/19 21:10, Thomas Monjalon wrote: > 2015-02-19 11:49, Tetsuya Mukawa: >> +/* attach the new virtual device, then store port_id of the device */= >> +static int >> +rte_eal_dev_attach_vdev(const char *vdevargs, uint8_t *port_id) >> +{ >> + char *args; >> + uint8_t new_port_id; >> + struct rte_eth_dev devs[RTE_MAX_ETHPORTS]; >> + >> + if ((vdevargs =3D=3D NULL) || (port_id =3D=3D NULL)) >> + goto err0; >> + >> + args =3D strdup(vdevargs); >> + if (args =3D=3D NULL) >> + goto err0; >> + >> + /* save current port status */ >> + if (rte_eth_dev_save(devs, sizeof(devs))) >> + goto err1; >> + /* add the vdevargs to devargs_list */ >> + if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, args)) >> + goto err1; > Could you explain why you store devargs in a list? I try to do same behavior when rte_eal_init() is called. If only hotplug doesn't do this, someone may be confused when they try to realize devargs_list. >> + /* parse vdevargs, then retrieve device name */ >> + get_vdev_name(args); >> + /* walk around dev_driver_list to find the driver of the device, >> + * then invoke probe function o the driver */ >> + if (rte_eal_vdev_find_and_init(args)) > TODO: get port_id from init. Yes, I will. I also add comment about it. >> + goto err2; >> + /* get port_id enabled by above procedures */ >> + if (rte_eth_dev_get_changed_port(devs, &new_port_id)) >> + goto err2; > [...] >> --- a/lib/librte_eal/common/include/rte_dev.h >> +++ b/lib/librte_eal/common/include/rte_dev.h >> @@ -47,6 +47,7 @@ extern "C" { >> #endif >> =20 >> #include >> +#include >> =20 >> /** Double linked list of device drivers. */ >> TAILQ_HEAD(rte_driver_list, rte_driver); >> @@ -57,6 +58,11 @@ TAILQ_HEAD(rte_driver_list, rte_driver); >> typedef int (rte_dev_init_t)(const char *name, const char *args); >> =20 >> /** >> + * Uninitilization function called for each device driver once. >> + */ >> +typedef int (rte_dev_uninit_t)(const char *name); > Why using name as parameter and not port_id? This function pointer will be implemented in PMDs. For example, in pcap PMD, rte_pmd_pcap_devuninit() is the function. static struct rte_driver pmd_pcap_drv =3D { .name =3D "eth_pcap", .type =3D PMD_VDEV, .init =3D rte_pmd_pcap_devinit, .uninit =3D rte_pmd_pcap_devuninit, }; "port_id" isn't needed in PMD. > > [...] >> --- a/lib/librte_eal/linuxapp/eal/Makefile >> +++ b/lib/librte_eal/linuxapp/eal/Makefile >> @@ -45,6 +45,7 @@ CFLAGS +=3D -I$(RTE_SDK)/lib/librte_eal/common/inclu= de >> CFLAGS +=3D -I$(RTE_SDK)/lib/librte_ring >> CFLAGS +=3D -I$(RTE_SDK)/lib/librte_mempool >> CFLAGS +=3D -I$(RTE_SDK)/lib/librte_malloc >> +CFLAGS +=3D -I$(RTE_SDK)/lib/librte_mbuf > Why do you need mbuf? To include rte_ethdev.h in this code, rte_mbuf.h is also needed. > [...] >> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> @@ -21,6 +21,8 @@ DPDK_2.0 { >> rte_eal_alarm_cancel; >> rte_eal_alarm_set; >> rte_eal_dev_init; >> + rte_eal_dev_attach; >> + rte_eal_dev_detach; > Please keep alphabetical order. > Sure, I will. Thanks, Tetsuya