From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1FE9FA04A5; Thu, 18 Jun 2020 16:35:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 796C81BF5C; Thu, 18 Jun 2020 16:35:17 +0200 (CEST) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by dpdk.org (Postfix) with ESMTP id 0CD991BF5B for ; Thu, 18 Jun 2020 16:35:16 +0200 (CEST) X-Originating-IP: 86.246.31.132 Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 0310F40013; Thu, 18 Jun 2020 14:35:14 +0000 (UTC) Date: Thu, 18 Jun 2020 16:35:09 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Parav Pandit Cc: "dev@dpdk.org" , "ferruh.yigit@intel.com" , Ori Kam , Matan Azrad Message-ID: <20200618143509.m45qzhzv3je4rqhs@u256.net> References: <20200610171728.89-1-parav@mellanox.com> <20200610171728.89-6-parav@mellanox.com> <20200615214631.lmubncrd6xggq3hu@u256.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [dpdk-dev] [RFC PATCH 5/6] bus/mlx5_pci: register a PCI driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" On 18/06/20 10:03 +0000, Parav Pandit wrote: > > > From: Gaëtan Rivet > > Sent: Tuesday, June 16, 2020 3:17 AM > > > > On 10/06/20 17:17 +0000, Parav Pandit wrote: > > > Create a mlx5 bus driver framework for invoking drivers of multiple > > > classes who have registered with the mlx5_pci bus driver. > > > > > > Validate user class arguments for supported class combinations. > > > > > > Signed-off-by: Parav Pandit > > > --- > > > drivers/bus/mlx5_pci/Makefile | 1 + > > > drivers/bus/mlx5_pci/meson.build | 2 +- > > > drivers/bus/mlx5_pci/mlx5_pci_bus.c | 253 > > ++++++++++++++++++++++++ > > > drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h | 1 + > > > 4 files changed, 256 insertions(+), 1 deletion(-) > > > [...] > > > + > > > + while (nstr) { > > > + /* Extract each individual class name */ > > > + found = strsep(&nstr, ":"); > > > > I have not seen the feature test macros (_DEFAULT_SOURCE) in the > > Makefile, it seems required for strsep()? > > > If its mandatory meson build should have complained? > Invoking the compiler without specific standard conformance will work. If someone adds for example -std=c11 however then _DEFAULT_SOURCE becomes necessary. It all depends on the range of compiler versions targeted by this code. I don't know the full coverage, but I see -std=c11 + -D_DEFAULT_SOURCE in most mlx5 code, which is why I'm asking for a double check here. [...] > > > + continue; > > > + > > > + if (class->loaded) > > > + class->remove(dev); > > > + } > > > + return 0; > > > +} > > > + > > > +static int > > > +mlx5_bus_pci_dma_map(struct rte_pci_device *dev, void *addr, > > > + uint64_t iova, size_t len) > > > +{ > > > + struct rte_mlx5_pci_driver *class; > > > + int ret = -EINVAL; > > > + > > > + TAILQ_FOREACH(class, &drv_list, next) { > > > + if (!class->dma_map) > > > + continue; > > > + > > > + return class->dma_map(dev, addr, iova, len); > > > > Is there a specific class that could have priority for the DMA? > > > No. > The code being written this way seems to point to multiple classes being able to have DMA ops. If that's not the case, you can add a sanity check to enforce than only the right classes have DMA ops defined. > > > + } > > > + return ret; > > > +} > > > + > > > +static int > > > +mlx5_bus_pci_dma_unmap(struct rte_pci_device *dev, void *addr, > > > + uint64_t iova, size_t len) > > > +{ > > > + struct rte_mlx5_pci_driver *class; > > > + int ret = -EINVAL; > > > + > > > + TAILQ_FOREACH_REVERSE(class, &drv_list, mlx5_pci_bus_drv_head, > > next) { > > > + if (!class->dma_unmap) > > > + continue; > > > + > > > + return class->dma_unmap(dev, addr, iova, len); > > > > If you have two classes A -> B having dma_map() + dma_unmap(), you will > > dma_map() with A then dma_unmap() with B, due to the _REVERSE() > > iteration? > > > There isn't plan for two drivers to do so. > If two classes do that its source of an error. > Will enhance the bus when that need arise. > You have well-defined edge-cases, but they are not apparent reading the code. Such error could be warned about and / or documented. > > Why use reversed iteration at all by the way for dinit? If your ops is sound > > any order should be ok. > > > Because deinit must be always reverse of init() code regardless. > This is a strong statement :) If this is a requirement for driver inter-dependencies to be properly implemented, this should be documented as such. Probably also explained in the high-level design documentation in the header exposing this driver API. Best, -- Gaëtan