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

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Apr 21, 2016 at 09:27:18AM +0200, David Marchand wrote:
> 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 ?
> 
Thats an option, yes, I'd rename it rte_pci_vendor_ids.h perhaps, but you get the idea.

> Or, we could rely on linux kernel headers (pci_ids.h), rather than
> maintain a header in dpdk.
Thats also a good idea.

> 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 ?
> 
If the dependency is unpalitable, I suppose its fine to duplicate the header
file.  My more direct point was really more just that you didn't need to
duplicate the vendor id macro multiple times within the dpdk project.  You can
still define the device ids internally to each pmd if you don't need it in other
locations, and the duplication against other projects there is ok by me.

> 
> >> > 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.
> 
Ok, so if I'm reading you correctly, you're indicating that test_pci is just
there to test the pci common infrastructure code, and happens to use a few well
known and supported pci id's (igb and ixgbe) to do that.  i.e. theres no need
for exhaustive pci device enumeration there, right?  If thats the case then you
can just create a data structure with the RTE_PCI_DEV macro and stop aliasing it
to all the device specific DECL macros, no?  My main concern here is really just
the needless aliasing of those macros.


> 
> - 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.
> 
Ok, but how does that relate to the use of device specific PCI ennumeration
values?  Looking at the KNI code I see no reason that they are required any
longer (or previously were), and if you're going to move them around you may as
well clean up the usage at the same time.

> 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 :-)
> 
These all sound good :)

> I don't mind doing trivial changes, but I don't have time for more on
> this series.
> 
Um, I'm not sure what to say here.  The whole point of review is to help improve
the code.  If you don't have time to do anything non-trivial, Why are we
reviewing it?

Neil
> 
> -- 
> David Marchand
>