e1000: backport ich9 support from 7.5.5 ?

e1000: backport ich9 support from 7.5.5 ?

[Mark McLoughlin]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

Hi Auke,
	I understand there is some delay in getting e1000-7.5.5 into the
upstream kernel given the major re-working of the chipset specific
parts.

	I wonder would it be feasible in the meantime to backport the ich9
support and push it upstream?

	A first-cut at the backport is attached. Note, this patch applies
against latest netdev-2.6-git, but I haven't actually tested this
version of the patch beyond compiling. A similar patch against 2.6.18
worked fine with some light testing.

Thanks,
Mark.

[-- Attachment #2: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel

Re: e1000: backport ich9 support from 7.5.5 ?

[Jason Lunz]

On Fri, Jun 29, 2007 at 06:29:18PM +0100, Mark McLoughlin wrote:
> 	I understand there is some delay in getting e1000-7.5.5 into the
> upstream kernel given the major re-working of the chipset specific
> parts.
> 
> 	I wonder would it be feasible in the meantime to backport the ich9
> support and push it upstream?

seconded - this driver reorg has been holding back support for the
newest e1000 hardware since at least 2.6.20, and I haven't seen any
indication that the 7.5.5 e1000 driver will even be added to 2.6.23.

What's the prognosis for the 7.5.5-series e1000 reorg going into
netdev-2.6?

Jason

Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jason Lunz wrote:
> On Fri, Jun 29, 2007 at 06:29:18PM +0100, Mark McLoughlin wrote:
>> 	I understand there is some delay in getting e1000-7.5.5 into the
>> upstream kernel given the major re-working of the chipset specific
>> parts.
>>
>> 	I wonder would it be feasible in the meantime to backport the ich9
>> support and push it upstream?
> 
> seconded - this driver reorg has been holding back support for the
> newest e1000 hardware since at least 2.6.20, and I haven't seen any
> indication that the 7.5.5 e1000 driver will even be added to 2.6.23.

I disagree, we should not break the current e1000 driver in the kernel while 
there is a new driver coming up that introduces ich9 support without breaking 
(the old e1000) support for all other devices. This is why we want to drop a new 
version of the e1000 driver upstream instead, and put all newer devices in that 
driver.

For distro's not following kernel.org releases we have the perfect solution: A 
fully tested 7.5.5 driver on sourceforge that was extensively tested against 
RHEL5 for instance, but also a lot of other older kernels.

> What's the prognosis for the 7.5.5-series e1000 reorg going into
> netdev-2.6?

I sure hope to get it out before 2.6.23 merge window close... But anything can 
unfortunately happen.


Auke

Re: e1000: backport ich9 support from 7.5.5 ?

[Jason Lunz]

On Fri, Jun 29, 2007 at 12:51:02PM -0700, Kok, Auke wrote:
> For distro's not following kernel.org releases we have the perfect 
> solution: A fully tested 7.5.5 driver on sourceforge that was extensively 
> tested against RHEL5 for instance, but also a lot of other older kernels.

I'm sure you and your team have tested the driver thoroughly, but you
cannot hope to match the amount of testing the driver is subjected to by
going into -mm and -rc kernels.

There have been several instances over the years in the e1000 driver
where new releases caused regressions on old or esoteric hardware, and
these were only later discovered by the community. This is why I prefer
kernel.org versions of the e1000 driver, even if they're a little older.

Ordinarily this is a pretty good strategy, because the mainline driver
hasn't usually lagged too far behind new hardware becoming available.
That's no longer true of the newer pcie e1000 parts and the 7.5.5.1
driver.

Is there any specific objection remaining to the merge of e1000 7.5.5.1
into 2.6.23? Is there anything I can do to expedite the process other
than cheer from the sidelines?

Jason

Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Jason Lunz wrote:
> What's the prognosis for the 7.5.5-series e1000 reorg going into
> netdev-2.6?


As I have posted previously, I am not keen on merging basically an e1000 
rewrite.  That basically throws away all Internet-wide testing so far. 
The rewrite was not done according to Linux kernel standards (read: 
progression of changes), so any problems will bisect into The Big 
Commit(tm) and go no further.  A rewrite also means the complete driver 
has to be reviewed from scratch, since such a large patch is basically 
impossible to review.  Finally, the rewrite doesn't do much to clean up 
the e1000 driver.

I warned Intel for months not to do things this way, but I guess my 
opinion counts for little :)

	Jeff

Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Kok, Auke wrote:
> I disagree, we should not break the current e1000 driver in the kernel 
> while there is a new driver coming up that introduces ich9 support 
> without breaking (the old e1000) support for all other devices. This is 
> why we want to drop a new version of the e1000 driver upstream instead, 
> and put all newer devices in that driver.

If it's clean -and- new hardware is quite different from older hardware, 
a new driver makes sense, and causes far less disruption to both new and 
old hardware.


> For distro's not following kernel.org releases we have the perfect 
> solution: A fully tested 7.5.5 driver on sourceforge that was 
> extensively tested against RHEL5 for instance, but also a lot of other 
> older kernels.

I respectfully object to the advertising, here.

Based on past history, "posted on sf.net and tested by Intel" does not 
equate to "extensively tested."  Field experience proves time and again 
that PickYourVendor test labs always miss the raft of problems that show 
up once code is upstream and in production use.  If the code has never 
seen Internet-wide testing in an upstream kernel, it's -not- widely nor 
extensively tested.

	Jeff




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jeff Garzik wrote:
> Jason Lunz wrote:
>> What's the prognosis for the 7.5.5-series e1000 reorg going into
>> netdev-2.6?
> 
> As I have posted previously, I am not keen on merging basically an e1000 
> rewrite.  That basically throws away all Internet-wide testing so far. 
> The rewrite was not done according to Linux kernel standards (read: 
> progression of changes), so any problems will bisect into The Big 
> Commit(tm) and go no further.  A rewrite also means the complete driver 
> has to be reviewed from scratch, since such a large patch is basically 
> impossible to review.  Finally, the rewrite doesn't do much to clean up 
> the e1000 driver.
> 
> I warned Intel for months not to do things this way, but I guess my 
> opinion counts for little :)

no, they do

However, I am stuck with a half rewritten e1000 codebase (the 7.4.x code base) 
and on top of that all the changes that you requested from us. This effort has 
taken me about two months of time.

Taking apart that work change for every change seems very wasteful to me: it 
mucks up the current e1000 driver potentially, might break it and that is 
exactly what we want to prevent.

That's why we want to introduce a second e1000 driver (named differently, pick 
any name) that contains the new code base, side-by-side into the kernel with the 
current e1000.

This would allow everyone to fallback and compare the new and old code 
instantly, for several kernel releases at least. After that period, we can 
retire the old codebase.

Given the size of the changes and the fact that this is an interal API change 
that gigantically changes how e1000 internally works, there is *no* way that we 
can introduce this change in any other way in my opinion.

Looking at that, I think that bringing a second and new e1000 driver into the 
kernel is the best thing to do, and that is what I have been working towards. 
This new e1000 codebase goes miles and miles beyond what I posted in april/march 
and what is in -mm. For instance: the new codebase no longer has mac_type checks 
in *any* part of e1000_main.c.

I will try to post it as soon as I can. Please bear with me until I get that out 
  to everyone.

Auke

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Andy Gospodarek]

On Fri, Jun 29, 2007 at 10:50:07AM -0700, Jason Lunz wrote:
> On Fri, Jun 29, 2007 at 06:29:18PM +0100, Mark McLoughlin wrote:
> > 	I understand there is some delay in getting e1000-7.5.5 into the
> > upstream kernel given the major re-working of the chipset specific
> > parts.
> > 
> > 	I wonder would it be feasible in the meantime to backport the ich9
> > support and push it upstream?
> 
> seconded - this driver reorg has been holding back support for the
> newest e1000 hardware since at least 2.6.20, and I haven't seen any
> indication that the 7.5.5 e1000 driver will even be added to 2.6.23.
> 

I'll ACK this as well.  I'd like to see ich9 support upstream as soon as
possible and I'd rather see it with the old driver than not at all.



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Andrew Morton]

On Fri, 29 Jun 2007 14:39:20 -0700
"Kok, Auke" <auke-jan.h.kok@intel.com> wrote:

> 
> That's why we want to introduce a second e1000 driver (named differently, pick 
> any name) that contains the new code base, side-by-side into the kernel with the 
> current e1000.

Sounds like a reasonable approach to me (it has plenty of precedent).  But
I forget what all the other issues were, so ignore me.

> This new e1000 codebase goes miles and miles beyond what I posted in april/march 
> and what is in -mm.

There are no e1000 changes in -mm (from you), I don't believe.  git-e1000
has been in permadrop mode since 2.6.22-rc1-mm1 due to a huge number of git
rejects/conflicts.

Is git://lost.foo-projects.org/~ahkok/git/netdev-2.6#mm the correct URL?


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Kok, Auke wrote:
> That's why we want to introduce a second e1000 driver (named 
> differently, pick any name) that contains the new code base, 
> side-by-side into the kernel with the current e1000.

We do not want to introduce duplicate drivers for the same hardware.  We 
spend -years- before the old driver gets removed.  There are installer 
headaches with distros, with duplicate drivers.  User confusion. 
Problems abound, as past history shows.

I meant a new driver for ICH9 and later hardware, or whatever hardware 
boundary between new-driver and old-driver you wish to pick.

That way the new driver doesn't carry all the old baggage with it, and 
can go quietly into old age.


> This would allow everyone to fallback and compare the new and old code 
> instantly, for several kernel releases at least. After that period, we 
> can retire the old codebase.

No it takes -years- to kill duplicate drivers, when you consider all the 
transition effects.  That is what history shows us time and again.


> Given the size of the changes and the fact that this is an interal API 
> change that gigantically changes how e1000 internally works, there is 
> *no* way that we can introduce this change in any other way in my opinion.

The standard way to introduce big changes is to (a) do them, and then 
(b) plot the steps involved in the transition, and create those steps 
that lead us to the goal.

	Jeff

Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Andrew Morton wrote:
> On Fri, 29 Jun 2007 14:39:20 -0700
> "Kok, Auke" <auke-jan.h.kok@intel.com> wrote:
> 
>> That's why we want to introduce a second e1000 driver (named differently, pick 
>> any name) that contains the new code base, side-by-side into the kernel with the 
>> current e1000.
> 
> Sounds like a reasonable approach to me (it has plenty of precedent).  But
> I forget what all the other issues were, so ignore me.

Given past history with duplicate drivers and the problems that they 
cause -- I know, I've caused some of those problems :( -- I strongly 
recommend against when it can be avoided.

Leaving e1000 with current hardware, and a new e1001 for newer hardware 
should be easier to manage for all involved, without the headaches that 
duplicate drivers cause.

An "e1001" approach also means we have a much greater chance of 
encouraging Intel down the path of clean driver-ness.

	Jeff

Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Andrew Morton wrote:
> On Fri, 29 Jun 2007 14:39:20 -0700
> "Kok, Auke" <auke-jan.h.kok@intel.com> wrote:
> 
>> That's why we want to introduce a second e1000 driver (named differently, pick 
>> any name) that contains the new code base, side-by-side into the kernel with the 
>> current e1000.
> 
> Sounds like a reasonable approach to me (it has plenty of precedent).  But
> I forget what all the other issues were, so ignore me.
> 
>> This new e1000 codebase goes miles and miles beyond what I posted in april/march 
>> and what is in -mm.
> 
> There are no e1000 changes in -mm (from you), I don't believe.  git-e1000
> has been in permadrop mode since 2.6.22-rc1-mm1 due to a huge number of git
> rejects/conflicts.
> 
> Is git://lost.foo-projects.org/~ahkok/git/netdev-2.6#mm the correct URL?

no, not yet, please remove that tree. I'll post a new one when ready.

Auke

RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jeff Garzik wrote:
> Andrew Morton wrote:
>> On Fri, 29 Jun 2007 14:39:20 -0700
>> "Kok, Auke" <auke-jan.h.kok@intel.com> wrote:
>>
>>> That's why we want to introduce a second e1000 driver (named differently, pick 
>>> any name) that contains the new code base, side-by-side into the kernel with the 
>>> current e1000.
>> Sounds like a reasonable approach to me (it has plenty of precedent).  But
>> I forget what all the other issues were, so ignore me.
> 
> Given past history with duplicate drivers and the problems that they 
> cause -- I know, I've caused some of those problems :( -- I strongly 
> recommend against when it can be avoided.
> 
> Leaving e1000 with current hardware, and a new e1001 for newer hardware 
> should be easier to manage for all involved, without the headaches that 
> duplicate drivers cause.
> 
> An "e1001" approach also means we have a much greater chance of 
> encouraging Intel down the path of clean driver-ness.

ok, FWIW, here is this new driver:


I've posted this new driver (currently called "e1000new") on my git tree:

    git-pull git://lost.foo-projects.org/~ahkok/git/netdev-2.6 e1000new

It contains a single git-commit with the following content (on top of current 
master branch from Jeff's netdev tree):

---
commit d2ec375736ac23a3432534112e0f8fc172ee9db4
Author: Auke Kok <auke-jan.h.kok@intel.com>
Date:   Fri Jun 29 15:48:04 2007 -0700

     e1000new: new e1000 driver for current e1000 hardware

     This driver is a cleaned up version of the current e1000 driver. It
     introduces a fully rewritten abstraction between MAC, PHY, NVM
     and other low-level hardware features such as manageability. Per-
     hardware family specific code.

     Mac type checks are almost completely eliminated and replaced with
     hardware feature/issue or capability flags. This allows us much
     easier going forward to add new hardware support or debug issues.

     Finally, the driver structures were reorganized and restructured
     to align better and remove holes. tx and rx structures are now
     basically the same and simplify setup code etc.

     Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>

:100644 100644 7d57f4a... 0bad8f3... M  drivers/net/Kconfig
:100644 100644 a77affa... 0b58e78... M  drivers/net/Makefile
:000000 100644 0000000... 5bdf481... A  drivers/net/e1000new/80003es2lan.c
:000000 100644 0000000... f139050... A  drivers/net/e1000new/80003es2lan.h
:000000 100644 0000000... 1279521... A  drivers/net/e1000new/82540.c
:000000 100644 0000000... dc20510... A  drivers/net/e1000new/82541.c
:000000 100644 0000000... 5fab7ff... A  drivers/net/e1000new/82541.h
:000000 100644 0000000... e18a1be... A  drivers/net/e1000new/82542.c
:000000 100644 0000000... dad89ae... A  drivers/net/e1000new/82543.c
:000000 100644 0000000... 6d14aba... A  drivers/net/e1000new/82543.h
:000000 100644 0000000... 3283841... A  drivers/net/e1000new/82571.c
:000000 100644 0000000... 5912f47... A  drivers/net/e1000new/82571.h
:000000 100644 0000000... 703d938... A  drivers/net/e1000new/Makefile
:000000 100644 0000000... 2de032e... A  drivers/net/e1000new/api.c
:000000 100644 0000000... e429f8c... A  drivers/net/e1000new/api.h
:000000 100644 0000000... 59f34db... A  drivers/net/e1000new/defines.h
:000000 100644 0000000... 71b775d... A  drivers/net/e1000new/e1000.h
:000000 100644 0000000... 803ed2c... A  drivers/net/e1000new/ethtool.c
:000000 100644 0000000... 848debc... A  drivers/net/e1000new/hw.h
:000000 100644 0000000... e5adbff... A  drivers/net/e1000new/ich8lan.c
:000000 100644 0000000... 557858f... A  drivers/net/e1000new/ich8lan.h
:000000 100644 0000000... 8f589a3... A  drivers/net/e1000new/mac.c
:000000 100644 0000000... 075e326... A  drivers/net/e1000new/mac.h
:000000 100644 0000000... 3b6f6bc... A  drivers/net/e1000new/main.c
:000000 100644 0000000... e5fee2e... A  drivers/net/e1000new/manage.c
:000000 100644 0000000... 27606ea... A  drivers/net/e1000new/manage.h
:000000 100644 0000000... 2c9ccb4... A  drivers/net/e1000new/nvm.c
:000000 100644 0000000... e59c8ad... A  drivers/net/e1000new/nvm.h
:000000 100644 0000000... 3c943f1... A  drivers/net/e1000new/param.c
:000000 100644 0000000... 45338ef... A  drivers/net/e1000new/phy.c
:000000 100644 0000000... 957957d... A  drivers/net/e1000new/phy.h
:000000 100644 0000000... 669ad03... A  drivers/net/e1000new/regs.h
---


You can also get it through http:

http://foo-projects.org/~sofar/e1000new.patch      [883KB]
http://foo-projects.org/~sofar/e1000new.patch.bz2  [121KB]

Unfortunately it's too big to send directly to the list.

While I understand everyone's reservation for going a certain course forward, I 
would really appreciate it if people would review this driver and give it 
attention. Any feedback is appreciated, and whatever course we might end up 
going, can be used.

Thanks,

Auke

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Arjan van de Ven]

Kok, Auke wrote:
> Jeff Garzik wrote:
>> Andrew Morton wrote:
>>> On Fri, 29 Jun 2007 14:39:20 -0700
>>> "Kok, Auke" <auke-jan.h.kok@intel.com> wrote:
>>>
>>>> That's why we want to introduce a second e1000 driver (named 
>>>> differently, pick any name) that contains the new code base, 
>>>> side-by-side into the kernel with the current e1000.
>>> Sounds like a reasonable approach to me (it has plenty of 
>>> precedent).  But
>>> I forget what all the other issues were, so ignore me.
>>
>> Given past history with duplicate drivers and the problems that they 
>> cause -- I know, I've caused some of those problems :( -- I strongly 
>> recommend against when it can be avoided.
>>
>> Leaving e1000 with current hardware, and a new e1001 for newer 
>> hardware should be easier to manage for all involved, without the 
>> headaches that duplicate drivers cause.
>>

Jeff,

ok first you hate the old e1000 and now you don't want to get rid of it ;)

I appreciate the pain a temporary dual driver situation gives; it 
comes down to a few things that I can think of right now, if you see 
more please add to the list.

1) users who find a bug in the new one silently use the old one rather 
than reporting the bug; and only scream when the old one eventually 
goes away (see ALSA/OSS duplication)

2) users who enable both in KConfig may get a "random" one

3) distros really prefer only 1 driver per PCI ID for their 
infrastructure tools

4) there will be resistance against deleting the old one meaning it 
might not happen


While the pain won't be zero, I know there are some simple things that 
can be done to make it significantly less:

1) Make sure that the various feature-removal files and KConfig 
clearly mark the old one as "going away" early on, make the old one 
printk that it's the older driver and stick a hard date on it. This 
should at least ease [1] and [4] above

2) After a minimal amount of shake-out time (say one kernel release), 
make the old e1000 not export the PCI IDs in it's module, but make it 
a manual modprobe. This means people can still use the old one, but 
distro tools will pick the new one automatically
[and if there is any doubt about a specific PCI ID model temporarily, 
that one can be done the other way around]

3) Have a config option which does what you say; allow both drivers to 
be there but with non-overlapping PCI IDs; where exactly the new/old 
split is is up for discussion, and it can even move over time

4) Work with the distro maintainers to default their development 
snapshots as quickly as possible to the new driver so that it gets as 
much testing as possible quickly; yet at the same time for their 
security fixes and released updates they can use the 3) option

5) once there is some confidence in the new driver, put a patch into 
-mm that makes the old driver really hard to select in KConfig; this 
in preparation for removal later on (hey this worked for devfs ;)

6) put more or less a code freeze on the old driver; it should remain 
compiling but it's ok to be bug-to-bug compatible with what is there 
at the moment the new driver goes into the tree.


Personally I'm convinced this is a managable process with a reasonable 
assurance that the old driver really can go away after 2 kernel 
releases (as long as Auke and co are on top of bugreports about 
regressions, I don't see why not). What is needed of course is some 
level of strictness/determination to get rid of the old one after the 
reasonable time period.

if you have other concerns about temporarily having two drivers that 
are real showstoppers , please put them on the table; it might well be 
possible to find a reasonable way to solve/mitigate them.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Andrew Grover]

On 6/29/07, Jeff Garzik <jeff@garzik.org> wrote:
> Given past history with duplicate drivers and the problems that they
> cause -- I know, I've caused some of those problems :( -- I strongly
> recommend against when it can be avoided.
>
> Leaving e1000 with current hardware, and a new e1001 for newer hardware
> should be easier to manage for all involved, without the headaches that
> duplicate drivers cause.
>
> An "e1001" approach also means we have a much greater chance of
> encouraging Intel down the path of clean driver-ness.

When I was at Intel, I heard a lot of "the linux community won't let
us split the driver". "Oh well, guess we gotta keep lugging around all
the old crap." So it was a (perhaps misunderstood) desire to make you
guys happy that led to e1000 turning into a monster.

I think making e1000new ICH9-and-newer isn't really the best place to
split it. The Windows e1000 driver got split on the PCI->PCIe
transition, something that clearly delineated what nics one driver
supported, and the other. There's no real technical reason for
splitting now other than "this was when e1000old collapsed under its
own weight".

The PCIe adapters are also the first ones to support multiple queues
IIRC, maybe that would be an another actual technical reason to split
it there?

-- Andy

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Andrew Grover]

On 6/29/07, Andrew Grover <andy.grover@gmail.com> wrote:
> I think making e1000new ICH9-and-newer isn't really the best place to
> split it. The Windows e1000 driver got split on the PCI->PCIe
> transition, something that clearly delineated what nics one driver
> supported, and the other. There's no real technical reason for
> splitting now other than "this was when e1000old collapsed under its
> own weight".
>
> The PCIe adapters are also the first ones to support multiple queues
> IIRC, maybe that would be an another actual technical reason to split
> it there?

I agree with Jeff BTW, the list of devices each one supports should
have zero overlap.

-- Andy

Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Andrew Grover wrote:
> I think making e1000new ICH9-and-newer isn't really the best place to
> split it. The Windows e1000 driver got split on the PCI->PCIe
> transition, something that clearly delineated what nics one driver
> supported, and the other. There's no real technical reason for
> splitting now other than "this was when e1000old collapsed under its
> own weight".
> 
> The PCIe adapters are also the first ones to support multiple queues
> IIRC, maybe that would be an another actual technical reason to split
> it there?

Can knowledgeable people characterize the PCIe adapters somehow?  Is 
that "ICH8-era and later", or does it go back further than that?

	Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Jim McCullough]

