[dpdk-dev] [PATCH v2 1/2] net/mlx5: fix multiple free issue when attr ctx is NULL

[dpdk-dev] [PATCH v2 1/2] net/mlx5: fix multiple free issue when attr ctx is NULL

[Raslan Darawsheh]

When attr_ctx is NULL it will attempt to free the list of devices twice.
Avoid double freeing the list by directly going to error handling.

Fixes: 771fa900 ("mlx5: introduce new driver for Mellanox ConnectX-4 adapters")
Cc: stable@dpdk.org

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>

---
v2 changes:
 Better handle for switch statement.
 Reword the commit log.
---
---
 drivers/net/mlx5/mlx5.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 5190b9f..3831e3d 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -690,18 +690,18 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		break;
 	}
 	if (attr_ctx == NULL) {
-		mlx5_glue->free_device_list(list);
 		switch (err) {
 		case 0:
 			DRV_LOG(ERR,
 				"cannot access device, is mlx5_ib loaded?");
 			err = ENODEV;
-			goto error;
+			break;
 		case EINVAL:
 			DRV_LOG(ERR,
 				"cannot use device, are drivers up to date?");
-			goto error;
+			break;
 		}
+		goto error;
 	}
 	ibv_dev = list[i];
 	DRV_LOG(DEBUG, "device opened");
-- 
2.7.4

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

[Raslan Darawsheh]

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

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);
 		break;
 	}
 	/*
-- 
2.7.4

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

[Yongseok Koh]

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