23/06/2021 02:03, Xueming Li:
> +static int
> +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *drv,
> + struct rte_auxiliary_device *dev)
[...]
> + AUXILIARY_LOG(DEBUG, "Probe driver: %s", drv->driver.name);
I think the debug log above is useless given the ones below.
> +
> + iova_mode = rte_eal_iova_mode();
> + if ((drv->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 &&
> + iova_mode != RTE_IOVA_VA) {
> + AUXILIARY_LOG(ERR, "Expecting VA IOVA mode but current mode is
> PA, not initializing");
The error log should mention which driver is requesting VA and not initializing.
> + return -EINVAL;
> + }
> +
> + dev->driver = drv;
> +
> + AUXILIARY_LOG(INFO, "Probe auxiliary driver: %s device: %s (socket %i)",
> + drv->driver.name, dev->name, dev->device.numa_node);
> + ret = drv->probe(drv, dev);
> + if (ret != 0)
> + dev->driver = NULL;
> + else
> + dev->device.driver = &drv->driver;
> +
> + return ret;
> +}
> +
> +/*
> + * Call the remove() function of the driver.
> + */
> +static int
> +rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev)
> +{
> + struct rte_auxiliary_driver *drv;
> + int ret = 0;
> +
> + if (dev == NULL)
> + return -EINVAL;
> +
> + drv = dev->driver;
> +
> + AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i",
> + dev->name, dev->device.numa_node);
> +
> + AUXILIARY_LOG(DEBUG, "remove driver: %s %s",
> + dev->name, drv->driver.name);
Better to merge both logs together.
> +
> + if (drv->remove != NULL) {
> + ret = drv->remove(dev);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* clear driver structure */
> + dev->driver = NULL;
> + dev->device.driver = NULL;
> +
> + return 0;
> +}
[...]
> +static int
> +auxiliary_parse(const char *name, void *addr)
> +{
> + struct rte_auxiliary_driver *drv = NULL;
> + const char **out = addr;
> +
> + /* Allow dummy name to prevent bus scan. */
> + if (strlen(name) == 0)
> + return 0;
Which syntax is it?
[...]
> + kvargs = rte_kvargs_parse(str, auxiliary_params_keys);
> + if (kvargs == NULL) {
> + RTE_LOG(ERR, EAL, "cannot parse argument list\n");
Should use AUXILIARY_LOG.
[...]
> +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> + /* Get numa node, default to 0 if not present */
s/numa/NUMA/
[...]
> +/*
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + */
Simpler: Scan the devices in the auxiliary bus.
> +int
> +auxiliary_scan(void)
> +{
[...]
> +++ b/drivers/bus/auxiliary/private.h
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2021 NVIDIA Corporation & Affiliates
> + */
> +
> +#ifndef _AUXILIARY_PRIVATE_H_
> +#define _AUXILIARY_PRIVATE_H_
nit: we can avoid the first and final underscores
[...]
> +/*
> + * Validate whether a device with given auxiliary device should be ignored
> + * or not.
> + */
> +bool auxiliary_ignore_device(const char *name);
Suggestion: auxiliary_is_ignored_device() ?
> +/*
> + * Add an auxiliary device to the auxiliary bus (append to auxiliary Device
s/Device/device/
> + * list). This function also updates the bus references of the auxiliary
> + * Device (and the generic device object embedded within.
s/(and/and/
> + */
> +void auxiliary_add_device(struct rte_auxiliary_device *aux_dev);
> +
> +/*
> + * Insert an auxiliary device in the auxiliary bus at a particular location
> + * in the device list. It also updates the auxiliary bus reference of the
> + * new devices to be inserted.
> + */
> +void auxiliary_insert_device(struct rte_auxiliary_device *exist_aux_dev,
> + struct rte_auxiliary_device *new_aux_dev);
> +
> +/*
> + * Match the auxiliary Driver and Device by driver function
no need of capital letters in driver and device.
> + */
> +bool auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
> + const struct rte_auxiliary_device *aux_dev);
> +
> +/*
> + * Iterate over internal devices, matching any device against the provided
What do you mean by "internal"?
> + * string.
> + */
> +void *auxiliary_dev_iterate(const void *start, const char *str,
> + const struct rte_dev_iterator *it);
> +
[...]
> +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> +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. */
No capital in Probe and Remove
> + rte_auxiliary_dma_map_t *dma_map; /**< Device dma map function. */
> + rte_auxiliary_dma_unmap_t *dma_unmap; /**< Device dma unmap function. */
s/dma/DMA/
There may be other occurences to check.
> + uint32_t drv_flags; /**< Flags RTE_AUXILIARY_DRV_*. */
> +};
The alignment of some comments above can be improved by one space.
[...]
> +/** Helper for auxiliary device registration from driver instance */
> +#define RTE_PMD_REGISTER_AUXILIARY(nm, auxiliary_drv) \
> + RTE_INIT(auxiliaryinitfn_ ##nm) \
> + { \
> + (auxiliary_drv).driver.name = RTE_STR(nm); \
> + rte_auxiliary_register(&(auxiliary_drv)); \
> + } \
> + RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
I think it is better not trying to align backslashes in such macro,
but leave only a single space.
[...]
> +++ b/drivers/bus/auxiliary/version.map
> @@ -0,0 +1,3 @@
> +EXPERIMENTAL {
> + # added in 21.08
> +};
Looks like you need to bring the symbols back :)
Looks good overrall, thanks.
