Re: A new 10GB Ethernet Driver by Chelsio Communications

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Andrew Morton]

Christoph Lameter <christoph@graphe.net> wrote:
>
> A Linux driver for the Chelsio 10Gb Ethernet Network Controller by
>  Chelsio (http://www.chelsio.com). This driver supports the Chelsio N210
>  NIC and is backward compatible with the Chelsio N110 model 10Gb NICs.

Thanks, Christoph.

The 400k patch was too large for the vger email server so I have uploaded it to

 http://www.zip.com.au/~akpm/linux/patches/stuff/a-new-10gb-ethernet-driver-by-chelsio-communications.patch

for reviewers.

> It supports AMD64, EM64T and x86 systems.

Is it only supported on those systems?  If so, why is that?

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Christoph Lameter]

On Fri, 11 Mar 2005, Andrew Morton wrote:

> > It supports AMD64, EM64T and x86 systems.
>
> Is it only supported on those systems?  If so, why is that?

The driver was only tested on those architectures.

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Adrian Bunk]

On Fri, Mar 11, 2005 at 11:21:32AM -0800, Andrew Morton wrote:
> Christoph Lameter <christoph@graphe.net> wrote:
> >
> > A Linux driver for the Chelsio 10Gb Ethernet Network Controller by
> >  Chelsio (http://www.chelsio.com). This driver supports the Chelsio N210
> >  NIC and is backward compatible with the Chelsio N110 model 10Gb NICs.
> 
> Thanks, Christoph.
> 
> The 400k patch was too large for the vger email server so I have uploaded it to
> 
>  http://www.zip.com.au/~akpm/linux/patches/stuff/a-new-10gb-ethernet-driver-by-chelsio-communications.patch
> 
> for reviewers.
>...

#if defined(MODULE) && ! defined(MODVERSIONS)
#define MODVERSIONS
#endif


Why?


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Stephen Hemminger]

On Fri, 11 Mar 2005 11:21:32 -0800
Andrew Morton <akpm@osdl.org> wrote:

