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, 07 Jul 2007 13:23:11 +0100
From:	James Chapman <jchapman@...alix.com>
To:	"Kok, Auke" <auke-jan.h.kok@...el.com>
CC:	Jeff Garzik <jeff@...zik.org>, 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 ?

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

-
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