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