> Christoph Lameter <christoph@graphe.net> wrote:
> >
> > A Linux driver for the Chelsio 10Gb Ethernet Network Controller by
> >  Chelsio (http://www.chelsio.com). This driver supports the Chelsio N210
> >  NIC and is backward compatible with the Chelsio N110 model 10Gb NICs.
> 
> Thanks, Christoph.
> 
> The 400k patch was too large for the vger email server so I have uploaded it to
> 
>  http://www.zip.com.au/~akpm/linux/patches/stuff/a-new-10gb-ethernet-driver-by-chelsio-communications.patch
> 
> for reviewers.

The performance recommendations in cxgb.txt are common to all fast devices,
and should be in one file rather than just for this device. I would rather
see ip-sysctl.txt updated or a new file on tuning recommendations started.
Some of them have consequences that aren't documented well.  
For example, turning off TCP timestamps risks data corruption from sequence wrap.

A new driver shouldn't need so may #ifdef's unless you want to putit on older
vendor versions of 2.4

Some accessor and wrapper functions like:
	t1_pci_read_config_4
	adapter_name
	t1_malloc
are just annoying noise.

Why have useless dead code like:




/* Interrupt handler */
+static int pm3393_interrupt_handler(struct cmac *cmac)
+{
+	u32 master_intr_status;
+/*
+    1. Read master interrupt register.
+    2. Read BLOCK's interrupt status registers.
+    3. Handle BLOCK interrupts.
+*/
+	/* Read the master interrupt status register. */
+	pmread(cmac, SUNI1x10GEXP_REG_MASTER_INTERRUPT_STATUS,
+	       &master_intr_status);
+	CH_DBG(cmac->adapter, INTR, "PM3393 intr cause 0x%x\n",
+	       master_intr_status);
+
+	/* Handle BLOCK's interrupts. */
+
+	if (SUNI1x10GEXP_BITMSK_TOP_PL4IO_INT & master_intr_status) {
+	}
+
+	if (SUNI1x10GEXP_BITMSK_TOP_IRAM_INT & master_intr_status) {
+	}
+
+	if (SUNI1x10GEXP_BITMSK_TOP_ERAM_INT & master_intr_status) {
+	}
+
+	/* SERDES */
+	if (SUNI1x10GEXP_BITMSK_TOP_XAUI_INT & master_intr_status) {
+	}
+
+	/* MSTAT */
+	if (SUNI1x10GEXP_BITMSK_TOP_MSTAT_INT & master_intr_status) {
+	}
+
+	/* RXXG */
+	if (SUNI1x10GEXP_BITMSK_TOP_RXXG_INT & master_intr_status) {
+	}
+
+	/* TXXG */
+	if (SUNI1x10GEXP_BITMSK_TOP_TXXG_INT & master_intr_status) {
+	}
+
+	/* XRF */
+	if (SUNI1x10GEXP_BITMSK_TOP_XRF_INT & master_intr_status) {
+	}
+
+	/* XTEF */
+	if (SUNI1x10GEXP_BITMSK_TOP_XTEF_INT & master_intr_status) {
+	}
+
+	/* MDIO */
+	if (SUNI1x10GEXP_BITMSK_TOP_MDIO_BUSY_INT & master_intr_status) {
+		/* Not used. 8000 uses MDIO through Elmer. */
+	}
+
+	/* RXOAM */
+	if (SUNI1x10GEXP_BITMSK_TOP_RXOAM_INT & master_intr_status) {
+	}
+
+	/* TXOAM */
+	if (SUNI1x10GEXP_BITMSK_TOP_TXOAM_INT & master_intr_status) {
+	}
+
+	/* IFLX */
+	if (SUNI1x10GEXP_BITMSK_TOP_IFLX_INT & master_intr_status) {
+	}
+
+	/* EFLX */
+	if (SUNI1x10GEXP_BITMSK_TOP_EFLX_INT & master_intr_status) {
+	}
+
+	/* PL4ODP */
+	if (SUNI1x10GEXP_BITMSK_TOP_PL4ODP_INT & master_intr_status) {
+	}
+
+	/* PL4IDU */
+	if (SUNI1x10GEXP_BITMSK_TOP_PL4IDU_INT & master_intr_status) {
+	}

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Adrian Bunk]

On Fri, Mar 11, 2005 at 11:21:32AM -0800, Andrew Morton wrote:
> Christoph Lameter <christoph@graphe.net> wrote:
> >
> > A Linux driver for the Chelsio 10Gb Ethernet Network Controller by
> >  Chelsio (http://www.chelsio.com). This driver supports the Chelsio N210
> >  NIC and is backward compatible with the Chelsio N110 model 10Gb NICs.
> 
> Thanks, Christoph.
> 
> The 400k patch was too large for the vger email server so I have uploaded it to
> 
>  http://www.zip.com.au/~akpm/linux/patches/stuff/a-new-10gb-ethernet-driver-by-chelsio-communications.patch
> 
> for reviewers.
>...

- my3126.c is unused (because t1_my3126_ops isn't used anywhere)
- what are the EXTRA_CFLAGS in drivers/net/chelsio/Makefile for?
- $(cxgb-y) in drivers/net/chelsio/Makefile seems to be unneeded
- completely unused global functions:
  - espi.c: t1_espi_get_intr_counts
  - sge.c: t1_sge_get_intr_counts
- the following functions can be made static:
  - sge.c: t1_espi_workaround
  - sge.c: t1_sge_tx
  - subr.c: __t1_tpi_read
  - subr.c: __t1_tpi_write
  - subr.c: t1_wait_op_done


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Jeff Garzik]

Andrew Morton wrote:
> Christoph Lameter <christoph@graphe.net> wrote:
> 
>>A Linux driver for the Chelsio 10Gb Ethernet Network Controller by
>> Chelsio (http://www.chelsio.com). This driver supports the Chelsio N210
>> NIC and is backward compatible with the Chelsio N110 model 10Gb NICs.
> 
> 
> Thanks, Christoph.
> 
> The 400k patch was too large for the vger email server so I have uploaded it to
> 
>  http://www.zip.com.au/~akpm/linux/patches/stuff/a-new-10gb-ethernet-driver-by-chelsio-communications.patch

step 1:  kill all the OS wrappers.

And do you really need hooks for multiple MACs, when only one MAC is 
really supported?  Typically these hooks are at a higher level anyway -- 
struct net_device.

	Jeff

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Pekka Enberg]

Some of my usual coding style comments...

On Fri, 11 Mar 2005 11:21:32 -0800, Andrew Morton <akpm@osdl.org> wrote:
> diff -puN /dev/null drivers/net/chelsio/osdep.h
> --- /dev/null	2003-09-15 06:40:47.000000000 -0700
> +++ 25-akpm/drivers/net/chelsio/osdep.h	2005-03-11 11:13:06.000000000 -0800
> +static inline void *t1_malloc(size_t len)
> +{
> +	void *m = kmalloc(len, GFP_KERNEL);
> +	if (m)
> +		memset(m, 0, len);
> +	return m;
> +}
> +
> +static inline void t1_free(void *v, size_t len)
> +{
> +	kfree(v);
> +}

Please do not introduce subsystem specific wrappers to kmalloc and kfree.

> +/*
> + * Allocates basic RX resources, consisting of memory mapped freelist Qs and a
> + * response Q.
> + */
> +static int alloc_rx_resources(struct sge *sge, struct sge_params *p)
> +{
> +	struct pci_dev *pdev = sge->adapter->pdev;
> +	unsigned int size, i;
> +
> +	for (i = 0; i < SGE_FREELQ_N; i++) {
> +		struct freelQ *Q = &sge->freelQ[i];
> +
> +		Q->genbit = 1;
> +		Q->entries_n = p->freelQ_size[i];
> +		Q->dma_offset = SGE_RX_OFFSET - sge->rx_pkt_pad;
> +		size = sizeof(struct freelQ_e) * Q->entries_n;
> +		Q->entries = (struct freelQ_e *)
> +			      pci_alloc_consistent(pdev, size, &Q->dma_addr);
> +		if (!Q->entries)
> +			goto err_no_mem;
> +		memset(Q->entries, 0, size);
> +		size = sizeof(struct freelQ_ce) * Q->entries_n;
> +		Q->centries = (struct freelQ_ce *) kmalloc(size, GFP_KERNEL);
> +		if (!Q->centries)
> +			goto err_no_mem;
> +		memset(Q->centries, 0, size);

Please drop the redundant casts and use kcalloc() here and in various
other places as
well.

Also, the patch has some whitespace damage (spaces instead of tabs).

				Pekka

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Pekka Enberg]

Hi,

Few more coding style comments.

On Fri, 11 Mar 2005 11:21:32 -0800, Andrew Morton <akpm@osdl.org> wrote:
> diff -puN /dev/null drivers/net/chelsio/cxgb2.c
> --- /dev/null	2003-09-15 06:40:47.000000000 -0700
> +++ 25-akpm/drivers/net/chelsio/cxgb2.c	2005-03-11 11:13:06.000000000 -0800
> @@ -0,0 +1,1284 @@
> +#ifndef HAVE_FREE_NETDEV
> +#define free_netdev(dev) kfree(dev)
> +#endif

Please drop this wrapper.

> +	printk(KERN_INFO "%s: %s (rev %d), %s %dMHz/%d-bit\n", adapter->name,
> +	       bi->desc, adapter->params.chip_revision,
> +	       adapter->params.pci.is_pcix ? "PCIX" : "PCI",
> +	       adapter->params.pci.speed, adapter->params.pci.width);
> +	return 0;
> +
> + out_release_adapter_res:
> +	t1_free_sw_modules(adapter);
> + out_free_dev:
> +	if (adapter) {
> +		if (adapter->tdev)
> +			kfree(adapter->tdev);

kfree() handles null pointers so please drop the redundant check.

> diff -puN /dev/null drivers/net/chelsio/gmac.h
> --- /dev/null	2003-09-15 06:40:47.000000000 -0700
> +++ 25-akpm/drivers/net/chelsio/gmac.h	2005-03-11 11:13:06.000000000 -0800
> @@ -0,0 +1,126 @@
> +
> +typedef struct _cmac_instance cmac_instance;

Please drop the typedef.

> diff -puN /dev/null drivers/net/chelsio/osdep.h
> --- /dev/null	2003-09-15 06:40:47.000000000 -0700
> +++ 25-akpm/drivers/net/chelsio/osdep.h	2005-03-11 11:13:06.000000000 -0800
> @@ -0,0 +1,222 @@
> +#define DRV_NAME "cxgb"
> +#define PFX      DRV_NAME ": "
> +
> +#define CH_ERR(fmt, ...)   printk(KERN_ERR PFX fmt, ## __VA_ARGS__)
> +#define CH_WARN(fmt, ...)  printk(KERN_WARNING PFX fmt, ## __VA_ARGS__)
> +#define CH_ALERT(fmt, ...) printk(KERN_ALERT PFX fmt, ## __VA_ARGS__)
> +
> +/*
> + * More powerful macro that selectively prints messages based on msg_enable.
> + * For info and debugging messages.
> + */
> +#define CH_MSG(adapter, level, category, fmt, ...) do { \
> +	if ((adapter)->msg_enable & NETIF_MSG_##category) \
> +		printk(KERN_##level PFX "%s: " fmt, (adapter)->name, \
> +		       ## __VA_ARGS__); \
> +} while (0)
> +
> +#ifdef DEBUG
> +# define CH_DBG(adapter, category, fmt, ...) \
> +        CH_MSG(adapter, DEBUG, category, fmt, ## __VA_ARGS__)
> +#else
> +# define CH_DBG(fmt, ...)
> +#endif

Please consider using dev_* helpers from <linux/device.h> instead.

> +
> +/* Additional NETIF_MSG_* categories */
> +#define NETIF_MSG_MMIO 0x8000000
> +
> +#define CH_DEVICE(devid, ssid, idx) \
> +	{ PCI_VENDOR_ID_CHELSIO, devid, PCI_ANY_ID, ssid, 0, 0, idx }
> +
> +#define SUPPORTED_PAUSE       (1 << 13)
> +#define SUPPORTED_LOOPBACK    (1 << 15)
> +
> +#define ADVERTISED_PAUSE      (1 << 13)
> +#define ADVERTISED_ASYM_PAUSE (1 << 14)
> +
> +/*
> + * Now that we have included the driver's main data structure,
> + * we typedef it to something the rest of the system understands.
> + */
> +typedef struct adapter adapter_t;

Please drop the typedef.

> +
> +#define DELAY_US(x) udelay(x)
> +
> +#define TPI_LOCK(adapter) spin_lock(&(adapter)->tpi_lock)
> +#define TPI_UNLOCK(adapter) spin_unlock(&(adapter)->tpi_lock)

Please drop the obfuscating wrappers.

> +void t1_elmer0_ext_intr(adapter_t *adapter);
> +void t1_link_changed(adapter_t *adapter, int port_id, int link_status,
> +			int speed, int duplex, int fc);
> +
> +static inline void DELAY_MS(unsigned long ms)
> +{
> +	unsigned long ticks = (ms * HZ + 999) / 1000 + 1;
> +
> +	while (ticks) {
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		ticks = schedule_timeout(ticks);
> +	}
> +}

Use msleep here.

> diff -puN /dev/null drivers/net/chelsio/subr.c
> --- /dev/null	2003-09-15 06:40:47.000000000 -0700
> +++ 25-akpm/drivers/net/chelsio/subr.c	2005-03-11 11:13:06.000000000 -0800
> +typedef struct {
> +	u32 format_version;
> +	u8 serial_number[16];
> +	u8 mac_base_address[6];
> +	u8 pad[2];           /* make multiple-of-4 size requirement explicit */
> +} chelsio_vpd_t;

Please drop the typedef.

				Pekka

Re: A new 10GB Ethernet Driver by Chelsio Communications

[Jeff Garzik]

Review comments (many) follow.

Still needs work.

Overall:  t1_write_reg_4 and friends should be converted to native 
readl/writel functions.  Don't use wrappers for low-level I/O functions.


> diff -puN /dev/null drivers/net/chelsio/ch_ethtool.h
> --- /dev/null	2004-08-10 19:55:00.000000000 -0600
> +++ 25-akpm/drivers/net/chelsio/ch_ethtool.h	2005-03-24 21:17:43.000000000 -0700
> +enum {
> +	ETHTOOL_SETREG,
> +	ETHTOOL_GETREG,

It's not a good idea to add "black hole" interfaces such as low level 
register read/write interfaces, generally.


> +	ETHTOOL_SETTPI,
> +	ETHTOOL_GETTPI,
> +	ETHTOOL_DEVUP,
> +	ETHTOOL_GETMTUTAB,
> +	ETHTOOL_SETMTUTAB,
> +	ETHTOOL_GETMTU,
> +	ETHTOOL_SET_PM,
> +	ETHTOOL_GET_PM,
> +	ETHTOOL_GET_TCAM,
> +	ETHTOOL_SET_TCAM,
> +	ETHTOOL_GET_TCB,
> +	ETHTOOL_READ_TCAM_WORD,
> +};

Please don't use the public ETHTOOL_ namespace for these values.


> +struct ethtool_reg {
> +	uint32_t cmd;
> +	uint32_t addr;
> +	uint32_t val;
> +};
> +
> +struct ethtool_mtus {
> +	uint32_t cmd;
> +	uint16_t mtus[NMTUS];
> +};
> +
> +struct ethtool_pm {
> +	uint32_t cmd;
> +	uint32_t tx_pg_sz;
> +	uint32_t tx_num_pg;
> +	uint32_t rx_pg_sz;
> +	uint32_t rx_num_pg;
> +	uint32_t pm_total;
> +};

some of these struct members are never used


> +struct ethtool_tcam {
> +	uint32_t cmd;
> +	uint32_t tcam_size;
> +	uint32_t nservers;
> +	uint32_t nroutes;
> +};
> +
> +struct ethtool_tcb {
> +	uint32_t cmd;
> +	uint32_t tcb_index;
> +	uint32_t tcb_data[TCB_WORDS];
> +};
> +
> +struct ethtool_tcam_word {
> +	uint32_t cmd;
> +	uint32_t addr;
> +	uint32_t buf[3];
> +};

1) don't use "ethtool_" namespace for these NIC-specific structs

2) unless this header is shared with userspace, you should be using 
u8/u16/u32 types.


> diff -puN /dev/null drivers/net/chelsio/common.h
> --- /dev/null	2004-08-10 19:55:00.000000000 -0600
> +++ 25-akpm/drivers/net/chelsio/common.h	2005-03-24 21:17:43.000000000 -0700
> +#define DIMOF(x) (sizeof(x)/sizeof(x[0]))

delete, and use standard ARRAY_SIZE() macro


> +#define NMTUS      8
> +#define MAX_NPORTS 4
> +#define TCB_SIZE   128

enum?


> +enum {
> +	CHBT_BOARD_7500,
> +	CHBT_BOARD_8000,
> +	CHBT_BOARD_CHT101,
> +	CHBT_BOARD_CHT110,
> +	CHBT_BOARD_CHT210,
> +	CHBT_BOARD_CHT204,
> +	CHBT_BOARD_N110,
> +	CHBT_BOARD_N210,
> +	CHBT_BOARD_COUGAR,
> +	CHBT_BOARD_6800,
> +	CHBT_BOARD_SIMUL
> +};
> +
> +enum {
> +	CHBT_TERM_FPGA,
> +	CHBT_TERM_T1,
> +	CHBT_TERM_T2,
> +	CHBT_TERM_T3
> +};
> +
> +enum {
> +	CHBT_MAC_CHELSIO_A,
> +	CHBT_MAC_IXF1010,
> +	CHBT_MAC_PM3393,
> +	CHBT_MAC_VSC7321,
> +	CHBT_MAC_DUMMY
> +};
> +
> +enum {
> +	CHBT_PHY_88E1041,
> +	CHBT_PHY_88E1111,
> +	CHBT_PHY_88X2010,
> +	CHBT_PHY_XPAK,
> +	CHBT_PHY_MY3126,
> +	CHBT_PHY_DUMMY
> +};
> +
> +enum {
> +	PAUSE_RX = 1,
> +	PAUSE_TX = 2,
> +	PAUSE_AUTONEG = 4
> +};

I believe these should be bits, in the "(1 << n)" notation?


> +/* Revisions of T1 chip */
> +#define TERM_T1A     0
> +#define TERM_T1B     1
> +#define TERM_T2      3

why not enums for these, too?


> +struct tp_params {
> +	unsigned int pm_size;
> +	unsigned int cm_size;
> +	unsigned int pm_rx_base;
> +	unsigned int pm_tx_base;
> +	unsigned int pm_rx_pg_size;
> +	unsigned int pm_tx_pg_size;
> +	unsigned int pm_rx_num_pgs;
> +	unsigned int pm_tx_num_pgs;
> +	unsigned int use_5tuple_mode;
> +};

where is this ever used?


> +struct sge_params {
> +	unsigned int cmdQ_size[2];
> +	unsigned int freelQ_size[2];
> +	unsigned int large_buf_capacity;
> +	unsigned int rx_coalesce_usecs;
> +	unsigned int last_rx_coalesce_raw;
> +	unsigned int default_rx_coalesce_usecs;
> +	unsigned int sample_interval_usecs;
> +	unsigned int coalesce_enable;
> +	unsigned int polling;
> +};
> +
> +struct mc5_params {
> +	unsigned int mode;	/* selects MC5 width */
> +	unsigned int nservers;	/* size of server region */
> +	unsigned int nroutes;	/* size of routing region */
> +};
> +
> +/* Default MC5 region sizes */
> +#define DEFAULT_SERVER_REGION_LEN 256
> +#define DEFAULT_RT_REGION_LEN 1024

enum?


> +struct pci_params {
> +	unsigned short speed;
> +	unsigned char  width;
> +	unsigned char  is_pcix;
> +};

please don't use "pci_" namespace.

Also, typically you want a boolean variable defined as a bit flag in an 
'unsigned long' variable, or at least a bitfield "unsigned foo : 1;".


> +struct adapter_params {
> +	struct sge_params sge;
> +	struct mc5_params mc5;
> +	struct tp_params  tp;
> +	struct pci_params pci;
> +
> +	const struct board_info *brd_info;
> +
> +	unsigned short mtus[NMTUS];
> +	unsigned int   nports;         /* # of ethernet ports */
> +	unsigned int   stats_update_period;
> +	unsigned short chip_revision;
> +	unsigned char  chip_version;
> +	unsigned char  is_asic;
> +};

ditto the above comment about booleans, for 'is_asic'


> +struct pci_err_cnt {
> +	unsigned int master_parity_err;
> +	unsigned int sig_target_abort;
> +	unsigned int rcv_target_abort;
> +	unsigned int rcv_master_abort;
> +	unsigned int sig_sys_err;
> +	unsigned int det_parity_err;
> +	unsigned int pio_parity_err;
> +	unsigned int wf_parity_err;
> +	unsigned int rf_parity_err;
> +	unsigned int cf_parity_err;
> +};

* counters should typically be unsigned long

* don't use "pci_" namespace


> +struct link_config {
> +	unsigned int   supported;        /* link capabilities */
> +	unsigned int   advertising;      /* advertised capabilities */
> +	unsigned short requested_speed;  /* speed user has requested */
> +	unsigned short speed;            /* actual link speed */
> +	unsigned char  requested_duplex; /* duplex user has requested */
> +	unsigned char  duplex;           /* actual link duplex */
> +	unsigned char  requested_fc;     /* flow control user has requested */
> +	unsigned char  fc;               /* actual link flow control */
> +	unsigned char  autoneg;          /* autonegotiating? */
> +};
> +
> +#define SPEED_INVALID   0xffff
> +#define DUPLEX_INVALID  0xff
> +
> +struct mdio_ops;
> +struct gmac;
> +struct gphy;
> +
> +struct board_info {
> +	unsigned char           board;
> +	unsigned char           port_number;
> +	unsigned long           caps;
> +	unsigned char           chip_term;
> +	unsigned char           chip_mac;
> +	unsigned char           chip_phy;
> +	unsigned int            clock_core;
> +	unsigned int            clock_mc3;
> +	unsigned int            clock_mc4;
> +	unsigned int            espi_nports;
> +	unsigned int            clock_cspi;
> +	unsigned int            clock_elmer0;
> +	unsigned char           mdio_mdien;
> +	unsigned char           mdio_mdiinv;
> +	unsigned char           mdio_mdc;
> +	unsigned char           mdio_phybaseaddr;
> +	struct gmac            *gmac;
> +	struct gphy            *gphy;
> +	struct mdio_ops	       *mdio_ops;

can this be made 'const' ?


> +	const char             *desc;
> +};
> +
> +#include "osdep.h"
> +
> +#ifndef PCI_VENDOR_ID_CHELSIO
> +#define PCI_VENDOR_ID_CHELSIO 0x1425
> +#endif

remove this, and patch include/linux/pci_ids.h

> +int t1_tpi_write(adapter_t *adapter, u32 addr, u32 value);
> +int t1_tpi_read(adapter_t *adapter, u32 addr, u32 *value);
> +
> +void t1_interrupts_enable(adapter_t *adapter);
> +void t1_interrupts_disable(adapter_t *adapter);
> +void t1_interrupts_clear(adapter_t *adapter);
> +int elmer0_ext_intr_handler(adapter_t *adapter);
> +int t1_slow_intr_handler(adapter_t *adapter);
> +
> +int t1_link_start(struct cphy *phy, struct cmac *mac, struct link_config *lc);
> +const struct board_info *t1_get_board_info(unsigned int board_id);
> +const struct board_info *t1_get_board_info_from_ids(unsigned int devid,
> +						    unsigned short ssid);
> +int t1_seeprom_read(adapter_t *adapter, u32 addr, u32 *data);
> +int t1_get_board_rev(adapter_t *adapter, const struct board_info *bi,
> +		     struct adapter_params *p);
> +int t1_init_hw_modules(adapter_t *adapter);
> +int t1_init_sw_modules(adapter_t *adapter, const struct board_info *bi);
> +void t1_free_sw_modules(adapter_t *adapter);
> +void t1_fatal_err(adapter_t *adapter);
> +#endif

please use the 'extern' prefix for these functions.

also, for all headers, please append a comment after the ending #endif, 
indicating what symbol the #endif closes.  example:

	#endif /* CHELSIO_COMMON_H */


> diff -puN /dev/null drivers/net/chelsio/cpl5_cmd.h
> --- /dev/null	2004-08-10 19:55:00.000000000 -0600
> +++ 25-akpm/drivers/net/chelsio/cpl5_cmd.h	2005-03-24 21:17:43.000000000 -0700
> +/*
> + * We want this header's alignment to be no more stringent than 2-byte aligned.
> + * All fields are u8 or u16 except for the length.  However that field is not
> + * used so we break it into 2 16-bit parts to easily meet our alignment needs.
> + */

Use

	struct foo {
		...
	} __attribute__ ((packed));

to eliminate the compiler's ABI struct member padding and alignment.


> +struct cpl_tx_pkt {
> +	__u8 opcode;
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +	__u8 iff:4;
> +	__u8 ip_csum_dis:1;
> +	__u8 l4_csum_dis:1;
> +	__u8 vlan_valid:1;
> +	__u8 rsvd:1;
> +#else
> +	__u8 rsvd:1;
> +	__u8 vlan_valid:1;
> +	__u8 l4_csum_dis:1;
> +	__u8 ip_csum_dis:1;
> +	__u8 iff:4;
> +#endif
> +	__u16 vlan;
> +	__u16 len_hi;
> +	__u16 len_lo;
> +};
> +
> +struct cpl_tx_pkt_lso {
> +	__u8 opcode;
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +	__u8 iff:4;
> +	__u8 ip_csum_dis:1;
> +	__u8 l4_csum_dis:1;
> +	__u8 vlan_valid:1;
> +	__u8 rsvd:1;
> +#else
> +	__u8 rsvd:1;
> +	__u8 vlan_valid:1;
> +	__u8 l4_csum_dis:1;
> +	__u8 ip_csum_dis:1;
> +	__u8 iff:4;
> +#endif
> +	__u16 vlan;
> +	__u32 len;
> +
> +	__u32 rsvd2;
> +	__u8 rsvd3;
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +	__u8 tcp_hdr_words:4;
> +	__u8 ip_hdr_words:4;
> +#else
> +	__u8 ip_hdr_words:4;
> +	__u8 tcp_hdr_words:4;
> +#endif
> +	__u16 eth_type_mss;
> +};
> +
> +struct cpl_rx_pkt {
> +	__u8 opcode;
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +	__u8 iff:4;
> +	__u8 csum_valid:1;
> +	__u8 bad_pkt:1;
> +	__u8 vlan_valid:1;
> +	__u8 rsvd:1;
> +#else
> +	__u8 rsvd:1;
> +	__u8 vlan_valid:1;
> +	__u8 bad_pkt:1;
> +	__u8 csum_valid:1;
> +	__u8 iff:4;
> +#endif
> +	__u16 csum;
> +	__u16 vlan;
> +	__u16 len;
> +};

two general comments:

1) prefer u8/u16/u32 kernel types to __u8/__u16/__u32 compiler types.

2) using bitflags in the code, rather than bitfields, are generally 
easier to maintain.  the choice is yours, but I encourage doing it like:

	struct foo {
		u32 bar;
	};

	...

	u32 tmp = le32_to_cpu(dma_structure->foo);
	u32 tmp1 = tmp & 0xf;	/* get lower 4 bits */
	u32 tmp2 = tmp & (1 << 30); /* get bit 30 */
	etc.


> +#endif
> diff -puN /dev/null drivers/net/chelsio/cxgb2.c
> --- /dev/null	2004-08-10 19:55:00.000000000 -0600
> +++ 25-akpm/drivers/net/chelsio/cxgb2.c	2005-03-24 21:17:43.000000000 -0700
> +static inline void cancel_mac_stats_update(struct adapter *ap)
> +{
> +	cancel_delayed_work(&ap->stats_update_task);
> +}
> +
> +#if BITS_PER_LONG == 64 && !defined(CONFIG_X86_64)
> +# define FMT64 "l"
> +#else
> +# define FMT64 "ll"
> +#endif
> +
> +# define DRV_TYPE ""

delete this, it is unused


> +# define MODULE_DESC "Chelsio Network Driver"

1) don't use the MODULE_xxx namespace for your own purposes

2) just include this string inline.  don't define a macro for it at all.


