Re: Mostly revert e1000/e1000e: Move PCI-Express device IDs over to e1000e
Linus Torvalds wrote:
On Tue, 29 Jan 2008, Randy Dunlap wrote:
Andrew was concerned about this when the driver was in -mm.
He asked for a patch that would set E1000E to same value as E1000
and I supplied that. Auke acked it IIRC. Other people vetoed it. :(
Yeah, I've been discussing with Jeff and the gang.
I think we have agreed on a solution where the ID's show up in the old
driver if the new driver is not enabled at all.
(And as a side note: it turns out that the problem I experienced didn't
come from the new e1000e driver after all, so I'll be removing the
EXPERIMENTAL flag again).
So I'd suggest the final patch be something like this, but I'm sendign it
out just as an example of how we could solve this, not necessarily as a
final patch.
Jeff, Auke, would something like this be acceptable? It makes it very
obvious in the driver table which entries are for the PCIE versions that
would be handled by the E1000E driver if it is enabled..
Untested, but as mentioned, this is more of a this looks maintainable and
like it should solve the issues rather than anything I was planning on
committing now.
Linus
---
drivers/net/Kconfig|5 ++-
drivers/net/e1000/e1000_main.c | 60 ++--
2 files changed, 37 insertions(+), 28 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 5a2d1dd..6c57540 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1992,7 +1992,7 @@ config E1000_DISABLE_PACKET_SPLIT
config E1000E
tristate Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support
- depends on PCI EXPERIMENTAL
+ depends on PCI
---help---
This driver supports the PCI-Express Intel(R) PRO/1000 gigabit
ethernet family of adapters. For PCI or PCI-X e1000 adapters,
@@ -2009,6 +2009,9 @@ config E1000E
To compile this driver as a module, choose M here. The module
will be called e1000e.
+config E1000E_ENABLED
+ def_bool E1000E != n
+
config IP1000
tristate IP1000 Gigabit Ethernet support
depends on PCI EXPERIMENTAL
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 3111af6..8c87940 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -47,6 +47,12 @@ static const char e1000_copyright[] = Copyright (c)
1999-2006 Intel Corporation
* Macro expands to...
* {PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id)}
*/
+#ifdef CONFIG_E1000E_ENABLED
+ #define PCIE(x)
+#else
+ #define PCIE(x) x,
+#endif
Patch gets my ACK, if you like, though an improvement would be to have
your Kconfig logic activate CONFIG_E1000_PCIEX. Then future janitors
could come along and disable unused code in addition to PCI IDs.
Jeff
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mostly revert e1000/e1000e: Move PCI-Express device IDs over to e1000e
Jeff Garzik wrote:
Linus Torvalds wrote:
On Tue, 29 Jan 2008, Randy Dunlap wrote:
Andrew was concerned about this when the driver was in -mm.
He asked for a patch that would set E1000E to same value as E1000
and I supplied that. Auke acked it IIRC. Other people vetoed it. :(
Yeah, I've been discussing with Jeff and the gang.
I think we have agreed on a solution where the ID's show up in the old
driver if the new driver is not enabled at all.
(And as a side note: it turns out that the problem I experienced
didn't come from the new e1000e driver after all, so I'll be removing
the EXPERIMENTAL flag again).
So I'd suggest the final patch be something like this, but I'm sendign
it out just as an example of how we could solve this, not necessarily
as a final patch.
Jeff, Auke, would something like this be acceptable? It makes it very
obvious in the driver table which entries are for the PCIE versions
that would be handled by the E1000E driver if it is enabled..
Untested, but as mentioned, this is more of a this looks maintainable
and like it should solve the issues rather than anything I was
planning on committing now.
Linus
---
drivers/net/Kconfig|5 ++-
drivers/net/e1000/e1000_main.c | 60
++--
2 files changed, 37 insertions(+), 28 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 5a2d1dd..6c57540 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1992,7 +1992,7 @@ config E1000_DISABLE_PACKET_SPLIT
config E1000E
tristate Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support
-depends on PCI EXPERIMENTAL
+depends on PCI
---help---
This driver supports the PCI-Express Intel(R) PRO/1000 gigabit
ethernet family of adapters. For PCI or PCI-X e1000 adapters,
@@ -2009,6 +2009,9 @@ config E1000E
To compile this driver as a module, choose M here. The module
will be called e1000e.
+config E1000E_ENABLED
+def_bool E1000E != n
+
config IP1000
tristate IP1000 Gigabit Ethernet support
depends on PCI EXPERIMENTAL
diff --git a/drivers/net/e1000/e1000_main.c
b/drivers/net/e1000/e1000_main.c
index 3111af6..8c87940 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -47,6 +47,12 @@ static const char e1000_copyright[] = Copyright
(c) 1999-2006 Intel Corporation
* Macro expands to...
* {PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id)}
*/
+#ifdef CONFIG_E1000E_ENABLED
+ #define PCIE(x) +#else
+ #define PCIE(x) x,
+#endif
Patch gets my ACK, if you like, though an improvement would be to have
your Kconfig logic activate CONFIG_E1000_PCIEX. Then future janitors
could come along and disable unused code in addition to PCI IDs.
Ack from my side as well, allthough I hope that this code will not live long as
I
would love to start taking out pci-e code out of e1000. If we merge this patch
then I suggest that we don't do that until for at least a whole cycle, since it
does not make much sense otherwise.
Auke
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mostly revert e1000/e1000e: Move PCI-Express device IDs over to e1000e
On Wed, Jan 30, 2008 at 04:51:04PM +1100, Linus Torvalds wrote: On Tue, 29 Jan 2008, Randy Dunlap wrote: Andrew was concerned about this when the driver was in -mm. He asked for a patch that would set E1000E to same value as E1000 and I supplied that. Auke acked it IIRC. Other people vetoed it. :( Yeah, I've been discussing with Jeff and the gang. I think we have agreed on a solution where the ID's show up in the old driver if the new driver is not enabled at all. (And as a side note: it turns out that the problem I experienced didn't come from the new e1000e driver after all, so I'll be removing the EXPERIMENTAL flag again). So I'd suggest the final patch be something like this, but I'm sendign it out just as an example of how we could solve this, not necessarily as a final patch. Jeff, Auke, would something like this be acceptable? It makes it very obvious in the driver table which entries are for the PCIE versions that would be handled by the E1000E driver if it is enabled.. Untested, but as mentioned, this is more of a this looks maintainable and like it should solve the issues rather than anything I was planning on committing now. I don't like it: We should aim at having exactly one driver for one card. Your patch has effects like e.g. a kernel behaving differently when adding and compiling the e1000e module later compared to having it originally in the .config. And fun like The card works on my machine with the e1000 driver, why doesn't it work in your machine with the e1000 driver?. And in terms of maintainability, people will disable the e1000e driver in their kernel for working around bugs in it instead of reporting the bugs. Exactly what we want to not happen. And unless we want to keep this situation forever, we anyway have to remove the support for the PCI-Express adapters from the e1000 driver at some point in time, so why not make a clear cut now? Whatever problems this causes will be the same now or in a few years. Linus cu Adrian -- Is there not promise of rain? Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. Only a promise, Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mostly revert e1000/e1000e: Move PCI-Express device IDs over to e1000e
Adrian Bunk wrote: Jeff, Auke, would something like this be acceptable? It makes it very obvious in the driver table which entries are for the PCIE versions that would be handled by the E1000E driver if it is enabled.. I don't like it: We should aim at having exactly one driver for one card. There is one thing I don't understand, but that may well be just me... From Linus' original patch: +++ b/drivers/net/e1000/e1000_main.c + INTEL_E1000_ETHERNET_DEVICE(0x108C), So, apparently support for 8086:108c was removed from the e1000 driver. From my lspci: $ lspci -nn | grep Ether 01:00.0 Ethernet controller [0200]: Intel Corporation 82573E Gigabit Ethernet Controller (Copper) [8086:108c] (rev 03) But when I look at where that card is sitting: $ readlink pci/devices/\:01\:00.0/driver ../../../../bus/pci/drivers/e1000 So, it's on the PCI bus, not on the PCI-Express bus (which I also have, but which has no devices on it). Or does the e1000e driver also support cards on the PCI bus? If that's the case then the original changelog entry Move PCI-Express device IDs over to e1000e is misleading as it's not only PCI-Express devices... Hmmm. Or does which driver is loaded decide on which bus the device ends up? Confused, FJP -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Mostly revert e1000/e1000e: Move PCI-Express device IDs over to e1000e
Frans Pop wrote: There is one thing I don't understand, but that may well be just me... From Linus' original patch: +++ b/drivers/net/e1000/e1000_main.c + INTEL_E1000_ETHERNET_DEVICE(0x108C), So, apparently support for 8086:108c was removed from the e1000 driver. When it was enabled to be supported by e1000e. From my lspci: $ lspci -nn | grep Ether 01:00.0 Ethernet controller [0200]: Intel Corporation 82573E Gigabit Ethernet Controller (Copper) [8086:108c] (rev 03) But when I look at where that card is sitting: $ readlink pci/devices/\:01\:00.0/driver ../../../../bus/pci/drivers/e1000 So, it's on the PCI bus, not on the PCI-Express bus (which I also have, but which has no devices on it). 82573E/L are PCIe devices only, don't let the use of PCI configuration space confuse you. All PCIe devices support PCI configuration space. This allows systems with PCIe to work right (or mostly right) with all the PCI supporting software like Linux. Or does the e1000e driver also support cards on the PCI bus? E1000e is targeted at the PCIe devices only. If that's the case then the original changelog entry Move PCI-Express device IDs over to e1000e is misleading as it's not only PCI-Express devices... Unfortunate bit of confusion over terminology. Hmmm. Or does which driver is loaded decide on which bus the device ends up? Hope this helped, Jesse -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
