DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: David Marchand <david.marchand@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Thomas Monjalon <thomas.monjalon@6wind.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	Panu Matilainen <pmatilai@redhat.com>,
	Christian Ehrhardt <christian.ehrhardt@canonical.com>,
	Wenzhuo Lu <wenzhuo.lu@intel.com>,
	"Zhang, Helin" <helin.zhang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
Date: Thu, 21 Apr 2016 08:08:35 -0400	[thread overview]
Message-ID: <20160421120835.GA22931@hmsreliant.think-freely.org> (raw)
In-Reply-To: <CALwxeUtkRMer-TsO2sqcZ8sJ0=NN6jK3ewNCbsZv=a2Ru+pjZg@mail.gmail.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
> 

  reply	other threads:[~2016-04-21 12:08 UTC|newest]

Thread overview: 92+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-05 19:37 [dpdk-dev] time to kill rte_pci_dev_ids.h Stephen Hemminger
2016-01-06  1:40 ` Thomas Monjalon
2016-01-08 13:20   ` David Marchand
2016-01-10 12:50     ` [dpdk-dev] [PATCH 00/11] kill global pci device id list David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 01/11] e1000: move pci device ids to driver David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 02/11] ixgbe: " David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 03/11] i40e: " David Marchand
2016-01-10 20:02         ` Stephen Hemminger
2016-01-12  8:45           ` David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 04/11] fm10k: " David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 05/11] virtio: " David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 06/11] vmxnet3: " David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 07/11] enic: " David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 08/11] bnx2x: " David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 09/11] doc: refresh headers list David Marchand
2016-01-12 14:06         ` Mcnamara, John
2016-01-16 15:10           ` David Marchand
2016-01-18  9:47             ` Thomas Monjalon
2016-01-18 16:25             ` Mcnamara, John
2016-01-10 12:50       ` [dpdk-dev] [PATCH 10/11] pci: no need for global device ids list David Marchand
2016-01-10 12:50       ` [dpdk-dev] [PATCH 11/11] pci: place all uio pci device ids in a dedicated section David Marchand
2016-01-10 12:58       ` [dpdk-dev] [PATCH 00/11] kill global pci device id list David Marchand
2016-01-10 13:24         ` Thomas Monjalon
2016-01-10 13:26           ` David Marchand
2016-01-10 13:27             ` David Marchand
2016-01-10 15:53       ` Zhang, Helin
2016-01-16 15:02         ` David Marchand
2016-01-21  1:12           ` Zhang, Helin
2016-01-18 12:30       ` [dpdk-dev] [PATCH v2 00/10] " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 01/10] e1000: move pci device ids to driver David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 02/10] ixgbe: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 03/10] i40e: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 04/10] fm10k: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 05/10] virtio: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 06/10] vmxnet3: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 07/10] enic: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 08/10] bnx2x: " David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 09/10] pci: no need for global device ids list David Marchand
2016-01-18 12:30         ` [dpdk-dev] [PATCH v2 10/10] pci: place all uio pci device ids in a dedicated section David Marchand
2016-01-19  7:30           ` Thomas Monjalon
2016-01-19 14:29             ` Neil Horman
2016-01-19 16:10               ` Stephen Hemminger
2016-01-19 20:56                 ` Neil Horman
2016-01-19 21:35                   ` Stephen Hemminger
2016-01-20 15:40                     ` Neil Horman
2016-02-24 11:37                       ` Bruce Richardson
2016-02-24 11:50                         ` Thomas Monjalon
2016-02-24 14:18                           ` Neil Horman
2016-04-20 12:43         ` [dpdk-dev] [PATCH v3 00/13] kill global pci device id list David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver David Marchand
2016-04-20 13:29             ` Neil Horman
2016-04-20 13:39               ` David Marchand
2016-04-20 18:15                 ` Neil Horman
2016-04-21  7:27                   ` David Marchand
2016-04-21 12:08                     ` Neil Horman [this message]
2016-04-21 12:41                       ` Thomas Monjalon
2016-04-22 12:13                         ` Neil Horman
2016-04-22 13:22                           ` Thomas Monjalon
2016-07-08 13:31             ` Thomas Monjalon
2016-07-11  5:33               ` Yuanhan Liu
2016-07-11  5:56                 ` Thomas Monjalon
2016-07-11  6:05                   ` Yuanhan Liu
2016-07-11 11:35                   ` David Marchand
2016-07-11 12:05                     ` Yuanhan Liu
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 02/13] ixgbe: " David Marchand
2016-04-29  1:34             ` Wu, Jingjing
2016-05-04  8:26               ` David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 03/13] i40e: " David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 04/13] fm10k: " David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 05/13] virtio: " David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 06/13] vmxnet3: " David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 07/13] enic: " David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 08/13] bnx2x: " David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 09/13] ena: remove unneeded pci macro David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 10/13] pci: no need for global device ids list David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 11/13] drivers: constify pci id tables David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 12/13] drivers: export pci drivers David Marchand
2016-04-20 12:43           ` [dpdk-dev] [PATCH v3 13/13] app: introduce dpdk-obj-info tool David Marchand
2016-04-21  8:07           ` [dpdk-dev] [PATCH v3 00/13] kill global pci device id list David Marchand
2016-04-21 12:13             ` Neil Horman
2016-07-11 14:40           ` [dpdk-dev] [PATCH v4 00/10] kill global pci device id list (almost) David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 01/10] eal: remove PCI device ids header from doxygen David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 02/10] net/e1000: move em PCI device ids to the driver David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 03/10] net/i40e: move " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 04/10] net/fm10k: " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 05/10] net/virtio: " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 06/10] net/vmxnet3: " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 07/10] net/enic: " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 08/10] net/bnx2x: " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 09/10] net/bnxt: " David Marchand
2016-07-11 14:40             ` [dpdk-dev] [PATCH v4 10/10] net/ena: remove unneeded PCI macro David Marchand
2016-07-11 16:27             ` [dpdk-dev] [PATCH v4 00/10] kill global pci device id list (almost) Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160421120835.GA22931@hmsreliant.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=bruce.richardson@intel.com \
    --cc=christian.ehrhardt@canonical.com \
    --cc=david.marchand@6wind.com \
    --cc=dev@dpdk.org \
    --cc=helin.zhang@intel.com \
    --cc=pmatilai@redhat.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas.monjalon@6wind.com \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).