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