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

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

[Kok, Auke]

[-- Attachment #1: Type: text/plain, Size: 4144 bytes --]


All,

here is the second version of the igb (82575) ethernet controller driver. This
driver was previously posted 2007-07-13. Many comments received were addressed:

- removed indirection wrappers in the same way as e1000e and ixgbe.
- cleaned up largely against sparse, checkpatch
- removed module parameters and moved functionality to ethtool ioctls
- new NAPI API rewrites
- by default the driver runs in multiqueue mode with 2 to 40 RX queues enabled.

Since the driver is still too large (allthough the patch shrunk from 558k to 416k,
almost 34% of its size) to post to this list I am attaching the bzipped patch
here. You can get the same driver alternatively from here:

http://foo-projects.org/~sofar/0001-igb-PCI-Express-82575-Gigabit-Ethernet-driver.patch
[416k]
http://foo-projects.org/~sofar/0001-igb-PCI-Express-82575-Gigabit-Ethernet-driver.patch.bz2
[74k]

or through git:
    git://lost.foo-projects.org/~ahkok/git/linux-2.6 #igb


There are several concerns still open for this driver:
- namespace collisions with e1000. Since there are cleanups planned for e1000
since pci-e hardware is now moved to e1000e, this might resolve them.
- hardware code is still a large API. we're expecting more hardware to be
supported by this driver in the future and it's not certain which parts we need to
keep or not.


Please review,


Cheers,

Auke

---
>From 02d13aaf92470678aaeb739f0b6e4c9f9687c7b8 Mon Sep 17 00:00:00 2001
From: Auke Kok <auke-jan.h.kok@intel.com>
Date: Thu, 6 Dec 2007 15:08:14 -0800
Subject: [PATCH] igb: PCI-Express 82575 Gigabit Ethernet driver

We are pleased to announce a new Gigabit Ethernet product and its
driver to the linux community. This product is the Intel(R) 82575
Gigabit Ethernet adapter family. Physical adapters will be available
to the public soon. These adapters come in 2- and 4-port versions
(copper PHY) currently. Other variants will be available later.

The 82575 chipset supports significantly different features that
warrant a new driver. The descriptor format is (just like the
ixgbe driver) different. The device can use multiple MSI-X vectors
and multiple queues for both send and receive. This allows us to
optimize some of the driver code specifically as well compared to
the e1000-supported devices.

This version of the igb driver no lnger uses fake netdevices and
incorporates napi_struct members for each ring to do the multi-
queue polling. multi-queue is enabled by default and the driver
supports NAPI mode only.

Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---
 drivers/net/Kconfig             |   22 +
 drivers/net/Makefile            |    1 +
 drivers/net/igb/Makefile        |   37 +
 drivers/net/igb/e1000_82575.c   | 1269 ++++++++++++
 drivers/net/igb/e1000_82575.h   |  198 ++
 drivers/net/igb/e1000_defines.h |  772 ++++++++
 drivers/net/igb/e1000_hw.h      |  599 ++++++
 drivers/net/igb/e1000_mac.c     | 1505 ++++++++++++++
 drivers/net/igb/e1000_mac.h     |   98 +
 drivers/net/igb/e1000_nvm.c     |  605 ++++++
 drivers/net/igb/e1000_nvm.h     |   40 +
 drivers/net/igb/e1000_phy.c     | 1807 +++++++++++++++++
 drivers/net/igb/e1000_phy.h     |   98 +
 drivers/net/igb/e1000_regs.h    |  277 +++
 drivers/net/igb/igb.h           |  300 +++
 drivers/net/igb/igb_ethtool.c   | 1926 ++++++++++++++++++
 drivers/net/igb/igb_main.c      | 4110 +++++++++++++++++++++++++++++++++++++++
 17 files changed, 13664 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/igb/Makefile
 create mode 100644 drivers/net/igb/e1000_82575.c
 create mode 100644 drivers/net/igb/e1000_82575.h
 create mode 100644 drivers/net/igb/e1000_defines.h
 create mode 100644 drivers/net/igb/e1000_hw.h
 create mode 100644 drivers/net/igb/e1000_mac.c
 create mode 100644 drivers/net/igb/e1000_mac.h
 create mode 100644 drivers/net/igb/e1000_nvm.c
 create mode 100644 drivers/net/igb/e1000_nvm.h
 create mode 100644 drivers/net/igb/e1000_phy.c
 create mode 100644 drivers/net/igb/e1000_phy.h
 create mode 100644 drivers/net/igb/e1000_regs.h
 create mode 100644 drivers/net/igb/igb.h
 create mode 100644 drivers/net/igb/igb_ethtool.c
 create mode 100644 drivers/net/igb/igb_main.c


[-- Attachment #2: 0001-igb-PCI-Express-82575-Gigabit-Ethernet-driver.patch.bz2 --]
[-- Type: application/x-bzip2, Size: 76046 bytes --]

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

[Kok, Auke]

Kok, Auke wrote:
> All,
> 
> here is the second version of the igb (82575) ethernet controller driver. This
> driver was previously posted 2007-07-13. Many comments received were addressed:
> 
> - removed indirection wrappers in the same way as e1000e and ixgbe.
> - cleaned up largely against sparse, checkpatch
> - removed module parameters and moved functionality to ethtool ioctls
> - new NAPI API rewrites
> - by default the driver runs in multiqueue mode with 2 to 40 RX queues enabled.
> 
> Since the driver is still too large (allthough the patch shrunk from 558k to 416k,
> almost 34% of its size) to post to this list I am attaching the bzipped patch
> here. You can get the same driver alternatively from here:
> 
> http://foo-projects.org/~sofar/0001-igb-PCI-Express-82575-Gigabit-Ethernet-driver.patch
> [416k]
> http://foo-projects.org/~sofar/0001-igb-PCI-Express-82575-Gigabit-Ethernet-driver.patch.bz2
> [74k]
> 
> or through git:
>     git://lost.foo-projects.org/~ahkok/git/linux-2.6 #igb
> 
> 
> There are several concerns still open for this driver:
> - namespace collisions with e1000. Since there are cleanups planned for e1000
> since pci-e hardware is now moved to e1000e, this might resolve them.
> - hardware code is still a large API. we're expecting more hardware to be
> supported by this driver in the future and it's not certain which parts we need to
> keep or not.


unfortunately a last-minute effort of mine inserted a stray character. Please
re-download the patch files from http or through git to get the updated patch that
fixes this issue. The changes needed are below.

Cheers,

Auke


---
diff --git a/drivers/net/igb/e1000_phy.c b/drivers/net/igb/e1000_phy.c
index e57222a..1c13156 100644
--- a/drivers/net/igb/e1000_phy.c
+++ b/drivers/net/igb/e1000_phy.c
@@ -1555,7 +1555,7 @@ s32 e1000_get_phy_info_igp(struct e1000_hw *hw)
 		goto out;

 	ret_val = hw->phy.ops.read_phy_reg(hw, IGP01E1000_PHY_PORT_STATUS,
-			`		   &data);
+					   &data);
 	if (ret_val)
 		goto out;

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

[Jeff Garzik]

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

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

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

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

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

In general, I try to communicate that I am not opposed to these hooks, 
you merely need to make sure they are needed in _each_ case.  Otherwise 
engineers WILL fall into the habit of writing bloated code simply 
because that's what their chosen driver framework has always done.

	Jeff

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

[Kok, Auke]

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