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

[Tetsuya Mukawa]

On 2015/02/18 10:17, Thomas Monjalon wrote:
> 2015-02-17 19:26, Tetsuya Mukawa:
>> 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.
> Obviously, it would be better to have it in dpdk-2.0.0-rc1.
> If not possible to fix it, would it be possible to work on other comments
> and keep this cleanup for post-rc1 integration?
> I feel this cleanup is important to get the right design but it wouldn't be
> fair to block this (old) patchset for this reason.
>

I appreciate for your suggestion. I will keep working on it for post-rc1.

Thanks,
Tetsuya