31/10/2018 18:46, Luca Boccassi: > Sorry, been otherwise busy - I can do what you and Chas have asked, but > the problem is that v1 already did that and the VMWare maintainers > asked to change it back. So can I assume that the v1 way is the way to > go?
I am expecting an answer from the vmxnet3 maintainer, or other VMware developpers. If they don't reply, we should work without them, and ask for a new maintainer in the community. I assume you can now work on a v3 reproducing what was done in v1. > On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote: > > Any update or question for this patch? > > If no update, it will miss 18.11. > > > > > > 27/10/2018 17:09, Thomas Monjalon: > > > 19/09/2018 17:47, Chas Williams: > > > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bl...@debian.org> > > > > wrote: > > > > > > > > > > The vmxnet3 driver can't call back into dev_close(), and > > > > > possibly > > > > > dev_stop(), in dev_uninit(). When dev_uninit() is called, > > > > > anything > > > > > that those routines would want to clean up has already been > > > > > released. > > > > > Further, for complete cleanup, it is necessary to release any > > > > > of the > > > > > queue resources during dev_close(). > > > > > This allows a vmxnet3 device to be hot-unplugged without > > > > > leaking > > > > > queues. > > > > > > > > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode > > > > > driver implementation") > > > > > Cc: sta...@dpdk.org > > > > > > > > > > Signed-off-by: Brian Russell <bruss...@brocade.com> > > > > > Signed-off-by: Luca Boccassi <bl...@debian.org> > > > > > --- > > > > > v2: add back extra close() call in uninit() for buggy > > > > > applications as > > > > > requested by the reviewers, and add debug log noting the > > > > > issue > > > > > > > > > > drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 > > > > > +++++++++++++++++++++++----- > > > > > 1 file changed, 29 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > index f1596ab19d..98e5d01890 100644 > > > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev > > > > > *eth_dev) > > > > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > > > > > return 0; > > > > > > > > This should probably be EPERM as well. Out of scope though. > > > > > > > > > > > > > > - if (hw->adapter_stopped == 0) > > > > > + if (hw->adapter_stopped == 0) { > > > > > + PMD_INIT_LOG(DEBUG, "Device has not been > > > > > closed."); > > > > > vmxnet3_dev_close(eth_dev); > > > > > > > > This just seems wrong. You have called uninit() will the driver > > > > is > > > > still busy. Instead of "fixing" the state of the driver, return > > > > EBUSY > > > > here. > > > > > > I agree. > > > If the port is not stopped, either you stop it or you return EBUSY. > > > > > > Closing the device should be done outside of this check. > > > It is OK to close from uninit if the app did not close it. > > > > > > [...] > > > > > +static void > > > > > +vmxnet3_free_queues(struct rte_eth_dev *dev) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + PMD_INIT_FUNC_TRACE(); > > > > > + > > > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > > > + void *rxq = dev->data->rx_queues[i]; > > > > > + > > > > > + vmxnet3_dev_rx_queue_release(rxq); > > > > > + } > > > > > + dev->data->nb_rx_queues = 0; > > > > > + > > > > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > > > > + void *txq = dev->data->tx_queues[i]; > > > > > + > > > > > + vmxnet3_dev_tx_queue_release(txq); > > > > > + } > > > > > + dev->data->nb_tx_queues = 0; > > > > > } > > > > > > > > > > /* > > > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev) > > > > > static void > > > > > vmxnet3_dev_close(struct rte_eth_dev *dev) > > > > > { > > > > > - struct vmxnet3_hw *hw = dev->data->dev_private; > > > > > - > > > > > PMD_INIT_FUNC_TRACE(); > > > > > > > > > > vmxnet3_dev_stop(dev); > > > > > - hw->adapter_stopped = 1; > > > > > + vmxnet3_free_queues(dev); > > > > > } > > > > > > Good clean-up on dev_close. > > > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE > > > for allowing > > > a real release of the port on close. > > > Note: every PMDs should migrate towards this behaviour. > > > > > > To make things clear (I will write a doc for -rc2): > > > - "stop" should be called by the app but the PMD is allowed to > > > force it. > > > - "close" may be called by the app, and the PMD should enforce > > > it in uninit. > > > With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely > > > release the port. > > > - "remove" (implemented in PMD as uninit) is responsible of > > > closing > > > ethdev ports if not already done, and release the > > > shared resources > > > which are not specific to a port. It removes the whole > > > EAL rte_device. > > > > > > PS: for any hotplug patch or questions, feel free to Cc me. > > > > > > > > > > > >