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.