[dpdk-dev] [PATCH] net/mlx5: allow multi probing

[dpdk-dev] [PATCH] net/mlx5: allow multi probing

[Ophir Munk]

Implement probing of a device multiple times, see [1].
Consecutive probing requests with a devargs string may contain
repetitive master and representors devices for which eth device should
be created. If an eth device already exists - silently ignore it.

[1]
Serie: ("eal: allow hotplug to skip an already probed device")
https://patches.dpdk.org/project/dpdk/list/?series=1580

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/mlx5/mlx5.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index b2b0aaa..16a8b9d 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -736,6 +736,7 @@
 	struct ether_addr mac;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int own_domain_id = 0;
+	uint16_t port_id;
 	unsigned int i;
 
 	/* Determine if this port representor is supposed to be spawned. */
@@ -758,6 +759,17 @@
 			return NULL;
 		}
 	}
+	/* Build device name */
+	if (!switch_info->representor)
+		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
+	else
+		snprintf(name, sizeof(name), "%s_representor_%u",
+			 dpdk_dev->name, switch_info->port_name);
+	/* if dev (master or representor) is already spawned - return NULL */
+	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
+		rte_errno = EBUSY;
+		return NULL;
+	}
 	/* Prepare shared data between primary and secondary process. */
 	mlx5_prepare_shared_data();
 	errno = 0;
@@ -864,11 +876,6 @@
 		DEBUG("ibv_query_device_ex() failed");
 		goto error;
 	}
-	if (!switch_info->representor)
-		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
-	else
-		snprintf(name, sizeof(name), "%s_representor_%u",
-			 dpdk_dev->name, switch_info->port_name);
 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		eth_dev = rte_eth_dev_attach_secondary(name);
@@ -1298,9 +1305,6 @@ struct mlx5_dev_spawn_data {
 	assert(pci_drv == &mlx5_driver);
 	errno = 0;
 
-	if (rte_dev_is_probed(&pci_dev->device))
-		return -EEXIST;
-
 	ibv_list = mlx5_glue->get_device_list(&ret);
 	if (!ibv_list) {
 		rte_errno = errno ? errno : ENOSYS;
@@ -1412,7 +1416,7 @@ struct mlx5_dev_spawn_data {
 		if (!list[i].eth_dev) {
 			if (rte_errno != EBUSY)
 				break;
-			/* Device is disabled, ignore it. */
+			/* Device is disabled or already spawned. Ignore it. */
 			continue;
 		}
 		restore = list[i].eth_dev->data->dev_flags;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: allow multi probing

[Yongseok Koh]

On Wed, Oct 03, 2018 at 01:01:23AM -0700, Ophir Munk wrote:
> Implement probing of a device multiple times, see [1].
> Consecutive probing requests with a devargs string may contain
> repetitive master and representors devices for which eth device should
> be created. If an eth device already exists - silently ignore it.
> 
> [1]
> Serie: ("eal: allow hotplug to skip an already probed device")

A typo? Series?

For the title, either 'multi-probing' or 'multiple probing'.
And I suggest

	net/mlx5: allow multiple probing for representor

And I have only nitpickings below ;-)

> https://patches.dpdk.org/project/dpdk/list/?series=1580
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index b2b0aaa..16a8b9d 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -736,6 +736,7 @@
>  	struct ether_addr mac;
>  	char name[RTE_ETH_NAME_MAX_LEN];
>  	int own_domain_id = 0;
> +	uint16_t port_id;
>  	unsigned int i;
>  
>  	/* Determine if this port representor is supposed to be spawned. */
> @@ -758,6 +759,17 @@
>  			return NULL;
>  		}
>  	}
> +	/* Build device name */

Better to put a period (.), if it is a sentence.

> +	if (!switch_info->representor)
> +		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> +	else
> +		snprintf(name, sizeof(name), "%s_representor_%u",
> +			 dpdk_dev->name, switch_info->port_name);
> +	/* if dev (master or representor) is already spawned - return NULL */

How about,
	/* Check if the device is already spawned. */

