On Sun, Jun 10, 2018 at 12:59:06PM +0000, Xueming(Steven) Li wrote:
> Hi Adrien,
> 
> The logic looks much more clear now with the split.
<snip>
> > -           len = snprintf(name, sizeof(name), PCI_PRI_FMT,
> > -                    pci_dev->addr.domain, pci_dev->addr.bus,
> > -                    pci_dev->addr.devid, pci_dev->addr.function);
> > -           if (attr.orig_attr.phys_port_cnt > 1)
> > -                   snprintf(name + len, sizeof(name), " port %u", i);
> > +           if (attr->orig_attr.phys_port_cnt > 1)
> > +                   snprintf(name, sizeof(name), "%s", dpdk_dev->name);
> > +           else
> > +                   snprintf(name, sizeof(name), "%s port %u",
> > +                            dpdk_dev->name, port);
> 
> Name contains port only if phys_port_cnt > 1 in previous logic, are you sure?

Nice catch, will fix it for v2. This wasn't noticed because this code is
replaced in a subsequent patch of the series.

<snip>
> > +   for (i = 0; i < attr.orig_attr.phys_port_cnt; ++i) {
> > +           eth_list[i] = mlx5_dev_spawn_one(dpdk_dev, ibv_dev, vf,
> > +                                            &attr, i + 1);
> > +           if (eth_list[i])
> > +                   continue;
> > +           /* Save rte_errno and roll back in case of failure. */
> > +           ret = rte_errno;
> > +           while (i--) {
> > +                   mlx5_dev_close(eth_list[i]);
> > +                   if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > +                           rte_free(eth_list[i]->data->dev_private);
> > +                   claim_zero(rte_eth_dev_release_port(eth_list[i]));
> > +           }
> > +           free(eth_list);
> > +           rte_errno = ret;
> > +           return NULL;
> 
> The code is correct, but I personally prefer to move complex error handling 
> to 
> dedicate "error:" block to make the code clear.

Since it's the only place where this failure can occur, I'll leave it as is
on the basis that doing so saves a goto statement. Those should be avoided
where possible. It would have been a different story if the same error
handling code was called from multiple places.

<snip>
> > +static int
> > +mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> > +          struct rte_pci_device *pci_dev) {
> > +   struct ibv_device **ibv_list;
> > +   struct rte_eth_dev **eth_list = NULL;
> > +   int vf;
> > +   int ret;
> > +
> > +   assert(pci_drv == &mlx5_driver);
> > +   switch (pci_dev->id.device_id) {
> > +   case PCI_DEVICE_ID_MELLANOX_CONNECTX4VF:
> > +   case PCI_DEVICE_ID_MELLANOX_CONNECTX4LXVF:
> > +   case PCI_DEVICE_ID_MELLANOX_CONNECTX5VF:
> > +   case PCI_DEVICE_ID_MELLANOX_CONNECTX5EXVF:
> > +           vf = 1;
> > +           break;
> > +   default:
> > +           vf = 0;
> > +   }
> 
> How about use a macro for vf detection and invoke in mlx5_dev_spawn_one().
> Seems it not used in outer callers.

mlx5_dev_spawn_one() can be invoked with IB devices not backed by PCI
(e.g. vdevs), for which the caller may still knowingly ask for VF behavior,
either by user request or through other means.

In this case, the caller happens to be a PCI probing function which, based
on the device ID, easily determines whether VF behavior shall be requested.

This is basically the only place where PCI device ID can be checked. Adding
a macro here would only obfuscate this check. Adding it in
mlx5_dev_spawn_one() would entangle PCI and generic code again, the opposite
of the purpose of this patch, therefore I'll leave it unmodified for v2.

-- 
Adrien Mazarguil
6WIND

Reply via email to