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.