"return NULL" is so obvious from the code.

> +	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
> +		rte_errno = EBUSY;

Semantically, shouldn't it be EEXIST and add the condition in probe()?

> +		return NULL;
> +	}
>  	/* Prepare shared data between primary and secondary process. */
>  	mlx5_prepare_shared_data();
>  	errno = 0;
> @@ -864,11 +876,6 @@
>  		DEBUG("ibv_query_device_ex() failed");
>  		goto error;
>  	}
> -	if (!switch_info->representor)
> -		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> -	else
> -		snprintf(name, sizeof(name), "%s_representor_%u",
> -			 dpdk_dev->name, switch_info->port_name);
>  	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>  		eth_dev = rte_eth_dev_attach_secondary(name);
> @@ -1298,9 +1305,6 @@ struct mlx5_dev_spawn_data {
>  	assert(pci_drv == &mlx5_driver);
>  	errno = 0;
>  
> -	if (rte_dev_is_probed(&pci_dev->device))
> -		return -EEXIST;
> -
>  	ibv_list = mlx5_glue->get_device_list(&ret);
>  	if (!ibv_list) {
>  		rte_errno = errno ? errno : ENOSYS;
> @@ -1412,7 +1416,7 @@ struct mlx5_dev_spawn_data {
>  		if (!list[i].eth_dev) {
>  			if (rte_errno != EBUSY)

I meant this to be,
			if (rte_errno != EBUSY && rte_errno != EEXIST)


Thanks,
Yongseok

>  				break;
> -			/* Device is disabled, ignore it. */
> +			/* Device is disabled or already spawned. Ignore it. */
>  			continue;
>  		}
>  		restore = list[i].eth_dev->data->dev_flags;
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH v2] net/mlx5: allow multiple probing for representor

[Ophir Munk]

Implement probing of a device multiple times, see [1].
Consecutive probing requests with a devargs string may contain
repetitive master and representors devices for which eth device should
be created. If an eth device already exists - silently ignore it.

[1]
https://patches.dpdk.org/patch/45601/
("eal: allow probing a device again")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1:
Initial release

v2:
Rebase + code review updates
https://patches.dpdk.org/patch/45927/

 drivers/net/mlx5/mlx5.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 9131290..a5a9eca 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -693,9 +693,10 @@
  *
  * @return
  *   A valid Ethernet device object on success, NULL otherwise and rte_errno
- *   is set. The following error is defined:
+ *   is set. The following errors are defined:
  *
  *   EBUSY: device is not supposed to be spawned.
+ *   EEXIST: device is already spawned
  */
 static struct rte_eth_dev *
 mlx5_dev_spawn(struct rte_device *dpdk_dev,
@@ -744,6 +745,7 @@
 	struct ether_addr mac;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int own_domain_id = 0;
+	uint16_t port_id;
 	unsigned int i;
 
 	/* Determine if this port representor is supposed to be spawned. */
@@ -766,6 +768,17 @@
 			return NULL;
 		}
 	}
+	/* Build device name. */
+	if (!switch_info->representor)
+		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
+	else
+		snprintf(name, sizeof(name), "%s_representor_%u",
+			 dpdk_dev->name, switch_info->port_name);
+	/* check if the device is already spawned */
+	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 	/* Prepare shared data between primary and secondary process. */
 	mlx5_prepare_shared_data();
 	errno = 0;
@@ -872,11 +885,6 @@
 		DEBUG("ibv_query_device_ex() failed");
 		goto error;
 	}
-	if (!switch_info->representor)
-		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
-	else
-		snprintf(name, sizeof(name), "%s_representor_%u",
-			 dpdk_dev->name, switch_info->port_name);
 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		eth_dev = rte_eth_dev_attach_secondary(name);
