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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2015-02-16 13:14, Tetsuya Mukawa:
> These functions are used for attaching or detaching a port.
> When rte_eal_dev_attach() is called, the function tries to realize the
> device name as pci address. If this is done successfully,
> rte_eal_dev_attach() will attach physical device port. If not, attaches
> virtual devive port.
> When rte_eal_dev_detach() is called, the function gets the device type
> of this port to know whether the port is come from physical or virtual.
> And then specific detaching function will be called.
> 
> v8:
> - Add missing symbol in version map.
>   (Thanks to Qiu, Michael and Iremonger, Bernard)
> v7:
> - Fix typo of warning messages.
>   (Thanks to Qiu, Michael)
> v5:
> - Change function names like below.
>   rte_eal_dev_find_and_invoke() to rte_eal_vdev_find_and_invoke().
>   rte_eal_dev_invoke() to rte_eal_vdev_invoke().
> - Add code to handle a return value of rte_eal_devargs_remove().
> - Fix pci address format in rte_eal_dev_detach().
> v4:
> - Fix comment.
> - Add error checking.
> - Fix indent of 'if' statement.
> - Change function name.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/common/eal_common_dev.c          | 276 ++++++++++++++++++++++++
>  lib/librte_eal/common/eal_private.h             |  11 +
>  lib/librte_eal/common/include/rte_dev.h         |  33 +++
>  lib/librte_eal/linuxapp/eal/Makefile            |   1 +
>  lib/librte_eal/linuxapp/eal/eal_pci.c           |   6 +-
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   2 +
>  6 files changed, 326 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index eae5656..3d169a4 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -32,10 +32,13 @@
>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> +#include <stdio.h>
> +#include <limits.h>
>  #include <string.h>
>  #include <inttypes.h>
>  #include <sys/queue.h>
>  
> +#include <rte_ethdev.h>
>  #include <rte_dev.h>
>  #include <rte_devargs.h>
>  #include <rte_debug.h>
> @@ -107,3 +110,276 @@ rte_eal_dev_init(void)
>  	}
>  	return 0;
>  }
> +
> +/* So far, DPDK hotplug function only supports linux */

This comment should be in the configuration.

> +#ifdef ENABLE_HOTPLUG
> +static void
> +rte_eal_vdev_invoke(struct rte_driver *driver,
> +		struct rte_devargs *devargs, enum rte_eal_invoke_type type)
> +{
> +	if ((driver == NULL) || (devargs == NULL))
> +		return;
> +
> +	switch (type) {
> +	case RTE_EAL_INVOKE_TYPE_PROBE:
> +		driver->init(devargs->virtual.drv_name, devargs->args);
> +		break;
> +	case RTE_EAL_INVOKE_TYPE_CLOSE:
> +		driver->uninit(devargs->virtual.drv_name, devargs->args);
> +		break;
> +	default:
> +		break;
> +	}
> +}

It would be clearer to directly call init and uninit instead of using this
"invoke" method.

> +
> +static int
> +rte_eal_vdev_find_and_invoke(const char *name, int type)
> +{
> +	struct rte_devargs *devargs;
> +	struct rte_driver *driver;
> +
> +	if (name == NULL)
> +		return -EINVAL;
> +
> +	/* call the init function for each virtual device */

This comment is wrong.

> +	TAILQ_FOREACH(devargs, &devargs_list, next) {
> +
> +		if (devargs->type != RTE_DEVTYPE_VIRTUAL)
> +			continue;
> +
> +		if (strncmp(name, devargs->virtual.drv_name, strlen(name)))
> +			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))) {

Why not use strcmp?

> +				rte_eal_vdev_invoke(driver, devargs, type);
> +				break;
> +			}
> +		}
> +
> +		if (driver == NULL) {
> +			RTE_LOG(WARNING, EAL, "no driver found for %s\n",
> +				  devargs->virtual.drv_name);
> +		}
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/* attach the new physical device, then store port_id of the device */
> +static int
> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
> +{
> +	uint8_t new_port_id;
> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> +
> +	if ((addr == NULL) || (port_id == NULL))
> +		goto err;
> +
> +	/* save current port status */
> +	if (rte_eth_dev_save(devs, sizeof(devs)))
> +		goto err;
> +	/* re-construct pci_device_list */
> +	if (rte_eal_pci_scan())
> +		goto err;
> +	/* invoke probe func of the driver can handle the new device */
> +	if (rte_eal_pci_probe_one(addr))
> +		goto err;

You should get the port_id from the previous function instead of searching it.

> +	/* get port_id enabled by above procedures */
> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> +		goto err;
> +
> +	*port_id = new_port_id;
> +	return 0;
> +err:
> +	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> +	return -1;
> +}

[...]

> +/* 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;
> +	/* 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 */
> +	if (rte_eal_vdev_find_and_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
> +		goto err2;

Again, you should get port_id from the attach procedure.

> +	/* get port_id enabled by above procedures */
> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> +		goto err2;

[...]

>  /**
> + * Uninitilization function called for each device driver once.
> + */
> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);

Why do you need args for uninit?