From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <grive@u256.net>
To: Parav Pandit <parav@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
Ori Kam <orika@mellanox.com>, Matan Azrad <matan@mellanox.com>
Message-ID: <20200618143509.m45qzhzv3je4rqhs@u256.net>
References: <20200610171728.89-1-parav@mellanox.com>
<20200610171728.89-6-parav@mellanox.com>
<20200615214631.lmubncrd6xggq3hu@u256.net>
<AM0PR05MB48661BC6BA1EA87AE273B79BD19B0@AM0PR05MB4866.eurprd05.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <AM0PR05MB48661BC6BA1EA87AE273B79BD19B0@AM0PR05MB4866.eurprd05.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On 18/06/20 10:03 +0000, Parav Pandit wrote:
>
> > From: Gaëtan Rivet <grive@u256.net>
> > 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 <parav@mellanox.com>
> > > ---
> > > 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