netdev - Re: RFC: igb: Intel 82575 gigabit ethernet driver (take #2)

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] [day] [month] [year] [list]

Date:	Thu, 10 Jan 2008 11:57:33 -0800
From:	"Kok, Auke" <auke-jan.h.kok@...el.com>
To:	Jeff Garzik <jgarzik@...ox.com>
CC:	"Kok, Auke" <auke-jan.h.kok@...el.com>,
	NetDev <netdev@...r.kernel.org>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	"Ronciak, John" <john.ronciak@...el.com>
Subject: Re: RFC: igb: Intel 82575 gigabit ethernet driver (take #2)

Jeff Garzik wrote:
> Looks pretty decent.  Main comments (style mostly, driver operation path
> seems sound):

thanks again for the comments. I am about to send an updated patch just before my
vacation and before I do let me just quickly touch on your comments below:

> * kill the bitfields and unions [in descriptor structs].  they are not
> endian-safe as presented, generate poor code, and are otherwise
> undesirable.

that bitfield was unused and so I removed the code. I don't see any more bitfields
at all now in this driver.

> * the basic operations are too verbose:  E1000_READ_REG(hw, REGISTER) is
> far more readable as ER32(REGISTER), following the style of other
> drivers.  Furthermore, the "E1000_" prefix, in addition to being overly
> redundant (used in each register read/write), it is also incorrect,
> because this is not E1000...

partially I agree, and I refined the register writes to remove the need for the
"hw" part.

However the hardware *is* e1000, we ended up making a new driver since it just
does not make sense to add all of this infrastructure for older chipsets anymore.

renaming everything (from e1000_ to igb_) would just make life for us really hard
looking up historical diffs, history etc. and most importantly compare with
e1000/e1000e when we encounter an issue that might affect the other drivers. For
now it is easier to just leave these alone.

I however do not rule out that we change this at a later stage ...

> * in general, rename everything with "e1000_" prefix.  this will
> eliminate plenty of human confusion in the long run.

I'm doing this for all functions, which solves the namespace collisions. The
"e1000" specific static structs (which are the same in igb as they are in e1000,
e1000e) as well as the registers (ditto) I'll keep unchanged for now.

> * API:   unless you have chips in the lab that will require an API hook,
> don't create one.  For example, a direct call to
> e1000_acquire_nvm_82575() should replace all ->acquire_nvm() hooks....
> if there are no chips in pipeline GUARANTEED to have a different
> ->acquire_nvm() feature.

Noted

Note also that there are already many less hooks as there are in e1000e. We did
already make an effort to scrub as many as we can.

Cheers,

Auke

--
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