[PATCH 1/1] net/mlx4: add port parameter

[PATCH 1/1] net/mlx4: add port parameter

[Gaetan Rivet]

Most ConnectX-3 adapters expose two physical ports on a single PCI bus
address.

Add a new port parameter allowing the user to choose
either or both physical ports to be used by the application.

This parameter is used as follows:

Selecting only the second port:
   -w 00:00.0,port=1

Selecting both ports:
   -w 00:00.0,port=0,port=1

If no parameter is given, the default behavior is unchanged: all ports
are probed.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 doc/guides/nics/mlx4.rst  |   6 +++
 drivers/net/mlx4/Makefile |   1 +
 drivers/net/mlx4/mlx4.c   | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/mlx4/mlx4.h   |   6 +++
 4 files changed, 117 insertions(+)

diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst
index 9a2e973..7992735 100644
--- a/doc/guides/nics/mlx4.rst
+++ b/doc/guides/nics/mlx4.rst
@@ -162,6 +162,12 @@ Run-time configuration
 
 - **ethtool** operations on related kernel interfaces also affect the PMD.
 
+- ``port`` parameter [int]
+
+  This parameter provides a physical port to probe and can be specified multiple
+  times for additional ports. All ports are probed by default if left
+  unspecified.
+
 Kernel module parameters
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/drivers/net/mlx4/Makefile b/drivers/net/mlx4/Makefile
index 1d463f7..f0ee282 100644
--- a/drivers/net/mlx4/Makefile
+++ b/drivers/net/mlx4/Makefile
@@ -43,6 +43,7 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_ether
 DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_mbuf
 DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_eal
 DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_mempool
+DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_kvargs
 
 # Basic CFLAGS.
 CFLAGS += -O3
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 35a680c..c4b9fbd 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -83,6 +83,7 @@
 #include <rte_alarm.h>
 #include <rte_memory.h>
 #include <rte_flow.h>
+#include <rte_kvargs.h>
 
 /* Generated configuration header. */
 #include "mlx4_autoconf.h"
@@ -125,6 +126,16 @@ struct mlx4_secondary_data {
 	rte_spinlock_t lock; /* Port configuration lock. */
 } mlx4_secondary_data[RTE_MAX_ETHPORTS];
 
