[dpdk-dev] [PATCHv4 2/5] drivers: Update driver registration macro usage

[Neil Horman]

On Wed, May 25, 2016 at 06:20:06PM +0200, Thomas Monjalon wrote:
> 2016-05-24 15:41, Neil Horman:
> > Modify the PMD_REGISTER_DRIVER macro, adding a name argument to it.  The
> > addition of a name argument creates a token that can be used for subsequent
> > macros in the creation of unique symbol names to export additional bits of
> > information for use by the pmdinfogen tool.  For example:
> > 
> > PMD_REGISTER_DRIVER(ena_driver, ena);
> > 
> > registers the ena_driver struct as it always did, and creates a symbol
> > const char this_pmd_name0[] __attribute__((used)) = "ena";
> > 
> > which pmdinfogen can search for and extract.
> 
> The EAL rework (http://dpdk.org/ml/archives/dev/2016-April/037691.html)
> integrates already a name:
> 
> +#define RTE_EAL_PCI_REGISTER(name, d) \
> +RTE_INIT(pciinitfn_ ##name); \
> +static void pciinitfn_ ##name(void) \
> +{ \
> +	rte_eal_pci_register(&d); \
> +}
> 
> I think it would be better to rebase on top of it.
> 
Those patches are over a month old and still in the new state according to
patchwork.  I'm not very comfortable rebasing (and implicitly blocking)
acceptance of this patch on that one.  Its really a just two lines of conflict.  I
would suggest that, whichever patch gets integrated first, the other series can
rebase on the new head.  It should be a pretty easy fix either way.

> > The subsequent macro
> > 
> > DRIVER_REGISTER_PCI_TABLE(ena, ena_pci_id_map);
> > 
> > creates a symbol
> > const char ena_pci_tbl_export[] __attribute__((used)) = "ena_pci_id_map";
> > 
> > Which allows pmdinfogen to find the pci table of this driver
> > 
> > Using this pattern, we can export arbitrary bits of information.
> > 
> > pmdinfo uses this information to extract hardware support from an object file
> > and create a json string to make hardware support info discoverable later.
> 
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -34,4 +34,6 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >  DIRS-y += net
> >  DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += crypto
> >  
> > +DEPDIRS-y += buildtools/pmdinfo
> 
> Why pmdinfo is a build dependency?
> 
beause pmdinfogen has to be built and available for use prior to compiling the
rest of the dpdk.  I suppose we could build it after, and then go back through
and check all the objects for driver info, but I'd rather build it first, and
search the objects as they are built.

> > --- 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>
> 
> Why not keep PCI stuff in rte_pci.h?
> 
I am.

> > +#define DRV_EXP_TAG(n, t) __##n##_##t
> > +
> > +#define DRIVER_REGISTER_PCI_TABLE(n, t) \
> > +static const char DRV_EXP_TAG(n, pci_tbl_export)[] __attribute__((used)) = RTE_STR(t)
> 
> I really dislike one-char variables, especially when there is no comment.
> Please choose comments or explicit variables.
> 

You mean you want the macro variables to be longer/more descriptive?  I suppose,
but in fairness, we have lots of macros that use single letter variables, I'm
not sure why your concerned about these specifically.  I'll change it though.

Neil