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

[Jeff Garzik <jeff@garzik.org>]

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.