Re: Mostly revert "e1000/e1000e: Move PCI-Express device IDs over to e1000e"

Re: Mostly revert "e1000/e1000e: Move PCI-Express device IDs over to e1000e"

[Jeff Garzik]

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

Re: Mostly revert "e1000/e1000e: Move PCI-Express device IDs over to e1000e"

[Kok, Auke]

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

Re: Mostly revert "e1000/e1000e: Move PCI-Express device IDs over to e1000e"

[Adrian Bunk]

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

Re: Mostly revert "e1000/e1000e: Move PCI-Express device IDs over to e1000e"

[Frans Pop]

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/0000\: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

RE: Mostly revert "e1000/e1000e: Move PCI-Express device IDs over to e1000e"

[Brandeburg, Jesse]

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/0000\: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