[-- Attachment #1.1: Type: text/plain, Size: 1583 bytes --]

It goes back to ICH7 for the PCIe support.  That also includes models used
as plugin PCIe devices.  I dont remember if ICH6 erra devices were PCI or
PCIe.  I'll leave that part for Auke.

On 6/29/07, Jeff Garzik <jeff@garzik.org> wrote:
>
> Andrew Grover wrote:
> > I think making e1000new ICH9-and-newer isn't really the best place to
> > split it. The Windows e1000 driver got split on the PCI->PCIe
> > transition, something that clearly delineated what nics one driver
> > supported, and the other. There's no real technical reason for
> > splitting now other than "this was when e1000old collapsed under its
> > own weight".
> >
> > The PCIe adapters are also the first ones to support multiple queues
> > IIRC, maybe that would be an another actual technical reason to split
> > it there?
>
> Can knowledgeable people characterize the PCIe adapters somehow?  Is
> that "ICH8-era and later", or does it go back further than that?
>
>         Jeff
>
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> E1000-devel mailing list
> E1000-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/e1000-devel
>



-- 
Jim McCullough

"Just because the standard provides a cliff in front of you, you are not
necessarily required to jump off it."

    Norman Diamond

[-- Attachment #2: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel

Re: e1000: backport ich9 support from 7.5.5 ?

[Jim McCullough]

Sorry if this is a duplication, I forgot to switch to plain text.

On 6/29/07, Jim McCullough <jim.mccullough@gmail.com> wrote:
> It goes back to ICH7 for the PCIe support.  That also includes models used as plugin PCIe devices.  I dont remember if ICH6 erra devices were PCI or PCIe.  I'll leave that part for Auke.
>
>
>  On 6/29/07, Jeff Garzik <jeff@garzik.org> wrote:
>
> >  Andrew Grover wrote:
> > > I think making e1000new ICH9-and-newer isn't really the best place to
> > > split it. The Windows e1000 driver got split on the PCI->PCIe
> > > transition, something that clearly delineated what nics one driver
> > > supported, and the other. There's no real technical reason for
> > > splitting now other than "this was when e1000old collapsed under its
> > > own weight".
> > >
> > > The PCIe adapters are also the first ones to support multiple queues
> > > IIRC, maybe that would be an another actual technical reason to split
> > > it there?
> >
> > Can knowledgeable people characterize the PCIe adapters somehow?  Is
> > that "ICH8-era and later", or does it go back further than that?
> >
> >         Jeff
> >
> >
> >
> > -------------------------------------------------------------------------
> > This SF.net email is sponsored by DB2 Express
> > Download DB2 Express C - the FREE version of DB2 express and take
> > control of your XML. No limits. Just data. Click to get it now.
> > http://sourceforge.net/powerbar/db2/
> > _______________________________________________
> > E1000-devel mailing list
> > E1000-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/e1000-devel
> >
>
>
>
> --
> Jim McCullough
>
> "Just because the standard provides a cliff in front of you, you are not necessarily required to jump off it."
>
>     Norman Diamond



-- 
Jim McCullough

"Just because the standard provides a cliff in front of you, you are
not necessarily required to jump off it."

    Norman Diamond

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jeff Garzik wrote:
> Andrew Grover wrote:
>> I think making e1000new ICH9-and-newer isn't really the best place to
>> split it. The Windows e1000 driver got split on the PCI->PCIe
>> transition, something that clearly delineated what nics one driver
>> supported, and the other. There's no real technical reason for
>> splitting now other than "this was when e1000old collapsed under its
>> own weight".
>>
>> The PCIe adapters are also the first ones to support multiple queues
>> IIRC, maybe that would be an another actual technical reason to split
>> it there?
> 
> Can knowledgeable people characterize the PCIe adapters somehow?  Is 
> that "ICH8-era and later", or does it go back further than that?

very brief outline:


all the pci-express adapters that are supported are extremely similar:

- they all support 2 queues
- the register sets are (almost entirely) identical
- there is minimal feature variance between 82571/2/3, esb2lan, ich8/9

The major differences between 82571/2/3, esb2lan and ich8/9 are PHY-based (4 
different PHY's basically, one for 82571/2/3, one for esb2lan and 2 for ich8/9, 
excluding fiber and serdes here) and NVM/EEPROM.


ich8 and 9 are consistent with 82571/2/3 - on-board nic's based on the 82571 
design with different PHY's, and added features for the newer demands. A driver 
split here would be possible but not justified IMHO.


this above discussion excludes the new server NIC hardware that we are about to 
release (aka 82575), which will have it's own driver, since the register set and 
descriptor types are completely different.

if you want a quick view of features by chipset type, open the patch and search 
forward to /e1000_setup_flags/. The list below shows historically which features 
were added from time to time. This one function that sets up all these features 
is a quick reference that easily shows the distinction between pre-pcie and pcie 
e1000 hardware.


Auke

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [E1000-devel] e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jim McCullough wrote:
> It goes back to ICH7 for the PCIe support.  That also includes models used
> as plugin PCIe devices.  I dont remember if ICH6 erra devices were PCI or
> PCIe.  I'll leave that part for Auke.
> 
> On 6/29/07, Jeff Garzik <jeff@garzik.org> wrote:
>> Andrew Grover wrote:
>>> I think making e1000new ICH9-and-newer isn't really the best place to
>>> split it. The Windows e1000 driver got split on the PCI->PCIe
>>> transition, something that clearly delineated what nics one driver
>>> supported, and the other. There's no real technical reason for
>>> splitting now other than "this was when e1000old collapsed under its
>>> own weight".
>>>
>>> The PCIe adapters are also the first ones to support multiple queues
>>> IIRC, maybe that would be an another actual technical reason to split
>>> it there?
>> Can knowledgeable people characterize the PCIe adapters somehow?  Is
>> that "ICH8-era and later", or does it go back further than that?

ich7 and 6 are sold with various on-board NICs, often pre-pcie e1000 silicon 
like 82547 and 82546. Some newer models (lenovo, toshiba) laptops pack 82573, 
but basically ich chipsets before ich8 did not have an in-chipset MAC, so they 
are irrelevant for this story.

only es2lan, ich8 and ich9 have true on-board MAC's.

Auke

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Roland Dreier]

one possibility would be to merge e1000new with support only for chips
not supported by e1000, and semi-freeze e1000 (fixes only, new device
support goes into e1000new).  Then if there are some devices that are
more naturally supported by e1000new, we could later on merge patches
to move support for those devices from e1000 to e1000new.  Presumably
this would simplify e1000, since it would have to support a smaller
set of older devices.  e1000new would stay relatively clean too since
it wouldn't have to handle anything too old.

And at every stage of the process, any given NIC would have only one
driver in the tree.

 - R.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Christoph Hellwig]

On Fri, Jun 29, 2007 at 07:31:55PM -0700, Kok, Auke wrote:
> all the pci-express adapters that are supported are extremely similar:
> 
> - they all support 2 queues
> - the register sets are (almost entirely) identical
> - there is minimal feature variance between 82571/2/3, esb2lan, ich8/9
> 
> The major differences between 82571/2/3, esb2lan and ich8/9 are PHY-based 
> (4 different PHY's basically, one for 82571/2/3, one for esb2lan and 2 for 
> ich8/9, excluding fiber and serdes here) and NVM/EEPROM.
> 
> 
> ich8 and 9 are consistent with 82571/2/3 - on-board nic's based on the 
> 82571 design with different PHY's, and added features for the newer 
> demands. A driver split here would be possible but not justified IMHO.

Sounds like the perfect split to me.  I'd suggest you rip out support
for older hardware from your new driver and do the resulting simplification
and post a new e1000e driver for this hardware, removing existing support
from e1000 at the same time.  Later you can do the feature flags and similar
improvements to the old driver driver in an incremental fashion without the
burden of having to keep up with new hardware.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Christoph Hellwig]

On Fri, Jun 29, 2007 at 04:57:46PM -0700, Andrew Grover wrote:
> When I was at Intel, I heard a lot of "the linux community won't let
> us split the driver". "Oh well, guess we gotta keep lugging around all
> the old crap." So it was a (perhaps misunderstood) desire to make you
> guys happy that led to e1000 turning into a monster.

Where exactly is that crap coming from?  We've always told people to
split drivers if things go to divergent and there's some interesting
examples where the vendors has a monster driver and mainline has two
nicely split ones (sk98lin vs skge and sky2 for example).

Re: e1000: backport ich9 support from 7.5.5 ?

[James Chapman]

Kok, Auke wrote:
> Jeff Garzik wrote:
>> Can knowledgeable people characterize the PCIe adapters somehow?  Is 
>> that "ICH8-era and later", or does it go back further than that?
> 
> ich8 and 9 are consistent with 82571/2/3 - on-board nic's based on the 
> 82571 design with different PHY's, and added features for the newer 
> demands. A driver split here would be possible but not justified IMHO.
>
 > if you want a quick view of features by chipset type, open the patch
 > and search forward to /e1000_setup_flags/.

I briefly looked over your new driver. I think it might benefit by 
moving common parts into one or more libraries (or modules) and have 
separate chip-specific drivers for 82540, 82541, 82542, 82543, 82571 
etc. Put all the chip-specific bits in a chip-specific driver and have 
each driver call into the common (shared) code. The device 
probe/init/remove would be done by the chip-specific code, not the 
common code like it is now. When built as a module, the chip-specific 
parts and the common parts could be built as separate modules; modprobe 
would load the common parts automatically. Most of the code would be in 
the common part since these chips are very similar.

Doing the above would lead naturally to a series of patches which will 
make it easier to review. When Intel release a new device variant in the 
future, it might be supported by a new, small driver rather than needing 
modifications to one big monolith. Also, the kernel would be loaded with 
only the code it needed to support the specific device(s) present.

BTW, since you're doing a major update, would it be a good time to 
switch to phylib to manage your PHYs?

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

James Chapman wrote:
> Kok, Auke wrote:
>> Jeff Garzik wrote:
>>> Can knowledgeable people characterize the PCIe adapters somehow?  Is 
>>> that "ICH8-era and later", or does it go back further than that?
>> ich8 and 9 are consistent with 82571/2/3 - on-board nic's based on the 
>> 82571 design with different PHY's, and added features for the newer 
>> demands. A driver split here would be possible but not justified IMHO.
>>
>  > if you want a quick view of features by chipset type, open the patch
>  > and search forward to /e1000_setup_flags/.
> 
> I briefly looked over your new driver. I think it might benefit by 
> moving common parts into one or more libraries (or modules) and have 
> separate chip-specific drivers for 82540, 82541, 82542, 82543, 82571 
> etc. Put all the chip-specific bits in a chip-specific driver and have 
> each driver call into the common (shared) code. The device 
> probe/init/remove would be done by the chip-specific code, not the 
> common code like it is now. When built as a module, the chip-specific 
> parts and the common parts could be built as separate modules; modprobe 
> would load the common parts automatically. Most of the code would be in 
> the common part since these chips are very similar.

splitting beyond the obvious does not make any sense and will just add 
confusion. I'm not against a pcie/pre-pcie split or even going a step further 
and looking into using PHYlib as you suggest below, but we should really avoid 
trying to create an unmaintainable mess where we have to patch 7 drivers for 
each generic issue we find.

> Doing the above would lead naturally to a series of patches which will 
> make it easier to review. When Intel release a new device variant in the 
> future, it might be supported by a new, small driver rather than needing 
> modifications to one big monolith. Also, the kernel would be loaded with 
> only the code it needed to support the specific device(s) present.
> 
> BTW, since you're doing a major update, would it be a good time to 
> switch to phylib to manage your PHYs?

well, PHYlib might help, so I'm not negative against doing that right now. It 
will be quite some work.

Auke

Re: e1000: backport ich9 support from 7.5.5 ?

[Mark McLoughlin]

Hi Auke,

On Fri, 2007-06-29 at 12:51 -0700, Kok, Auke wrote:
> Jason Lunz wrote:
> > On Fri, Jun 29, 2007 at 06:29:18PM +0100, Mark McLoughlin wrote:
> >> 	I understand there is some delay in getting e1000-7.5.5 into the
> >> upstream kernel given the major re-working of the chipset specific
> >> parts.
> >>
> >> 	I wonder would it be feasible in the meantime to backport the ich9
> >> support and push it upstream?
> > 
> > seconded - this driver reorg has been holding back support for the
> > newest e1000 hardware since at least 2.6.20, and I haven't seen any
> > indication that the 7.5.5 e1000 driver will even be added to 2.6.23.
> 
> I disagree, we should not break the current e1000 driver in the kernel while 
> there is a new driver coming up that introduces ich9 support without breaking 
> (the old e1000) support for all other devices. This is why we want to drop a new 
> version of the e1000 driver upstream instead, and put all newer devices in that 
> driver.

	Clearly some major changes are happening around e1000, but the point
I'm making is that ich9 support, in particular, doesn't need to be held
hostage to these longer term changes. I think the backport shows that:

  - ich9 differs only very slightly from ich8, which is already 
    supported upstream. The patch largely consists of:

-    if (hw->mac_type == e1000_ich8lan)
+    if (hw->mac_type == e1000_ich8lan || hw->mac_type == e1000_ich9lan)

  - adding ich9 support is very unlikely to affect support for any
    other chipsets

Thanks,
Mark.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[James Chapman]

Kok, Auke wrote:
> James Chapman wrote:
>> I briefly looked over your new driver. I think it might benefit by 
>> moving common parts into one or more libraries (or modules) and have 
>> separate chip-specific drivers for 82540, 82541, 82542, 82543, 82571 
>> etc. Put all the chip-specific bits in a chip-specific driver and have 
>> each driver call into the common (shared) code. The device 
>> probe/init/remove would be done by the chip-specific code, not the 
>> common code like it is now. When built as a module, the chip-specific 
>> parts and the common parts could be built as separate modules; 
>> modprobe would load the common parts automatically. Most of the code 
>> would be in the common part since these chips are very similar.
> 
> splitting beyond the obvious does not make any sense and will just add 
> confusion.

Users wouldn't need to know which driver to load - the right driver 
would be loaded automatically. Distros would ship all of them (and the 
common bits). The few users who know which device(s) they have could 
build only the required drivers if they wanted to do so.

> I'm not against a pcie/pre-pcie split or even going a step 
> further and looking into using PHYlib as you suggest below, but we 
> should really avoid trying to create an unmaintainable mess where we 
> have to patch 7 drivers for each generic issue we find.

The generic issues would be in the common part so would be patched once.

