[RFC,v2] bus/auxiliary: introduce auxiliary bus

  Auxiliary [1] provides a way to split function into child-devices
representing sub-domains of functionality. Each auxiliary_device
represents a part of its parent functionality.

Auxiliary device is identified by unique device name, sysfs path:
  /sys/bus/auxiliary/devices/<name>

[1] kernel auxiliary bus document:
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Cc: Wang Haiyue <haiyue.wang@intel.com>
---
 MAINTAINERS                               |   5 +
 drivers/bus/auxiliary/auxiliary_common.c  | 408 ++++++++++++++++++++++
 drivers/bus/auxiliary/auxiliary_params.c  |  58 +++
 drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
 drivers/bus/auxiliary/meson.build         |  11 +
 drivers/bus/auxiliary/private.h           | 120 +++++++
 drivers/bus/auxiliary/rte_bus_auxiliary.h | 199 +++++++++++
 drivers/bus/auxiliary/version.map         |  10 +
 drivers/bus/meson.build                   |   1 +
 9 files changed, 959 insertions(+)
 create mode 100644 drivers/bus/auxiliary/auxiliary_common.c
 create mode 100644 drivers/bus/auxiliary/auxiliary_params.c
 create mode 100644 drivers/bus/auxiliary/linux/auxiliary.c
 create mode 100644 drivers/bus/auxiliary/meson.build
 create mode 100644 drivers/bus/auxiliary/private.h
 create mode 100644 drivers/bus/auxiliary/rte_bus_auxiliary.h
 create mode 100644 drivers/bus/auxiliary/version.map
  

