> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Xueming Li
> Sent: Tuesday, April 13, 2021 11:23
> To: Thomas Monjalon <tho...@monjalon.net>
> Cc: dev@dpdk.org; xuemi...@nvidia.com; Asaf Penso <as...@nvidia.com>; Parav
> Pandit <pa...@nvidia.com>;
> Ray Kinsella <m...@ashroe.eu>; Neil Horman <nhor...@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 <xuemi...@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
