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 B593AA0546; Thu, 16 Jul 2020 18:43:35 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7CAB91BEA3; Thu, 16 Jul 2020 18:43:35 +0200 (CEST) Received: from mail-io1-f68.google.com (mail-io1-f68.google.com [209.85.166.68]) by dpdk.org (Postfix) with ESMTP id A9E8F2C38 for ; Thu, 16 Jul 2020 18:43:34 +0200 (CEST) Received: by mail-io1-f68.google.com with SMTP id q74so6741116iod.1 for ; Thu, 16 Jul 2020 09:43:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=UuO5dcUD6C1C/yY1EtEgUz6UFiPvrRLdK9DB8JKtas4=; b=cdXToZW7u/aHcSchs3qL0PUCBvRqB0ldjyAnvHYWzqHkpbUD2psvvh40UyU2MVN13G dHYkZdg0+mo6qVLM7jWw/205xffjdZsjZSijLlMZwERC+v77a2PWnfTMjmdoEUUWpxQW A5voTxOUzPhnOmb2k28oSgvwGFatGtUAzFtMX8z4bFEkFWIHHtfxGwow94dL2W2ua9+0 cmur2O/LFRpfBlQrNwDma3D+1yxKNIgGPEvAsjTeiUl5U6qGKI2y46boTnwwjXAWuucm KUMl4pqORgf58F+9TqkuOjNexTziAen1lhcYi94O5MfjcFPJq7lKWVOjtn5inqd+Hf2t 8DQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=UuO5dcUD6C1C/yY1EtEgUz6UFiPvrRLdK9DB8JKtas4=; b=qlFx4iDAaLUiTif0eiFUqG9P1bqPHBi/TlNtnyrvI8mZNp5YYzvM45awFjTnU3S/2c gvyyPleTUAEZx5b1KgQejL83KRfkZPsF9XvQtT9fSWkVDtHwb3Qoa8n9AYJOKkSztwxF pL4lcQNFhJ0Onyaarajs+SfzX0orHvUs9j5FRWrJePZuGLE2n6l/sGO8tqcXfaSRZSlg WthvGvpzZSX2ZtsPXZgawK60HtotsGrw32Q2dKTBzl2iG9bEeNL98B4gWqFXxDyaVXuY fYUEZeLjbCU7b2Xp7v8KTGnGM4squRlClQCGpIwTH2wliJxEnFeIs+P1QOJ9K8+ZsT92 kiKg== X-Gm-Message-State: AOAM530FQd6AYVTPUh7jfhhplUOBieZ3VD7DAS5B7Hlhm5pmFQPGdKgC KPC75uSXftthva6euByfMaz25YCuYYVVku011vA= X-Google-Smtp-Source: ABdhPJzAnVpdju+wbRSp7BwY1KSzVQkszoC8SxwN+8NvW9jS6XYvqBJl5pf7pxjt7VOS6KrykU5AbUq+CVAzwbQK+ZE= X-Received: by 2002:a5e:8b44:: with SMTP id z4mr5524650iom.123.1594917813826; Thu, 16 Jul 2020 09:43:33 -0700 (PDT) MIME-Version: 1.0 References: <20200713151319.17547-2-manishc@marvell.com> <1914663.Rf5fFZbPsG@thomas> <72498440.BHRkrRebfW@thomas> In-Reply-To: <72498440.BHRkrRebfW@thomas> From: Jerin Jacob Date: Thu, 16 Jul 2020 22:13:17 +0530 Message-ID: To: Thomas Monjalon Cc: =?UTF-8?Q?Ga=C3=ABtan_Rivet?= , Manish Chopra , Ferruh Yigit , Igor Russkikh , dpdk-dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h 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 Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon wrote= : > > 16/07/2020 15:02, Jerin Jacob: > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon w= rote: > > > > > > 16/07/2020 13:55, Jerin Jacob: > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon wrote: > > > > > > > > > > 16/07/2020 12:27, Jerin Jacob: > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Ga=C3=ABtan Rivet wrote: > > > > > > > > > > > > > > On 16/07/20 12:08 +0200, Ga=C3=ABtan Rivet wrote: > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply. > > > > > > > > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote: > > > > > > > > > This is merely copy of latest linux/pci_regs.h in > > > > > > > > > order to avoid dependency of dpdk on user headers. > > > > > > > > > > > > > > > > > > > > > > > > > I guess this dependency is an issue on non-linux systems, w= hen you must > > > > > > > > use those defines in a generic implementation. Can you conf= irm this is > > > > > > > > the motivation here? > > > > > > > > > > > > > > > > If so, I think it would be clearer to state "in order to av= oid > > > > > > > > dependency of DPDK on linux headers". > > > > > > > > > > > > > > > > > > > > > > To add to it, if this is actually the motivation to add this = header, I > > > > > > > don't think it is sufficient. > > > > > > > > > > > > > > You can restrict the function definition to the linux part of= the > > > > > > > PCI bus driver instead, using stubs for other systems. > > > > > > > > > > > > > > > > Signed-off-by: Manish Chopra > > > > > > > > > Signed-off-by: Igor Russkikh > > > > > > > > > --- > > > > > > > > > drivers/bus/pci/linux/pci_uio.c | 2 +- > > > > > > > > > drivers/bus/pci/linux/pci_vfio.c | 2 +- > > > > > > > > > drivers/net/bnx2x/bnx2x.h | 2 +- > > > > > > > > > drivers/net/hns3/hns3_ethdev_vf.c | 2 +- > > > > > > > > > drivers/vdpa/ifc/base/ifcvf_osdep.h | 2 +- > > > > > > > > > lib/librte_pci/Makefile | 1 + > > > > > > > > > lib/librte_pci/meson.build | 2 +- > > > > > > > > > lib/librte_pci/rte_pci_regs.h | 1075 +++++++++++++= ++++++++++++++ > > > > > > > > > 8 files changed, 1082 insertions(+), 6 deletions(-) > > > > > > > > > create mode 100644 lib/librte_pci/rte_pci_regs.h > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_p= ci/rte_pci_regs.h > > > > > > > > > new file mode 100644 > > > > > > > > > index 000000000..1d11f4de5 > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h > > > > > > > > > @@ -0,0 +1,1075 @@ > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-n= ote */ > > > > > > > > > +/* > > > > > > > > > > > > > > > > This file is delivered alongside the PCI lib, targeting use= rspace. > > > > > > > > This seems to be an exception to the license policy describ= ed in > > > > > > > > license/README. Code shared between kernel and userspace is= expected > > > > > > > > to be dual-licensed BSD-3 and GPL-2.0. > > > > > > > > > > > > > > > > As it is a copy of Linux user includes, re-licensing it as = BSD-3 as well > > > > > > > > is not possible. > > > > > > > > > > > > > > > > So I think it might require a techboard + governing board e= xception > > > > > > > > approval. Ferruh or Thomas, what do you think? > > > > > > > > > > > > I think, instead of importing GPL-2.0 file, We can add the cons= tants > > > > > > as need by the DPDK > > > > > > as symbols start from RTE_PCI_*(It will fix up the namespace as= well). > > > > > > > > > > If symbols can be found in /usr/include/, don't add anything. > > > > > > > > Not by default on all the distros. It is part of pciutils library. > > > > Moreover, we need these symbols for Windows OS as well. > > > > IMO, We should add absolute minimum constants that needed for DPDK = as RTE_PCI_* > > > > > > I am for mandating the dependency instead of copying it. > > > > You mean _pciutils_ package as a mandatory dependency to DPDK. > > There is already this dependency: > #include I just checked in archlinux, PCI headers can be provided by # pacman -F /usr/include/pci/header.h usr/include/pci/header.h is owned by core/pciutils 3.7.0- # pacman -F /usr/include/linux/pci.h usr/include/linux/pci.h is owned by core/linux-api-headers 5.4.17-1 > I'm missing the real justification for this patch. See below. > Is there some missing definitions? > Is there some environments where this file is missing? > > > > pciutils cannot be installed on Windows? > > > Why do you care about Windows? > > > I don't see any contribution for qede on Windows. > > > > You closely review the patch, it not about qede. The proposed file > > comes at lib/librte_pci/rte_pci_regs.h which is common to Windows. > > The series is for qede. I'm trying to understand the motivation. First version of qede driver sent with defined generic PCI symbols and generic PCI function like pci_find_next_ext_capability() in qede driver. In the review, I suggested using generic rte_ function as a) It is not specific to qede. b) Other drivers also doing the same thing in their own driver space as there is no dpdk API for the same. This patches create generic API for pci_find_next_ext_capability() and remove duplicate implementation from the drivers. http://patches.dpdk.org/patch/73959/ > >