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

[Thomas Monjalon <thomas@monjalon.net>]

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.