Looking at your new driver code, there are separate source files for the 
5 chips I mention, hence my leaning towards a driver per device. I 
counted 24 funcptrs that are set up in order for the common code to call 
out to driver-private chip specific functions. Wouldn't most of these go 
away if the chip-specific code called common code, not the other way round?

Let me use an example to illustrate what I mean. e1000_setup_link() is 
common code. It simply calls a function pointed to by func.setup_link, 
which was initialized to a chip-specific function, e.g. 
e1000_setup_link_82543() in e1000_init_mac_params_82543(). It turns out 
that e1000_setup_link() is called by chip-specific code from 
e1000_init_hw_82543() so the abstraction through e1000_setup_link() 
wouldn't be needed for init. However, e1000_setup_link() is also called 
from the common driver/ethtool entry points e1000_open() and 
e1000_set_pause_params(). These entry points could be moved in to the 
chip specific code, calling out to common code when necessary: the 
funcptrs/abstractions wouldn't then be needed. This would obviously lead 
to some common boilerplate code in each chip specific driver for the 
driver entry points open/close etc and occasionally some common changes 
might be needed in that code. However, I'm sure most bugs will be in the 
guts of the driver, not the boilerplate code so the duplication of the 
driver boilerplate code wouldn't matter so much. I think maintenance 
effort would actually reduce with this kind of structure.

I don't want to sound negative though; it's great that you and Intel are 
putting a lot of work into this driver. You know much more about the 
actual chip feature differences/workarounds than I do so if you don't 
think the approach I suggest will work, I'm happy to just drop this thread.

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Williams, Mitch A]

 

Mark McLoughlin wrote:

>> I disagree, we should not break the current e1000 driver in 
>the kernel while 
>> there is a new driver coming up that introduces ich9 support 
>without breaking 
>> (the old e1000) support for all other devices. This is why 
>we want to drop a new 
>> version of the e1000 driver upstream instead, and put all 
>newer devices in that 
>> driver.
>
>	Clearly some major changes are happening around e1000, 
>but the point
>I'm making is that ich9 support, in particular, doesn't need to be held
>hostage to these longer term changes. I think the backport shows that:
>

There seems to be a lot of concern over obsoleting the e1000 driver
too quickly, and with confusing users (and startup scripts) about which
driver to load.

Obviously, we at Intel want to get e1000new into the kernel as quickly
as possible, and to obsolete e1000 also as quickly as possible.  The
point of this exercise is to reduce our support and maintenance burden,
and e1000new has shown itself internally to be much easier to work on.

So how about this:
- We include e1000new in 2.6.23, along side e1000.  We expose ICH9
  device IDs in e1000new, and gate the rest of the IDs inside
  #ifndef CONFIG_E1000.  This gives distis ICH9 support, along
  with a guarantee of the known e1000 driver.  It also lets the more
  bleeding-edge people turn off e1000 and run with just e1000new to
  work out any kinks.
- For 2.6.24, we reverse the situation:  Gate all device IDs in e1000
  behind #ifndef CONFIG_E1000NEW, and expose all of them in e1000new.
  At this point we (i.e. the community, not just Intel) should be 
  confident in e1000new, and we can mark e1000 as obsolete.  It's still
  a fallback option for those with old/funky/misconfigured hardware.
- After a year (or whatever), we remove e1000.

Seems to me that this plan should appease either everybody or nobody.
We get ICH9 support out there, e1000new gets in the kernel and
exercised,
and we get to set a definite date for obsoleting e1000.

-Mitch

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Rick Jones]

> There seems to be a lot of concern over obsoleting the e1000 driver
> too quickly, and with confusing users (and startup scripts) about which
> driver to load.

Yes.

> Obviously, we at Intel want to get e1000new into the kernel as quickly
> as possible, and to obsolete e1000 also as quickly as possible.  The
> point of this exercise is to reduce our support and maintenance burden,
> and e1000new has shown itself internally to be much easier to work on.
> 
> So how about this:
> - We include e1000new in 2.6.23, along side e1000.  We expose ICH9
>   device IDs in e1000new, and gate the rest of the IDs inside
>   #ifndef CONFIG_E1000.  This gives distis ICH9 support, along
>   with a guarantee of the known e1000 driver.  It also lets the more
>   bleeding-edge people turn off e1000 and run with just e1000new to
>   work out any kinks.
> - For 2.6.24, we reverse the situation:  Gate all device IDs in e1000
>   behind #ifndef CONFIG_E1000NEW, and expose all of them in e1000new.
>   At this point we (i.e. the community, not just Intel) should be 
>   confident in e1000new, and we can mark e1000 as obsolete.  It's still
>   a fallback option for those with old/funky/misconfigured hardware.
> - After a year (or whatever), we remove e1000.
> 
> Seems to me that this plan should appease either everybody or nobody.
> We get ICH9 support out there, e1000new gets in the kernel and
> exercised, and we get to set a definite date for obsoleting e1000.

What sort of timeframes are we looking at with 2.6.23 and then 2.6.24 
and how might they map to distro releases?  Much as everyone 
wants/encourages folks to test the kernel.org kernels and such, the 
_real_ exposure still seems to come with a distro release.

Rambling a bit, and recognizing that "e1000new" was probably just a 
strawman name, but I suspect that a _very_ different name for the new 
driver would be a good thing, rather than a varition on the e1000 theme. 
  "The Law of the Telephone Game" pretty much guaratees that something 
with "e1000" in its name will be shortened to just "e1000."

How about "elk" (ee el kay) - applying that same "telephone" game 
(consistency? nah) to e1000 to get e1k, which if you look at it quickly 
enough looks like elk.  OK, that's half in jest, but only half.  I may 
be wrong, but I don't think I've seen anyone shortening the current 
e1000 to e1k?

rick jones

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Jason Lunz]

On Mon, Jul 02, 2007 at 05:10:48PM -0700, Rick Jones wrote:
> Rambling a bit, and recognizing that "e1000new" was probably just a 
> strawman name, but I suspect that a _very_ different name for the new 
> driver would be a good thing, rather than a varition on the e1000 theme. 

I rather liked the "e1000e" name suggested by Christoph Hellwig. That
may not be different enough for you, though. Anything but "e1000new";
it won't be new forever.

Jason

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jason Lunz wrote:
> On Mon, Jul 02, 2007 at 05:10:48PM -0700, Rick Jones wrote:
>> Rambling a bit, and recognizing that "e1000new" was probably just a 
>> strawman name, but I suspect that a _very_ different name for the new 
>> driver would be a good thing, rather than a varition on the e1000 theme. 
> 
> I rather liked the "e1000e" name suggested by Christoph Hellwig. That
> may not be different enough for you, though. Anything but "e1000new";
> it won't be new forever.

hehe, yes, true. And the e1000new was just a placeholder. e1000e seems much more 
attractive for me, especially if we end up splitting the driver at the pci/pci-e 
boundary (like ixgb and ixgbe are).

Auke



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: e1000: backport ich9 support from 7.5.5 ?

[Christoph Hellwig]

On Mon, Jul 02, 2007 at 04:52:52PM -0700, Williams, Mitch A wrote:
> - We include e1000new in 2.6.23, along side e1000.  We expose ICH9
>   device IDs in e1000new, and gate the rest of the IDs inside
>   #ifndef CONFIG_E1000.

No.  Hardware support in one driver should never be affected by config
options in another drivers.

Also I really think there shouldn't be one single driver for all the
hardware as outlined by intel people in this thread.  I'd say add a new
e1000e driver for all the pci-e hardware and have a temporary config
option to disable those device already supported by the existing e1000
driver.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [E1000-devel] e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Christoph Hellwig wrote:
> On Mon, Jul 02, 2007 at 04:52:52PM -0700, Williams, Mitch A wrote:
>> - We include e1000new in 2.6.23, along side e1000.  We expose ICH9
>>   device IDs in e1000new, and gate the rest of the IDs inside
>>   #ifndef CONFIG_E1000.
> 
> No.  Hardware support in one driver should never be affected by config
> options in another drivers.

Agreed.


> Also I really think there shouldn't be one single driver for all the
> hardware as outlined by intel people in this thread.

Agreed.

(as I've been saying)

	Jeff

Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Christoph Hellwig wrote:
> On Fri, Jun 29, 2007 at 07:31:55PM -0700, Kok, Auke wrote:
>> all the pci-express adapters that are supported are extremely similar:
>>
>> - they all support 2 queues
>> - the register sets are (almost entirely) identical
>> - there is minimal feature variance between 82571/2/3, esb2lan, ich8/9
>>
>> The major differences between 82571/2/3, esb2lan and ich8/9 are PHY-based 
>> (4 different PHY's basically, one for 82571/2/3, one for esb2lan and 2 for 
>> ich8/9, excluding fiber and serdes here) and NVM/EEPROM.
>>
>> ich8 and 9 are consistent with 82571/2/3 - on-board nic's based on the 
>> 82571 design with different PHY's, and added features for the newer 
>> demands. A driver split here would be possible but not justified IMHO.
> 
> Sounds like the perfect split to me.  I'd suggest you rip out support
> for older hardware from your new driver and do the resulting simplification
> and post a new e1000e driver for this hardware, removing existing support
> from e1000 at the same time.  Later you can do the feature flags and similar
> improvements to the old driver driver in an incremental fashion without the
> burden of having to keep up with new hardware.

Jeff,

it seems that this is the preferred path to go including "e1000e" as the new 
driver name for all 8257x family based adapters. Just for the record I'd like 
your acknowledgement on the following plan:


1a) We post an e1000e driver that implements support for all 8257x (ich8/9, 
es2lan etc) devices.
1b) We post a patch that drops support for all of these devices in the form of a 
pci-ID removal (no code removed) for e1000.

2) we post patches that remove code support for non-8254x devices at a later stage.

3) we backport any and all cleanups and flags from e1000e to e1000 where applicable.


This plan leaves a significant gap that I'm worrying about: after step (1) we 
basically have forced everyone to switch without providing a fallback (allthough 
we have our out-of-tree driver, but no in-kernel version in case issues exist).


Comments?


I have no idea how internally this is going to get accepted, there will 
certainly be some fireworks around here soon :).


Auke

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Kok, Auke wrote:
> Christoph Hellwig wrote:
>> On Fri, Jun 29, 2007 at 07:31:55PM -0700, Kok, Auke wrote:
>>> all the pci-express adapters that are supported are extremely similar:
>>>
>>> - they all support 2 queues
>>> - the register sets are (almost entirely) identical
>>> - there is minimal feature variance between 82571/2/3, esb2lan, ich8/9
>>>
>>> The major differences between 82571/2/3, esb2lan and ich8/9 are PHY-based 
>>> (4 different PHY's basically, one for 82571/2/3, one for esb2lan and 2 for 
>>> ich8/9, excluding fiber and serdes here) and NVM/EEPROM.
>>>
>>> ich8 and 9 are consistent with 82571/2/3 - on-board nic's based on the 
>>> 82571 design with different PHY's, and added features for the newer 
>>> demands. A driver split here would be possible but not justified IMHO.
>> Sounds like the perfect split to me.  I'd suggest you rip out support
>> for older hardware from your new driver and do the resulting simplification
>> and post a new e1000e driver for this hardware, removing existing support
>> from e1000 at the same time.  Later you can do the feature flags and similar
>> improvements to the old driver driver in an incremental fashion without the
>> burden of having to keep up with new hardware.
> 
> Jeff,
> 
> it seems that this is the preferred path to go including "e1000e" as the new 
> driver name for all 8257x family based adapters. Just for the record I'd like 
> your acknowledgement on the following plan:
> 
> 
> 1a) We post an e1000e driver that implements support for all 8257x (ich8/9, 
> es2lan etc) devices.
> 1b) We post a patch that drops support for all of these devices in the form of a 
> pci-ID removal (no code removed) for e1000.
> 
> 2) we post patches that remove code support for non-8254x devices at a later stage.
> 
> 3) we backport any and all cleanups and flags from e1000e to e1000 where applicable.
> 
> 
> This plan leaves a significant gap that I'm worrying about: after step (1) we 
> basically have forced everyone to switch without providing a fallback (allthough 
> we have our out-of-tree driver, but no in-kernel version in case issues exist).

Actually, this plan temporarily allows users to manually bind "e1000" to 
pci-express adapters in case they wish to do so, thereby providing a fallback 
driver for everyone.

If we leave the "e1000" driver untouched (not removing code support for pci-e 
adapters) for at least a full kernel release, this should be reasonable for 
everyone I hope. After we have gained some confidence in "e1000e" we can start 
removing code from "e1000" for pci-e adapters.

