[dpdk-dev,09/11] eal: move PCI table macro

  Remove include of rte_pci.h in the generic header rte_dev.h
and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.

Fixes: cb6696d22023 ("drivers: update registration macro usage")

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 lib/librte_eal/common/include/rte_dev.h | 6 +-----
 lib/librte_eal/common/include/rte_pci.h | 5 +++++
 2 files changed, 6 insertions(+), 5 deletions(-)
  

2016-07-07 17:36, Thomas Monjalon:
> Remove include of rte_pci.h in the generic header rte_dev.h
> and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> 
> Fixes: cb6696d22023 ("drivers: update registration macro usage")
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Suggested-by: David Marchand <david.marchand@6wind.com>
  

On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
> Remove include of rte_pci.h in the generic header rte_dev.h
> and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> 
> Fixes: cb6696d22023 ("drivers: update registration macro usage")
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>  lib/librte_eal/common/include/rte_dev.h | 6 +-----
>  lib/librte_eal/common/include/rte_pci.h | 5 +++++
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index e6f0d4c..68ca7ef 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -48,7 +48,7 @@ extern "C" {
>  
>  #include <stdio.h>
>  #include <sys/queue.h>
> -#include <rte_pci.h>
> +
>  #include <rte_log.h>
>  
>  __attribute__((format(printf, 2, 0)))
> @@ -195,10 +195,6 @@ DRIVER_EXPORT_NAME(nm, __COUNTER__)
>  
>  #define DRV_EXP_TAG(name, tag) __##name##_##tag
>  
> -#define DRIVER_REGISTER_PCI_TABLE(name, table) \
> -static const char DRV_EXP_TAG(name, pci_tbl_export)[] __attribute__((used)) = \
> -RTE_STR(table)
> -
>  #define DRIVER_REGISTER_PARAM_STRING(name, str) \
>  static const char DRV_EXP_TAG(name, param_string_export)[] \
>  __attribute__((used)) = str
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index fa74962..3b0d26a 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -188,6 +188,11 @@ struct rte_pci_device {
>  	.subsystem_device_id = PCI_ANY_ID
>  #endif
>  
> +#define DRIVER_REGISTER_PCI_TABLE(name, table) \
> +static const char __##name##_pci_tbl_export[] \
> +	__attribute__((used)) = \
> +	RTE_STR(table)
> +
>  struct rte_pci_driver;
>  
>  /**
> -- 
> 2.7.0
> 
> 

This seems strange to me, in that its odd for the driver information export
macros to be spread out in multiple locations.  Specifically it enjoins the use
of the DRV_EXP_TAG macro, which helps centralize tag naming.  Perhaps the happy
medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
its own pmd_register.h header?
  

2016-07-07 12:11, Neil Horman:
> On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
> > Remove include of rte_pci.h in the generic header rte_dev.h
> > and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
[...]
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -48,7 +48,7 @@ extern "C" {
> >  
> >  #include <stdio.h>
> >  #include <sys/queue.h>
> > -#include <rte_pci.h>
> > +
> >  #include <rte_log.h>
> >  
> >  __attribute__((format(printf, 2, 0)))
> > @@ -195,10 +195,6 @@ DRIVER_EXPORT_NAME(nm, __COUNTER__)
> >  
> >  #define DRV_EXP_TAG(name, tag) __##name##_##tag
> >  
> > -#define DRIVER_REGISTER_PCI_TABLE(name, table) \
> > -static const char DRV_EXP_TAG(name, pci_tbl_export)[] __attribute__((used)) = \
> > -RTE_STR(table)
> > -
> >  #define DRIVER_REGISTER_PARAM_STRING(name, str) \
> >  static const char DRV_EXP_TAG(name, param_string_export)[] \
> >  __attribute__((used)) = str
> > diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> > index fa74962..3b0d26a 100644
> > --- a/lib/librte_eal/common/include/rte_pci.h
> > +++ b/lib/librte_eal/common/include/rte_pci.h
> > @@ -188,6 +188,11 @@ struct rte_pci_device {
> >  	.subsystem_device_id = PCI_ANY_ID
> >  #endif
> >  
> > +#define DRIVER_REGISTER_PCI_TABLE(name, table) \
> > +static const char __##name##_pci_tbl_export[] \
> > +	__attribute__((used)) = \
> > +	RTE_STR(table)
> > +
> 
> This seems strange to me, in that its odd for the driver information export
> macros to be spread out in multiple locations.  Specifically it enjoins the use
> of the DRV_EXP_TAG macro, which helps centralize tag naming.  Perhaps the happy
> medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
> its own pmd_register.h header?

I don't know.
David, your opinion?
  

Hello Thomas, Neil,

(will be back in a couple of days, thanks Thomas for pointing this thread)


On Thu, Jul 7, 2016 at 6:25 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-07-07 12:11, Neil Horman:
>> On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
>> > Remove include of rte_pci.h in the generic header rte_dev.h
>> > and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> [...]
>>
>> This seems strange to me, in that its odd for the driver information export
>> macros to be spread out in multiple locations.  Specifically it enjoins the use
>> of the DRV_EXP_TAG macro, which helps centralize tag naming.  Perhaps the happy
>> medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
>> its own pmd_register.h header?
>
> I don't know.
> David, your opinion?

- The suggestion I did offline to Thomas was to move pci stuff in pci headers.
We are trying to move from the "all pci" code in eal to accomodate for
other "buses" / architectures.
Having a pci macro in a generic header like rte_dev.h is wrong to me.
Moving this to a new header like pmd_register.h sounds like a new
generic header with pci specific stuff in it.
So, I am not sure I follow you Neil.

Can you elaborate ?


- Why do you want to centralise the tag naming ?
To avoid collisions ?
Well, adding those tags should not happen that often and I think we
can maintain this with careful reviews.
  

On Fri, Jul 08, 2016 at 10:49:25AM +0200, David Marchand wrote:
> Hello Thomas, Neil,
> 
> (will be back in a couple of days, thanks Thomas for pointing this thread)
> 
> 
> On Thu, Jul 7, 2016 at 6:25 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-07-07 12:11, Neil Horman:
> >> On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
> >> > Remove include of rte_pci.h in the generic header rte_dev.h
> >> > and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> > [...]
> >>
> >> This seems strange to me, in that its odd for the driver information export
> >> macros to be spread out in multiple locations.  Specifically it enjoins the use
> >> of the DRV_EXP_TAG macro, which helps centralize tag naming.  Perhaps the happy
> >> medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
> >> its own pmd_register.h header?
> >
> > I don't know.
> > David, your opinion?
> 
> - The suggestion I did offline to Thomas was to move pci stuff in pci headers.
> We are trying to move from the "all pci" code in eal to accomodate for
> other "buses" / architectures.
I get that, but I'm not sure that applies here.  The macro in question is
specific to pci busses, and if there is additional bus information to export, it
will have its own macro (e.g. DRIVER_REGISTER_USB_TABLE or some such).  While
I could see that being an argument for putting each macro in with its own bus
type, I think thats the wrong organization here, in that people writing drivers
will want to know what export macros are available and will expect to look in a
single place for it.

> Having a pci macro in a generic header like rte_dev.h is wrong to me.
> Moving this to a new header like pmd_register.h sounds like a new
> generic header with pci specific stuff in it.
Well, yes, but I see that as no different than rte_ethdev.c or rte_pdump.c.
both of those files will need to know about all the different types of busses
you support and have to include those corresponding header files (i.e. they will
have to include rte_pci.h, rte_usb.h, rte_i2c.h, etc).  This is really no
different in my mind. 

> So, I am not sure I follow you Neil.
> 
> Can you elaborate ?
> 
I suppose the best way to describe it is that while I understand and support the
desire to separate and abstract bus information away from device function, I
think theres a pragmatic descision here to prioritize functional domain over
header inclusion.  That is to say, I think when people are writing a driver, it
will be helpful to have all the export macros in a single location so they know
what information they can export, and that includes registration of various bus
type identifiers.  So a file like pmd_registration.h that includes rte_pci.h,
rte_usb.h, rte_i2c.h, etc is more useful to a developer, than spreading these
macros out to those various header files, for the sake of avoiding a potentially
unneeded include.

> 
> - Why do you want to centralise the tag naming ?
> To avoid collisions ?
Yes, and to centralize that information.  Since the pmdinfogen tool needs to
know what those tag names are as well, its useful to keep them in the same area
to maintain co-ordination.  Its also useful because it means we can use one
macro to define tag naming convention, instead of having to re-implement or
dead-reckon it in multiple files.

> Well, adding those tags should not happen that often and I think we
> can maintain this with careful reviews.
I don't agree with that.  This discussion is based on the fact that you expect
that we will be adding additional bus types in the future correct?  Well, given
that we have a pci bus specific export macro, I would expect that to proliferate
to every other bus type as well, and so we can expect to at least have a new
tag added for every bus that is added, in addition to any other bus agnostic
information people wish to export (just off hand, looking at the linux modinfo
section, we might expect module author, module version, alias names, licensing
infomration, and others to be potential export candidates).  So, depending on
how much this is adopted, I think we can potentially expect a great deal of
additional tagging to be needed.

Neil
  

2016-07-08 09:56, Neil Horman:
> On Fri, Jul 08, 2016 at 10:49:25AM +0200, David Marchand wrote:
> > Hello Thomas, Neil,
> > 
> > (will be back in a couple of days, thanks Thomas for pointing this thread)
> > 
> > 
> > On Thu, Jul 7, 2016 at 6:25 PM, Thomas Monjalon
> > <thomas.monjalon@6wind.com> wrote:
> > > 2016-07-07 12:11, Neil Horman:
> > >> On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
> > >> > Remove include of rte_pci.h in the generic header rte_dev.h
> > >> > and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> > > [...]
> > >>
> > >> This seems strange to me, in that its odd for the driver information export
> > >> macros to be spread out in multiple locations.  Specifically it enjoins the use
> > >> of the DRV_EXP_TAG macro, which helps centralize tag naming.  Perhaps the happy
> > >> medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
> > >> its own pmd_register.h header?
> > >
> > > I don't know.
> > > David, your opinion?
> > 
> > - The suggestion I did offline to Thomas was to move pci stuff in pci headers.
> > We are trying to move from the "all pci" code in eal to accomodate for
> > other "buses" / architectures.
> I get that, but I'm not sure that applies here.  The macro in question is
> specific to pci busses, and if there is additional bus information to export, it
> will have its own macro (e.g. DRIVER_REGISTER_USB_TABLE or some such).  While
> I could see that being an argument for putting each macro in with its own bus
> type, I think thats the wrong organization here, in that people writing drivers
> will want to know what export macros are available and will expect to look in a
> single place for it.
> 
> > Having a pci macro in a generic header like rte_dev.h is wrong to me.
> > Moving this to a new header like pmd_register.h sounds like a new
> > generic header with pci specific stuff in it.
> Well, yes, but I see that as no different than rte_ethdev.c or rte_pdump.c.
> both of those files will need to know about all the different types of busses
> you support and have to include those corresponding header files (i.e. they will
> have to include rte_pci.h, rte_usb.h, rte_i2c.h, etc).  This is really no
> different in my mind. 
> 
> > So, I am not sure I follow you Neil.
> > 
> > Can you elaborate ?
> > 
> I suppose the best way to describe it is that while I understand and support the
> desire to separate and abstract bus information away from device function, I
> think theres a pragmatic descision here to prioritize functional domain over
> header inclusion.  That is to say, I think when people are writing a driver, it
> will be helpful to have all the export macros in a single location so they know
> what information they can export, and that includes registration of various bus
> type identifiers.  So a file like pmd_registration.h that includes rte_pci.h,
> rte_usb.h, rte_i2c.h, etc is more useful to a developer, than spreading these
> macros out to those various header files, for the sake of avoiding a potentially
> unneeded include.
> 
> > 
> > - Why do you want to centralise the tag naming ?
> > To avoid collisions ?
> Yes, and to centralize that information.  Since the pmdinfogen tool needs to
> know what those tag names are as well, its useful to keep them in the same area
> to maintain co-ordination.  Its also useful because it means we can use one
> macro to define tag naming convention, instead of having to re-implement or
> dead-reckon it in multiple files.
> 
> > Well, adding those tags should not happen that often and I think we
> > can maintain this with careful reviews.
> I don't agree with that.  This discussion is based on the fact that you expect
> that we will be adding additional bus types in the future correct?  Well, given
> that we have a pci bus specific export macro, I would expect that to proliferate
> to every other bus type as well, and so we can expect to at least have a new
> tag added for every bus that is added, in addition to any other bus agnostic
> information people wish to export (just off hand, looking at the linux modinfo
> section, we might expect module author, module version, alias names, licensing
> infomration, and others to be potential export candidates).  So, depending on
> how much this is adopted, I think we can potentially expect a great deal of
> additional tagging to be needed.

Anyway, this macro do not need rte_pci.h.
So the minimal patch can be to just remove this include.
  

On Fri, Jul 08, 2016 at 04:03:40PM +0200, Thomas Monjalon wrote:
> 2016-07-08 09:56, Neil Horman:
> > On Fri, Jul 08, 2016 at 10:49:25AM +0200, David Marchand wrote:
> > > Hello Thomas, Neil,
> > > 
> > > (will be back in a couple of days, thanks Thomas for pointing this thread)
> > > 
> > > 
> > > On Thu, Jul 7, 2016 at 6:25 PM, Thomas Monjalon
> > > <thomas.monjalon@6wind.com> wrote:
> > > > 2016-07-07 12:11, Neil Horman:
> > > >> On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
> > > >> > Remove include of rte_pci.h in the generic header rte_dev.h
> > > >> > and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> > > > [...]
> > > >>
> > > >> This seems strange to me, in that its odd for the driver information export
> > > >> macros to be spread out in multiple locations.  Specifically it enjoins the use
> > > >> of the DRV_EXP_TAG macro, which helps centralize tag naming.  Perhaps the happy
> > > >> medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
> > > >> its own pmd_register.h header?
> > > >
> > > > I don't know.
> > > > David, your opinion?
> > > 
> > > - The suggestion I did offline to Thomas was to move pci stuff in pci headers.
> > > We are trying to move from the "all pci" code in eal to accomodate for
> > > other "buses" / architectures.
> > I get that, but I'm not sure that applies here.  The macro in question is
> > specific to pci busses, and if there is additional bus information to export, it
> > will have its own macro (e.g. DRIVER_REGISTER_USB_TABLE or some such).  While
> > I could see that being an argument for putting each macro in with its own bus
> > type, I think thats the wrong organization here, in that people writing drivers
> > will want to know what export macros are available and will expect to look in a
> > single place for it.
> > 
> > > Having a pci macro in a generic header like rte_dev.h is wrong to me.
> > > Moving this to a new header like pmd_register.h sounds like a new
> > > generic header with pci specific stuff in it.
> > Well, yes, but I see that as no different than rte_ethdev.c or rte_pdump.c.
> > both of those files will need to know about all the different types of busses
> > you support and have to include those corresponding header files (i.e. they will
> > have to include rte_pci.h, rte_usb.h, rte_i2c.h, etc).  This is really no
> > different in my mind. 
> > 
> > > So, I am not sure I follow you Neil.
> > > 
> > > Can you elaborate ?
> > > 
> > I suppose the best way to describe it is that while I understand and support the
> > desire to separate and abstract bus information away from device function, I
> > think theres a pragmatic descision here to prioritize functional domain over
> > header inclusion.  That is to say, I think when people are writing a driver, it
> > will be helpful to have all the export macros in a single location so they know
> > what information they can export, and that includes registration of various bus
> > type identifiers.  So a file like pmd_registration.h that includes rte_pci.h,
> > rte_usb.h, rte_i2c.h, etc is more useful to a developer, than spreading these
> > macros out to those various header files, for the sake of avoiding a potentially
> > unneeded include.
> > 
> > > 
> > > - Why do you want to centralise the tag naming ?
> > > To avoid collisions ?
> > Yes, and to centralize that information.  Since the pmdinfogen tool needs to
> > know what those tag names are as well, its useful to keep them in the same area
> > to maintain co-ordination.  Its also useful because it means we can use one
> > macro to define tag naming convention, instead of having to re-implement or
> > dead-reckon it in multiple files.
> > 
> > > Well, adding those tags should not happen that often and I think we
> > > can maintain this with careful reviews.
> > I don't agree with that.  This discussion is based on the fact that you expect
> > that we will be adding additional bus types in the future correct?  Well, given
> > that we have a pci bus specific export macro, I would expect that to proliferate
> > to every other bus type as well, and so we can expect to at least have a new
> > tag added for every bus that is added, in addition to any other bus agnostic
> > information people wish to export (just off hand, looking at the linux modinfo
> > section, we might expect module author, module version, alias names, licensing
> > infomration, and others to be potential export candidates).  So, depending on
> > how much this is adopted, I think we can potentially expect a great deal of
> > additional tagging to be needed.
> 
> Anyway, this macro do not need rte_pci.h.
> So the minimal patch can be to just remove this include.
> 
Oh my gosh!  I've been an idiot!  you're absolutely right. The macro is just
defining a string to point to the pci_table symbol name, its not typed to the
pci_tbl symbol at all, and so we can just remove the header file.  Somehow I had
it in my head that the macro created a pci specific typed symbol, but it totally
doesn't.  Apologies.

Yes, just removing the include <rte_pci.h> is exactly the right move here.
Neil