Re: [PATCH] bus/uacce: introduce UACCE bus

[fengchengwen <fengchengwen@huawei.com>]

Hi Huisong,

On 2024/1/15 16:14, lihuisong (C) wrote:
> Hi chengwen,
> 
> lgtm,
> with below to changes,
> Acked-by: Huisong Li <lihuisong@huawei.com>
> 
> 
> 在 2023/12/8 14:18, Chengwen Feng 写道:
>> UACCE (Unified/User-space-access-intended Accelerator Framework) was
>> upstream to Linux kernel version 5.7, and it targets to provide Shared
>> Virtual Addressing (SVA) between accelerators and processes. So
>> accelerator can access any data structure of the main cpu. [1] for more
>> information.
>>
>> This commit introduces UACCE bus, so that the accelerator devices could
>> seen at DPDK and could be further registered as a compress, crypto, dma
>> and ethdev device.
>>
>> [1] https://docs.kernel.org/misc-devices/uacce.html
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>   MAINTAINERS                          |   4 +
>>   drivers/bus/meson.build              |   1 +
>>   drivers/bus/uacce/bus_uacce_driver.h | 254 ++++++++++
>>   drivers/bus/uacce/meson.build        |  12 +
>>   drivers/bus/uacce/uacce.c            | 702 +++++++++++++++++++++++++++
>>   drivers/bus/uacce/version.map        |  15 +
>>   6 files changed, 988 insertions(+)
>>   create mode 100644 drivers/bus/uacce/bus_uacce_driver.h
>>   create mode 100644 drivers/bus/uacce/meson.build
>>   create mode 100644 drivers/bus/uacce/uacce.c
>>   create mode 100644 drivers/bus/uacce/version.map
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0d1c8126e3..89711029d5 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -604,6 +604,10 @@ Platform bus driver
>>   M: Tomasz Duszynski <tduszynski@marvell.com>
>>   F: drivers/bus/platform/
>>   +UACCE bus driver
>> +M: Chengwen Feng <fengchengwen@huawei.com>
>> +F: drivers/bus/uacce/
>> +
>>   VDEV bus driver
>>   F: drivers/bus/vdev/
>>   F: app/test/test_vdev.c
>> diff --git a/drivers/bus/meson.build b/drivers/bus/meson.build
>> index a78b4283bf..d67db8576b 100644
>> --- a/drivers/bus/meson.build
>> +++ b/drivers/bus/meson.build
>> @@ -9,6 +9,7 @@ drivers = [
>>           'ifpga',
>>           'pci',
>>           'platform',
>> +        'uacce',
>>           'vdev',
>>           'vmbus',
>>   ]
>> diff --git a/drivers/bus/uacce/bus_uacce_driver.h b/drivers/bus/uacce/bus_uacce_driver.h
>> new file mode 100644
>> index 0000000000..0276154658
>> --- /dev/null
>> +++ b/drivers/bus/uacce/bus_uacce_driver.h
>> @@ -0,0 +1,254 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2023 HiSilicon Limited
>> + */
>> +
>> +#ifndef BUS_UACCE_DRIVER_H
>> +#define BUS_UACCE_DRIVER_H
>> +
>> +/**
>> + * @file
>> + *
>> + * HiSilicon UACCE bus interface.
>> + */
>> +
>> +#include <inttypes.h>
>> +#include <stdlib.h>
>> +#include <linux/types.h>
>> +
>> +#include <rte_compat.h>
>> +#include <rte_devargs.h>
>> +#include <dev_driver.h>
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif /* __cplusplus */
>> +
>> +#define RTE_UACCE_DEV_PATH_SIZE        256
>> +#define RTE_UACCE_API_NAME_SIZE        64
>> +#define RTE_UACCE_ALGS_NAME_SIZE    384
>> +#define RTE_UACCE_ATTR_MAX_SIZE        384
>> +
>> +/*
>> + * Definition for queue file region type.
>> + */
>> +enum rte_uacce_qfrt {
>> +    RTE_UACCE_QFRT_MMIO = 0, /**< Device mmio region. */
>> +    RTE_UACCE_QFRT_DUS,      /**< Device user share region. */
>> +    RTE_UACCE_QFRT_BUTT
>> +};
>> +
>> +struct rte_uacce_driver;
>> +
>> +/**
>> + * A structure describing a UACCE device.
>> + */
>> +struct rte_uacce_device {
>> +    RTE_TAILQ_ENTRY(rte_uacce_device) next;  /**< Next in device list. */
>> +    struct rte_device device;                /**< Inherit core device. */
>> +    struct rte_uacce_driver *driver;         /**< Driver used in probing. */
>> +    char name[RTE_DEV_NAME_MAX_LEN];         /**< Device name. */
>> +    char dev_root[RTE_UACCE_DEV_PATH_SIZE];  /**< Sysfs path with device name. */
>> +    char cdev_path[RTE_UACCE_DEV_PATH_SIZE]; /**< Device path in devfs. */
>> +    char api[RTE_UACCE_API_NAME_SIZE];       /**< Device context type. */
>> +    char algs[RTE_UACCE_ALGS_NAME_SIZE];     /**< Device supported algorithms. */
>> +    uint32_t flags;                          /**< Device flags. */
>> +    int numa_node;                           /**< NUMA node connection, -1 if unknown. */
>> +    uint32_t qfrt_sz[RTE_UACCE_QFRT_BUTT];   /**< Queue file region type's size. */
>> +};
>> +
>> +/**
>> + * @internal
>> + * Helper macro for drivers that need to convert to struct rte_uacce_device.
>> + */
>> +#define RTE_DEV_TO_UACCE_DEV(ptr) \
>> +    container_of(ptr, struct rte_uacce_device, device)
>> +
>> +#define RTE_DEV_TO_UACCE_DEV_CONST(ptr) \
>> +    container_of(ptr, const struct rte_uacce_device, device)
>> +
>> +/**
>> + * A structure describing an ID for a UACCE driver. Each driver provides a
>> + * table of these IDs for each device that it supports.
>> + */
>> +struct rte_uacce_id {
>> +    const char *dev_api;   /**< Device context type. */
>> +    /** Device algorithm.
>> +     * If this field is NULL, only dev_api is matched. Otherwise, in
>> +     * addition to match dev_api, dev_alg must be a subset of device's
>> +     * algs.
>> +     */
>> +    const char *dev_alg;
>> +};
>> +
>> +/**
>> + * Initialization function for the driver called during probing.
>> + */
>> +typedef int (rte_uacce_probe_t)(struct rte_uacce_driver *, struct rte_uacce_device *);
>> +
>> +/**
>> + * Uninitialization function for the driver called during hotplugging.
>> + */
>> +typedef int (rte_uacce_remove_t)(struct rte_uacce_device *);
>> +
>> +/**
>> + * A structure describing a UACCE driver.
>> + */
>> +struct rte_uacce_driver {
>> +    RTE_TAILQ_ENTRY(rte_uacce_driver) next;    /**< Next in list. */
>> +    struct rte_driver driver;               /**< Inherit core driver. */
>> +    struct rte_uacce_bus *bus;              /**< UACCE bus reference. */
>> +    rte_uacce_probe_t *probe;               /**< Device probe function. */
>> +    rte_uacce_remove_t *remove;             /**< Device remove function. */
>> +    const struct rte_uacce_id *id_table;    /**< ID table, NULL terminated. */
>> +};
>> +
>> +/**
>> + * Get available queue number.
>> + *
>> + * @param dev
>> + *   A pointer to a rte_uacce_device structure describing the device
>> + *   to use.
>> + *
>> + * @note The available queues on the device may changes dynamically,
>> + * for examples, other process may alloc or free queues.
>> + *
>> + * @return
>> + *   0 on success. Otherwise negative value is returned.
>> + */
>> +__rte_internal
>> +int rte_uacce_avail_queues(struct rte_uacce_device *dev);
>> +
>> +/*
>> + * The queue context for a UACCE queue.
>> + */
>> +struct rte_uacce_qcontex {
>> +    int fd;                       /**< The file descriptor associated to the queue. */
>> +    struct rte_uacce_device *dev; /**< The device associated to the queue. */
>> +    void *qfrt_base[RTE_UACCE_QFRT_BUTT]; /**< The qfrt mmap's memory base. */
>> +};
>> +
>> +/**
>> + * Alloc one queue.
>> + *
>> + * @param dev
>> + *   A pointer to a rte_uacce_device structure describing the device to use.
>> + * @param qctx
>> + *   Pointer to queue context, which is used to store the queue information
>> + *   that is successfully applied for.
>> + *
>> + * @return
>> + *   0 on success. Otherwise negative value is returned.
>> + */
>> +__rte_internal
>> +int rte_uacce_queue_alloc(struct rte_uacce_device *dev, struct rte_uacce_qcontex *qctx);
>> +
>> +/**
>> + * Free one queue.
>> + *
>> + * @param qctx
>> + *   Pointer to queue context, which allocated by @see rte_uacce_alloc_queue.
>> + *
>> + * @note Once the queue is freed, any operations on the queue (including
>> + * control-plane and data-plane, and also read & write mmap region) are not
>> + * allowed.
>> + */
>> +__rte_internal
>> +void rte_uacce_queue_free(struct rte_uacce_qcontex *qctx);
>> +
>> +/**
>> + * Start one queue.
>> + *
>> + * @param qctx
>> + *   Pointer to queue context, which allocated by @see rte_uacce_alloc_queue.
>> + *
>> + * @return
>> + *   0 on success. Otherwise negative value is returned.
>> + */
>> +__rte_internal
>> +int rte_uacce_queue_start(struct rte_uacce_qcontex *qctx);
>> +
>> +/**
>> + * Send ioctl command to one queue.
>> + *
>> + * @param qctx
>> + *   Pointer to queue context, which allocated by @see rte_uacce_alloc_queue.
>> + * @param cmd
>> + *   ioctl command.
>> + *   @note The nr must not conflict with the definition in Linux kerel:
>> + *   include/uapi/misc/uacce/uacce.h. It is recommended that the driver
>> + *   custom nr start from 64.
>> + * @param arg
>> + *   Command input & output buffer.
>> + *
>> + * @return
>> + *   0 on success. Otherwise negative value is returned.
>> + */
>> +__rte_internal
>> +int rte_uacce_queue_ioctl(struct rte_uacce_qcontex *qctx, unsigned long cmd, void *arg);
>> +
>> +/**
>> + * Mmap queue file region.
>> + *
>> + * @param qctx
>> + *   Pointer to queue context, which allocated by @see rte_uacce_alloc_queue.
>> + * @param qfrt
>> + *   The queue file region type. Must be RTE_UACCE_QFRT_MMIO or
>> + *   RTE_UACCE_QFRT_DUS.
>> + *
>> + * @return
>> + *   Non-NULL on success. Otherwise NULL is returned.
>> + */
>> +__rte_internal
>> +void *rte_uacce_queue_mmap(struct rte_uacce_qcontex *qctx, enum rte_uacce_qfrt qfrt);
>> +
>> +/**
>> + * Unmap queue file region.
>> + *
>> + * @param qctx
>> + *   Pointer to queue context, which allocated by @see rte_uacce_alloc_queue.
>> + * @param qfrt
>> + *   The queue file region type. Must be RTE_UACCE_QFRT_MMIO or
>> + *   RTE_UACCE_QFRT_DUS.
>> + *
>> + * @return
>> + *   Non-NULL on success. Otherwise NULL is returned.
>> + */
>> +__rte_internal
>> +void rte_uacce_queue_unmap(struct rte_uacce_qcontex *qctx, enum rte_uacce_qfrt qfrt);
>> +
>> +/**
>> + * Register a UACCE driver.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_uacce_driver structure describing the driver to be
>> + *   registered.
>> + */
>> +__rte_internal
>> +void rte_uacce_register(struct rte_uacce_driver *driver);
>> +
>> +/**
>> + * Unregister a UACCE driver.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_uacce_driver structure describing the driver to be
>> + *   unregistered.
>> + */
>> +__rte_internal
>> +void rte_uacce_unregister(struct rte_uacce_driver *driver);
>> +
>> +/**
>> + * Helper for UACCE device registration from driver instance.
>> + */
>> +#define RTE_PMD_REGISTER_UACCE(nm, uacce_drv) \
>> +        RTE_INIT(uacceinitfn_ ##nm) \
>> +        {\
>> +            (uacce_drv).driver.name = RTE_STR(nm);\
>> +            rte_uacce_register(&uacce_drv); \
>> +        } \
>> +        RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif /* __cplusplus */
>> +
>> +#endif /* BUS_UACCE_DRIVER_H */
>> diff --git a/drivers/bus/uacce/meson.build b/drivers/bus/uacce/meson.build
>> new file mode 100644
>> index 0000000000..b48d6db11a
>> --- /dev/null
>> +++ b/drivers/bus/uacce/meson.build
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2023 HiSilicon Limited.
>> +
>> +if not is_linux
>> +    build = false
>> +    reason = 'only supported on Linux'
>> +endif
>> +
>> +sources = files('uacce.c')
>> +driver_sdk_headers += files('bus_uacce_driver.h')
>> +
>> +deps += ['kvargs']
>> diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
>> new file mode 100644
>> index 0000000000..8e824c44df
>> --- /dev/null
>> +++ b/drivers/bus/uacce/uacce.c
>> @@ -0,0 +1,702 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2023 HiSilicon Limited
>> + */
>> +
>> +#include <dirent.h>
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +
>> +#include <rte_bitops.h>
>> +#include <rte_common.h>
>> +#include <rte_devargs.h>
>> +#include <rte_errno.h>
>> +#include <rte_log.h>
>> +#include <rte_kvargs.h>
>> +#include <bus_driver.h>
>> +
>> +#include "bus_uacce_driver.h"
>> +
>> +#define UACCE_BUS_CLASS_PATH    "/sys/class/uacce"
>> +
>> +/* UACCE device flag of SVA. */
>> +#define UACCE_DEV_FLGA_SVA    RTE_BIT32(0)
>> +
>> +/* Support -a uacce:device-name when start DPDK application. */
>> +#define UACCE_DEV_PREFIX    "uacce:"
>> +
>> +/*
>> + * Structure describing the UACCE bus.
>> + */
>> +struct rte_uacce_bus {
>> +    struct rte_bus bus;                    /* Inherit the generic class. */
>> +    TAILQ_HEAD(, rte_uacce_device) device_list; /* List of devices. */
>> +    TAILQ_HEAD(, rte_uacce_driver) driver_list; /* List of drivers. */
>> +};
>> +
>> +/* Forward declaration of UACCE bus. */
>> +static struct rte_uacce_bus uacce_bus;
>> +
>> +enum uacce_params {
>> +    RTE_UACCE_PARAM_NAME,
>> +};
>> +
>> +static const char *const uacce_params_keys[] = {
>> +    [RTE_UACCE_PARAM_NAME] = "name",
>> +    NULL,
>> +};
>> +
>> +#define FOREACH_DEVICE_ON_UACCEBUS(p)    \
>> +        RTE_TAILQ_FOREACH(p, &uacce_bus.device_list, next)
>> +#define FOREACH_DRIVER_ON_UACCEBUS(p)    \
>> +        RTE_TAILQ_FOREACH(p, &uacce_bus.driver_list, next)
>> +
>> +extern int uacce_bus_logtype;
>> +#define UACCE_BUS_LOG(level, fmt, args...) \
>> +    rte_log(RTE_LOG_ ## level, uacce_bus_logtype, "uacce: " fmt "\n", \
>> +        ##args)
>> +#define UACCE_BUS_ERR(fmt, args...) UACCE_BUS_LOG(ERR, fmt, ##args)
>> +#define UACCE_BUS_WARN(fmt, args...) UACCE_BUS_LOG(WARNING, fmt, ##args)
>> +#define UACCE_BUS_INFO(fmt, args...) UACCE_BUS_LOG(INFO, fmt, ##args)
>> +#define UACCE_BUS_DEBUG(fmt, args...) UACCE_BUS_LOG(DEBUG, fmt, ##args)
>> +
>> +
>> +static struct rte_devargs *
>> +uacce_devargs_lookup(const char *dev_name)
>> +{
>> +    char name[RTE_UACCE_DEV_PATH_SIZE] = {0};
>> +    struct rte_devargs *devargs;
>> +
>> +    snprintf(name, sizeof(name), "%s%s", UACCE_DEV_PREFIX, dev_name);
>> +    RTE_EAL_DEVARGS_FOREACH("uacce", devargs) {
>> +        if (strcmp(devargs->name, name) == 0)
>> +            return devargs;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static bool
>> +uacce_ignore_device(const char *dev_name)
>> +{
>> +    struct rte_devargs *devargs = uacce_devargs_lookup(dev_name);
>> +
>> +    switch (uacce_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;
>> +}
>> +
>> +/*
>> + * Returns the number of bytes read (removed last newline) on success.
>> + * Otherwise negative value is returned.
>> + */
>> +static int
>> +uacce_read_attr(const char *dev_root, const char *attr, char *buf, uint32_t sz)
>> +{
>> +    char filename[PATH_MAX] = {0};
>> +    int ret;
>> +    int fd;
>> +    int i;
>> +
>> +    snprintf(filename, sizeof(filename), "%s/%s", dev_root, attr);
>> +    fd = open(filename, O_RDONLY, 0);
>> +    if (fd < 0) {
>> +        UACCE_BUS_ERR("failed to open %s", filename);
>> +        return -EIO;
>> +    }
>> +
>> +    ret = read(fd, buf, sz);
>> +    if (ret > 0) {
>> +        /* Remove the last new line character. */
>> +        for (i = ret - 1; i >= 0; i--) {
>> +            if (buf[i] == '\n') {
>> +                buf[i] = '\0';
>> +                ret--;
>> +                break;
>> +            }
>> +        }
>> +    }
>> +    if (ret <= 0) {
> close(fd) is also needed here.

ack

>> +        UACCE_BUS_ERR("failed to read %s", filename);
>> +        return -EIO;
>> +    }
>> +
>> +    close(fd);
>> +
>> +    return ret;
>> +}
>> +
>> +/* 0 on success. Otherwise negative value is returned. */
>> +static int
>> +uacce_read_attr_int(const char *dev_root, const char *attr, int *val)
>> +{
>> +    char buf[RTE_UACCE_ATTR_MAX_SIZE] = {0};
>> +    char *s = NULL;
>> +    int ret;
>> +
>> +    ret = uacce_read_attr(dev_root, attr, buf, sizeof(buf) - 1);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    *val = strtol(buf, &s, 0);
>> +    if (s[0] != '\0') {
>> +        UACCE_BUS_ERR("read attr %s/%s expect an integer value", dev_root, attr);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/* 0 on success. Otherwise negative value is returned. */
>> +static int
>> +uacce_read_attr_u32(const char *dev_root, const char *attr, uint32_t *val)
>> +{
>> +    char buf[RTE_UACCE_ATTR_MAX_SIZE] = {0};
>> +    char *s = NULL;
>> +    int ret;
>> +
>> +    ret = uacce_read_attr(dev_root, attr, buf, sizeof(buf) - 1);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    *val = strtoul(buf, &s, 0);
>> +    if (s[0] != '\0') {
>> +        UACCE_BUS_ERR("read attr %s/%s expect an uint32 value", dev_root, attr);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +uacce_read_api(struct rte_uacce_device *dev)
>> +{
>> +    int ret = uacce_read_attr(dev->dev_root, "api", dev->api, sizeof(dev->api) - 1);
>> +    if (ret < 0)
>> +        return ret;
>> +    return 0;
> Could you use return uacce_read_attr()? it is more concise.

This function should return zero when success. and uacce_read_attr will return readed bytes after read.
So it need to convert like above.

>> +}
>> +
>> +static int
>> +uacce_read_algs(struct rte_uacce_device *dev)
>> +{
>> +    int ret = uacce_read_attr(dev->dev_root, "algorithms", dev->algs, sizeof(dev->algs) - 1);
>> +    if (ret < 0)
>> +        return ret;
>> +    return 0;
> return uacce_read_attr()

same as above reason.

>> +}
>> +
>> +static int
>> +uacce_read_flags(struct rte_uacce_device *dev)
>> +{
>> +    return uacce_read_attr_u32(dev->dev_root, "flags", &dev->flags);
>> +}
>> +
>> +static void
>> +uacce_read_numa_node(struct rte_uacce_device *dev)
>> +{
>> +    int ret = uacce_read_attr_int(dev->dev_root, "device/numa_node", &dev->numa_node);
>> +    if (ret != 0) {
>> +        UACCE_BUS_WARN("read attr numa_node failed! set to default");
>> +        dev->numa_node = -1;
>> +    }
>> +}
>> +
>> +static int
>> +uacce_read_qfrt_sz(struct rte_uacce_device *dev)
>> +{
>> +    int ret = uacce_read_attr_u32(dev->dev_root, "region_mmio_size",
>> +                      &dev->qfrt_sz[RTE_UACCE_QFRT_MMIO]);
>> +    if (ret != 0)
>> +        return ret;
>> +    return uacce_read_attr_u32(dev->dev_root, "region_dus_size",
>> +                   &dev->qfrt_sz[RTE_UACCE_QFRT_DUS]);
>> +}
>> +
>> +static int
>> +uacce_verify(struct rte_uacce_device *dev)
>> +{
>> +    if (!(dev->flags & UACCE_DEV_FLGA_SVA)) {
>> +        UACCE_BUS_WARN("device %s don't support SVA, skip it!", dev->name);
>> +        return 1; /* >0 will skip this device. */
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Scan one UACCE sysfs entry, and fill the devices list from it.
>> + * It reads api/algs/flags/numa_node/region-size (please refer Linux kernel:
>> + * Documentation/ABI/testing/sysfs-driver-uacce) and stores them for later
>> + * device-driver matching, driver init...
>> + */
>> +static int
>> +uacce_scan_one(const char *dev_name)
>> +{
>> +    struct rte_uacce_device *dev = NULL;
> This assignment for dev is redundant.

ack

>> +    int ret;
>> +
>> +    dev = calloc(1, sizeof(*dev));
>> +    if (!dev)
>> +        return -ENOMEM;
>> +
>> +    dev->device.bus = &uacce_bus.bus;
>> +    dev->device.name = dev->name;
>> +    dev->device.devargs = uacce_devargs_lookup(dev_name);
>> +    snprintf(dev->name, sizeof(dev->name), "%s", dev_name);
>> +    snprintf(dev->dev_root, sizeof(dev->dev_root), "%s/%s",
>> +         UACCE_BUS_CLASS_PATH, dev_name);
>> +    snprintf(dev->cdev_path, sizeof(dev->cdev_path), "/dev/%s", dev_name);
>> +
>> +    ret = uacce_read_api(dev);
>> +    if (ret != 0)
>> +        goto err;
>> +    ret = uacce_read_algs(dev);
>> +    if (ret != 0)
>> +        goto err;
>> +    ret = uacce_read_flags(dev);
>> +    if (ret != 0)
>> +        goto err;
>> +    uacce_read_numa_node(dev);
>> +    ret = uacce_read_qfrt_sz(dev);
>> +    if (ret != 0)
>> +        goto err;
>> +
>> +    ret = uacce_verify(dev);
>> +    if (ret != 0)
>> +        goto err;
>> +
>> +    TAILQ_INSERT_TAIL(&uacce_bus.device_list, dev, next);
>> +    return 0;
>> +
>> +err:
>> +    free(dev);
>> +    return ret;
>> +}
>> +
>> +static int
>> +uacce_scan(void)
>> +{
>> +    struct dirent *e;
>> +    DIR *dir;
>> +
>> +    dir = opendir(UACCE_BUS_CLASS_PATH);
>> +    if (dir == NULL) {
>> +        UACCE_BUS_LOG(INFO, "open %s failed!", UACCE_BUS_CLASS_PATH);
>> +        return 0;
> Why return 0 here?

This sitation should not treated as error.
If return non-zero, the rte_bus_scan() will output a error trace "Scan for uacce bus failed".
But the reason maybe the havn't loaded uacce.ko.
So I think it's okay to return zero here.

>> +    }
>> +
>> +    while ((e = readdir(dir)) != NULL) {
>> +        if (e->d_name[0] == '.')
>> +            continue;
>> +
>> +        if (strlen(e->d_name) >= RTE_DEV_NAME_MAX_LEN) {
>> +            UACCE_BUS_LOG(WARNING, "uacce device name %s too long, skip it!",
>> +                      e->d_name);
>> +            continue;
>> +        }
>> +
>> +        if (uacce_ignore_device(e->d_name))
>> +            continue;
>> +
>> +        if (uacce_scan_one(e->d_name) < 0)
>> +            goto error;
>> +    }
>> +    closedir(dir);
>> +    return 0;
>> +
>> +error:
>> +    closedir(dir);
>> +    return -1;
>> +}
>> +
>> +static bool
>> +uacce_match(const struct rte_uacce_driver *dr, const struct rte_uacce_device *dev)
>> +{
>> +    const struct rte_uacce_id *id_table;
>> +    uint32_t len;
>> +    char *map;
>> +
>> +    for (id_table = dr->id_table; id_table->dev_api != NULL; id_table++) {
>> +        if (strcmp(id_table->dev_api, dev->api) != 0)
>> +            continue;
>> +
>> +        if (id_table->dev_alg == NULL)
>> +            return 1;
> This func() should use "ture" or "false" as return value.

ack

> Should here return "false"？

it means only match dev_api if dev_alg is NULL, so it's okay to return 1(true) here.

>> +
>> +        /* The dev->algs's algrothims is separated by new line, for
>> +         * example: dev->algs could be: aaa\nbbbb\ncc, which has three
>> +         * algorithms: aaa, bbbb and cc.
>> +         * The id_table->dev_alg should be a single algrithm, e.g. bbbb.
>> +         */
>> +        map = strstr(dev->algs, id_table->dev_alg);
>> +        if (map == NULL)
>> +            continue;
>> +        if (map != dev->algs && map[-1] != '\n')
>> +            continue;
>> +        len = strlen(id_table->dev_alg);
>> +        if (map[len] != '\0' && map[len] != '\n')
>> +            continue;
>> +
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +uacce_probe_one_driver(struct rte_uacce_driver *dr, struct rte_uacce_device *dev)
>> +{
>> +    const char *dev_name = dev->name;
>> +    bool already_probed;
>> +    int ret;
>> +
>> +    if (!uacce_match(dr, dev))
>> +        /* Match of device and driver failed */
>> +        return 1;
>> +
>> +    already_probed = rte_dev_is_probed(&dev->device);
>> +    if (already_probed) {
>> +        UACCE_BUS_INFO("device %s is already probed", dev_name);
>> +        return -EEXIST;
>> +    }
>> +
>> +    UACCE_BUS_DEBUG("probe device %s using driver %s", dev_name, dr->driver.name);
>> +
>> +    ret = dr->probe(dr, dev);
>> +    if (ret != 0) {
>> +        UACCE_BUS_ERR("probe device %s with driver %s failed %d",
>> +                  dev_name, dr->driver.name, ret);
>> +    } else {
>> +        dev->device.driver = &dr->driver;
>> +        dev->driver = dr;
>> +        UACCE_BUS_DEBUG("probe device %s with driver %s success",
>> +                dev_name, dr->driver.name);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int
>> +uacce_probe_all_drivers(struct rte_uacce_device *dev)
>> +{
>> +    struct rte_uacce_driver *dr = NULL;
>> +    int rc = 0;
> reduntant assignment.

ack

>> +
>> +    FOREACH_DRIVER_ON_UACCEBUS(dr) {
>> +        rc = uacce_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
>> +uacce_probe(void)
>> +{
>> +    struct rte_uacce_device *dev = NULL;
>> +    size_t probed = 0, failed = 0;
>> +    int ret = 0;
> This ret is reduntant assignment.

ack

Will send v2 to fix the place which acks.

Thanks.