> +static char driver_name[]    = DRV_NAME;

delete this, and just use DRV_NAME inline.  The compiler should merge 
references to duplicate read-only strings.


> +static char driver_version[] = "2.1.0";

ditto the DRV_NAME issue.

> +#define PCI_DMA_64BIT ~0ULL
> +#define PCI_DMA_32BIT 0xffffffffULL

delete, and use DMA_{32,64}BIT_MASK in include/linux/dma-mapping.h.


> +#define MAX_CMDQ_ENTRIES 16384
> +#define MAX_CMDQ1_ENTRIES 1024
> +#define MAX_RX_BUFFERS 16384
> +#define MAX_RX_JUMBO_BUFFERS 16384
> +#define MAX_TX_BUFFERS_HIGH	16384U
> +#define MAX_TX_BUFFERS_LOW	1536U
> +#define MIN_FL_ENTRIES 32
> +
> +#define PORT_MASK ((1 << MAX_NPORTS) - 1)

enum?


> +#define DFLT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK | \
> +			 NETIF_MSG_TIMER | NETIF_MSG_IFDOWN | NETIF_MSG_IFUP |\
> +			 NETIF_MSG_RX_ERR | NETIF_MSG_TX_ERR)
> +
> +/*
> + * The EEPROM is actually bigger but only the first few bytes are used so we
> + * only report those.
> + */
> +#define EEPROM_SIZE 32
> +
> +MODULE_DESCRIPTION(MODULE_DESC);
> +MODULE_AUTHOR("Chelsio Communications");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(pci, t1_pci_tbl);
> +
> +static int dflt_msg_enable = DFLT_MSG_ENABLE;
> +
> +MODULE_PARM(dflt_msg_enable, "i");
> +MODULE_PARM_DESC(dflt_msg_enable, "Chelsio T1 message enable bitmap");
> +
> +
> +static const char pci_speed[][4] = {
> +	"33", "66", "100", "133"
> +};
> +
> +/*
> + * Setup MAC to receive the types of packets we want.
> + */
> +static void t1_set_rxmode(struct net_device *dev)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct cmac *mac = adapter->port[dev->if_port].mac;
> +	struct t1_rx_mode rm;
> +
> +	rm.dev = dev;
> +	rm.idx = 0;
> +	rm.list = dev->mc_list;
> +	mac->ops->set_rx_mode(mac, &rm);
> +}

