Re: [PATCH] net/memif: allow stopping and closing device

[yoursunny <sunnylandh@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2662 bytes --]

Hi Ferruh

You have to rely on user to call stop before calling close/remove.
This is mandated in ethdev library, as implemented in:
febc855b358e ("ethdev: forbid closing started device")

Yours, Junxiao

On Wed, Nov 24, 2021, 06:02 Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 11/18/2021 5:33 PM, Junxiao Shi wrote:
> > Bugzilla ID: 888
> > Fixes: febc855b358e ("ethdev: forbid closing started device")
> >
> > Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
>
> Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.
>
> > ---
> >   drivers/net/memif/rte_eth_memif.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/memif/rte_eth_memif.c
> b/drivers/net/memif/rte_eth_memif.c
> > index 43d7378329..e3d523af57 100644
> > --- a/drivers/net/memif/rte_eth_memif.c
> > +++ b/drivers/net/memif/rte_eth_memif.c
> > @@ -1260,6 +1260,13 @@ memif_dev_start(struct rte_eth_dev *dev)
> >       return ret;
> >   }
> >
> > +static int
> > +memif_dev_stop(struct rte_eth_dev *dev)
> > +{
> > +     memif_disconnect(dev);
>
> Is the 'memif_dev_stop()' safe to be called multiple times?
> If 'memif_dev_close()' calls 'memif_dev_stop()' (see below), it is possible
> to call 'memif_dev_stop()' multiple times, so it should be protected.
>
> > +     return 0;
> > +}
> > +
> >   static int
> >   memif_dev_close(struct rte_eth_dev *dev)
> >   {
> > @@ -1268,7 +1275,6 @@ memif_dev_close(struct rte_eth_dev *dev)
> >
> >       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> >               memif_msg_enq_disconnect(pmd->cc, "Device closed", 0);
> > -             memif_disconnect(dev);
> >
> >               for (i = 0; i < dev->data->nb_rx_queues; i++)
> >                       (*dev->dev_ops->rx_queue_release)(dev, i);
> > @@ -1276,8 +1282,6 @@ memif_dev_close(struct rte_eth_dev *dev)
> >                       (*dev->dev_ops->tx_queue_release)(dev, i);
> >
> >               memif_socket_remove_device(dev);
> > -     } else {
> > -             memif_disconnect(dev);
>
> Should we add 'memif_dev_stop()' within the close function?
> Otherwise we are relying on user to stop, but at least in remove path
> ('rte_pmd_memif_remove()') that may not be the case.
>
> >       }
> >
> >       rte_free(dev->process_private);
> > @@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev
> *dev, uint16_t qid __rte_unused)
> >
> >   static const struct eth_dev_ops ops = {
> >       .dev_start = memif_dev_start,
> > +     .dev_stop = memif_dev_stop,
> >       .dev_close = memif_dev_close,
> >       .dev_infos_get = memif_dev_info,
> >       .dev_configure = memif_dev_configure,
> >
>
>

[-- Attachment #2: Type: text/html, Size: 3762 bytes --]