LinuxLists.cc - [PATCH][ATM] assorted he driver cleanup

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

Em Thu, May 29, 2003 at 12:09:54PM -0400, chas williams escreveu:
> the three following changesets attempt to bring the he atm
> driver into conformance with the accepted linux coding style,
> fix some chattiness the irq handler, and address the stack
> usage issue in he_init_cs_block_rcm().

great, comments below

>
> # This is a BitKeeper generated patch for the following project:
> # Project Name: Linux kernel tree
> # This patch format is intended for GNU patch command version 2.5 or higher.
> # This patch includes the following deltas:
> # ChangeSet 1.1158 -> 1.1159
> # drivers/atm/he.c 1.7 -> 1.8
> #
> # The following is the BitKeeper ChangeSet Log
> # --------------------------------------------
> # 03/05/23 [email protected] 1.1159
> # he coding style conformance
> # --------------------------------------------
> #
> diff -Nru a/drivers/atm/he.c b/drivers/atm/he.c
> --- a/drivers/atm/he.c Thu May 29 11:48:40 2003
> +++ b/drivers/atm/he.c Thu May 29 11:48:40 2003
> @@ -132,9 +132,9 @@
>
> #undef DEBUG
> #ifdef DEBUG
> -#define HPRINTK(fmt,args...) hprintk(fmt,args)
> +#define HPRINTK(fmt,args...) printk(KERN_DEBUG DEV_LABEL "%d: " fmt, he_dev->number , ##args)
> #else
> -#define HPRINTK(fmt,args...) do { } while(0)
> +#define HPRINTK(fmt,args...) do { } while (0)
> #endif /* DEBUG */
>
>
> @@ -179,9 +179,7 @@
> phy_put: he_phy_put,
> phy_get: he_phy_get,
> proc_read: he_proc_read,
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,1)
> owner: THIS_MODULE
> -#endif
> };
>
> /* see the comments in he.h about global_lock */
> @@ -189,7 +187,7 @@
> #define HE_SPIN_LOCK(dev, flags) spin_lock_irqsave(&(dev)->global_lock, flags)
> #define HE_SPIN_UNLOCK(dev, flags) spin_unlock_irqrestore(&(dev)->global_lock, flags)

Is the above really needed?

> -#define he_writel(dev, val, reg) do { writel(val, (dev)->membase + (reg)); wmb(); } while(0)
> +#define he_writel(dev, val, reg) do { writel(val, (dev)->membase + (reg)); wmb(); } while (0)
> #define he_readl(dev, reg) readl((dev)->membase + (reg))
>
> /* section 2.12 connection memory access */
> @@ -203,7 +201,7 @@
> (void) he_readl(he_dev, CON_DAT);
> #endif
> he_writel(he_dev, flags | CON_CTL_WRITE | CON_CTL_ADDR(addr), CON_CTL);
> - while(he_readl(he_dev, CON_CTL) & CON_CTL_BUSY);
> + while (he_readl(he_dev, CON_CTL) & CON_CTL_BUSY);
> }
>
> #define he_writel_rcm(dev, val, reg) \
> @@ -219,7 +217,7 @@
> he_readl_internal(struct he_dev *he_dev, unsigned addr, unsigned flags)
> {
> he_writel(he_dev, flags | CON_CTL_READ | CON_CTL_ADDR(addr), CON_CTL);
> - while(he_readl(he_dev, CON_CTL) & CON_CTL_BUSY);
> + while (he_readl(he_dev, CON_CTL) & CON_CTL_BUSY);
> return he_readl(he_dev, CON_DAT);
> }
>
> @@ -374,7 +372,8 @@
>
> printk(KERN_INFO "he: %s\n", version);
>
> - if (pci_enable_device(pci_dev)) return -EIO;
> + if (pci_enable_device(pci_dev))
> + return -EIO;
> if (pci_set_dma_mask(pci_dev, HE_DMA_MASK) != 0) {
> printk(KERN_WARNING "he: no suitable dma available\n");
> err = -EIO;
> @@ -407,7 +406,8 @@
> goto init_one_failure;
> }
> he_dev->next = NULL;
> - if (he_devs) he_dev->next = he_devs;
> + if (he_devs)
> + he_dev->next = he_devs;
> he_devs = he_dev;
> return 0;
>
> @@ -447,11 +447,11 @@
>
> unsigned exp = 0;
>
> - if (rate == 0) return(0);
> + if (rate == 0)
> + return(0);

return is not a function, so 'return 0;' is the preferred style

