[dpdk-dev] [PATCH 7/7] net/mlx5: add parameter for port representors

[Adrien Mazarguil]

On Tue, Jun 12, 2018 at 01:43:18PM +0000, Xueming(Steven) Li wrote:
<snip>
> > > >  	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