List comments

GET /api/patches/74698/comments/
HTTP 200 OK
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

[
    {
        "id": 116615,
        "web_url": "https://patches.dpdk.org/comment/116615/",
        "msgid": "<CAJFAV8xT0Uo9b73SfpA3Tt15A1K5+0BgLnNMuehbx5jyjtNKwg@mail.gmail.com>",
        "date": "2020-07-24T14:40:05",
        "subject": "Re: [dpdk-dev] [PATCH v8 08/10] common/mlx5: introduce layer to\n support multiple class drivers",
        "submitter": {
            "id": 1173,
            "url": "https://patches.dpdk.org/api/people/1173/",
            "name": "David Marchand",
            "email": "david.marchand@redhat.com"
        },
        "content": "This is not a thorough review, just caught one issue for makefile and some nits.\n\n\nOn Thu, Jul 23, 2020 at 10:11 PM Parav Pandit <parav@mellanox.com> wrote:\n>\n> From: Thomas Monjalon <thomas@monjalon.net>\n>\n> Add generic mlx5 PCI PMD layer as part of existing common_mlx5\n> module. This enables multiple classes (net, regex, vdpa) PMDs\n> to be supported at same time.\n>\n> Signed-off-by: Parav Pandit <parav@mellanox.com>\n> Acked-by: Matan Azrad <matan@mellanox.com>\n> ---\n>  drivers/Makefile                              |  10 +-\n>  drivers/common/Makefile                       |   4 -\n>  drivers/common/meson.build                    |   2 +-\n>  drivers/common/mlx5/Makefile                  |   1 +\n>  drivers/common/mlx5/meson.build               |   9 +-\n>  drivers/common/mlx5/mlx5_common.c             |   2 +\n>  drivers/common/mlx5/mlx5_common_pci.c         | 541 ++++++++++++++++++\n>  drivers/common/mlx5/mlx5_common_pci.h         |  77 +++\n>  .../common/mlx5/rte_common_mlx5_version.map   |   2 +\n>  drivers/meson.build                           |   1 +\n>  10 files changed, 638 insertions(+), 11 deletions(-)\n>  create mode 100644 drivers/common/mlx5/mlx5_common_pci.c\n>  create mode 100644 drivers/common/mlx5/mlx5_common_pci.h\n>\n> diff --git a/drivers/Makefile b/drivers/Makefile\n> index 1551272ef..7f06162dc 100644\n> --- a/drivers/Makefile\n> +++ b/drivers/Makefile\n> @@ -8,8 +8,12 @@ DIRS-y += bus\n>  DEPDIRS-bus := common\n>  DIRS-y += mempool\n>  DEPDIRS-mempool := common bus\n> +ifeq ($(findstring y,$(CONFIG_RTE_LIBRTE_MLX5_PMD)$(CONFIG_RTE_LIBRTE_MLX5_VDPA_PMD)$(CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD)),y)\n> +DIRS-y += common/mlx5\n> +DEPDIRS-common/mlx5 := bus\n> +endif\n>  DIRS-y += net\n> -DEPDIRS-net := common bus mempool\n> +DEPDIRS-net := common bus mempool common/mlx5\n\nWill it still work for people not building with mlx5?\n\nI would have seen something like:\nDEPDIRS-net/mlx5 := xx xx xx common/mlx5\nOr maybe\nDEPDIRS-net/mlx5 += common/mlx5\n\n\n>  DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += baseband\n>  DEPDIRS-baseband := common bus mempool\n>  DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += crypto\n> @@ -19,9 +23,9 @@ DEPDIRS-common/qat := bus mempool\n>  DIRS-$(CONFIG_RTE_LIBRTE_COMPRESSDEV) += compress\n>  DEPDIRS-compress := bus mempool\n>  DIRS-$(CONFIG_RTE_LIBRTE_REGEXDEV) += regex\n> -DEPDIRS-regex := common bus\n> +DEPDIRS-regex := common bus common/mlx5\n\nDEPDIRS-regex/mlx5 := xx xx xx common/mlx5\n\n>  DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += vdpa\n> -DEPDIRS-vdpa := common bus mempool\n> +DEPDIRS-vdpa := common bus mempool common/mlx5\n\nIdem.\n\n\n>  DIRS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += event\n>  DEPDIRS-event := common bus mempool net crypto\n>  DIRS-$(CONFIG_RTE_LIBRTE_RAWDEV) += raw\n> diff --git a/drivers/common/Makefile b/drivers/common/Makefile\n> index cbc71077c..cfb6b4dc8 100644\n> --- a/drivers/common/Makefile\n> +++ b/drivers/common/Makefile\n> @@ -36,8 +36,4 @@ ifneq (,$(findstring y,$(IAVF-y)))\n>  DIRS-y += iavf\n>  endif\n>\n> -ifeq ($(findstring y,$(CONFIG_RTE_LIBRTE_MLX5_PMD)$(CONFIG_RTE_LIBRTE_MLX5_VDPA_PMD)$(CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD)),y)\n> -DIRS-y += mlx5\n> -endif\n> -\n>  include $(RTE_SDK)/mk/rte.subdir.mk\n> diff --git a/drivers/common/meson.build b/drivers/common/meson.build\n> index 5db7e29b1..9ed4c04ba 100644\n> --- a/drivers/common/meson.build\n> +++ b/drivers/common/meson.build\n> @@ -6,6 +6,6 @@ if is_windows\n>  endif\n>\n>  std_deps = ['eal']\n> -drivers = ['cpt', 'dpaax', 'iavf', 'mlx5', 'mvep', 'octeontx', 'octeontx2', 'qat']\n> +drivers = ['cpt', 'dpaax', 'iavf', 'mvep', 'octeontx', 'octeontx2', 'qat']\n>  config_flag_fmt = 'RTE_LIBRTE_@0@_COMMON'\n>  driver_name_fmt = 'rte_common_@0@'\n> diff --git a/drivers/common/mlx5/Makefile b/drivers/common/mlx5/Makefile\n> index 6b89a6c85..eaabb8c10 100644\n> --- a/drivers/common/mlx5/Makefile\n> +++ b/drivers/common/mlx5/Makefile\n> @@ -22,6 +22,7 @@ SRCS-y += linux/mlx5_common_verbs.c\n>  SRCS-y += mlx5_common_mp.c\n>  SRCS-y += mlx5_common_mr.c\n>  SRCS-y += mlx5_malloc.c\n> +SRCS-y += mlx5_common_pci.c\n\nAlphabetical order?\n\n\n>  ifeq ($(CONFIG_RTE_IBVERBS_LINK_DLOPEN),y)\n>  INSTALL-y-lib += $(LIB_GLUE)\n>  endif\n> diff --git a/drivers/common/mlx5/meson.build b/drivers/common/mlx5/meson.build\n> index 70e2c1c32..8e5608703 100644\n> --- a/drivers/common/mlx5/meson.build\n> +++ b/drivers/common/mlx5/meson.build\n> @@ -1,19 +1,22 @@\n>  # SPDX-License-Identifier: BSD-3-Clause\n>  # Copyright 2019 Mellanox Technologies, Ltd\n>\n> -if not (is_linux or is_windows)\n> +if not is_linux\n>         build = false\n> -       reason = 'only supported on Linux and Windows'\n> +       reason = 'only supported on Linux'\n>         subdir_done()\n>  endif\n>\n> -deps += ['hash', 'pci', 'net', 'eal', 'kvargs']\n> +config_flag_fmt = 'RTE_LIBRTE_@0@_COMMON'\n> +driver_name_fmt = 'rte_common_@0@'\n> +deps += ['hash', 'pci', 'bus_pci', 'net', 'eal', 'kvargs']\n>  sources += files(\n>         'mlx5_devx_cmds.c',\n>         'mlx5_common.c',\n>         'mlx5_common_mp.c',\n>         'mlx5_common_mr.c',\n>         'mlx5_malloc.c',\n> +       'mlx5_common_pci.c',\n\nIdem order.\n\n\n>  )\n>\n>  cflags_options = [\n> diff --git a/drivers/common/mlx5/mlx5_common.c b/drivers/common/mlx5/mlx5_common.c\n> index 2b336bb2d..fd818ef24 100644\n> --- a/drivers/common/mlx5/mlx5_common.c\n> +++ b/drivers/common/mlx5/mlx5_common.c\n> @@ -14,6 +14,7 @@\n>  #include \"mlx5_common_os.h\"\n>  #include \"mlx5_common_utils.h\"\n>  #include \"mlx5_malloc.h\"\n> +#include \"mlx5_common_pci.h\"\n\nIdem order.\n\n\n>\n>  int mlx5_common_logtype;\n>\n> @@ -100,6 +101,7 @@ mlx5_common_init(void)\n>                 return;\n>\n>         mlx5_glue_constructor();\n> +       mlx5_common_pci_init();\n>         mlx5_common_initialized = true;\n>  }\n>\n> diff --git a/drivers/common/mlx5/mlx5_common_pci.c b/drivers/common/mlx5/mlx5_common_pci.c\n> new file mode 100644\n> index 000000000..b42f7469f\n> --- /dev/null\n> +++ b/drivers/common/mlx5/mlx5_common_pci.c\n> @@ -0,0 +1,541 @@\n> +/* SPDX-License-Identifier: BSD-3-Clause\n> + * Copyright 2020 Mellanox Technologies Ltd\n> + */\n> +\n> +#include <stdlib.h>\n\nNit: headers usually come in separate blocks, \"system headers\", \"dpdk\nheaders\", \"drivers headers\".\nSo missing an empty line here.\n\n> +#include <rte_malloc.h>\n\n\n> +#include \"mlx5_common_utils.h\"\n> +#include \"mlx5_common_pci.h\"\n> +\n> +struct mlx5_pci_device {\n> +      struct rte_pci_device *pci_dev;\n> +      TAILQ_ENTRY(mlx5_pci_device) next;\n> +      uint32_t classes_loaded;\n> +};\n> +\n> +/* Head of list of drivers. */\n> +static TAILQ_HEAD(mlx5_pci_bus_drv_head, mlx5_pci_driver) drv_list =\n> +                               TAILQ_HEAD_INITIALIZER(drv_list);\n> +\n> +/* Head of mlx5 pci devices. */\n> +static TAILQ_HEAD(mlx5_pci_devices_head, mlx5_pci_device) devices_list =\n> +                               TAILQ_HEAD_INITIALIZER(devices_list);\n> +\n> +static const struct {\n> +       const char *name;\n> +       unsigned int driver_class;\n> +} mlx5_classes[] = {\n> +       { .name = \"vdpa\", .driver_class = MLX5_CLASS_VDPA },\n> +       { .name = \"net\", .driver_class = MLX5_CLASS_NET },\n> +       { .name = \"regex\", .driver_class = MLX5_CLASS_REGEX },\n> +};\n> +\n> +static const unsigned int mlx5_class_combinations[] = {\n> +       MLX5_CLASS_NET,\n> +       MLX5_CLASS_VDPA,\n> +       MLX5_CLASS_REGEX,\n> +       /* New class combination should be added here.\n> +        * For example a new multi class device combination\n> +        * can be MLX5_CLASS_FOO | MLX5_CLASS_BAR.\n> +        */\n> +};\n> +\n> +static int\n> +class_name_to_value(const char *class_name)\n> +{\n> +       unsigned int i;\n> +\n> +       for (i = 0; i < RTE_DIM(mlx5_classes); i++) {\n> +               if (strcmp(class_name, mlx5_classes[i].name) == 0)\n> +                       return mlx5_classes[i].driver_class;\n> +       }\n> +       return -EINVAL;\n> +}\n> +\n> +static struct mlx5_pci_driver *\n> +driver_get(uint32_t class)\n> +{\n> +       struct mlx5_pci_driver *driver;\n> +\n> +       TAILQ_FOREACH(driver, &drv_list, next) {\n> +               if (driver->driver_class == class)\n> +                       return driver;\n> +       }\n> +       return NULL;\n> +}\n> +\n> +static int\n> +bus_cmdline_options_handler(__rte_unused const char *key,\n> +                           const char *class_names, void *opaque)\n> +{\n> +       int *ret = opaque;\n> +       char *nstr_org;\n> +       int class_val;\n> +       char *found;\n> +       char *nstr;\n> +\n> +       *ret = 0;\n> +       nstr = strdup(class_names);\n> +       if (!nstr) {\n> +               *ret = -ENOMEM;\n> +               return *ret;\n> +       }\n> +       nstr_org = nstr;\n> +       while (nstr) {\n> +               /* Extract each individual class name. Multiple\n> +                * class key,value is supplied as class=net:vdpa:foo:bar.\n> +                */\n> +               found = strsep(&nstr, \":\");\n> +               if (!found)\n> +                       continue;\n> +               /* Check if its a valid class. */\n> +               class_val = class_name_to_value(found);\n> +               if (class_val < 0) {\n> +                       *ret = -EINVAL;\n> +                       goto err;\n> +               }\n> +               *ret |= class_val;\n> +       }\n> +err:\n> +       free(nstr_org);\n> +       if (*ret < 0)\n> +               DRV_LOG(ERR, \"Invalid mlx5 class options %s.\"\n> +                       \" Maybe typo in device class argument setting?\",\n> +                       class_names);\n> +       return *ret;\n> +}\n> +\n> +static int\n> +parse_class_options(const struct rte_devargs *devargs)\n> +{\n> +       const char *key = MLX5_CLASS_ARG_NAME;\n> +       struct rte_kvargs *kvlist;\n> +       int ret = 0;\n> +\n> +       if (devargs == NULL)\n> +               return 0;\n> +       kvlist = rte_kvargs_parse(devargs->args, NULL);\n> +       if (kvlist == NULL)\n> +               return 0;\n> +       if (rte_kvargs_count(kvlist, key))\n> +               rte_kvargs_process(kvlist, key, bus_cmdline_options_handler,\n> +                                  &ret);\n> +       rte_kvargs_free(kvlist);\n> +       return ret;\n> +}\n> +\n> +static bool\n> +mlx5_bus_match(const struct mlx5_pci_driver *drv,\n> +              const struct rte_pci_device *pci_dev)\n> +{\n> +       const struct rte_pci_id *id_table;\n> +\n> +       for (id_table = drv->pci_driver.id_table; id_table->vendor_id != 0;\n> +            id_table++) {\n> +               /* Check if device's ids match the class driver's ids. */\n> +               if (id_table->vendor_id != pci_dev->id.vendor_id &&\n> +                   id_table->vendor_id != PCI_ANY_ID)\n> +                       continue;\n> +               if (id_table->device_id != pci_dev->id.device_id &&\n> +                   id_table->device_id != PCI_ANY_ID)\n> +                       continue;\n> +               if (id_table->subsystem_vendor_id !=\n> +                   pci_dev->id.subsystem_vendor_id &&\n> +                   id_table->subsystem_vendor_id != PCI_ANY_ID)\n> +                       continue;\n> +               if (id_table->subsystem_device_id !=\n> +                   pci_dev->id.subsystem_device_id &&\n> +                   id_table->subsystem_device_id != PCI_ANY_ID)\n> +                       continue;\n> +               if (id_table->class_id != pci_dev->id.class_id &&\n> +                   id_table->class_id != RTE_CLASS_ANY_ID)\n> +                       continue;\n> +               return true;\n> +       }\n> +       return false;\n> +}\n> +\n> +static int\n> +is_valid_class_combination(uint32_t user_classes)\n> +{\n> +       unsigned int i;\n> +\n> +       /* Verify if user specified valid supported combination. */\n\nAre there invalid supported combinations? :-)\n\n/* Verify if the user specified a supported combination. */\n\n\n> +       for (i = 0; i < RTE_DIM(mlx5_class_combinations); i++) {\n> +               if (mlx5_class_combinations[i] == user_classes)\n> +                       return 0;\n> +       }\n> +       /* Not found any valid class combination. */\n> +       return -EINVAL;\n> +}\n> +\n> +static struct mlx5_pci_device *\n> +pci_to_mlx5_device(const struct rte_pci_device *pci_dev)\n> +{\n> +       struct mlx5_pci_device *dev;\n> +\n> +       TAILQ_FOREACH(dev, &devices_list, next) {\n> +               if (dev->pci_dev == pci_dev)\n> +                       return dev;\n> +       }\n> +       return NULL;\n> +}\n> +\n> +static bool\n> +device_class_enabled(const struct mlx5_pci_device *device, uint32_t class)\n> +{\n> +       return (device->classes_loaded & class) ? true : false;\n> +}\n> +\n> +static void\n> +dev_release(struct mlx5_pci_device *dev)\n> +{\n> +       TAILQ_REMOVE(&devices_list, dev, next);\n> +       rte_free(dev);\n> +}\n> +\n> +static int\n> +drivers_remove(struct mlx5_pci_device *dev, uint32_t enabled_classes)\n> +{\n> +       struct mlx5_pci_driver *driver;\n> +       int local_ret = -ENODEV;\n> +       unsigned int i = 0;\n> +       int ret = 0;\n> +\n> +       enabled_classes &= dev->classes_loaded;\n> +       while (enabled_classes) {\n> +               driver = driver_get(RTE_BIT64(i));\n> +               if (driver) {\n> +                       local_ret = driver->pci_driver.remove(dev->pci_dev);\n> +                       if (!local_ret)\n> +                               dev->classes_loaded &= ~RTE_BIT64(i);\n> +                       else if (ret == 0)\n> +                               ret = local_ret;\n> +               }\n> +               enabled_classes &= ~RTE_BIT64(i);\n> +               i++;\n> +       }\n> +       if (local_ret)\n> +               ret = local_ret;\n> +       return ret;\n> +}\n> +\n> +static int\n> +drivers_probe(struct mlx5_pci_device *dev, struct rte_pci_driver *pci_drv,\n> +             struct rte_pci_device *pci_dev, uint32_t user_classes)\n> +{\n> +       struct mlx5_pci_driver *driver;\n> +       uint32_t enabled_classes = 0;\n> +       bool already_loaded;\n> +       int ret;\n> +\n> +       TAILQ_FOREACH(driver, &drv_list, next) {\n> +               if ((driver->driver_class & user_classes) == 0)\n> +                       continue;\n> +               if (!mlx5_bus_match(driver, pci_dev))\n> +                       continue;\n> +               already_loaded = dev->classes_loaded & driver->driver_class;\n> +               if (already_loaded &&\n> +                   !(driver->pci_driver.drv_flags & RTE_PCI_DRV_PROBE_AGAIN)) {\n> +                       DRV_LOG(ERR, \"Device %s is already probed\\n\",\n> +                               pci_dev->device.name);\n> +                       ret = -EEXIST;\n> +                       goto probe_err;\n> +               }\n> +               ret = driver->pci_driver.probe(pci_drv, pci_dev);\n> +               if (ret < 0) {\n> +                       DRV_LOG(ERR, \"Failed to load driver = %s.\\n\",\n> +                               driver->pci_driver.driver.name);\n> +                       goto probe_err;\n> +               }\n> +               enabled_classes |= driver->driver_class;\n> +       }\n> +       dev->classes_loaded |= enabled_classes;\n> +       return 0;\n> +probe_err:\n> +       /* Only unload drivers which are enabled which were enabled\n> +        * in this probe instance.\n> +        */\n> +       drivers_remove(dev, enabled_classes);\n> +       return ret;\n> +}\n> +\n> +/**\n> + * DPDK callback to register to probe multiple drivers for a PCI device.\n> + *\n> + * @param[in] pci_drv\n> + *   PCI driver structure.\n> + * @param[in] dev\n> + *   PCI device information.\n> + *\n> + * @return\n> + *   0 on success, a negative errno value otherwise and rte_errno is set.\n> + */\n> +static int\n> +mlx5_common_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,\n> +                     struct rte_pci_device *pci_dev)\n> +{\n> +       struct mlx5_pci_device *dev;\n> +       uint32_t user_classes = 0;\n> +       bool new_device = false;\n> +       int ret;\n> +\n> +       ret = parse_class_options(pci_dev->device.devargs);\n> +       if (ret < 0)\n> +               return ret;\n> +       user_classes = ret;\n> +       if (user_classes) {\n> +               /* Validate combination here. */\n> +               ret = is_valid_class_combination(user_classes);\n> +               if (ret) {\n> +                       DRV_LOG(ERR, \"Unsupported mlx5 classes supplied.\");\n> +                       return ret;\n> +               }\n> +       } else {\n> +               /* Default to net class. */\n> +               user_classes = MLX5_CLASS_NET;\n> +       }\n> +       dev = pci_to_mlx5_device(pci_dev);\n> +       if (!dev) {\n> +               dev = rte_zmalloc(\"mlx5_pci_device\", sizeof(*dev), 0);\n> +               if (!dev)\n> +                       return -ENOMEM;\n> +               dev->pci_dev = pci_dev;\n> +               TAILQ_INSERT_HEAD(&devices_list, dev, next);\n> +               new_device = true;\n> +       }\n> +       ret = drivers_probe(dev, pci_drv, pci_dev, user_classes);\n> +       if (ret)\n> +               goto class_err;\n> +       return 0;\n> +class_err:\n> +       if (new_device)\n> +               dev_release(dev);\n> +       return ret;\n> +}\n> +\n> +/**\n> + * DPDK callback to remove one or more drivers for a PCI device.\n> + *\n> + * This function removes all drivers probed for a given PCI device.\n> + *\n> + * @param[in] pci_dev\n> + *   Pointer to the PCI device.\n> + *\n> + * @return\n> + *   0 on success, the function cannot fail.\n> + */\n> +static int\n> +mlx5_common_pci_remove(struct rte_pci_device *pci_dev)\n> +{\n> +       struct mlx5_pci_device *dev;\n> +       int ret;\n> +\n> +       dev = pci_to_mlx5_device(pci_dev);\n> +       if (!dev)\n> +               return -ENODEV;\n> +       /* Matching device found, cleanup and unload drivers. */\n> +       ret = drivers_remove(dev, dev->classes_loaded);\n> +       if (!ret)\n> +               dev_release(dev);\n> +       return ret;\n> +}\n> +\n> +static int\n> +mlx5_common_pci_dma_map(struct rte_pci_device *pci_dev, void *addr,\n> +                       uint64_t iova, size_t len)\n> +{\n> +       struct mlx5_pci_driver *driver = NULL;\n> +       struct mlx5_pci_driver *temp;\n> +       struct mlx5_pci_device *dev;\n> +       int ret = -EINVAL;\n> +\n> +       dev = pci_to_mlx5_device(pci_dev);\n> +       if (!dev)\n> +               return -ENODEV;\n> +       TAILQ_FOREACH(driver, &drv_list, next) {\n> +               if (device_class_enabled(dev, driver->driver_class) &&\n> +                   driver->pci_driver.dma_map) {\n> +                       ret = driver->pci_driver.dma_map(pci_dev, addr,\n> +                                                        iova, len);\n> +                       if (ret)\n> +                               goto map_err;\n> +               }\n> +       }\n> +       return ret;\n> +map_err:\n> +       TAILQ_FOREACH(temp, &drv_list, next) {\n> +               if (temp == driver)\n> +                       break;\n> +               if (device_class_enabled(dev, temp->driver_class) &&\n> +                   temp->pci_driver.dma_map && temp->pci_driver.dma_unmap)\n> +                       temp->pci_driver.dma_unmap(pci_dev, addr, iova, len);\n> +       }\n> +       return ret;\n> +}\n> +\n> +static int\n> +mlx5_common_pci_dma_unmap(struct rte_pci_device *pci_dev, void *addr,\n> +                         uint64_t iova, size_t len)\n> +{\n> +       struct mlx5_pci_driver *driver;\n> +       struct mlx5_pci_device *dev;\n> +       int local_ret = -EINVAL;\n> +       int ret;\n> +\n> +       dev = pci_to_mlx5_device(pci_dev);\n> +       if (!dev)\n> +               return -ENODEV;\n> +       ret = 0;\n> +       /* There is no unmap error recovery in current implementation. */\n> +       TAILQ_FOREACH_REVERSE(driver, &drv_list, mlx5_pci_bus_drv_head, next) {\n> +               if (device_class_enabled(dev, driver->driver_class) &&\n> +                   driver->pci_driver.dma_unmap) {\n> +                       local_ret = driver->pci_driver.dma_unmap(pci_dev, addr,\n> +                                                                iova, len);\n> +                       if (local_ret && (ret == 0))\n> +                               ret = local_ret;\n> +               }\n> +       }\n> +       if (local_ret)\n> +               ret = local_ret;\n> +       return ret;\n> +}\n> +\n> +/* PCI ID table is build dynamically based on registered mlx5 drivers. */\n\nbuilt\n\n> +static struct rte_pci_id *mlx5_pci_id_table;\n> +\n> +static struct rte_pci_driver mlx5_pci_driver = {\n> +       .driver = {\n> +               .name = \"mlx5_pci\",\n> +       },\n> +       .probe = mlx5_common_pci_probe,\n> +       .remove = mlx5_common_pci_remove,\n> +       .dma_map = mlx5_common_pci_dma_map,\n> +       .dma_unmap = mlx5_common_pci_dma_unmap,\n> +};\n> +\n> +static int\n> +pci_id_table_size_get(const struct rte_pci_id *id_table)\n> +{\n> +       int table_size = 0;\n> +\n> +       for (; id_table->vendor_id != 0; id_table++)\n> +               table_size++;\n> +       return table_size;\n> +}\n> +\n> +static bool\n> +pci_id_exists(const struct rte_pci_id *id, const struct rte_pci_id *table,\n> +             int next_idx)\n> +{\n> +       int current_size = next_idx - 1;\n> +       int i;\n> +\n> +       for (i = 0; i < current_size; i++) {\n> +               if (id->device_id == table[i].device_id &&\n> +                   id->vendor_id == table[i].vendor_id &&\n> +                   id->subsystem_vendor_id == table[i].subsystem_vendor_id &&\n> +                   id->subsystem_device_id == table[i].subsystem_device_id)\n> +                       return true;\n> +       }\n> +       return false;\n> +}\n> +\n> +static void\n> +pci_id_insert(struct rte_pci_id *new_table, int *next_idx,\n> +             const struct rte_pci_id *id_table)\n> +{\n> +       /* Traverse the id_table, check if entry exists in new_table;\n> +        * Add non duplicate entries to new table.\n> +        */\n> +       for (; id_table->vendor_id != 0; id_table++) {\n> +               if (!pci_id_exists(id_table, new_table, *next_idx)) {\n> +                       /* New entry; add to the table. */\n> +                       new_table[*next_idx] = *id_table;\n> +                       (*next_idx)++;\n> +               }\n> +       }\n> +}\n> +\n> +static int\n> +pci_ids_table_update(const struct rte_pci_id *driver_id_table)\n> +{\n> +       const struct rte_pci_id *id_iter;\n> +       struct rte_pci_id *updated_table;\n> +       struct rte_pci_id *old_table;\n> +       int num_ids = 0;\n> +       int i = 0;\n> +\n> +       old_table = mlx5_pci_id_table;\n> +       if (old_table)\n> +               num_ids = pci_id_table_size_get(old_table);\n> +       num_ids += pci_id_table_size_get(driver_id_table);\n> +       /* Increase size by one for the termination entry of vendor_id = 0. */\n> +       num_ids += 1;\n> +       updated_table = calloc(num_ids, sizeof(*updated_table));\n> +       if (!updated_table)\n> +               return -ENOMEM;\n> +       if (TAILQ_EMPTY(&drv_list)) {\n> +               /* Copy the first driver's ID table. */\n> +               for (id_iter = driver_id_table; id_iter->vendor_id != 0;\n> +                    id_iter++, i++)\n> +                       updated_table[i] = *id_iter;\n> +       } else {\n> +               /* First copy existing table entries. */\n> +               for (id_iter = old_table; id_iter->vendor_id != 0;\n> +                    id_iter++, i++)\n> +                       updated_table[i] = *id_iter;\n> +               /* New id to be added at the end of current ID table. */\n> +               pci_id_insert(updated_table, &i, driver_id_table);\n> +       }\n> +       /* Terminate table with empty entry. */\n> +       updated_table[i].vendor_id = 0;\n> +       mlx5_pci_driver.id_table = updated_table;\n> +       mlx5_pci_id_table = updated_table;\n> +       if (old_table)\n> +               free(old_table);\n> +       return 0;\n> +}\n> +\n> +void\n> +mlx5_pci_driver_register(struct mlx5_pci_driver *driver)\n> +{\n> +       int ret;\n> +\n> +       ret = pci_ids_table_update(driver->pci_driver.id_table);\n> +       if (ret)\n> +               return;\n> +       mlx5_pci_driver.drv_flags |= driver->pci_driver.drv_flags;\n> +       TAILQ_INSERT_TAIL(&drv_list, driver, next);\n> +}\n> +\n> +void mlx5_common_pci_init(void)\n> +{\n> +       const struct rte_pci_id empty_table[] = {\n> +               {\n> +                       .vendor_id = 0\n> +               },\n> +       };\n> +\n> +       /* All mlx5 PMDs constructor runs at same priority. So any of the PMD\n\n/* All mlx5 PMDs constructors run at the same priority. So any of the PMD\n\n\n> +        * including this one can register the PCI table first. If any other\n> +        * PMD(s) have registered the PCI ID table, No need to register an empty\n> +        * default one.\n> +        */\n> +       if (mlx5_pci_id_table == NULL && pci_ids_table_update(empty_table))\n> +                       return;\n> +       rte_pci_register(&mlx5_pci_driver);\n> +}\n> +\n> +RTE_FINI(mlx5_common_pci_finish)\n> +{\n> +       if (mlx5_pci_id_table != NULL) {\n> +               /* Constructor doesn't register with PCI bus if it failed\n> +                * to build the table.\n> +                */\n> +               rte_pci_unregister(&mlx5_pci_driver);\n> +               free(mlx5_pci_id_table);\n> +       }\n> +}\n\nI don't like the asymmetry between multiple constructors calling a\ncommon init / and a single destructor in the common code.\nBut this is not really an issue.\n\n\n> +RTE_PMD_EXPORT_NAME(mlx5_common_pci, __COUNTER__);\n\nThe pci driver name is mlx5_pci iirc.\n\nBut if there is no info about this driver to export, you can remove\nRTE_PMD_EXPORT_NAME.\n\n\n> diff --git a/drivers/common/mlx5/mlx5_common_pci.h b/drivers/common/mlx5/mlx5_common_pci.h\n> new file mode 100644\n> index 000000000..41b73e17a\n> --- /dev/null\n> +++ b/drivers/common/mlx5/mlx5_common_pci.h\n> @@ -0,0 +1,77 @@\n> +/* SPDX-License-Identifier: BSD-3-Clause\n> + * Copyright 2020 Mellanox Technologies, Ltd\n> + */\n> +\n> +#ifndef _MLX5_COMMON_PCI_H_\n> +#define _MLX5_COMMON_PCI_H_\n> +\n> +/**\n> + * @file\n> + *\n> + * RTE Mellanox PCI Driver Interface\n> + * Mellanox ConnectX PCI device supports multiple class (net/vdpa/regex)\n> + * devices. This layer enables creating such multiple class of devices on a\n> + * single PCI device by allowing to bind multiple class specific device\n> + * driver to attach to mlx5_pci driver.\n> + *\n> + * -----------    ------------    -------------\n> + * |   mlx5  |    |   mlx5   |    |   mlx5    |\n> + * | net pmd |    | vdpa pmd |    | regex pmd |\n> + * -----------    ------------    -------------\n> + *      \\              |                 /\n> + *       \\             |                /\n> + *        \\       --------------       /\n> + *         \\______|   mlx5     |_____ /\n> + *                | pci common |\n> + *                --------------\n> + *                     |\n> + *                 -----------\n> + *                 |   mlx5  |\n> + *                 | pci dev |\n> + *                 -----------\n> + *\n> + * - mlx5 pci driver binds to mlx5 PCI devices defined by PCI\n> + *   ID table of all related mlx5 PCI devices.\n> + * - mlx5 class driver such as net, vdpa, regex PMD defines its\n> + *   specific PCI ID table and mlx5 bus driver probes matching\n> + *   class drivers.\n> + * - mlx5 pci bus driver is cental place that validates supported\n\ncentral\n\n\n> + *   class combinations.\n> + */\n> +\n> +#ifdef __cplusplus\n> +extern \"C\" {\n> +#endif /* __cplusplus */\n> +\n> +#include <rte_pci.h>\n> +#include <rte_bus_pci.h>\n> +\n> +#include <mlx5_common.h>\n> +\n> +void mlx5_common_pci_init(void);\n> +\n> +/**\n> + * A structure describing a mlx5 pci driver.\n> + */\n> +struct mlx5_pci_driver {\n> +       struct rte_pci_driver pci_driver;       /**< Inherit core pci driver. */\n> +       uint32_t driver_class;  /**< Class of this driver, enum mlx5_class */\n> +       TAILQ_ENTRY(mlx5_pci_driver) next;\n> +};\n> +\n> +/**\n> + * Register a mlx5_pci device driver.\n> + *\n> + * @param driver\n> + *   A pointer to a mlx5_pci_driver structure describing the driver\n> + *   to be registered.\n> + */\n> +__rte_internal\n> +void\n> +mlx5_pci_driver_register(struct mlx5_pci_driver *driver);\n> +\n> +#ifdef __cplusplus\n> +}\n> +#endif /* __cplusplus */\n> +\n> +#endif /* _MLX5_COMMON_PCI_H_ */\n> diff --git a/drivers/common/mlx5/rte_common_mlx5_version.map b/drivers/common/mlx5/rte_common_mlx5_version.map\n> index 65f25252a..73cf72548 100644\n> --- a/drivers/common/mlx5/rte_common_mlx5_version.map\n> +++ b/drivers/common/mlx5/rte_common_mlx5_version.map\n> @@ -91,5 +91,7 @@ INTERNAL {\n>         mlx5_malloc;\n>         mlx5_realloc;\n>         mlx5_free;\n> +\n> +       mlx5_pci_driver_register;\n>  };\n>\n> diff --git a/drivers/meson.build b/drivers/meson.build\n> index e6d0409aa..95df7ef1d 100644\n> --- a/drivers/meson.build\n> +++ b/drivers/meson.build\n> @@ -5,6 +5,7 @@\n>  subdirs = [\n>         'common',\n>         'bus',\n> +       'common/mlx5', # depends on bus.\n>         'mempool', # depends on common and bus.\n>         'net',     # depends on common, bus, mempool\n>         'raw',     # depends on common, bus and net.\n> --\n> 2.25.4\n>\n\nReviewed-by: David Marchand <david.marchand@redhat.com>",
        "headers": {
            "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n <mailto:dev-request@dpdk.org?subject=subscribe>",
            "X-Mailman-Version": "2.1.15",
            "X-Mimecast-Originator": "redhat.com",
            "Authentication-Results": "relay.mimecast.com;\n auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com",
            "X-Mimecast-Spam-Score": "0",
            "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n s=mimecast20190719; t=1595601622;\n h=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n to:to:cc:cc:mime-version:mime-version:content-type:content-type:\n in-reply-to:in-reply-to:references:references;\n bh=QyPpHTPx7huOh96whO0X2tplTPjwuY3f9WkRf8BgwX0=;\n b=QUH7FXUhiKmczycQz5bOt6k6LyZfVwQhHtpx3XemYxhgCxFGzTBJ3BTHROwyNjXJkATowS\n 5t1Vp7n+CwU4eVSmHkqQMr+f+axzYH4Gz9+42rvF/EE/SmfuO0dlDa4CZOxEzOHweR9I2k\n 8HxMFprIMvMFerKj7AFCPTS6rxFOo0o=",
            "Precedence": "list",
            "X-Gm-Message-State": "AOAM533n4IXJKBO9Adg7QcNLn49wtAIx8x4A2BWHrphQLiOe+u0gvfNK\n x8UKfO3OR1cyF9N8xrkYoPcblLgvi89DR7SxkbA3/BLwYDoX83urRPWf6lG3d9y4+6RWH8fdTIX\n wZDO/8e/LgAyAWdPk2ME=",
            "X-Google-Smtp-Source": "\n ABdhPJz3J5AdXSfQNSDxJ9XJNJBi5J8rN3hUjv7ESFHTIYk7gn6dIHrdjYuNndS0EJodVsgzW+nS/ZIZKP6i5G1lTsE=",
            "List-Post": "<mailto:dev@dpdk.org>",
            "MIME-Version": "1.0",
            "List-Id": "DPDK patches and discussions <dev.dpdk.org>",
            "X-BeenThere": "dev@dpdk.org",
            "References": "<20200610171728.89-2-parav@mellanox.com>\n <20200723200910.376581-1-parav@mellanox.com>\n <20200723200910.376581-9-parav@mellanox.com>",
            "Subject": "Re: [dpdk-dev] [PATCH v8 08/10] common/mlx5: introduce layer to\n support multiple class drivers",
            "Content-Type": "text/plain; charset=\"UTF-8\"",
            "From": "David Marchand <david.marchand@redhat.com>",
            "Received": [
                "from dpdk.org (dpdk.org [92.243.14.124])\n\tby inbox.dpdk.org (Postfix) with ESMTP id 03BF9A0527;\n\tFri, 24 Jul 2020 16:41:45 +0200 (CEST)",
                "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 255301C038;\n\tFri, 24 Jul 2020 16:40:24 +0200 (CEST)",
                "from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com\n [205.139.110.61]) by dpdk.org (Postfix) with ESMTP id EE1661C031\n for <dev@dpdk.org>; Fri, 24 Jul 2020 16:40:22 +0200 (CEST)",
                "from mail-vk1-f197.google.com (mail-vk1-f197.google.com\n [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id\n us-mta-60-ZsNvW_ZkOtqOJQ7I22iSvA-1; Fri, 24 Jul 2020 10:40:18 -0400",
                "by mail-vk1-f197.google.com with SMTP id d132so2983117vke.16\n for <dev@dpdk.org>; Fri, 24 Jul 2020 07:40:18 -0700 (PDT)"
            ],
            "List-Archive": "<http://mails.dpdk.org/archives/dev/>",
            "X-Original-To": "patchwork@inbox.dpdk.org",
            "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n <mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20161025;\n h=x-gm-message-state:mime-version:references:in-reply-to:from:date\n :message-id:subject:to:cc;\n bh=QyPpHTPx7huOh96whO0X2tplTPjwuY3f9WkRf8BgwX0=;\n b=c5/blMs6H3pbXi8VgL2vSmsVD3c/eZbob9KQzJZn9mJBa5upakagGwqfmwOsRvBd3G\n xVTeaftYCUYlLBcY0kPCr0UXhh4XrxoUhlzoG0edYd/TLUST++dbWMe8f+wmaQ5KFeCF\n HcPikaIZOUE1ZqUPZ3taN8agjoOn5kNhDWuMie4VcOOlAg7G5/5JG/bOzTt+VnLFXBRk\n K3KMbDxVDmoBp/EPjmPfY2PY3QLnWqth3X6cnVakAKmjEJLPw3s8VbF7Pnw1ZbeGVfnj\n sziD95NU4Ak+4DavhGhbh2o95lnBeasWwWBD4Tx1d9JJJUIP4j3ahOGOzgZEoQ9OGsD3\n y7gw==",
            "X-MC-Unique": "ZsNvW_ZkOtqOJQ7I22iSvA-1",
            "Message-ID": "\n <CAJFAV8xT0Uo9b73SfpA3Tt15A1K5+0BgLnNMuehbx5jyjtNKwg@mail.gmail.com>",
            "Date": "Fri, 24 Jul 2020 16:40:05 +0200",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>",
            "X-Received": [
                "by 2002:a9f:2b8d:: with SMTP id y13mr8207153uai.126.1595601617462;\n Fri, 24 Jul 2020 07:40:17 -0700 (PDT)",
                "by 2002:a9f:2b8d:: with SMTP id y13mr8207120uai.126.1595601616773;\n Fri, 24 Jul 2020 07:40:16 -0700 (PDT)"
            ],
            "To": "Parav Pandit <parav@mellanox.com>",
            "Delivered-To": "patchwork@inbox.dpdk.org",
            "In-Reply-To": "<20200723200910.376581-9-parav@mellanox.com>",
            "Cc": "dev <dev@dpdk.org>, Gaetan Rivet <grive@u256.net>,\n \"Yigit, Ferruh\" <ferruh.yigit@intel.com>,\n Thomas Monjalon <thomas@monjalon.net>,\n Raslan <rasland@mellanox.com>, Ori Kam <orika@mellanox.com>,\n Matan Azrad <matan@mellanox.com>, Joyce Kong <joyce.kong@arm.com>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Return-Path": "<dev-bounces@dpdk.org>"
        }
    }
]