Re: [dpdk-dev] [PATCH v2 1/3] nic_uio: Fix to allow any device to be bound to nic_uio

[Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>]

Hi David,

On Mon, Jul 20, 2015 at 09:43:57 +0200, David Marchand wrote:
>    Hum, what bothers me is that you do not rely on the same criteria to
>    re-attach the devices to nic_uio.
>    See below.
> 
>       lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 48
>      +++++++++------------------------
>       1 file changed, 13 insertions(+), 35 deletions(-)
> 
>      diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
>      b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
>      index 2354e84..f868dc8 100644
>      --- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
>      +++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
>      [snip]
>      @@ -195,11 +177,10 @@ nic_uio_probe (device_t dev)
>       {
>              int i;
> 
>      -       for (i = 0; i < NUM_DEVICES; i++)
>      -               if (pci_get_vendor(dev) == devices[i].vend &&
>      -                       pci_get_device(dev) == devices[i].dev) {
>      -
>      -                       device_set_desc(dev, "Intel(R) DPDK PCI
>      Device");
>      +       for (i = 0; i < num_detached; i++)
>      +               if (pci_get_vendor(dev) ==
>      pci_get_vendor(detached_devices[i]) &&
>      +                   pci_get_device(dev) ==
>      pci_get_device(detached_devices[i])) {
>      +                       device_set_desc(dev, "DPDK PCI Device");
>                              return BUS_PROBE_SPECIFIC;
>                      }
> 
>    When going through the probe stuff, the device vendor and type are used as
>    the matching criteria.
> 
>      @@ -256,7 +237,6 @@ static void
>       nic_uio_load(void)
>       {
>              uint32_t bus, device, function;
>      -       int i;
>              device_t dev;
>              char bdf_str[256];
>              char *token, *remaining;
>      @@ -295,17 +275,15 @@ nic_uio_load(void)
>                      if (dev == NULL)
>                              continue;
> 
>      -               for (i = 0; i < NUM_DEVICES; i++)
>      -                       if (pci_get_vendor(dev) == devices[i].vend &&
>      -                                       pci_get_device(dev) ==
>      devices[i].dev) {
>      -                                               if (num_detached <
>      MAX_DETACHED_DEVICES) {
>      -                                                     
>       printf("nic_uio_load: detaching and storing dev=%p\n", dev);
>      -                                                     
>       detached_devices[num_detached++] = dev;
>      -                                               } else
>      -                                                     
>       printf("nic_uio_load: reached MAX_DETACHED_DEVICES=%d. dev=%p won't be
>      reattached\n",
>      -                                                             
>       MAX_DETACHED_DEVICES, dev);
>      -                                               device_detach(dev);
>      -                       }
>      +               if (num_detached < MAX_DETACHED_DEVICES) {
>      +                       printf("nic_uio_load: detaching and storing
>      dev=%p\n",
>      +                              dev);
>      +                       detached_devices[num_detached++] = dev;
>      +               } else {
>      +                       printf("nic_uio_load: reached
>      MAX_DETACHED_DEVICES=%d. dev=%p won't be reattached\n",
>      +                              MAX_DETACHED_DEVICES, dev);
>      +               }
>      +               device_detach(dev);
>              }
>       }
> 
>    But here at init time, the bdfs informations are used to detach the pci
>    devices.
>    I would say this is safer we have the same criteria in both cases.
>    I think that the pci addresses are the best criteria since this is what
>    the user gives.
>    Don't we have them in the dev pointer ?

It looks like we can get them via pci_get_bus(), pci_get_slot(), and
pci_get_function().  Will add check for these 3 info instead of vendor
and device in probe to make it consistent.

> 
>    Btw, with this change, we would then be limited to MAX_DETACHED_DEVICES
>    devices even if 128 pci devices looks quite big enough to me.
>    This part could be reworked (later).
>    --
>    David Marchand

Thanks,
Rahul