From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f181.google.com (mail-pd0-f181.google.com [209.85.192.181]) by dpdk.org (Postfix) with ESMTP id 4056CB569 for ; Fri, 20 Feb 2015 11:32:29 +0100 (CET) Received: by pdno5 with SMTP id o5so6737582pdn.8 for ; Fri, 20 Feb 2015 02:32:28 -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=U11V6+IE8b2XRbGKe0M2QbCyQgonVqHmBTpJP343/VU=; b=E1kucKXEnV1FkBREArR9fzAkYfHx1MDeSz7vforB9EXx9QqMxp4V0SmmBNkKd/qbJY z8hsEvvGuG6T9dzt0FPXqfNlDVQyBLFmGjOrCSKWnzpiRZbtZSJRQrNxtC3WyzZo4b5e uFFG1VQth/BO3yfGW3u/4j63pKEp5gF4B70iOht/R3Ppk66N3cPCVeaFI2bSMHZ46jEL OejK4i4mGqSlPGGECqxIDaMqIw61xrvx+c2a3kTYOhhKEscm7YsmsmamNbprtu7tK+Le u6lOBzC6sW9WdIV6W7Ukovk6HfIK91gkUDTo3CBUBcssxsCvZgP8o5Z951xNka4iKC4F oxBQ== X-Gm-Message-State: ALoCoQnqjFInJ01QyskVWfyhZjELyNld4KvdbkqOsx78TZXytNhKe99g42rS0RJNw9uf/POlrhs3 X-Received: by 10.70.91.75 with SMTP id cc11mr3253197pdb.150.1424428348640; Fri, 20 Feb 2015 02:32:28 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id w17sm26450065pdj.83.2015.02.20.02.32.26 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 Feb 2015 02:32:28 -0800 (PST) Message-ID: <54E70D38.6090209@igel.co.jp> Date: Fri, 20 Feb 2015 19:32:24 +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: Maxime Leroy References: <1424060073-23484-2-git-send-email-mukawa@igel.co.jp> <1424414390-18509-1-git-send-email-mukawa@igel.co.jp> <1424414390-18509-14-git-send-email-mukawa@igel.co.jp> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v10 13/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: Fri, 20 Feb 2015 10:32:29 -0000 On 2015/02/20 19:14, Maxime Leroy wrote: > Hi Tetsuya, > > On Fri, Feb 20, 2015 at 7:39 AM, Tetsuya Mukawa wro= te: >> These functions are used for attaching or detaching a port. > [..] >> + >> +static void >> +get_vdev_name(char *vdevargs) >> +{ >> + char *sep; >> + >> + if (vdevargs =3D=3D NULL) >> + return; >> + >> + /* set the first ',' to '\0' to split name and arguments */ >> + sep =3D strchr(vdevargs, ','); >> + if (sep !=3D NULL) >> + sep[0] =3D '\0'; >> +} >> + >> +/* 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; > You don't need to add devargs to the devargs_list. > > The devargs_list is only needed at the init to store the arguments > when we parse the command line. Then, at initialization, > rte_eal_dev_init creates the devices from this list . > > Instead of adding the devargs in the list, you could have the following= code: > > static int > rte_eal_dev_attach_vdev(const char *vdevargs, uint8_t *port_id) > { > ... > /* save current port status */ > if (rte_eth_dev_save(devs, sizeof(devs))) > goto err; > > devargs =3D rte_eal_parse_devargs_str(RTE_ > DEVTYPE_VIRTUAL, vdevargs); > if (devargs =3D=3D NULL) > goto err; > > if (rte_eal_vdev_devinit(devargs) < 0) > goto err; > > if (rte_eth_dev_get_changed_port(devs, &new_port_id)) > goto err; > > ... > } > > What do you think ? Hi Maxime, I appreciate for your comment. When rte_eal_init() is called, if we have "--vdev" options, these will be stored in vdevargs as you describe above. I agree this is the current behavior of DPDK. When we call hotplug functions, I guess doing same thing will be more consistent design. For example, we can do like below. 1. $ ./testpmd --vdev 'eth_pcap' -- -i 2. testpmd>port detach Also we can do like below. 1. $ ./testpmd -- -i 2. testpmd> port attach eth_pcap 3. testpmd> port detach After doing above cases, we have no port. But in only first case, vdevargs still has a "--vdev" option value. So I guess current hotplug implementation is more consistent design. How about? Regards, Tetsuya >> + /* parse vdevargs, then retrieve device name */ >> + get_vdev_name(args); >> + /* walk around dev_driver_list to find the driver of the devic= e, >> + * then invoke probe function o the driver. >> + * TODO: >> + * rte_eal_vdev_find_and_init() should return port_id, >> + * And rte_eth_dev_save() and rte_eth_dev_get_changed_port() >> + * should be removed. */ >> + if (rte_eal_vdev_find_and_init(args)) >> + goto err2; >> + /* get port_id enabled by above procedures */ >> + if (rte_eth_dev_get_changed_port(devs, &new_port_id)) >> + goto err2; >> + >> + free(args); >> + *port_id =3D new_port_id; >> + return 0; >> +err2: >> + rte_eal_devargs_remove(RTE_DEVTYPE_VIRTUAL, args); >> +err1: >> + free(args); >> +err0: >> + RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); >> + return -1; >> +} >> + >> +/* detach the new virtual device, then store the name of the device *= / >> +static int >> +rte_eal_dev_detach_vdev(uint8_t port_id, char *vdevname) >> +{ >> + char name[RTE_ETH_NAME_MAX_LEN]; >> + >> + if (vdevname =3D=3D NULL) >> + goto err; >> + >> + /* check whether the driver supports detach feature, or not */= >> + if (rte_eth_dev_is_detachable(port_id)) >> + goto err; >> + >> + /* get device name by port id */ >> + if (rte_eth_dev_get_name_by_port(port_id, name)) >> + goto err; >> + /* walk around dev_driver_list to find the driver of the devic= e, >> + * then invoke close function o the driver */ >> + if (rte_eal_vdev_find_and_uninit(name)) >> + goto err; >> + /* remove the vdevname from devargs_list */ >> + if (rte_eal_devargs_remove(RTE_DEVTYPE_VIRTUAL, name)) >> + goto err; > The same here, you don't need to remove devargs from the devargs_list. > > Instead of removing the devargs in the list, you could have the followi= ng code:: > > static int > rte_eal_dev_detach_vdev(uint8_t port_id, char *vdevname) > { > ... > /* check whether the driver supports detach feature, or not */= > if (rte_eth_dev_is_detachable(port_id)) > goto err; > > /* get device name by port id */ > if (rte_eth_dev_get_name_by_port(port_id, name)) > goto err; > > if (rte_eal_vdev_uninit(name)) > goto err; > ... > } > > What do you think ? > > > Regards, > > Maxime