@@ -1306,9 +1314,6 @@ struct mlx5_dev_spawn_data {
 	assert(pci_drv == &mlx5_driver);
 	errno = 0;
 
-	if (rte_dev_is_probed(&pci_dev->device))
-		return -EEXIST;
-
 	ibv_list = mlx5_glue->get_device_list(&ret);
 	if (!ibv_list) {
 		rte_errno = errno ? errno : ENOSYS;
@@ -1418,9 +1423,9 @@ struct mlx5_dev_spawn_data {
 		list[i].eth_dev = mlx5_dev_spawn
 			(&pci_dev->device, list[i].ibv_dev, vf, &list[i].info);
 		if (!list[i].eth_dev) {
-			if (rte_errno != EBUSY)
+			if (rte_errno != EBUSY && rte_errno != EEXIST)
 				break;
-			/* Device is disabled, ignore it. */
+			/* Device is disabled or already spawned. Ignore it. */
 			continue;
 		}
 		restore = list[i].eth_dev->data->dev_flags;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: allow multi probing

[Ophir Munk]

v2 sent

> -----Original Message-----
> From: Yongseok Koh
> Sent: Friday, October 05, 2018 4:42 AM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Olga Shern <olgas@mellanox.com>
> Subject: Re: [PATCH] net/mlx5: allow multi probing
> 
> On Wed, Oct 03, 2018 at 01:01:23AM -0700, Ophir Munk wrote:
> > Implement probing of a device multiple times, see [1].
> > Consecutive probing requests with a devargs string may contain
> > repetitive master and representors devices for which eth device should
> > be created. If an eth device already exists - silently ignore it.
> >
> > [1]
> > Serie: ("eal: allow hotplug to skip an already probed device")
> 
> A typo? Series?
> 
> For the title, either 'multi-probing' or 'multiple probing'.
> And I suggest
> 
> 	net/mlx5: allow multiple probing for representor
> 
> And I have only nitpickings below ;-)
> 
> > https://patches.dpdk.org/project/dpdk/list/?series=1580
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > b2b0aaa..16a8b9d 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -736,6 +736,7 @@
> >  	struct ether_addr mac;
> >  	char name[RTE_ETH_NAME_MAX_LEN];
> >  	int own_domain_id = 0;
> > +	uint16_t port_id;
> >  	unsigned int i;
> >
> >  	/* Determine if this port representor is supposed to be spawned. */
> > @@ -758,6 +759,17 @@
> >  			return NULL;
> >  		}
> >  	}
> > +	/* Build device name */
> 
> Better to put a period (.), if it is a sentence.
> 
> > +	if (!switch_info->representor)
> > +		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> > +	else
> > +		snprintf(name, sizeof(name), "%s_representor_%u",
> > +			 dpdk_dev->name, switch_info->port_name);
> > +	/* if dev (master or representor) is already spawned - return NULL
> > +*/
> 
> How about,
> 	/* Check if the device is already spawned. */
> 
> "return NULL" is so obvious from the code.
> 
> > +	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
> > +		rte_errno = EBUSY;
> 
> Semantically, shouldn't it be EEXIST and add the condition in probe()?
> 
> > +		return NULL;
> > +	}
> >  	/* Prepare shared data between primary and secondary process. */
> >  	mlx5_prepare_shared_data();
> >  	errno = 0;
> > @@ -864,11 +876,6 @@
> >  		DEBUG("ibv_query_device_ex() failed");
> >  		goto error;
> >  	}
> > -	if (!switch_info->representor)
> > -		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> > -	else
> > -		snprintf(name, sizeof(name), "%s_representor_%u",
> > -			 dpdk_dev->name, switch_info->port_name);
> >  	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
> >  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> >  		eth_dev = rte_eth_dev_attach_secondary(name);
> > @@ -1298,9 +1305,6 @@ struct mlx5_dev_spawn_data {
> >  	assert(pci_drv == &mlx5_driver);
> >  	errno = 0;
> >
> > -	if (rte_dev_is_probed(&pci_dev->device))
> > -		return -EEXIST;
> > -
> >  	ibv_list = mlx5_glue->get_device_list(&ret);
> >  	if (!ibv_list) {
> >  		rte_errno = errno ? errno : ENOSYS; @@ -1412,7 +1416,7
> @@ struct
> > mlx5_dev_spawn_data {
> >  		if (!list[i].eth_dev) {
> >  			if (rte_errno != EBUSY)
> 
> I meant this to be,
> 			if (rte_errno != EBUSY && rte_errno != EEXIST)
> 
> 
> Thanks,
> Yongseok
> 
> >  				break;
> > -			/* Device is disabled, ignore it. */
> > +			/* Device is disabled or already spawned. Ignore it.
> */
> >  			continue;
> >  		}
> >  		restore = list[i].eth_dev->data->dev_flags;
> > --
> > 1.8.3.1
> >

Re: [dpdk-dev] [PATCH v2] net/mlx5: allow multiple probing for representor

[Yongseok Koh]

> On Oct 5, 2018, at 4:06 AM, Ophir Munk <ophirmu@mellanox.com> wrote:
> 
> Implement probing of a device multiple times, see [1].
> Consecutive probing requests with a devargs string may contain
> repetitive master and representors devices for which eth device should
> be created. If an eth device already exists - silently ignore it.
> 
> [1]
> https://patches.dpdk.org/patch/45601/
> ("eal: allow probing a device again")
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>
 
Thanks

> v1:
> Initial release
> 
> v2:
> Rebase + code review updates
> https://patches.dpdk.org/patch/45927/
> 
> drivers/net/mlx5/mlx5.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 9131290..a5a9eca 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -693,9 +693,10 @@
>  *
>  * @return
>  *   A valid Ethernet device object on success, NULL otherwise and rte_errno
> - *   is set. The following error is defined:
> + *   is set. The following errors are defined:
>  *
>  *   EBUSY: device is not supposed to be spawned.
> + *   EEXIST: device is already spawned
>  */
> static struct rte_eth_dev *
> mlx5_dev_spawn(struct rte_device *dpdk_dev,
> @@ -744,6 +745,7 @@
> 	struct ether_addr mac;
> 	char name[RTE_ETH_NAME_MAX_LEN];
> 	int own_domain_id = 0;
> +	uint16_t port_id;
> 	unsigned int i;
> 
> 	/* Determine if this port representor is supposed to be spawned. */
> @@ -766,6 +768,17 @@
> 			return NULL;
> 		}
> 	}
> +	/* Build device name. */
> +	if (!switch_info->representor)
> +		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> +	else
> +		snprintf(name, sizeof(name), "%s_representor_%u",
> +			 dpdk_dev->name, switch_info->port_name);
> +	/* check if the device is already spawned */
> +	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
> +		rte_errno = EEXIST;
> +		return NULL;
> +	}
> 	/* Prepare shared data between primary and secondary process. */
> 	mlx5_prepare_shared_data();
> 	errno = 0;
> @@ -872,11 +885,6 @@
> 		DEBUG("ibv_query_device_ex() failed");
> 		goto error;
> 	}
> -	if (!switch_info->representor)
> -		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> -	else
> -		snprintf(name, sizeof(name), "%s_representor_%u",
> -			 dpdk_dev->name, switch_info->port_name);
> 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
> 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> 		eth_dev = rte_eth_dev_attach_secondary(name);
> @@ -1306,9 +1314,6 @@ struct mlx5_dev_spawn_data {
> 	assert(pci_drv == &mlx5_driver);
> 	errno = 0;
> 
> -	if (rte_dev_is_probed(&pci_dev->device))
> -		return -EEXIST;
> -
> 	ibv_list = mlx5_glue->get_device_list(&ret);
> 	if (!ibv_list) {
> 		rte_errno = errno ? errno : ENOSYS;
> @@ -1418,9 +1423,9 @@ struct mlx5_dev_spawn_data {
> 		list[i].eth_dev = mlx5_dev_spawn
> 			(&pci_dev->device, list[i].ibv_dev, vf, &list[i].info);
> 		if (!list[i].eth_dev) {
> -			if (rte_errno != EBUSY)
> +			if (rte_errno != EBUSY && rte_errno != EEXIST)
> 				break;
> -			/* Device is disabled, ignore it. */
> +			/* Device is disabled or already spawned. Ignore it. */
> 			continue;
> 		}
> 		restore = list[i].eth_dev->data->dev_flags;
> -- 
> 1.8.3.1
> 

Re: [dpdk-dev] [PATCH v2] net/mlx5: allow multiple probing for representor

[Ophir Munk]

PATCH v3 is expected to accommodate more updates in hotplug series by Thomas.
Please do not merge this patch.

> -----Original Message-----
> From: Yongseok Koh
> Sent: Friday, October 05, 2018 9:06 PM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Olga Shern <olgas@mellanox.com>
> Subject: Re: [PATCH v2] net/mlx5: allow multiple probing for representor
> 
> > On Oct 5, 2018, at 4:06 AM, Ophir Munk <ophirmu@mellanox.com> wrote:
> >
> > Implement probing of a device multiple times, see [1].
> > Consecutive probing requests with a devargs string may contain
> > repetitive master and representors devices for which eth device should
> > be created. If an eth device already exists - silently ignore it.
> >
> > [1]
> > https://patches.dpdk.org/patch/45601/
> > ("eal: allow probing a device again")
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> 
> Thanks
> 
> > v1:
> > Initial release
> >
> > v2:
> > Rebase + code review updates
> > https://patches.dpdk.org/patch/45927/
> >
> > drivers/net/mlx5/mlx5.c | 27 ++++++++++++++++-----------
> > 1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 9131290..a5a9eca 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -693,9 +693,10 @@
> >  *
> >  * @return
> >  *   A valid Ethernet device object on success, NULL otherwise and
> rte_errno
> > - *   is set. The following error is defined:
> > + *   is set. The following errors are defined:
> >  *
> >  *   EBUSY: device is not supposed to be spawned.
> > + *   EEXIST: device is already spawned
> >  */
> > static struct rte_eth_dev *
> > mlx5_dev_spawn(struct rte_device *dpdk_dev, @@ -744,6 +745,7 @@
> > 	struct ether_addr mac;
> > 	char name[RTE_ETH_NAME_MAX_LEN];
> > 	int own_domain_id = 0;
> > +	uint16_t port_id;
> > 	unsigned int i;
> >
> > 	/* Determine if this port representor is supposed to be spawned. */
> > @@ -766,6 +768,17 @@
> > 			return NULL;
> > 		}
> > 	}
> > +	/* Build device name. */
> > +	if (!switch_info->representor)
> > +		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> > +	else
> > +		snprintf(name, sizeof(name), "%s_representor_%u",
> > +			 dpdk_dev->name, switch_info->port_name);
> > +	/* check if the device is already spawned */
> > +	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
> > +		rte_errno = EEXIST;
> > +		return NULL;
> > +	}
> > 	/* Prepare shared data between primary and secondary process. */
> > 	mlx5_prepare_shared_data();
> > 	errno = 0;
> > @@ -872,11 +885,6 @@
> > 		DEBUG("ibv_query_device_ex() failed");
> > 		goto error;
> > 	}
> > -	if (!switch_info->representor)
> > -		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
> > -	else
> > -		snprintf(name, sizeof(name), "%s_representor_%u",
> > -			 dpdk_dev->name, switch_info->port_name);
> > 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
> > 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > 		eth_dev = rte_eth_dev_attach_secondary(name);
> > @@ -1306,9 +1314,6 @@ struct mlx5_dev_spawn_data {
> > 	assert(pci_drv == &mlx5_driver);
> > 	errno = 0;
> >
> > -	if (rte_dev_is_probed(&pci_dev->device))
> > -		return -EEXIST;
> > -
> > 	ibv_list = mlx5_glue->get_device_list(&ret);
> > 	if (!ibv_list) {
> > 		rte_errno = errno ? errno : ENOSYS;
> > @@ -1418,9 +1423,9 @@ struct mlx5_dev_spawn_data {
> > 		list[i].eth_dev = mlx5_dev_spawn
> > 			(&pci_dev->device, list[i].ibv_dev, vf, &list[i].info);
> > 		if (!list[i].eth_dev) {
> > -			if (rte_errno != EBUSY)
> > +			if (rte_errno != EBUSY && rte_errno != EEXIST)
> > 				break;
> > -			/* Device is disabled, ignore it. */
> > +			/* Device is disabled or already spawned. Ignore it.
> */
> > 			continue;
> > 		}
> > 		restore = list[i].eth_dev->data->dev_flags;
> > --
> > 1.8.3.1
> >

[dpdk-dev] [PATCH v3] net/mlx5: allow multiple probing for representor

[Ophir Munk]

Implement probing of a rte device multiple times, see [1].
Set PCI driver RTE_PCI_DRV_PROBE_AGAIN flag to enable multiple probing
of the PCI device by the PCI common driver.
Consecutive probing requests with a devargs string may contain
repetitive master and representors devices for which eth device should
be created only once. In case an eth device already exists - silently
ignore it.

[1]
https://patches.dpdk.org/patch/45601/
("eal: allow probing a device again")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1:
Initial release

v2:
Rebase + code review updates
https://patches.dpdk.org/patch/45927/

v3:
- Set PCI driver RTE_PCI_DRV_PROBE_AGAIN flag required by PCI common driver
- Update commit message

 drivers/net/mlx5/mlx5.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 2821aec..c08323c 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -696,9 +696,10 @@
  *
  * @return
  *   A valid Ethernet device object on success, NULL otherwise and rte_errno
- *   is set. The following error is defined:
+ *   is set. The following errors are defined:
  *
  *   EBUSY: device is not supposed to be spawned.
+ *   EEXIST: device is already spawned
  */
 static struct rte_eth_dev *
 mlx5_dev_spawn(struct rte_device *dpdk_dev,
@@ -747,6 +748,7 @@
 	struct ether_addr mac;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int own_domain_id = 0;
+	uint16_t port_id;
 	unsigned int i;
 
 	/* Determine if this port representor is supposed to be spawned. */
@@ -769,6 +771,17 @@
 			return NULL;
 		}
 	}
+	/* Build device name. */
+	if (!switch_info->representor)
+		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
+	else
+		snprintf(name, sizeof(name), "%s_representor_%u",
+			 dpdk_dev->name, switch_info->port_name);
+	/* check if the device is already spawned */
+	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 	/* Prepare shared data between primary and secondary process. */
 	mlx5_prepare_shared_data();
 	errno = 0;
@@ -875,11 +888,6 @@
 		DEBUG("ibv_query_device_ex() failed");
 		goto error;
 	}
-	if (!switch_info->representor)
-		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
-	else
-		snprintf(name, sizeof(name), "%s_representor_%u",
-			 dpdk_dev->name, switch_info->port_name);
 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		eth_dev = rte_eth_dev_attach_secondary(name);
@@ -1423,9 +1431,9 @@ struct mlx5_dev_spawn_data {
 		list[i].eth_dev = mlx5_dev_spawn
 			(&pci_dev->device, list[i].ibv_dev, vf, &list[i].info);
 		if (!list[i].eth_dev) {
-			if (rte_errno != EBUSY)
+			if (rte_errno != EBUSY && rte_errno != EEXIST)
 				break;
-			/* Device is disabled, ignore it. */
+			/* Device is disabled or already spawned. Ignore it. */
 			continue;
 		}
 		restore = list[i].eth_dev->data->dev_flags;
@@ -1520,7 +1528,8 @@ struct mlx5_dev_spawn_data {
 	},
 	.id_table = mlx5_pci_id_map,
 	.probe = mlx5_pci_probe,
-	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV,
+	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV |
+			RTE_PCI_DRV_PROBE_AGAIN,
 };
 
 #ifdef RTE_LIBRTE_MLX5_DLOPEN_DEPS
-- 
1.8.3.1

[dpdk-dev] [PATCH v4 1/3] net/mlx5: allow multiple probing for representor

[Ophir Munk]

Implement probing of a rte device multiple times, see [1].
Set PCI driver RTE_PCI_DRV_PROBE_AGAIN flag to enable multiple probing
of the PCI device by the PCI common driver.
Consecutive probing requests with a devargs string may contain
repetitive master and representors devices for which eth device should
be created only once. In case an eth device already exists - silently
ignore it.

[1]
commit e9d159c3d534 ("eal: allow probing a device again")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1:
Initial release

v2:
Rebase + code review updates
https://patches.dpdk.org/patch/45927/

v3:
- Set PCI driver RTE_PCI_DRV_PROBE_AGAIN flag required by PCI common driver
- Update commit message

v4:
Update commit message. Specify reference [1] as commit number

 drivers/net/mlx5/mlx5.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 297cbff..93b4057 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -694,9 +694,10 @@
  *
  * @return
  *   A valid Ethernet device object on success, NULL otherwise and rte_errno
- *   is set. The following error is defined:
+ *   is set. The following errors are defined:
  *
  *   EBUSY: device is not supposed to be spawned.
+ *   EEXIST: device is already spawned
  */
 static struct rte_eth_dev *
 mlx5_dev_spawn(struct rte_device *dpdk_dev,
@@ -745,6 +746,7 @@
 	struct ether_addr mac;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int own_domain_id = 0;
+	uint16_t port_id;
 	unsigned int i;
 
 	/* Determine if this port representor is supposed to be spawned. */
@@ -767,6 +769,17 @@
 			return NULL;
 		}
 	}
