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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 7 Jul 2007 11:59:44 -0700
From:	"Andrew Grover" <andy.grover@...il.com>
To:	"Jeff Garzik" <jeff@...zik.org>
Cc:	"Kok, Auke" <auke-jan.h.kok@...el.com>, netdev@...r.kernel.org,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Jason Lunz" <lunz@...lexsecurity.com>,
	"Mark McLoughlin" <markmc@...hat.com>,
	e1000-devel@...ts.sourceforge.net,
	"Arjan van de Ven" <arjan@...ux.intel.com>,
	"Ronciak, John" <john.ronciak@...el.com>
Subject: Re: RFR: New e1000 driver (e1000new), was: Re: e1000: backport ich9 support from 7.5.5 ?

On 7/6/07, Jeff Garzik <jeff@...zik.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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html