Re: [dpdk-dev] [PATCH v10 13/14] eal/pci: Add rte_eal_dev_attach/detach() functions

[Maxime Leroy <maxime.leroy@6wind.com>]

Hi Tetsuya,

On Fri, Feb 20, 2015 at 7:39 AM, Tetsuya Mukawa <mukawa@igel.co.jp> 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