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

[Thomas Monjalon]

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.