On 10/05/2021 14:47, Xueming Li wrote:
> Auxiliary [1] provides a way to split function into child-devices
> representing sub-domains of functionality. Each auxiliary_device
> represents a part of its parent functionality.
> 
> Auxiliary device is identified by unique device name, sysfs path:
>   /sys/bus/auxiliary/devices/<name>
> 
> [1] kernel auxiliary bus document:
> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Cc: Wang Haiyue <haiyue.wang@intel.com>
> ---
>  MAINTAINERS                               |   5 +
>  drivers/bus/auxiliary/auxiliary_common.c  | 408 ++++++++++++++++++++++
>  drivers/bus/auxiliary/auxiliary_params.c  |  58 +++
>  drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
>  drivers/bus/auxiliary/meson.build         |  11 +
>  drivers/bus/auxiliary/private.h           | 120 +++++++
>  drivers/bus/auxiliary/rte_bus_auxiliary.h | 199 +++++++++++
>  drivers/bus/auxiliary/version.map         |  10 +
>  drivers/bus/meson.build                   |   1 +
>  9 files changed, 959 insertions(+)
>  create mode 100644 drivers/bus/auxiliary/auxiliary_common.c
>  create mode 100644 drivers/bus/auxiliary/auxiliary_params.c
>  create mode 100644 drivers/bus/auxiliary/linux/auxiliary.c
>  create mode 100644 drivers/bus/auxiliary/meson.build
>  create mode 100644 drivers/bus/auxiliary/private.h
>  create mode 100644 drivers/bus/auxiliary/rte_bus_auxiliary.h
>  create mode 100644 drivers/bus/auxiliary/version.map
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 44f3d322ed..ccab5b09c8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -522,6 +522,11 @@ F: doc/guides/mempool/octeontx2.rst
>  Bus Drivers
>  -----------
>  
> +Auxiliary bus driver
> +M: Parav Pandit <parav@nvidia.com>
> +M: Xueming Li <xuemingl@nvidia.com>
> +F: drivers/bus/auxiliary/
> +
>  Intel FPGA bus
>  M: Rosen Xu <rosen.xu@intel.com>
>  F: drivers/bus/ifpga/
> diff --git a/drivers/bus/auxiliary/auxiliary_common.c b/drivers/bus/auxiliary/auxiliary_common.c
> new file mode 100644
> index 0000000000..cef85ae991
> --- /dev/null
> +++ b/drivers/bus/auxiliary/auxiliary_common.c
> @@ -0,0 +1,408 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#include <string.h>
> +#include <inttypes.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/queue.h>
> +#include <rte_errno.h>
> +#include <rte_interrupts.h>
> +#include <rte_log.h>
> +#include <rte_bus.h>
> +#include <rte_per_lcore.h>
> +#include <rte_memory.h>
> +#include <rte_eal.h>
> +#include <rte_eal_paging.h>
> +#include <rte_string_fns.h>
> +#include <rte_common.h>
> +#include <rte_devargs.h>
> +
> +#include "private.h"
> +#include "rte_bus_auxiliary.h"
> +
> +
> +int auxiliary_bus_logtype;
> +
> +static struct rte_devargs *
> +auxiliary_devargs_lookup(const char *name)
> +{
> +	struct rte_devargs *devargs;
> +
> +	RTE_EAL_DEVARGS_FOREACH("auxiliary", devargs) {
> +		if (strcmp(devargs->name, name) == 0)
> +			return devargs;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Test whether the auxiliary device exist
> + */
> +__rte_weak bool
> +auxiliary_dev_exists(const char *name)
> +{
> +	RTE_SET_USED(name);
> +	return false;
> +}
> +
> +/*
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + */
> +__rte_weak int
> +auxiliary_scan(void)
> +{
> +	return 0;
> +}
> +
> +void
> +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev)
> +{
> +	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> +}
> +
> +/*
> + * Match the auxiliary Driver and Device using driver function.
> + */
> +bool
> +auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
> +		const struct rte_auxiliary_device *aux_dev)
> +{
> +	if (aux_drv->match == NULL)
> +		return false;
> +	return aux_drv->match(aux_dev->name);
> +}
> +
> +/*
> + * Call the probe() function of the driver.
> + */
> +static int
> +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> +			       struct rte_auxiliary_device *dev)
> +{
> +	enum rte_iova_mode iova_mode;
> +	int ret;
> +
> +	if ((dr == NULL) || (dev == NULL))
> +		return -EINVAL;
> +
> +	/* The device is not blocked; Check if driver supports it */
> +	if (!auxiliary_match(dr, dev))
> +		/* Match of device and driver failed */
> +		return 1;
> +
> +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> +		      dev->name, dev->device.numa_node);
> +
> +	/* no initialization when marked as blocked, return without error */
> +	if (dev->device.devargs != NULL &&
> +	    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
> +		AUXILIARY_LOG(INFO, "  Device is blocked, not initializing\n");
> +		return -1;
> +	}
> +
> +	if (dev->device.numa_node < 0) {
> +		AUXILIARY_LOG(WARNING, "  Invalid NUMA socket, default to 0\n");
> +		dev->device.numa_node = 0;
> +	}
> +
> +	AUXILIARY_LOG(DEBUG, "  Probe driver: %s\n", dr->driver.name);
> +
> +	iova_mode = rte_eal_iova_mode();
> +	if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) &&
> +	    iova_mode != RTE_IOVA_VA) {
> +		AUXILIARY_LOG(ERR, "  Expecting VA IOVA mode but current mode is PA, not initializing\n");
> +		return -EINVAL;
> +	}
> +
> +	dev->driver = dr;
> +
> +	AUXILIARY_LOG(INFO, "Probe auxiliary driver: %s device: %s (socket %i)\n",
> +		      dr->driver.name, dev->name, dev->device.numa_node);
> +	ret = dr->probe(dr, dev);
> +	if (ret)
> +		dev->driver = NULL;
> +	else
> +		dev->device.driver = &dr->driver;
> +
> +	return ret;
> +}
> +
> +/*
> + * Call the remove() function of the driver.
> + */
> +static int
> +rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev)
> +{
> +	struct rte_auxiliary_driver *dr;
> +	int ret = 0;
> +
> +	if (dev == NULL)
> +		return -EINVAL;
> +
> +	dr = dev->driver;
> +
> +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> +		      dev->name, dev->device.numa_node);
> +
> +	AUXILIARY_LOG(DEBUG, "  remove driver: %s %s\n",
> +		      dev->name, dr->driver.name);
> +
> +	if (dr->remove) {
> +		ret = dr->remove(dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* clear driver structure */
> +	dev->driver = NULL;
> +	dev->device.driver = NULL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Call the probe() function of all registered driver for the given device.
> + * Return < 0 if initialization failed.
> + * Return 1 if no driver is found for this device.
> + */
> +static int
> +auxiliary_probe_all_drivers(struct rte_auxiliary_device *dev)
> +{
> +	struct rte_auxiliary_driver *dr;
> +	int rc;
> +
> +	if (dev == NULL)
> +		return -EINVAL;
> +
> +	FOREACH_DRIVER_ON_AUXILIARYBUS(dr) {
> +		if (!dr->match(dev->name))
> +			continue;
> +
> +		rc = rte_auxiliary_probe_one_driver(dr, dev);
> +		if (rc < 0)
> +			/* negative value is an error */
> +			return rc;
> +		if (rc > 0)
> +			/* positive value means driver doesn't support it */
> +			continue;
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/*
> + * Scan the content of the auxiliary bus, and call the probe() function for
> + *
> + * all registered drivers that have a matching entry in its id_table
> + * for discovered devices.
> + */
> +static int
> +auxiliary_probe(void)
> +{
> +	struct rte_auxiliary_device *dev = NULL;
> +	size_t probed = 0, failed = 0;
> +	int ret = 0;
> +
> +	FOREACH_DEVICE_ON_AUXILIARYBUS(dev) {
> +		probed++;
> +
> +		ret = auxiliary_probe_all_drivers(dev);
> +		if (ret < 0) {
> +			if (ret != -EEXIST) {
> +				AUXILIARY_LOG(ERR, "Requested device %s cannot be used\n",
> +					      dev->name);
> +				rte_errno = errno;
> +				failed++;
> +			}
> +			ret = 0;
> +		}
> +	}
> +
> +	return (probed && probed == failed) ? -1 : 0;
> +}
> +
> +static int
> +auxiliary_parse(const char *name, void *addr)
> +{
> +	struct rte_auxiliary_driver *dr = NULL;
> +	const char **out = addr;
> +
> +	FOREACH_DRIVER_ON_AUXILIARYBUS(dr) {
> +		if (dr->match(name))
> +			break;
> +	}
> +	if (dr != NULL && addr != NULL)
> +		*out = name;
> +	return dr != NULL ? 0 : -1;
> +}
> +
> +/* register a driver */
> +void
> +rte_auxiliary_register(struct rte_auxiliary_driver *driver)
> +{
> +	TAILQ_INSERT_TAIL(&auxiliary_bus.driver_list, driver, next);
> +	driver->bus = &auxiliary_bus;
> +}
> +
> +/* unregister a driver */
> +void
> +rte_auxiliary_unregister(struct rte_auxiliary_driver *driver)
> +{
> +	TAILQ_REMOVE(&auxiliary_bus.driver_list, driver, next);
> +	driver->bus = NULL;
> +}
> +
> +/* Add a device to auxiliary bus */
> +void
> +auxiliary_add_device(struct rte_auxiliary_device *aux_dev)
> +{
> +	TAILQ_INSERT_TAIL(&auxiliary_bus.device_list, aux_dev, next);
> +}
> +
> +/* Insert a device into a predefined position in auxiliary bus */
> +void
> +auxiliary_insert_device(struct rte_auxiliary_device *exist_aux_dev,
> +			struct rte_auxiliary_device *new_aux_dev)
> +{
> +	TAILQ_INSERT_BEFORE(exist_aux_dev, new_aux_dev, next);
> +}
> +
> +/* Remove a device from auxiliary bus */
> +static void
> +rte_auxiliary_remove_device(struct rte_auxiliary_device *auxiliary_dev)
> +{
> +	TAILQ_REMOVE(&auxiliary_bus.device_list, auxiliary_dev, next);
> +}
> +
> +static struct rte_device *
> +auxiliary_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
> +		      const void *data)
> +{
> +	const struct rte_auxiliary_device *pstart;
> +	struct rte_auxiliary_device *adev;
> +
> +	if (start != NULL) {
> +		pstart = RTE_DEV_TO_AUXILIARY_CONST(start);
> +		adev = TAILQ_NEXT(pstart, next);
> +	} else {
> +		adev = TAILQ_FIRST(&auxiliary_bus.device_list);
> +	}
> +	while (adev != NULL) {
> +		if (cmp(&adev->device, data) == 0)
> +			return &adev->device;
> +		adev = TAILQ_NEXT(adev, next);
> +	}
> +	return NULL;
> +}
> +
> +static int
> +auxiliary_plug(struct rte_device *dev)
> +{
> +	if (!auxiliary_dev_exists(dev->name))
> +		return -ENOENT;
> +	return auxiliary_probe_all_drivers(RTE_DEV_TO_AUXILIARY(dev));
> +}
> +
> +static int
> +auxiliary_unplug(struct rte_device *dev)
> +{
> +	struct rte_auxiliary_device *adev;
> +	int ret;
> +
> +	adev = RTE_DEV_TO_AUXILIARY(dev);
> +	ret = rte_auxiliary_driver_remove_dev(adev);
> +	if (ret == 0) {
> +		rte_auxiliary_remove_device(adev);
> +		rte_devargs_remove(dev->devargs);
> +		free(adev);
> +	}
> +	return ret;
> +}
> +
> +static int
> +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
> +{
> +	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
> +
> +	if (dev == NULL || !aux_dev->driver) {
> +		rte_errno = EINVAL;
> +		return -1;
> +	}
> +	if (aux_dev->driver->dma_map)
> +		return aux_dev->driver->dma_map(aux_dev, addr, iova, len);
> +	rte_errno = ENOTSUP;
> +	return -1;
> +}
> +
> +static int
> +auxiliary_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova,
> +		    size_t len)
> +{
> +	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
> +
> +	if (dev == NULL || !aux_dev->driver) {
> +		rte_errno = EINVAL;
> +		return -1;
> +	}
> +	if (aux_dev->driver->dma_unmap)
> +		return aux_dev->driver->dma_unmap(aux_dev, addr, iova, len);
> +	rte_errno = ENOTSUP;
> +	return -1;
> +}
> +
> +bool
> +auxiliary_ignore_device(const char *name)
> +{
> +	struct rte_devargs *devargs = auxiliary_devargs_lookup(name);
> +
> +	switch (auxiliary_bus.bus.conf.scan_mode) {
> +	case RTE_BUS_SCAN_ALLOWLIST:
> +		if (devargs && devargs->policy == RTE_DEV_ALLOWED)
> +			return false;
> +		break;
> +	case RTE_BUS_SCAN_UNDEFINED:
> +	case RTE_BUS_SCAN_BLOCKLIST:
> +		if (devargs == NULL || devargs->policy != RTE_DEV_BLOCKED)
> +			return false;
> +		break;
> +	}
> +	return true;
> +}
> +
> +static enum rte_iova_mode
> +auxiliary_get_iommu_class(void)
> +{
> +	const struct rte_auxiliary_driver *drv;
> +
> +	FOREACH_DRIVER_ON_AUXILIARYBUS(drv) {
> +		if (drv->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)
> +			return RTE_IOVA_VA;
> +	}
> +
> +	return RTE_IOVA_DC;
> +}
> +
> +struct rte_auxiliary_bus auxiliary_bus = {
> +	.bus = {
> +		.scan = auxiliary_scan,
> +		.probe = auxiliary_probe,
> +		.find_device = auxiliary_find_device,
> +		.plug = auxiliary_plug,
> +		.unplug = auxiliary_unplug,
> +		.parse = auxiliary_parse,
> +		.dma_map = auxiliary_dma_map,
> +		.dma_unmap = auxiliary_dma_unmap,
> +		.get_iommu_class = auxiliary_get_iommu_class,
> +		.dev_iterate = auxiliary_dev_iterate,
> +	},
> +	.device_list = TAILQ_HEAD_INITIALIZER(auxiliary_bus.device_list),
> +	.driver_list = TAILQ_HEAD_INITIALIZER(auxiliary_bus.driver_list),
> +};
> +
> +RTE_REGISTER_BUS(auxiliary, auxiliary_bus.bus);
> +RTE_LOG_REGISTER(auxiliary_bus_logtype, bus.auxiliary, NOTICE);
> diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> new file mode 100644
> index 0000000000..5a1b029839
> --- /dev/null
> +++ b/drivers/bus/auxiliary/auxiliary_params.c
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#include <string.h>
> +
> +#include <rte_bus.h>
> +#include <rte_dev.h>
> +#include <rte_errno.h>
> +#include <rte_kvargs.h>
> +
> +#include "private.h"
> +#include "rte_bus_auxiliary.h"
> +
> +enum auxiliary_params {
> +	RTE_AUXILIARY_PARAM_NAME,
> +};
> +
> +static const char * const auxiliary_params_keys[] = {
> +	[RTE_AUXILIARY_PARAM_NAME] = "name",
> +};
> +
> +static int
> +auxiliary_dev_match(const struct rte_device *dev,
> +	      const void *_kvlist)
> +{
> +	const struct rte_kvargs *kvlist = _kvlist;
> +	int ret;
> +
> +	ret = rte_kvargs_process(kvlist,
> +			auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME],
> +			rte_kvargs_strcmp, (void *)(uintptr_t)dev->name);
> +
> +	return ret != 0 ? -1 : 0;
> +}
> +
> +void *
> +auxiliary_dev_iterate(const void *start,
> +		    const char *str,
> +		    const struct rte_dev_iterator *it __rte_unused)
> +{
> +	rte_bus_find_device_t find_device;
> +	struct rte_kvargs *kvargs = NULL;
> +	struct rte_device *dev;
> +
> +	if (str != NULL) {
> +		kvargs = rte_kvargs_parse(str, auxiliary_params_keys);
> +		if (kvargs == NULL) {
> +			RTE_LOG(ERR, EAL, "cannot parse argument list\n");
> +			rte_errno = EINVAL;
> +			return NULL;
> +		}
> +	}
> +	find_device = auxiliary_bus.bus.find_device;
> +	dev = find_device(start, auxiliary_dev_match, kvargs);
> +	rte_kvargs_free(kvargs);
> +	return dev;
> +}
> diff --git a/drivers/bus/auxiliary/linux/auxiliary.c b/drivers/bus/auxiliary/linux/auxiliary.c
> new file mode 100644
> index 0000000000..b75bb4d4a6
> --- /dev/null
> +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#include <string.h>
> +#include <dirent.h>
> +
> +#include <rte_log.h>
> +#include <rte_bus.h>
> +#include <rte_malloc.h>
> +#include <rte_devargs.h>
> +#include <rte_memcpy.h>
> +#include <eal_filesystem.h>
> +
> +#include "../rte_bus_auxiliary.h"
> +#include "../private.h"
> +
> +#define AUXILIARY_SYSFS_PATH "/sys/bus/auxiliary/devices"
> +
> +/**
> + * @file
> + * Linux auxiliary probing.
> + */
> +
> +/* Scan one auxiliary sysfs entry, and fill the devices list from it. */
> +static int
> +auxiliary_scan_one(const char *dirname, const char *name)
> +{
> +	struct rte_auxiliary_device *dev;
> +	struct rte_auxiliary_device *dev2;
> +	char filename[PATH_MAX];
> +	unsigned long tmp;
> +	int ret;
> +
> +	dev = malloc(sizeof(*dev));
> +	if (dev == NULL)
> +		return -1;
> +
> +	memset(dev, 0, sizeof(*dev));
> +	if (rte_strscpy(dev->name, name, sizeof(dev->name)) < 0) {
> +		free(dev);
> +		return -1;
> +	}
> +	dev->device.name = dev->name;
> +	dev->device.bus = &auxiliary_bus.bus;
> +
> +	/* Get numa node, default to 0 if not present */
> +	snprintf(filename, sizeof(filename), "%s/%s/numa_node",
> +		 dirname, name);
> +	if (access(filename, F_OK) != -1) {
> +		if (eal_parse_sysfs_value(filename, &tmp) == 0)
> +			dev->device.numa_node = tmp;
> +		else
> +			dev->device.numa_node = -1;
> +	} else {
> +		dev->device.numa_node = 0;
> +	}
> +
> +	auxiliary_on_scan(dev);
> +
> +	/* Device is valid, add in list (sorted) */
> +	TAILQ_FOREACH(dev2, &auxiliary_bus.device_list, next) {
> +		ret = strcmp(dev->name, dev2->name);
> +		if (ret > 0)
> +			continue;
> +		if (ret < 0) {
> +			auxiliary_insert_device(dev2, dev);
> +		} else { /* already registered */
> +			if (rte_dev_is_probed(&dev2->device) &&
> +			    dev2->device.devargs != dev->device.devargs) {
> +				/* To probe device with new devargs. */
> +				rte_devargs_remove(dev2->device.devargs);
> +				auxiliary_on_scan(dev2);
> +			}
> +			free(dev);
> +		}
> +		return 0;
> +	}
> +	auxiliary_add_device(dev);
> +	return 0;
> +}
> +
> +/*
> + * Test whether the auxiliary device exist
> + */
> +bool
> +auxiliary_dev_exists(const char *name)
> +{
> +	DIR *dir;
> +	char dirname[PATH_MAX];
> +
> +	snprintf(dirname, sizeof(dirname), "%s/%s",
> +		 AUXILIARY_SYSFS_PATH, name);
> +	dir = opendir(dirname);
> +	if (dir == NULL)
> +		return false;
> +	closedir(dir);
> +	return true;
> +}
> +
> +/*
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + */
> +int
> +auxiliary_scan(void)
> +{
> +	struct dirent *e;
> +	DIR *dir;
> +	char dirname[PATH_MAX];
> +	struct rte_auxiliary_driver *drv;
> +
> +	dir = opendir(AUXILIARY_SYSFS_PATH);
> +	if (dir == NULL) {
> +		AUXILIARY_LOG(INFO, "%s not found, is auxiliary module loaded?\n",
> +			      AUXILIARY_SYSFS_PATH);
> +		return 0;
> +	}
> +
> +	while ((e = readdir(dir)) != NULL) {
> +		if (e->d_name[0] == '.')
> +			continue;
> +
> +		if (auxiliary_ignore_device(e->d_name))
> +			continue;
> +
> +		snprintf(dirname, sizeof(dirname), "%s/%s",
> +			 AUXILIARY_SYSFS_PATH, e->d_name);
> +
> +		/* Ignore if no driver can handle. */
> +		FOREACH_DRIVER_ON_AUXILIARYBUS(drv) {
> +			if (drv->match(e->d_name))
> +				break;
> +		}
> +		if (drv == NULL)
> +			continue;
> +
> +		if (auxiliary_scan_one(dirname, e->d_name) < 0)
> +			goto error;
> +	}
> +	closedir(dir);
> +	return 0;
> +
> +error:
> +	closedir(dir);
> +	return -1;
> +}
> diff --git a/drivers/bus/auxiliary/meson.build b/drivers/bus/auxiliary/meson.build
> new file mode 100644
> index 0000000000..f85608afd0
> --- /dev/null
> +++ b/drivers/bus/auxiliary/meson.build
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 Mellanox Technologies, Ltd
> +
> +headers = files('rte_bus_auxiliary.h')
> +sources = files('auxiliary_common.c',
> +	'auxiliary_params.c')
> +if is_linux
> +	sources += files('linux/auxiliary.c')
> +endif
> +deps += ['kvargs']
> +
> diff --git a/drivers/bus/auxiliary/private.h b/drivers/bus/auxiliary/private.h
> new file mode 100644
> index 0000000000..3529348900
> --- /dev/null
> +++ b/drivers/bus/auxiliary/private.h
> @@ -0,0 +1,120 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#ifndef _AUXILIARY_PRIVATE_H_
> +#define _AUXILIARY_PRIVATE_H_
> +
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include "rte_bus_auxiliary.h"
> +
> +extern struct rte_auxiliary_bus auxiliary_bus;
> +extern int auxiliary_bus_logtype;
> +
> +#define AUXILIARY_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)
> +
> +/* Auxiliary bus iterators */
> +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p)	\
> +		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> +
> +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p)	\
> +		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
> +
> +/**
> + * Test whether the auxiliary device exist
> + *
> + * @param name
> + *  Auxiliary device name
> + * @return
> + *  true on exists, false otherwise
> + */
> +bool auxiliary_dev_exists(const char *name);
> +
> +/**
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + *
> + * @return
> + *  0 on success, negative on error
> + */
> +int auxiliary_scan(void);
> +
> +/**
> + * Setup or update device when being scanned.
> + *
> + * @param aux_dev
> + *	AUXILIARY device.
> + */
> +void auxiliary_on_scan(struct rte_auxiliary_device *aux_dev);
> +
> +/**
> + * Validate whether a device with given auxiliary device should be ignored
> + * or not.
> + *
> + * @param name
> + *	Auxiliary name of device to be validated
> + * @return
> + *	true: if device is to be ignored,
> + *	false: if device is to be scanned,
> + */
> +bool auxiliary_ignore_device(const char *name);
> +
> +/**
> + * Add an auxiliary device to the auxiliary bus (append to auxiliary Device
> + * list). This function also updates the bus references of the auxiliary
> + * Device (and the generic device object embedded within.
> + *
> + * @param aux_dev
> + *	AUXILIARY device to add
> + * @return void
> + */
> +void auxiliary_add_device(struct rte_auxiliary_device *aux_dev);
> +
> +/**
> + * Insert an auxiliary device in the auxiliary bus at a particular location
> + * in the device list. It also updates the auxiliary bus reference of the
> + * new devices to be inserted.
> + *
> + * @param exist_aux_dev
> + *	Existing auxiliary device in auxiliary bus
> + * @param new_aux_dev
> + *	AUXILIARY device to be added before exist_aux_dev
> + * @return void
> + */
> +void auxiliary_insert_device(struct rte_auxiliary_device *exist_aux_dev,
> +			     struct rte_auxiliary_device *new_aux_dev);
> +
> +/**
> + * Match the auxiliary Driver and Device by driver function
> + *
> + * @param aux_drv
> + *      auxiliary driver
> + * @param aux_dev
> + *      auxiliary device to match against the driver
> + * @return
> + *      the driver can handle the device
> + */
> +bool auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
> +		     const struct rte_auxiliary_device *aux_dev);
> +
> +/**
> + * Iterate over internal devices, matching any device against the provided
> + * string.
> + *
> + * @param start
> + *   Iteration starting point.
> + * @param str
> + *   Device string to match against.
> + * @param it
> + *   (unused) iterator structure.
> + * @return
> + *   A pointer to the next matching device if any.
> + *   NULL otherwise.
> + */
> +void *auxiliary_dev_iterate(const void *start, const char *str,
> +			    const struct rte_dev_iterator *it);
> +
> +#endif /* _AUXILIARY_PRIVATE_H_ */
> diff --git a/drivers/bus/auxiliary/rte_bus_auxiliary.h b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> new file mode 100644
> index 0000000000..d681464602
> --- /dev/null
> +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> @@ -0,0 +1,199 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#ifndef _RTE_BUS_AUXILIARY_H_
> +#define _RTE_BUS_AUXILIARY_H_
> +
> +/**
> + * @file
> + *
> + * RTE Auxiliary Bus Interface.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/queue.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +
> +#include <rte_debug.h>
> +#include <rte_interrupts.h>
> +#include <rte_dev.h>
> +#include <rte_bus.h>
> +#include <rte_kvargs.h>
> +
> +/* Forward declarations */
> +struct rte_auxiliary_driver;
> +struct rte_auxiliary_bus;
> +struct rte_auxiliary_device;
> +
> +/**
> + * Match function for the driver to decide if device can be handled.
> + *
> + * @param name
> + *   Pointer to the auxiliary device name.
> + * @return
> + *   Whether the driver can handle the auxiliary device.
> + */
> +typedef bool(*rte_auxiliary_match_t) (const char *name);
> +
> +/**
> + * Initialization function for the driver called during auxiliary probing.
> + *
> + * @param drv
> + *   Pointer to the auxiliary driver.
> + * @param dev
> + *   Pointer to the auxiliary device.
> + * @return
> + *   - 0 On success.
> + *   - Negative value and rte_errno is set otherwise.
> + */
> +typedef int(*rte_auxiliary_probe_t) (struct rte_auxiliary_driver *drv,
> +				     struct rte_auxiliary_device *dev);
> +
> +/**
> + * Uninitialization function for the driver called during hotplugging.
> + *
> + * @param dev
> + *   Pointer to the auxiliary device.
> + * @return
> + *   - 0 On success.
> + *   - Negative value and rte_errno is set otherwise.
> + */
> +typedef int (*rte_auxiliary_remove_t)(struct rte_auxiliary_device *dev);
> +
> +/**
> + * Driver-specific DMA mapping. After a successful call the device
> + * will be able to read/write from/to this segment.
> + *
> + * @param dev
> + *   Pointer to the auxiliary device.
> + * @param addr
> + *   Starting virtual address of memory to be mapped.
> + * @param iova
> + *   Starting IOVA address of memory to be mapped.
> + * @param len
> + *   Length of memory segment being mapped.
> + * @return
> + *   - 0 On success.
> + *   - Negative value and rte_errno is set otherwise.
> + */
> +typedef int (*rte_auxiliary_dma_map_t)(struct rte_auxiliary_device *dev,
> +				       void *addr, uint64_t iova, size_t len);
> +
> +/**
> + * Driver-specific DMA un-mapping. After a successful call the device
> + * will not be able to read/write from/to this segment.
> + *
> + * @param dev
> + *   Pointer to the auxiliary device.
> + * @param addr
> + *   Starting virtual address of memory to be unmapped.
> + * @param iova
> + *   Starting IOVA address of memory to be unmapped.
> + * @param len
> + *   Length of memory segment being unmapped.
> + * @return
> + *   - 0 On success.
> + *   - Negative value and rte_errno is set otherwise.
> + */
> +typedef int (*rte_auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
> +					 void *addr, uint64_t iova, size_t len);
> +
> +/**
> + * A structure describing an auxiliary device.
> + */
> +struct rte_auxiliary_device {
> +	TAILQ_ENTRY(rte_auxiliary_device) next;   /**< Next probed device. */
> +	char name[RTE_DEV_NAME_MAX_LEN + 1];      /**< ASCII device name */
> +	struct rte_device device;                 /**< Inherit core device */
> +	struct rte_intr_handle intr_handle;       /**< Interrupt handle */
> +	struct rte_auxiliary_driver *driver;      /**< driver used in probing */
> +};
> +
> +/** List of auxiliary devices */
> +TAILQ_HEAD(rte_auxiliary_device_list, rte_auxiliary_device);
> +/** List of auxiliary drivers */
> +TAILQ_HEAD(rte_auxiliary_driver_list, rte_auxiliary_driver);
> +
> +/**
> + * Structure describing the auxiliary bus
> + */
> +struct rte_auxiliary_bus {
> +	struct rte_bus bus;                  /**< Inherit the generic class */
> +	struct rte_auxiliary_device_list device_list;  /**< List of devices */
> +	struct rte_auxiliary_driver_list driver_list;  /**< List of drivers */
> +};
> +
> +/**
> + * A structure describing an auxiliary driver.
> + */
> +struct rte_auxiliary_driver {
> +	TAILQ_ENTRY(rte_auxiliary_driver) next; /**< Next in list. */
> +	struct rte_driver driver;            /**< Inherit core driver. */
> +	struct rte_auxiliary_bus *bus;       /**< Auxiliary bus reference. */
> +	rte_auxiliary_match_t match;         /**< Device match function. */
> +	rte_auxiliary_probe_t probe;         /**< Device Probe function. */
> +	rte_auxiliary_remove_t remove;       /**< Device Remove function. */
> +	rte_auxiliary_dma_map_t dma_map;     /**< Device dma map function. */
> +	rte_auxiliary_dma_unmap_t dma_unmap; /**< Device dma unmap function. */
> +	uint32_t drv_flags;                  /**< Flags RTE_auxiliary_DRV_*. */
> +};
> +
> +/**
> + * @internal
> + * Helper macro for drivers that need to convert to struct rte_auxiliary_device.
> + */
> +#define RTE_DEV_TO_AUXILIARY(ptr) \
> +	container_of(ptr, struct rte_auxiliary_device, device)
> +
> +#define RTE_DEV_TO_AUXILIARY_CONST(ptr) \
> +	container_of(ptr, const struct rte_auxiliary_device, device)
> +
> +#define RTE_ETH_DEV_TO_AUXILIARY(eth_dev) \
> +	RTE_DEV_TO_AUXILIARY((eth_dev)->device)
> +
> +/** Device driver needs IOVA as VA and cannot work with IOVA as PA */
> +#define RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA 0x002
> +
> +/**
> + * Register an auxiliary driver.
> + *
> + * @param driver
> + *   A pointer to a rte_auxiliary_driver structure describing the driver
> + *   to be registered.
> + */
> +__rte_experimental
> +void rte_auxiliary_register(struct rte_auxiliary_driver *driver);
> +
> +/** Helper for auxiliary device registration from driver instance */
> +#define RTE_PMD_REGISTER_AUXILIARY(nm, auxiliary_drv)		\
> +	RTE_INIT(auxiliaryinitfn_##nm)				\
> +	{							\
> +		(auxiliary_drv).driver.name = RTE_STR(nm);	\
> +		rte_auxiliary_register(&auxiliary_drv);		\
> +	}							\
> +	RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> +
> +/**
> + * Unregister an auxiliary driver.
> + *
> + * @param driver
> + *   A pointer to a rte_auxiliary_driver structure describing the driver
> + *   to be unregistered.
> + */
> +__rte_experimental
> +void rte_auxiliary_unregister(struct rte_auxiliary_driver *driver);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_BUS_AUXILIARY_H_ */
> diff --git a/drivers/bus/auxiliary/version.map b/drivers/bus/auxiliary/version.map
> new file mode 100644
> index 0000000000..3d270baea7
> --- /dev/null
> +++ b/drivers/bus/auxiliary/version.map
> @@ -0,0 +1,10 @@
> +DPDK_21 {
> +	local: *;
> +};
> +
> +EXPERIMENTAL {
> +	global:
> +

Add a comment here to indicate the release in which the expiermental symbols where added. 

> +	rte_auxiliary_register;
> +	rte_auxiliary_unregister;
> +};
> diff --git a/drivers/bus/meson.build b/drivers/bus/meson.build
> index c770c6ba1e..791536e606 100644
> --- a/drivers/bus/meson.build
> +++ b/drivers/bus/meson.build
> @@ -2,6 +2,7 @@
>  # Copyright(c) 2017 Intel Corporation
>  
>  drivers = [
> +        'auxiliary',
>          'dpaa',
>          'fslmc',
>          'ifpga',
>
  

10/05/2021 15:47, Xueming Li:
> Auxiliary [1] provides a way to split function into child-devices

Auxiliary -> Auxiliary bus

> representing sub-domains of functionality. Each auxiliary_device

auxiliary_device -> auxiliary device

> represents a part of its parent functionality.
> 
> Auxiliary device is identified by unique device name, sysfs path:
>   /sys/bus/auxiliary/devices/<name>
> 
> [1] kernel auxiliary bus document:
> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
[...]
> +++ b/drivers/bus/auxiliary/auxiliary_common.c
[...]
> +int auxiliary_bus_logtype;

You don't need to declare this variable, it is done in a macro.

> +
> +static struct rte_devargs *
> +auxiliary_devargs_lookup(const char *name)
> +{
> +	struct rte_devargs *devargs;
> +
> +	RTE_EAL_DEVARGS_FOREACH("auxiliary", devargs) {

"auxiliary" should be defined as a macro.

[...]
> +/*
> + * Test whether the auxiliary device exist
> + */
> +__rte_weak bool

The comment should explain it is a stub for OS
not supporting auxiliary bus.

> +auxiliary_dev_exists(const char *name)
> +{
> +	RTE_SET_USED(name);
> +	return false;
> +}
> +
> +/*
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + */

Same here about the stub comment.

> +__rte_weak int
> +auxiliary_scan(void)
> +{
> +	return 0;
> +}
> +

Please insert a comment here.

> +void
> +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev)
> +{
> +	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> +}
> +
> +/*
> + * Match the auxiliary Driver and Device using driver function.

driver and device do not need capital letters

> + */
> +bool
> +auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
> +		const struct rte_auxiliary_device *aux_dev)
> +{
> +	if (aux_drv->match == NULL)
> +		return false;
> +	return aux_drv->match(aux_dev->name);
> +}
> +
> +/*
> + * Call the probe() function of the driver.
> + */
> +static int
> +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> +			       struct rte_auxiliary_device *dev)
> +{
> +	enum rte_iova_mode iova_mode;
> +	int ret;
> +
> +	if ((dr == NULL) || (dev == NULL))
> +		return -EINVAL;
> +
> +	/* The device is not blocked; Check if driver supports it */

Please keep all the comments more uniform by starting with a capital
and ends with a point.

[...]
> +RTE_REGISTER_BUS(auxiliary, auxiliary_bus.bus);
> +RTE_LOG_REGISTER(auxiliary_bus_logtype, bus.auxiliary, NOTICE);

Please replace with RTE_LOG_REGISTER_DEFAULT.

[...]
> +++ b/drivers/bus/auxiliary/linux/auxiliary.c
[...]
> +/**
> + * @file
> + * Linux auxiliary probing.
> + */

No need of doxygen comment in a .c file.

[...]
> +++ b/drivers/bus/auxiliary/meson.build
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 Mellanox Technologies, Ltd
> +
> +headers = files('rte_bus_auxiliary.h')
> +sources = files('auxiliary_common.c',
> +	'auxiliary_params.c')
> +if is_linux
> +	sources += files('linux/auxiliary.c')
> +endif
> +deps += ['kvargs']

Please change the indent with spaces
and check with devtools/check-meson.py

[...]
> +++ b/drivers/bus/auxiliary/private.h
[...]
> +/* Auxiliary bus iterators */
> +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p)	\

Please avoid tabs at the end of the line.
A space is enough before the backslash.

> +		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> +
> +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p)	\
> +		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
> +
> +/**
> + * Test whether the auxiliary device exist
> + *
> + * @param name
> + *  Auxiliary device name
> + * @return
> + *  true on exists, false otherwise
> + */
> +bool auxiliary_dev_exists(const char *name);

