On 2015/02/17 9:56, Thomas Monjalon wrote: > 2015-02-16 13:14, Tetsuya Mukawa: >> The patch adds function pointer to rte_pci_driver and eth_driver >> structure. These function pointers are used when ports are detached. >> Also, the patch adds rte_eth_dev_uninit(). So far, it's not called >> by anywhere, but it will be called when port hotplug function is >> implemented. >> >> v6: >> - Fix rte_eth_dev_uninit() to handle a return value of uninit >> function of PMD. >> v4: >> - Add parameter checking. >> - Change function names. >> >> Signed-off-by: Tetsuya Mukawa <mukawa at igel.co.jp> >> --- >> lib/librte_eal/common/include/rte_pci.h | 7 +++++ >> lib/librte_ether/rte_ethdev.c | 47 >> +++++++++++++++++++++++++++++++++ >> lib/librte_ether/rte_ethdev.h | 24 +++++++++++++++++ >> 3 files changed, 78 insertions(+) >> >> diff --git a/lib/librte_eal/common/include/rte_pci.h >> b/lib/librte_eal/common/include/rte_pci.h >> index 4814cd7..87ca4cf 100644 >> --- a/lib/librte_eal/common/include/rte_pci.h >> +++ b/lib/librte_eal/common/include/rte_pci.h >> @@ -189,12 +189,19 @@ struct rte_pci_driver; >> typedef int (pci_devinit_t)(struct rte_pci_driver *, struct rte_pci_device >> *); >> >> /** >> + * Uninitialisation function for the driver called during hotplugging. >> + */ >> +typedef int (pci_devuninit_t)( >> + struct rte_pci_driver *, struct rte_pci_device *); >> + >> +/** >> * A structure describing a PCI driver. >> */ >> struct rte_pci_driver { >> TAILQ_ENTRY(rte_pci_driver) next; /**< Next in list. */ >> const char *name; /**< Driver name. */ >> pci_devinit_t *devinit; /**< Device init. function. */ >> + pci_devuninit_t *devuninit; /**< Device uninit function. */ >> struct rte_pci_id *id_table; /**< ID table, NULL terminated. >> */ >> uint32_t drv_flags; /**< Flags contolling handling >> of device. */ >> }; >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c >> index 2463d18..58d8072 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -326,6 +326,52 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv, >> return diag; >> } >> >> +static int >> +rte_eth_dev_uninit(struct rte_pci_driver *pci_drv, >> + struct rte_pci_device *pci_dev) > There's something strange here: this is an uninit of an ethdev but the > parameter > is a pci_dev.
rte_eth_dev_init and rte_eth_dev_uninit are called by PCI layer. I guess PCI layer cannot handle eth device or eth driver directly. I guess receiving pci dev in eth layer may be fair. But as you said, pci_drv can be removed. > The driver parameter shouldn't be needed. I will change the function like below. static int rte_eth_dev_uninit(struct rte_pci_device *pci_dev) >> +{ >> + struct eth_driver *eth_drv; >> + struct rte_eth_dev *eth_dev; >> + char ethdev_name[RTE_ETH_NAME_MAX_LEN]; >> + int ret; >> + >> + if ((pci_drv == NULL) || (pci_dev == NULL)) >> + return -EINVAL; >> + >> + /* Create unique Ethernet device name using PCI address */ >> + snprintf(ethdev_name, RTE_ETH_NAME_MAX_LEN, "%d:%d.%d", >> + pci_dev->addr.bus, pci_dev->addr.devid, >> + pci_dev->addr.function); > You should call a function common to init and uninit to generate > the same unique name. Sure, I will. >> + >> + eth_dev = rte_eth_dev_allocated(ethdev_name); >> + if (eth_dev == NULL) >> + return -ENODEV; >> + >> + eth_drv = (struct eth_driver *)pci_drv; >> + >> + /* Invoke PMD device uninit function */ >> + if (*eth_drv->eth_dev_uninit) { >> + ret = (*eth_drv->eth_dev_uninit)(eth_drv, eth_dev); >> + if (ret) >> + return ret; >> + } >> + >> + /* free ether device */ >> + rte_eth_dev_free(eth_dev); >> + >> + /* init user callbacks */ >> + TAILQ_INIT(&(eth_dev->callbacks)); > Please comment more why you are resetting callbacks. > An init in an uninit function seems weird ;) I agree. This code can be removed. (Actually callbacks will be initialized when device is initialized.) >> + >> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) >> + rte_free(eth_dev->data->dev_private); >> + >> + eth_dev->pci_dev = NULL; >> + eth_dev->driver = NULL; >> + eth_dev->data = NULL; >> + >> + return 0; >> +} >> + >> /** >> * Register an Ethernet [Poll Mode] driver. >> * >> @@ -344,6 +390,7 @@ void >> rte_eth_driver_register(struct eth_driver *eth_drv) >> { >> eth_drv->pci_drv.devinit = rte_eth_dev_init; >> + eth_drv->pci_drv.devuninit = rte_eth_dev_uninit; >> rte_eal_pci_register(ð_drv->pci_drv); >> } >> >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h >> index fbe7ac1..91d9e86 100644 >> --- a/lib/librte_ether/rte_ethdev.h >> +++ b/lib/librte_ether/rte_ethdev.h >> @@ -1678,6 +1678,27 @@ typedef int (*eth_dev_init_t)(struct eth_driver >> *eth_drv, >> >> /** >> * @internal >> + * Finalization function of an Ethernet driver invoked for each matching >> + * Ethernet PCI device detected during the PCI closing phase. >> + * >> + * @param eth_drv >> + * The pointer to the [matching] Ethernet driver structure supplied by >> + * the PMD when it registered itself. >> + * @param eth_dev >> + * The *eth_dev* pointer is the address of the *rte_eth_dev* structure >> + * associated with the matching device and which have been [automatically] >> + * allocated in the *rte_eth_devices* array. >> + * @return >> + * - 0: Success, the device is properly finalized by the driver. >> + * In particular, the driver MUST free the *dev_ops* pointer >> + * of the *eth_dev* structure. >> + * - <0: Error code of the device initialization failure. >> + */ >> +typedef int (*eth_dev_uninit_t)(struct eth_driver *eth_drv, >> + struct rte_eth_dev *eth_dev); >> + >> +/** >> + * @internal >> * The structure associated with a PMD Ethernet driver. >> * >> * Each Ethernet driver acts as a PCI driver and is represented by a generic >> @@ -1687,11 +1708,14 @@ typedef int (*eth_dev_init_t)(struct eth_driver >> *eth_drv, >> * >> * - The *eth_dev_init* function invoked for each matching PCI device. >> * >> + * - The *eth_dev_uninit* function invoked for each matching PCI device. >> + * >> * - The size of the private data to allocate for each matching device. >> */ >> struct eth_driver { >> struct rte_pci_driver pci_drv; /**< The PMD is also a PCI driver. */ >> eth_dev_init_t eth_dev_init; /**< Device init function. */ >> + eth_dev_uninit_t eth_dev_uninit; /**< Device uninit function. */ >> unsigned int dev_private_size; /**< Size of device private data. */ >> }; >> >> >