[v1] 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>
---
 MAINTAINERS                               |   5 +
 drivers/bus/auxiliary/auxiliary_common.c  | 391 ++++++++++++++++++++++
 drivers/bus/auxiliary/auxiliary_params.c  |  58 ++++
 drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
 drivers/bus/auxiliary/meson.build         |  17 +
 drivers/bus/auxiliary/private.h           | 118 +++++++
 drivers/bus/auxiliary/rte_bus_auxiliary.h | 180 ++++++++++
 drivers/bus/auxiliary/version.map         |  10 +
 drivers/bus/meson.build                   |   2 +-
 9 files changed, 927 insertions(+), 1 deletion(-)
 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
  

I would like to Cc those who are interested in the auxiliary bus,
but it's difficult to know who will implement it.
Can we expect some reviews?


13/04/2021 05:23, Xueming Li:
> 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>
  

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> Sent: Tuesday, April 13, 2021 11:23
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; xuemingl@nvidia.com; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>;
> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> Subject: [dpdk-dev] [PATCH v1] 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>
> ---
>  MAINTAINERS                               |   5 +
>  drivers/bus/auxiliary/auxiliary_common.c  | 391 ++++++++++++++++++++++
>  drivers/bus/auxiliary/auxiliary_params.c  |  58 ++++
>  drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
>  drivers/bus/auxiliary/meson.build         |  17 +
>  drivers/bus/auxiliary/private.h           | 118 +++++++
>  drivers/bus/auxiliary/rte_bus_auxiliary.h | 180 ++++++++++
>  drivers/bus/auxiliary/version.map         |  10 +
>  drivers/bus/meson.build                   |   2 +-
>  9 files changed, 927 insertions(+), 1 deletion(-)
>  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
> 


> --- /dev/null
> +++ b/drivers/bus/auxiliary/auxiliary_common.c
> @@ -0,0 +1,391 @@
> +/* 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;
> +}
> +
> +void
> +auxiliary_on_scan(struct rte_auxiliary_device *dev)
> +{
> +	struct rte_devargs *devargs;
> +
> +	devargs = auxiliary_devargs_lookup(dev->name);
> +	dev->device.devargs = devargs;

Can be simple as:

dev->device.devargs = auxiliary_devargs_lookup(dev->name);

> +}
> +
> +/*
> + * Match the auxiliary Driver and Device using driver function.
> + */
> +bool
> +auxiliary_match(const struct rte_auxiliary_driver *auxiliary_drv,
> +		    const struct rte_auxiliary_device *auxiliary_dev)

How about these auxiliary variable name style ?

const struct rte_auxiliary_driver *aux_drv,
const struct rte_auxiliary_device *aux_dev

> +{
> +	if (auxiliary_drv->match == NULL)
> +		return false;
> +	return auxiliary_drv->match(auxiliary_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)
> +{
> +	int ret;
> +	enum rte_iova_mode iova_mode;
> +

RCT style ?
	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) > 0 &&

'(dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)' should work, no need '> 0'

> +	    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 = NULL;
> +	int rc = 0;

These two variables need no initialization.

> +
> +	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;
> +}
> +


> +static int
> +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
> +{
> +	struct rte_auxiliary_device *adev = RTE_DEV_TO_AUXILIARY(dev);

How about to use 'aux_dev', instead of 'adev' ?

> +
> +	if (!adev || !adev->driver) {

' RTE_DEV_TO_AUXILIARY' is container of 'dev', so it should check 'dev != NULL',
not '!adev'. ; -)


> +		rte_errno = EINVAL;
> +		return -1;
> +	}
> +	if (adev->driver->dma_map)
> +		return adev->driver->dma_map(adev, 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 *adev = RTE_DEV_TO_AUXILIARY(dev);
> +
> +	if (!adev || !adev->driver) {

The same comment as 'auxiliary_dma_map'

> +		rte_errno = EINVAL;
> +		return -1;
> +	}
> +	if (adev->driver->dma_unmap)
> +		return adev->driver->dma_unmap(adev, addr, iova, len);
> +	rte_errno = ENOTSUP;
> +	return -1;
> +}
> +