Given it is not an API, no need of doxygen.
And the function is so simple that no need of comment at all.

[...]
> +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> +#ifndef _RTE_BUS_AUXILIARY_H_
> +#define _RTE_BUS_AUXILIARY_H_

No need of underscores before and after.
(yes I know a lot of DPDK files use such scheme)

> +
> +/**
> + * @file
> + *
> + * RTE Auxiliary Bus Interface.

No need of RTE, it has no meaning here.

> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/queue.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +
> +#include <rte_debug.h>
> +#include <rte_interrupts.h>
> +#include <rte_dev.h>
> +#include <rte_bus.h>
> +#include <rte_kvargs.h>
> +
> +/* Forward declarations */
> +struct rte_auxiliary_driver;
> +struct rte_auxiliary_bus;
> +struct rte_auxiliary_device;
> +
> +/**
> + * Match function for the driver to decide if device can be handled.
> + *
> + * @param name
> + *   Pointer to the auxiliary device name.
> + * @return
> + *   Whether the driver can handle the auxiliary device.
> + */
> +typedef bool(*rte_auxiliary_match_t) (const char *name);

I disagree with the earlier comment asking for typedef pointer
(based on one of my patches).
Actually Andrew's suggestion makes sense:
	http://mails.dpdk.org/archives/dev/2021-June/210665.html

