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 6BD6FA0547; Mon, 21 Jun 2021 18:11:38 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E444841158; Mon, 21 Jun 2021 18:11:37 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id 4A94440040 for ; Mon, 21 Jun 2021 18:11:36 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id A697F5C00C4; Mon, 21 Jun 2021 12:11:34 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Mon, 21 Jun 2021 12:11:34 -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= zPUCBceWIVuiTXAfURELSh+44yhDEWK51dnWeDEASk0=; b=cd5ilMNd848RWHqv 3ClceEUeUht8qm2DwsoA8YrdE3ACpKnncD3Wm/tXj2H1PZkFtzsapG/K6MUlWZvG 1piGJiODUNICkQyt48NsIQiNaarNLueHlMdtjxekN7huq//9fDOqpC8ugLwD15ef X7Xqe3jb/Gtrz1lrQCgC/7UnggYOodSRL686JCWLcJ+mJpCub71WX/hxC+Fc7RIA +N2sKjbRvAIsRT5KNRhnutAOb48EBVLExpHqtZnaxa3GxVEOnY0WgJ4n6OJzHFkl KXHwOh3fOPGQtFFhqzZS1iFQXBm+4YwmSW66xJ3dwmt9YI5TQLAuvsn6lzsTDMo+ oACIQw== 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=zPUCBceWIVuiTXAfURELSh+44yhDEWK51dnWeDEAS k0=; b=WScurMIlWGyXUswOhn7Ezi/UocEEoP+1HdKlwDtL2yRLqX8mXVKb2CKWi fNfRtJMg2e0X+DZQuVpoOL5qIVdd2xtbYSUEamSs5JExLbE5jbsxn+q0YxH7hExN Qh8ew3CyEpJvPEO/n0ahk2xQyZft12M5C9i7uKlVEIzxf0n8jU1ZbMaWAJoMlzLc H9+IngF4THWojxbJirilMUxWH/nC5xKuPq5OLMZ0uFwodPcdO/5x2KTRPG4rxSrj ZfhPnZXJCh24hQF/W/5ketOYAl5D/KEJteWYjR2EbRHLLbS47TshZp7CIAeyIc5T +FVmMMuiNUIeaNaDTL4WFsXBw8DUQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfeefledgleelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepjeduteevueejteehieeggfeiueeikefffefhjeetueeihefhhfdv udfhheehfeehnecuffhomhgrihhnpehkvghrnhgvlhdrohhrghenucevlhhushhtvghruf hiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghl ohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 21 Jun 2021 12:11:32 -0400 (EDT) From: Thomas Monjalon To: Parav Pandit , Xueming Li Cc: dev@dpdk.org, Wang Haiyue , Kinsella Ray , david.marchand@redhat.com, ferruh.yigit@intel.com Date: Mon, 21 Jun 2021 18:11:29 +0200 Message-ID: <2722998.9jiA4btz4K@thomas> In-Reply-To: <20210613125846.19852-2-xuemingl@nvidia.com> References: <20210510134732.2174-1-xuemingl@nvidia.com> <20210613125846.19852-2-xuemingl@nvidia.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v4 2/2] 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" 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/ > > Devargs syntax of auxiliary device: > -a auxiliary:[,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 [...] > --- 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:[,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 > +#include 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.