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.