delete mac->ops->set_rx_mode() hook, and call pm3393_set_rx_mode() directly


> +static void link_report(struct port_info *p)
> +{
> +	if (!netif_carrier_ok(p->dev))
> +		printk(KERN_INFO "%s: link is down\n", p->dev->name);
> +	else {
> +		const char *s = "10 Mbps";
> +
> +		switch (p->link_config.speed) {
> +			case SPEED_10000: s = "10 Gbps"; break;
> +			case SPEED_1000:  s = "1000 Mbps"; break;
> +			case SPEED_100:   s = "100 Mbps"; break;
> +		}
> +
> +		printk(KERN_INFO "%s: link is up at %s, %s duplex\n",
> +		       p->dev->name, s,
> +		       p->link_config.duplex == DUPLEX_FULL ? "full" : "half");
> +	}
> +}

Consider using a printk() message as close as possible to the one in 
drivers/net/mii.c:

                 printk(KERN_INFO "%s: link up, %sMbps, %s-duplex, lpa 
0x%04X\n",


> +void t1_link_changed(struct adapter *adapter, int port_id, int link_stat,
> +			int speed, int duplex, int pause)
> +{
> +	struct port_info *p = &adapter->port[port_id];
> +
> +	if (link_stat != netif_carrier_ok(p->dev)) {
> +		if (link_stat)
> +			netif_carrier_on(p->dev);
> +		else
> +			netif_carrier_off(p->dev);
> +		link_report(p);
> +
> +	}
> +}
> +
> +static void link_start(struct port_info *p)
> +{
> +	struct cmac *mac = p->mac;
> +
> +	mac->ops->reset(mac);
> +	if (mac->ops->macaddress_set)
> +		mac->ops->macaddress_set(mac, p->dev->dev_addr);
> +	t1_set_rxmode(p->dev);
> +	t1_link_start(p->phy, mac, &p->link_config);
> +	mac->ops->enable(mac, MAC_DIRECTION_RX | MAC_DIRECTION_TX);
> +}

delete hooks, call MAC functions directly


> +static void enable_hw_csum(struct adapter *adapter)
> +{
> +	if (adapter->flags & TSO_CAPABLE)
> +		t1_tp_set_ip_checksum_offload(adapter->tp, 1); /* for TSO only */
> +	if (adapter->flags & UDP_CSUM_CAPABLE)
> +		t1_tp_set_udp_checksum_offload(adapter->tp, 1);
> +	t1_tp_set_tcp_checksum_offload(adapter->tp, 1);
> +}
> +
> +/*
> + * Things to do upon first use of a card.
> + * This must run with the rtnl lock held.
> + */
> +static int cxgb_up(struct adapter *adapter)
> +{
> +	int err = 0;
> +
> +	if (!(adapter->flags & FULL_INIT_DONE)) {
> +		err = t1_init_hw_modules(adapter);
> +		if (err)
> +			goto out_err;
> +
> +		enable_hw_csum(adapter);
> +		adapter->flags |= FULL_INIT_DONE;
> +	}
> +
> +	t1_interrupts_clear(adapter);
> +
> +	if ((err = request_irq(adapter->pdev->irq, &t1_interrupt, SA_SHIRQ,
> +			       adapter->name, adapter)))
> +		goto out_err;
> +
> +	t1_sge_start(adapter->sge);
> +	t1_interrupts_enable(adapter);
> +
> +	err = 0;
> + out_err:
> +	return err;

leak on error.  You should free the resources allocated on 
t1_init_hw_modules().


> + * Release resources when all the ports have been stopped.
> + */
> +static void cxgb_down(struct adapter *adapter)
> +{
> +	t1_sge_stop(adapter->sge);
> +	t1_interrupts_disable(adapter);
> +	free_irq(adapter->pdev->irq, adapter);
> +}
> +
> +static int cxgb_open(struct net_device *dev)
> +{
> +	int err;
> +	struct adapter *adapter = dev->priv;
> +	int other_ports = adapter->open_device_map & PORT_MASK;
> +
> +	if (!adapter->open_device_map && (err = cxgb_up(adapter)) < 0)
> +		return err;
> +
> +	__set_bit(dev->if_port, &adapter->open_device_map);

do you need atomicity for adapter->open_device_map ?

If not, just open-code the bitsetting:

	adapter->open_device_map |= (1 << n)


> +	link_start(&adapter->port[dev->if_port]);
> +	netif_start_queue(dev);
> +	if (!other_ports && adapter->params.stats_update_period)
> +		schedule_mac_stats_update(adapter,
> +					  adapter->params.stats_update_period);
> +	return 0;
> +}
> +
> +static int cxgb_close(struct net_device *dev)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct port_info *p = &adapter->port[dev->if_port];
> +	struct cmac *mac = p->mac;
> +
> +	netif_stop_queue(dev);
> +	mac->ops->disable(mac, MAC_DIRECTION_TX | MAC_DIRECTION_RX);
> +	netif_carrier_off(dev);

delete hook


> +	clear_bit(dev->if_port, &adapter->open_device_map);
> +	if (adapter->params.stats_update_period &&
> +	    !(adapter->open_device_map & PORT_MASK)) {
> +		/* Stop statistics accumulation. */
> +		smp_mb__after_clear_bit();
> +		spin_lock(&adapter->work_lock);   /* sync with update task */
> +		spin_unlock(&adapter->work_lock);
> +		cancel_mac_stats_update(adapter);

reconsider.  instead, you should flush the workqueue.

and also, the atomicity of adapter->open_device_map isn't needed.


> +	if (!adapter->open_device_map)
> +		cxgb_down(adapter);
> +	return 0;
> +}
> +
> +static struct net_device_stats *t1_get_stats(struct net_device *dev)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct port_info *p = &adapter->port[dev->if_port];
> +	struct net_device_stats *ns = &p->netstats;
> +	const struct cmac_statistics *pstats;
> +
> +	/* Do a full update of the MAC stats */
> +	pstats = p->mac->ops->statistics_update(p->mac,
> +						      MAC_STATS_UPDATE_FULL);
> +
> +	ns->tx_packets = pstats->TxUnicastFramesOK +
> +		pstats->TxMulticastFramesOK + pstats->TxBroadcastFramesOK;
> +
> +	ns->rx_packets = pstats->RxUnicastFramesOK +
> +		pstats->RxMulticastFramesOK + pstats->RxBroadcastFramesOK;
> +
> +	ns->tx_bytes = pstats->TxOctetsOK;
> +	ns->rx_bytes = pstats->RxOctetsOK;
> +
> +	ns->tx_errors = pstats->TxLateCollisions + pstats->TxLengthErrors +
> +		pstats->TxUnderrun + pstats->TxFramesAbortedDueToXSCollisions;
> +	ns->rx_errors = pstats->RxDataErrors + pstats->RxJabberErrors +
> +		pstats->RxFCSErrors + pstats->RxAlignErrors +
> +		pstats->RxSequenceErrors + pstats->RxFrameTooLongErrors +
> +		pstats->RxSymbolErrors + pstats->RxRuntErrors;
> +
> +	ns->multicast  = pstats->RxMulticastFramesOK;
> +	ns->collisions = pstats->TxTotalCollisions;
> +
> +	/* detailed rx_errors */
> +	ns->rx_length_errors = pstats->RxFrameTooLongErrors +
> +		pstats->RxJabberErrors;
> +	ns->rx_over_errors   = 0;
> +	ns->rx_crc_errors    = pstats->RxFCSErrors;
> +	ns->rx_frame_errors  = pstats->RxAlignErrors;
> +	ns->rx_fifo_errors   = 0;
> +	ns->rx_missed_errors = 0;
> +
> +	/* detailed tx_errors */
> +	ns->tx_aborted_errors   = pstats->TxFramesAbortedDueToXSCollisions;
> +	ns->tx_carrier_errors   = 0;
> +	ns->tx_fifo_errors      = pstats->TxUnderrun;
> +	ns->tx_heartbeat_errors = 0;
> +	ns->tx_window_errors    = pstats->TxLateCollisions;
> +	return ns;
> +}
> +
> +static u32 get_msglevel(struct net_device *dev)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	return adapter->msg_enable;
> +}
> +
> +static void set_msglevel(struct net_device *dev, u32 val)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	adapter->msg_enable = val;
> +}
> +
> +static char stats_strings[][ETH_GSTRING_LEN] = {
> +	"TxOctetsOK",
> +	"TxOctetsBad",
> +	"TxUnicastFramesOK",
> +	"TxMulticastFramesOK",
> +	"TxBroadcastFramesOK",
> +	"TxPauseFrames",
> +	"TxFramesWithDeferredXmissions",
> +	"TxLateCollisions",
> +	"TxTotalCollisions",
> +	"TxFramesAbortedDueToXSCollisions",
> +	"TxUnderrun",
> +	"TxLengthErrors",
> +	"TxInternalMACXmitError",
> +	"TxFramesWithExcessiveDeferral",
> +	"TxFCSErrors",
> +
> +	"RxOctetsOK",
> +	"RxOctetsBad",
> +	"RxUnicastFramesOK",
> +	"RxMulticastFramesOK",
> +	"RxBroadcastFramesOK",
> +	"RxPauseFrames",
> +	"RxFCSErrors",
> +	"RxAlignErrors",
> +	"RxSymbolErrors",
> +	"RxDataErrors",
> +	"RxSequenceErrors",
> +	"RxRuntErrors",
> +	"RxJabberErrors",
> +	"RxInternalMACRcvError",
> +	"RxInRangeLengthErrors",
> +	"RxOutOfRangeLengthField",
> +	"RxFrameTooLongErrors"
> +};
> +
> +static void get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	strcpy(info->driver, driver_name);
> +	strcpy(info->version, driver_version);
> +	strcpy(info->fw_version, "N/A");
> +	strcpy(info->bus_info, pci_name(adapter->pdev));
> +}
> +
> +static int get_stats_count(struct net_device *dev)
> +{
> +	return ARRAY_SIZE(stats_strings);
> +}
> +
> +static void get_strings(struct net_device *dev, u32 stringset, u8 *data)
> +{
> +	if (stringset == ETH_SS_STATS)
> +		memcpy(data, stats_strings, sizeof(stats_strings));
> +}
> +
> +static void get_stats(struct net_device *dev, struct ethtool_stats *stats,
> +		      u64 *data)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct cmac *mac = adapter->port[dev->if_port].mac;
> +	const struct cmac_statistics *s;
> +
> +	s = mac->ops->statistics_update(mac, MAC_STATS_UPDATE_FULL);

