> -----Original Message----- > From: Thomas Monjalon <tho...@monjalon.net> > Sent: Tuesday, June 8, 2021 3:53 PM > To: Xueming(Steven) Li <xuemi...@nvidia.com> > Cc: dev@dpdk.org; Parav Pandit <pa...@nvidia.com>; Ray Kinsella > <m...@ashroe.eu>; Wang Haiyue <haiyue.w...@intel.com>; > andrew.rybche...@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 <xuemi...@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.
