On 2015/02/23 22:29, Maxime Leroy wrote:
> Hi Tetsuya,
>
> On Mon, Feb 23, 2015 at 6:09 AM, Tetsuya Mukawa <mukawa at 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
