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