DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Xueming 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
Date: Thu, 24 Jun 2021 18:18:43 +0200	[thread overview]
Message-ID: <2020318.PlHIg8rldJ@thomas> (raw)
In-Reply-To: <20210623000349.631468-2-xuemingl@nvidia.com>

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.



  reply	other threads:[~2021-06-24 16:18 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-11 13:01 [dpdk-dev] [RFC] " Xueming Li
2021-04-12  8:29 ` Xueming(Steven) Li
2021-04-13  3:23 ` [dpdk-dev] [PATCH v1] " Xueming Li
2021-04-13  8:49   ` Thomas Monjalon
2021-04-14  2:59   ` Wang, Haiyue
2021-04-14  8:17     ` Thomas Monjalon
2021-04-14  8:30       ` Wang, Haiyue
2021-04-14 15:49       ` Xueming(Steven) Li
2021-04-14 15:39     ` Xueming(Steven) Li
2021-04-14 16:13       ` Wang, Haiyue
2021-04-15  7:35   ` Wang, Haiyue
2021-04-15  7:46     ` Xueming(Steven) Li
2021-04-15  7:51       ` Wang, Haiyue
2021-04-15  7:55         ` Xueming(Steven) Li
2021-04-15  7:59           ` Thomas Monjalon
2021-04-15  8:06             ` Wang, Haiyue
2021-05-10 13:47   ` [dpdk-dev] [RFC v2] " Xueming Li
2021-05-11  9:47     ` Kinsella, Ray
2021-06-10  3:30       ` Xueming(Steven) Li
2021-06-08  7:53     ` Thomas Monjalon
2021-06-08  8:41       ` Wang, Haiyue
2021-06-10  6:29         ` Xueming(Steven) Li
2021-06-10 15:16           ` Wang, Haiyue
2021-06-10  6:30       ` Xueming(Steven) Li
2021-06-13  8:19     ` [dpdk-dev] [PATCH v3 1/2] devargs: add common key definition Xueming Li
2021-06-13  8:19     ` [dpdk-dev] [PATCH v3 2/2] bus/auxiliary: introduce auxiliary bus Xueming Li
2021-06-13 12:58     ` [dpdk-dev] [PATCH v4 1/2] devargs: add common key definition Xueming Li
2021-06-21  8:08       ` Thomas Monjalon
2021-06-23  0:03       ` [dpdk-dev] [PATCH v5 " Xueming Li
2021-06-24 10:05         ` Thomas Monjalon
2021-06-23  0:03       ` [dpdk-dev] [PATCH v5 2/2] bus/auxiliary: introduce auxiliary bus Xueming Li
2021-06-24 16:18         ` Thomas Monjalon [this message]
2021-06-25  3:26           ` Xueming(Steven) Li
2021-06-25 12:03             ` Thomas Monjalon
2021-06-25 11:47         ` [dpdk-dev] [PATCH v6 1/2] devargs: add common key definition Xueming Li
2021-07-04 15:51           ` Andrew Rybchenko
2021-07-05  5:36           ` [dpdk-dev] [PATCH v7 " Xueming Li
2021-07-05  5:36           ` [dpdk-dev] [PATCH v7 2/2] bus/auxiliary: introduce auxiliary bus Xueming Li
2021-07-05  6:47             ` Xueming(Steven) Li
2021-07-05  6:45           ` [dpdk-dev] [PATCH v8 1/2] devargs: add common key definition Xueming Li
2021-07-05  9:26             ` Xueming(Steven) Li
2021-07-05  6:45           ` [dpdk-dev] [PATCH v8 2/2] bus/auxiliary: introduce auxiliary bus Xueming Li
2021-07-05  9:19             ` Andrew Rybchenko
2021-07-05  9:30               ` Xueming(Steven) Li
2021-07-05  9:35                 ` Andrew Rybchenko
2021-07-05 14:57                   ` Thomas Monjalon
2021-07-05 15:06                     ` Andrew Rybchenko
2021-07-05 16:47             ` Thomas Monjalon
2021-06-25 11:47         ` [dpdk-dev] [PATCH v6 " Xueming Li
2021-07-04 16:13           ` Andrew Rybchenko
2021-07-05  5:47             ` Xueming(Steven) Li
2021-08-04  9:50           ` Kinsella, Ray
2021-08-04  9:56             ` Xueming(Steven) Li
2021-08-04 10:00           ` Kinsella, Ray
     [not found]             ` <DM4PR12MB5373DBD9E73E5E0E8505C129A1F19@DM4PR12MB5373.namprd12.prod.outlook.com>
     [not found]               ` <97d5d1b3-40c3-09ac-2978-83c984b30af0@ashroe.eu>
     [not found]                 ` <DM4PR12MB53736410D2C07101F872363EA1F19@DM4PR12MB5373.namprd12.prod.outlook.com>
2021-08-04 12:14                   ` Kinsella, Ray
2021-08-04 13:00                     ` Xueming(Steven) Li
2021-08-04 13:12                       ` Thomas Monjalon
2021-08-04 13:53                         ` Kinsella, Ray
2021-08-04 14:13                           ` Thomas Monjalon
2021-06-13 12:58     ` [dpdk-dev] [PATCH v4 " Xueming Li
2021-06-21 16:11       ` Thomas Monjalon
2021-06-22 23:50         ` Xueming(Steven) Li
2021-06-23  8:15           ` Thomas Monjalon
2021-06-23 14:52             ` Xueming(Steven) Li
2021-06-24  6:37               ` Thomas Monjalon
2021-06-24  8:42                 ` Xueming(Steven) Li
2021-06-23  8:21           ` Thomas Monjalon
2021-06-23 13:54             ` Xueming(Steven) Li
2021-06-25  4:34 ` [dpdk-dev] [RFC] " Stephen Hemminger
2021-06-25 11:24   ` Xueming(Steven) Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2020318.PlHIg8rldJ@thomas \
    --to=thomas@monjalon.net \
    --cc=dev@dpdk.org \
    --cc=haiyue.wang@intel.com \
    --cc=mdr@ashroe.eu \
    --cc=xuemingl@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).