Christoph Lameter <[email protected]> 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?
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.
On Fri, Mar 11, 2005 at 11:21:32AM -0800, Andrew Morton wrote:
> Christoph Lameter <[email protected]> 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
On Fri, 11 Mar 2005 11:21:32 -0800
Andrew Morton <[email protected]> wrote:
> Christoph Lameter <[email protected]> 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) {
+ }
On Fri, Mar 11, 2005 at 11:21:32AM -0800, Andrew Morton wrote:
> Christoph Lameter <[email protected]> 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
Andrew Morton wrote:
> Christoph Lameter <[email protected]> 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
Some of my usual coding style comments...
On Fri, 11 Mar 2005 11:21:32 -0800, Andrew Morton <[email protected]> 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
Hi,
Few more coding style comments.
On Fri, 11 Mar 2005 11:21:32 -0800, Andrew Morton <[email protected]> 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