Re: [dpdk-dev] [PATCH v2 2/2] net/mlx5: free eth dev port in case of error

[Yongseok Koh <yskoh@mellanox.com>]

On Mon, May 07, 2018 at 06:10:33PM +0300, Raslan Darawsheh wrote:

Please refine the title. No use of 'eth dev...'

> If something went wrong in mlx5 pci prop need to free the eth_dev
> that was previously allocated during the port setup.

Shouldn't it be sent to stable branches if it is a bug?

> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> 
> ---
> v2 changes:
>  Reword the commit log.
> ---
> ---
>  drivers/net/mlx5/mlx5.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 3831e3d..bffe90f 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1063,6 +1063,8 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  			claim_zero(mlx5_glue->dealloc_pd(pd));
>  		if (ctx)
>  			claim_zero(mlx5_glue->close_device(ctx));
> +		if (eth_dev)
> +			rte_eth_dev_release_port(eth_dev);

I worry about the case where secondary process has some failure above. Secondary
process doesn't call rte_eth_dev_allocate() but rte_eth_dev_attach_secondary().
But, rte_eth_dev_release_port() clears the shared data,
	memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));

Then, the primary process will be malfunctioning. This seems a bug in the
rte_eth_dev_release_port(), 

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index e5605242d..20180146b 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -352,7 +352,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)

	eth_dev->state = RTE_ETH_DEV_UNUSED;

-       memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+       if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+               memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));

	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);

Or _detach_secondary() may be necessary. You might want to talk to Thomas
because he is the maintainer of ethdev.

Thanks,
Yongseok