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 EDFE3A034F; Fri, 8 Oct 2021 12:33:54 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 90F1F41109; Fri, 8 Oct 2021 12:33:54 +0200 (CEST) Received: from new4-smtp.messagingengine.com (new4-smtp.messagingengine.com [66.111.4.230]) by mails.dpdk.org (Postfix) with ESMTP id CAD6641104 for ; Fri, 8 Oct 2021 12:33:53 +0200 (CEST) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 59453580FB2; Fri, 8 Oct 2021 06:33:53 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Fri, 08 Oct 2021 06:33:53 -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=fm2; bh= OxR5p+U8paomcvNtVmpUnmQQgsIeHsJxJaXBVLYLBzk=; b=uCEL+oV+cq8ZnvOS TzPe/m5Em7usC63v9An2M3km2QE3p1iMEbZb+ckuabZI8Y2yCfTbtGi9y6FAr0N0 FAipgUKorTv/goNbZa11I9pyIWhbEwfvwiaDB21IsEfmb6yKuTRTG7ajogoRtiH0 vpAxn57KGeCkcxBKHiALG4w8/NawU5sG6TanukFdqOZcWDOE24D6DyGZysJxT2Fr UAhOhW5fg3vaTSGlcai7IK6ilkmCewyUgVxpkYAvVlvdVbBK4DedNxgfis5sFq6K d5zf7oki5gd/KXdpoj9fJsC28XwU6el5/mNd9g7GBwPPWTa/JALEwSvkSxovxPi6 ghf2YQ== 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=fm1; bh=OxR5p+U8paomcvNtVmpUnmQQgsIeHsJxJaXBVLYLB zk=; b=QXBVNYwCP7/61ntiWHmM6j983kkbAGCo0awbb3grMgOky49kJRSr2LGSm bQc6P8NG2nMQRizwdaq+sxgQpNLTsiW+cix7lIkjXGs5XSh8S4lO6mxOW/LcZTlf 1rTsK1eGZlUhR6daS1dhjbvNdDEBdsUpu3S9fXijMv0r9MJZOIToOhyaRClONZjT dx/faEmH5MSgnfdrU7EfrDrVVYRSNu0BOHhnW6jgE0Z7E52/zqBoqSHGee7O0YLp 5wGi3OuJDHmyyFlv7FcLCqpamuuCtoh1cHS5micaT4ispzPxa97s0FZ3mty15s3j NWCJ1nmnpEnv+6o3rlEGDzrZe2tow== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvddttddgvdejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkjghfggfgtgesthfure dttddtvdenucfhrhhomhepvfhhohhmrghsucfoohhnjhgrlhhonhcuoehthhhomhgrshes mhhonhhjrghlohhnrdhnvghtqeenucggtffrrghtthgvrhhnpeffvdffjeeuteelfeeile duudeugfetjeelveefkeejfeeigeehteffvdekfeegudenucffohhmrghinhepughpughk rdhorhhgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomh epthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 8 Oct 2021 06:33:49 -0400 (EDT) From: Thomas Monjalon To: fengchengwen Cc: ferruh.yigit@intel.com, bruce.richardson@intel.com, jerinj@marvell.com, jerinjacobk@gmail.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org, mb@smartsharesystems.com, nipun.gupta@nxp.com, hemant.agrawal@nxp.com, maxime.coquelin@redhat.com, honnappa.nagarahalli@arm.com, david.marchand@redhat.com, sburla@marvell.com, pkapoor@marvell.com, konstantin.ananyev@intel.com, conor.walsh@intel.com, kevin.laatz@intel.com Date: Fri, 08 Oct 2021 12:18:55 +0200 Message-ID: <2689219.tMN6Yd1t8y@thomas> In-Reply-To: References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <11425429.3DvE4F450j@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v23 2/6] dmadev: add control plane function support 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" 08/10/2021 09:55, fengchengwen: > On 2021/10/6 18:46, Thomas Monjalon wrote: > > 24/09/2021 12:53, Chengwen Feng: > >> --- a/lib/dmadev/rte_dmadev.c > >> +++ b/lib/dmadev/rte_dmadev.c > >> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name) > >> if (dev == NULL) > >> return -EINVAL; > >> > >> + if (dev->state == RTE_DMA_DEV_READY) > >> + return rte_dma_close(dev->dev_id); > > > > What is the logic here? > > The only exposed function should be rte_dma_close() > > and it should call the freeing function. > > The API should use the dev_id. As you said somewhere else, > > the name is only for debugging. > > Please remove the function rte_dma_pmd_release(const char *name). > > The rte_dma_pmd_release corresponding to pmd_allocate, so both use the 'const char *name' parameter. > > The rte_dma_pmd_release is also used for error handling when PMD init. Therefore, a status variable > is used here. If the device is not ready, only resources need to be released. Otherwise, the close > interface of the driver is invoked. > > For PMD, the rte_dma_pmd_release is only wrap for dev_close when remove device, it does not need to > implement two callbacks. > > If we replace rte_dma_pmd_release with rte_dma_close, then we should invoke rte_dma_close in error > handling when PMD init, this can lead to conceptual inconsistencies because the initialization has > not been successful. > > So I think it's better keep rte_dma_pmd_release. I will review again this logic in the next version. > >> +/** DMA device support memory-to-memory transfer. > >> + * > >> + * @see struct rte_dma_info::dev_capa > >> + */ > >> +#define RTE_DMA_CAPA_MEM_TO_MEM RTE_BIT64(0) > >> +/** DMA device support memory-to-device transfer. > >> + * > >> + * @see struct rte_dma_info::dev_capa > >> + */ > >> +#define RTE_DMA_CAPA_MEM_TO_DEV RTE_BIT64(1) > > > > Same comment as in earlier version: please group the flags > > in a doxygen group. Example of doxygen group: > > https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/ > > Tried, but found it didn't coexist well with multi-line comments. What is not working? Example? I think you didn't get what to do. You must add a comment to give a title and group all these flags. > >> --- a/lib/dmadev/rte_dmadev_core.h > >> +++ b/lib/dmadev/rte_dmadev_core.h > >> +/** @internal Used to get device information of a device. */ > >> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev, > >> + struct rte_dma_info *dev_info, > >> + uint32_t info_sz); > > > > Please move all driver interfaces in a file dedicated to drivers. > > There are three head file: rte_dmadev.h, rte_dmadev_core.h, rte_dmadev_pmd.h > And we build the following dependency: > > rte_dmadev.h ---> rte_dmadev_core.h // mainly because dataplane inline API. > ^ > | > --------------------- > | | > Application rte_dmadev_pmd.h > ^ > | > DMA PMD > > > If move all driver interfaces to rte_dmadev_pmd.h from rte_dmadev_core.h, bidirectional > dependency may exist, e.g. > > rte_dmadev.h ---> rte_dmadev_core.h ---> rte_dmadev_pmd.h > ^ > | > --------------------- > | | > Application rte_dmadev_pmd.h > ^ > | > DMA PMD > > So I think it's better keep it that way. Please make sure only what is needed for inline is kept in the "core.h" You should look at the current effort done by Konstanti in ethdev to hide everything.