[...]
> +++ b/drivers/bus/auxiliary/version.map
> +DPDK_21 {
> +	local: *;
> +};

We don't need this empty section.

> +
> +EXPERIMENTAL {
> +	global:
> +

As asked by Ray, we should add this line:
	# added in 21.08

> +	rte_auxiliary_register;
> +	rte_auxiliary_unregister;
> +};

Release notes are missing.
  

Hi Andrew,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, June 8, 2021 15:53
> To: Xueming Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Wang, Haiyue
> <haiyue.wang@intel.com>; andrew.rybchenko@oktetlabs.ru
> Subject: Re: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus
> 


> > +
> > +/**
> > + * Match function for the driver to decide if device can be handled.
> > + *
> > + * @param name
> > + *   Pointer to the auxiliary device name.
> > + * @return
> > + *   Whether the driver can handle the auxiliary device.
> > + */
> > +typedef bool(*rte_auxiliary_match_t) (const char *name);
> 
> I disagree with the earlier comment asking for typedef pointer
> (based on one of my patches).
> Actually Andrew's suggestion makes sense:
> 	http://mails.dpdk.org/archives/dev/2021-June/210665.html
> 

I didn't get the error, but the same warning, I missed something ? ;-)

1. w/o pointer

#include <stdio.h>

typedef int (gpu_malloc_t)(void *dev, size_t size, void **ptr);

static gpu_malloc_t *mlx5_gpu_malloc_fn;

static int
mlx5_gpu_malloc(size_t size, void **ptr)
{
	return 0;
}

int main()
{
	mlx5_gpu_malloc_fn = mlx5_gpu_malloc;

	mlx5_gpu_malloc_fn(NULL, 0, NULL);

	return 0;
}


gcc -Wall fun.c
fun.c: In function 'main':
fun.c:15:21: warning: assignment to 'int (*)(void *, size_t,  void **)' {aka 'int (*)(void *, long unsigned int,  void **)'} from incompatible pointer type 'int (*)(size_t,  void **)' {aka 'int (*)(long unsigned int,  void **)'} [-Wincompatible-pointer-types]
   15 |  mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
      |                     ^



2. w pointer

#include <stdio.h>

typedef int (*gpu_malloc_t)(void *dev, size_t size, void **ptr);

static gpu_malloc_t mlx5_gpu_malloc_fn;

static int
mlx5_gpu_malloc(size_t size, void **ptr)
{
	return 0;
}

int main()
{
	mlx5_gpu_malloc_fn = mlx5_gpu_malloc;

	mlx5_gpu_malloc_fn(NULL, 0, NULL);

	return 0;
}

gcc -Wall fun.c
fun.c: In function 'main':
fun.c:15:21: warning: assignment to 'gpu_malloc_t' {aka 'int (*)(void *, long unsigned int,  void **)'} from incompatible pointer type 'int (*)(size_t,  void **)' {aka 'int (*)(long unsigned int,  void **)'} [-Wincompatible-pointer-types]
   15 |  mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
      |                     ^

