Re: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

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