From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so2.wedos.net (wes1-so2.wedos.net [46.28.106.16]) by dpdk.org (Postfix) with ESMTP id 1BE16F94 for ; Thu, 14 Jul 2016 17:57:01 +0200 (CEST) Received: from jvn (dynamic-109-81-211-32.ipv4.broadband.iol.cz [109.81.211.32]) by wes1-so2.wedos.net (Postfix) with ESMTPSA id 3rr0gw4RfVz8nX; Thu, 14 Jul 2016 17:57:00 +0200 (CEST) Date: Thu, 14 Jul 2016 17:57:07 +0200 From: Jan Viktorin To: Shreyansh jain Cc: , , Message-ID: <20160714175707.43755987@jvn> In-Reply-To: <578722DB.3030900@nxp.com> References: <1466510566-9240-1-git-send-email-shreyansh.jain@nxp.com> <1468303282-2806-1-git-send-email-shreyansh.jain@nxp.com> <1468303282-2806-6-git-send-email-shreyansh.jain@nxp.com> <20160713112043.2fd847ca@jvn> <20160713193430.65b6df9c@jvn> <578722DB.3030900@nxp.com> Organization: RehiveTech X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 05/17] eal: introduce init macros X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jul 2016 15:57:01 -0000 On Thu, 14 Jul 2016 10:57:55 +0530 Shreyansh jain wrote: > Hi Jan, > > On Wednesday 13 July 2016 11:04 PM, Jan Viktorin wrote: > > On Wed, 13 Jul 2016 11:20:43 +0200 > > Jan Viktorin wrote: > > > >> Hello Shreyansh, > >> > >> On Tue, 12 Jul 2016 11:31:10 +0530 > >> Shreyansh Jain wrote: > >> > >>> Introduce a RTE_INIT macro used to mark an init function as a constructor. > >>> Current eal macros have been converted to use this (no functional impact). > >>> DRIVER_REGISTER_PCI is added as a helper for pci drivers. > >>> > >>> Suggested-by: Jan Viktorin > >>> Signed-off-by: David Marchand > >>> Signed-off-by: Shreyansh Jain > >>> --- > >> > >> [...] > >> > >>> +#define RTE_INIT(func) \ > >>> +static void __attribute__((constructor, used)) func(void) > >>> + > >>> #ifdef __cplusplus > >>> } > >>> #endif > >>> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h > >>> index fa74962..3027adf 100644 > >>> --- a/lib/librte_eal/common/include/rte_pci.h > >>> +++ b/lib/librte_eal/common/include/rte_pci.h > >>> @@ -470,6 +470,14 @@ void rte_eal_pci_dump(FILE *f); > >>> */ > >>> void rte_eal_pci_register(struct rte_pci_driver *driver); > >>> > >>> +/** Helper for PCI device registeration from driver (eth, crypto) instance */ > >>> +#define DRIVER_REGISTER_PCI(nm, drv) \ > >>> +RTE_INIT(pciinitfn_ ##nm); \ > >>> +static void pciinitfn_ ##nm(void) \ > >>> +{ \ > >> > >> You are missing setting the name here like PMD_REGISTER_DRIVER does > >> now. Or should I include it in my patch set? > >> > >> (drv).name = RTE_STR(nm); > > That is a miss from my side. > I will publish v7 with this. You want this right away or should I wait a little while (more reviews, or any pending additions as per Thomas's notes) before publishing? Please. The time is almost gone. 18/7/2016 is the release (according to the roadmap)... I have to fix it in my patchset, otherwise it does not build (after moving the .name from rte_pci_driver to rte_driver). > > > > > Moreover, it should accept the rte_pci_driver *, shouldn't it? Here, it > > expects a wrapper around it (eth_driver)... I now, my SoC patches were > > supposing the some... but I think it is wrong. > > > > The original David's patch set contains calls like this: > > > > RTE_EAL_PCI_REGISTER(bnx2xvf, rte_bnx2xvf_pmd.pci_drv); > > > > So, I think, we should go the original way. > > I have a slightly different opinion of the above. > IMO, aim of the helpers is to hide the PCI details and continue to make driver consider itself as a generic ETH driver. In that case, dereferencing pci_drv would be done by macro. In this case, I'd prefer to see DRIVER_REGISTER_PCI_ETH. At first, this was also my way of thinking. But I've changed my mind. I find it to be a bit overdesigned. > > Also, considering that in future pci_drv would also have soc_drv, the helpers can effectively hide the intra-structure naming of these. It would help when more such device types (would there be?) are introduced - in which case, driver framework has a consistent coding convention. Hide? I am afraid, I don't understand clearly what you mean. > > But, I am ok switching back to David's way as well - I don't have any strong argument against that. I'd like to preserve the clear semantics. That is DRIVER_REGISTER_PCI -> give a pci device. Has anybody a different opinion? David? Thomas? > > > > > Jan > > > >> > >>> + rte_eal_pci_register(&drv.pci_drv); \ > >>> +} > >>> + > >>> /** > >>> * Unregister a PCI driver. > >>> * > [...] > > - > Shreyansh > -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic