From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f176.google.com (mail-pd0-f176.google.com [209.85.192.176]) by dpdk.org (Postfix) with ESMTP id A7B1CB6DB for ; Tue, 17 Feb 2015 09:51:07 +0100 (CET) Received: by pdjz10 with SMTP id z10so42371774pdj.0 for ; Tue, 17 Feb 2015 00:51:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=TLwYFeI5DVrnxdLP/uSIUnTNMkL8V1gOIF3zdKneyf0=; b=B3WCfM0SuTviWPl8iKrGV1lZ9kKA7C5GbgYboWXvcFdHFHq5+J6Tdyg7Lwx9+D/haj tBdc20VbixfBaGtjdkDbbJtFJuc/IVE450DnHGC4SOnyD4gS9JvOXcMhnpQTgEK9zcJh g1jXlekFNWfI/6CjaoD9ouXuNsof2U05jjPNEWG2ieoZ/QNAhqyjXqEG1kqbg+6phvvp UdTO5Vd0GQ02LiPmYrwFXhbcJ4s5eCRtdPn+PUPccsMpL9cDQ50yBgmUP38wR2qiT0Bk gfnlFqHElYupFjlYgdwN6atI+x+N1sjb5xDSl0r7UJg864XVxAl9Cup0cM082P3D+eIo TVKg== X-Gm-Message-State: ALoCoQlaRyc80BaSS+hb3Po/n3ILWipgVsf3snBDOITUw+TtZEyvHRwA+KOU1WrbjM1ux++XXjYg X-Received: by 10.66.66.133 with SMTP id f5mr27500294pat.20.1424163067108; Tue, 17 Feb 2015 00:51:07 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id pm2sm16821613pbb.81.2015.02.17.00.51.05 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 17 Feb 2015 00:51:06 -0800 (PST) Message-ID: <54E300F8.8000503@igel.co.jp> Date: Tue, 17 Feb 2015 17:51:04 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Thomas Monjalon References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp> <1424060073-23484-1-git-send-email-mukawa@igel.co.jp> <1424060073-23484-13-git-send-email-mukawa@igel.co.jp> <83798909.brLNWNC9tj@xps13> In-Reply-To: <83798909.brLNWNC9tj@xps13> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v8 12/14] eal/pci: Add rte_eal_dev_attach/detach() functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Feb 2015 08:51:08 -0000 On 2015/02/17 10:48, Thomas Monjalon wrote: > 2015-02-16 13:14, Tetsuya Mukawa: >> These functions are used for attaching or detaching a port. >> When rte_eal_dev_attach() is called, the function tries to realize the= >> device name as pci address. If this is done successfully, >> rte_eal_dev_attach() will attach physical device port. If not, attache= s >> virtual devive port. >> When rte_eal_dev_detach() is called, the function gets the device type= >> of this port to know whether the port is come from physical or virtual= =2E >> And then specific detaching function will be called. >> >> v8: >> - Add missing symbol in version map. >> (Thanks to Qiu, Michael and Iremonger, Bernard) >> v7: >> - Fix typo of warning messages. >> (Thanks to Qiu, Michael) >> v5: >> - Change function names like below. >> rte_eal_dev_find_and_invoke() to rte_eal_vdev_find_and_invoke(). >> rte_eal_dev_invoke() to rte_eal_vdev_invoke(). >> - Add code to handle a return value of rte_eal_devargs_remove(). >> - Fix pci address format in rte_eal_dev_detach(). >> v4: >> - Fix comment. >> - Add error checking. >> - Fix indent of 'if' statement. >> - Change function name. >> >> Signed-off-by: Tetsuya Mukawa >> --- >> lib/librte_eal/common/eal_common_dev.c | 276 +++++++++++++++= +++++++++ >> lib/librte_eal/common/eal_private.h | 11 + >> lib/librte_eal/common/include/rte_dev.h | 33 +++ >> lib/librte_eal/linuxapp/eal/Makefile | 1 + >> lib/librte_eal/linuxapp/eal/eal_pci.c | 6 +- >> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 2 + >> 6 files changed, 326 insertions(+), 3 deletions(-) >> >> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/c= ommon/eal_common_dev.c >> index eae5656..3d169a4 100644 >> --- 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 DAM= AGE. >> */ >> =20 >> +#include >> +#include >> #include >> #include >> #include >> =20 >> +#include >> #include >> #include >> #include >> @@ -107,3 +110,276 @@ rte_eal_dev_init(void) >> } >> return 0; >> } >> + >> +/* So far, DPDK hotplug function only supports linux */ > This comment should be in the configuration. Sure I will. > >> +#ifdef ENABLE_HOTPLUG >> +static void >> +rte_eal_vdev_invoke(struct rte_driver *driver, >> + struct rte_devargs *devargs, enum rte_eal_invoke_type type) >> +{ >> + if ((driver =3D=3D NULL) || (devargs =3D=3D NULL)) >> + return; >> + >> + switch (type) { >> + case RTE_EAL_INVOKE_TYPE_PROBE: >> + driver->init(devargs->virtual.drv_name, devargs->args); >> + break; >> + case RTE_EAL_INVOKE_TYPE_CLOSE: >> + driver->uninit(devargs->virtual.drv_name, devargs->args); >> + break; >> + default: >> + break; >> + } >> +} > It would be clearer to directly call init and uninit instead of using t= his > "invoke" method. Sure, I will change it. >> + >> +static int >> +rte_eal_vdev_find_and_invoke(const char *name, int type) >> +{ >> + struct rte_devargs *devargs; >> + struct rte_driver *driver; >> + >> + if (name =3D=3D NULL) >> + return -EINVAL; >> + >> + /* call the init function for each virtual device */ > This comment is wrong. Thanks, I will fix it. >> + TAILQ_FOREACH(devargs, &devargs_list, next) { >> + >> + if (devargs->type !=3D RTE_DEVTYPE_VIRTUAL) >> + continue; >> + >> + if (strncmp(name, devargs->virtual.drv_name, strlen(name))) >> + continue; >> + >> + TAILQ_FOREACH(driver, &dev_driver_list, next) { >> + if (driver->type !=3D PMD_VDEV) >> + continue; >> + >> + /* search a driver prefix in virtual device name */ >> + if (!strncmp(driver->name, devargs->virtual.drv_name, >> + strlen(driver->name))) { > Why not use strcmp? I will replace it. >> + rte_eal_vdev_invoke(driver, devargs, type); >> + break; >> + } >> + } >> + >> + if (driver =3D=3D NULL) { >> + RTE_LOG(WARNING, EAL, "no driver found for %s\n", >> + devargs->virtual.drv_name); >> + } >> + return 0; >> + } >> + return 1; >> +} >> + >> +/* attach the new physical device, then store port_id of the device *= / >> +static int >> +rte_eal_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 =3D=3D NULL) || (port_id =3D=3D 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 */ >> + if (rte_eal_pci_probe_one(addr)) >> + goto err; > You should get the port_id from the previous function instead of search= ing it. I agree this will beautify this code, but actually to do like above changes current DPDK code much more, and it will not be clear, and not beautiful. Could I explain it more. Problem is initialization sequence of virtual device and physical device are completely different. (1) Attaching a physical device case - To return port id, pci_invoke_all_drivers() needs to return port id. - It means "devinit" of "struct rte_pci_driver" needs to return port_id. - "devinit" will be rte_eth_dev_init(). But if the device is virtual, this function is not implemented. (2) Attaching virtual device case - To return port id from rte_eal_pci_probe_one(), pci_invoke_all_drivers() needs to return port id. - It means "init" of "struct rte_driver" needs to return port_id. - "init" will be implemented in PMD. But this function has different usage in physical device and virtual device. - Especially, In the case of physical device, "init" doesn't allocate eth device, so cannot return port id. As a result, to remove rte_eth_dev_save() and rte_eth_dev_get_changed_port(), below different functions needs to return port id. - "devinit" of "struct rte_pci_driver". - "init" of "struct rte_driver". That is why I implement like above. >> + /* get port_id enabled by above procedures */ >> + if (rte_eth_dev_get_changed_port(devs, &new_port_id)) >> + goto err; >> + >> + *port_id =3D new_port_id; >> + return 0; >> +err: >> + RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); >> + return -1; >> +} > [...] > >> +/* 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; >> + /* 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_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE)) >> + goto err2; > Again, you should get port_id from the attach procedure. > >> + /* get port_id enabled by above procedures */ >> + if (rte_eth_dev_get_changed_port(devs, &new_port_id)) >> + goto err2; > [...] > >> /** >> + * Uninitilization function called for each device driver once. >> + */ >> +typedef int (rte_dev_uninit_t)(const char *name, const char *args); > Why do you need args for uninit? > I just added for the case that finalization code of PMD needs it. But, probably "args" parameter can be removed.