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

[Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>]

On 2015/02/17 10:48, Thomas Monjalon wrote:
> 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-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
>> ---
>>  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.

Sure I will.

>
>> +#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.

Sure, I will change it.

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

Thanks, I will fix it.

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

I will replace it.

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

I agree this will beautify this code, but actually to do like above
changes current DPDK code much more, and it will not be clear, and not
beautiful.

Could I explain it more.
Problem is initialization sequence of virtual device and physical device
are completely different.

(1) Attaching a physical device case
- To return port id, pci_invoke_all_drivers() needs to return port id.
- It means "devinit" of "struct rte_pci_driver" needs to return port_id.
- "devinit" will be rte_eth_dev_init(). But if the device is virtual,
this function is not implemented.

(2) Attaching virtual device case
- To return port id from rte_eal_pci_probe_one(),
pci_invoke_all_drivers() needs to return port id.
- It means "init" of "struct rte_driver" needs to return port_id.
- "init" will be implemented in PMD. But this function has different
usage in physical device and virtual device.
- Especially, In the case of physical device, "init" doesn't allocate
eth device, so cannot return port id.

As a result, to remove  rte_eth_dev_save() and
rte_eth_dev_get_changed_port(), below different functions needs to
return port id.
 - "devinit" of "struct rte_pci_driver".
 - "init" of "struct rte_driver".

That is why I implement like above.

>> +	/* 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?
>

I just added for the case that finalization code of PMD needs it.
But, probably "args" parameter can be removed.