How does that sound?

Auke

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Kok, Auke wrote:
> 1a) We post an e1000e driver that implements support for all 8257x 
> (ich8/9, es2lan etc) devices.
> 1b) We post a patch that drops support for all of these devices in the 
> form of a pci-ID removal (no code removed) for e1000.
> 
> 2) we post patches that remove code support for non-8254x devices at a 
> later stage.
> 
> 3) we backport any and all cleanups and flags from e1000e to e1000 where 
> applicable.
> 
> 
> This plan leaves a significant gap that I'm worrying about: after step 
> (1) we basically have forced everyone to switch without providing a 
> fallback (allthough we have our out-of-tree driver, but no in-kernel 
> version in case issues exist).
> 
> 
> Comments?

I like the general gist of it.  My own suggestion would be

1) Create e1001.c, which is the SMALLEST POSSIBLE "it works" driver for 
ICH9.  No feature enablement (no TSO, no MQ, no IOAT, not even checksum 
offload), just a rock solid, no frills driver.  Think like e100.c :)

2) We review the hell out of e1001, and merge it.  This enables ICH9 
users, and ONLY ICH9 users, on e1001 in the upstream kernel, allowing us 
to blithely ignore driver->driver migration issues present with other chips.


At this point, e1001 is out in the field and in the upstream kernel in 
the absolute shortest amount of time.  Users of chips NOT found in 
current e1000 driver, and all future chips, can be enabled with little 
impediments, in parallel with the steps below.

(all the "3^y" steps can occur in parallel)

3^1) Add features to e1001, enabling new gizmos on new hardware.  This 
can proceed in parallel with any e1000 work.  This allows users to use 
git-bisect to find driver bugs -- or even hardware bugs, since you have 
a baseline working e1001 driver.

3^2) Add full ICH8 (8257x?) support to e1001.

4) Drop 8257x PCI IDs from e1000.  See who complains.  Lather, rinse, 
repeat.

5) Figure out how the hell to clean up the current mess that is e1000, 
and how to make sure e1000 and e1001 stay clean, long term.

	Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

OK, just looked through the driver.  I think its structured inside-out 
from what it should be.

Comments:

* is a clear improvement from current e1000

* The multitude of tiny, fine-grained operations for MAC, NVM, PHY, etc. 
is a signal that organization is backwards.  You should be creating 
hardware-specific high level operations (PHY layer hooks, net_device 
hooks, interrupt handler) that call out to more-generic functions when 
necessary.  Doing so eliminates the need to create a new hook for every 
little twirl in the code path.

In the long run, a driver is easier to maintain if you can easily follow 
the code path for a particular hardware generation.  Creating 
e1001_8257x_do_this_thing(), which calls more generic code as needed, is 
easier to review and doesn't require all sorts of indirection through APIs.

Doing so also means that many workarounds for older hardware "disappear" 
from the most-travelled code paths over time.  The 64k boundary check 
found in e1000new is an easy example of something that really shouldn't 
pollute newer code at all [yes, even though it reduces to 'return 1' for 
most].

* The multitude of files makes it difficult to review.  Much easier in 
one file, or a small few.

* bitfields

* check for PCI DMA mapping failure

* atomic_t irq_sem is reinventing the wheel (and too heavy for you 
needs, too?).  You might as well use a lock or mutex or whatnot at that 
point, since you are using a locked instruction.  tg3 might also have 
some hints in this regard.

* like I noted in the last email, the quickest path to upstream is to 
start SMALL.  Create the smallest working driver, review it heavily, get 
it upstream.  Add all hardware&features after that.

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Kok, Auke]

Jeff Garzik wrote:
> OK, just looked through the driver.  I think its structured inside-out 
> from what it should be.
> 
> Comments:
> 
> * is a clear improvement from current e1000
> 
> * The multitude of tiny, fine-grained operations for MAC, NVM, PHY, etc. 
> is a signal that organization is backwards.  You should be creating 
> hardware-specific high level operations (PHY layer hooks, net_device 
> hooks, interrupt handler) that call out to more-generic functions when 
> necessary.  Doing so eliminates the need to create a new hook for every 
> little twirl in the code path.
> 
> In the long run, a driver is easier to maintain if you can easily follow 
> the code path for a particular hardware generation.  Creating 
> e1001_8257x_do_this_thing(), which calls more generic code as needed, is 
> easier to review and doesn't require all sorts of indirection through APIs.
> 
> Doing so also means that many workarounds for older hardware "disappear" 
> from the most-travelled code paths over time.  The 64k boundary check 
> found in e1000new is an easy example of something that really shouldn't 
> pollute newer code at all [yes, even though it reduces to 'return 1' for 
> most].

are you talking about the internals of e1000_phy/mac/nvm etc? i agree that the 
amount of forward/backward mapping in here is a bit of a spaghetti and could be 
more clear

> * The multitude of files makes it difficult to review.  Much easier in 
> one file, or a small few.

well, at least the files are reasonably well structured by topic. Combining 
small files just to make reviewing easier seems strange to me, besides making it 
easier to jump around in an editor it doesn't add any value to the code 
organization, and just encourages more forward declarations and horrible 
ordering issues.

> * bitfields

consider these gone ;)

> * check for PCI DMA mapping failure

*draws blank* specific one? I'm not seeing what you mean here

> * atomic_t irq_sem is reinventing the wheel (and too heavy for you 
> needs, too?).  You might as well use a lock or mutex or whatnot at that 
> point, since you are using a locked instruction.  tg3 might also have 
> some hints in this regard.

absolutely, I really don't like irq_sem and several people insist even that it 
can be removed (allthough I've demonstrated that plain removing it actually 
breaks on pci-e adapters). I had left it in since it currently works fine, but 
it's high on my list to fix.

> * like I noted in the last email, the quickest path to upstream is to 
> start SMALL.  Create the smallest working driver, review it heavily, get 
> it upstream.  Add all hardware&features after that.

In this line, I will try to drop things like multiqueue from it completely to 
avoid making it over-complex (and not used at all right now), as is the case in 
a lot of points right now.

Thanks for reviewing.

Auke

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Jeff Garzik wrote:
> Kok, Auke wrote:
>> 1a) We post an e1000e driver that implements support for all 8257x 
>> (ich8/9, es2lan etc) devices.
>> 1b) We post a patch that drops support for all of these devices in the 
>> form of a pci-ID removal (no code removed) for e1000.
>>
>> 2) we post patches that remove code support for non-8254x devices at a 
>> later stage.
>>
>> 3) we backport any and all cleanups and flags from e1000e to e1000 where 
>> applicable.
>>
>>
>> This plan leaves a significant gap that I'm worrying about: after step 
>> (1) we basically have forced everyone to switch without providing a 
>> fallback (allthough we have our out-of-tree driver, but no in-kernel 
>> version in case issues exist).
>>
>>
>> Comments?
> 
> I like the general gist of it.  My own suggestion would be
> 
> 1) Create e1001.c, which is the SMALLEST POSSIBLE "it works" driver for 
> ICH9.  No feature enablement (no TSO, no MQ, no IOAT, not even checksum 
> offload), just a rock solid, no frills driver.  Think like e100.c :)
> 
> 2) We review the hell out of e1001, and merge it.  This enables ICH9 
> users, and ONLY ICH9 users, on e1001 in the upstream kernel, allowing us 
> to blithely ignore driver->driver migration issues present with other chips.
> 
> 
> At this point, e1001 is out in the field and in the upstream kernel in 
> the absolute shortest amount of time.  Users of chips NOT found in 
> current e1000 driver, and all future chips, can be enabled with little 
> impediments, in parallel with the steps below.
> 
> (all the "3^y" steps can occur in parallel)
> 
> 3^1) Add features to e1001, enabling new gizmos on new hardware.  This 
> can proceed in parallel with any e1000 work.  This allows users to use 
> git-bisect to find driver bugs -- or even hardware bugs, since you have 
> a baseline working e1001 driver.
> 
> 3^2) Add full ICH8 (8257x?) support to e1001.
> 
> 4) Drop 8257x PCI IDs from e1000.  See who complains.  Lather, rinse, 
> repeat.
> 
> 5) Figure out how the hell to clean up the current mess that is e1000, 
> and how to make sure e1000 and e1001 stay clean, long term.


This is not acceptable and hardly fair to expect from us.

It also exposes users to endless delays and uncertainties as to a final 
resolution. Not to mention that writing a driver from scratch for (just) ich9 
will take significant time, is silly since it's almost identical to ich8 etc..

I don't think that anyone besides you and maybe one or two others are interested 
in doing this rewrite from scratch. The rest of us would rather see something 
much more similar to my original suggestion which relieves the ich9-wishes for 
everyone and provides a good starting point to go forward. Not to mention that a 
lot of that code is already there, and at least cleaned up quite a bit already.


Auke

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[James Chapman]

Kok, Auke wrote:
> Jeff Garzik wrote:
>> * The multitude of tiny, fine-grained operations for MAC, NVM, PHY, 
>> etc. is a signal that organization is backwards.  You should be 
>> creating hardware-specific high level operations (PHY layer hooks, 
>> net_device hooks, interrupt handler) that call out to more-generic 
>> functions when necessary.  Doing so eliminates the need to create a 
>> new hook for every little twirl in the code path.

I think this is the key point that needs to be addressed in the new 
driver. All of the netdevice glue such as PCI ID tables, netdev open, 
close etc should be in its own driver for each device, which calls out 
to common code where appropriate.

> are you talking about the internals of e1000_phy/mac/nvm etc? i agree 
> that the amount of forward/backward mapping in here is a bit of a 
> spaghetti and could be more clear

Make it more clear by restructuring it, not by adding comments though.

>> * The multitude of files makes it difficult to review.  Much easier in 
>> one file, or a small few.
> 
> well, at least the files are reasonably well structured by topic. 
> Combining small files just to make reviewing easier seems strange to me, 
> besides making it easier to jump around in an editor it doesn't add any 
> value to the code organization, and just encourages more forward 
> declarations and horrible ordering issues.

There's a lot of common code across the e1000 family. But all of the 
chip-specific code for an individual driver could be in one file. I 
envisage something like this:-

e1000_core.c	- common code used by all drivers of the e1000 family.
		  Exports functions used by actual drivers. Loadable
		  as a separate module when built as a module.
e1000_82541.c	- driver for 82541
e1000_82542.c	- driver for 82542
e1000_xxxxx.c	- driver for xxxxx

If phylib were used, the phy-specific parts would move out naturally 
into a phy driver for each phy device. And if e1000_core.c were 
implemented well, few changes should occur over time as more e1000 
devices are released.

There would obviously be no overlap of PCI IDs in each driver. For the 
initial version, support just one device - more can be added later. The 
important thing is to get the structure right in the initial version.

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[James Chapman]

Kok, Auke wrote:
> I don't think that anyone besides you and maybe one or two others are 
> interested in doing this rewrite from scratch. 

I count myself as one of those others. :) I don't think it's a rewrite 
though; it's a rework.

> The rest of us would 
> rather see something much more similar to my original suggestion which 
> relieves the ich9-wishes for everyone and provides a good starting point 
> to go forward. Not to mention that a lot of that code is already there, 
> and at least cleaned up quite a bit already.

But the structure of it isn't right, as mentioned by Jeff and I in 
previous posts. Any restructuring of your new driver will obviously take 
time. In my view using the urgency of needing ich9 support to get the 
new driver in isn't the right tradeoff. Perhaps the ich9 code should be 
backported into the existing e1000, which is where this thread started? 
This will buy time to restructure the new driver to make sure it is 
right for the future.

I agree with Jeff that the initial version of the new driver should have 
only essential features - bells and whistles can be added incrementally.

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Andrew Grover]

On 7/6/07, Jeff Garzik <jeff@garzik.org> wrote:
> OK, just looked through the driver.  I think its structured inside-out
> from what it should be.

> * The multitude of tiny, fine-grained operations for MAC, NVM, PHY, etc.
> is a signal that organization is backwards.  You should be creating
> hardware-specific high level operations (PHY layer hooks, net_device
> hooks, interrupt handler) that call out to more-generic functions when
> necessary.  Doing so eliminates the need to create a new hook for every
> little twirl in the code path.

I think the nice thing about the driver's organization is that it
gives a clear overview of how the driver works, without delving into
generation-specific details. This fixes a problem that the old driver
had, where it was very easy to get lost in the woods of
family-specific workarounds.

