Re: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus

["Wang, Haiyue" <haiyue.wang@intel.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