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

[Tetsuya Mukawa]

On 2015/02/20 19:14, Maxime Leroy wrote:
> Hi Tetsuya,
>
> On Fri, Feb 20, 2015 at 7:39 AM, Tetsuya Mukawa <mukawa at 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 ?

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 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