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

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

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, June 25, 2021 12:19 AM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Wang Haiyue <haiyue.wang@intel.com>; Kinsella Ray <mdr@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.
>