>
  

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Tuesday, May 11, 2021 5:48 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Neil Horman <nhorman@tuxdriver.com>; Wang Haiyue
> <haiyue.wang@intel.com>
> Subject: Re: [RFC v2] bus/auxiliary: introduce auxiliary bus
> 


// snip //

> > diff --git a/drivers/bus/auxiliary/version.map b/drivers/bus/auxiliary/version.map
> > new file mode 100644
> > index 0000000000..3d270baea7
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/version.map
> > @@ -0,0 +1,10 @@
> > +DPDK_21 {
> > +	local: *;
> > +};
> > +
> > +EXPERIMENTAL {
> > +	global:
> > +
> 
> Add a comment here to indicate the release in which the expiermental symbols where added.

Good suggestion, thanks!

> 
> > +	rte_auxiliary_register;
> > +	rte_auxiliary_unregister;
> > +};

// snip //
  

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Tuesday, June 8, 2021 4:41 PM
> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; andrew.rybchenko@oktetlabs.ru
> Subject: RE: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus
> 
> Hi Andrew,
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Tuesday, June 8, 2021 15:53
> > To: Xueming Li <xuemingl@nvidia.com>
> > Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> > <mdr@ashroe.eu>; Wang, Haiyue <haiyue.wang@intel.com>;
> > andrew.rybchenko@oktetlabs.ru
> > Subject: Re: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary
> > bus
> >
> 
> 
> > > +
> > > +/**
> > > + * Match function for the driver to decide if device can be handled.
> > > + *
> > > + * @param name
> > > + *   Pointer to the auxiliary device name.
> > > + * @return
> > > + *   Whether the driver can handle the auxiliary device.
> > > + */
> > > +typedef bool(*rte_auxiliary_match_t) (const char *name);
> >
> > I disagree with the earlier comment asking for typedef pointer (based
> > on one of my patches).
> > Actually Andrew's suggestion makes sense:
> > 	http://mails.dpdk.org/archives/dev/2021-June/210665.html
> >
> 
> I didn't get the error, but the same warning, I missed something ? ;-)
> 
> 1. w/o pointer
> 
> #include <stdio.h>
> 
> typedef int (gpu_malloc_t)(void *dev, size_t size, void **ptr);
> 
> static gpu_malloc_t *mlx5_gpu_malloc_fn;
> 
> static int
> mlx5_gpu_malloc(size_t size, void **ptr) {
> 	return 0;
> }
> 
> int main()
> {
> 	mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
> 
> 	mlx5_gpu_malloc_fn(NULL, 0, NULL);
> 
> 	return 0;
> }
> 
> 
> gcc -Wall fun.c
> fun.c: In function 'main':
> fun.c:15:21: warning: assignment to 'int (*)(void *, size_t,  void **)' {aka 'int (*)(void *, long unsigned int,  void **)'} from incompatible
> pointer type 'int (*)(size_t,  void **)' {aka 'int (*)(long unsigned int,  void **)'} [-Wincompatible-pointer-types]
>    15 |  mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
>       |                     ^
> 
> 
> 
> 2. w pointer
> 
> #include <stdio.h>
> 
> typedef int (*gpu_malloc_t)(void *dev, size_t size, void **ptr);
> 
> static gpu_malloc_t mlx5_gpu_malloc_fn;
> 
> static int
> mlx5_gpu_malloc(size_t size, void **ptr) {
> 	return 0;
> }
> 
> int main()
> {
> 	mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
> 
> 	mlx5_gpu_malloc_fn(NULL, 0, NULL);
> 
> 	return 0;
> }
> 
> gcc -Wall fun.c
> fun.c: In function 'main':
> fun.c:15:21: warning: assignment to 'gpu_malloc_t' {aka 'int (*)(void *, long unsigned int,  void **)'} from incompatible pointer type
> 'int (*)(size_t,  void **)' {aka 'int (*)(long unsigned int,  void **)'} [-Wincompatible-pointer-types]
>    15 |  mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
>       |                     ^
> 
> >

