From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f182.google.com (mail-we0-f182.google.com [74.125.82.182]) by dpdk.org (Postfix) with ESMTP id B95ADB555 for ; Fri, 20 Feb 2015 11:14:31 +0100 (CET) Received: by wevm14 with SMTP id m14so4632352wev.13 for ; Fri, 20 Feb 2015 02:14:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=zjLkp1yZrS9Qi0SlIIlWZq4Me4eAbxrB1XJgh6ec2iI=; b=Q6MDbI3t4D0NibGDbYkgsvYf1KLwtVil5jyaRYWl5TBNJbebO1h4xqSvyH8eAohLSv L+aUaXaw8ZjbD8qH2ChMPWVTaVq6hFJtbnXr8KnAS3sIXvVoLkxI0Lfvtk4QsGKjpfq4 GQN+XzLmyqOA7CpQX7RIWkLuvEUBstuzFxZTsmRvTYDJ98l3unDzEh5cMVeNhzJ3YG9P g1sQweKxYEHAZzFM6VK7vjfVTDd0+nEAP2d3pWUPOiLjHS/BoCvjo3Wr9KXVZSugUvY/ ZhooaoL0Ncci26oNGfQuPUo7mGvRZdRvLqLkg8jXN6SAM/tQr1OVeqGjaR/bNYUTKixa He3Q== X-Gm-Message-State: ALoCoQmATtZAc6vgWFYUnxzINO9yvxlgcA/2NOqfgz6ep+bsy7DEz9zf0uZNa83VqHlYbZzgpqYj MIME-Version: 1.0 X-Received: by 10.194.241.197 with SMTP id wk5mr407406wjc.44.1424427271546; Fri, 20 Feb 2015 02:14:31 -0800 (PST) Received: by 10.194.185.204 with HTTP; Fri, 20 Feb 2015 02:14:31 -0800 (PST) In-Reply-To: <1424414390-18509-14-git-send-email-mukawa@igel.co.jp> 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> Date: Fri, 20 Feb 2015 11:14:31 +0100 Message-ID: From: Maxime Leroy To: Tetsuya Mukawa Content-Type: text/plain; charset=UTF-8 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:14:32 -0000 Hi Tetsuya, On Fri, Feb 20, 2015 at 7:39 AM, Tetsuya Mukawa wrote: > These functions are used for attaching or detaching a port. [..] > + > +static void > +get_vdev_name(char *vdevargs) > +{ > + char *sep; > + > + if (vdevargs == NULL) > + return; > + > + /* set the first ',' to '\0' to split name and arguments */ > + sep = strchr(vdevargs, ','); > + if (sep != NULL) > + sep[0] = '\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 == NULL) || (port_id == NULL)) > + goto err0; > + > + args = strdup(vdevargs); > + if (args == 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 = rte_eal_parse_devargs_str(RTE_ DEVTYPE_VIRTUAL, vdevargs); if (devargs == 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 ? > + /* 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. > + * 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 = 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 == 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 device, > + * 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 following 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