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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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