Re: [dpdk-dev] [PATCH v4 2/2] bus/auxiliary: introduce auxiliary bus

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

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, June 22, 2021 12:11 AM
> To: Parav Pandit <parav@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Wang Haiyue <haiyue.wang@intel.com>; Kinsella Ray <mdr@ashroe.eu>; david.marchand@redhat.com;
> ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH v4 2/2] bus/auxiliary: introduce auxiliary bus
> 
> 13/06/2021 14:58, Xueming Li:
> > Auxiliary bus [1] provides a way to split function into child-devices
> > representing sub-domains of functionality. Each auxiliary device
> > represents a part of its parent functionality.
> >
> > Auxiliary device is identified by unique device name, sysfs path:
> >   /sys/bus/auxiliary/devices/<name>
> >
> > Devargs syntax of auxiliary device:
> >   -a auxiliary:<name>[,args...]
> 
> What about suggesting the new generic syntax?

I'll list both.

> 
> > [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>
> [...]
> > --- a/doc/guides/rel_notes/release_21_08.rst
> > +++ b/doc/guides/rel_notes/release_21_08.rst
> > @@ -55,6 +55,13 @@ New Features
> >       Also, make sure to start the actual text at the margin.
> >       =======================================================
> >
> > +* **Added auxiliary bus support.**
> > +
> > +  * Auxiliary bus provides a way to split function into child-devices
> > +    representing sub-domains of functionality. Each auxiliary device
> > +    represents a part of its parent functionality.
> > +  * Devargs syntax of auxiliary device: -a auxiliary:<name>[,args...]
> 
> I am not sure the release notes are the right place to provide a guide of the syntax, and this syntax is not the new generice one with
> "bus=" that we want to promote.
> I would just remove this last line from the release notes
> 
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/auxiliary_common.c
> > @@ -0,0 +1,419 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2021 Mellanox Technologies, Ltd
> 
> I think we should use the NVIDIA copyright now.

Good catch!

> 
> > +static struct rte_devargs *
> > +auxiliary_devargs_lookup(const char *name) {
> > +	struct rte_devargs *devargs;
> > +
> > +	RTE_EAL_DEVARGS_FOREACH(RTE_BUS_AXILIARY_NAME, devargs) {
> 
> Missing an "U" in RTE_BUS_AXILIARY_NAME
> 
> [...]
> > +/*
> > + * Scan the content of the auxiliary bus, and the devices in the
> > +devices
> > + * list
> 
> Simpler: Scan the devices in the auxiliary bus.
> 
> [...]
> > +/**
> > + * Update a device being scanned.
> 
> Not clear what is updated.
> It seems to be just the devargs part?
> 
> > + *
> > + * @param aux_dev
> > + *	AUXILIARY device.
> > + */
> 
> Should not be a doxygen comment.
> 
> > +void
> > +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev) {
> > +	aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> > +}
> 
> [...]
> > +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. */
> 
> I don't understand why the comment about "not blocked" here.
> The policy check is below.
> 
> > +	if (!auxiliary_match(dr, dev))
> > +		/* Match of device and driver failed */
> > +		return 1;
> > +
> > +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> > +		      dev->name, dev->device.numa_node);
> > +
> > +	/* No initialization when marked as blocked, return without error. */
> > +	if (dev->device.devargs != NULL &&
> > +	    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
> > +		AUXILIARY_LOG(INFO, "  Device is blocked, not initializing\n");
> 
> Please no indent inside logs.
> And no \n as it is already in the macro.
> 
> > +		return -1;
> > +	}
> 
> [...]
> > +static int
> > +rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev) {
> > +	struct rte_auxiliary_driver *dr;
> 
> Not sure this variable is needed.
> If you keep it, please "drv" is better.
> 
> > +	int ret = 0;
> > +
> > +	if (dev == NULL)
> > +		return -EINVAL;
> > +
> > +	dr = dev->driver;
> > +
> > +	AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i\n",
> > +		      dev->name, dev->device.numa_node);
> > +
> > +	AUXILIARY_LOG(DEBUG, "  remove driver: %s %s\n",
> > +		      dev->name, dr->driver.name);
> > +
> > +	if (dr->remove) {
> > +		ret = dr->remove(dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> [...]
> > +/*
> > + * Scan the content of the auxiliary bus, and call the probe()
> > +function for
> > + *
> > + * all registered drivers that have a matching entry in its id_table
> > + * for discovered devices.
> 
> Please elaborate what is the id_table.

Hmm, legacy code form pci bus, remove it.

> 
> [...]
> > +static int
> > +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova,
> > +size_t len) {
> > +	struct rte_auxiliary_device *aux_dev = RTE_DEV_TO_AUXILIARY(dev);
> > +
> > +	if (dev == NULL || !aux_dev->driver) {
> 
> For all pointers, please compare with NULL, they are not booleans.
> 
> > +		rte_errno = EINVAL;
> > +		return -1;
> > +	}
> > +	if (aux_dev->driver->dma_map)
> > +		return aux_dev->driver->dma_map(aux_dev, addr, iova, len);
> > +	rte_errno = ENOTSUP;
> > +	return -1;
> 
> I would prever the reverse logic: error first and callback return at last.
> 
> [...]
> Some code is not reviewed here to not make this mail too long.
> [...]
> 
> > --- /dev/null
> > +++ 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')
> 
> I think it should with a comma and the parenthesis on next line.
> Please check style of other meson files which were re-styled recently.
> 
> > +if is_linux
> > +    sources += files('linux/auxiliary.c') endif deps += ['kvargs']
> > +
> 
> Empty line at EOF
> 
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/private.h
> > @@ -0,0 +1,112 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2021 Mellanox Technologies, Ltd  */
> > +
> > +#ifndef _AUXILIARY_PRIVATE_H_
> > +#define _AUXILIARY_PRIVATE_H_
> > +
> > +#include <stdbool.h>
> > +#include <stdio.h>
> 
> An empty line is missing here.
> 
> > +#include "rte_bus_auxiliary.h"
> > +
> > +extern struct rte_auxiliary_bus auxiliary_bus; extern int
> > +auxiliary_bus_logtype;
> > +
> > +#define AUXILIARY_LOG(level, fmt, args...) \
> > +	rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, "%s(): " fmt "\n", \
> > +		__func__, ##args)
> 
> I suggest this better (pedantic-compliant) format:
> #define AUXILIARY_LOG(level, ...) \
>     rte_log(RTE_LOG_ ## level, auxiliary_bus_logtype, RTE_FMT("auxiliary bus: " \
>         RTE_FMT_HEAD(__VA_ARGS__,) "\n", RTE_FMT_TAIL(__VA_ARGS__,)))
> 
> I think the __func__ should not be needed if log is well written.

Thanks!

> 
> > +
> > +/* Auxiliary bus iterators */
> > +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p) \
> > +		TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> > +
> > +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p) \
> > +		TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
> 
> An underscore is missing between AUXILIARY and BUS.
> 
> > +
> > +bool auxiliary_dev_exists(const char *name);
> > +
> > +/**
> > + * Scan the content of the auxiliary bus, and the devices in the
> > +devices
> > + * list
> > + *
> > + * @return
> > + *  0 on success, negative on error
> > + */
> 
> You can make the comments shorter as it is private (no doxygen).
> 
> > +int auxiliary_scan(void);
> 
> [...]
> > + * @return void
> 
> Especially this comment is useless :)
> 
> [...]
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> [...]
> > +typedef bool(rte_auxiliary_match_t) (const char *name);
> 
> I think checkpatch will complain about the space between parens.
> 
> [...]
> > +struct rte_auxiliary_device {
> > +	TAILQ_ENTRY(rte_auxiliary_device) next;   /**< Next probed device. */
> > +	char name[RTE_DEV_NAME_MAX_LEN + 1];      /**< ASCII device name */
> > +	struct rte_device device;                 /**< Inherit core device */
> 
> core device should be before the name.
> 
> > +	struct rte_intr_handle intr_handle;       /**< Interrupt handle */
> > +	struct rte_auxiliary_driver *driver;      /**< driver used in probing */
> 
> Why in probing?
> I suggest "Device driver"

A SF device could be probed by a class driver t then another class driver, the driver field will be overridden by later probe.
Will change to "last device driver"

> 
> > +};
> > +
> > +/** List of auxiliary devices */
> > +TAILQ_HEAD(rte_auxiliary_device_list, rte_auxiliary_device);
> > +/** List of auxiliary drivers */
> > +TAILQ_HEAD(rte_auxiliary_driver_list, rte_auxiliary_driver);
> > +
> > +/**
> > + * Structure describing the auxiliary bus  */ struct
> > +rte_auxiliary_bus {
> > +	struct rte_bus bus;                  /**< Inherit the generic class */
> > +	struct rte_auxiliary_device_list device_list;  /**< List of devices */
> > +	struct rte_auxiliary_driver_list driver_list;  /**< List of drivers
> > +*/ };
> > +
> > +/**
> > + * A structure describing an auxiliary driver.
> > + */
> > +struct rte_auxiliary_driver {
> > +	TAILQ_ENTRY(rte_auxiliary_driver) next; /**< Next in list. */
> > +	struct rte_driver driver;            /**< Inherit core driver. */
> > +	struct rte_auxiliary_bus *bus;       /**< Auxiliary bus reference. */
> > +	rte_auxiliary_match_t *match;         /**< Device match function. */
> > +	rte_auxiliary_probe_t *probe;         /**< Device Probe function. */
> > +	rte_auxiliary_remove_t *remove;       /**< Device Remove function. */
> > +	rte_auxiliary_dma_map_t *dma_map;     /**< Device dma map function. */
> > +	rte_auxiliary_dma_unmap_t *dma_unmap; /**< Device dma unmap function. */
> > +	uint32_t drv_flags;                  /**< Flags RTE_auxiliary_DRV_*. */
> 
> Wrong search/replace missing capital letters.
> 
> [...]
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/version.map
> > @@ -0,0 +1,7 @@
> > +EXPERIMENTAL {
> > +	global:
> > +
> > +	# added in 21.08
> > +	rte_auxiliary_register;
> > +	rte_auxiliary_unregister;
> > +};
> 
> After more thoughts, shouldn't it be an internal symbol?
> It is used only by DPDK drivers.
> 

So users will not be able to compose their own driver and register with auxiliary bus?z

Agree with all other great comments, thanks!