[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Mon, Jul 11, 2016 at 01:35:46PM +0200, David Marchand wrote: > Hello all, > > On Mon, Jul 11, 2016 at 7:56 AM, Thomas Monjalon > wrote: > > 2016-07-11 13:33, Yuanhan Liu: > >> I'm not quite sure I understood it well: are you asking us to resend > >> what David has already send, say me for resending the virtio part? > >> > >> If so, what's the point of that? What's worse, it's likely to fail > >> apply (due to conflicts), as every one of us make a patch based on > >> the same base while touching some same files. > > > > Good point. > > There were some changes since the patches from David (and a new bnxt). > > That's why I was thinking to ask maintainers to take care of this change. > > But maybe it's better to do the change in one patchset. > > David, ok to refresh these patches? > > Now that we have a modinfo-like infra and the test code is exempt from > igb pci ids, all that remains (to fully get rid of this header) are > bypass api and kni/ethtool. > So the deal with Thomas is that I refresh those patches letting igb > and ixgbe pmd as is. > > Will send this later. Thanks, and feel free to put my ack for the virtio part. Acked-by: Yuanhan Liu Great work, BTW! --yliu
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Mon, Jul 11, 2016 at 07:56:16AM +0200, Thomas Monjalon wrote: > 2016-07-11 13:33, Yuanhan Liu: > > On Fri, Jul 08, 2016 at 03:31:27PM +0200, Thomas Monjalon wrote: > > > 2016-04-20 14:43, David Marchand: > > > > test application and kni still want to know e1000 pci devices. > > > > So let's create headers in the driver that will be used by them. > > > > > > There is also an usage of ixgbe ID for the broken bypass API. > > > > > > Sharing those PCI ids outside of the drivers was really a wrong idea. > > > So this a plan to get rid of them: > > > > > > 1/ remove need in PCI autotest (done: http://dpdk.org/commit/1dbba165) > > > 2/ move PCI ids in bnx2x, bnxt, e1000, enic, fm10k, i40e, virtio, vmxnet3 > > > 3/ remove KNI ethtool (only igb/ixgbe support) > > > 4/ remove bypass API or move it to ixgbe specific API > > > 5/ move remaining PCI ids in igb and ixgbe drivers > > > > > > Please driver maintainers, move your PCI ids in your drivers as soon as > > > possible. Thanks > > > > Hi Thomas, > > > > I'm not quite sure I understood it well: are you asking us to resend > > what David has already send, say me for resending the virtio part? > > > > If so, what's the point of that? What's worse, it's likely to fail > > apply (due to conflicts), as every one of us make a patch based on > > the same base while touching some same files. > > Good point. > There were some changes since the patches from David (and a new bnxt). > That's why I was thinking to ask maintainers to take care of this change. > But maybe it's better to do the change in one patchset. Yes, I'd think so. --yliu > David, ok to refresh these patches?
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Fri, Jul 08, 2016 at 03:31:27PM +0200, Thomas Monjalon wrote: > 2016-04-20 14:43, David Marchand: > > test application and kni still want to know e1000 pci devices. > > So let's create headers in the driver that will be used by them. > > There is also an usage of ixgbe ID for the broken bypass API. > > Sharing those PCI ids outside of the drivers was really a wrong idea. > So this a plan to get rid of them: > > 1/ remove need in PCI autotest (done: http://dpdk.org/commit/1dbba165) > 2/ move PCI ids in bnx2x, bnxt, e1000, enic, fm10k, i40e, virtio, vmxnet3 > 3/ remove KNI ethtool (only igb/ixgbe support) > 4/ remove bypass API or move it to ixgbe specific API > 5/ move remaining PCI ids in igb and ixgbe drivers > > Please driver maintainers, move your PCI ids in your drivers as soon as > possible. Thanks Hi Thomas, I'm not quite sure I understood it well: are you asking us to resend what David has already send, say me for resending the virtio part? If so, what's the point of that? What's worse, it's likely to fail apply (due to conflicts), as every one of us make a patch based on the same base while touching some same files. --yliu
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
2016-07-11 13:33, Yuanhan Liu: > On Fri, Jul 08, 2016 at 03:31:27PM +0200, Thomas Monjalon wrote: > > 2016-04-20 14:43, David Marchand: > > > test application and kni still want to know e1000 pci devices. > > > So let's create headers in the driver that will be used by them. > > > > There is also an usage of ixgbe ID for the broken bypass API. > > > > Sharing those PCI ids outside of the drivers was really a wrong idea. > > So this a plan to get rid of them: > > > > 1/ remove need in PCI autotest (done: http://dpdk.org/commit/1dbba165) > > 2/ move PCI ids in bnx2x, bnxt, e1000, enic, fm10k, i40e, virtio, vmxnet3 > > 3/ remove KNI ethtool (only igb/ixgbe support) > > 4/ remove bypass API or move it to ixgbe specific API > > 5/ move remaining PCI ids in igb and ixgbe drivers > > > > Please driver maintainers, move your PCI ids in your drivers as soon as > > possible. Thanks > > Hi Thomas, > > I'm not quite sure I understood it well: are you asking us to resend > what David has already send, say me for resending the virtio part? > > If so, what's the point of that? What's worse, it's likely to fail > apply (due to conflicts), as every one of us make a patch based on > the same base while touching some same files. Good point. There were some changes since the patches from David (and a new bnxt). That's why I was thinking to ask maintainers to take care of this change. But maybe it's better to do the change in one patchset. David, ok to refresh these patches?
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
2016-04-20 14:43, David Marchand: > test application and kni still want to know e1000 pci devices. > So let's create headers in the driver that will be used by them. There is also an usage of ixgbe ID for the broken bypass API. Sharing those PCI ids outside of the drivers was really a wrong idea. So this a plan to get rid of them: 1/ remove need in PCI autotest (done: http://dpdk.org/commit/1dbba165) 2/ move PCI ids in bnx2x, bnxt, e1000, enic, fm10k, i40e, virtio, vmxnet3 3/ remove KNI ethtool (only igb/ixgbe support) 4/ remove bypass API or move it to ixgbe specific API 5/ move remaining PCI ids in igb and ixgbe drivers Please driver maintainers, move your PCI ids in your drivers as soon as possible. Thanks
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
2016-04-22 08:13, Neil Horman: > On Thu, Apr 21, 2016 at 02:41:38PM +0200, Thomas Monjalon wrote: > > 2016-04-21 08:08, Neil Horman: > > > On Thu, Apr 21, 2016 at 09:27:18AM +0200, David Marchand wrote: > > > > 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, thanks for reviewing. > > If David has no time to improve this series, maybe someone else can take > > over. > > I don't see any problem to have several authors on the same series. > > Fair point, and yes, I've not done anything toward this goal at all, so I > admit some work is better than none. We just can't allow lack of time to be a > reason to do something halfway. Yes we can have steps in patchwork. But no, it won't be merged until it is complete. Volunteers are welcome to follow-up on this series. Thanks
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
2016-04-21 08:08, Neil Horman: > On Thu, Apr 21, 2016 at 09:27:18AM +0200, David Marchand wrote: > > 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, thanks for reviewing. If David has no time to improve this series, maybe someone else can take over. I don't see any problem to have several authors on the same series.
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Wed, Apr 20, 2016 at 8:15 PM, Neil Horman 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
>> 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
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Thu, Apr 21, 2016 at 09:27:18AM +0200, David Marchand wrote:
> On Wed, Apr 20, 2016 at 8:15 PM, Neil Horman 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
> >> 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
>
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Wed, Apr 20, 2016 at 3:29 PM, Neil Horman 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.
> 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.
>>
>
>> +
>> +#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.
--
David Marchand
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
test application and kni still want to know e1000 pci devices.
So let's create headers in the driver that will be used by them.
I wanted to reuse base/ headers, but because of some headaches trying to resolve
macros redefinition collisions in kni (with ixgbe next commit), I left it as is.
Signed-off-by: David Marchand
---
app/test/Makefile | 3 +
app/test/test_pci.c | 3 +-
drivers/net/e1000/em_ethdev.c | 2 +-
drivers/net/e1000/em_pci_dev_ids.h | 208 +++
drivers/net/e1000/igb_ethdev.c | 4 +-
drivers/net/e1000/igb_pci_dev_ids.h | 165 +++
lib/librte_eal/common/include/rte_pci_dev_ids.h | 254
lib/librte_eal/linuxapp/kni/Makefile| 1 +
lib/librte_eal/linuxapp/kni/kni_misc.c | 4 +-
9 files changed, 384 insertions(+), 260 deletions(-)
create mode 100644 drivers/net/e1000/em_pci_dev_ids.h
create mode 100644 drivers/net/e1000/igb_pci_dev_ids.h
diff --git a/app/test/Makefile b/app/test/Makefile
index a4907d5..8a1f54c 100644
--- a/app/test/Makefile
+++ b/app/test/Makefile
@@ -171,6 +171,9 @@ CFLAGS_test_memcpy_perf.o += -fno-var-tracking-assignments
endif
endif
+# pci tests want to know some pci devices ids
+CFLAGS_test_pci.o += -I$(RTE_SDK)/drivers/net/e1000
+
# this application needs libraries first
DEPDIRS-y += lib drivers
diff --git a/app/test/test_pci.c b/app/test/test_pci.c
index 0ed357e..7215936 100644
--- a/app/test/test_pci.c
+++ b/app/test/test_pci.c
@@ -77,8 +77,9 @@ struct rte_pci_id my_driver_id2[] = {
/* IGB & EM NICS */
#define RTE_PCI_DEV_ID_DECL_EM(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
+#include
#define RTE_PCI_DEV_ID_DECL_IGB(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
-#include
+#include
{ .vendor_id = 0, /* sentinel */ },
};
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 1f80c05..203a9e5 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -139,7 +139,7 @@ static enum e1000_fc_mode em_fc_setting = e1000_fc_full;
static const struct rte_pci_id pci_id_em_map[] = {
#define RTE_PCI_DEV_ID_DECL_EM(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
-#include "rte_pci_dev_ids.h"
+#include "em_pci_dev_ids.h"
{0},
};
diff --git a/drivers/net/e1000/em_pci_dev_ids.h
b/drivers/net/e1000/em_pci_dev_ids.h
new file mode 100644
index 000..736a68e
--- /dev/null
+++ b/drivers/net/e1000/em_pci_dev_ids.h
@@ -0,0 +1,208 @@
+/*-
+ * This file is provided under a dual BSD/GPLv2 license. When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * The full GNU General Public License is included in this distribution
+ * in the file called LICENSE.GPL.
+ *
+ * Contact Information:
+ * Intel Corporation
+ *
+ * BSD LICENSE
+ *
+ * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Wed, Apr 20, 2016 at 03:39:59PM +0200, David Marchand wrote:
> On Wed, Apr 20, 2016 at 3:29 PM, Neil Horman 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)
> >>
> >
> >> +
> >> +#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
>
[dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver
On Wed, Apr 20, 2016 at 02:43:44PM +0200, David Marchand wrote:
> test application and kni still want to know e1000 pci devices.
> So let's create headers in the driver that will be used by them.
>
> I wanted to reuse base/ headers, but because of some headaches trying to
> resolve
> macros redefinition collisions in kni (with ixgbe next commit), I left it as
> is.
>
> Signed-off-by: David Marchand
> ---
> app/test/Makefile | 3 +
> app/test/test_pci.c | 3 +-
> drivers/net/e1000/em_ethdev.c | 2 +-
> drivers/net/e1000/em_pci_dev_ids.h | 208 +++
> drivers/net/e1000/igb_ethdev.c | 4 +-
> drivers/net/e1000/igb_pci_dev_ids.h | 165 +++
> lib/librte_eal/common/include/rte_pci_dev_ids.h | 254
>
> lib/librte_eal/linuxapp/kni/Makefile| 1 +
> lib/librte_eal/linuxapp/kni/kni_misc.c | 4 +-
> 9 files changed, 384 insertions(+), 260 deletions(-)
> create mode 100644 drivers/net/e1000/em_pci_dev_ids.h
> create mode 100644 drivers/net/e1000/igb_pci_dev_ids.h
>
> diff --git a/app/test/Makefile b/app/test/Makefile
> index a4907d5..8a1f54c 100644
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -171,6 +171,9 @@ CFLAGS_test_memcpy_perf.o += -fno-var-tracking-assignments
> endif
> endif
>
> +# pci tests want to know some pci devices ids
> +CFLAGS_test_pci.o += -I$(RTE_SDK)/drivers/net/e1000
> +
> # this application needs libraries first
> DEPDIRS-y += lib drivers
>
> diff --git a/app/test/test_pci.c b/app/test/test_pci.c
> index 0ed357e..7215936 100644
> --- a/app/test/test_pci.c
> +++ b/app/test/test_pci.c
> @@ -77,8 +77,9 @@ struct rte_pci_id my_driver_id2[] = {
>
> /* IGB & EM NICS */
> #define RTE_PCI_DEV_ID_DECL_EM(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
> +#include
> #define RTE_PCI_DEV_ID_DECL_IGB(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
> -#include
> +#include
>
> { .vendor_id = 0, /* sentinel */ },
> };
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index 1f80c05..203a9e5 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -139,7 +139,7 @@ static enum e1000_fc_mode em_fc_setting = e1000_fc_full;
> static const struct rte_pci_id pci_id_em_map[] = {
>
> #define RTE_PCI_DEV_ID_DECL_EM(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
> -#include "rte_pci_dev_ids.h"
> +#include "em_pci_dev_ids.h"
>
> {0},
> };
> diff --git a/drivers/net/e1000/em_pci_dev_ids.h
> b/drivers/net/e1000/em_pci_dev_ids.h
> new file mode 100644
> index 000..736a68e
> --- /dev/null
> +++ b/drivers/net/e1000/em_pci_dev_ids.h
> @@ -0,0 +1,208 @@
> +/*-
> + * This file is provided under a dual BSD/GPLv2 license. When using or
> + * redistributing this file, you may do so under either license.
> + *
> + * GPL LICENSE SUMMARY
> + *
> + * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * The full GNU General Public License is included in this distribution
> + * in the file called LICENSE.GPL.
> + *
> + * Contact Information:
> + * Intel Corporation
> + *
> + * BSD LICENSE
> + *
> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Intel Corporation nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
