Re: [PATCH v3 01/13] e1000: move pci device ids to driver

[Neil Horman <nhorman@tuxdriver.com>]

On Wed, Apr 20, 2016 at 03:39:59PM +0200, David Marchand wrote:
> On Wed, Apr 20, 2016 at 3:29 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >> +#ifndef RTE_PCI_DEV_ID_DECL_EM
> >> +#define RTE_PCI_DEV_ID_DECL_EM(vend, dev)
> >> +#endif
> >> +
> >> +#ifndef PCI_VENDOR_ID_INTEL
> >> +/** Vendor ID used by Intel devices */
> >> +#define PCI_VENDOR_ID_INTEL 0x8086
> >> +#endif
> >> +
> > This is broken, PCI_VENDOR_ID_INTEL should be defined in a central location for
> > all pci drivers, not redefined in every compilation unit.  And you can likely
> 
> Well we can keep the vendors in a common header, but I don't see the benefit.
> 
?
The fact that you won't have to do this
#ifndef PCI_VENDOR_ID_INTEL
#define PCI_VENDOR_ID_INTEL ...
#endif
in every pmd


> > just remvoe the RTE_PCI_DEV_ID_DECL_* macros from each patch and use the
> > RTE_PCI_DEV macro, as all the DECL macros just evaluate to that anyway.
> 
> app/test/test_pci.c:#define RTE_PCI_DEV_ID_DECL_IGB(vend, dev)
> {RTE_PCI_DEVICE(vend, dev)},
> lib/librte_eal/linuxapp/kni/kni_misc.c: #define
> RTE_PCI_DEV_ID_DECL_IGB(vend, dev) case (dev):
> 
> All this stuff is because of pci tests and kni code.
> 
You're going to have to explain a bit further than that.  Why does the kni code
and pci testing code require that each driver have their own macro that just
maps to the same macro underneath?  Looking at the test_pci code, it appears its
done because we used to contain all our pci device ids in a single file, and the
device specific macros were used to selectively enable devices that were there
for testing.  But the point of this series is in part to separate out the device
ids to their own locations, so it seems like you will have to fix up the pci
tests anyway (and additionally it would be nice to include more than just EM,
IGB, and IXGBE in thsoe tests anyway, though I understand thats outside the
scope of this series)

> >><snip>
> >
> >> +
> >> +#undef RTE_PCI_DEV_ID_DECL_EM
> > Why are you undefining this here?  Is it because its in a header file and you're
> > concerned about multiple inclusion?  You should use an ifdef guard instead, or
> > just define all the device id's in the header and ennumerate the pci table in
> > one of the C files for the driver.  That should apply to all the patches in this
> > series I think
> 
> Just moved the code.
> 
Ok, do you plan on cleaning that up?

Neil

> 
> -- 
> David Marchand
>