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? > [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> [...] > --- 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. > +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. [...] > +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. > + > +/* 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" > +}; > + > +/** 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.