> --- /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)
> +{
> +	int ret;
> +	const struct rte_kvargs *kvlist = _kvlist;
> +

RCT.

> +	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..7888b6c5da
> --- /dev/null
> +++ b/drivers/bus/auxiliary/linux/auxiliary.c
                              ^
                              |
Seems no need to add one more directory 'linux' layer, as the meson said "linux only".


> @@ -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_exists(const char *name)

is_auxiliary_support() ?

> +{
> +	DIR *dir;
> +	char dirname[PATH_MAX];
> +
> +	snprintf(dirname, sizeof(dirname), "%s/%s",
> +		 AUXILIARY_SYSFS_PATH, name);
> +	dir = opendir(AUXILIARY_SYSFS_PATH);
> +	if (dir == NULL)
> +		return true;

false

> +	closedir(dir);
> +	return false;

true



> --
> 2.25.1
  

14/04/2021 04:59, Wang, Haiyue:
> From: Xueming Li
[...]
> > +void
> > +auxiliary_on_scan(struct rte_auxiliary_device *dev)
> > +{
> > +	struct rte_devargs *devargs;
> > +
> > +	devargs = auxiliary_devargs_lookup(dev->name);
> > +	dev->device.devargs = devargs;
> 
> Can be simple as:
> 
> dev->device.devargs = auxiliary_devargs_lookup(dev->name);
> 
> > +}
> > +
> > +/*
> > + * Match the auxiliary Driver and Device using driver function.
> > + */
> > +bool
> > +auxiliary_match(const struct rte_auxiliary_driver *auxiliary_drv,
> > +		    const struct rte_auxiliary_device *auxiliary_dev)
> 
> How about these auxiliary variable name style ?
> 
> const struct rte_auxiliary_driver *aux_drv,
> const struct rte_auxiliary_device *aux_dev

+1

[...]
> > +static int
> > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > +			       struct rte_auxiliary_device *dev)
> > +{
> > +	int ret;
> > +	enum rte_iova_mode iova_mode;
> > +
> 
> RCT style ?
> 	enum rte_iova_mode iova_mode;
> 	int ret;

I don't see the benefit of reverse christmas tree.

> > +	if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 &&
> 
> '(dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)' should work, no need '> 0'

Yes it's acceptable to consider bit testing as a boolean.

[...]
> > +static int
> > +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
> > +{
> > +	struct rte_auxiliary_device *adev = RTE_DEV_TO_AUXILIARY(dev);
> 
> How about to use 'aux_dev', instead of 'adev' ?
> 
> > +
> > +	if (!adev || !adev->driver) {
> 
> ' RTE_DEV_TO_AUXILIARY' is container of 'dev', so it should check 'dev != NULL',
> not '!adev'. ; -)

Yes and should be explicit NULL comparison.

[...]
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/linux/auxiliary.c
>                               ^
>                               |
> Seems no need to add one more directory 'linux' layer, as the meson said "linux only".

I disagree.
Linux sub-directory is more explicit.
And who knows? There could be an implementation on other OSes in future.
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, April 14, 2021 16:18
> To: Xueming Li <xuemingl@nvidia.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>
> Subject: Re: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> 14/04/2021 04:59, Wang, Haiyue:
> > From: Xueming Li


> 
> [...]
> > > +static int
> > > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > > +			       struct rte_auxiliary_device *dev)
> > > +{
> > > +	int ret;
> > > +	enum rte_iova_mode iova_mode;
> > > +
> >
> > RCT style ?
> > 	enum rte_iova_mode iova_mode;
> > 	int ret;
> 
> I don't see the benefit of reverse christmas tree.
> 

I'm impacted by kernel code style ;-)


> > > +	if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 &&
> [...]
> > > --- /dev/null
> > > +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> >                               ^
> >                               |
> > Seems no need to add one more directory 'linux' layer, as the meson said "linux only".
> 
> I disagree.
> Linux sub-directory is more explicit.
> And who knows? There could be an implementation on other OSes in future.

Make sense.

> 
>
  

Hi Haiyue,

> > +
> > +/*
> > + * Test whether the auxiliary device exist  */ bool
> > +auxiliary_exists(const char *name)
> 
> is_auxiliary_support() ?

This function test whether an auxiliary device exists, name parameter is device name.
I think auxiliary_dev_exists(name) make more sense.

Agree with other comments, thanks!
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, April 14, 2021 4:18 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>
> Subject: Re: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> 14/04/2021 04:59, Wang, Haiyue:
> > From: Xueming Li
> [...]
> > > +void
> > > +auxiliary_on_scan(struct rte_auxiliary_device *dev) {
> > > +	struct rte_devargs *devargs;
> > > +
> > > +	devargs = auxiliary_devargs_lookup(dev->name);
> > > +	dev->device.devargs = devargs;
> >
> > Can be simple as:
> >
> > dev->device.devargs = auxiliary_devargs_lookup(dev->name);
> >
> > > +}
> > > +
> > > +/*
> > > + * Match the auxiliary Driver and Device using driver function.
> > > + */
> > > +bool
> > > +auxiliary_match(const struct rte_auxiliary_driver *auxiliary_drv,
> > > +		    const struct rte_auxiliary_device *auxiliary_dev)
> >
> > How about these auxiliary variable name style ?
> >
> > const struct rte_auxiliary_driver *aux_drv, const struct
> > rte_auxiliary_device *aux_dev
> 
> +1
> 
> [...]
> > > +static int
> > > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > > +			       struct rte_auxiliary_device *dev) {
> > > +	int ret;
> > > +	enum rte_iova_mode iova_mode;
> > > +
> >
> > RCT style ?
> > 	enum rte_iova_mode iova_mode;
> > 	int ret;
> 
> I don't see the benefit of reverse christmas tree.
> 
> > > +	if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 &&
> >
> > '(dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)' should work, no need '> 0'
> 
> Yes it's acceptable to consider bit testing as a boolean.
> 
> [...]
> > > +static int
> > > +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t
> > > +iova, size_t len) {
> > > +	struct rte_auxiliary_device *adev = RTE_DEV_TO_AUXILIARY(dev);
> >
> > How about to use 'aux_dev', instead of 'adev' ?
> >
> > > +
> > > +	if (!adev || !adev->driver) {
> >
> > ' RTE_DEV_TO_AUXILIARY' is container of 'dev', so it should check 'dev
> > != NULL', not '!adev'. ; -)
> 
> Yes and should be explicit NULL comparison.
> 
> [...]
> > > --- /dev/null
> > > +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> >                               ^
> >                               |
> > Seems no need to add one more directory 'linux' layer, as the meson said "linux only".
> 
> I disagree.
> Linux sub-directory is more explicit.
> And who knows? There could be an implementation on other OSes in future.

This reminds me to change meson, allows bus always available to avoid compilation error,
also need to add stubs with __rte_weak for all functions in common file.

> 
>
  

> -----Original Message-----
> From: Xueming(Steven) Li <xuemingl@nvidia.com>
> Sent: Wednesday, April 14, 2021 23:39
> To: Wang, Haiyue <haiyue.wang@intel.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> Hi Haiyue,
> 
> > > +
> > > +/*
> > > + * Test whether the auxiliary device exist  */ bool
> > > +auxiliary_exists(const char *name)
> >
> > is_auxiliary_support() ?
> 
> This function test whether an auxiliary device exists, name parameter is device name.
> I think auxiliary_dev_exists(name) make more sense.

More clear now. ;-)

> 
> Agree with other comments, thanks!
  

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> Sent: Tuesday, April 13, 2021 11:23
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; xuemingl@nvidia.com; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>;
> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> Subject: [dpdk-dev] [PATCH v1] 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>
> ---
>  MAINTAINERS                               |   5 +
>  drivers/bus/auxiliary/auxiliary_common.c  | 391 ++++++++++++++++++++++
>  drivers/bus/auxiliary/auxiliary_params.c  |  58 ++++
>  drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
>  drivers/bus/auxiliary/meson.build         |  17 +
>  drivers/bus/auxiliary/private.h           | 118 +++++++
>  drivers/bus/auxiliary/rte_bus_auxiliary.h | 180 ++++++++++
>  drivers/bus/auxiliary/version.map         |  10 +
>  drivers/bus/meson.build                   |   2 +-
>  9 files changed, 927 insertions(+), 1 deletion(-)
>  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
> 


> --- /dev/null
> +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> @@ -0,0 +1,180 @@
> +/* 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.
> + */
> +typedef bool(auxiliary_match_t) (const char *);
> +
> +/**
> + * Initialization function for the driver called during auxiliary probing.
> + */
> +typedef int(auxiliary_probe_t) (struct rte_auxiliary_driver*,
> +				struct rte_auxiliary_device*);
> +
> +/**
> + * Uninitialization function for the driver called during hotplugging.
> + */
> +typedef int (auxiliary_remove_t)(struct rte_auxiliary_device *);
> +
> +/**
> + * 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 (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 (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. */
> +	auxiliary_match_t *match;            /**< Device match function. */
> +	auxiliary_probe_t *probe;            /**< Device Probe function. */
> +	auxiliary_remove_t *remove;          /**< Device Remove function. */
> +	auxiliary_dma_map_t *dma_map;        /**< Device dma map function. */
> +	auxiliary_dma_unmap_t *dma_unmap;    /**< Device dma unmap function. */

These API type can be pointer type defined, then no need "*":

typedef int (*auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
				    void *addr, uint64_t iova, size_t len);

auxiliary_dma_unmap_t dma_unmap;

Like:
https://patchwork.dpdk.org/project/dpdk/patch/20210331224547.2217759-1-thomas@monjalon.net/

typedef int (*rte_dev_dma_map_t)(struct rte_device *dev,
				 void *addr, uint64_t iova, size_t len);

> --
> 2.25.1
  

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Thursday, April 15, 2021 3:36 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>
> Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> > Sent: Tuesday, April 13, 2021 11:23
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; xuemingl@nvidia.com; Asaf Penso <asafp@nvidia.com>;
> > Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Neil
> > Horman <nhorman@tuxdriver.com>
> > Subject: [dpdk-dev] [PATCH v1] 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>
> > ---
> >  MAINTAINERS                               |   5 +
> >  drivers/bus/auxiliary/auxiliary_common.c  | 391
> > ++++++++++++++++++++++  drivers/bus/auxiliary/auxiliary_params.c  |  58 ++++
> >  drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
> >  drivers/bus/auxiliary/meson.build         |  17 +
> >  drivers/bus/auxiliary/private.h           | 118 +++++++
> >  drivers/bus/auxiliary/rte_bus_auxiliary.h | 180 ++++++++++
> >  drivers/bus/auxiliary/version.map         |  10 +
> >  drivers/bus/meson.build                   |   2 +-
> >  9 files changed, 927 insertions(+), 1 deletion(-)  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
> >
> 
> 
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> > @@ -0,0 +1,180 @@
> > +/* 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.
> > + */
> > +typedef bool(auxiliary_match_t) (const char *);
> > +
> > +/**
> > + * Initialization function for the driver called during auxiliary probing.
> > + */
> > +typedef int(auxiliary_probe_t) (struct rte_auxiliary_driver*,
> > +				struct rte_auxiliary_device*);
> > +
> > +/**
> > + * Uninitialization function for the driver called during hotplugging.
> > + */
> > +typedef int (auxiliary_remove_t)(struct rte_auxiliary_device *);
> > +
> > +/**
> > + * 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 (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 (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. */
> > +	auxiliary_match_t *match;            /**< Device match function. */
> > +	auxiliary_probe_t *probe;            /**< Device Probe function. */
> > +	auxiliary_remove_t *remove;          /**< Device Remove function. */
> > +	auxiliary_dma_map_t *dma_map;        /**< Device dma map function. */
> > +	auxiliary_dma_unmap_t *dma_unmap;    /**< Device dma unmap function. */
> 
> These API type can be pointer type defined, then no need "*":
> 
> typedef int (*auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
> 				    void *addr, uint64_t iova, size_t len);
> 
> auxiliary_dma_unmap_t dma_unmap;
> 
> Like:
> https://patchwork.dpdk.org/project/dpdk/patch/20210331224547.2217759-1-thomas@monjalon.net/
> 
> typedef int (*rte_dev_dma_map_t)(struct rte_device *dev,
> 				 void *addr, uint64_t iova, size_t len);

Thanks, is there a reason to prefer pointer type? 

Thoma's patch looks good, will rebase on it once accepted.

> 
> > --
> > 2.25.1
  

> -----Original Message-----
> From: Xueming(Steven) Li <xuemingl@nvidia.com>
> Sent: Thursday, April 15, 2021 15:46
> To: Wang, Haiyue <haiyue.wang@intel.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Thursday, April 15, 2021 3:36 PM
> > To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>; Neil Horman
> > <nhorman@tuxdriver.com>
> > Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> > > Sent: Tuesday, April 13, 2021 11:23
> > > To: Thomas Monjalon <thomas@monjalon.net>
> > > Cc: dev@dpdk.org; xuemingl@nvidia.com; Asaf Penso <asafp@nvidia.com>;
> > > Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Neil
> > > Horman <nhorman@tuxdriver.com>
> > > Subject: [dpdk-dev] [PATCH v1] 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>
> > > ---
> > >  MAINTAINERS                               |   5 +
> > >  drivers/bus/auxiliary/auxiliary_common.c  | 391
> > > ++++++++++++++++++++++  drivers/bus/auxiliary/auxiliary_params.c  |  58 ++++
> > >  drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
> > >  drivers/bus/auxiliary/meson.build         |  17 +
> > >  drivers/bus/auxiliary/private.h           | 118 +++++++
> > >  drivers/bus/auxiliary/rte_bus_auxiliary.h | 180 ++++++++++
> > >  drivers/bus/auxiliary/version.map         |  10 +
> > >  drivers/bus/meson.build                   |   2 +-
> > >  9 files changed, 927 insertions(+), 1 deletion(-)  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
> > >
> >
> >
> > > --- /dev/null
> > > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> > > @@ -0,0 +1,180 @@
> > > +/* 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.
> > > + */
> > > +typedef bool(auxiliary_match_t) (const char *);
> > > +
> > > +/**
> > > + * Initialization function for the driver called during auxiliary probing.
> > > + */
> > > +typedef int(auxiliary_probe_t) (struct rte_auxiliary_driver*,
> > > +				struct rte_auxiliary_device*);
> > > +
> > > +/**
> > > + * Uninitialization function for the driver called during hotplugging.
> > > + */
> > > +typedef int (auxiliary_remove_t)(struct rte_auxiliary_device *);
> > > +
> > > +/**
> > > + * 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 (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 (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. */
> > > +	auxiliary_match_t *match;            /**< Device match function. */
> > > +	auxiliary_probe_t *probe;            /**< Device Probe function. */
> > > +	auxiliary_remove_t *remove;          /**< Device Remove function. */
> > > +	auxiliary_dma_map_t *dma_map;        /**< Device dma map function. */
> > > +	auxiliary_dma_unmap_t *dma_unmap;    /**< Device dma unmap function. */
> >
> > These API type can be pointer type defined, then no need "*":
> >
> > typedef int (*auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
> > 				    void *addr, uint64_t iova, size_t len);
> >
> > auxiliary_dma_unmap_t dma_unmap;
> >
> > Like:
> > https://patchwork.dpdk.org/project/dpdk/patch/20210331224547.2217759-1-thomas@monjalon.net/
> >
> > typedef int (*rte_dev_dma_map_t)(struct rte_device *dev,
> > 				 void *addr, uint64_t iova, size_t len);
> 
> Thanks, is there a reason to prefer pointer type?

Good practice to make code beautiful ? ;-)

> 
> Thoma's patch looks good, will rebase on it once accepted.

I mean the function type is defined as pointer type.

> 
> >
> > > --
> > > 2.25.1
  

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Thursday, April 15, 2021 3:52 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>
> Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> > -----Original Message-----
> > From: Xueming(Steven) Li <xuemingl@nvidia.com>
> > Sent: Thursday, April 15, 2021 15:46
> > To: Wang, Haiyue <haiyue.wang@intel.com>; NBU-Contact-Thomas Monjalon
> > <thomas@monjalon.net>
> > Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit
> > <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> > <nhorman@tuxdriver.com>
> > Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary
> > bus
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > Sent: Thursday, April 15, 2021 3:36 PM
> > > To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
> > > Monjalon <thomas@monjalon.net>
> > > Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit
> > > <parav@nvidia.com>; Ray Kinsella
> > <mdr@ashroe.eu>; Neil Horman
> > > <nhorman@tuxdriver.com>
> > > Subject: RE: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce
> > > auxiliary bus
> > >
> > > > -----Original Message-----
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> > > > Sent: Tuesday, April 13, 2021 11:23
> > > > To: Thomas Monjalon <thomas@monjalon.net>
> > > > Cc: dev@dpdk.org; xuemingl@nvidia.com; Asaf Penso
> > > > <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> > > > <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> > > > Subject: [dpdk-dev] [PATCH v1] 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.ht
> > > > ml
> > > >
> > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > ---
> > > >  MAINTAINERS                               |   5 +
> > > >  drivers/bus/auxiliary/auxiliary_common.c  | 391
> > > > ++++++++++++++++++++++  drivers/bus/auxiliary/auxiliary_params.c
> > > > ++++++++++++++++++++++ |  58 ++++
> > > >  drivers/bus/auxiliary/linux/auxiliary.c   | 147 ++++++++
> > > >  drivers/bus/auxiliary/meson.build         |  17 +
> > > >  drivers/bus/auxiliary/private.h           | 118 +++++++
> > > >  drivers/bus/auxiliary/rte_bus_auxiliary.h | 180 ++++++++++
> > > >  drivers/bus/auxiliary/version.map         |  10 +
> > > >  drivers/bus/meson.build                   |   2 +-
> > > >  9 files changed, 927 insertions(+), 1 deletion(-)  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
> > > >
> > >
> > >
> > > > --- /dev/null
> > > > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> > > > @@ -0,0 +1,180 @@
> > > > +/* 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.
> > > > + */
> > > > +typedef bool(auxiliary_match_t) (const char *);
> > > > +
> > > > +/**
> > > > + * Initialization function for the driver called during auxiliary probing.
> > > > + */
> > > > +typedef int(auxiliary_probe_t) (struct rte_auxiliary_driver*,
> > > > +				struct rte_auxiliary_device*);
> > > > +
> > > > +/**
> > > > + * Uninitialization function for the driver called during hotplugging.
> > > > + */
> > > > +typedef int (auxiliary_remove_t)(struct rte_auxiliary_device *);
> > > > +
> > > > +/**
> > > > + * 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 (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 (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. */
> > > > +	auxiliary_match_t *match;            /**< Device match function. */
> > > > +	auxiliary_probe_t *probe;            /**< Device Probe function. */
> > > > +	auxiliary_remove_t *remove;          /**< Device Remove function. */
> > > > +	auxiliary_dma_map_t *dma_map;        /**< Device dma map function. */
> > > > +	auxiliary_dma_unmap_t *dma_unmap;    /**< Device dma unmap function. */
> > >
> > > These API type can be pointer type defined, then no need "*":
> > >
> > > typedef int (*auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
> > > 				    void *addr, uint64_t iova, size_t len);
> > >
> > > auxiliary_dma_unmap_t dma_unmap;
> > >
> > > Like:
> > > https://patchwork.dpdk.org/project/dpdk/patch/20210331224547.2217759
> > > -1-thomas@monjalon.net/
> > >
> > > typedef int (*rte_dev_dma_map_t)(struct rte_device *dev,
> > > 				 void *addr, uint64_t iova, size_t len);
> >
> > Thanks, is there a reason to prefer pointer type?
> 
> Good practice to make code beautiful ? ;-)
> 
> >
> > Thoma's patch looks good, will rebase on it once accepted.
> 
> I mean the function type is defined as pointer type.
Yes, I'm talking another topic :)
His patch makes dma map/unmap higher level to rte_bus, so that no need to define the api again here.

> 
> >
> > >
> > > > --
> > > > 2.25.1
  

15/04/2021 09:55, Xueming(Steven) Li:
> From: Wang, Haiyue <haiyue.wang@intel.com>
> > From: Xueming(Steven) Li <xuemingl@nvidia.com>
> > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> > > > > +/**
> > > > > + * 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. */
> > > > > +	auxiliary_match_t *match;            /**< Device match function. */
> > > > > +	auxiliary_probe_t *probe;            /**< Device Probe function. */
> > > > > +	auxiliary_remove_t *remove;          /**< Device Remove function. */
> > > > > +	auxiliary_dma_map_t *dma_map;        /**< Device dma map function. */
> > > > > +	auxiliary_dma_unmap_t *dma_unmap;    /**< Device dma unmap function. */
> > > >
> > > > These API type can be pointer type defined, then no need "*":
> > > >
> > > > typedef int (*auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
> > > > 				    void *addr, uint64_t iova, size_t len);
> > > >
> > > > auxiliary_dma_unmap_t dma_unmap;
> > > >
> > > > Like:
> > > > https://patchwork.dpdk.org/project/dpdk/patch/20210331224547.2217759
> > > > -1-thomas@monjalon.net/
> > > >
> > > > typedef int (*rte_dev_dma_map_t)(struct rte_device *dev,
> > > > 				 void *addr, uint64_t iova, size_t len);
> > >
> > > Thanks, is there a reason to prefer pointer type?
> > 
> > Good practice to make code beautiful ? ;-)

