[dpdk-dev] [PATCH 1/2] net/mlx4: fix minor resource leak during init

[dpdk-dev] [PATCH 1/2] net/mlx4: fix minor resource leak during init

[Adrien Mazarguil]

Temporary IB device context and list are not freed in case of a successful
initialization of the device.

This issue is caused by the two following commits, the first of which
causes initialization to return early, while the second one goes a bit
overboard while switching to negative errno values; an internal variable
(err) is needed to tell success from failure at the end of the function
since rte_errno is not reliable enough.

Fixes: f2318196c71a ("net/mlx4: remove limitation on number of instances")
Fixes: 9d14b27308a0 ("net/mlx4: standardize on negative errno values")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 9f8ecd072..e604c5b8c 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -589,14 +589,14 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 	ibv_dev = list[i];
 	DEBUG("device opened");
 	if (mlx4_glue->query_device(attr_ctx, &device_attr)) {
-		rte_errno = ENODEV;
+		err = ENODEV;
 		goto error;
 	}
 	INFO("%u port(s) detected", device_attr.phys_port_cnt);
 	conf.ports.present |= (UINT64_C(1) << device_attr.phys_port_cnt) - 1;
 	if (mlx4_args(pci_dev->device.devargs, &conf)) {
 		ERROR("failed to process device arguments");
-		rte_errno = EINVAL;
+		err = EINVAL;
 		goto error;
 	}
 	/* Use all ports when none are defined */
@@ -604,7 +604,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		conf.ports.enabled = conf.ports.present;
 	/* Retrieve extended device attributes. */
 	if (mlx4_glue->query_device_ex(attr_ctx, NULL, &device_attr_ex)) {
-		rte_errno = ENODEV;
+		err = ENODEV;
 		goto error;
 	}
 	assert(device_attr.max_sge >= MLX4_MAX_SGE);
@@ -623,18 +623,18 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		DEBUG("using port %u", port);
 		ctx = mlx4_glue->open_device(ibv_dev);
 		if (ctx == NULL) {
-			rte_errno = ENODEV;
+			err = ENODEV;
 			goto port_error;
 		}
 		/* Check port status. */
 		err = mlx4_glue->query_port(ctx, port, &port_attr);
 		if (err) {
-			rte_errno = err;
-			ERROR("port query failed: %s", strerror(rte_errno));
+			err = ENODEV;
+			ERROR("port query failed: %s", strerror(err));
 			goto port_error;
 		}
 		if (port_attr.link_layer != IBV_LINK_LAYER_ETHERNET) {
-			rte_errno = ENOTSUP;
+			err = ENOTSUP;
 			ERROR("port %d is not configured in Ethernet mode",
 			      port);
 			goto port_error;
@@ -644,15 +644,16 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			      port, mlx4_glue->port_state_str(port_attr.state),
 			      port_attr.state);
 		/* Make asynchronous FD non-blocking to handle interrupts. */
-		if (mlx4_fd_set_non_blocking(ctx->async_fd) < 0) {
+		err = mlx4_fd_set_non_blocking(ctx->async_fd);
+		if (err) {
 			ERROR("cannot make asynchronous FD non-blocking: %s",
-			      strerror(rte_errno));
+			      strerror(err));
 			goto port_error;
 		}
 		/* Allocate protection domain. */
 		pd = mlx4_glue->alloc_pd(ctx);
 		if (pd == NULL) {
-			rte_errno = ENOMEM;
+			err = ENOMEM;
 			ERROR("PD allocation failure");
 			goto port_error;
 		}
@@ -661,7 +662,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 				   sizeof(*priv),
 				   RTE_CACHE_LINE_SIZE);
 		if (priv == NULL) {
-			rte_errno = ENOMEM;
+			err = ENOMEM;
 			ERROR("priv allocation failure");
 			goto port_error;
 		}
