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 CDCE4A0546; Thu, 16 Jul 2020 18:57:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9130A1BED2; Thu, 16 Jul 2020 18:57:39 +0200 (CEST) Received: from wout1-smtp.messagingengine.com (wout1-smtp.messagingengine.com [64.147.123.24]) by dpdk.org (Postfix) with ESMTP id D77D61BEA3 for ; Thu, 16 Jul 2020 18:57:37 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailout.west.internal (Postfix) with ESMTP id 56C71976; Thu, 16 Jul 2020 12:57:36 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Thu, 16 Jul 2020 12:57:36 -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= GkLxXAGX1eExHBkHIERjw5m9Tf2+lQTYO7TyoW0QvCQ=; b=lcsrKAdaGrSnYfAw IcKDglawtaLyPS02jetEx5jYhEPndEWqVkAlsqtYANevkWrfApbbCgxGxBhaXv/W yHx30/geXeo30Nz25jv7Zc0Pw+gJkuNTyRpKek7Ofwlp3LxpWJjRt0QdrkwKmVww DtfY8EJf5rw+NzwgYoR36dfMNZOhDuG5Gb7ZzppH+mSY0YY6dE3sT9Sv8UIrGSUe snQIy0kALKN1mqw0ZLiM6jNt4Q5M3iBJKhYNkckAFvQS4lf625vxT2GsOeEPsZLJ 4/dXx7krnCmM0hb5nXsLhgaW7tapZj/e7NkwbkMXbR/MO+3tdOt8vpjPcjpOoFfE HFrRaA== 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=GkLxXAGX1eExHBkHIERjw5m9Tf2+lQTYO7TyoW0Qv CQ=; b=CXXSEu821/OUqJQ0azbByatKq+MiIY9e5zQ2Llc14P0YCrbY/1yWPoNyC L7z7jP7XR/JTom/yeX65MrhYCPAOhFmcq+dcmku9Mf0tmUQTzkxEbEdOuq9V7r1B MW2ZRMtIgGhfEGQGTKn5Kr0HwbOJJapqsdcDLwSBGJbRgSdUHupKWykIj/hDWNjY CVR3D5jER0B40yfHL5KoVBEX+0iVtkZ2+cOQleWHBGJHoPMmpkDMWHQlDWMrmn64 vS6AagTPCeiQ1K+AjWgeL1gMaMjGvEActXQfAhd4z555kuMuZAaokUqYU1+Cgap/ o4w8dcFiLPQiZC+Jty6pQdt8Jlscw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedrfeeggddutdekucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtqhertddttddunecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepfeffgeeitdefteffueeuffelueduvdelteeklefgvedukedvueeu geeikefhlefgnecuffhomhgrihhnpeguphgukhdrohhrghenucfkphepjeejrddufeegrd dvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhf rhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id C9F7B328005D; Thu, 16 Jul 2020 12:57:34 -0400 (EDT) From: Thomas Monjalon To: Jerin Jacob Cc: =?ISO-8859-1?Q?Ga=EBtan?= Rivet , Manish Chopra , Ferruh Yigit , Igor Russkikh , dpdk-dev Date: Thu, 16 Jul 2020 18:57:33 +0200 Message-ID: <33229780.9rCRSYNQsn@thomas> In-Reply-To: References: <20200713151319.17547-2-manishc@marvell.com> <72498440.BHRkrRebfW@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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" 16/07/2020 18:43, Jerin Jacob: > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon wro= te: > > > > 16/07/2020 15:02, Jerin Jacob: > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon = wrote: > > > > > > > > 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=EBtan Rivet wrote: > > > > > > > > > > > > > > > > On 16/07/20 12:08 +0200, Ga=EBtan 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,= when you must > > > > > > > > > use those defines in a generic implementation. Can you co= nfirm this is > > > > > > > > > the motivation here? > > > > > > > > > > > > > > > > > > If so, I think it would be clearer to state "in order to = avoid > > > > > > > > > dependency of DPDK on linux headers". > > > > > > > > > > > > > > > > > > > > > > > > > To add to it, if this is actually the motivation to add thi= s 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= _pci/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= =2Dnote */ > > > > > > > > > > +/* > > > > > > > > > > > > > > > > > > This file is delivered alongside the PCI lib, targeting u= serspace. > > > > > > > > > This seems to be an exception to the license policy descr= ibed 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 a= s BSD-3 as well > > > > > > > > > is not possible. > > > > > > > > > > > > > > > > > > So I think it might require a techboard + governing board= exception > > > > > > > > > approval. Ferruh or Thomas, what do you think? > > > > > > > > > > > > > > I think, instead of importing GPL-2.0 file, We can add the co= nstants > > > > > > > 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 DPD= K 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 >=20 > I just checked in archlinux, PCI headers can be provided by >=20 > # pacman -F /usr/include/pci/header.h > usr/include/pci/header.h is owned by core/pciutils 3.7.0- >=20 > # pacman -F /usr/include/linux/pci.h > usr/include/linux/pci.h is owned by core/linux-api-headers 5.4.17-1 >=20 >=20 > > I'm missing the real justification for this patch. >=20 > See below. >=20 > > 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. >=20 > First version of qede driver sent with defined generic PCI symbols and > generic PCI function like pci_find_next_ext_capability() in qede driver. That's a pity the v2 is not threaded with v1, I would have found these explanations easily myself. > 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/ I agree it's good to have an API for such thing. So far such feature is supported in drivers on Linux, requiring only Linux headers to be installed. Do we need more?