delete hook

> +	*data++ = s->TxOctetsOK;
> +	*data++ = s->TxOctetsBad;
> +	*data++ = s->TxUnicastFramesOK;
> +	*data++ = s->TxMulticastFramesOK;
> +	*data++ = s->TxBroadcastFramesOK;
> +	*data++ = s->TxPauseFrames;
> +	*data++ = s->TxFramesWithDeferredXmissions;
> +	*data++ = s->TxLateCollisions;
> +	*data++ = s->TxTotalCollisions;
> +	*data++ = s->TxFramesAbortedDueToXSCollisions;
> +	*data++ = s->TxUnderrun;
> +	*data++ = s->TxLengthErrors;
> +	*data++ = s->TxInternalMACXmitError;
> +	*data++ = s->TxFramesWithExcessiveDeferral;
> +	*data++ = s->TxFCSErrors;
> +
> +	*data++ = s->RxOctetsOK;
> +	*data++ = s->RxOctetsBad;
> +	*data++ = s->RxUnicastFramesOK;
> +	*data++ = s->RxMulticastFramesOK;
> +	*data++ = s->RxBroadcastFramesOK;
> +	*data++ = s->RxPauseFrames;
> +	*data++ = s->RxFCSErrors;
> +	*data++ = s->RxAlignErrors;
> +	*data++ = s->RxSymbolErrors;
> +	*data++ = s->RxDataErrors;
> +	*data++ = s->RxSequenceErrors;
> +	*data++ = s->RxRuntErrors;
> +	*data++ = s->RxJabberErrors;
> +	*data++ = s->RxInternalMACRcvError;
> +	*data++ = s->RxInRangeLengthErrors;
> +	*data++ = s->RxOutOfRangeLengthField;
> +	*data++ = s->RxFrameTooLongErrors;

