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

Tetsuya Mukawa mukawa at igel.co.jp
Tue Feb 17 11:26:42 CET 2015


On 2015/02/17 18:23, Thomas Monjalon wrote:
> 2015-02-17 17:51, Tetsuya Mukawa:
>> On 2015/02/17 10:48, Thomas Monjalon wrote:
>>> 2015-02-16 13:14, Tetsuya Mukawa:
>>>> +/* 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".
> Yes, exactly,
> I think you shouldn't hesitate to improve existing EAL code.
> And I also think we should try to remove differences between virtual and
> pci devices.

I strongly agree with it. But I haven't investigated how to remove it so
far.
To be honest, I want to submit hotplug patches to DPDK-2.0.
Is above functionality needed to merge the hotplug patches?
I guess I will not be able to remove it by 23rd.

Regards,
Tetsuya

>> 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.
> Yes I think
>
>




More information about the dev mailing list