> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Friday, June 25, 2021 12:19 AM
> To: Xueming(Steven) Li <xuemi...@nvidia.com>
> Cc: dev@dpdk.org; Wang Haiyue <haiyue.w...@intel.com>; Kinsella Ray
> <m...@ashroe.eu>
> Subject: Re: [dpdk-dev] [PATCH v5 2/2] bus/auxiliary: introduce auxiliary bus
>
> 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?
Allow empty device name "auxiliary:" to bypass entire auxiliary bus scan.
>
> [...]
> > + 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.
>
