On 7/6/07, Jeff Garzik [off-list ref] 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/