+	/* Build device name. */
+	if (!switch_info->representor)
+		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
+	else
+		snprintf(name, sizeof(name), "%s_representor_%u",
+			 dpdk_dev->name, switch_info->port_name);
+	/* check if the device is already spawned */
+	if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 	/* Prepare shared data between primary and secondary process. */
 	mlx5_prepare_shared_data();
 	errno = 0;
@@ -873,11 +886,6 @@
 		DEBUG("ibv_query_device_ex() failed");
 		goto error;
 	}
-	if (!switch_info->representor)
-		rte_strlcpy(name, dpdk_dev->name, sizeof(name));
-	else
-		snprintf(name, sizeof(name), "%s_representor_%u",
-			 dpdk_dev->name, switch_info->port_name);
 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		eth_dev = rte_eth_dev_attach_secondary(name);
@@ -1421,9 +1429,9 @@ struct mlx5_dev_spawn_data {
 		list[i].eth_dev = mlx5_dev_spawn
 			(&pci_dev->device, list[i].ibv_dev, vf, &list[i].info);
 		if (!list[i].eth_dev) {
-			if (rte_errno != EBUSY)
+			if (rte_errno != EBUSY && rte_errno != EEXIST)
 				break;
-			/* Device is disabled, ignore it. */
+			/* Device is disabled or already spawned. Ignore it. */
 			continue;
 		}
 		restore = list[i].eth_dev->data->dev_flags;
@@ -1518,7 +1526,8 @@ struct mlx5_dev_spawn_data {
 	},
 	.id_table = mlx5_pci_id_map,
 	.probe = mlx5_pci_probe,
-	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV,
+	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV |
+			RTE_PCI_DRV_PROBE_AGAIN,
 };
 
 #ifdef RTE_LIBRTE_MLX5_DLOPEN_DEPS
