From: Thomas Monjalon <thomas@monjalon.net>
To: Parav Pandit <parav@nvidia.com>, Xueming Li <xuemingl@nvidia.com>
Cc: dev@dpdk.org, Wang Haiyue <haiyue.wang@intel.com>,
Kinsella Ray <mdr@ashroe.eu>,
david.marchand@redhat.com, ferruh.yigit@intel.com
Subject: Re: [dpdk-dev] [PATCH v4 2/2] bus/auxiliary: introduce auxiliary bus
Date: Mon, 21 Jun 2021 18:11:29 +0200 [thread overview]
Message-ID: <2722998.9jiA4btz4K@thomas> (raw)
In-Reply-To: <20210613125846.19852-2-xuemingl@nvidia.com>
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 <xuemingl@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.
next prev parent reply other threads:[~2021-06-21 16:11 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
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 [this message]
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=2722998.9jiA4btz4K@thomas \
--to=thomas@monjalon.net \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=haiyue.wang@intel.com \
--cc=mdr@ashroe.eu \
--cc=parav@nvidia.com \
--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).