From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id C2E16A034F;
Tue, 8 Jun 2021 09:53:17 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 4FD6F410E7;
Tue, 8 Jun 2021 09:53:17 +0200 (CEST)
Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com
[66.111.4.28]) by mails.dpdk.org (Postfix) with ESMTP id 2102E4013F
for <dev@dpdk.org>; Tue, 8 Jun 2021 09:53:16 +0200 (CEST)
Received: from compute3.internal (compute3.nyi.internal [10.202.2.43])
by mailout.nyi.internal (Postfix) with ESMTP id 32C325C01AC;
Tue, 8 Jun 2021 03:53:13 -0400 (EDT)
Received: from mailfrontend1 ([10.202.2.162])
by compute3.internal (MEProxy); Tue, 08 Jun 2021 03:53:13 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
from:to:cc:subject:date:message-id:in-reply-to:references
:mime-version:content-transfer-encoding:content-type; s=fm1; bh=
27CbI6nf9bOJADRtVw6Dcgfz05wAzLA/yKuTgy/QjHw=; b=mjmjTayj3isdTREd
s6N/id0bjZu0PplY50ZO8+RqeYCIgmIe3kH8ZbP0J/oepU12T0Q1C2M3gD1YVelJ
8Veas8Fz3PHzUwjbSe9mkqP57AcyPAYnhwSkLD73ZSVGgRplfKx5XqbTCEZrM1p0
424RQObE+/Xzz8n/sZRdsqC6KmTd9ACwl/E7+LL2WGUU7GY4N8gKXrIf0iQCRreR
rmnsChefKkXNsLOJ0DhNNnjwUj9xu2anIXalkS/D56lpDQlcgilfzkjg3ZAo/4WU
BAnHH3dOHv25VVl3gQCpZfNa4CP7yjhiS7s0qRmwRyBBEsqu0D3Zh/coUYV6vn4t
MJ8xxg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
messagingengine.com; h=cc:content-transfer-encoding:content-type
:date:from:in-reply-to:message-id:mime-version:references
:subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
:x-sasl-enc; s=fm3; bh=27CbI6nf9bOJADRtVw6Dcgfz05wAzLA/yKuTgy/Qj
Hw=; b=OvMqzVTsK88QHcnN+v+Amlbz0eIEJelY9VRoDwuasayW0OknPBPB0F3XA
XxJup5b8ei5+8cBNQJXPnTIilL0hqNlIfyrzFdxKm3EgIDNCbUd+T8co56ixZpy/
ZhgAtl1Y4iG/FUE5HEmSnITHc+axwT/EL8l4Tt8QxoDHC1yw9ImNyQ0GOVyQdXF5
8cwx6uxkdA8qfjT/QonrJ9n0HRk9CQ6pfzMhpwwax+wROtAaod3Qhbc7+rqdMTCv
AyOmD3ylM9X6eilQKuSsxuEHP/9UyRPfmrB7yTjR+V4MKANaR4rPlYda3DcD8fDy
hfK5PAX34aguSewszMHojZrgpLgTg==
X-ME-Sender: <xms:6CG_YIti_TNOLVf9Y8nd2JHixN_t-jVjyebEgW9DoWiwSECrEjBKrQ>
<xme:6CG_YFfzhXNwdKsOqzoz6wVAP4joCm4mqFlsT8roJvfm1jAx7PFtpTAEFy6z6Nyzb
ZYMdAcWm5VkIckTeg>
X-ME-Received: <xmr:6CG_YDwePUaQOXMOEjtM32d3SckadjvuoyLvZuekI1CRTaV70xYY4_OXOy11x_WJuyALSbgWEGmNdbVl09SpN-EWcg>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfedtkedgudduiecutefuodetggdotefrod
ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh
necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd
enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm
rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc
ggtffrrghtthgvrhhnpeevvdfhleeviefhteehtefgkeehlefhgfefkeejgfektdffgeeu
vdetfffhjefgvdenucffohhmrghinhepkhgvrhhnvghlrdhorhhgpdguphgukhdrohhrgh
enucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho
mhgrshesmhhonhhjrghlohhnrdhnvght
X-ME-Proxy: <xmx:6CG_YLMzY_0s2ruJL4i1n_ILCI3rDktHSUdWvCAiACoiU0CXwy0DNw>
<xmx:6CG_YI-Z9yIF7e9KIBQdiHrRRK3Q9DCBPX4NfaYHyBtQNPGtbfR3UA>
<xmx:6CG_YDV4HWqglxlSvIBq_rXEpq9Ws8InbvIKlfvYqscKZ11sVp_vsw>
<xmx:6SG_YMb4Za8KatSZ4KQS1_Obly2jNYzexeHuYQ8YG6zwVPa0CLDCLA>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue,
8 Jun 2021 03:53:11 -0400 (EDT)
From: Thomas Monjalon <thomas@monjalon.net>
To: Xueming Li <xuemingl@nvidia.com>
Cc: dev@dpdk.org, Parav Pandit <parav@nvidia.com>, Ray Kinsella <mdr@ashroe.eu>,
Wang Haiyue <haiyue.wang@intel.com>, andrew.rybchenko@oktetlabs.ru
Date: Tue, 08 Jun 2021 09:53:10 +0200
Message-ID: <2237980.2ZMkXp7bNk@thomas>
In-Reply-To: <20210510134732.2174-1-xuemingl@nvidia.com>
References: <20210413032329.25551-1-xuemingl@nvidia.com>
<20210510134732.2174-1-xuemingl@nvidia.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [dpdk-dev] [RFC v2] bus/auxiliary: introduce auxiliary bus
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
10/05/2021 15:47, Xueming Li:
> Auxiliary [1] provides a way to split function into child-devices
Auxiliary -> Auxiliary bus
> representing sub-domains of functionality. Each auxiliary_device
auxiliary_device -> auxiliary device
> represents a part of its parent functionality.
>
> Auxiliary device is identified by unique device name, sysfs path:
> /sys/bus/auxiliary/devices/<name>
>
> [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>
[...]
> +++ b/drivers/bus/auxiliary/auxiliary_common.c
[...]
> +int auxiliary_bus_logtype;
You don't need to declare this variable, it is done in a macro.
> +
> +static struct rte_devargs *
> +auxiliary_devargs_lookup(const char *name)
> +{
> + struct rte_devargs *devargs;
> +
> + RTE_EAL_DEVARGS_FOREACH("auxiliary", devargs) {
"auxiliary" should be defined as a macro.
[...]
> +/*
> + * Test whether the auxiliary device exist
> + */
> +__rte_weak bool
The comment should explain it is a stub for OS
not supporting auxiliary bus.
> +auxiliary_dev_exists(const char *name)
> +{
> + RTE_SET_USED(name);
> + return false;
> +}
> +
> +/*
> + * Scan the content of the auxiliary bus, and the devices in the devices
> + * list
> + */
Same here about the stub comment.
> +__rte_weak int
> +auxiliary_scan(void)
> +{
> + return 0;
> +}
> +
Please insert a comment here.
> +void
> +auxiliary_on_scan(struct rte_auxiliary_device *aux_dev)
> +{
> + aux_dev->device.devargs = auxiliary_devargs_lookup(aux_dev->name);
> +}
> +
> +/*
> + * Match the auxiliary Driver and Device using driver function.
driver and device do not need capital letters
> + */
> +bool
> +auxiliary_match(const struct rte_auxiliary_driver *aux_drv,
> + const struct rte_auxiliary_device *aux_dev)
> +{
> + if (aux_drv->match == NULL)
> + return false;
> + return aux_drv->match(aux_dev->name);
> +}
> +
> +/*
> + * Call the probe() function of the driver.
> + */
> +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 */
Please keep all the comments more uniform by starting with a capital
and ends with a point.
[...]
> +RTE_REGISTER_BUS(auxiliary, auxiliary_bus.bus);
> +RTE_LOG_REGISTER(auxiliary_bus_logtype, bus.auxiliary, NOTICE);
Please replace with RTE_LOG_REGISTER_DEFAULT.
[...]
> +++ b/drivers/bus/auxiliary/linux/auxiliary.c
[...]
> +/**
> + * @file
> + * Linux auxiliary probing.
> + */
No need of doxygen comment in a .c file.
[...]
> +++ 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')
> +if is_linux
> + sources += files('linux/auxiliary.c')
> +endif
> +deps += ['kvargs']
Please change the indent with spaces
and check with devtools/check-meson.py
[...]
> +++ b/drivers/bus/auxiliary/private.h
[...]
> +/* Auxiliary bus iterators */
> +#define FOREACH_DEVICE_ON_AUXILIARYBUS(p) \
Please avoid tabs at the end of the line.
A space is enough before the backslash.
> + TAILQ_FOREACH(p, &(auxiliary_bus.device_list), next)
> +
> +#define FOREACH_DRIVER_ON_AUXILIARYBUS(p) \
> + TAILQ_FOREACH(p, &(auxiliary_bus.driver_list), next)
> +
> +/**
> + * Test whether the auxiliary device exist
> + *
> + * @param name
> + * Auxiliary device name
> + * @return
> + * true on exists, false otherwise
> + */
> +bool auxiliary_dev_exists(const char *name);
Given it is not an API, no need of doxygen.
And the function is so simple that no need of comment at all.
[...]
> +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h
> +#ifndef _RTE_BUS_AUXILIARY_H_
> +#define _RTE_BUS_AUXILIARY_H_
No need of underscores before and after.
(yes I know a lot of DPDK files use such scheme)
> +
> +/**
> + * @file
> + *
> + * RTE Auxiliary Bus Interface.
No need of RTE, it has no meaning here.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/queue.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +
> +#include <rte_debug.h>
> +#include <rte_interrupts.h>
> +#include <rte_dev.h>
> +#include <rte_bus.h>
> +#include <rte_kvargs.h>
> +
> +/* Forward declarations */
> +struct rte_auxiliary_driver;
> +struct rte_auxiliary_bus;
> +struct rte_auxiliary_device;
> +
> +/**
> + * Match function for the driver to decide if device can be handled.
> + *
> + * @param name
> + * Pointer to the auxiliary device name.
> + * @return
> + * Whether the driver can handle the auxiliary device.
> + */
> +typedef bool(*rte_auxiliary_match_t) (const char *name);
I disagree with the earlier comment asking for typedef pointer
(based on one of my patches).
Actually Andrew's suggestion makes sense:
http://mails.dpdk.org/archives/dev/2021-June/210665.html
[...]
> +++ b/drivers/bus/auxiliary/version.map
> +DPDK_21 {
> + local: *;
> +};
We don't need this empty section.
> +
> +EXPERIMENTAL {
> + global:
> +
As asked by Ray, we should add this line:
# added in 21.08
> + rte_auxiliary_register;
> + rte_auxiliary_unregister;
> +};
Release notes are missing.