Honestly, I am not sure which one is better,
having the pointer type hidden in the typedef or explicit?

> > > Thoma's patch looks good, will rebase on it once accepted.
> > 
> > I mean the function type is defined as pointer type.
> Yes, I'm talking another topic :)
> His patch makes dma map/unmap higher level to rte_bus, so that no need to define the api again here.

I think my patch will be abandoned.
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, April 15, 2021 15:59
> To: Wang, Haiyue <haiyue.wang@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>; david.marchand@redhat.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> 15/04/2021 09:55, Xueming(Steven) Li:
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > From: Xueming(Steven) Li <xuemingl@nvidia.com>
> > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Xueming Li
> > > > > > +/**
> > > > > > + * 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. */
> > > > > > +	auxiliary_match_t *match;            /**< Device match function. */
> > > > > > +	auxiliary_probe_t *probe;            /**< Device Probe function. */
> > > > > > +	auxiliary_remove_t *remove;          /**< Device Remove function. */
> > > > > > +	auxiliary_dma_map_t *dma_map;        /**< Device dma map function. */
> > > > > > +	auxiliary_dma_unmap_t *dma_unmap;    /**< Device dma unmap function. */
> > > > >
> > > > > These API type can be pointer type defined, then no need "*":
> > > > >
> > > > > typedef int (*auxiliary_dma_unmap_t)(struct rte_auxiliary_device *dev,
> > > > > 				    void *addr, uint64_t iova, size_t len);
> > > > >
> > > > > auxiliary_dma_unmap_t dma_unmap;
> > > > >
> > > > > Like:
> > > > > https://patchwork.dpdk.org/project/dpdk/patch/20210331224547.2217759
> > > > > -1-thomas@monjalon.net/
> > > > >
> > > > > typedef int (*rte_dev_dma_map_t)(struct rte_device *dev,
> > > > > 				 void *addr, uint64_t iova, size_t len);
> > > >
> > > > Thanks, is there a reason to prefer pointer type?
> > >
> > > Good practice to make code beautiful ? ;-)
> 
> Honestly, I am not sure which one is better,
> having the pointer type hidden in the typedef or explicit?

I got this style from 'struct eth_dev_ops', Steven can make the final decision,
just a suggestion.

> 
> > > > Thoma's patch looks good, will rebase on it once accepted.
> > >
> > > I mean the function type is defined as pointer type.
> > Yes, I'm talking another topic :)
> > His patch makes dma map/unmap higher level to rte_bus, so that no need to define the api again here.
> 
> I think my patch will be abandoned.
>