@@ -691,9 +692,10 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		DEBUG("FCS stripping toggling is %ssupported",
 		      priv->hw_fcs_strip ? "" : "not ");
 		/* Configure the first MAC address by default. */
-		if (mlx4_get_mac(priv, &mac.addr_bytes)) {
+		err = mlx4_get_mac(priv, &mac.addr_bytes);
+		if (err) {
 			ERROR("cannot get MAC address, is mlx4_en loaded?"
-			      " (rte_errno: %s)", strerror(rte_errno));
+			      " (error: %s)", strerror(err));
 			goto port_error;
 		}
 		INFO("port %u MAC address is %02x:%02x:%02x:%02x:%02x:%02x",
@@ -726,8 +728,8 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			eth_dev = rte_eth_dev_allocate(name);
 		}
 		if (eth_dev == NULL) {
+			err = ENOMEM;
 			ERROR("can not allocate rte ethdev");
-			rte_errno = ENOMEM;
 			goto port_error;
 		}
 		eth_dev->data->dev_private = priv;
@@ -773,8 +775,6 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 			rte_eth_dev_release_port(eth_dev);
 		break;
 	}
-	if (i == device_attr.phys_port_cnt)
-		return 0;
 	/*
 	 * XXX if something went wrong in the loop above, there is a resource
 	 * leak (ctx, pd, priv, dpdk ethdev) but we can do nothing about it as
@@ -786,8 +786,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		claim_zero(mlx4_glue->close_device(attr_ctx));
 	if (list)
 		mlx4_glue->free_device_list(list);
-	assert(rte_errno >= 0);
-	return -rte_errno;
+	if (err)
+		rte_errno = err;
+	return -err;
 }
 
 static const struct rte_pci_id mlx4_pci_id_map[] = {
-- 
2.11.0

[dpdk-dev] [PATCH 2/2] net/mlx5: remove limitation on number of instances

[Adrien Mazarguil]

This artificial limitation was inherited from the mlx4 code base and has no
purpose other than adding unnecessary noise.

This patch is a port of commit f2318196c71a ("net/mlx4: remove limitation
on number of instances").

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Cc: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5.c | 54 +-------------------------------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 006665600..8d1c2347e 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -396,39 +396,6 @@ const struct eth_dev_ops mlx5_dev_ops_isolate = {
 	.is_removed = mlx5_is_removed,
 };
 
-static struct {
-	struct rte_pci_addr pci_addr; /* associated PCI address */
-	uint32_t ports; /* physical ports bitfield. */
-} mlx5_dev[32];
-
-/**
- * Get device index in mlx5_dev[] from PCI bus address.
- *
- * @param[in] pci_addr
- *   PCI bus address to look for.
- *
- * @return
- *   mlx5_dev[] index on success, -1 on failure.
- */
-static int
-mlx5_dev_idx(struct rte_pci_addr *pci_addr)
-{
-	unsigned int i;
-	int ret = -1;
-
-	assert(pci_addr != NULL);
-	for (i = 0; (i != RTE_DIM(mlx5_dev)); ++i) {
-		if ((mlx5_dev[i].pci_addr.domain == pci_addr->domain) &&
-		    (mlx5_dev[i].pci_addr.bus == pci_addr->bus) &&
-		    (mlx5_dev[i].pci_addr.devid == pci_addr->devid) &&
-		    (mlx5_dev[i].pci_addr.function == pci_addr->function))
-			return i;
-		if ((mlx5_dev[i].ports == 0) && (ret == -1))
-			ret = i;
-	}
-	return ret;
-}
-
 /**
  * Verify and store value for device argument.
  *
@@ -698,7 +665,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	unsigned int mprq_max_stride_size_n = 0;
 	unsigned int mprq_min_stride_num_n = 0;
 	unsigned int mprq_max_stride_num_n = 0;
-	int idx;
 	int i;
 	struct mlx5dv_context attrs_out = {0};
 #ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
@@ -708,16 +674,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	/* Prepare shared data between primary and secondary process. */
 	mlx5_prepare_shared_data();
 	assert(pci_drv == &mlx5_driver);
