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

[David Marchand <david.marchand@6wind.com>]

On Wed, Apr 20, 2016 at 8:15 PM, Neil Horman <nhorman@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@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