>
> rate <<= 9;
> - while (rate > 0x3ff)
> - {
> + while (rate > 0x3ff) {
> ++exp;
> rate >>= 1;
> }
> @@ -472,16 +472,14 @@
>
> he_writel(he_dev, lbufd_index, RLBF0_H);
>
> - for (i = 0, lbuf_count = 0; i < he_dev->r0_numbuffs; ++i)
> - {
> + for (i = 0, lbuf_count = 0; i < he_dev->r0_numbuffs; ++i) {
> lbufd_index += 2;
> lbuf_addr = (row_offset + (lbuf_count * lbuf_bufsize)) / 32;
>
> he_writel_rcm(he_dev, lbuf_addr, lbm_offset);
> he_writel_rcm(he_dev, lbufd_index, lbm_offset + 1);
>
> - if (++lbuf_count == lbufs_per_row)
> - {
> + if (++lbuf_count == lbufs_per_row) {
> lbuf_count = 0;
> row_offset += he_dev->bytes_per_row;
> }
> @@ -505,16 +503,14 @@
>
> he_writel(he_dev, lbufd_index, RLBF1_H);
>
> - for (i = 0, lbuf_count = 0; i < he_dev->r1_numbuffs; ++i)
> - {
> + for (i = 0, lbuf_count = 0; i < he_dev->r1_numbuffs; ++i) {
> lbufd_index += 2;
> lbuf_addr = (row_offset + (lbuf_count * lbuf_bufsize)) / 32;
>
> he_writel_rcm(he_dev, lbuf_addr, lbm_offset);
> he_writel_rcm(he_dev, lbufd_index, lbm_offset + 1);
>
> - if (++lbuf_count == lbufs_per_row)
> - {
> + if (++lbuf_count == lbufs_per_row) {
> lbuf_count = 0;
> row_offset += he_dev->bytes_per_row;
> }
> @@ -538,16 +534,14 @@
>
> he_writel(he_dev, lbufd_index, TLBF_H);
>
> - for (i = 0, lbuf_count = 0; i < he_dev->tx_numbuffs; ++i)
> - {
> + for (i = 0, lbuf_count = 0; i < he_dev->tx_numbuffs; ++i) {
> lbufd_index += 1;
> lbuf_addr = (row_offset + (lbuf_count * lbuf_bufsize)) / 32;
>
> he_writel_rcm(he_dev, lbuf_addr, lbm_offset);
> he_writel_rcm(he_dev, lbufd_index, lbm_offset + 1);
>
> - if (++lbuf_count == lbufs_per_row)
> - {
> + if (++lbuf_count == lbufs_per_row) {
> lbuf_count = 0;
> row_offset += he_dev->bytes_per_row;
> }
> @@ -562,8 +556,7 @@
> {
> he_dev->tpdrq_base = pci_alloc_consistent(he_dev->pci_dev,
> CONFIG_TPDRQ_SIZE * sizeof(struct he_tpdrq), &he_dev->tpdrq_phys);
> - if (he_dev->tpdrq_base == NULL)
> - {
> + if (he_dev->tpdrq_base == NULL) {
> hprintk("failed to alloc tpdrq\n");
> return -ENOMEM;
> }
> @@ -588,7 +581,7 @@
>
> /* 5.1.7 cs block initialization */
>
> - for(reg = 0; reg < 0x20; ++reg)
> + for (reg = 0; reg < 0x20; ++reg)
> he_writel_mbox(he_dev, 0x0, CS_STTIM0 + reg);
>
> /* rate grid timer reload values */
> @@ -597,8 +590,7 @@
> rate = he_dev->atm_dev->link_rate;
> delta = rate / 16 / 2;
>
> - for(reg = 0; reg < 0x10; ++reg)
> - {
> + for (reg = 0; reg < 0x10; ++reg) {
> /* 2.4 internal transmit function
> *
> * we initialize the first row in the rate grid.
> @@ -610,8 +602,7 @@
> rate -= delta;
> }
>
> - if (he_is622(he_dev))
> - {
> + if (he_is622(he_dev)) {
> /* table 5.2 (4 cells per lbuf) */
> he_writel_mbox(he_dev, 0x000800fa, CS_ERTHR0);
> he_writel_mbox(he_dev, 0x000c33cb, CS_ERTHR1);
> @@ -640,9 +631,7 @@
> /* table 5.9 */
> he_writel_mbox(he_dev, 0x5, CS_OTPPER);
> he_writel_mbox(he_dev, 0x14, CS_OTWPER);
> - }
> - else
> - {
> + } else {
> /* table 5.1 (4 cells per lbuf) */
> he_writel_mbox(he_dev, 0x000400ea, CS_ERTHR0);
> he_writel_mbox(he_dev, 0x00063388, CS_ERTHR1);
> @@ -671,12 +660,11 @@
> /* table 5.9 */
> he_writel_mbox(he_dev, 0x6, CS_OTPPER);
> he_writel_mbox(he_dev, 0x1e, CS_OTWPER);
> -
> }
>
> he_writel_mbox(he_dev, 0x8, CS_OTTLIM);
>
> - for(reg = 0; reg < 0x8; ++reg)
> + for (reg = 0; reg < 0x8; ++reg)
> he_writel_mbox(he_dev, 0x0, CS_HGRRT0 + reg);
>
> }
> @@ -720,8 +708,7 @@
> * in order to construct the rate to group table below
> */
>
> - for (j = 0; j < 16; j++)
> - {
> + for (j = 0; j < 16; j++) {
> rategrid[0][j] = rate;
> rate -= delta;
> }
> @@ -742,8 +729,7 @@
> */
>
> rate_atmf = 0;
> - while (rate_atmf < 0x400)
> - {
> + while (rate_atmf < 0x400) {
> man = (rate_atmf & 0x1f) << 4;
> exp = rate_atmf >> 5;
>
> @@ -753,12 +739,12 @@
> */
> rate_cps = (unsigned long long) (1 << exp) * (man + 512) >> 9;
>
> - if (rate_cps < 10) rate_cps = 10;
> - /* 2.2.1 minimum payload rate is 10 cps */
> + if (rate_cps < 10)
> + rate_cps = 10; /* 2.2.1 minimum payload rate is 10 cps */
>
> for (i = 255; i > 0; i--)
> - if (rategrid[i/16][i%16] >= rate_cps) break;
> - /* pick nearest rate instead? */
> + if (rategrid[i/16][i%16] >= rate_cps)
> + break; /* pick nearest rate instead? */
>
> /*
> * each table entry is 16 bits: (rate grid index (8 bits)
> @@ -773,12 +759,17 @@
> /* this is pretty, but avoids _divdu3 and is mostly correct */
> buf = 0;
> mult = he_dev->atm_dev->link_rate / ATM_OC3_PCR;
> - if (rate_cps > (68 * mult)) buf = 1;
> - if (rate_cps > (136 * mult)) buf = 2;
> - if (rate_cps > (204 * mult)) buf = 3;
> - if (rate_cps > (272 * mult)) buf = 4;
> + if (rate_cps > (68 * mult))
> + buf = 1;
> + if (rate_cps > (136 * mult))
> + buf = 2;
> + if (rate_cps > (204 * mult))
> + buf = 3;
> + if (rate_cps > (272 * mult))
> + buf = 4;
> #endif
> - if (buf > buf_limit) buf = buf_limit;
> + if (buf > buf_limit)
> + buf = buf_limit;
> reg = (reg<<16) | ((i<<8) | buf);
>
> #define RTGTBL_OFFSET 0x400
> @@ -801,8 +792,7 @@
> #ifdef USE_RBPS_POOL
> he_dev->rbps_pool = pci_pool_create("rbps", he_dev->pci_dev,
> CONFIG_RBPS_BUFSIZE, 8, 0);
> - if (he_dev->rbps_pool == NULL)
> - {
> + if (he_dev->rbps_pool == NULL) {
> hprintk("unable to create rbps pages\n");
> return -ENOMEM;
> }
> @@ -817,16 +807,14 @@
>
> he_dev->rbps_base = pci_alloc_consistent(he_dev->pci_dev,
> CONFIG_RBPS_SIZE * sizeof(struct he_rbp), &he_dev->rbps_phys);
> - if (he_dev->rbps_base == NULL)
> - {
> + if (he_dev->rbps_base == NULL) {
> hprintk("failed to alloc rbps\n");
> return -ENOMEM;
> }
> memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp));
> he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL);
>
> - for (i = 0; i < CONFIG_RBPS_SIZE; ++i)
> - {
> + for (i = 0; i < CONFIG_RBPS_SIZE; ++i) {
> dma_addr_t dma_handle;
> void *cpuaddr;
>
> @@ -868,16 +856,14 @@
> #ifdef USE_RBPL_POOL
> he_dev->rbpl_pool = pci_pool_create("rbpl", he_dev->pci_dev,
> CONFIG_RBPL_BUFSIZE, 8, 0);
> - if (he_dev->rbpl_pool == NULL)
> - {
> + if (he_dev->rbpl_pool == NULL) {
> hprintk("unable to create rbpl pool\n");
> return -ENOMEM;
> }
> #else /* !USE_RBPL_POOL */
> he_dev->rbpl_pages = (void *) pci_alloc_consistent(he_dev->pci_dev,
> CONFIG_RBPL_SIZE * CONFIG_RBPL_BUFSIZE, &he_dev->rbpl_pages_phys);
> - if (he_dev->rbpl_pages == NULL)
> - {
> + if (he_dev->rbpl_pages == NULL) {
> hprintk("unable to create rbpl pages\n");
> return -ENOMEM;
> }
> @@ -885,16 +871,14 @@
>
> he_dev->rbpl_base = pci_alloc_consistent(he_dev->pci_dev,
> CONFIG_RBPL_SIZE * sizeof(struct he_rbp), &he_dev->rbpl_phys);
> - if (he_dev->rbpl_base == NULL)
> - {
> + if (he_dev->rbpl_base == NULL) {
> hprintk("failed to alloc rbpl\n");
> return -ENOMEM;
> }
> memset(he_dev->rbpl_base, 0, CONFIG_RBPL_SIZE * sizeof(struct he_rbp));
> he_dev->rbpl_virt = kmalloc(CONFIG_RBPL_SIZE * sizeof(struct he_virt), GFP_KERNEL);
>
> - for (i = 0; i < CONFIG_RBPL_SIZE; ++i)
> - {
> + for (i = 0; i < CONFIG_RBPL_SIZE; ++i) {
> dma_addr_t dma_handle;
> void *cpuaddr;
>
> @@ -910,7 +894,6 @@
> he_dev->rbpl_virt[i].virt = cpuaddr;
> he_dev->rbpl_base[i].status = RBP_LOANED | (i << RBP_INDEX_OFF);
> he_dev->rbpl_base[i].phys = dma_handle;
> -
> }
> he_dev->rbpl_tail = &he_dev->rbpl_base[CONFIG_RBPL_SIZE-1];
>
> @@ -929,8 +912,7 @@
>
> he_dev->rbrq_base = pci_alloc_consistent(he_dev->pci_dev,
> CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq), &he_dev->rbrq_phys);
> - if (he_dev->rbrq_base == NULL)
> - {
> + if (he_dev->rbrq_base == NULL) {
> hprintk("failed to allocate rbrq\n");
> return -ENOMEM;
> }
> @@ -942,13 +924,11 @@
> he_writel(he_dev,
> RBRQ_THRESH(CONFIG_RBRQ_THRESH) | RBRQ_SIZE(CONFIG_RBRQ_SIZE-1),
> G0_RBRQ_Q + (group * 16));
> - if (irq_coalesce)
> - {
> + if (irq_coalesce) {
> hprintk("coalescing interrupts\n");
> he_writel(he_dev, RBRQ_TIME(768) | RBRQ_COUNT(7),
> G0_RBRQ_I + (group * 16));
> - }
> - else
> + } else
> he_writel(he_dev, RBRQ_TIME(0) | RBRQ_COUNT(1),
> G0_RBRQ_I + (group * 16));
>
> @@ -956,8 +936,7 @@
>
> he_dev->tbrq_base = pci_alloc_consistent(he_dev->pci_dev,
> CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq), &he_dev->tbrq_phys);
> - if (he_dev->tbrq_base == NULL)
> - {
> + if (he_dev->tbrq_base == NULL) {
> hprintk("failed to allocate tbrq\n");
> return -ENOMEM;
> }
> @@ -983,8 +962,7 @@
>
> he_dev->irq_base = pci_alloc_consistent(he_dev->pci_dev,
> (CONFIG_IRQ_SIZE+1) * sizeof(struct he_irq), &he_dev->irq_phys);
> - if (he_dev->irq_base == NULL)
> - {
> + if (he_dev->irq_base == NULL) {
> hprintk("failed to allocate irq\n");
> return -ENOMEM;
> }
> @@ -994,7 +972,7 @@
> he_dev->irq_head = he_dev->irq_base;
> he_dev->irq_tail = he_dev->irq_base;
>
> - for(i=0; i < CONFIG_IRQ_SIZE; ++i)
> + for (i=0; i < CONFIG_IRQ_SIZE; ++i)

use spaces before and after basic operators

for (i = 0; i < CONFIG_IRQ_SIZE; ++i)

> he_dev->irq_base[i].isw = ITYPE_INVALID;
>
> he_writel(he_dev, he_dev->irq_phys, IRQ0_BASE);
> @@ -1026,8 +1004,7 @@
> he_writel(he_dev, 0x0, GRP_54_MAP);
> he_writel(he_dev, 0x0, GRP_76_MAP);
>
> - if (request_irq(he_dev->pci_dev->irq, he_irq_handler, SA_INTERRUPT|SA_SHIRQ, DEV_LABEL, he_dev))
> - {
> + if (request_irq(he_dev->pci_dev->irq, he_irq_handler, SA_INTERRUPT|SA_SHIRQ, DEV_LABEL, he_dev)) {
> hprintk("irq %d already in use\n", he_dev->pci_dev->irq);
> return -EINVAL;
> }
> @@ -1067,46 +1044,39 @@
> */
>
> /* 4.3 pci bus controller-specific initialization */
> - if (pci_read_config_dword(pci_dev, GEN_CNTL_0, &gen_cntl_0) != 0)
> - {
> + if (pci_read_config_dword(pci_dev, GEN_CNTL_0, &gen_cntl_0) != 0) {
> hprintk("can't read GEN_CNTL_0\n");

Humm, shouldn't we release the irq here? What about using gotos for exception
handing?

> return -EINVAL;
> }
> gen_cntl_0 |= (MRL_ENB | MRM_ENB | IGNORE_TIMEOUT);
> - if (pci_write_config_dword(pci_dev, GEN_CNTL_0, gen_cntl_0) != 0)
> - {
> + if (pci_write_config_dword(pci_dev, GEN_CNTL_0, gen_cntl_0) != 0) {
> hprintk("can't write GEN_CNTL_0.\n");

ditto

> return -EINVAL;
> }
>
> - if (pci_read_config_word(pci_dev, PCI_COMMAND, &command) != 0)
> - {
> + if (pci_read_config_word(pci_dev, PCI_COMMAND, &command) != 0) {
> hprintk("can't read PCI_COMMAND.\n");

ditto

> return -EINVAL;
> }
>
> command |= (PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE);
> - if (pci_write_config_word(pci_dev, PCI_COMMAND, command) != 0)
> - {
> + if (pci_write_config_word(pci_dev, PCI_COMMAND, command) != 0) {
> hprintk("can't enable memory.\n");

ditto

> return -EINVAL;
> }
>
> - if (pci_read_config_byte(pci_dev, PCI_CACHE_LINE_SIZE, &cache_size))
> - {
> + if (pci_read_config_byte(pci_dev, PCI_CACHE_LINE_SIZE, &cache_size)) {
> hprintk("can't read cache line size?\n");

ditto

> return -EINVAL;
> }
>
> - if (cache_size < 16)
> - {
> + if (cache_size < 16) {
> cache_size = 16;
> if (pci_write_config_byte(pci_dev, PCI_CACHE_LINE_SIZE, cache_size))
> hprintk("can't set cache line size to %d\n", cache_size);
> }
>
> - if (pci_read_config_byte(pci_dev, PCI_LATENCY_TIMER, &timer))
> - {
> + if (pci_read_config_byte(pci_dev, PCI_LATENCY_TIMER, &timer)) {
> hprintk("can't read latency timer?\n");

ditto

I'll stop here :-)

2003-05-29 16:19:57

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Arnaldo Carvalho de Melo w
rites:
>> @@ -189,7 +187,7 @@
>> #define HE_SPIN_LOCK(dev, flags) spin_lock_irqsave(&(dev)->global_lock,
>flags)
>> #define HE_SPIN_UNLOCK(dev, flags) spin_unlock_irqrestore(&(dev)->global_l
>ock, flags)
>
>Is the above really needed?

well, according to the programmer's guide:

8.1.7 PCI Transaction Ordering Error

PROBLEM: The PCI Bus Controller, in addition to bus master and general
target functionality, acts as a bridge to a local bus. If a read is
issued to the local bus and the read COMPLETES on the local bus but is
not yet completed on the PCI bus, a subsequent write to the local bus
that completes on the PCI bus will cause the write data to be written
to the last local bus read address.

RESOLUTION: In an environment where PCI bus bridge settings cannot be
controlled, card drivers must lock read and write access to the ATM
Network Controller and the SONET Framer device. In an environment where
PCI bus bridge settings can be controlled, features that allow reordering
of reads and writes from separate processors (i.e., Posted memory write
features) should be disabled.

2003-05-29 16:20:58

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Christoph Hellwig writes:
>btw, could you also remove the BUS_INT_WAR hacks? There shouldn't
>be anz SHUB1.0 Altix systems around anymore..

we still have a few :) (but they arent in service so i guess i could
remove it).

2003-05-29 16:53:27

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

Em Thu, May 29, 2003 at 12:31:17PM -0400, chas williams escreveu:
> In message <[email protected]>,Arnaldo Carvalho de Melo w
> rites:
> >> @@ -189,7 +187,7 @@
> >> #define HE_SPIN_LOCK(dev, flags) spin_lock_irqsave(&(dev)->global_lock,
> >flags)
> >> #define HE_SPIN_UNLOCK(dev, flags) spin_unlock_irqrestore(&(dev)->global_l
> >ock, flags)
> >
> >Is the above really needed?
>
> well, according to the programmer's guide:

no, no, I was talking just about the need for HE_SPIN_LOCK wrapper, not the
locking, i.e. couldn't it be just:

spin_lock_irqsave(&dev->global_lock, flags)

used so that it is clear that it is a irqsave variation, etc?

- Arnaldo

2003-05-29 17:13:11

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Arnaldo Carvalho de Melo w
rites:
>no, no, I was talking just about the need for HE_SPIN_LOCK wrapper, not the
>locking, i.e. couldn't it be just:
>
>spin_lock_irqsave(&dev->global_lock, flags)
>
>used so that it is clear that it is a irqsave variation, etc?

i suppose i could change it all back. at one point, he_spin_lock()
was 'optmized' away on non smp machines (since a single cpu doesnt
tickle the particular h/w problem).

i didnt want a ton of #ifdef CONFIG_SMP in the driver.

2003-05-29 17:23:00

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

Em Thu, May 29, 2003 at 01:12:13PM -0400, chas williams escreveu:
> In message <[email protected]>,Arnaldo Carvalho de Melo w
> rites:
> >no, no, I was talking just about the need for HE_SPIN_LOCK wrapper, not the
> >locking, i.e. couldn't it be just:
> >
> >spin_lock_irqsave(&dev->global_lock, flags)
> >
> >used so that it is clear that it is a irqsave variation, etc?
>
> i suppose i could change it all back. at one point, he_spin_lock()
> was 'optmized' away on non smp machines (since a single cpu doesnt
> tickle the particular h/w problem).
>
> i didnt want a ton of #ifdef CONFIG_SMP in the driver.

Sure thing, but hey, spin_lock_irqsave and friends take care of how to behave
when in up or smp, i.e. its how all the other drivers use spinlocks 8)

- Arnaldo

2003-05-29 18:09:56

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Arnaldo Carvalho de Melo w
rites:
>> @@ -1067,46 +1044,39 @@
>> */
>>
>> /* 4.3 pci bus controller-specific initialization */
>> - if (pci_read_config_dword(pci_dev, GEN_CNTL_0, &gen_cntl_0) != 0)
>> - {
>> + if (pci_read_config_dword(pci_dev, GEN_CNTL_0, &gen_cntl_0) != 0) {
>> hprintk("can't read GEN_CNTL_0\n");
>
>Humm, shouldn't we release the irq here? What about using gotos for exception
>handing?

what irq? the irq is grabbed in he_init_irq. if he_start() fails, we
call he_stop() and it frees the irq.

2003-05-29 18:20:46

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

Em Thu, May 29, 2003 at 02:08:59PM -0400, chas williams escreveu:
> In message <[email protected]>,Arnaldo Carvalho de Melo w
> rites:
> >> @@ -1067,46 +1044,39 @@
> >> */
> >>
> >> /* 4.3 pci bus controller-specific initialization */
> >> - if (pci_read_config_dword(pci_dev, GEN_CNTL_0, &gen_cntl_0) != 0)
> >> - {
> >> + if (pci_read_config_dword(pci_dev, GEN_CNTL_0, &gen_cntl_0) != 0) {
> >> hprintk("can't read GEN_CNTL_0\n");
> >
> >Humm, shouldn't we release the irq here? What about using gotos for exception
> >handing?
>
> what irq? the irq is grabbed in he_init_irq. if he_start() fails, we
> call he_stop() and it frees the irq.

OK sir, sorry for the noise on this one :-)

2003-05-30 02:48:59

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

From: chas williams <[email protected]>
Date: Thu, 29 May 2003 12:09:54 -0400

the three following changesets attempt to bring the he atm
driver into conformance with the accepted linux coding style,
fix some chattiness the irq handler, and address the stack
usage issue in he_init_cs_block_rcm().

Applied, thanks.

You can integrate ideas posted in this thread in subsequent
patches.

BTW, can you use more consistent changeset messages? I always
at least allude to what is being changed, for example I changed
all of your messages to be of the form:

[ATM]: Blah blah blah in HE driver.

This tells the reader that:

1) It's an ATM change.

2) It's to the HE driver.

3) The change made was "Blah blah blah" :-)

2003-05-30 08:41:51

by Dave Jones

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

On Thu, May 29, 2003 at 08:01:01PM -0700, David S. Miller wrote:

> BTW, can you use more consistent changeset messages? I always
> at least allude to what is being changed, for example I changed
> all of your messages to be of the form:
>
> [ATM]: Blah blah blah in HE driver.
>
> This tells the reader that:
>
> 1) It's an ATM change.
> 2) It's to the HE driver.
> 3) The change made was "Blah blah blah" :-)

I'd also add...
4) keep the first line a brief oneliner summary, as it
gets truncated when Linus generates the shortlog.

A number of people (self included) have adopted this
impromptu 'standard' for bk comments. Maybe it should
get documented in Documentation/BK-usage/bk-kernel-howto.txt

Dave

2003-05-30 13:23:03

by Jeff Garzik

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

On Fri, May 30, 2003 at 09:57:26AM +0100, Dave Jones wrote:
> A number of people (self included) have adopted this
> impromptu 'standard' for bk comments. Maybe it should
> get documented in Documentation/BK-usage/bk-kernel-howto.txt

Post a patch! ;-)

Jeff

2003-05-30 13:36:41

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,"David S. Miller" write
s:
>You can integrate ideas posted in this thread in subsequent
>patches.

[ATM]: more coding style cleanups in HE driver

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.1161 -> 1.1162
# drivers/atm/he.c 1.10 -> 1.11
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/30 [email protected] 1.1162
# he.c:
# [ATM]: more coding style cleanups in HE driver
# --------------------------------------------
#
diff -Nru a/drivers/atm/he.c b/drivers/atm/he.c
--- a/drivers/atm/he.c Fri May 30 09:10:54 2003
+++ b/drivers/atm/he.c Fri May 30 09:10:54 2003
@@ -77,14 +77,6 @@
#include <linux/atmdev.h>
#include <linux/atm.h>
#include <linux/sonet.h>
-#ifndef ATM_OC12_PCR
-#define ATM_OC12_PCR (622080000/1080*1040/8/53)
-#endif
-
-#ifdef BUS_INT_WAR
-void sn_add_polled_interrupt(int irq, int interval);
-void sn_delete_polled_interrupt(int irq);
-#endif

#define USE_TASKLET
#define USE_HE_FIND_VCC
@@ -171,22 +163,17 @@

static struct atmdev_ops he_ops =
{
- open: he_open,
- close: he_close,
- ioctl: he_ioctl,
- send: he_send,
- sg_send: he_sg_send,
- phy_put: he_phy_put,
- phy_get: he_phy_get,
- proc_read: he_proc_read,
- owner: THIS_MODULE
+ .open = he_open,
+ .close = he_close,
+ .ioctl = he_ioctl,
+ .send = he_send,
+ .sg_send = he_sg_send,
+ .phy_put = he_phy_put,
+ .phy_get = he_phy_get,
+ .proc_read = he_proc_read,
+ .owner = THIS_MODULE
};

-/* see the comments in he.h about global_lock */
-
-#define HE_SPIN_LOCK(dev, flags) spin_lock_irqsave(&(dev)->global_lock, flags)
-#define HE_SPIN_UNLOCK(dev, flags) spin_unlock_irqrestore(&(dev)->global_lock, flags)
-
#define he_writel(dev, val, reg) do { writel(val, (dev)->membase + (reg)); wmb(); } while (0)
#define he_readl(dev, reg) readl((dev)->membase + (reg))

@@ -233,26 +220,26 @@

/* figure 2.2 connection id */

-#define he_mkcid(dev, vpi, vci) (((vpi<<(dev)->vcibits) | vci) & 0x1fff)
+#define he_mkcid(dev, vpi, vci) (((vpi << (dev)->vcibits) | vci) & 0x1fff)

/* 2.5.1 per connection transmit state registers */

#define he_writel_tsr0(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 0)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 0)
#define he_readl_tsr0(dev, cid) \
- he_readl_tcm(dev, CONFIG_TSRA | (cid<<3) | 0)
+ he_readl_tcm(dev, CONFIG_TSRA | (cid << 3) | 0)

#define he_writel_tsr1(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 1)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 1)

#define he_writel_tsr2(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 2)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 2)

#define he_writel_tsr3(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 3)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 3)

#define he_writel_tsr4(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 4)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 4)

/* from page 2-20
*
@@ -263,43 +250,43 @@
*/

#define he_writel_tsr4_upper(dev, val, cid) \
- he_writel_internal(dev, val, CONFIG_TSRA | (cid<<3) | 4, \
+ he_writel_internal(dev, val, CONFIG_TSRA | (cid << 3) | 4, \
CON_CTL_TCM \
| CON_BYTE_DISABLE_2 \
| CON_BYTE_DISABLE_1 \
| CON_BYTE_DISABLE_0)

#define he_readl_tsr4(dev, cid) \
- he_readl_tcm(dev, CONFIG_TSRA | (cid<<3) | 4)
+ he_readl_tcm(dev, CONFIG_TSRA | (cid << 3) | 4)

#define he_writel_tsr5(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 5)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 5)

#define he_writel_tsr6(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 6)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 6)

#define he_writel_tsr7(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRA | (cid<<3) | 7)
+ he_writel_tcm(dev, val, CONFIG_TSRA | (cid << 3) | 7)

#define he_writel_tsr8(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRB | (cid<<2) | 0)
+ he_writel_tcm(dev, val, CONFIG_TSRB | (cid << 2) | 0)

#define he_writel_tsr9(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRB | (cid<<2) | 1)
+ he_writel_tcm(dev, val, CONFIG_TSRB | (cid << 2) | 1)

#define he_writel_tsr10(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRB | (cid<<2) | 2)
+ he_writel_tcm(dev, val, CONFIG_TSRB | (cid << 2) | 2)

#define he_writel_tsr11(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRB | (cid<<2) | 3)
+ he_writel_tcm(dev, val, CONFIG_TSRB | (cid << 2) | 3)

#define he_writel_tsr12(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRC | (cid<<1) | 0)
+ he_writel_tcm(dev, val, CONFIG_TSRC | (cid << 1) | 0)

#define he_writel_tsr13(dev, val, cid) \
- he_writel_tcm(dev, val, CONFIG_TSRC | (cid<<1) | 1)
+ he_writel_tcm(dev, val, CONFIG_TSRC | (cid << 1) | 1)

#define he_writel_tsr14(dev, val, cid) \
@@ -315,30 +302,30 @@
/* 2.7.1 per connection receive state registers */

#define he_writel_rsr0(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 0)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 0)
#define he_readl_rsr0(dev, cid) \
- he_readl_rcm(dev, 0x00000 | (cid<<3) | 0)
+ he_readl_rcm(dev, 0x00000 | (cid << 3) | 0)

#define he_writel_rsr1(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 1)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 1)

#define he_writel_rsr2(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 2)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 2)

#define he_writel_rsr3(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 3)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 3)

#define he_writel_rsr4(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 4)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 4)

#define he_writel_rsr5(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 5)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 5)

#define he_writel_rsr6(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 6)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 6)

#define he_writel_rsr7(dev, val, cid) \
- he_writel_rcm(dev, val, 0x00000 | (cid<<3) | 7)
+ he_writel_rcm(dev, val, 0x00000 | (cid << 3) | 7)

static __inline__ struct atm_vcc*
he_find_vcc(struct he_dev *he_dev, unsigned cid)
@@ -349,7 +336,7 @@
int vci;

vpi = cid >> he_dev->vcibits;
- vci = cid & ((1<<he_dev->vcibits)-1);
+ vci = cid & ((1 << he_dev->vcibits) - 1);

spin_lock_irqsave(&he_dev->atm_dev->lock, flags);
for (vcc = he_dev->atm_dev->vccs; vcc; vcc = vcc->next)
@@ -443,12 +430,12 @@
static unsigned
rate_to_atmf(unsigned rate) /* cps to atm forum format */
{
-#define NONZERO (1<<14)
+#define NONZERO (1 << 14)

unsigned exp = 0;

if (rate == 0)
- return(0);
+ return 0;

rate <<= 9;
while (rate > 0x3ff) {
@@ -669,10 +656,10 @@

}

-static int __init
+static void __init
he_init_cs_block_rcm(struct he_dev *he_dev)
{
- unsigned (*rategrid)[16][16];
+ unsigned rategrid[16][16];
unsigned rate, delta;
int i, j, reg;

@@ -680,10 +667,6 @@
unsigned long long rate_cps;
int mult, buf, buf_limit = 4;

- rategrid = kmalloc( sizeof(unsigned) * 16 * 16, GFP_KERNEL);
- if (!rategrid)
- return -ENOMEM;
-
/* initialize rate grid group table */

for (reg = 0x0; reg < 0xff; ++reg)
@@ -713,16 +696,16 @@
*/

for (j = 0; j < 16; j++) {
- (*rategrid)[0][j] = rate;
+ rategrid[0][j] = rate;
rate -= delta;
}

for (i = 1; i < 16; i++)
for (j = 0; j < 16; j++)
if (i > 14)
- (*rategrid)[i][j] = (*rategrid)[i - 1][j] / 4;
+ rategrid[i][j] = rategrid[i - 1][j] / 4;
else
- (*rategrid)[i][j] = (*rategrid)[i - 1][j] / 2;
+ rategrid[i][j] = rategrid[i - 1][j] / 2;

/*
* 2.4 transmit internal function
@@ -747,7 +730,7 @@
rate_cps = 10; /* 2.2.1 minimum payload rate is 10 cps */

for (i = 255; i > 0; i--)
- if ((*rategrid)[i/16][i%16] >= rate_cps)
+ if (rategrid[i/16][i%16] >= rate_cps)
break; /* pick nearest rate instead? */

/*
@@ -761,32 +744,30 @@
(he_dev->atm_dev->link_rate * 2);
#else
/* this is pretty, but avoids _divdu3 and is mostly correct */
- buf = 0;
mult = he_dev->atm_dev->link_rate / ATM_OC3_PCR;
- if (rate_cps > (68 * mult))
- buf = 1;
- if (rate_cps > (136 * mult))
- buf = 2;
- if (rate_cps > (204 * mult))
- buf = 3;
if (rate_cps > (272 * mult))
buf = 4;
+ else if (rate_cps > (204 * mult))
+ buf = 3;
+ else if (rate_cps > (136 * mult))
+ buf = 2;
+ else if (rate_cps > (68 * mult))
+ buf = 1;
+ else
+ buf = 0;
#endif
if (buf > buf_limit)
buf = buf_limit;
- reg = (reg<<16) | ((i<<8) | buf);
+ reg = (reg << 16) | ((i << 8) | buf);

#define RTGTBL_OFFSET 0x400

if (rate_atmf & 0x1)
he_writel_rcm(he_dev, reg,
- CONFIG_RCMABR + RTGTBL_OFFSET + (rate_atmf>>1));
+ CONFIG_RCMABR + RTGTBL_OFFSET + (rate_atmf >> 1));

++rate_atmf;
}
-
- kfree(rategrid);
- return 0;
}

static int __init
@@ -839,7 +820,7 @@
he_dev->rbps_base[i].phys = dma_handle;

}
- he_dev->rbps_tail = &he_dev->rbps_base[CONFIG_RBPS_SIZE-1];
+ he_dev->rbps_tail = &he_dev->rbps_base[CONFIG_RBPS_SIZE - 1];

he_writel(he_dev, he_dev->rbps_phys, G0_RBPS_S + (group * 32));
he_writel(he_dev, RBPS_MASK(he_dev->rbps_tail),
@@ -848,7 +829,7 @@
G0_RBPS_BS + (group * 32));
he_writel(he_dev,
RBP_THRESH(CONFIG_RBPS_THRESH) |
- RBP_QSIZE(CONFIG_RBPS_SIZE-1) |
+ RBP_QSIZE(CONFIG_RBPS_SIZE - 1) |
RBP_INT_ENB,
G0_RBPS_QI + (group * 32));
#else /* !USE_RBPS */
@@ -902,7 +883,7 @@
he_dev->rbpl_base[i].status = RBP_LOANED | (i << RBP_INDEX_OFF);
he_dev->rbpl_base[i].phys = dma_handle;
}
- he_dev->rbpl_tail = &he_dev->rbpl_base[CONFIG_RBPL_SIZE-1];
+ he_dev->rbpl_tail = &he_dev->rbpl_base[CONFIG_RBPL_SIZE - 1];

he_writel(he_dev, he_dev->rbpl_phys, G0_RBPL_S + (group * 32));
he_writel(he_dev, RBPL_MASK(he_dev->rbpl_tail),
@@ -911,7 +892,7 @@
G0_RBPL_BS + (group * 32));
he_writel(he_dev,
RBP_THRESH(CONFIG_RBPL_THRESH) |
- RBP_QSIZE(CONFIG_RBPL_SIZE-1) |
+ RBP_QSIZE(CONFIG_RBPL_SIZE - 1) |
RBP_INT_ENB,
G0_RBPL_QI + (group * 32));

@@ -929,7 +910,7 @@
he_writel(he_dev, he_dev->rbrq_phys, G0_RBRQ_ST + (group * 16));
he_writel(he_dev, 0, G0_RBRQ_H + (group * 16));
he_writel(he_dev,
- RBRQ_THRESH(CONFIG_RBRQ_THRESH) | RBRQ_SIZE(CONFIG_RBRQ_SIZE-1),
+ RBRQ_THRESH(CONFIG_RBRQ_THRESH) | RBRQ_SIZE(CONFIG_RBRQ_SIZE - 1),
G0_RBRQ_Q + (group * 16));
if (irq_coalesce) {
hprintk("coalescing interrupts\n");
@@ -979,7 +960,7 @@
he_dev->irq_head = he_dev->irq_base;
he_dev->irq_tail = he_dev->irq_base;

- for (i=0; i < CONFIG_IRQ_SIZE; ++i)
+ for (i = 0; i < CONFIG_IRQ_SIZE; ++i)
he_dev->irq_base[i].isw = ITYPE_INVALID;

he_writel(he_dev, he_dev->irq_phys, IRQ0_BASE);
@@ -1018,11 +999,6 @@

he_dev->irq = he_dev->pci_dev->irq;

-#ifdef BUS_INT_WAR
- HPRINTK("sn_add_polled_interrupt(irq %d, 1)\n", he_dev->irq);
- sn_add_polled_interrupt(he_dev->irq, 1);
-#endif
-
return 0;
}

@@ -1138,12 +1114,12 @@
pci_write_config_dword(pci_dev, GEN_CNTL_0, gen_cntl_0);

/* 4.7 read prom contents */
- for (i=0; i<PROD_ID_LEN; ++i)
+ for (i = 0; i < PROD_ID_LEN; ++i)
he_dev->prod_id[i] = read_prom_byte(he_dev, PROD_ID + i);

he_dev->media = read_prom_byte(he_dev, MEDIA);

- for (i=0; i<6; ++i)
+ for (i = 0; i < 6; ++i)
dev->esi[i] = read_prom_byte(he_dev, MAC_ADDR + i);

hprintk("%s%s, %x:%x:%x:%x:%x:%x\n",
@@ -1323,15 +1299,15 @@
he_writel(he_dev, 0x0, TXAAL5_PROTO);

he_writel(he_dev, PHY_INT_ENB |
- (he_is622(he_dev) ? PTMR_PRE(67-1) : PTMR_PRE(50-1)),
+ (he_is622(he_dev) ? PTMR_PRE(67 - 1) : PTMR_PRE(50 - 1)),
RH_CONFIG);

/* 5.1.3 initialize connection memory */

- for (i=0; i < TCM_MEM_SIZE; ++i)
+ for (i = 0; i < TCM_MEM_SIZE; ++i)
he_writel_tcm(he_dev, 0, i);

- for (i=0; i < RCM_MEM_SIZE; ++i)
+ for (i = 0; i < RCM_MEM_SIZE; ++i)
he_writel_rcm(he_dev, 0, i);

/*
@@ -1484,8 +1460,7 @@

/* 5.1.8 cs block connection memory initialization */

- if (he_init_cs_block_rcm(he_dev) < 0)
- return -ENOMEM;
+ he_init_cs_block_rcm(he_dev);

/* 5.1.10 initialize host structures */

@@ -1512,7 +1487,7 @@
}

he_dev->tpd_head = he_dev->tpd_base;
- he_dev->tpd_end = &he_dev->tpd_base[CONFIG_NUMTPDS-1];
+ he_dev->tpd_end = &he_dev->tpd_base[CONFIG_NUMTPDS - 1];
#endif

if (he_init_group(he_dev, 0) != 0)
@@ -1652,12 +1627,8 @@
he_dev->atm_dev->phy->stop(he_dev->atm_dev);
#endif /* CONFIG_ATM_HE_USE_SUNI */

- if (he_dev->irq) {
-#ifdef BUS_INT_WAR
- sn_delete_polled_interrupt(he_dev->irq);
-#endif
+ if (he_dev->irq)
free_irq(he_dev->irq, he_dev);
- }

if (he_dev->irq_base)
pci_free_consistent(he_dev->pci_dev, (CONFIG_IRQ_SIZE+1)
@@ -1669,7 +1640,7 @@

if (he_dev->rbpl_base) {
#ifdef USE_RBPL_POOL
- for (i=0; i<CONFIG_RBPL_SIZE; ++i) {
+ for (i = 0; i < CONFIG_RBPL_SIZE; ++i) {
void *cpuaddr = he_dev->rbpl_virt[i].virt;
dma_addr_t dma_handle = he_dev->rbpl_base[i].phys;

@@ -1691,7 +1662,7 @@
#ifdef USE_RBPS
if (he_dev->rbps_base) {
#ifdef USE_RBPS_POOL
- for (i=0; i<CONFIG_RBPS_SIZE; ++i) {
+ for (i = 0; i < CONFIG_RBPS_SIZE; ++i) {
void *cpuaddr = he_dev->rbps_virt[i].virt;
dma_addr_t dma_handle = he_dev->rbps_base[i].phys;

@@ -1790,7 +1761,7 @@
}

#define AAL5_LEN(buf,len) \
- ((((unsigned char *)(buf))[(len)-6]<<8) | \
+ ((((unsigned char *)(buf))[(len)-6] << 8) | \
(((unsigned char *)(buf))[(len)-5]))

/* 2.10.1.2 receive
@@ -1800,7 +1771,7 @@
*/

#define TCP_CKSUM(buf,len) \
- ((((unsigned char *)(buf))[(len)-2]<<8) | \
+ ((((unsigned char *)(buf))[(len)-2] << 8) | \
(((unsigned char *)(buf))[(len-1)]))

static int
@@ -2171,7 +2142,7 @@

HPRINTK("tasklet (0x%lx)\n", data);
#ifdef USE_TASKLET
- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
#endif

while (he_dev->irq_head != he_dev->irq_tail) {
@@ -2209,10 +2180,10 @@
case ITYPE_PHY:
HPRINTK("phy interrupt\n");
#ifdef CONFIG_ATM_HE_USE_SUNI
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
if (he_dev->atm_dev->phy && he_dev->atm_dev->phy->interrupt)
he_dev->atm_dev->phy->interrupt(he_dev->atm_dev);
- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
#endif
break;
case ITYPE_OTHER:
@@ -2257,7 +2228,7 @@
(void) he_readl(he_dev, INT_FIFO); /* 8.1.2 controller errata */
}
#ifdef USE_TASKLET
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
#endif
}

@@ -2271,7 +2242,7 @@
if (he_dev == NULL)
return IRQ_NONE;

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);

he_dev->irq_tail = (struct he_irq *) (((unsigned long)he_dev->irq_base) |
(*he_dev->irq_tailoffset << 2));
@@ -2301,7 +2272,7 @@
(void) he_readl(he_dev, INT_FIFO);
#endif
}
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
return IRQ_RETVAL(handled);

}
@@ -2438,9 +2409,9 @@
goto open_failed;
}

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
tsr0 = he_readl_tsr0(he_dev, cid);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

if (TSR0_CONN_STATE(tsr0) != 0) {
hprintk("cid 0x%x not idle (tsr0 = 0x%x)\n", cid, tsr0);
@@ -2467,7 +2438,7 @@
goto open_failed;
}

- HE_SPIN_LOCK(he_dev, flags); /* also protects he_dev->cs_stper[] */
+ spin_lock_irqsave(&he_dev->global_lock, flags); /* also protects he_dev->cs_stper[] */

/* find an unused cs_stper register */
for (reg = 0; reg < HE_NUM_CS_STPER; ++reg)
@@ -2477,7 +2448,7 @@

if (reg == HE_NUM_CS_STPER) {
err = -EBUSY;
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
goto open_failed;
}

@@ -2495,7 +2466,7 @@

he_writel_mbox(he_dev, rate_to_atmf(period/2),
CS_STPER0 + reg);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

tsr0 = TSR0_CBR | TSR0_GROUP(0) | tsr0_aal |
TSR0_RC_INDEX(reg);
@@ -2506,7 +2477,7 @@
goto open_failed;
}

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);

he_writel_tsr0(he_dev, tsr0, cid);
he_writel_tsr4(he_dev, tsr4 | 1, cid);
@@ -2528,7 +2499,7 @@
#ifdef CONFIG_IA64_SGI_SN2
(void) he_readl_tsr0(he_dev, cid);
#endif
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
}

if (vcc->qos.rxtp.traffic_class != ATM_NONE) {
@@ -2549,11 +2520,11 @@
goto open_failed;
}

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);

rsr0 = he_readl_rsr0(he_dev, cid);
if (rsr0 & RSR0_OPEN_CONN) {
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

hprintk("cid 0x%x not idle (rsr0 = 0x%x)\n", cid, rsr0);
err = -EBUSY;
@@ -2585,7 +2556,7 @@
(void) he_readl_rsr0(he_dev, cid);
#endif

- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

#ifndef USE_HE_FIND_VCC
HE_LOOKUP_VCC(he_dev, cid) = vcc;
@@ -2631,7 +2602,7 @@

/* wait for previous close (if any) to finish */

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
while (he_readl(he_dev, RCC_STAT) & RCC_BUSY) {
HPRINTK("close cid 0x%x RCC_BUSY\n", cid);
udelay(250);
@@ -2645,7 +2616,7 @@
(void) he_readl_rsr0(he_dev, cid);
#endif
he_writel_mbox(he_dev, cid, RXCON_CLOSE);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

timeout = schedule_timeout(30*HZ);

@@ -2693,7 +2664,7 @@

/* 2.3.1.1 generic close operations with flush */

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
he_writel_tsr4_upper(he_dev, TSR4_FLUSH_CONN, cid);
/* also clears TSR4_SESSION_ENDED */
#ifdef CONFIG_IA64_SGI_SN2
@@ -2724,7 +2695,7 @@
add_wait_queue(&he_vcc->tx_waitq, &wait);
set_current_state(TASK_UNINTERRUPTIBLE);
__enqueue_tpd(he_dev, tpd, cid);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

timeout = schedule_timeout(30*HZ);

@@ -2736,7 +2707,7 @@
goto close_tx_incomplete;
}

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
while (!((tsr4 = he_readl_tsr4(he_dev, cid)) & TSR4_SESSION_ENDED)) {
HPRINTK("close tx cid 0x%x !TSR4_SESSION_ENDED (tsr4 = 0x%x)\n", cid, tsr4);
udelay(250);
@@ -2761,7 +2732,7 @@

he_dev->total_bw -= he_dev->cs_stper[reg].pcr;
}
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

HPRINTK("close tx cid 0x%x complete\n", cid);
}
@@ -2818,7 +2789,7 @@
return -EINVAL;
}
#endif
- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);

tpd = __alloc_tpd(he_dev);
if (tpd == NULL) {
@@ -2827,7 +2798,7 @@
else
dev_kfree_skb_any(skb);
atomic_inc(&vcc->stats->tx_err);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
return -ENOMEM;
}

@@ -2869,7 +2840,7 @@
else
dev_kfree_skb_any(skb);
atomic_inc(&vcc->stats->tx_err);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
return -ENOMEM;
}
tpd->status |= TPD_USERCELL;
@@ -2884,7 +2855,7 @@

}

- tpd->iovec[slot-1].len |= TPD_LST;
+ tpd->iovec[slot - 1].len |= TPD_LST;
#else
tpd->address0 = pci_map_single(he_dev->pci_dev, skb->data, skb->len, PCI_DMA_TODEVICE);
tpd->length0 = skb->len | TPD_LST;
@@ -2897,7 +2868,7 @@
ATM_SKB(skb)->vcc = vcc;

__enqueue_tpd(he_dev, tpd, cid);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

atomic_inc(&vcc->stats->tx);

@@ -2919,7 +2890,7 @@

copy_from_user(&reg, (struct he_ioctl_reg *) arg,
sizeof(struct he_ioctl_reg));
- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
switch (reg.type) {
case HE_REGTYPE_PCI:
reg.val = he_readl(he_dev, reg.addr);
@@ -2940,7 +2911,7 @@
err = -EINVAL;
break;
}
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
if (err == 0)
copy_to_user((struct he_ioctl_reg *) arg, &reg,
sizeof(struct he_ioctl_reg));
@@ -2966,12 +2937,12 @@

HPRINTK("phy_put(val 0x%x, addr 0x%lx)\n", val, addr);

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
he_writel(he_dev, val, FRAMER + (addr*4));
#ifdef CONFIG_IA64_SGI_SN2
(void) he_readl(he_dev, FRAMER + (addr*4));
#endif
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);
}

@@ -2982,9 +2953,9 @@
struct he_dev *he_dev = HE_DEV(atm_dev);
unsigned reg;

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
reg = he_readl(he_dev, FRAMER + (addr*4));
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

HPRINTK("phy_get(addr 0x%lx) =0x%x\n", addr, reg);
return reg;
@@ -3015,12 +2986,12 @@
if (!left--)
return sprintf(page, "Mismatched Cells VPI/VCI Not Open Dropped Cells RCM Dropped Cells\n");

- HE_SPIN_LOCK(he_dev, flags);
+ spin_lock_irqsave(&he_dev->global_lock, flags);
mcc += he_readl(he_dev, MCC);
oec += he_readl(he_dev, OEC);
dcc += he_readl(he_dev, DCC);
cec += he_readl(he_dev, CEC);
- HE_SPIN_UNLOCK(he_dev, flags);
+ spin_unlock_irqrestore(&he_dev->global_lock, flags);

if (!left--)
return sprintf(page, "%16ld %16ld %13ld %17ld\n\n",
@@ -3090,26 +3061,26 @@
he_writel(he_dev, val, HOST_CNTL);

/* Send READ instruction */
- for (i=0; i<sizeof(readtab)/sizeof(readtab[0]); i++) {
+ for (i = 0; i < sizeof(readtab)/sizeof(readtab[0]); i++) {
he_writel(he_dev, val | readtab[i], HOST_CNTL);
udelay(EEPROM_DELAY);
}

/* Next, we need to send the byte address to read from */
- for (i=7; i>=0; i--) {
+ for (i = 7; i >= 0; i--) {
he_writel(he_dev, val | clocktab[j++] | (((addr >> i) & 1) << 9), HOST_CNTL);
udelay(EEPROM_DELAY);
he_writel(he_dev, val | clocktab[j++] | (((addr >> i) & 1) << 9), HOST_CNTL);
udelay(EEPROM_DELAY);
}

- j=0;
+ j = 0;

val &= 0xFFFFF7FF; /* Turn off write enable */
he_writel(he_dev, val, HOST_CNTL);

/* Now, we can read data from the EEPROM by clocking it in */
- for (i=7; i>=0; i--) {
+ for (i = 7; i >= 0; i--) {
he_writel(he_dev, val | clocktab[j++], HOST_CNTL);
udelay(EEPROM_DELAY);
tmp_read = he_readl(he_dev, HOST_CNTL);

2003-05-30 13:49:11

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

>[ATM]: more coding style cleanups in HE driver

ignore this one... it undoes the rategrid fixes.

2003-06-01 18:53:06

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Arnaldo Carvalho de Melo writes:
>Sure thing, but hey, spin_lock_irqsave and friends take care of how to behave
>when in up or smp, i.e. its how all the other drivers use spinlocks 8)

but on a single processor machine (i.e. #undef CONFIG_SMP) there is no
chance that there will be reads/writes from other processors so i dont
need any locking OR protection from interrupts. so the degenerate case
of spin_lock_irqsave() isnt quite as dengerate as i would like for this
particular spin lock.

2003-06-01 20:45:02

by Alan

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

On Sul, 2003-06-01 at 19:57, chas williams wrote:
> In message <[email protected]>,Arnaldo Carvalho de Melo writes:
> >Sure thing, but hey, spin_lock_irqsave and friends take care of how to behave
> >when in up or smp, i.e. its how all the other drivers use spinlocks 8)
>
> but on a single processor machine (i.e. #undef CONFIG_SMP) there is no
> chance that there will be reads/writes from other processors so i dont
> need any locking OR protection from interrupts. so the degenerate case
> of spin_lock_irqsave() isnt quite as dengerate as i would like for this
> particular spin lock.

Then why are you using spin_lock_irqsave ?

2003-06-01 22:38:45

by Francois Romieu

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

An unconditional HE_SPIN_UNLOCK(he_dev, flags); stands behind the
'close_tx_incomplete' label in he_close(). The following patch should cure
a possible unlock of a non-locked lock (courtesy of kbugs.org, see
http://kbugs.org/cgi-bin/index.py?page=source&version=2.5.70&file=drivers/atm/he.c#line2840).

--- linux-2.5.70-1.1229.7.33-to-1.1330/drivers/atm/he.c Mon Jun 2 00:33:38 2003
+++ linux-2.5.70-1.1229.7.33-to-1.1330/drivers/atm/he.c Mon Jun 2 00:34:29 2003
@@ -2731,12 +2731,13 @@ he_close(struct atm_vcc *vcc)
remove_wait_queue(&he_vcc->tx_waitq, &wait);
set_current_state(TASK_RUNNING);

+ HE_SPIN_LOCK(he_dev, flags);
+
if (timeout == 0) {
hprintk("close tx timeout cid 0x%x\n", cid);
goto close_tx_incomplete;
}

- HE_SPIN_LOCK(he_dev, flags);
while (!((tsr4 = he_readl_tsr4(he_dev, cid)) & TSR4_SESSION_ENDED)) {
HPRINTK("close tx cid 0x%x !TSR4_SESSION_ENDED (tsr4 = 0x%x)\n", cid, tsr4);
udelay(250);

2003-06-01 22:54:30

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Alan Cox writes:
>> but on a single processor machine (i.e. #undef CONFIG_SMP) there is no
>> chance that there will be reads/writes from other processors so i dont
>> need any locking OR protection from interrupts. so the degenerate case
>> of spin_lock_irqsave() isnt quite as dengerate as i would like for this
>> particular spin lock.
>
>Then why are you using spin_lock_irqsave ?

meaning just use spin_lock() or what?

2003-06-02 01:31:25

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

From: chas williams <[email protected]>
Date: Sun, 01 Jun 2003 18:58:26 -0400

In message <[email protected]>,Alan Cox writes:
>Then why are you using spin_lock_irqsave ?

meaning just use spin_lock() or what?

Alan/Chas, there are two different issues here:

1) Aparently the bug only needs to be worked around when
multiple cpus can access the card at the same time.

Therefore on uniprocessor the bug isn't relevant.

2) Therefore, the lock needs to protect register accesses
from all contexts. Therefore he needs an IRQ protecting
lock.

Therefore it isn't legal for him to use a non-IRQ protecting
spinlock.

I personally don't think it's worth all the maintainence cost
to special case all of this junk for uniprocessor.

2003-06-02 01:21:01

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

From: chas williams <[email protected]>
Date: Sun, 01 Jun 2003 14:57:02 -0400

but on a single processor machine (i.e. #undef CONFIG_SMP) there is
no chance that there will be reads/writes from other processors so
i dont need any locking OR protection from interrupts.

Either you need to pretect a code sequence from an IRQ context
execution while holding the lock or you don't. :-)

Really, all your macros just make the driver that much less
portable. And I don't see what you'll gain by having interrupts
enabled for such short sequences of code on uniprocessor.

2003-06-02 01:33:30

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

Em Sun, Jun 01, 2003 at 06:42:54PM -0700, David S. Miller escreveu:
> From: chas williams <[email protected]>
> Date: Sun, 01 Jun 2003 18:58:26 -0400
>
> In message <[email protected]>,Alan Cox writes:
> >Then why are you using spin_lock_irqsave ?
>
> meaning just use spin_lock() or what?
>
> Alan/Chas, there are two different issues here:
>
> 1) Aparently the bug only needs to be worked around when
> multiple cpus can access the card at the same time.
>
> Therefore on uniprocessor the bug isn't relevant.
>
> 2) Therefore, the lock needs to protect register accesses
> from all contexts. Therefore he needs an IRQ protecting
> lock.
>
> Therefore it isn't legal for him to use a non-IRQ protecting
> spinlock.
>
> I personally don't think it's worth all the maintainence cost
> to special case all of this junk for uniprocessor.

Agreed.

- Arnaldo

2003-06-02 02:30:34

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,"David S. Miller" writes:
>1) Aparently the bug only needs to be worked around when
> multiple cpus can access the card at the same time.
> Therefore on uniprocessor the bug isn't relevant.

right. which is why i optimized the HE_SPIN_LOCK away on
!CONFIG_SMP. actually, its probably not a problem on certain
smp machines either. i dont believe the i386 will reorder
read/writes from multiple cpus so in theory it would be safe to
do away with this lock on smp i386's. the only arch that
can reorder (and actually has posted read/writes) is the ia64/sn2.

>I personally don't think it's worth all the maintainence cost
>to special case all of this junk for uniprocessor.

well it wasnt much cost but i essentially decided this some
time ago and just defined HE_SPIN_LOCK to be spin_lock_irqsave.
the lock probably needs to be more fine-grained though in the
tasklet handler though.

2003-06-02 14:35:31

by Alan

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

On Llu, 2003-06-02 at 03:34, chas williams wrote:
> smp machines either. i dont believe the i386 will reorder
> read/writes from multiple cpus so in theory it would be safe to
> do away with this lock on smp i386's. the only arch that
> can reorder (and actually has posted read/writes) is the ia64/sn2.

The only i386 platforms that will re-order writes I know of are the
Natsemi Geode (which will do magic so that the write order seen
by PCI is always ordered), Pentium Pro (fence bug) and IDT
Winchip (but only to main memory)

2003-06-04 04:40:37

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

From: chas williams <[email protected]>
Date: Fri, 30 May 2003 10:29:58 -0400

>ignore this one... it undoes the rategrid fixes.

ok, this one should be right. sorry about the previous nonsense.
this ones a bit larger since i fixed some whitespace issues.

[ATM]: more cleanup in HE driver

Applied, thanks.

2003-06-18 20:56:23

[permalink] [raw]

Subject: Re: [PATCH][ATM] assorted he driver cleanup

In message <[email protected]>,Francois Romieu w
rites:
>An unconditional HE_SPIN_UNLOCK(he_dev, flags); stands behind the
>'close_tx_incomplete' label in he_close(). The following patch should cure
>a possible unlock of a non-locked lock (courtesy of kbugs.org, see
>http://kbugs.org/cgi-bin/index.py?page=source&version=2.5.70&file=drivers/atm/
>he.c#line2840).

dave, please apply the following patch:

[atm]: he: cure possible unlock of a non-locked lock

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.1333 -> 1.1334
# drivers/atm/he.c 1.15 -> 1.16
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/06/18 [email protected] 1.1334
# fix possible unlock of a non-locked lock
# --------------------------------------------
#
diff -Nru a/drivers/atm/he.c b/drivers/atm/he.c
--- a/drivers/atm/he.c Wed Jun 18 17:07:21 2003
+++ b/drivers/atm/he.c Wed Jun 18 17:07:21 2003
@@ -2685,12 +2685,13 @@
remove_wait_queue(&he_vcc->tx_waitq, &wait);
set_current_state(TASK_RUNNING);

+ spin_lock_irqsave(&he_dev->global_lock, flags);
+
if (timeout == 0) {
hprintk("close tx timeout cid 0x%x\n", cid);
goto close_tx_incomplete;
}

- spin_lock_irqsave(&he_dev->global_lock, flags);
while (!((tsr4 = he_readl_tsr4(he_dev, cid)) & TSR4_SESSION_ENDED)) {
HPRINTK("close tx cid 0x%x !TSR4_SESSION_ENDED (tsr4 = 0x%x)\n", cid, tsr4);
udelay(250);

2003-06-19 00:56:17