other drivers insert a programmer sanity check, to ensure that the 
number of stat values output in this function matches the number 
returned by the ->get_stats_count() hook.


> +static int get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct port_info *p = &adapter->port[dev->if_port];
> +
> +	cmd->supported = p->link_config.supported;
> +	cmd->advertising = p->link_config.advertising;
> +
> +	if (netif_carrier_ok(dev)) {
> +		cmd->speed = p->link_config.speed;
> +		cmd->duplex = p->link_config.duplex;
> +	} else {
> +		cmd->speed = -1;
> +		cmd->duplex = -1;
> +	}

I'm concerned about this -1 value, which userland isn't really expecting.

It appears e1000 is doing this too :(


> +	cmd->port = (cmd->supported & SUPPORTED_TP) ? PORT_TP : PORT_FIBRE;
> +	cmd->phy_address = p->phy->addr;
> +	cmd->transceiver = XCVR_EXTERNAL;
> +	cmd->autoneg = p->link_config.autoneg;
> +	cmd->maxtxpkt = 0;
> +	cmd->maxrxpkt = 0;
> +	return 0;
> +}
> +
> +static int speed_duplex_to_caps(int speed, int duplex)
> +{
> +	int cap = 0;
> +
> +	switch (speed) {
> +	case SPEED_10:
> +		if (duplex == DUPLEX_FULL)
> +			cap = SUPPORTED_10baseT_Full;
> +		else
> +			cap = SUPPORTED_10baseT_Half;
> +		break;
> +	case SPEED_100:
> +		if (duplex == DUPLEX_FULL)
> +			cap = SUPPORTED_100baseT_Full;
> +		else
> +			cap = SUPPORTED_100baseT_Half;
> +		break;
> +	case SPEED_1000:
> +		if (duplex == DUPLEX_FULL)
> +			cap = SUPPORTED_1000baseT_Full;
> +		else
> +			cap = SUPPORTED_1000baseT_Half;
> +		break;
> +	case SPEED_10000:
> +		if (duplex == DUPLEX_FULL)
> +			cap = SUPPORTED_10000baseT_Full;
> +	}
> +	return cap;
> +}

might be nice to make this a generic function.

but leave that until after the driver is merged.



> +static int set_coalesce(struct net_device *dev, struct ethtool_coalesce *c)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	unsigned int sge_coalesce_usecs = 0;
> +
> +	sge_coalesce_usecs = adapter->params.sge.last_rx_coalesce_raw;
> +	sge_coalesce_usecs /= board_info(adapter)->clock_core / 1000000;

do you _ever_ use clock_core value, when you are not dividing it by some 
constant?

It seems like the variable should be scaled once, rather than performing 
a divide each time you use it.


> +	if ( (adapter->params.sge.coalesce_enable && !c->use_adaptive_rx_coalesce) &&
> +	     (c->rx_coalesce_usecs == sge_coalesce_usecs) ) {
> +		adapter->params.sge.rx_coalesce_usecs =
> +			adapter->params.sge.default_rx_coalesce_usecs;
> +	} else {
> +		adapter->params.sge.rx_coalesce_usecs = c->rx_coalesce_usecs;
> +	}

style:  singleton C statements shouldn't be enclosed in braces {}


> +	adapter->params.sge.last_rx_coalesce_raw = adapter->params.sge.rx_coalesce_usecs;
> +	adapter->params.sge.last_rx_coalesce_raw *= (board_info(adapter)->clock_core / 1000000);
> +	adapter->params.sge.sample_interval_usecs = c->rate_sample_interval;
> +	adapter->params.sge.coalesce_enable = c->use_adaptive_rx_coalesce;
> +	t1_sge_set_coalesce_params(adapter->sge, &adapter->params.sge);
> +	return 0;
> +}
> +

