From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfedtkedgudduiecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeevvdfhleeviefhteehtefgkeehlefhgfefkeejgfektdffgeeu vdetfffhjefgvdenucffohhmrghinhepkhgvrhhnvghlrdhorhhgpdguphgukhdrohhrgh enucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 8 Jun 2021 03:53:11 -0400 (EDT) From: Thomas Monjalon To: Xueming Li Cc: dev@dpdk.org, Parav Pandit , Ray Kinsella , Wang Haiyue , 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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/ > > [1] kernel auxiliary bus document: > https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html > > Signed-off-by: Xueming Li [...] > +++ 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +/* 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.