I don't mind as both suggestion works.
From usage perspective, it's straightforward for anybody to see an pointer field and know "that's a pointer to a something".
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, June 8, 2021 3:53 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Wang Haiyue <haiyue.wang@intel.com>;
> andrew.rybchenko@oktetlabs.ru
> Subject: Re: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus
> 
> 10/05/2021 15:47, Xueming Li:
> > Auxiliary [1] provides a way to split function into child-devices
> 
> Auxiliary -> Auxiliary bus
> 
> > representing sub-domains of functionality. Each auxiliary_device
> 
> auxiliary_device -> auxiliary device
> 
> > represents a part of its parent functionality.
> >
> > Auxiliary device is identified by unique device name, sysfs path:
> >   /sys/bus/auxiliary/devices/<name>
> >
> > [1] kernel auxiliary bus document:
> > https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html
> >
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> [...]
> > +++ b/drivers/bus/auxiliary/auxiliary_common.c
> [...]
> > +int auxiliary_bus_logtype;
> 
> You don't need to declare this variable, it is done in a macro.
> 
> > +
> > +static struct rte_devargs *
> > +auxiliary_devargs_lookup(const char *name) {
> > +	struct rte_devargs *devargs;
> > +
> > +	RTE_EAL_DEVARGS_FOREACH("auxiliary", devargs) {
> 
> "auxiliary" should be defined as a macro.
> 
> [...]
> > +/*
> > + * Test whether the auxiliary device exist  */ __rte_weak bool
> 
> The comment should explain it is a stub for OS not supporting auxiliary bus.
> 
> > +auxiliary_dev_exists(const char *name) {
> > +	RTE_SET_USED(name);
> > +	return false;
> > +}
> > +
> > +/*
> > + * Scan the content of the auxiliary bus, and the devices in the
> > +devices
> > + * list
> > + */
> 
> Same here about the stub comment.
> 
> > +__rte_weak int
> > +auxiliary_scan(void)
> > +{
> > +	return 0;
> > +}
> > +
> 
> Please insert a comment here.
> 
> > +void
> > +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev) {
> > +	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> > +}
> > +
> > +/*
> > + * Match the auxiliary Driver and Device using driver function.
> 
> driver and device do not need capital letters
> 
> > + */
> > +bool
> > +auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
> > +		const struct rte_auxiliary_device *aux_dev) {
> > +	if (aux_drv->match == NULL)
> > +		return false;
> > +	return aux_drv->match(aux_dev->name); }
> > +
> > +/*
> > + * Call the probe() function of the driver.
> > + */
> > +static int
> > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > +			       struct rte_auxiliary_device *dev)
> > +{
> > +	enum rte_iova_mode iova_mode;
> > +	int ret;
> > +
> > +	if ((dr == NULL) || (dev == NULL))
> > +		return -EINVAL;
> > +
> > +	/* The device is not blocked; Check if driver supports it */
> 
> Please keep all the comments more uniform by starting with a capital
> and ends with a point.
> 
> [...]
> > +RTE_REGISTER_BUS(auxiliary, auxiliary_bus.bus);
> > +RTE_LOG_REGISTER(auxiliary_bus_logtype, bus.auxiliary, NOTICE);
> 
> Please replace with RTE_LOG_REGISTER_DEFAULT.
> 
> [...]
> > +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> [...]
> > +/**
> > + * @file
> > + * Linux auxiliary probing.
> > + */
> 
> No need of doxygen comment in a .c file.
> 
> [...]
> > +++ b/drivers/bus/auxiliary/meson.build
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright 2021 Mellanox Technologies, Ltd
> > +
> > +headers = files('rte_bus_auxiliary.h')
> > +sources = files('auxiliary_common.c',
> > +	'auxiliary_params.c')
> > +if is_linux
> > +	sources += files('linux/auxiliary.c')
> > +endif
> > +deps += ['kvargs']
> 
> Please change the indent with spaces
> and check with devtools/check-meson.py
> 
> [...]
> > +++ b/drivers/bus/auxiliary/private.h
> [...]
> > +/* Auxiliary bus iterators */
> > +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p)	\
> 
> Please avoid tabs at the end of the line.
> A space is enough before the backslash.
> 
> > +		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> > +
> > +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p)	\
> > +		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
> > +
> > +/**
> > + * Test whether the auxiliary device exist
> > + *
> > + * @param name
> > + *  Auxiliary device name
> > + * @return
> > + *  true on exists, false otherwise
> > + */
> > +bool auxiliary_dev_exists(const char *name);
> 
> Given it is not an API, no need of doxygen.
> And the function is so simple that no need of comment at all.
> 
> [...]
> > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> > +#ifndef _RTE_BUS_AUXILIARY_H_
> > +#define _RTE_BUS_AUXILIARY_H_
> 
> No need of underscores before and after.
> (yes I know a lot of DPDK files use such scheme)
> 
> > +
> > +/**
> > + * @file
> > + *
> > + * RTE Auxiliary Bus Interface.
> 
> No need of RTE, it has no meaning here.
> 
> > + */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <limits.h>
> > +#include <errno.h>
> > +#include <sys/queue.h>
> > +#include <stdint.h>
> > +#include <inttypes.h>
> > +
> > +#include <rte_debug.h>
> > +#include <rte_interrupts.h>
> > +#include <rte_dev.h>
> > +#include <rte_bus.h>
> > +#include <rte_kvargs.h>
> > +
> > +/* Forward declarations */
> > +struct rte_auxiliary_driver;
> > +struct rte_auxiliary_bus;
> > +struct rte_auxiliary_device;
> > +
> > +/**
> > + * Match function for the driver to decide if device can be handled.
> > + *
> > + * @param name
> > + *   Pointer to the auxiliary device name.
> > + * @return
> > + *   Whether the driver can handle the auxiliary device.
> > + */
> > +typedef bool(*rte_auxiliary_match_t) (const char *name);
> 
> I disagree with the earlier comment asking for typedef pointer
> (based on one of my patches).
> Actually Andrew's suggestion makes sense:
> 	http://mails.dpdk.org/archives/dev/2021-June/210665.html
> 
> [...]
> > +++ b/drivers/bus/auxiliary/version.map
> > +DPDK_21 {
> > +	local: *;
> > +};
> 
> We don't need this empty section.
> 
> > +
> > +EXPERIMENTAL {
> > +	global:
> > +
> 
> As asked by Ray, we should add this line:
> 	# added in 21.08
> 
> > +	rte_auxiliary_register;
> > +	rte_auxiliary_unregister;
> > +};
> 
> Release notes are missing.
> 