> In the long run, a driver is easier to maintain if you can easily follow
> the code path for a particular hardware generation.  Creating
> e1001_8257x_do_this_thing(), which calls more generic code as needed, is
> easier to review and doesn't require all sorts of indirection through APIs.

Basically make a library of e1000-routines. The alternative is a
polymorphic approach (i.e. using function pointers so generic code can
call specific code). Both approaches are used extensively in the
kernel -- I happen to think the latter approach is better suited to
the e1000's current issue.

(For what is a driver but specific code called from generic code? This
just reuses this helpful design pattern at a lower level.)

> Doing so also means that many workarounds for older hardware "disappear"
> from the most-travelled code paths over time.  The 64k boundary check
> found in e1000new is an easy example of something that really shouldn't
> pollute newer code at all [yes, even though it reduces to 'return 1' for
> most].

Given that the new driver would only support PCIe devices (i.e.
relatively new/good HW) I would think this would cut down on the
"workaround" hooks, leaving the "HW is genuinely different" hooks.

> * The multitude of files makes it difficult to review.  Much easier in
> one file, or a small few.

Removing support for pre-PCIe in the new driver would help alleviate this.

Regards -- Andy

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Francois Romieu]

Kok, Auke <auke-jan.h.kok@intel.com> :
> Jeff Garzik wrote:
> >Kok, Auke wrote:
[...]
> This is not acceptable and hardly fair to expect from us.
> 
> It also exposes users to endless delays and uncertainties as to a final 
> resolution. Not to mention that writing a driver from scratch for (just) 
> ich9 will take significant time, is silly since it's almost identical to 
> ich8 etc..
> 
> I don't think that anyone besides you and maybe one or two others are 
> interested in doing this rewrite from scratch.

So far I'd say him and at least two others. Is it time to count them ?

Everybody is cool and polite but after a week of discussion this is
going nowhere.

-- 
Ueimor

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Francois Romieu wrote:
> Kok, Auke <auke-jan.h.kok@intel.com> :
>> Jeff Garzik wrote:
>>> Kok, Auke wrote:
> [...]
>> This is not acceptable and hardly fair to expect from us.
>>
>> It also exposes users to endless delays and uncertainties as to a final 
>> resolution. Not to mention that writing a driver from scratch for (just) 
>> ich9 will take significant time, is silly since it's almost identical to 
>> ich8 etc..
>>
>> I don't think that anyone besides you and maybe one or two others are 
>> interested in doing this rewrite from scratch.
> 
> So far I'd say him and at least two others. Is it time to count them ?
> 
> Everybody is cool and polite but after a week of discussion this is
> going nowhere.

It is really hard to stay polite for me now. After we were told to clean up the 
driver and implement things like feature flags and organize all the various 
hardware dependent bits, we did exactly that. And now we're told to rewrite the 
driver from scratch. ("thanks for the effort but no thanks, do this instead").

If that does not make your hair on the back of your neck stand up...

I politely suggested a middle way that would allows us to continue working on 
the driver and take it to the next step. This would in my opinion be the least 
destructive plan and get us moving forward quickly. Jeff's alternative plan can 
set us back another year. Perhaps it won't, but I fear that it will, and it will 
  be a hard thing to sell to my group and other parties.

I agree that rewriting drivers that are bad from scratch is a good thing. But in 
this case we're talking about a driver with a good reputation that has 
functioned really well for everyone for as long as it's out there. Perhaps you 
disagree on this, but lets just agree on that e1000 never really broke in the 
last 7 years even though we added over 50 different type of adapters to it, and 
it's still performance wise a good driver.

As a matter of fact, I find it hard to believe that e1000 has a bad reputation. 
Almost all of the direct feedback that I get from people using e1000 is 
positive. We regularly get good feedback on our response rate too.

We as a group are committed to keep repairing and improving e1000 and I think 
that we have shown our good intent with some of the stuff that we've recently 
shown. I think it's also polite to let us do our work and work *with* us, 
instead of dumping our ideas ("I like the gist of it, but") and basically 
sending us off with nothing. And that means that the linux community also is 
stuck with nothing.

I would really like to continue with my original plan that I posted that follows 
Christoph's idea. I hope you can all agree with that so we can get on with this.


Auke

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Stephen Hemminger]

On Sat, 07 Jul 2007 14:54:11 -0700
"Kok, Auke" <auke-jan.h.kok@intel.com> wrote:

> Francois Romieu wrote:
> > Kok, Auke <auke-jan.h.kok@intel.com> :
> >> Jeff Garzik wrote:
> >>> Kok, Auke wrote:
> > [...]
> >> This is not acceptable and hardly fair to expect from us.
> >>
> >> It also exposes users to endless delays and uncertainties as to a final 
> >> resolution. Not to mention that writing a driver from scratch for (just) 
> >> ich9 will take significant time, is silly since it's almost identical to 
> >> ich8 etc..
> >>
> >> I don't think that anyone besides you and maybe one or two others are 
> >> interested in doing this rewrite from scratch.
> > 
> > So far I'd say him and at least two others. Is it time to count them ?
> > 
> > Everybody is cool and polite but after a week of discussion this is
> > going nowhere.
> 
> It is really hard to stay polite for me now. After we were told to clean up the 
> driver and implement things like feature flags and organize all the various 
> hardware dependent bits, we did exactly that. And now we're told to rewrite the 
> driver from scratch. ("thanks for the effort but no thanks, do this instead").
> 
> If that does not make your hair on the back of your neck stand up...
> 
> I politely suggested a middle way that would allows us to continue working on 
> the driver and take it to the next step. This would in my opinion be the least 
> destructive plan and get us moving forward quickly. Jeff's alternative plan can 
> set us back another year. Perhaps it won't, but I fear that it will, and it will 
>   be a hard thing to sell to my group and other parties.
> 
> I agree that rewriting drivers that are bad from scratch is a good thing. But in 
> this case we're talking about a driver with a good reputation that has 
> functioned really well for everyone for as long as it's out there. Perhaps you 
> disagree on this, but lets just agree on that e1000 never really broke in the 
> last 7 years even though we added over 50 different type of adapters to it, and 
> it's still performance wise a good driver.
> 
> As a matter of fact, I find it hard to believe that e1000 has a bad reputation. 
> Almost all of the direct feedback that I get from people using e1000 is 
> positive. We regularly get good feedback on our response rate too.
> 
> We as a group are committed to keep repairing and improving e1000 and I think 
> that we have shown our good intent with some of the stuff that we've recently 
> shown. I think it's also polite to let us do our work and work *with* us, 
> instead of dumping our ideas ("I like the gist of it, but") and basically 
> sending us off with nothing. And that means that the linux community also is 
> stuck with nothing.
> 
> I would really like to continue with my original plan that I posted that follows 
> Christoph's idea. I hope you can all agree with that so we can get on with this.

I think rather than having a meta-discussion, which always seems to degenerate
into finger pointing. Let's look at the code. The problem with popular drivers
(as I too well know) is that user's seem to find every wart.  There are lots of
old drivers that never seem to get the same scrutiny, and if they did would
be tarred and feathered.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[James Chapman]

Stephen Hemminger wrote:
> I think rather than having a meta-discussion, which always seems to degenerate
> into finger pointing. Let's look at the code. The problem with popular drivers
> (as I too well know) is that user's seem to find every wart.  There are lots of
> old drivers that never seem to get the same scrutiny, and if they did would
> be tarred and feathered.

Auke has already posted an early snapshot, archived here:

http://marc.info/?l=linux-netdev&m=118315951209350&w=3

Jeff and I have already commented on the code.

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Arjan van de Ven]

Stephen Hemminger wrote:
>> I would really like to continue with my original plan that I posted that follows 
>> Christoph's idea. I hope you can all agree with that so we can get on with this.
> 
> I think rather than having a meta-discussion, which always seems to degenerate
> into finger pointing. Let's look at the code. The problem with popular drivers
> (as I too well know) is that user's seem to find every wart.  There are lots of
> old drivers that never seem to get the same scrutiny, and if they did would
> be tarred and feathered.


I'd second this; also lets be honest and fair about things and use a 
similar standard for all drivers; are we going to ask all driver 
submitters to remove NAPI, TSO and other stuff? I would hope not. Are 
all new drivers that have even a single bitfield going to be rejected? 
That's be a new rule but ok, if it applies to all drivers it's fair.

So the question in my opinion should be "is this driver good enough 
for merging, and if not, what specifically is wrong enough to hold of 
merging" not "what would the perfect ideal 
we-have-all-the-time-in-the-world driver be" and certainly not "how is 
this going to work in an enterprise distro" since the later is the 
problem for the enterprise disro that they get paid to solve, not for 
kernel.org kernels.

What is there now is a driver that works, is relatively clean (and yes 
if you look deep enough you can ALWAYS nitpick on any code, even 
Linus' or Jeff's best code) and provides good performance with all the 
features a modern ethernet driver is supposed to have.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Kok, Auke wrote:
> This is not acceptable and hardly fair to expect from us.
> 
> It also exposes users to endless delays and uncertainties as to a final 
> resolution. Not to mention that writing a driver from scratch for (just) 
> ich9 will take significant time, is silly since it's almost identical to 
> ich8 etc..
> 
> I don't think that anyone besides you and maybe one or two others are 
> interested in doing this rewrite from scratch. The rest of us would 
> rather see something much more similar to my original suggestion which 
> relieves the ich9-wishes for everyone and provides a good starting point 
> to go forward. Not to mention that a lot of that code is already there, 
> and at least cleaned up quite a bit already.

Let's review the history here:

* For -years-, Intel has been told to stream structural cleanups into 
e1000 (and ixgb), along with feature enablement.  None of these cleanups 
happened, and the result was today's bloated driver.

* Intel posts a new e1000 driver, which is just as complex, if not more so.

* Intel proposes a Linux user transition plan that amounts to an abrupt 
NIC driver switch for users on a widely used NIC platform.

* Intel immediately dismisses all alternate paths to e1000 change, and 
all major e1000new structural comments.

Thus in effect you are presenting the Linux community with one option 
(and only one) -- your e1000new as it exists today -- and demanding that 
that option be accepted.

More importantly, Intel is asking the Linux community to TRUST that 
Intel will be a good driver MAINTAINER, despite lack of encouraging 
evidence in that regard.

Will we continue to see INATTENTION to basic driver maintenance, with 
100% of resources being devoted new features, and 0% devoted to thinking 
about how best to create a maintainable driver for those features long term?

With e1000new, I see a driver that's internally modular, but no more 
maintainable.  I also see a driver that runs directly counter to what we 
normally do in this situation -- split it up into multiple drivers. 
When you find yourself reinventing a net driver API, that's a big hint 
your engineering is ass-backwards.

It's not acceptable and hardly fair to expect the Linux community to 
blindly swallow the single option you've presented us.

We're talking about one of the most widely used NIC drivers here. 
Proposing to flip a switch and dump all users on the new driver in a 
couple months time does not serve Linux users at all.  Introducing the 
new driver more slowly -- starting with ICH9 and any other PCI IDs not 
covered by e1000 -- gives the driver a chance to prove itself in the 
field and be introduced more slowly, while at the same time not 
dramatically disturbing the existing, working e1000 installed base.

Starting with a new driver for the new hardware is the right way to go. 
    Get that driver into the field, get it right, and THEN use that 
field experience to see what PCI IDs you want to transition from the 
older driver.  That path is minimal disruption for Linux users, and 
maximizes the changes of Linux users running a working, proven driver. 
And coincidentally, that path also gives you the freedom to do heavy 
development on the new driver, with minimal disruption of the majority 
of your userbase.

So in sum, (a) Intel does not appear to have thought through the driver 
transition, (b) Intel has not proven itself to be a maintainer, and (c) 
e1000new needs to be restructured.

	Jeff




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Arjan van de Ven wrote:
> I'd second this; also lets be honest and fair about things and use a 
> similar standard for all drivers; are we going to ask all driver 
> submitters to remove NAPI, TSO and other stuff? I would hope not. Are 

That was merely a suggestion.  My general meaning was "small driver is 
easier to get into the kernel and into the field."


> all new drivers that have even a single bitfield going to be rejected? 
> That's be a new rule but ok, if it applies to all drivers it's fair.

That's one of a gazillion checklist items that a new driver has to pass 
through.  You know how it works for new drivers.