+struct mlx4_conf {
+	uint8_t active_ports;
+};
+
+/* Available parameters list. */
+const char *pmd_mlx4_init_params[] = {
+	MLX4_PMD_PORT_KVARG,
+	NULL,
+};
+
 /**
  * Check if running as a secondary process.
  *
@@ -5396,6 +5407,84 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
 	}
 }
 
+/**
+ * Verify and store value for device argument.
+ *
+ * @param[in] key
+ *   Key argument to verify.
+ * @param[in] val
+ *   Value associated with key.
+ * @param out
+ *   User data.
+ *
+ * @return
+ *   0 on success, negative errno value on failure.
+ */
+static int
+mlx4_arg_parse(const char *key, const char *val, void *out)
+{
+	struct mlx4_conf *conf = out;
+	unsigned long tmp;
+
+	errno = 0;
+	tmp = strtoul(val, NULL, 0);
+	if (errno) {
+		WARN("%s: \"%s\" is not a valid integer", key, val);
+		return -errno;
+	}
+	if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
+		if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
+			ERROR("invalid port index %lu (max: %u)",
+				tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
+			return -EINVAL;
+		}
+		conf->active_ports |= 1 << tmp;
+	} else {
+		WARN("%s: unknown parameter", key);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/**
+ * Parse device parameters.
+ *
+ * @param devargs
+ *   Device arguments structure.
+ *
+ * @return
+ *   0 on success, negative errno value on failure.
+ */
+static int
+mlx4_args(struct rte_devargs *devargs, struct mlx4_conf *conf)
+{
+	struct rte_kvargs *kvlist;
+	unsigned arg_count;
+	int ret = 0;
+	int i;
+
+	if (devargs == NULL)
+		return 0;
+	kvlist = rte_kvargs_parse(devargs->args, pmd_mlx4_init_params);
+	if (kvlist == NULL) {
+		ERROR("failed to parse kvargs");
+		return -EINVAL;
+	}
+	/* Process parameters. */
+	for (i = 0; pmd_mlx4_init_params[i]; ++i) {
+		arg_count = rte_kvargs_count(kvlist, MLX4_PMD_PORT_KVARG);
+		while (arg_count-- > 0) {
+			ret = rte_kvargs_process(kvlist, MLX4_PMD_PORT_KVARG,
+					mlx4_arg_parse, conf);
+			if (ret != 0)
+				goto free_kvlist;
+		}
+	}
+free_kvlist:
+	rte_kvargs_free(kvlist);
+	return ret;
+}
+
 static struct eth_driver mlx4_driver;
 
 /**
@@ -5420,6 +5509,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 	int err = 0;
 	struct ibv_context *attr_ctx = NULL;
 	struct ibv_device_attr device_attr;
+	struct mlx4_conf conf = {
+		.active_ports = 0,
+	};
 	unsigned int vf;
 	int idx;
 	int i;
@@ -5490,6 +5582,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		goto error;
 	INFO("%u port(s) detected", device_attr.phys_port_cnt);
 
+	if (mlx4_args(pci_dev->device.devargs, &conf)) {
+		ERROR("failed to process device arguments");
+		goto error;
+	}
+	/* Use all ports when none are defined */
+	if (conf.active_ports == 0) {
+		for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
+			conf.active_ports |= 1 << i;
+	}
 	for (i = 0; i < device_attr.phys_port_cnt; i++) {
 		uint32_t port = i + 1; /* ports are indexed from one */
 		uint32_t test = (1 << i);
@@ -5503,6 +5604,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 #endif /* HAVE_EXP_QUERY_DEVICE */
 		struct ether_addr mac;
 
+		/* If port is not active, skip. */
+		if (!(conf.active_ports & (1 << i)))
+			continue;
 #ifdef HAVE_EXP_QUERY_DEVICE
 		exp_device_attr.comp_mask = IBV_EXP_DEVICE_ATTR_EXP_CAP_FLAGS;
 #ifdef RSS_SUPPORT
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index 877ed79..e9659eb 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -81,6 +81,9 @@
 /* Request send completion once in every 64 sends, might be less. */
 #define MLX4_PMD_TX_PER_COMP_REQ 64
 
+/* Maximum number of physical ports. */
+#define MLX4_PMD_MAX_PHYS_PORTS 2
+
 /* Maximum number of Scatter/Gather Elements per Work Request. */
 #ifndef MLX4_PMD_SGE_WR_N
 #define MLX4_PMD_SGE_WR_N 4
@@ -113,6 +116,9 @@
 /* Alarm timeout. */
 #define MLX4_ALARM_TIMEOUT_US 100000
 
+/* Port parameter. */
+#define MLX4_PMD_PORT_KVARG "port"
+
 enum {
 	PCI_VENDOR_ID_MELLANOX = 0x15b3,
 };
-- 
2.1.4

Re: [PATCH 1/1] net/mlx4: add port parameter

[Legacy, Allain]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
> Sent: Friday, March 03, 2017 10:40 AM
...
> +	errno = 0;
> +	tmp = strtoul(val, NULL, 0);
The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number. 

    char *end = NULL;
    tmp = strtoull(val, &end, 0);
    if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))


> +	if (errno) {
> +		WARN("%s: \"%s\" is not a valid integer", key, val);
> +		return -errno;
> +	}
> +	if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
> +		if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
> +			ERROR("invalid port index %lu (max: %u)",
> +				tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
> +			return -EINVAL;
> +		}
> +		conf->active_ports |= 1 << tmp;
> +	} else {
> +		WARN("%s: unknown parameter", key);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
The usage of strtoul() should be moved to be within the strcmp(MLX4_PMD_PORT_KVARG, key) IF statement.  That way the "val" would only be parsed if "key" is "port" and it is expected that "val" is an integer.


> +	if (mlx4_args(pci_dev->device.devargs, &conf)) {
> +		ERROR("failed to process device arguments");
> +		goto error;
> +	}
It would be helpful for debugging if the error message included the devargs so that we can see what is wrong with the input. 


> +	/* Use all ports when none are defined */
> +	if (conf.active_ports == 0) {
> +		for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
> +			conf.active_ports |= 1 << i;
> +	}

Rather than use a loop to populate all active fields would a #define with an all ports mask be better suited to this.  Or alternatively just change the IF statement below to use the following and avoid the need for this loop altogether:

if (conf.active_ports & !(conf.active_ports & (1 << i)))
	continue;

Re: [PATCH 1/1] net/mlx4: add port parameter

[Stephen Hemminger]

On Fri, 10 Mar 2017 16:24:32 +0000
"Legacy, Allain" <Allain.Legacy@windriver.com> wrote:

> The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number. 
> 
>     char *end = NULL;
>     tmp = strtoull(val, &end, 0);
>     if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))

Extra () no necessary here.
Also errno is not set unless the return value is ULLONG_MAX. It will be last
value.

Something like:
	tmp = strtoull(val, &end, 0);
	if (!*val || !*end || (tmp == ULLONG_MAX && errno))
...

       If  endptr  is  not  NULL,  strtoul()  stores  the address of the first
       invalid character in *endptr.  If there were no  digits  at  all,  str‐
       toul()  stores  the  original value of nptr in *endptr (and returns 0).
       In particular, if *nptr is not '\0' but **endptr is '\0' on return, the
       entire string is valid.
...

RETURN VALUE
       The strtoul() function returns either the result of the conversion  or,
       if  there  was  a leading minus sign, the negation of the result of the
       conversion represented as an unsigned value, unless the original  (non‐
       negated)  value  would  overflow; in the latter case, strtoul() returns
       ULONG_MAX and sets errno to ERANGE.  Precisely the same holds for  str‐
       toull() (with ULLONG_MAX instead of ULONG_MAX).

Re: [PATCH 1/1] net/mlx4: add port parameter

[Gaëtan Rivet]

Hey, thanks for reading.

On Fri, Mar 10, 2017 at 04:24:32PM +0000, Legacy, Allain wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
>> Sent: Friday, March 03, 2017 10:40 AM
>...
>> +	errno = 0;
>> +	tmp = strtoul(val, NULL, 0);
>The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number.
>
>    char *end = NULL;
>    tmp = strtoull(val, &end, 0);
>    if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
>
>

Thanks for the suggestion, I'd keep the strtoul though ;).
I will see into it for the v2, keeping in mind Stephen's remarks as 
well.

>> +	if (errno) {
>> +		WARN("%s: \"%s\" is not a valid integer", key, val);
>> +		return -errno;
>> +	}
>> +	if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
>> +		if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
>> +			ERROR("invalid port index %lu (max: %u)",
>> +				tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
>> +			return -EINVAL;
>> +		}
>> +		conf->active_ports |= 1 << tmp;
>> +	} else {
>> +		WARN("%s: unknown parameter", key);
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>The usage of strtoul() should be moved to be within the strcmp(MLX4_PMD_PORT_KVARG, key) IF statement.  That way the "val" would only be parsed if "key" is "port" and it is expected that "val" is an integer.
>
>

This function was aimed at being generic.
If we consider that no other parameter would ever be added, then the 
strcmp should be scraped altogether, as this callback is only called 
upon parsing this parameter in the kvlist in the first place.

But we are in the control path, avoiding a useless strtoul at the price 
of making the function less useful seems an unnecessary tradeoff to me.

>> +	if (mlx4_args(pci_dev->device.devargs, &conf)) {
>> +		ERROR("failed to process device arguments");
>> +		goto error;
>> +	}
>It would be helpful for debugging if the error message included the devargs so that we can see what is wrong with the input.
>
>

Agreed.

>> +	/* Use all ports when none are defined */
>> +	if (conf.active_ports == 0) {
>> +		for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
>> +			conf.active_ports |= 1 << i;
>> +	}
>
>Rather than use a loop to populate all active fields would a #define with an all ports mask be better suited to this.  Or alternatively just change the IF statement below to use the following and avoid the need for this loop altogether:
>
>if (conf.active_ports & !(conf.active_ports & (1 << i)))
>	continue;
>
>

I do not agree with removing this loop.

Your second solution will scatter the relevant bits concerning the 
default value of the active_port configuration option. While being 
slightly slicker it hides it unnecessarily from the reader.

The first solution might be interesting, however it makes this option 
dependent on two defines instead of one. If one had to change the 
default MAX_PHYS_PORT value for mlx4 (however unlikely it might be), 
then they would have to change the valid ALL_PORTS mask as well. In 
principle this contradicts DRY[1].

[1]: https://en.wikipedia.org/wiki/Don't_repeat_yourself

-- 
Gaëtan Rivet
6WIND

Re: [PATCH 1/1] net/mlx4: add port parameter

[Gaëtan Rivet]

slight additional remark below.

On Fri, Mar 10, 2017 at 06:11:59PM +0100, Gaëtan Rivet wrote:
>Hey, thanks for reading.
>
>On Fri, Mar 10, 2017 at 04:24:32PM +0000, Legacy, Allain wrote:
>>>-----Original Message-----
>>>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
>>>Sent: Friday, March 03, 2017 10:40 AM
>>...
>>>+	errno = 0;
>>>+	tmp = strtoul(val, NULL, 0);
>>The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number.
>>
>>   char *end = NULL;
>>   tmp = strtoull(val, &end, 0);
>>   if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
>>
>>
>
>Thanks for the suggestion, I'd keep the strtoul though ;).
>I will see into it for the v2, keeping in mind Stephen's remarks as 
>well.
>
>>>+	if (errno) {
>>>+		WARN("%s: \"%s\" is not a valid integer", key, val);
>>>+		return -errno;
>>>+	}
>>>+	if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
>>>+		if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
>>>+			ERROR("invalid port index %lu (max: %u)",
>>>+				tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
>>>+			return -EINVAL;
>>>+		}
>>>+		conf->active_ports |= 1 << tmp;
>>>+	} else {
>>>+		WARN("%s: unknown parameter", key);
>>>+		return -EINVAL;
>>>+	}
>>>+	return 0;
>>>+}
>>The usage of strtoul() should be moved to be within the strcmp(MLX4_PMD_PORT_KVARG, key) IF statement.  That way the "val" would only be parsed if "key" is "port" and it is expected that "val" is an integer.
>>
>>
>
>This function was aimed at being generic.
>If we consider that no other parameter would ever be added, then the 
>strcmp should be scraped altogether, as this callback is only called 
>upon parsing this parameter in the kvlist in the first place.
>
>But we are in the control path, avoiding a useless strtoul at the 
>price of making the function less useful seems an unnecessary tradeoff 
>to me.
>
>>>+	if (mlx4_args(pci_dev->device.devargs, &conf)) {
>>>+		ERROR("failed to process device arguments");
>>>+		goto error;
>>>+	}
>>It would be helpful for debugging if the error message included the devargs so that we can see what is wrong with the input.
>>
>>
>
>Agreed.
>

Actually, on second thought, here the devargs that was problematic has 
already been shown with a warning while it was being parsed.

>>>+	/* Use all ports when none are defined */
>>>+	if (conf.active_ports == 0) {
>>>+		for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
>>>+			conf.active_ports |= 1 << i;
>>>+	}
>>
>>Rather than use a loop to populate all active fields would a #define with an all ports mask be better suited to this.  Or alternatively just change the IF statement below to use the following and avoid the need for this loop altogether:
>>
>>if (conf.active_ports & !(conf.active_ports & (1 << i)))
>>	continue;
>>
>>
>
>I do not agree with removing this loop.
>
>Your second solution will scatter the relevant bits concerning the 
>default value of the active_port configuration option. While being 
>slightly slicker it hides it unnecessarily from the reader.
>
>The first solution might be interesting, however it makes this option 
>dependent on two defines instead of one. If one had to change the 
>default MAX_PHYS_PORT value for mlx4 (however unlikely it might be), 
>then they would have to change the valid ALL_PORTS mask as well. In 
>principle this contradicts DRY[1].
>
>[1]: https://en.wikipedia.org/wiki/Don't_repeat_yourself
>
>-- 
>Gaëtan Rivet
>6WIND

-- 
Gaëtan Rivet
6WIND

Re: [PATCH 1/1] net/mlx4: add port parameter

[Legacy, Allain]

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Friday, March 10, 2017 12:12 PM
> The first solution might be interesting, however it makes this option
> dependent on two defines instead of one. If one had to change the
> default MAX_PHYS_PORT value for mlx4 (however unlikely it might be),
> then they would have to change the valid ALL_PORTS mask as well. In
> principle this contradicts DRY[1].
> 
> [1]: https://en.wikipedia.org/wiki/Don't_repeat_yourself

#define MLX4_PMD_MAX_PHYS_PORTS 2
#define MLX4_PMD_ALL_PHYS_PORTS ((1 << MLX4_PMD_MAX_PHYS_PORTS) - 1)

Re: [PATCH 1/1] net/mlx4: add port parameter

[Adrien Mazarguil]

On Fri, Mar 03, 2017 at 04:40:06PM +0100, Gaetan Rivet wrote:
> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> address.
> 
> Add a new port parameter allowing the user to choose
> either or both physical ports to be used by the application.
> 
> This parameter is used as follows:
> 
> Selecting only the second port:
>    -w 00:00.0,port=1
> 
> Selecting both ports:
>    -w 00:00.0,port=0,port=1
> 
> If no parameter is given, the default behavior is unchanged: all ports
> are probed.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>

I think this patch is good as is. Whatever value results from users
specifying random characters as argument to the port parameter is their
problem, as long as the resulting value is verified to be within bounds,
it's fine.

I'm not saying that checking all possible failure modes of strtoul() is
useless, just that it seems overkill in this specific case. Using atoi()
without any error checking would have been perfectly fine as well.

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

Re: [PATCH 1/1] net/mlx4: add port parameter

[Ferruh Yigit]

On 3/16/2017 11:04 AM, Adrien Mazarguil wrote:
> On Fri, Mar 03, 2017 at 04:40:06PM +0100, Gaetan Rivet wrote:
>> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
>> address.
>>
>> Add a new port parameter allowing the user to choose
>> either or both physical ports to be used by the application.
>>
>> This parameter is used as follows:
>>
>> Selecting only the second port:
>>    -w 00:00.0,port=1
>>
>> Selecting both ports:
>>    -w 00:00.0,port=0,port=1
>>
>> If no parameter is given, the default behavior is unchanged: all ports
>> are probed.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> 
> I think this patch is good as is. Whatever value results from users
> specifying random characters as argument to the port parameter is their
> problem, as long as the resulting value is verified to be within bounds,
> it's fine.
> 
> I'm not saying that checking all possible failure modes of strtoul() is
> useless, just that it seems overkill in this specific case. Using atoi()
> without any error checking would have been perfectly fine as well.
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Hi Adrien, Gaetan,

Are all comments addressed for this patch? It looks like discussion is
going on?

Thanks,
ferruh

Re: [PATCH 1/1] net/mlx4: add port parameter

[Adrien Mazarguil]

Hi Ferruh,

On Mon, Mar 20, 2017 at 01:24:36PM +0000, Ferruh Yigit wrote:
> On 3/16/2017 11:04 AM, Adrien Mazarguil wrote:
> > On Fri, Mar 03, 2017 at 04:40:06PM +0100, Gaetan Rivet wrote:
> >> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> >> address.
> >>
> >> Add a new port parameter allowing the user to choose
> >> either or both physical ports to be used by the application.
> >>
> >> This parameter is used as follows:
> >>
> >> Selecting only the second port:
> >>    -w 00:00.0,port=1
> >>
> >> Selecting both ports:
> >>    -w 00:00.0,port=0,port=1
> >>
> >> If no parameter is given, the default behavior is unchanged: all ports
> >> are probed.
> >>
> >> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > 
> > I think this patch is good as is. Whatever value results from users
> > specifying random characters as argument to the port parameter is their
> > problem, as long as the resulting value is verified to be within bounds,
> > it's fine.
> > 
> > I'm not saying that checking all possible failure modes of strtoul() is
> > useless, just that it seems overkill in this specific case. Using atoi()
> > without any error checking would have been perfectly fine as well.
> > 
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Hi Adrien, Gaetan,
> 
> Are all comments addressed for this patch? It looks like discussion is
> going on?

I think it's over, no more comments will follow.

-- 
Adrien Mazarguil
6WIND

Re: [PATCH 1/1] net/mlx4: add port parameter

[Ferruh Yigit]

On 3/3/2017 3:40 PM, Gaetan Rivet wrote:
> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> address.
> 
> Add a new port parameter allowing the user to choose
> either or both physical ports to be used by the application.
> 
> This parameter is used as follows:
> 
> Selecting only the second port:
>    -w 00:00.0,port=1
> 
> Selecting both ports:
>    -w 00:00.0,port=0,port=1
> 
> If no parameter is given, the default behavior is unchanged: all ports
> are probed.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Hi Gaetan,

Getting following checkpatch warning [1], can you please address it?

Thanks,
ferruh

[1]
WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#156: FILE: drivers/net/mlx4/mlx4.c:5462:
+       unsigned arg_count;

[PATCH v2 1/1] net/mlx4: add port parameter

[Gaetan Rivet]

Most ConnectX-3 adapters expose two physical ports on a single PCI bus
address.

Add a new port parameter allowing the user to choose
either or both physical ports to be used by the application.

This parameter is used as follows:

Selecting only the second port:
   -w 00:00.0,port=1

Selecting both ports:
   -w 00:00.0,port=0,port=1

If no parameter is given, the default behavior is unchanged: all ports
are probed.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 doc/guides/nics/mlx4.rst  |   6 +++
 drivers/net/mlx4/Makefile |   1 +
 drivers/net/mlx4/mlx4.c   | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/mlx4/mlx4.h   |   6 +++
 4 files changed, 117 insertions(+)

diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst
index 9a2e973..7992735 100644
--- a/doc/guides/nics/mlx4.rst
+++ b/doc/guides/nics/mlx4.rst
@@ -162,6 +162,12 @@ Run-time configuration
 
 - **ethtool** operations on related kernel interfaces also affect the PMD.
 
+- ``port`` parameter [int]
+
+  This parameter provides a physical port to probe and can be specified multiple
+  times for additional ports. All ports are probed by default if left
+  unspecified.
+
 Kernel module parameters
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/drivers/net/mlx4/Makefile b/drivers/net/mlx4/Makefile
index 1d463f7..f0ee282 100644
--- a/drivers/net/mlx4/Makefile
+++ b/drivers/net/mlx4/Makefile
@@ -43,6 +43,7 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_ether
 DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_mbuf
 DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_eal
 DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_mempool
+DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_kvargs
 
 # Basic CFLAGS.
 CFLAGS += -O3
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 35a680c..311e0b7 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -83,6 +83,7 @@
 #include <rte_alarm.h>
 #include <rte_memory.h>
 #include <rte_flow.h>
+#include <rte_kvargs.h>
 
 /* Generated configuration header. */
 #include "mlx4_autoconf.h"
@@ -125,6 +126,16 @@ struct mlx4_secondary_data {
 	rte_spinlock_t lock; /* Port configuration lock. */
 } mlx4_secondary_data[RTE_MAX_ETHPORTS];
 
+struct mlx4_conf {
+	uint8_t active_ports;
+};
+
+/* Available parameters list. */
+const char *pmd_mlx4_init_params[] = {
+	MLX4_PMD_PORT_KVARG,
+	NULL,
+};
+
 /**
  * Check if running as a secondary process.
  *
@@ -5396,6 +5407,84 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
 	}
 }
 
+/**
+ * Verify and store value for device argument.
+ *
+ * @param[in] key
+ *   Key argument to verify.
+ * @param[in] val
+ *   Value associated with key.
+ * @param out
+ *   User data.
+ *
+ * @return
+ *   0 on success, negative errno value on failure.
+ */
+static int
+mlx4_arg_parse(const char *key, const char *val, void *out)
+{
+	struct mlx4_conf *conf = out;
+	unsigned long tmp;
+
+	errno = 0;
+	tmp = strtoul(val, NULL, 0);
+	if (errno) {
+		WARN("%s: \"%s\" is not a valid integer", key, val);
+		return -errno;
+	}
+	if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
+		if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
+			ERROR("invalid port index %lu (max: %u)",
+				tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
+			return -EINVAL;
+		}
+		conf->active_ports |= 1 << tmp;
+	} else {
+		WARN("%s: unknown parameter", key);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/**
+ * Parse device parameters.
+ *
+ * @param devargs
+ *   Device arguments structure.
+ *
+ * @return
+ *   0 on success, negative errno value on failure.
+ */
+static int
+mlx4_args(struct rte_devargs *devargs, struct mlx4_conf *conf)
+{
+	struct rte_kvargs *kvlist;
+	unsigned int arg_count;
+	int ret = 0;
+	int i;
+
+	if (devargs == NULL)
+		return 0;
+	kvlist = rte_kvargs_parse(devargs->args, pmd_mlx4_init_params);
+	if (kvlist == NULL) {
+		ERROR("failed to parse kvargs");
+		return -EINVAL;
+	}
+	/* Process parameters. */
+	for (i = 0; pmd_mlx4_init_params[i]; ++i) {
+		arg_count = rte_kvargs_count(kvlist, MLX4_PMD_PORT_KVARG);
+		while (arg_count-- > 0) {
+			ret = rte_kvargs_process(kvlist, MLX4_PMD_PORT_KVARG,
+					mlx4_arg_parse, conf);
+			if (ret != 0)
+				goto free_kvlist;
+		}
+	}
+free_kvlist:
+	rte_kvargs_free(kvlist);
+	return ret;
+}
+
 static struct eth_driver mlx4_driver;
 
 /**
@@ -5420,6 +5509,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 	int err = 0;
 	struct ibv_context *attr_ctx = NULL;
 	struct ibv_device_attr device_attr;
+	struct mlx4_conf conf = {
+		.active_ports = 0,
+	};
 	unsigned int vf;
 	int idx;
 	int i;
@@ -5490,6 +5582,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		goto error;
 	INFO("%u port(s) detected", device_attr.phys_port_cnt);
 
+	if (mlx4_args(pci_dev->device.devargs, &conf)) {
+		ERROR("failed to process device arguments");
+		goto error;
+	}
+	/* Use all ports when none are defined */
+	if (conf.active_ports == 0) {
+		for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
+			conf.active_ports |= 1 << i;
+	}
 	for (i = 0; i < device_attr.phys_port_cnt; i++) {
 		uint32_t port = i + 1; /* ports are indexed from one */
 		uint32_t test = (1 << i);
@@ -5503,6 +5604,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 #endif /* HAVE_EXP_QUERY_DEVICE */
 		struct ether_addr mac;
 
+		/* If port is not active, skip. */
+		if (!(conf.active_ports & (1 << i)))
+			continue;
 #ifdef HAVE_EXP_QUERY_DEVICE
 		exp_device_attr.comp_mask = IBV_EXP_DEVICE_ATTR_EXP_CAP_FLAGS;
 #ifdef RSS_SUPPORT
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index 877ed79..e9659eb 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -81,6 +81,9 @@
 /* Request send completion once in every 64 sends, might be less. */
 #define MLX4_PMD_TX_PER_COMP_REQ 64
 
+/* Maximum number of physical ports. */
+#define MLX4_PMD_MAX_PHYS_PORTS 2
+
 /* Maximum number of Scatter/Gather Elements per Work Request. */
 #ifndef MLX4_PMD_SGE_WR_N
 #define MLX4_PMD_SGE_WR_N 4
@@ -113,6 +116,9 @@
 /* Alarm timeout. */
 #define MLX4_ALARM_TIMEOUT_US 100000
 
+/* Port parameter. */
+#define MLX4_PMD_PORT_KVARG "port"
+
 enum {
 	PCI_VENDOR_ID_MELLANOX = 0x15b3,
 };
-- 
2.1.4

Re: [PATCH 1/1] net/mlx4: add port parameter

[Gaëtan Rivet]

Hi Ferruh,

On Mon, Mar 27, 2017 at 04:08:51PM +0100, Ferruh Yigit wrote:
>On 3/3/2017 3:40 PM, Gaetan Rivet wrote:
>> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
>> address.
>>
>> Add a new port parameter allowing the user to choose
>> either or both physical ports to be used by the application.
>>
>> This parameter is used as follows:
>>
>> Selecting only the second port:
>>    -w 00:00.0,port=1
>>
>> Selecting both ports:
>>    -w 00:00.0,port=0,port=1
>>
>> If no parameter is given, the default behavior is unchanged: all ports
>> are probed.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>
>Hi Gaetan,
>
>Getting following checkpatch warning [1], can you please address it?
>

Sure, here it is: http://dpdk.org/ml/archives/dev/2017-March/061582.html

Sorry for that, I was checking it with an older checkpatch.pl.

>Thanks,
>ferruh
>
>[1]
>WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
>#156: FILE: drivers/net/mlx4/mlx4.c:5462:
>+       unsigned arg_count;

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v2 1/1] net/mlx4: add port parameter

[Ferruh Yigit]

On 3/27/2017 4:41 PM, Gaetan Rivet wrote:
> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> address.
> 
> Add a new port parameter allowing the user to choose
> either or both physical ports to be used by the application.
> 
> This parameter is used as follows:
> 
> Selecting only the second port:
>    -w 00:00.0,port=1
> 
> Selecting both ports:
>    -w 00:00.0,port=0,port=1
> 
> If no parameter is given, the default behavior is unchanged: all ports
> are probed.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>

    Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied to dpdk-next-net/master, thanks.