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

Message ID 1468303282-2806-6-git-send-email-shreyansh.jain@nxp.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Shreyansh Jain July 12, 2016, 6:01 a.m. UTC
  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>
---
 lib/librte_eal/common/include/rte_dev.h   | 4 ++--
 lib/librte_eal/common/include/rte_eal.h   | 3 +++
 lib/librte_eal/common/include/rte_pci.h   | 8 ++++++++
 lib/librte_eal/common/include/rte_tailq.h | 4 ++--
 4 files changed, 15 insertions(+), 4 deletions(-)
  

Comments

Jan Viktorin July 13, 2016, 9:20 a.m. UTC | #1
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);

> +	rte_eal_pci_register(&drv.pci_drv); \
> +}
> +
>  /**
>   * Unregister a PCI driver.
>   *
> diff --git a/lib/librte_eal/common/include/rte_tailq.h b/lib/librte_eal/common/include/rte_tailq.h
> index 4a686e6..71ed3bb 100644
> --- a/lib/librte_eal/common/include/rte_tailq.h
> +++ b/lib/librte_eal/common/include/rte_tailq.h
> @@ -148,8 +148,8 @@ struct rte_tailq_head *rte_eal_tailq_lookup(const char *name);
>  int rte_eal_tailq_register(struct rte_tailq_elem *t);
>  
>  #define EAL_REGISTER_TAILQ(t) \
> -void tailqinitfn_ ##t(void); \
> -void __attribute__((constructor, used)) tailqinitfn_ ##t(void) \
> +RTE_INIT(tailqinitfn_ ##t); \
> +static void tailqinitfn_ ##t(void) \
>  { \
>  	if (rte_eal_tailq_register(&t) < 0) \
>  		rte_panic("Cannot initialize tailq: %s\n", t.name); \
  
Jan Viktorin July 13, 2016, 5:34 p.m. UTC | #2
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);

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.

Jan

> 
> > +	rte_eal_pci_register(&drv.pci_drv); \
> > +}
> > +
> >  /**
> >   * Unregister a PCI driver.
> >   *
> > diff --git a/lib/librte_eal/common/include/rte_tailq.h b/lib/librte_eal/common/include/rte_tailq.h
> > index 4a686e6..71ed3bb 100644
> > --- a/lib/librte_eal/common/include/rte_tailq.h
> > +++ b/lib/librte_eal/common/include/rte_tailq.h
> > @@ -148,8 +148,8 @@ struct rte_tailq_head *rte_eal_tailq_lookup(const char *name);
> >  int rte_eal_tailq_register(struct rte_tailq_elem *t);
> >  
> >  #define EAL_REGISTER_TAILQ(t) \
> > -void tailqinitfn_ ##t(void); \
> > -void __attribute__((constructor, used)) tailqinitfn_ ##t(void) \
> > +RTE_INIT(tailqinitfn_ ##t); \
> > +static void tailqinitfn_ ##t(void) \
> >  { \
> >  	if (rte_eal_tailq_register(&t) < 0) \
> >  		rte_panic("Cannot initialize tailq: %s\n", t.name); \  
> 
> 
>
  
Shreyansh Jain July 14, 2016, 5:27 a.m. UTC | #3
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?

> 
> 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.

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.

But, I am ok switching back to David's way as well - I don't have any strong argument against that.

> 
> Jan
> 
>>
>>> +	rte_eal_pci_register(&drv.pci_drv); \
>>> +}
>>> +
>>>  /**
>>>   * Unregister a PCI driver.
>>>   *
[...]

-
Shreyansh
  
Jan Viktorin July 14, 2016, 3:57 p.m. UTC | #4
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
>
  
Shreyansh Jain July 15, 2016, 10:48 a.m. UTC | #5
On Thursday 14 July 2016 09:27 PM, Jan Viktorin wrote:
> 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).
> 

I didn't consider 18/Jul.
Please go ahead. I will continue to send v7 _without_ the above change so that your patchset doesn't break. This way you will not get blocked because of me.

>>
>>>
>>> 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.

There is:

DRIVER_REGISTER_PCI(...)
DRIVER_REGISTER_PCI_TABLE(...)

Wouldn't DRIVER_REGISTER_PCI_ETH look out-of-place?

> 
>>
>> 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.

DRIVER_REGISTER_PCI(eth_driver)
DRIVER_REGISTER_SOC(eth_driver)
DRIVER_REGISTER_XXX(eth_driver)
...

In either case, the caller always creates the eth_driver and populates internal specific driver structure (pci_drv) as a sub-part of eth_driver specification. Macro 'hides' the internal structure name (pci_drv, soc_drv...).
But again, nothing critical. Just a way of usage. We might not even have a 'XXX' in near future.

> 
>>
>> 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?

Yes please.
Or else, if nothing comes up soon, I will simply go ahead and change to DRIVER_REGISTER_PCI(eth_driver.pci_drv) as this trivial issue shouldn't hold back this series.

> 
>>
>>>
>>> Jan
>>>   
>>>>  
>>>>> +	rte_eal_pci_register(&drv.pci_drv); \
>>>>> +}
>>>>> +
>>>>>  /**
>>>>>   * Unregister a PCI driver.
>>>>>   *  
>> [...]
>>
>> -
>> Shreyansh
>>
> 
> 
>
  
Thomas Monjalon July 15, 2016, 12:38 p.m. UTC | #6
Hi guys,

2016-07-15 16:18, Shreyansh jain:
> On Thursday 14 July 2016 09:27 PM, Jan Viktorin wrote:
> > 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).
> 
> I didn't consider 18/Jul.
> Please go ahead. I will continue to send v7 _without_ the above change
> so that your patchset doesn't break. This way you will not get blocked
> because of me.
[...]
> > I'd like to preserve the clear semantics. That is DRIVER_REGISTER_PCI
> > -> give a pci device.
> > 
> > Has anybody a different opinion? David? Thomas?
> 
> Yes please.
> Or else, if nothing comes up soon, I will simply go ahead and change to
> DRIVER_REGISTER_PCI(eth_driver.pci_drv) as this trivial issue shouldn't
> hold back this series.

I'm sorry I have no time to review these series shortly.

I think it is too late to consider an integration in 16.07 unfortunately.
I feel you are doing a great job and we can target to have these changes
in the early days of 16.11 (in August).
As this is a big refactoring, it is probably a good idea to integrate it
in the beginning of the next release integration cycle.
  
Shreyansh Jain July 28, 2016, 9:36 a.m. UTC | #7
Hi Jan,

On Friday 15 July 2016 04:18 PM, Shreyansh jain wrote:
> On Thursday 14 July 2016 09:27 PM, Jan Viktorin wrote:
>> 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).
>>
> 
> I didn't consider 18/Jul.
> Please go ahead. I will continue to send v7 _without_ the above change so that your patchset doesn't break. This way you will not get blocked because of me.
> 

Now that we have already skipped the 16.07, I will fix this in my code and release an updated version as soon as 16.07 is officially available. Is that OK?

Also, I have fixed most review comments except [1]. I am still not sure of the impact of this change. So, I will publish the v7 without this and then probably a v8 in case this change has any impact. That way we can have your patchset not blocked because of this series.

[1] http://dpdk.org/ml/archives/dev/2016-July/044004.html

[...]


-
Shreyansh
  
Jan Viktorin July 30, 2016, 8:14 a.m. UTC | #8
On Thu, 28 Jul 2016 15:06:10 +0530
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> Hi Jan,
> 
> On Friday 15 July 2016 04:18 PM, Shreyansh jain wrote:
> > On Thursday 14 July 2016 09:27 PM, Jan Viktorin wrote:  

[...]

> >>>>> 	(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).
> >>  
> > 
> > I didn't consider 18/Jul.
> > Please go ahead. I will continue to send v7 _without_ the above change so that your patchset doesn't break. This way you will not get blocked because of me.
> >   
> 
> Now that we have already skipped the 16.07, I will fix this in my code and release an updated version as soon as 16.07 is officially available. Is that OK?

Sure :). The thing is that during August-September, I've got significantly
less time for this then in June-July :/. That's why I was trying to fit in
the 16.07.

> 
> Also, I have fixed most review comments except [1]. I am still not sure of the impact of this change. So, I will publish the v7 without this and then probably a v8 in case this change has any impact.

I have no idea about this. Hopefully, it's not a very serious thing.

> That way we can have your patchset not blocked because of this series.

Thank you!

> 
> [1] http://dpdk.org/ml/archives/dev/2016-July/044004.html

> 
> [...]
> 
> 
> -
> Shreyansh
  
Shreyansh Jain Aug. 1, 2016, 11 a.m. UTC | #9
Hi Jan,

On Saturday 30 July 2016 01:44 PM, Jan Viktorin wrote:
> On Thu, 28 Jul 2016 15:06:10 +0530
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
>> Hi Jan,
>>
>> On Friday 15 July 2016 04:18 PM, Shreyansh jain wrote:
>>> On Thursday 14 July 2016 09:27 PM, Jan Viktorin wrote:  
> 
> [...]
> 
>>>>>>> 	(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).
>>>>  
>>>
>>> I didn't consider 18/Jul.
>>> Please go ahead. I will continue to send v7 _without_ the above change so that your patchset doesn't break. This way you will not get blocked because of me.
>>>   
>>
>> Now that we have already skipped the 16.07, I will fix this in my code and release an updated version as soon as 16.07 is officially available. Is that OK?
> 
> Sure :). The thing is that during August-September, I've got significantly
> less time for this then in June-July :/. That's why I was trying to fit in
> the 16.07.

Well, then lets try and get things out for review as soon as we can.
I have sent across v7 of rte_driver set.
Unfortunately, I forgot to add your sign-off. If the set works for you, can you bulk ack?

> 
>>
>> Also, I have fixed most review comments except [1]. I am still not sure of the impact of this change. So, I will publish the v7 without this and then probably a v8 in case this change has any impact.
> 
> I have no idea about this. Hopefully, it's not a very serious thing.
> 
>> That way we can have your patchset not blocked because of this series.
> 
> Thank you!
> 
>>
>> [1] http://dpdk.org/ml/archives/dev/2016-July/044004.html
> 
>>
>> [...]
>>
>>
>> -
>> Shreyansh
> 
> 
>
  

Patch

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 95789f9..994650b 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -185,8 +185,8 @@  static const char DRIVER_EXPORT_NAME_ARRAY(this_pmd_name, idx) \
 __attribute__((used)) = RTE_STR(name)
 
 #define PMD_REGISTER_DRIVER(drv, nm)\
-void devinitfn_ ##drv(void);\
-void __attribute__((constructor, used)) devinitfn_ ##drv(void)\
+RTE_INIT(devinitfn_ ##drv);\
+static void devinitfn_ ##drv(void)\
 {\
 	(drv).name = RTE_STR(nm);\
 	rte_eal_driver_register(&drv);\
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index a71d6f5..186f3c6 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -252,6 +252,9 @@  static inline int rte_gettid(void)
 	return RTE_PER_LCORE(_thread_id);
 }
 
+#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) \
+{ \
+	rte_eal_pci_register(&drv.pci_drv); \
+}
+
 /**
  * Unregister a PCI driver.
  *
diff --git a/lib/librte_eal/common/include/rte_tailq.h b/lib/librte_eal/common/include/rte_tailq.h
index 4a686e6..71ed3bb 100644
--- a/lib/librte_eal/common/include/rte_tailq.h
+++ b/lib/librte_eal/common/include/rte_tailq.h
@@ -148,8 +148,8 @@  struct rte_tailq_head *rte_eal_tailq_lookup(const char *name);
 int rte_eal_tailq_register(struct rte_tailq_elem *t);
 
 #define EAL_REGISTER_TAILQ(t) \
-void tailqinitfn_ ##t(void); \
-void __attribute__((constructor, used)) tailqinitfn_ ##t(void) \
+RTE_INIT(tailqinitfn_ ##t); \
+static void tailqinitfn_ ##t(void) \
 { \
 	if (rte_eal_tailq_register(&t) < 0) \
 		rte_panic("Cannot initialize tailq: %s\n", t.name); \