> +static int ethtool_ioctl(struct net_device *dev, void *useraddr)
> +{
> +	u32 cmd;
> +	struct adapter *adapter = dev->priv;
> +
> +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> +		return -EFAULT;
> +
> +	switch (cmd) {
> +	case ETHTOOL_SETREG: {
> +		struct ethtool_reg edata;
> +
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +		if (copy_from_user(&edata, useraddr, sizeof(edata)))
> +			return -EFAULT;
> +		if ((edata.addr & 3) != 0 || edata.addr >= adapter->mmio_len)
> +			return -EINVAL;
> +		if (edata.addr == A_ESPI_MISC_CONTROL)
> +			t1_espi_set_misc_ctrl(adapter, edata.val);
> +		else {
> +			if (edata.addr == 0x950)
> +				t1_sge_set_ptimeout(adapter, edata.val);
> +			else
> +				writel(edata.val, adapter->regs + edata.addr);
> +		}
> +		break;
> +	}
> +	case ETHTOOL_GETREG: {
> +		struct ethtool_reg edata;
> +
> +		if (copy_from_user(&edata, useraddr, sizeof(edata)))
> +			return -EFAULT;
> +		if ((edata.addr & 3) != 0 || edata.addr >= adapter->mmio_len)
> +			return -EINVAL;
> +		if (edata.addr >= 0x900 && edata.addr <= 0x93c)
> +			edata.val = t1_espi_get_mon(adapter, edata.addr, 1);
> +		else {
> +			if (edata.addr == 0x950)
> +				edata.val = t1_sge_get_ptimeout(adapter);
> +			else
> +				edata.val = readl(adapter->regs + edata.addr);
> +		}
> +		if (copy_to_user(useraddr, &edata, sizeof(edata)))
> +			return -EFAULT;
> +		break;
> +	}
> +	case ETHTOOL_SETTPI: {
> +		struct ethtool_reg edata;
> +
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +		if (copy_from_user(&edata, useraddr, sizeof(edata)))
> +			return -EFAULT;
> +		if ((edata.addr & 3) != 0)
> +			return -EINVAL;
> +		t1_tpi_write(adapter, edata.addr, edata.val);
> +		break;
> +	}
> +	case ETHTOOL_GETTPI: {
> +		struct ethtool_reg edata;
> +
> +		if (copy_from_user(&edata, useraddr, sizeof(edata)))
> +			return -EFAULT;
> +		if ((edata.addr & 3) != 0)
> +			return -EINVAL;
> +		t1_tpi_read(adapter, edata.addr, &edata.val);
> +		if (copy_to_user(useraddr, &edata, sizeof(edata)))
> +			return -EFAULT;
> +		break;
> +	}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}

eliminate get/set register ioctls.  don't use ethtool namespace.


> +static int t1_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct mii_ioctl_data *data = (struct mii_ioctl_data *)&req->ifr_data;
> +
> +	switch (cmd) {
> +	case SIOCGMIIPHY:
> +		data->phy_id = adapter->port[dev->if_port].phy->addr;
> +		/* FALLTHRU */
> +	case SIOCGMIIREG: {
> +		struct cphy *phy = adapter->port[dev->if_port].phy;
> +		u32 val;
> +
> +		if (!phy->mdio_read) return -EOPNOTSUPP;

one C statement per line


> +		phy->mdio_read(adapter, data->phy_id, 0, data->reg_num & 0x1f,
> +			       &val);
> +		data->val_out = val;
> +		break;
> +	}
> +	case SIOCSMIIREG: {
> +		struct cphy *phy = adapter->port[dev->if_port].phy;
> +
> +		if (!capable(CAP_NET_ADMIN)) return -EPERM;
> +		if (!phy->mdio_write) return -EOPNOTSUPP;

one C statement per line


> +		phy->mdio_write(adapter, data->phy_id, 0, data->reg_num & 0x1f,
> +				data->val_in);
> +		break;
> +	}
> +
> +	case SIOCCHETHTOOL:
> +		return ethtool_ioctl(dev, (void *)req->ifr_data);

don't obfuscate.  Use SIOCDEVPRIVATE.


> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int t1_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +	int ret;
> +	struct adapter *adapter = dev->priv;
> +	struct cmac *mac = adapter->port[dev->if_port].mac;
> +
> +	if (!mac->ops->set_mtu)
> +		return -EOPNOTSUPP;

delete hook


> +	if (new_mtu < 68)
> +		return -EINVAL;
> +	if ((ret = mac->ops->set_mtu(mac, new_mtu)))
> +		return ret;

ditto


> +	dev->mtu = new_mtu;
> +	return 0;
> +}
> +
> +static int t1_set_mac_addr(struct net_device *dev, void *p)
> +{
> +	struct adapter *adapter = dev->priv;
> +	struct cmac *mac = adapter->port[dev->if_port].mac;
> +	struct sockaddr *addr = p;
> +
> +	if (!mac->ops->macaddress_set)
> +		return -EOPNOTSUPP;
> +
> +	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
> +	mac->ops->macaddress_set(mac, dev->dev_addr);
> +	return 0;

ditto


> +#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
> +static void vlan_rx_register(struct net_device *dev,
> +				   struct vlan_group *grp)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	spin_lock_irq(&adapter->async_lock);
> +	adapter->vlan_grp = grp;
> +	t1_set_vlan_accel(adapter, grp != NULL);
> +	spin_unlock_irq(&adapter->async_lock);
> +}
> +
> +static void vlan_rx_kill_vid(struct net_device *dev, unsigned short vid)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	spin_lock_irq(&adapter->async_lock);
> +	if (adapter->vlan_grp)
> +		adapter->vlan_grp->vlan_devices[vid] = NULL;
> +	spin_unlock_irq(&adapter->async_lock);
> +}
> +#endif
> +
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +static void t1_netpoll(struct net_device *dev)
> +{
> +	struct adapter *adapter = dev->priv;
> +
> +	t1_interrupt(adapter->pdev->irq, adapter, NULL);
> +}
> +#endif

missing disable/enable_irq() calls


> +/*
> + * Periodic accumulation of MAC statistics.  This is used only if the MAC
> + * does not have any other way to prevent stats counter overflow.
> + */

counters will always overflow.

The stats reader is responsible to collecting stats at a rate rapid 
enough to compensate for this.
> 
> +static void ext_intr_task(void *data)
> +{
> +       u32 enable;
> +       struct adapter *adapter = data;
> +
> +       elmer0_ext_intr_handler(adapter);
> +
> +       /* Now reenable external interrupts */
> +       t1_write_reg_4(adapter, A_PL_CAUSE, F_PL_INTR_EXT);
> +       enable = t1_read_reg_4(adapter, A_PL_ENABLE);
> +       t1_write_reg_4(adapter, A_PL_ENABLE, enable | F_PL_INTR_EXT);
> +       adapter->slow_intr_mask |= F_PL_INTR_EXT;
> +}

> +/*
> + * Interrupt-context handler for elmer0 external interrupts.
> + */
> +void t1_elmer0_ext_intr(struct adapter *adapter)
> +{
> +	u32 enable = t1_read_reg_4(adapter, A_PL_ENABLE);
> +
> +	/*
> +	 * Schedule a task to handle external interrupts as we require
> +	 * a process context.  We disable EXT interrupts in the interim
> +	 * and let the task reenable them when it's done.
> +	 */
> +	adapter->slow_intr_mask &= ~F_PL_INTR_EXT;
> +	t1_write_reg_4(adapter, A_PL_ENABLE, enable & ~F_PL_INTR_EXT);
> +	schedule_work(&adapter->ext_intr_handler_task);
> +}

the above two functions have a tiny race window


> +static int __devinit init_one(struct pci_dev *pdev,
> +			      const struct pci_device_id *ent)
> +{
> +	static int version_printed;
> +
> +	int i, err, pci_using_dac = 0;
> +	unsigned long mmio_start, mmio_len;
> +	const struct board_info *bi;
> +	struct adapter *adapter = NULL;
> +	struct port_info *pi;
> +
> +	if (!version_printed) {
> +		printk(KERN_INFO "%s - version %s\n", driver_string,
> +		       driver_version);
> +		++version_printed;
> +	}
> +
> +	err = pci_enable_device(pdev);
> +	if (err)
> +		return err;
> +
> +	if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
> +		CH_ERR("%s: cannot find PCI device memory base address\n",
> +		       pci_name(pdev));
> +		err = -ENODEV;
> +		goto out_disable_pdev;
> +	}
> +
> +	if (!pci_set_dma_mask(pdev, PCI_DMA_64BIT)) {
> +		pci_using_dac = 1;
> +		if (pci_set_consistent_dma_mask(pdev, PCI_DMA_64BIT)) {
> +			CH_ERR("%s: unable to obtain 64-bit DMA for"
> +			       "consistent allocations\n", pci_name(pdev));
> +			err = -ENODEV;
> +			goto out_disable_pdev;
> +		}
> +	} else if ((err = pci_set_dma_mask(pdev, PCI_DMA_32BIT)) != 0) {
> +		CH_ERR("%s: no usable DMA configuration\n", pci_name(pdev));
> +		goto out_disable_pdev;
> +	}

missing call to pci_set_consistent_dma_mask()

see drivers/net/tg3.c for an example.


> +	err = pci_request_regions(pdev, driver_name);
> +	if (err) {
> +		CH_ERR("%s: cannot obtain PCI resources\n", pci_name(pdev));
> +		goto out_disable_pdev;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	mmio_start = pci_resource_start(pdev, 0);
> +	mmio_len = pci_resource_len(pdev, 0);
> +	bi = t1_get_board_info(ent->driver_data);
> +
> +	for (i = 0; i < bi->port_number; ++i) {
> +		struct net_device *netdev;
> +
> +		netdev = alloc_etherdev(adapter ? 0 : sizeof(*adapter));
> +		if (!netdev) {
> +			err = -ENOMEM;
> +			goto out_free_dev;
> +		}
> +
> +		SET_MODULE_OWNER(netdev);
> +		SET_NETDEV_DEV(netdev, &pdev->dev);
> +
> +		if (!adapter) {
> +			adapter = netdev->priv;
> +			adapter->pdev = pdev;
> +			adapter->port[0].dev = netdev;  /* so we don't leak it */
> +
> +			adapter->regs = ioremap(mmio_start, mmio_len);
> +			if (!adapter->regs) {
> +				CH_ERR("%s: cannot map device registers\n",
> +				       pci_name(pdev));
> +				err = -ENOMEM;
> +				goto out_free_dev;
> +			}
> +
> +			if (t1_get_board_rev(adapter, bi, &adapter->params)) {
> +				err = -ENODEV;	  /* Can't handle this chip rev */
> +				goto out_free_dev;
> +			}
> +
> +			adapter->name = pci_name(pdev);
> +			adapter->msg_enable = dflt_msg_enable;
> +			adapter->mmio_len = mmio_len;
> +
> +			init_MUTEX(&adapter->mib_mutex);
> +			spin_lock_init(&adapter->tpi_lock);
> +			spin_lock_init(&adapter->work_lock);
> +			spin_lock_init(&adapter->async_lock);
> +
> +			INIT_WORK(&adapter->ext_intr_handler_task,
> +				  ext_intr_task, adapter);
> +			INIT_WORK(&adapter->stats_update_task, mac_stats_task,
> +				  adapter);
> +
> +			pci_set_drvdata(pdev, netdev);
> +
> +		}
> +
> +		pi = &adapter->port[i];
> +		pi->dev = netdev;
> +		netif_carrier_off(netdev);
> +		netdev->irq = pdev->irq;
> +		netdev->if_port = i;

Why are you setting this?  Does the value actually affect any operations?


> +		netdev->mem_start = mmio_start;
> +		netdev->mem_end = mmio_start + mmio_len - 1;

don't set this at all


> +		netdev->priv = adapter;
> +		netdev->features |= NETIF_F_SG | NETIF_F_IP_CSUM;
> +		adapter->flags |= RX_CSUM_ENABLED | TCP_CSUM_CAPABLE;
> +		if (pci_using_dac)
> +			netdev->features |= NETIF_F_HIGHDMA;
> +		if (vlan_tso_capable(adapter)) {
> +			adapter->flags |= UDP_CSUM_CAPABLE;
> +#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
> +			adapter->flags |= VLAN_ACCEL_CAPABLE;
> +			netdev->features |=
> +				NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
> +			netdev->vlan_rx_register = vlan_rx_register;
> +			netdev->vlan_rx_kill_vid = vlan_rx_kill_vid;
> +#endif
> +			adapter->flags |= TSO_CAPABLE;
> +			netdev->features |= NETIF_F_TSO;
> +		}
> +
> +		netdev->open = cxgb_open;
> +		netdev->stop = cxgb_close;
> +		netdev->hard_start_xmit = t1_start_xmit;
> +		netdev->hard_header_len += (adapter->flags & TSO_CAPABLE) ?
> +			sizeof(struct cpl_tx_pkt_lso) :
> +			sizeof(struct cpl_tx_pkt);

it's a bit unusual to be messing with hard_header_len.

probably unwise.


> +		netdev->get_stats = t1_get_stats;
> +		netdev->set_multicast_list = t1_set_rxmode;
> +		netdev->do_ioctl = t1_ioctl;
> +		netdev->change_mtu = t1_change_mtu;
> +		netdev->set_mac_address = t1_set_mac_addr;
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +		netdev->poll_controller = t1_netpoll;
> +#endif
> +		netdev->weight = 64;
> +
> +		SET_ETHTOOL_OPS(netdev, &t1_ethtool_ops);
> +	}
> +
> +	if (t1_init_sw_modules(adapter, bi) < 0) {
> +		err = -ENODEV;
> +		goto out_free_dev;
> +	}
> +
> +	/*
> +	 * The card is now ready to go.  If any errors occur during device
> +	 * registration we do not fail the whole card but rather proceed only
> +	 * with the ports we manage to register successfully.  However we must
> +	 * register at least one net device.
> +	 */
> +	for (i = 0; i < bi->port_number; ++i) {
> +		err = register_netdev(adapter->port[i].dev);
> +		if (err)
> +			CH_WARN("%s: cannot register net device %s, skipping\n",
> +				pci_name(pdev), adapter->port[i].dev->name);
> +		else {
> +			/*
> +			 * Change the name we use for messages to the name of
> +			 * the first successfully registered interface.
> +			 */
> +			if (!adapter->registered_device_map)
> +				adapter->name = adapter->port[i].dev->name;
> +
> +			__set_bit(i, &adapter->registered_device_map);
> +		}
> +	}
> +	if (!adapter->registered_device_map) {
> +		CH_ERR("%s: could not register any net devices\n",
> +		       pci_name(pdev));
> +		goto out_release_adapter_res;
> +	}
> +
> +	printk(KERN_INFO "%s: %s (rev %d), %s %dMHz/%d-bit\n", adapter->name,
> +	       bi->desc, adapter->params.chip_revision,
> +	       adapter->params.pci.is_pcix ? "PCIX" : "PCI",
> +	       adapter->params.pci.speed, adapter->params.pci.width);
> +	return 0;
> +
> + out_release_adapter_res:
> +	t1_free_sw_modules(adapter);
> + out_free_dev:
> +	if (adapter) {
> +		if (adapter->regs)
> +			iounmap(adapter->regs);
> +		for (i = bi->port_number - 1; i >= 0; --i)
> +			if (adapter->port[i].dev)
> +				free_netdev(adapter->port[i].dev);
> +	}
> +	pci_release_regions(pdev);
> + out_disable_pdev:
> +	pci_disable_device(pdev);
> +	pci_set_drvdata(pdev, NULL);
> +	return err;
> +}
> +
> +static inline void t1_sw_reset(struct pci_dev *pdev)
> +{
> +	pci_write_config_dword(pdev, A_PCICFG_PM_CSR, 3);
> +	pci_write_config_dword(pdev, A_PCICFG_PM_CSR, 0);
> +}

there should be standard functions for D3->D0 transition, IIRC



> +static void __devexit remove_one(struct pci_dev *pdev)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +
> +	if (dev) {
> +		int i;
> +		struct adapter *adapter = dev->priv;
> +
> +		for_each_port(adapter, i)
> +			if (test_bit(i, &adapter->registered_device_map))
> +				unregister_netdev(adapter->port[i].dev);
> +
> +		t1_free_sw_modules(adapter);
> +		iounmap(adapter->regs);
> +		while (--i >= 0)
> +			if (adapter->port[i].dev)
> +				free_netdev(adapter->port[i].dev);
> +		pci_release_regions(pdev);
> +		pci_disable_device(pdev);
> +		pci_set_drvdata(pdev, NULL);
> +		t1_sw_reset(pdev);
> +	}
> +}
> +
> +static struct pci_driver driver = {
> +	.name     = driver_name,
> +	.id_table = t1_pci_tbl,
> +	.probe    = init_one,
> +	.remove   = __devexit_p(remove_one),
> +};
> +
> +static int __init t1_init_module(void)
> +{
> +	return pci_module_init(&driver);
> +}
> +
> +static void __exit t1_cleanup_module(void)
> +{
> +	pci_unregister_driver(&driver);
> +}
> +
> +module_init(t1_init_module);
> +module_exit(t1_cleanup_module);
> +
> diff -puN /dev/null drivers/net/chelsio/cxgb2.h
> --- /dev/null	2004-08-10 19:55:00.000000000 -0600
> +++ 25-akpm/drivers/net/chelsio/cxgb2.h	2005-03-24 21:17:43.000000000 -0700

> +/* This belongs in if_ether.h */
> +#define ETH_P_CPL5 0xf

What is CPL5?

Might as well patch if_ether.h.


> +struct cmac;
> +struct cphy;
> +
> +struct port_info {
> +	struct net_device *dev;
> +	struct cmac *mac;
> +	struct cphy *phy;
> +	struct link_config link_config;
> +	struct net_device_stats netstats;
> +};
> +
> +struct cxgbdev;
> +struct t1_sge;
> +struct pemc3;
> +struct pemc4;
> +struct pemc5;
> +struct peulp;
> +struct petp;
> +struct pecspi;
> +struct peespi;
> +struct work_struct;
> +struct vlan_group;
> +
> +enum {					/* adapter flags */
> +	FULL_INIT_DONE        = 0x1,
> +	USING_MSI             = 0x2,
> +	TSO_CAPABLE           = 0x4,
> +	TCP_CSUM_CAPABLE      = 0x8,
> +	UDP_CSUM_CAPABLE      = 0x10,
> +	VLAN_ACCEL_CAPABLE    = 0x20,
> +	RX_CSUM_ENABLED       = 0x40,
> +};

use bit style '1 << n' ?


> +struct adapter {
> +	u8 *regs;
> +	struct pci_dev *pdev;
> +	unsigned long registered_device_map;
> +	unsigned long open_device_map;
> +	unsigned int flags;

unsigned long is generally better for bitflag variables


> +	const char *name;
> +	int msg_enable;
> +	u32 mmio_len;
> +
> +	struct work_struct ext_intr_handler_task;
> +	struct adapter_params params;
> +
> +	struct vlan_group *vlan_grp;
> +
> +	/* Terminator modules. */
> +	struct sge    *sge;
> +	struct pemc3  *mc3;
> +	struct pemc4  *mc4;
> +	struct pemc5  *mc5;
> +	struct petp   *tp;
> +	struct pecspi *cspi;
> +	struct peespi *espi;
> +	struct peulp  *ulp;
> +
> +	struct port_info port[MAX_NPORTS];
> +	struct work_struct stats_update_task;
> +	struct timer_list stats_update_timer;
> +
> +	struct semaphore mib_mutex;
> +	spinlock_t tpi_lock;
> +	spinlock_t work_lock;
> +
> +	spinlock_t async_lock ____cacheline_aligned; /* guards async operations */

can you explain this?  You really shouldn't mess with attributes on 
spinlocks.  That's highly arch-specific.


I gave up reviewing at this point.  That's plenty of changes to tackle, 
particularly the removal of all the t1_write_reg_4() wrappers.

	Jeff