Re: [PATCH 05/14] net/mlx5: add multiport IB device support to probing

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, March 21, 2019 2:58 PM, Slava Ovsiienko:
> Subject: RE: [PATCH 05/14] net/mlx5: add multiport IB device support to
> probing
> 
> Sorry, missed some comments. Here is my extra answers.
> 

[...]

> > -----Original 
callback to sort device data.
> > > >   *
> > > > @@ -1380,7 +1381,9 @@ struct mlx5_dev_spawn_data {
> > > >  	       struct rte_pci_device *pci_dev)  {
> > > >  	struct ibv_device **ibv_list;
> > > > -	unsigned int n = 0;
> > > > +	unsigned int nd = 0;
> > > > +	unsigned int np = 0;
> > > > +	unsigned int ns = 0;
> > >
> > > This fields names are not informative. Find a better ones.
> >
> > Would the adding clarifying comments be enough ?

Yes it will be OK.

> >
> > nd - Number of (PCI) Devices   (nd != 1 means we have multiple devices
> with
> > the same BDF - old schema)
> > np - Number of (device) Ports (nd =1, np 1...n means we have new
> > multiport
> > device) ns - Number to Spawn  (deduced index - number of iterations)
> >
> > This names are used as indices, long names may make code less
> > readable, IMHO.
> >
> > >
> > > >  	struct mlx5_dev_config dev_config;
> > > >  	int ret;
> > > >
> > > > @@ -1392,8 +1395,14 @@ struct mlx5_dev_spawn_data {
> > > >  		DRV_LOG(ERR, "cannot list devices, is ib_uverbs loaded?");
> > > >  		return -rte_errno;
> > > >  	}
> > > > -
> > > > +	/*
> > > > +	 * First scan the list of all Infiniband devices to find
> > > > +	 * matching ones, gathering into the list.
> > > > +	 */
> > > >  	struct ibv_device *ibv_match[ret + 1];
> > > > +	int nl_route = -1;
> > > > +	int nl_rdma = -1;
> > > > +	unsigned int i;
> > > >
> > > >  	while (ret-- > 0) {
> > > >  		struct rte_pci_addr pci_addr;
> > > > @@ -1408,77 +1417,183 @@ struct mlx5_dev_spawn_data {
> > > >  			continue;
> > > >  		DRV_LOG(INFO, "PCI information matches for device
> \"%s\"",
> > > >  			ibv_list[ret]->name);
> > > > -		ibv_match[n++] = ibv_list[ret];
> > > > +		ibv_match[nd++] = ibv_list[ret];
> > > > +	}
> > > > +	ibv_match[nd] = NULL;
> > > > +	if (!nd) {
> > > > +		/* No device macthes, just complain and bail out. */
> > > > +		mlx5_glue->free_device_list(ibv_list);
> > > > +		DRV_LOG(WARNING,
> > > > +			"no Verbs device matches PCI device " PCI_PRI_FMT
> > > > ","
> > > > +			" are kernel drivers loaded?",
> > > > +			pci_dev->addr.domain, pci_dev->addr.bus,
> > > > +			pci_dev->addr.devid, pci_dev->addr.function);
> > > > +		rte_errno = ENOENT;
> > > > +		ret = -rte_errno;
> > > > +		return ret;
> > > > +	}
> > > > +	nl_route = mlx5_nl_init(NETLINK_ROUTE);
> > > > +	nl_rdma = mlx5_nl_init(NETLINK_RDMA);
> > > > +	if (nd == 1) {
> > > > +		/*
> > > > +		 * Found single matching device may have multiple ports.
> > > > +		 * Each port may be representor, we have to check the port
> > > > +		 * number and check the representors existence.
> > > > +		 */
> > > > +		if (nl_rdma >= 0)
> > > > +			np = mlx5_nl_portnum(nl_rdma, ibv_match[0]-
> > > > >name);
> > > > +		if (!np)
> > > > +			DRV_LOG(WARNING, "can not get IB device \"%s\""
> > > > +					 " ports number", ibv_match[0]-
> > > > >name);
> > >
> > > This warning is misleading. On old kernels it is expected to have
> > > multiple IB devices instead of a single one w/ multiple ports.
> > > The level should be changed for debug, and the syntax to express it
> > > is not an error.
> 
> On old kernels we should get np = 1. If np == 0 it means an error, even if
> there is old kernel. Zero np means that is something is going in wrong way
> and we should notify the user. We do not expect this behavior from old/new
> kernels, so this message should not be annoying.

OK.