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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
