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 CC56BA0547; Thu, 24 Jun 2021 18:18:49 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5878140040; Thu, 24 Jun 2021 18:18:49 +0200 (CEST) Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by mails.dpdk.org (Postfix) with ESMTP id E577A4003C for ; Thu, 24 Jun 2021 18:18:47 +0200 (CEST) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 9F5825C006D; Thu, 24 Jun 2021 12:18:46 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Thu, 24 Jun 2021 12:18:46 -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= iycaZ+DOiyXDpRjI4l2IWCt6mqjUUpC8nfnsJZhRbzw=; b=WWtZHtNeluvgKgDg k5CxI1P+V0r30/m6z1F/AgSy2GE9T5lRdq7INrKLo5j7hzU5k1Xw6qGeUW7jYoS4 DVi5L/x4uutl08Z/ryEZw3iftNPaOAWThW0Y+IaFFBUqT74hxFxkEtpEwA9X2RdU mmlyiV+mpTWaGIYLdX5WAxi7BGM4Min+JiB5qIYYiGAVn6t+9tMNzjPBQUG7/Mk8 Z/rtHEaPnrkppIFT0zyAJCuCk8rjXqkBuWAWlYbeZGYW2yDLdlF3LOr35Ecj2P3o zESyskL+/x7efAxbtJis0P6ZemzFsmZLIh8sgw05OJ6zGL65QWkLkljgzGpZdJlw B5Md1A== 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=iycaZ+DOiyXDpRjI4l2IWCt6mqjUUpC8nfnsJZhRb zw=; b=uLUrR2Iwxx7WgXnhSjMJrG0+mm49ihQnKNpfXs1keST4AiCBFBBz0lhMW UT20TjAfAE8a4vs1ouhH/kjs2IX+uELKuvMXW+6UlvT04WG84QlZws+9r7QYP3F0 2aQTL3RgBc1e3cOXaXm/TG7cDZe0ak1zsGJ9mSACbIpSQ1dCXBdVk5onnIa94yxk yX3CU+2RzV5F1DZbnmvmBJPRpd7zGdegsm0aSEPuP9J2DJzl4jxATdp6LaWlVW7X PM5qUFZsf59o15ObaIG+O/MX0T2ff58i17x/ov4x7hvZADKm8I0CddUTK8ql2/9k tB0S++AL2ohJWgoSjGDoxywIdOFlw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfeeghedguddttdcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdej ueeiiedvffegheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 24 Jun 2021 12:18:45 -0400 (EDT) From: Thomas Monjalon To: Xueming Li Cc: dev@dpdk.org, Wang Haiyue , Kinsella Ray Date: Thu, 24 Jun 2021 18:18:43 +0200 Message-ID: <2020318.PlHIg8rldJ@thomas> In-Reply-To: <20210623000349.631468-2-xuemingl@nvidia.com> References: <20210613125846.19852-1-xuemingl@nvidia.com> <20210623000349.631468-2-xuemingl@nvidia.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v5 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" 23/06/2021 02:03, Xueming Li: > +static int > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *drv, > + struct rte_auxiliary_device *dev) [...] > + AUXILIARY_LOG(DEBUG, "Probe driver: %s", drv->driver.name); I think the debug log above is useless given the ones below. > + > + iova_mode = rte_eal_iova_mode(); > + if ((drv->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 && > + iova_mode != RTE_IOVA_VA) { > + AUXILIARY_LOG(ERR, "Expecting VA IOVA mode but current mode is PA, not initializing"); The error log should mention which driver is requesting VA and not initializing. > + return -EINVAL; > + } > + > + dev->driver = drv; > + > + AUXILIARY_LOG(INFO, "Probe auxiliary driver: %s device: %s (socket %i)", > + drv->driver.name, dev->name, dev->device.numa_node); > + ret = drv->probe(drv, dev); > + if (ret != 0) > + dev->driver = NULL; > + else > + dev->device.driver = &drv->driver; > + > + return ret; > +} > + > +/* > + * Call the remove() function of the driver. > + */ > +static int > +rte_auxiliary_driver_remove_dev(struct rte_auxiliary_device *dev) > +{ > + struct rte_auxiliary_driver *drv; > + int ret = 0; > + > + if (dev == NULL) > + return -EINVAL; > + > + drv = dev->driver; > + > + AUXILIARY_LOG(DEBUG, "Auxiliary device %s on NUMA socket %i", > + dev->name, dev->device.numa_node); > + > + AUXILIARY_LOG(DEBUG, "remove driver: %s %s", > + dev->name, drv->driver.name); Better to merge both logs together. > + > + if (drv->remove != NULL) { > + ret = drv->remove(dev); > + if (ret < 0) > + return ret; > + } > + > + /* clear driver structure */ > + dev->driver = NULL; > + dev->device.driver = NULL; > + > + return 0; > +} [...] > +static int > +auxiliary_parse(const char *name, void *addr) > +{ > + struct rte_auxiliary_driver *drv = NULL; > + const char **out = addr; > + > + /* Allow dummy name to prevent bus scan. */ > + if (strlen(name) == 0) > + return 0; Which syntax is it? [...] > + kvargs = rte_kvargs_parse(str, auxiliary_params_keys); > + if (kvargs == NULL) { > + RTE_LOG(ERR, EAL, "cannot parse argument list\n"); Should use AUXILIARY_LOG. [...] > +++ b/drivers/bus/auxiliary/linux/auxiliary.c > + /* Get numa node, default to 0 if not present */ s/numa/NUMA/ [...] > +/* > + * Scan the content of the auxiliary bus, and the devices in the devices > + * list > + */ Simpler: Scan the devices in the auxiliary bus. > +int > +auxiliary_scan(void) > +{ [...] > +++ b/drivers/bus/auxiliary/private.h > @@ -0,0 +1,75 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (c) 2021 NVIDIA Corporation & Affiliates > + */ > + > +#ifndef _AUXILIARY_PRIVATE_H_ > +#define _AUXILIARY_PRIVATE_H_ nit: we can avoid the first and final underscores [...] > +/* > + * Validate whether a device with given auxiliary device should be ignored > + * or not. > + */ > +bool auxiliary_ignore_device(const char *name); Suggestion: auxiliary_is_ignored_device() ? > +/* > + * Add an auxiliary device to the auxiliary bus (append to auxiliary Device s/Device/device/ > + * list). This function also updates the bus references of the auxiliary > + * Device (and the generic device object embedded within. s/(and/and/ > + */ > +void auxiliary_add_device(struct rte_auxiliary_device *aux_dev); > + > +/* > + * Insert an auxiliary device in the auxiliary bus at a particular location > + * in the device list. It also updates the auxiliary bus reference of the > + * new devices to be inserted. > + */ > +void auxiliary_insert_device(struct rte_auxiliary_device *exist_aux_dev, > + struct rte_auxiliary_device *new_aux_dev); > + > +/* > + * Match the auxiliary Driver and Device by driver function no need of capital letters in driver and device. > + */ > +bool auxiliary_match(const struct rte_auxiliary_driver *aux_drv, > + const struct rte_auxiliary_device *aux_dev); > + > +/* > + * Iterate over internal devices, matching any device against the provided What do you mean by "internal"? > + * string. > + */ > +void *auxiliary_dev_iterate(const void *start, const char *str, > + const struct rte_dev_iterator *it); > + [...] > +++ b/drivers/bus/auxiliary/rte_bus_auxiliary.h > +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. */ No capital in Probe and Remove > + rte_auxiliary_dma_map_t *dma_map; /**< Device dma map function. */ > + rte_auxiliary_dma_unmap_t *dma_unmap; /**< Device dma unmap function. */ s/dma/DMA/ There may be other occurences to check. > + uint32_t drv_flags; /**< Flags RTE_AUXILIARY_DRV_*. */ > +}; The alignment of some comments above can be improved by one space. [...] > +/** Helper for auxiliary device registration from driver instance */ > +#define RTE_PMD_REGISTER_AUXILIARY(nm, auxiliary_drv) \ > + RTE_INIT(auxiliaryinitfn_ ##nm) \ > + { \ > + (auxiliary_drv).driver.name = RTE_STR(nm); \ > + rte_auxiliary_register(&(auxiliary_drv)); \ > + } \ > + RTE_PMD_EXPORT_NAME(nm, __COUNTER__) I think it is better not trying to align backslashes in such macro, but leave only a single space. [...] > +++ b/drivers/bus/auxiliary/version.map > @@ -0,0 +1,3 @@ > +EXPERIMENTAL { > + # added in 21.08 > +}; Looks like you need to bring the symbols back :) Looks good overrall, thanks.