* Re: A new 10GB Ethernet Driver by Chelsio Communications [not found] <Pine.LNX.4.58.0503110356340.14213@server.graphe.net> @ 2005-03-11 19:21 ` Andrew Morton 2005-03-11 19:51 ` Christoph Lameter ` (5 more replies) 0 siblings, 6 replies; 8+ messages in thread From: Andrew Morton @ 2005-03-11 19:21 UTC (permalink / raw) To: Christoph Lameter; +Cc: linux-kernel, mark, netdev, Jeff Garzik 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton @ 2005-03-11 19:51 ` Christoph Lameter 2005-03-11 22:24 ` Adrian Bunk ` (4 subsequent siblings) 5 siblings, 0 replies; 8+ messages in thread From: Christoph Lameter @ 2005-03-11 19:51 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, mark, netdev, Jeff Garzik 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton 2005-03-11 19:51 ` Christoph Lameter @ 2005-03-11 22:24 ` Adrian Bunk 2005-03-11 22:35 ` Stephen Hemminger ` (3 subsequent siblings) 5 siblings, 0 replies; 8+ messages in thread From: Adrian Bunk @ 2005-03-11 22:24 UTC (permalink / raw) To: Andrew Morton; +Cc: Christoph Lameter, linux-kernel, mark, netdev, Jeff Garzik 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton 2005-03-11 19:51 ` Christoph Lameter 2005-03-11 22:24 ` Adrian Bunk @ 2005-03-11 22:35 ` Stephen Hemminger 2005-03-11 22:58 ` Adrian Bunk ` (2 subsequent siblings) 5 siblings, 0 replies; 8+ messages in thread From: Stephen Hemminger @ 2005-03-11 22:35 UTC (permalink / raw) To: Andrew Morton; +Cc: Christoph Lameter, linux-kernel, mark, netdev, Jeff Garzik 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) { + } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton ` (2 preceding siblings ...) 2005-03-11 22:35 ` Stephen Hemminger @ 2005-03-11 22:58 ` Adrian Bunk 2005-03-12 4:49 ` Jeff Garzik 2005-03-14 11:10 ` Pekka Enberg 5 siblings, 0 replies; 8+ messages in thread From: Adrian Bunk @ 2005-03-11 22:58 UTC (permalink / raw) To: Andrew Morton; +Cc: Christoph Lameter, linux-kernel, mark, netdev, Jeff Garzik 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton ` (3 preceding siblings ...) 2005-03-11 22:58 ` Adrian Bunk @ 2005-03-12 4:49 ` Jeff Garzik 2005-03-14 11:10 ` Pekka Enberg 5 siblings, 0 replies; 8+ messages in thread From: Jeff Garzik @ 2005-03-12 4:49 UTC (permalink / raw) To: Andrew Morton, Christoph Lameter; +Cc: linux-kernel, mark, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton ` (4 preceding siblings ...) 2005-03-12 4:49 ` Jeff Garzik @ 2005-03-14 11:10 ` Pekka Enberg 2005-03-14 11:40 ` Pekka Enberg 5 siblings, 1 reply; 8+ messages in thread From: Pekka Enberg @ 2005-03-14 11:10 UTC (permalink / raw) To: Andrew Morton Cc: Christoph Lameter, linux-kernel, mark, netdev, Jeff Garzik, penberg 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: A new 10GB Ethernet Driver by Chelsio Communications 2005-03-14 11:10 ` Pekka Enberg @ 2005-03-14 11:40 ` Pekka Enberg 0 siblings, 0 replies; 8+ messages in thread From: Pekka Enberg @ 2005-03-14 11:40 UTC (permalink / raw) To: Andrew Morton Cc: Christoph Lameter, linux-kernel, mark, netdev, Jeff Garzik, penberg 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2005-03-14 11:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <Pine.LNX.4.58.0503110356340.14213@server.graphe.net>
2005-03-11 19:21 ` A new 10GB Ethernet Driver by Chelsio Communications Andrew Morton
2005-03-11 19:51 ` Christoph Lameter
2005-03-11 22:24 ` Adrian Bunk
2005-03-11 22:35 ` Stephen Hemminger
2005-03-11 22:58 ` Adrian Bunk
2005-03-12 4:49 ` Jeff Garzik
2005-03-14 11:10 ` Pekka Enberg
2005-03-14 11:40 ` Pekka Enberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox