On Tue, Jun 12, 2018 at 01:43:18PM +0000, Xueming(Steven) Li wrote:
<snip>

quoted

quoted

quoted

 	void *tmp;
 	unsigned int i;
 	unsigned int j = 0;
 	unsigned int n = 0;
 	int ret;

+	if (dpdk_dev->devargs) {
+		ret = rte_eth_devargs_parse(dpdk_dev->devargs->args, &eth_da);
+		if (ret)
+			goto error;
+	} else {
+		memset(&eth_da, 0, sizeof(eth_da));
+	}
 next:
+	if (j) {
+		unsigned int k;
+
+		for (k = 0; k < eth_da.nb_representor_ports; ++k)
+			if (eth_da.representor_ports[k] == j - 1)
+				break;
+		if (k == eth_da.nb_representor_ports)
+			goto skip;
+	}
 	errno = 0;
 	ctx = mlx5_glue->open_device(ibv_dev[j]);

Need a range check for j here.

I think it's properly checked. j == 0 stands for "master device", always found at index 0 and probed.
Representors devices, if any, start at index 1 which triggers the previous block. This block makes
sure that a given representor is indeed enabled before either spawning the related device (pass
through with a valid "j") or skipping it altogether (goto skip).

Yes, this code looks good. What I wanted to ask what if dev args specify an invalid rep id, e.g. 33.
This code walk through silently w/o warning, it works, but it better to have a warning if input id out of range.

You're right. On the other hand this provides a means to spawn all
representors without necessarily knowing how many can be instantiated first,
e.g. by always providing a "representor=[0-31]" argument, since no special
keyword is defined to request them all.

Not saying it's a good or bad thing, but somewhat harmless. Just like
specifying "-w {DBDF}" arguments with invalid addresses, nonexistent
representors are silently ignored.

In any case, this can be improved later. We're already seeing a couple of
limitations with the representor argument, namely the lack of hot-plug
support, which will need to be addressed as well.

-- 
Adrien Mazarguil
6WIND