Re: [dpdk-dev] [PATCH v6 05/17] eal: introduce init macros

[Jan Viktorin <viktorin@rehivetech.com>]

On Thu, 14 Jul 2016 10:57:55 +0530
Shreyansh jain <shreyansh.jain@nxp.com> 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 <viktorin@rehivetech.com> wrote:
> >   
> >> Hello Shreyansh,
> >>
> >> On Tue, 12 Jul 2016 11:31:10 +0530
> >> Shreyansh Jain <shreyansh.jain@nxp.com> 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 <viktorin@rehivetech.com>
> >>> Signed-off-by: David Marchand <david.marchand@6wind.com>
> >>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> >>> ---    
> >>
> >> [...]
> >>  
> >>> +#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