On Wed, Apr 20, 2016 at 8:15 PM, Neil Horman <nhorman at tuxdriver.com> wrote: > 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 at 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
Ok, so you would keep the rte_pci_dev_ids.h with just the vendors in it ? Or, we could rely on linux kernel headers (pci_ids.h), rather than maintain a header in dpdk. But this would add a dependency build on dpdk and I am not sure there is something equivalent on freebsd (afaics all drivers seem to duplicate the pci vendor id). Any freebsd gourou reading this ? >> > 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) - test_pci.c should be about testing pci infrastructure. Relying on igb / ixgbe (or whatever pci device if we extend the list to all pci ids) being present on the system to run successfully is flawed but I have no better idea. - kni implements specific ethtool stuff based on pci ids. kni contains duplicated code from the kernel and it uses those ids to drive to the right ops. The solutions I have imagined so far : * use a shared header for the devices that it supports * drop the use of pci ids between kni library and kni kernel driver, instead use the pmd name that would be resolved internally by the kni library at RTE_KNI_IOCTL_CREATE time, and use this in the kni kernel driver * drop kni :-) I don't mind doing trivial changes, but I don't have time for more on this series. -- David Marchand