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

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: Auke Kok <[email protected]>
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.

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

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

---

index e57222a..1c13156 100644
@@ -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;

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

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:

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

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

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.

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