Thanks for all the good catches, will update and reflect in next version.
  

> -----Original Message-----
> From: Xueming(Steven) Li <xuemingl@nvidia.com>
> Sent: Thursday, June 10, 2021 14:30
> To: Wang, Haiyue <haiyue.wang@intel.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>;
> andrew.rybchenko@oktetlabs.ru
> Subject: RE: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Tuesday, June 8, 2021 4:41 PM
> > To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Xueming(Steven) Li <xuemingl@nvidia.com>
> > Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>;
> andrew.rybchenko@oktetlabs.ru
> > Subject: RE: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus
> >
> > Hi Andrew,
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Tuesday, June 8, 2021 15:53
> > > To: Xueming Li <xuemingl@nvidia.com>
> > > Cc: dev@dpdk.org; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> > > <mdr@ashroe.eu>; Wang, Haiyue <haiyue.wang@intel.com>;
> > > andrew.rybchenko@oktetlabs.ru
> > > Subject: Re: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary
> > > bus
> > >
> >
> >
> > > > +
> > > > +/**
> > > > + * Match function for the driver to decide if device can be handled.
> > > > + *
> > > > + * @param name
> > > > + *   Pointer to the auxiliary device name.
> > > > + * @return
> > > > + *   Whether the driver can handle the auxiliary device.
> > > > + */
> > > > +typedef bool(*rte_auxiliary_match_t) (const char *name);
> > >
> > > I disagree with the earlier comment asking for typedef pointer (based
> > > on one of my patches).
> > > Actually Andrew's suggestion makes sense:
> > > 	http://mails.dpdk.org/archives/dev/2021-June/210665.html
> > >
> >
> > I didn't get the error, but the same warning, I missed something ? ;-)
> >
> > 1. w/o pointer
> >
> > #include <stdio.h>
> >
> > typedef int (gpu_malloc_t)(void *dev, size_t size, void **ptr);
> >
> > static gpu_malloc_t *mlx5_gpu_malloc_fn;
> >
> > static int
> > mlx5_gpu_malloc(size_t size, void **ptr) {
> > 	return 0;
> > }
> >
> > int main()
> > {
> > 	mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
> >
> > 	mlx5_gpu_malloc_fn(NULL, 0, NULL);
> >
> > 	return 0;
> > }
> >
> >
> > gcc -Wall fun.c
> > fun.c: In function 'main':
> > fun.c:15:21: warning: assignment to 'int (*)(void *, size_t,  void **)' {aka 'int (*)(void *, long
> unsigned int,  void **)'} from incompatible
> > pointer type 'int (*)(size_t,  void **)' {aka 'int (*)(long unsigned int,  void **)'} [-
> Wincompatible-pointer-types]
> >    15 |  mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
> >       |                     ^
> >
> >
> >
> > 2. w pointer
> >
> > #include <stdio.h>
> >
> > typedef int (*gpu_malloc_t)(void *dev, size_t size, void **ptr);
> >
> > static gpu_malloc_t mlx5_gpu_malloc_fn;
> >
> > static int
> > mlx5_gpu_malloc(size_t size, void **ptr) {
> > 	return 0;
> > }
> >
> > int main()
> > {
> > 	mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
> >
> > 	mlx5_gpu_malloc_fn(NULL, 0, NULL);
> >
> > 	return 0;
> > }
> >
> > gcc -Wall fun.c
> > fun.c: In function 'main':
> > fun.c:15:21: warning: assignment to 'gpu_malloc_t' {aka 'int (*)(void *, long unsigned int,  void
> **)'} from incompatible pointer type
> > 'int (*)(size_t,  void **)' {aka 'int (*)(long unsigned int,  void **)'} [-Wincompatible-pointer-
> types]
> >    15 |  mlx5_gpu_malloc_fn = mlx5_gpu_malloc;
> >       |                     ^
> >
> > >
> 
> I don't mind as both suggestion works.

I just want to understand what kind of error the compiler will complain about. ;-)

> From usage perspective, it's straightforward for anybody to see an pointer field and know "that's a
> pointer to a something".

>