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

[David Marchand <david.marchand@6wind.com>]

Hello Rahul,

On Mon, Jul 20, 2015 at 8:41 AM, Rahul Lakkireddy <
rahul.lakkireddy@chelsio.com> wrote:

> nic_uio requires the pci ids to be present in rte_pci_dev_ids.h in order to
> bind the devices to nic_uio.  However, it's better to remove this
> whitelist of
> pci ids, and instead rely on hw.nic_uio.bdfs kenv parameter to allow
> binding
> any device to nic_uio.
>
> Suggested-by: David Marchand <david.marchand@6wind.com>
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
>

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 ?



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