-- 
1.8.3.1

[dpdk-dev] [PATCH v4 2/3] net/mlx5: release port on close

[Ophir Munk]

With the introduction of representors several eth devices are using
the same rte device (e.g. a PCI bus). It is therefore required to
release the eth device resources during an eth device close operation
rather than during an rte device removal (detach) operation.
In current version many PMDs are still releasing the eth device as
part of the rte device removal. In order to allow a smooth transition
for all PMDs to behave correctly an ethdev flag RTE_ETH_DEV_CLOSE_REMOVE
is used. When this flag is set it indicates to rte_eth_dev_close() to
call rte_eth_dev_release_port(), so the port is freed during the close
operation.

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1-v3
This patch was not part of the serie

v4:
Initial version

 drivers/net/mlx5/mlx5.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 93b4057..7a8b45a 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -337,6 +337,17 @@
 	}
 	memset(priv, 0, sizeof(*priv));
 	priv->domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
+	/*
+	 * flag to rte_eth_dev_close() that it should release the port resources
+	 * (calling rte_eth_dev_release_port()) in addition to closing it.
+	 */
+	dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+	/*
+	 * Reset mac_addrs to NULL such that it is not freed as part of
+	 * rte_eth_dev_release_port(). mac_addrs is part of dev_private so
+	 * it is freed when dev_private is freed.
+	 */
+	dev->data->mac_addrs = NULL;
 }
 
 const struct eth_dev_ops mlx5_dev_ops = {
-- 
1.8.3.1

[dpdk-dev] [PATCH v4 3/3] net/mlx5: close all ports on remove

[Ophir Munk]

With the introduction of representors several eth devices are using
the same rte device (e.g. a PCI bus). When calling port detach on one
eth device it is required that all eth devices belonging to the
same rte device have been closed in advance, then the rte device
itself can be removed/detached.
This commit implements this requirement implicitly by adding a
remove callback to struct rte_pci_driver.
The new behavior can be demonstrated in testpmd.
First we attach a representor 0 using PCI address 0000:08:00.0
testpmd> port attach  0000:08:00.0,representor=[0]
Attaching a new port...
EAL: PCI device 0000:08:00.0 on NUMA socket 0
EAL:   probe driver: 15b3:1013 net_mlx5
Port 0 is attached.
Done
Port 1 is attached.
Done

Port 0 is the master device (PF) - an ethdev of the PCI address.
Port 1 is representor 0 - another ethdev (representing a VF) using the
same PCI address. Next we detach port 1
testpmd> port detach 1
Removing a device...
Port 0 is closed
Port 1 is closed
Now total ports is 0
Done

Since port 0 has been implicitly closed we cannot act on it anymore.
testpmd> port stop 0
Invalid port 0

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1-v3
This patch was not part of the serie

v4:
Initial version

 drivers/net/mlx5/mlx5.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 7a8b45a..9f7a099 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1485,6 +1485,22 @@ struct mlx5_dev_spawn_data {
 	return ret;
 }
 
+static int
+mlx5_pci_remove(struct rte_pci_device *pci_dev)
+{
+	uint16_t port_id;
+	struct rte_eth_dev *port;
+
+	for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
+		port = &rte_eth_devices[port_id];
+		if (port->state != RTE_ETH_DEV_UNUSED &&
+				port->device == &pci_dev->device)
+			rte_eth_dev_close(port_id);
+	}
+
+	return 0;
+}
+
 static const struct rte_pci_id mlx5_pci_id_map[] = {
 	{
 		RTE_PCI_DEVICE(PCI_VENDOR_ID_MELLANOX,
@@ -1537,6 +1553,7 @@ struct mlx5_dev_spawn_data {
 	},
 	.id_table = mlx5_pci_id_map,
 	.probe = mlx5_pci_probe,
+	.remove = mlx5_pci_remove,
 	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV |
 			RTE_PCI_DRV_PROBE_AGAIN,
 };
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4 1/3] net/mlx5: allow multiple probing for representor

[Shahaf Shuler]

Tuesday, October 23, 2018 9:26 PM, Ophir Munk:
> Subject: [PATCH v4 1/3] net/mlx5: allow multiple probing for representor
> 
> Implement probing of a rte device multiple times, see [1].
> Set PCI driver RTE_PCI_DRV_PROBE_AGAIN flag to enable multiple probing
> of the PCI device by the PCI common driver.
> Consecutive probing requests with a devargs string may contain repetitive
> master and representors devices for which eth device should be created only
> once. In case an eth device already exists - silently ignore it.
> 
> [1]
> commit e9d159c3d534 ("eal: allow probing a device again")
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>

Series applied to next-net-mlx, thanks.