"it looks better than our last try" and "it looks better than that ugly 
driver over there" does not grant you a free pass on review.

I'm not asking for perfection, just something other than

* e1000 gets feedback
* Intel disappears for months
* Intel reappears with e1000 rewrite
* Intel fights tooth and nail when the driver is not accepted verboten

And specifically I worry that three years down the road we will reach 
the same point again, when e1000new is even more complex.

Will Intel shrug its shoulders and decide its time for another rewrite?


> So the question in my opinion should be "is this driver good enough for 
> merging, and if not, what specifically is wrong enough to hold of 
> merging" not "what would the perfect ideal 
> we-have-all-the-time-in-the-world driver be" and certainly not "how is 
> this going to work in an enterprise distro" since the later is the 
> problem for the enterprise disro that they get paid to solve, not for 
> kernel.org kernels.
> 
> What is there now is a driver that works, is relatively clean (and yes 
> if you look deep enough you can ALWAYS nitpick on any code, even Linus' 
> or Jeff's best code) and provides good performance with all the features 
> a modern ethernet driver is supposed to have.

Is this is the attitude, what's the point of even posting the driver for 
review?  Intel posted e1000new on June 29.  Feedback was then posted. 
Ten days later, without a single revision, Intel declares its own driver 
clean and working and good enough for merging as-is.

	Jeff




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Stephen Hemminger wrote:
> I think rather than having a meta-discussion, which always seems to degenerate
> into finger pointing. Let's look at the code. The problem with popular drivers
> (as I too well know) is that user's seem to find every wart.  There are lots of
> old drivers that never seem to get the same scrutiny, and if they did would
> be tarred and feathered.


"it's not as scruffy as that driver in the corner" is no way to maintain 
a kernel.

	Jeff

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Arjan van de Ven wrote:
> Kok, Auke wrote:
>> Jeff Garzik wrote:
>>> Andrew Morton wrote:
>>>> On Fri, 29 Jun 2007 14:39:20 -0700
>>>> "Kok, Auke" <auke-jan.h.kok@intel.com> wrote:
>>>>
>>>>> That's why we want to introduce a second e1000 driver (named 
>>>>> differently, pick any name) that contains the new code base, 
>>>>> side-by-side into the kernel with the current e1000.
>>>> Sounds like a reasonable approach to me (it has plenty of 
>>>> precedent).  But
>>>> I forget what all the other issues were, so ignore me.
>>>
>>> Given past history with duplicate drivers and the problems that they 
>>> cause -- I know, I've caused some of those problems :( -- I strongly 
>>> recommend against when it can be avoided.
>>>
>>> Leaving e1000 with current hardware, and a new e1001 for newer 
>>> hardware should be easier to manage for all involved, without the 
>>> headaches that duplicate drivers cause.
>>>
> 
> Jeff,
> 
> ok first you hate the old e1000 and now you don't want to get rid of it ;)

No -- e1000 is big and bloated but also stable and working and a key 
popular NIC driver for a lot of people.

That means the transition has a wide impact, and thus should be 
considered a lot more carefully than (say) rewriting 8139cp.

I reject the notion that a "flag day" switchover for a huge mass of 
e1000 users is the correct path.  I do not think that best serves Linux 
users.

It is better to get a new driver out in the field and working for a 
small population, let time pass, then consider if we really want to 
transition the entire e1000 population to the new driver.  That leaves 
the mass of e1000 users with a working driver while the new driver 
proves itself.  Since a small population of users is required to use the 
new driver, by virtue of new/old PCI ID split, you are guaranteed a 
population of test users immediately for the new driver.

When the new driver proves itself stable, you will have plenty of 
knowledge from which to decide whether to transition 8257x users, all 
e1000 users, or whatever.


> I appreciate the pain a temporary dual driver situation gives; it comes 
> down to a few things that I can think of right now, if you see more 
> please add to the list.
> 
> 1) users who find a bug in the new one silently use the old one rather 
> than reporting the bug; and only scream when the old one eventually goes 
> away (see ALSA/OSS duplication)
> 
> 2) users who enable both in KConfig may get a "random" one
> 
> 3) distros really prefer only 1 driver per PCI ID for their 
> infrastructure tools
> 
> 4) there will be resistance against deleting the old one meaning it 
> might not happen

You are missing the largest source of pain and headache:  Users will use 
the default driver, which means no field testing at all until flag day, 
with obvious results.

Furthermore, Linux kernel history demonstrates that "temporary dual 
driver situations" are rarely temporary.  Thus, selling it as such in 
the face of all contrary experience is pure hyperbole.

	Jeff




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Jeff Garzik]

Roland Dreier wrote:
> one possibility would be to merge e1000new with support only for chips
> not supported by e1000, and semi-freeze e1000 (fixes only, new device
> support goes into e1000new).

indeed.

	Jeff

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[James Chapman]

James Chapman wrote:
> I envisage something like this:-
> 
> e1000_core.c    - common code used by all drivers of the e1000 family.
>           Exports functions used by actual drivers. Loadable
>           as a separate module when built as a module.
> e1000_82541.c    - driver for 82541
> e1000_82542.c    - driver for 82542
> e1000_xxxxx.c    - driver for xxxxx

Please ignore the statement I made above. Having read the code and chip 
docs again, I realize I jumped to the wrong conclusions when I saw 
separate source files for each of the 7 chip variants in the new driver.

Just wanted to nip it in the bud before it causes any wasted time 
discussing it. :)

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Andrew Grover]

On 7/8/07, Jeff Garzik <jeff@garzik.org> wrote:
> * e1000 gets feedback
> * Intel disappears for months
> * Intel reappears with e1000 rewrite

* you ask them for another complete (simpler) rewrite

> * Intel fights tooth and nail when the driver is not accepted verboten

I don't think it must be as-is (i.e. replacing e1000 for all HW) but
don't throw away all the work they've done in architecting the driver
to cleanly handle multiple chip generations.

How about:

1) Considering e1000new's current design, but for ICH9 only
2) test test test
3) Sometime in the future, considering incrementally moving previous
PCIe generations' support from e1000 to e1000new (like I initially
wanted, since that at least means there would be some technical reason
for where the split occurs :-)

Regards -- Andy

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Arjan van de Ven]

Jeff Garzik wrote:
> Arjan van de Ven wrote:
>> I'd second this; also lets be honest and fair about things and use a 
>> similar standard for all drivers; are we going to ask all driver 
>> submitters to remove NAPI, TSO and other stuff? I would hope not. Are 
> 
> That was merely a suggestion.  My general meaning was "small driver is 
> easier to get into the kernel and into the field."

it didn't quite come across as a suggestion, but if it was only a 
suggestion then ok; it's not really a practical option.

>> all new drivers that have even a single bitfield going to be rejected? 
>> That's be a new rule but ok, if it applies to all drivers it's fair.
> 
> That's one of a gazillion checklist items that a new driver has to pass 
> through.  You know how it works for new drivers.
> 
> "it looks better than our last try" and "it looks better than that ugly 
> driver over there" does not grant you a free pass on review.

I know how it works for new drivers; they need to be good enough to 
maintain forward and form a burden on the stack/maintainers. Any 
remaining items need to be small enough to not be a 'risk' for users.


>> What is there now is a driver that works, is relatively clean (and yes 
>> if you look deep enough you can ALWAYS nitpick on any code, even 
>> Linus' or Jeff's best code) and provides good performance with all the 
>> features a modern ethernet driver is supposed to have.
> 
> Is this is the attitude, what's the point of even posting the driver for 
> review?  Intel posted e1000new on June 29.  Feedback was then posted. 

and feedback is being incorperated.

> Ten days later, without a single revision, Intel declares its own driver 
> clean and working and good enough for merging as-is.

No. Arjan looked at driver and sees a rather clean driver compared to 
some of the other recently merged drivers, sees a list of relatively 
simple todos (some of which obviously aren't merge stoppers, others 
are) and hopes that this driver will be treated objectively without 
the appearance of having different levels of bars depending on what 
your email address ends with.

Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Arjan van de Ven]

> I reject the notion that a "flag day" switchover for a huge mass of 
> e1000 users is the correct path.  I do not think that best serves Linux 
> users.

so the options we have is do it one pci id a time; and the suggestion 
by others like Christoph has been to make the split at the PCI Express 
switchover. Do you agree with that approach?

> 
>> I appreciate the pain a temporary dual driver situation gives; it 
>> comes down to a few things that I can think of right now, if you see 
>> more please add to the list.
>>
>> 1) users who find a bug in the new one silently use the old one rather 
>> than reporting the bug; and only scream when the old one eventually 
>> goes away (see ALSA/OSS duplication)
>>
>> 2) users who enable both in KConfig may get a "random" one
>>
>> 3) distros really prefer only 1 driver per PCI ID for their 
>> infrastructure tools
>>
>> 4) there will be resistance against deleting the old one meaning it 
>> might not happen
> 
> You are missing the largest source of pain and headache:  Users will use 
> the default driver, which means no field testing at all until flag day, 
> with obvious results.

which is why the proposal that was below the text you quoted talks 
about working with the distro kernel maintainers and get the new 
driver default for their development releases etc etc.... that adds
a significant level of granularity. For example I'd not be surprised 
if Fedora and Ubuntu test releases have orders of magnitude more users 
than kernel.org "self compiloe" kernels.


> Furthermore, Linux kernel history demonstrates that "temporary dual 
> driver situations" are rarely temporary.  Thus, selling it as such in 
> the face of all contrary experience is pure hyperbole.

history also demonstrates it can be done (just look at Adrian Bunk and 
the OSS drivers) as long as someone is determined to do it. It's not 
easy, especially cases where the two drivers have different 
maintainers who disagree, or represent different 
interfaces/functionality; but it's very not impossible either.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [E1000-devel] RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

[Jonathan Lundell]

On Jul 8, 2007, at 1:14 PM, Arjan van de Ven wrote:

>> I reject the notion that a "flag day" switchover for a huge mass of
>> e1000 users is the correct path.  I do not think that best serves  
>> Linux
>> users.
>
> so the options we have is do it one pci id a time; and the suggestion
> by others like Christoph has been to make the split at the PCI Express
> switchover. Do you agree with that approach?

My preference, as someone who has to support pretty much all  
generations of e1000-supported hardware in the field running critical  
applications is for a single well-structured driver. Qualifying a new  
release of the driver is a big deal, and having to qualify two of  
them doesn't really appeal to me.

We're seeing at least a couple of generations of mainboard that will  
have both PCIe and PCI-X slots (and adapters) in them, so even in the  
same box a PCIe split would require us to use both drivers. If  
splitting the driver really makes for substantial simplification of  
both, I suppose that's a compensating benefit, but it's not purely  
beneficial.

The e1000 driver (and the team behind it) has been a blessing to our  
business. Whatever they're doing right, I'd like to see them keep  
doing it.

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Andrew Grover wrote:
> On 7/8/07, Jeff Garzik <jeff@garzik.org> wrote:
>> * e1000 gets feedback
>> * Intel disappears for months
>> * Intel reappears with e1000 rewrite
> 
> * you ask them for another complete (simpler) rewrite
> 
>> * Intel fights tooth and nail when the driver is not accepted verboten
> 
> I don't think it must be as-is (i.e. replacing e1000 for all HW) but
> don't throw away all the work they've done in architecting the driver
> to cleanly handle multiple chip generations.
> 
> How about:
> 
> 1) Considering e1000new's current design, but for ICH9 only
> 2) test test test
> 3) Sometime in the future, considering incrementally moving previous
> PCIe generations' support from e1000 to e1000new (like I initially
> wanted, since that at least means there would be some technical reason
> for where the split occurs :-)

That plan would be fine...  as long as the e1000new driver internals 
were restructured as I've been describing.

If one arrives at a driver containing an internal API that is flexible 
enough to implement support for almost -any- NIC, then that's a sign 
that it needs to be organized in a different fashion.

	Jeff




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Arjan van de Ven wrote:
> Jeff Garzik wrote:
>> Is this is the attitude, what's the point of even posting the driver 
>> for review?  Intel posted e1000new on June 29.  Feedback was then posted. 
> 
> and feedback is being incorperated.
> 
>> Ten days later, without a single revision, Intel declares its own 
>> driver clean and working and good enough for merging as-is.
> 
> No. Arjan looked at driver and sees a rather clean driver compared to 
> some of the other recently merged drivers, sees a list of relatively 
> simple todos (some of which obviously aren't merge stoppers, others are) 
> and hopes that this driver will be treated objectively without the 
> appearance of having different levels of bars depending on what your 
> email address ends with.


Ignoring small potatoes, the merge stoppers in my mind are:

1) Transition plan.  I strongly oppose switching all e1000 users en 
masse to a new driver, especially so soon.  Flag day transitions to 
unproven drivers suck.  Defaults don't work:  users use the old driver 
until the default changes, which means the new driver gets little field 
testing.

Regardless of my opinion of old-e1000 maintainability, top priority is 
to keep users running on a stable driver until new driver is stable.  I 
would propose merging a new driver with only the PCI IDs not already in 
the kernel, get that stable, then consider moving the rest of the 
PCI-Express PCI IDs (or others?).


2) Internal API.  An "it can do anything" API is a hint that the driver 
should be structured differently.  Perhaps a divorce between pre-PCIe 
and PCIe will help things (or 8257x vs other?).  I tend to think that 
both e1000 and e1000new could be cleaned up substantially by such a 
split.  Also, specifically for PHYs, we already have a phy layer that 
can be used a focal point for PHY modularity.

Overall, within minor chip revs you'll probably create standard 
branches.  But within major chip revs, you really should be looking at 
separate code paths rather than trying to shoehorn a wide variety of 
chips down the same (highly modular!) hot path.  That slows down 
everybody to the same speed (least common denominator), and makes it 
more difficult to follow the code path for a single chip.

If the chips are sufficiently different, it is better to write two 
distinct RX/TX/etc. code paths than to create a single, highly modular 
yet highly dense code path.  Easier to read.  Easier to tune.  Easier to 
leave chip errata behind, as time progresses.


3) Looming over everything else, is I wonder if Intel has recognized 
that a maintainership problem exists: lack of ongoing cleanups and lack 
of "focus" on issues not directly related to enabling new features?

My only "bias" against Intel is that I am -nervous- that past history 
will continue, with regards to the "stuff it into the driver and move 
on" driver maintenance regime.  Even most clean driver in the world will 
fall upon hard times if under-maintained.

Receiving feedback, disappearing for months, and then reappearing with 
"blessed" changes is not adequate.  netdev@vger.kernel.org and 
netdev-2.6.git need to be the primary vehicle for e1000[new] 
development, maintained through small incremental changes streaming into 
the kernel.  (though I do understand the constraints of hardware 
go-public release dates)

	Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Stephen Hemminger]

Let me ask a different question. Why do we have some many user reported
problems with network drivers, not just the E1000's?

Is it lack of specifications? test infrastructure?
Or as I suspect lots of it has to with the myriad of chip revisions, including
cases where designs get reworked by OEM's.

If the problem were more understood, then the community would have more confidence
with new drivers and other changes.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Arjan van de Ven]

Jeff Garzik wrote:

<I'll leave the first two to Auke; the discussion I had with Auke on 
Friday was that he felt that a lot of the "ugly" stuff you complained 
about will go away if there is a PCI-E/older split, the PCI-E cards 
are more or less of the same "major generation", while pre-PCI-E is 
different>

> 3) Looming over everything else, is I wonder if Intel has recognized 
> that a maintainership problem exists: lack of ongoing cleanups and lack 
> of "focus" on issues not directly related to enabling new features?
> 
> My only "bias" against Intel is that I am -nervous- that past history 
> will continue, with regards to the "stuff it into the driver and move 
> on" driver maintenance regime.  Even most clean driver in the world will 
> fall upon hard times if under-maintained.

We (the Intel people on this mail and various others inside Intel) 
have been working hard to fix this issue several levels of management 
up in the networking group and I am confident we have all the buy-in 
to do the right thing now, and Auke has the time to do this. This was 
quite an effort at Intel-internal politics, and that's obviously 
something you don't (want to) care about or should have to care 
about.... but the end result is that finally in the last month or two 
things changed enough that I feel that the maintenance issue can be 
done right. The flipside sort of is that there needs to be a chance to 
prove this ;)

(and if you feel that it's not working please contact me so that I can 
re-escalate things; I'll also be monitoring how things go)

> Receiving feedback, disappearing for months, and then reappearing with 
> "blessed" changes is not adequate.  netdev@vger.kernel.org and 
> netdev-2.6.git need to be the primary vehicle for e1000[new] 
> development, maintained through small incremental changes streaming into 
> the kernel.  (though I do understand the constraints of hardware 
> go-public release dates)

Actually the biggest constraint for new hardware support is having 
working prototypes ;-)
The market is such that there may be only a short time between having 
a working prototype and a launch. (Before that there are design specs 
of course, but reality is that unless you can do basic unit testing 
shipping code for such new things is not a good thing, the chance of 
things "just working" is obviously pretty low).

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Jeff Garzik wrote:
> Ignoring small potatoes, the merge stoppers in my mind are:
> 
> 1) Transition plan.  I strongly oppose switching all e1000 users en 
> masse to a new driver, especially so soon.  Flag day transitions to 
> unproven drivers suck.  Defaults don't work:  users use the old driver 
> until the default changes, which means the new driver gets little field 
> testing.
> 
> Regardless of my opinion of old-e1000 maintainability, top priority is 
> to keep users running on a stable driver until new driver is stable.  I 
> would propose merging a new driver with only the PCI IDs not already in 
> the kernel, get that stable, then consider moving the rest of the 
> PCI-Express PCI IDs (or others?).

I would strongly vote for taking a stripped down e1000new then, mask out all the 
pci id's except ich9, remove all code for pre-pci-e silicon and remove the most 
annoying and needlessly complexing code like the semi-implemented multiqueue 
code that is in there.

How we are going to improve the internal api then can subsequently be done 
upstream in steps: implement using phylib, reorganize the code. This would give 
the community a view on the progress.

I fear that if I spend yet another 2 months offline working on making a minimal 
ich9 driver I will lose even more time and patience: Even though the current 
driver (with pre-pci-e stripped) might not be as nice as you want, at least we 
can work together on it. I would rather go with something I know that works, 
isn't too bad, and we have time and start reviewing upstream immediately.

> 2) Internal API.  An "it can do anything" API is a hint that the driver 
> should be structured differently.  Perhaps a divorce between pre-PCIe 
> and PCIe will help things (or 8257x vs other?).  I tend to think that 
> both e1000 and e1000new could be cleaned up substantially by such a 
> split.  Also, specifically for PHYs, we already have a phy layer that 
> can be used a focal point for PHY modularity.

Agreed. All current e1000 pci-e hardware is based on the same mac, so it's the 
logical split. The differences are PHYs and manageability, but the interface is 
rather similar throughout, as well as features.

> Overall, within minor chip revs you'll probably create standard 
> branches.  But within major chip revs, you really should be looking at 
> separate code paths rather than trying to shoehorn a wide variety of 
> chips down the same (highly modular!) hot path.  That slows down 
> everybody to the same speed (least common denominator), and makes it 
> more difficult to follow the code path for a single chip.

looking at this with respect to e1000e (a pci-e only e1000 driver) - this would 
make perfect sense: most of the irq and rx/tx paths are identical across the 
board. So this confirms IMO that we should not split beyond this.


Auke

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Kok, Auke wrote:
> I would strongly vote for taking a stripped down e1000new then, mask out 
> all the pci id's except ich9, remove all code for pre-pci-e silicon and 
> remove the most annoying and needlessly complexing code like the 
> semi-implemented multiqueue code that is in there.

I'm fine for that as a path forward.  I would think that would zap a lot 
of the hooks.


> How we are going to improve the internal api then can subsequently be 
> done upstream in steps: implement using phylib, reorganize the code. 
> This would give the community a view on the progress.
> 
> I fear that if I spend yet another 2 months offline working on making a 
> minimal ich9 driver I will lose even more time and patience: Even though 
> the current driver (with pre-pci-e stripped) might not be as nice as you 
> want, at least we can work together on it. I would rather go with 
> something I know that works, isn't too bad, and we have time and start 
> reviewing upstream immediately.

"minimal ich9 driver" is more a metaphor, describing the seed from which 
a clean driver would logically grow:  imagine a minimal ich9 driver, 
then add TSO, add support for other PCI-e phys, etc.  Whether you do 
this exercise in your head or on the computer makes no difference, as 
long as you can see what the end result should look like.

Not asking for yet another driver...  Start with e1000new or whatever 
base you like.  Post driver for review, get feedback, update, lather, 
rinse, repeat.  If we're all responsive, it should not take long at all 
to get something upstream-mergeable.  Solve the "big potato" issues 
especially :)


> Agreed. All current e1000 pci-e hardware is based on the same mac, so 
> it's the logical split. The differences are PHYs and manageability, but 

OK

	Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Jeff Garzik wrote:
> Kok, Auke wrote:
>> I would strongly vote for taking a stripped down e1000new then, mask out 
>> all the pci id's except ich9, remove all code for pre-pci-e silicon and 
>> remove the most annoying and needlessly complexing code like the 
>> semi-implemented multiqueue code that is in there.
> 
> I'm fine for that as a path forward.  I would think that would zap a lot 
> of the hooks.
> 
>> How we are going to improve the internal api then can subsequently be 
>> done upstream in steps: implement using phylib, reorganize the code. 
>> This would give the community a view on the progress.
>>
>> I fear that if I spend yet another 2 months offline working on making a 
>> minimal ich9 driver I will lose even more time and patience: Even though 
>> the current driver (with pre-pci-e stripped) might not be as nice as you 
>> want, at least we can work together on it. I would rather go with 
>> something I know that works, isn't too bad, and we have time and start 
>> reviewing upstream immediately.
> 
> "minimal ich9 driver" is more a metaphor, describing the seed from which 
> a clean driver would logically grow:  imagine a minimal ich9 driver, 
> then add TSO, add support for other PCI-e phys, etc.  Whether you do 
> this exercise in your head or on the computer makes no difference, as 
> long as you can see what the end result should look like.
> 
> Not asking for yet another driver...  Start with e1000new or whatever 
> base you like.  Post driver for review, get feedback, update, lather, 
> rinse, repeat.  If we're all responsive, it should not take long at all 
> to get something upstream-mergeable.  Solve the "big potato" issues 
> especially :)
> 
> 
>> Agreed. All current e1000 pci-e hardware is based on the same mac, so 
>> it's the logical split. The differences are PHYs and manageability, but 
> 
> OK

Sorry for not replying to this earlier (I was on vacation for 2 days). We had 
some internal discussions and all the noses appear to be facing in the proper 
direction. I'm very happy with this and it will be my highest priority to make 
this work. I will attempt to get a quick start on this and come with a start 
point driver as soon as I can. Thanks to everyones who commented!

Auke

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Jeff Garzik]

Kok, Auke wrote:
> Sorry for not replying to this earlier (I was on vacation for 2 days). 
> We had some internal discussions and all the noses appear to be facing 
> in the proper direction. I'm very happy with this and it will be my 
> highest priority to make this work. I will attempt to get a quick start 
> on this and come with a start point driver as soon as I can. Thanks to 
> everyones who commented!

Sounds good to me.  My only further comment would be to agree and 
encourage posting of quick-start soon.  Don't worry about much beyond 
"it works" tests before posting the first public version.  We'll 
inevitably need a couple more minor revisions before merging, as with 
all new drivers.

We all seem to be going in the right direction, so the process should 
not take months.  I'll make it a priority to review each revision as 
soon as possible.

	Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Splitting e1000 (Was: Re: e1000: backport ich9 support from 7.5.5 ?)

[Kok, Auke]

Jeff Garzik wrote:
> Kok, Auke wrote:
>> Sorry for not replying to this earlier (I was on vacation for 2 days). 
>> We had some internal discussions and all the noses appear to be facing 
>> in the proper direction. I'm very happy with this and it will be my 
>> highest priority to make this work. I will attempt to get a quick start 
>> on this and come with a start point driver as soon as I can. Thanks to 
>> everyones who commented!
> 
> Sounds good to me.  My only further comment would be to agree and 
> encourage posting of quick-start soon.  Don't worry about much beyond 
> "it works" tests before posting the first public version.  We'll 
> inevitably need a couple more minor revisions before merging, as with 
> all new drivers.
> 
> We all seem to be going in the right direction, so the process should 
> not take months.  I'll make it a priority to review each revision as 
> soon as possible.

absolutely.

I personally hope to have a draft ich9 version next week ready. Let's see if I 
make that - a lot of people would be happy to get it that soon ;)


Auke