-	/* Get mlx5_dev[] index. */
-	idx = mlx5_dev_idx(&pci_dev->addr);
-	if (idx == -1) {
-		DRV_LOG(ERR, "this driver cannot support any more adapters");
-		err = ENOMEM;
-		goto error;
-	}
-	DRV_LOG(DEBUG, "using driver device index %d", idx);
-	/* Save PCI address. */
-	mlx5_dev[idx].pci_addr = pci_dev->addr;
 	list = mlx5_glue->get_device_list(&i);
 	if (list == NULL) {
 		assert(errno);
@@ -873,7 +829,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		char name[RTE_ETH_NAME_MAX_LEN];
 		int len;
 		uint32_t port = i + 1; /* ports are indexed from one */
-		uint32_t test = (1 << i);
 		struct ibv_context *ctx = NULL;
 		struct ibv_port_attr port_attr;
 		struct ibv_pd *pd = NULL;
@@ -908,7 +863,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 			 pci_dev->addr.devid, pci_dev->addr.function);
 		if (device_attr.orig_attr.phys_port_cnt > 1)
 			snprintf(name + len, sizeof(name), " port %u", i);
-		mlx5_dev[idx].ports |= test;
 		if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 			eth_dev = rte_eth_dev_attach_secondary(name);
 			if (eth_dev == NULL) {
@@ -948,7 +902,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 			rte_eth_dev_probing_finish(eth_dev);
 			continue;
 		}
-		DRV_LOG(DEBUG, "using port %u (%08" PRIx32 ")", port, test);
+		DRV_LOG(DEBUG, "using port %u", port);
 		ctx = mlx5_glue->open_device(ibv_dev);
 		if (ctx == NULL) {
 			err = ENODEV;
@@ -979,7 +933,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 			err = ENOMEM;
 			goto port_error;
 		}
-		mlx5_dev[idx].ports |= test;
 		/* from rte_ethdev.c */
 		priv = rte_zmalloc("ethdev private structure",
 				   sizeof(*priv),
@@ -1210,11 +1163,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	 * long as the dpdk does not provide a way to deallocate a ethdev and a
 	 * way to enumerate the registered ethdevs to free the previous ones.
 	 */
-	/* no port found, complain */
-	if (!mlx5_dev[idx].ports) {
-		rte_errno = ENODEV;
-		err = rte_errno;
-	}
 error:
 	if (attr_ctx)
 		claim_zero(mlx5_glue->close_device(attr_ctx));
-- 
2.11.0

Re: [dpdk-dev] [PATCH 2/2] net/mlx5: remove limitation on number of instances

[Yongseok Koh]

> On May 22, 2018, at 8:37 AM, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> This artificial limitation was inherited from the mlx4 code base and has no
> purpose other than adding unnecessary noise.
> 
> This patch is a port of commit f2318196c71a ("net/mlx4: remove limitation
> on number of instances").
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Cc: Yongseok Koh <yskoh@mellanox.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>
 
Thanks

Re: [dpdk-dev] [PATCH 1/2] net/mlx4: fix minor resource leak during init

[Shahaf Shuler]

Tuesday, May 22, 2018 6:37 PM, Adrien Mazarguil:
> Subject: [PATCH 1/2] net/mlx4: fix minor resource leak during init
> 
> Temporary IB device context and list are not freed in case of a successful
> initialization of the device.
> 
> This issue is caused by the two following commits, the first of which causes
> initialization to return early, while the second one goes a bit overboard while
> switching to negative errno values; an internal variable
> (err) is needed to tell success from failure at the end of the function since
> rte_errno is not reliable enough.
> 
> Fixes: f2318196c71a ("net/mlx4: remove limitation on number of instances")
> Fixes: 9d14b27308a0 ("net/mlx4: standardize on negative errno values")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Series applied to next-net-mlx, thanks.