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

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/02/23 22:29, Maxime Leroy wrote:
> Hi Tetsuya,
>
> On Mon, Feb 23, 2015 at 6:09 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>> These functions are used for attaching or detaching a port.
> [...]
>> +static int
>> +rte_eal_vdev_init(const char *name, const char *args)
>> +{
>> +       struct rte_driver *driver;
>> +
>> +       if (name == NULL)
>> +               return -EINVAL;
>> +
>> +       TAILQ_FOREACH(driver, &dev_driver_list, next) {
>> +               if (driver->type != PMD_VDEV)
>> +                       continue;
>> +
>> +               /*
>> +                * search a driver prefix in virtual device name.
>> +                * For example, if the driver is pcap PMD, driver->name
>> +                * will be "eth_pcap", but "name" will be "eth_pcapN".
>> +                * So use strncmp to compare.
>> +                */
>> +               if (!strncmp(driver->name, name, strlen(driver->name))) {
>> +                       driver->init(name, args);
>> +                       break;
> Please return the value given by init: return driver->init(name, args); .
>
>> +               }
>> +       }
>> +
>> +       if (driver == NULL) {
>> +               RTE_LOG(WARNING, EAL, "no driver found for %s\n", name);
> --> should be : RTE_LOG(ERR .
>
>
>> +               return -EINVAL;
>> +       }
>> +       return 0;
>> +}
>> +
>>  int
>>  rte_eal_dev_init(void)
>>  {
>> @@ -79,23 +113,10 @@ rte_eal_dev_init(void)
>>                 if (devargs->type != RTE_DEVTYPE_VIRTUAL)
>>                         continue;
>>
>> -               TAILQ_FOREACH(driver, &dev_driver_list, next) {
>> -                       if (driver->type != PMD_VDEV)
>> -                               continue;
>> -
>> -                       /* search a driver prefix in virtual device name */
>> -                       if (!strncmp(driver->name, devargs->virtual.drv_name,
>> -                                       strlen(driver->name))) {
>> -                               driver->init(devargs->virtual.drv_name,
>> -                                       devargs->args);
>> -                               break;
>> -                       }
>> -               }
>> -
>> -               if (driver == NULL) {
>> +               if (rte_eal_vdev_init(devargs->virtual.drv_name,
>> +                                       devargs->args))
>>                         rte_panic("no driver found for %s\n",
>>                                   devargs->virtual.drv_name);
> instead of that:
>
> if (rte_eal_vdev_init(devargs->virtual.drv_name, devargs->args)) {
>           RTE_LOG(ERR, "failed to initialize %s device\n",
> devargs->virtual.drv_name);
>           return -1;
> }
>
>> -               }
>>         }
>>
>>         /* Once the vdevs are initalized, start calling all the pdev drivers */
>> @@ -107,3 +128,237 @@ rte_eal_dev_init(void)
>>         }
>>         return 0;
>>  }
>> +
>> +/* So far, DPDK hotplug function only supports linux */
>> +#ifdef RTE_LIBRTE_EAL_HOTPLUG
>> +static int
>> +rte_eal_vdev_uninit(const char *name)
>> +{
>> +       struct rte_driver *driver;
>> +
>> +       if (name == NULL)
>> +               return -EINVAL;
>> +
>> +       TAILQ_FOREACH(driver, &dev_driver_list, next) {
>> +               if (driver->type != PMD_VDEV)
>> +                       continue;
>> +
>> +               /*
>> +                * search a driver prefix in virtual device name.
>> +                * For example, if the driver is pcap PMD, driver->name
>> +                * will be "eth_pcap", but "name" will be "eth_pcapN".
>> +                * So use strncmp to compare.
>> +                */
>> +               if (!strncmp(driver->name, name, strlen(driver->name))) {
>> +                       driver->uninit(name);
> Please return the value given by uninit: return driver->uninit(name, args);
>
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (driver == NULL) {
>> +               RTE_LOG(WARNING, EAL, "no driver found for %s\n", name);
>> +               return 1;
> As it's an error, the function should return a negative value ( i.e. -EINVAL).
> Please set the log level to ERR.
>
>> +       }
>> +       return 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 *name = NULL, *args = NULL;
>> +       uint8_t new_port_id;
>> +       struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
>> +       int ret = -1;
>> +
>> +       if ((vdevargs == NULL) || (port_id == NULL))
>> +               goto end;
>> +
>> +       /* parse vdevargs, then retrieve device name and args */
>> +       if (rte_eal_parse_devargs_str(vdevargs, &name, &args))
>> +               goto end;
>> +
>> +       /* save current port status */
>> +       if (rte_eth_dev_save(devs, sizeof(devs)))
>> +               goto end;
>> +       /* walk around dev_driver_list to find the driver of the device,
>> +        * then invoke probe function o the driver.
>> +        * TODO:
>> +        * rte_eal_vdev_init() should return port_id,
>> +        * And rte_eth_dev_save() and rte_eth_dev_get_changed_port()
>> +        * should be removed. */
>> +       if (rte_eal_vdev_init(name, args))
>> +               goto end;
>> +       /* get port_id enabled by above procedures */
>> +       if (rte_eth_dev_get_changed_port(devs, &new_port_id))
>> +               goto end;
>> +       ret = 0;
>> +
> Please set port_id here, i.e. :  *port_id = new_port_id;
>
>> +end:
>> +       if (name)
>> +               free(name);
>> +       if (args)
>> +               free(args);
>> +
>> +       *port_id = new_port_id;
> and not here.
>
>
>> +       if (ret < 0)
>> +               RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
>> +       return ret;
>> +}
>> +
> e\n");
>> +       return -1;
>> +}
>> +
Hi Maxime,

I appreciate your comments.
I've change like your comments, and send new one soon.

Regards,
Tetsuya


> Regards,
>
> Maxime