26 messages, 6 authors, 2006-12-13 · open the first message on its own page
From: Linas Vepstas <hidden>
Date: 2006-12-06 22:32:27
Andrew, Please apply and forward upstream. This series of 16 small patches consist of mostly of various cleanups, a few fixes, and a clarification of the flow of the RX side of the spidernet ethernet driver. The first patch, though, is a resubmit of an old patch. --linas
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:28:01
The current driver code performs 512 DMA mappings of a bunch of 32-byte structures. This is silly, as they are all in contiguous memory. Ths patch changes the code to DMA map the entie area with just one call. Signed-off-by: Linas Vepstas <redacted> Acked-by: Joel Schopp <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 107 +++++++++++++++++++++++------------------------ drivers/net/spider_net.h | 16 ++----- 2 files changed, 59 insertions(+), 64 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 15:53:23.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 15:56:20.000000000 -0600@@ -269,25 +269,6 @@ spider_net_get_descr_status(struct spide } /** - * spider_net_free_chain - free descriptor chain - * @card: card structure - * @chain: address of chain - * - */ -static void -spider_net_free_chain(struct spider_net_card *card, - struct spider_net_descr_chain *chain) -{ - struct spider_net_descr *descr; - - for (descr = chain->tail; !descr->bus_addr; descr = descr->next) { - pci_unmap_single(card->pdev, descr->bus_addr, - SPIDER_NET_DESCR_SIZE, PCI_DMA_BIDIRECTIONAL); - descr->bus_addr = 0; - } -} - -/** * spider_net_init_chain - links descriptor chain * @card: card structure * @chain: address of chain
@@ -299,15 +280,15 @@ spider_net_free_chain(struct spider_net_ * * returns 0 on success, <0 on failure */ -static int +static void spider_net_init_chain(struct spider_net_card *card, struct spider_net_descr_chain *chain, struct spider_net_descr *start_descr, + dma_addr_t buf, int no) { int i; struct spider_net_descr *descr; - dma_addr_t buf; descr = start_descr; memset(descr, 0, sizeof(*descr) * no);
@@ -316,17 +297,12 @@ spider_net_init_chain(struct spider_net_ for (i=0; i<no; i++, descr++) { descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; - buf = pci_map_single(card->pdev, descr, - SPIDER_NET_DESCR_SIZE, - PCI_DMA_BIDIRECTIONAL); - - if (pci_dma_mapping_error(buf)) - goto iommu_error; - descr->bus_addr = buf; + descr->next_descr_addr = 0; descr->next = descr + 1; descr->prev = descr - 1; + buf += sizeof(struct spider_net_descr); } /* do actual circular list */ (descr-1)->next = start_descr;
@@ -335,17 +311,6 @@ spider_net_init_chain(struct spider_net_ spin_lock_init(&chain->lock); chain->head = start_descr; chain->tail = start_descr; - - return 0; - -iommu_error: - descr = start_descr; - for (i=0; i < no; i++, descr++) - if (descr->bus_addr) - pci_unmap_single(card->pdev, descr->bus_addr, - SPIDER_NET_DESCR_SIZE, - PCI_DMA_BIDIRECTIONAL); - return -ENOMEM; } /**
@@ -1652,24 +1617,32 @@ spider_net_open(struct net_device *netde { struct spider_net_card *card = netdev_priv(netdev); struct spider_net_descr *descr; - int i, result; + int result = -ENOMEM; - result = -ENOMEM; - if (spider_net_init_chain(card, &card->tx_chain, card->descr, - card->num_tx_desc)) - goto alloc_tx_failed; + card->descr_dma_addr = pci_map_single(card->pdev, card->descr, + (card->num_tx_desc+card->num_rx_desc)*sizeof(struct spider_net_descr), + PCI_DMA_BIDIRECTIONAL); + if (pci_dma_mapping_error(card->descr_dma_addr)) + return -ENOMEM; + + spider_net_init_chain(card, &card->tx_chain, card->descr, + card->descr_dma_addr, + card->num_tx_desc); card->low_watermark = NULL; /* rx_chain is after tx_chain, so offset is descr + tx_count */ - if (spider_net_init_chain(card, &card->rx_chain, + spider_net_init_chain(card, &card->rx_chain, card->descr + card->num_tx_desc, - card->num_rx_desc)) - goto alloc_rx_failed; + card->descr_dma_addr + + card->num_tx_desc * sizeof(struct spider_net_descr), + card->num_rx_desc); descr = card->rx_chain.head; - for (i=0; i < card->num_rx_desc; i++, descr++) + do { descr->next_descr_addr = descr->next->bus_addr; + descr = descr->next; + } while (descr != card->rx_chain.head); /* allocate rx skbs */ if (spider_net_alloc_rx_skbs(card))
@@ -1695,10 +1668,21 @@ spider_net_open(struct net_device *netde register_int_failed: spider_net_free_rx_chain_contents(card); alloc_skbs_failed: - spider_net_free_chain(card, &card->rx_chain); -alloc_rx_failed: - spider_net_free_chain(card, &card->tx_chain); -alloc_tx_failed: + descr = card->rx_chain.head; + do { + descr->bus_addr = 0; + descr = descr->next; + } while (descr != card->rx_chain.head); + + descr = card->tx_chain.head; + do { + descr->bus_addr = 0; + descr = descr->next; + } while (descr != card->tx_chain.head); + + pci_unmap_single(card->pdev, card->descr_dma_addr, + (card->num_tx_desc+card->num_rx_desc)*sizeof(struct spider_net_descr), + PCI_DMA_BIDIRECTIONAL); return result; }
@@ -1901,6 +1885,7 @@ int spider_net_stop(struct net_device *netdev) { struct spider_net_card *card = netdev_priv(netdev); + struct spider_net_descr *descr; tasklet_kill(&card->rxram_full_tl); netif_poll_disable(netdev);
@@ -1924,9 +1909,23 @@ spider_net_stop(struct net_device *netde /* release chains */ spider_net_release_tx_chain(card, 1); + spider_net_free_rx_chain_contents(card); + + descr = card->rx_chain.head; + do { + descr->bus_addr = 0; + descr = descr->next; + } while (descr != card->rx_chain.head); + + descr = card->tx_chain.head; + do { + descr->bus_addr = 0; + descr = descr->next; + } while (descr != card->tx_chain.head); - spider_net_free_chain(card, &card->tx_chain); - spider_net_free_chain(card, &card->rx_chain); + pci_unmap_single(card->pdev, card->descr_dma_addr, + (card->num_tx_desc+card->num_rx_desc)*sizeof(struct spider_net_descr), + PCI_DMA_BIDIRECTIONAL); return 0; }
Index: linux-2.6.19-git7/drivers/net/spider_net.h ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.h 2006-12-06 15:53:23.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.h 2006-12-06 15:56:20.000000000 -0600@@ -397,8 +397,6 @@ struct spider_net_descr_chain { * 701b8000 would be correct, but every packets gets that flag */ #define SPIDER_NET_DESTROY_RX_FLAGS 0x700b8000 -#define SPIDER_NET_DESCR_SIZE 32 - /* this will be bigger some time */ struct spider_net_options { int rx_csum; /* for rx: if 0 ip_summed=NONE,
@@ -437,28 +435,26 @@ struct spider_net_card { void __iomem *regs; + int num_rx_desc; + int num_tx_desc; struct spider_net_descr_chain tx_chain; struct spider_net_descr_chain rx_chain; struct spider_net_descr *low_watermark; + dma_addr_t descr_dma_addr; - struct net_device_stats netdev_stats; - - struct spider_net_options options; - - spinlock_t intmask_lock; struct tasklet_struct rxram_full_tl; struct timer_list tx_timer; - struct work_struct tx_timeout_task; atomic_t tx_timeout_task_counter; wait_queue_head_t waitq; /* for ethtool */ int msg_enable; - int num_rx_desc; - int num_tx_desc; + struct net_device_stats netdev_stats; struct spider_net_extra_stats spider_stats; + struct spider_net_options options; + /* Must be last element in the structure */ struct spider_net_descr descr[0]; };
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:29:57
This patch adds net_ratelimit to many of the printks in order to limit extraneous warning messages This patch supercedes all previous ratelimit patches. This has been tested, please apply. From: James K Lewis <redacted> Signed-off-by: James K Lewis <redacted> Signed-off-by: Linas Vepstas <redacted> ---- drivers/net/spider_net.c | 11 +++++------ drivers/net/spider_net.h | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 15:56:20.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 15:58:27.000000000 -0600@@ -1008,11 +1008,10 @@ spider_net_decode_one_descr(struct spide if ( (status != SPIDER_NET_DESCR_COMPLETE) && (status != SPIDER_NET_DESCR_FRAME_END) ) { - if (netif_msg_rx_err(card)) { + if (netif_msg_rx_err(card)) pr_err("%s: RX descriptor with state %d\n", card->netdev->name, status); - card->spider_stats.rx_desc_unk_state++; - } + card->spider_stats.rx_desc_unk_state++; goto refill; }
@@ -1331,7 +1330,7 @@ spider_net_handle_error_irq(struct spide case SPIDER_NET_GRFAFLLINT: /* fallthrough */ case SPIDER_NET_GRMFLLINT: if (netif_msg_intr(card) && net_ratelimit()) - pr_debug("Spider RX RAM full, incoming packets " + pr_err("Spider RX RAM full, incoming packets " "might be discarded!\n"); spider_net_rx_irq_off(card); tasklet_schedule(&card->rxram_full_tl);
@@ -1349,7 +1348,7 @@ spider_net_handle_error_irq(struct spide case SPIDER_NET_GDCDCEINT: /* fallthrough */ case SPIDER_NET_GDBDCEINT: /* fallthrough */ case SPIDER_NET_GDADCEINT: - if (netif_msg_intr(card)) + if (netif_msg_intr(card) && net_ratelimit()) pr_err("got descriptor chain end interrupt, " "restarting DMAC %c.\n", 'D'-(i-SPIDER_NET_GDDDCEINT)/3);
@@ -1420,7 +1419,7 @@ spider_net_handle_error_irq(struct spide break; } - if ((show_error) && (netif_msg_intr(card))) + if ((show_error) && (netif_msg_intr(card)) && net_ratelimit()) pr_err("Got error interrupt on %s, GHIINT0STS = 0x%08x, " "GHIINT1STS = 0x%08x, GHIINT2STS = 0x%08x\n", card->netdev->name,
Index: linux-2.6.19-git7/drivers/net/spider_net.h ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.h 2006-12-06 15:56:20.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.h 2006-12-06 15:57:28.000000000 -0600@@ -24,7 +24,7 @@ #ifndef _SPIDER_NET_H #define _SPIDER_NET_H -#define VERSION "1.6 A" +#define VERSION "1.6 B" #include "sungem_phy.h"
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:31:41
The RX packet handling can be called from several places, yet does not protect the rx ring structure. This patch places the ring buffer pointers under a lock. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 15:58:27.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:01:49.000000000 -0600@@ -969,28 +969,33 @@ static int spider_net_decode_one_descr(struct spider_net_card *card, int napi) { struct spider_net_descr_chain *chain = &card->rx_chain; - struct spider_net_descr *descr = chain->tail; + struct spider_net_descr *descr; int status; int result; + unsigned long flags; + + spin_lock_irqsave(&chain->lock, flags); + descr = chain->tail; status = spider_net_get_descr_status(descr); if (status == SPIDER_NET_DESCR_CARDOWNED) { /* nothing in the descriptor yet */ - result=0; - goto out; + spin_unlock_irqrestore(&chain->lock, flags); + return 0; } if (status == SPIDER_NET_DESCR_NOT_IN_USE) { /* not initialized yet, the ring must be empty */ + spin_unlock_irqrestore(&chain->lock, flags); spider_net_refill_rx_chain(card); spider_net_enable_rxdmac(card); - result=0; - goto out; + return 0; } /* descriptor definitively used -- move on tail */ chain->tail = descr->next; + spin_unlock_irqrestore(&chain->lock, flags); result = 0; if ( (status == SPIDER_NET_DESCR_RESPONSE_ERROR) ||
@@ -1022,7 +1027,6 @@ refill: /* change the descriptor state: */ if (!napi) spider_net_refill_rx_chain(card); -out: return result; }
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:33:21
Refactor how spider_net_refill_rx_chain() is called. No functional change; this just simplifies the code by moving the subroutine call to a more appropriate spot. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:01:49.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:02:58.000000000 -0600@@ -1023,10 +1023,8 @@ spider_net_decode_one_descr(struct spide /* ok, we've got a packet in descr */ result = spider_net_pass_skb_up(descr, card, napi); refill: - descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; /* change the descriptor state: */ - if (!napi) - spider_net_refill_rx_chain(card); + descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; return result; }
@@ -1205,8 +1203,11 @@ spider_net_set_mac(struct net_device *ne static void spider_net_handle_rxram_full(struct spider_net_card *card) { - while (spider_net_decode_one_descr(card, 0)) - ; + int rc = 1; + while (rc) { + rc = spider_net_decode_one_descr(card, 0); + spider_net_refill_rx_chain(card); + } spider_net_enable_rxchtails(card); spider_net_enable_rxdmac(card); netif_rx_schedule(card->netdev);
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:34:53
One of the unlikely error branches has an skb memory leak. Fix this by handling the error conditions consistently. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:02:58.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:00.000000000 -0600@@ -884,8 +884,8 @@ spider_net_do_ioctl(struct net_device *n * * returns 1 on success, 0 if no packet was passed to the stack * - * iommu-unmaps the skb, fills out skb structure and passes the data to the - * stack. The descriptor state is not changed. + * Fills out skb structure and passes the data to the stack. + * The descriptor state is not changed. */ static int spider_net_pass_skb_up(struct spider_net_descr *descr,
@@ -900,10 +900,6 @@ spider_net_pass_skb_up(struct spider_net netdev = card->netdev; - /* unmap descriptor */ - pci_unmap_single(card->pdev, descr->buf_addr, SPIDER_NET_MAX_FRAME, - PCI_DMA_FROMDEVICE); - /* the cases we'll throw away the packet immediately */ if (data_error & SPIDER_NET_DESTROY_RX_FLAGS) { if (netif_msg_rx_err(card))
@@ -998,6 +994,11 @@ spider_net_decode_one_descr(struct spide spin_unlock_irqrestore(&chain->lock, flags); result = 0; + + /* unmap descriptor */ + pci_unmap_single(card->pdev, descr->buf_addr, + SPIDER_NET_MAX_FRAME, PCI_DMA_FROMDEVICE); + if ( (status == SPIDER_NET_DESCR_RESPONSE_ERROR) || (status == SPIDER_NET_DESCR_PROTECTION_ERROR) || (status == SPIDER_NET_DESCR_FORCE_END) ) {
@@ -1005,8 +1006,6 @@ spider_net_decode_one_descr(struct spide pr_err("%s: dropping RX descriptor with state %d\n", card->netdev->name, status); card->netdev_stats.rx_dropped++; - pci_unmap_single(card->pdev, descr->buf_addr, - SPIDER_NET_MAX_FRAME, PCI_DMA_FROMDEVICE); dev_kfree_skb_irq(descr->skb); goto refill; }
@@ -1014,9 +1013,10 @@ spider_net_decode_one_descr(struct spide if ( (status != SPIDER_NET_DESCR_COMPLETE) && (status != SPIDER_NET_DESCR_FRAME_END) ) { if (netif_msg_rx_err(card)) - pr_err("%s: RX descriptor with state %d\n", + pr_err("%s: RX descriptor with unkown state %d\n", card->netdev->name, status); card->spider_stats.rx_desc_unk_state++; + dev_kfree_skb_irq(descr->skb); goto refill; }
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:36:11
Another skb leak in an error branch. Fix this by adding call to dev_kfree_skb_irq() after moving to a more appropriate spot. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:00.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:04.000000000 -0600@@ -897,19 +897,8 @@ spider_net_pass_skb_up(struct spider_net data_status = descr->data_status; data_error = descr->data_error; - netdev = card->netdev; - /* the cases we'll throw away the packet immediately */ - if (data_error & SPIDER_NET_DESTROY_RX_FLAGS) { - if (netif_msg_rx_err(card)) - pr_err("error in received descriptor found, " - "data_status=x%08x, data_error=x%08x\n", - data_status, data_error); - card->spider_stats.rx_desc_error++; - return 0; - } - skb = descr->skb; skb->dev = netdev; skb_put(skb, descr->valid_size);
@@ -1020,6 +1009,18 @@ spider_net_decode_one_descr(struct spide goto refill; } + /* The cases we'll throw away the packet immediately */ + if (descr->data_error & SPIDER_NET_DESTROY_RX_FLAGS) { + if (netif_msg_rx_err(card)) + pr_err("%s: error in received descriptor found, " + "data_status=x%08x, data_error=x%08x\n", + card->netdev->name, + descr->data_status, descr->data_error); + card->spider_stats.rx_desc_error++; + dev_kfree_skb_irq(descr->skb); + goto refill; + } + /* ok, we've got a packet in descr */ result = spider_net_pass_skb_up(descr, card, napi); refill:
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:37:37
Simplify the somewhat convoluted use of return codes in the rx buffre handling. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:04.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:06.000000000 -0600@@ -882,12 +882,10 @@ spider_net_do_ioctl(struct net_device *n * @card: card structure * @napi: whether caller is in NAPI context * - * returns 1 on success, 0 if no packet was passed to the stack - * * Fills out skb structure and passes the data to the stack. * The descriptor state is not changed. */ -static int +static void spider_net_pass_skb_up(struct spider_net_descr *descr, struct spider_net_card *card, int napi) {
@@ -935,8 +933,6 @@ spider_net_pass_skb_up(struct spider_net /* update netdevice statistics */ card->netdev_stats.rx_packets++; card->netdev_stats.rx_bytes += skb->len; - - return 1; } /**
@@ -956,7 +952,6 @@ spider_net_decode_one_descr(struct spide struct spider_net_descr_chain *chain = &card->rx_chain; struct spider_net_descr *descr; int status; - int result; unsigned long flags; spin_lock_irqsave(&chain->lock, flags);
@@ -982,8 +977,6 @@ spider_net_decode_one_descr(struct spide chain->tail = descr->next; spin_unlock_irqrestore(&chain->lock, flags); - result = 0; - /* unmap descriptor */ pci_unmap_single(card->pdev, descr->buf_addr, SPIDER_NET_MAX_FRAME, PCI_DMA_FROMDEVICE);
@@ -995,8 +988,7 @@ spider_net_decode_one_descr(struct spide pr_err("%s: dropping RX descriptor with state %d\n", card->netdev->name, status); card->netdev_stats.rx_dropped++; - dev_kfree_skb_irq(descr->skb); - goto refill; + goto bad_desc; } if ( (status != SPIDER_NET_DESCR_COMPLETE) &&
@@ -1005,8 +997,7 @@ spider_net_decode_one_descr(struct spide pr_err("%s: RX descriptor with unkown state %d\n", card->netdev->name, status); card->spider_stats.rx_desc_unk_state++; - dev_kfree_skb_irq(descr->skb); - goto refill; + goto bad_desc; } /* The cases we'll throw away the packet immediately */
@@ -1017,16 +1008,18 @@ spider_net_decode_one_descr(struct spide card->netdev->name, descr->data_status, descr->data_error); card->spider_stats.rx_desc_error++; - dev_kfree_skb_irq(descr->skb); - goto refill; + goto bad_desc; } - /* ok, we've got a packet in descr */ - result = spider_net_pass_skb_up(descr, card, napi); -refill: - /* change the descriptor state: */ + /* Ok, we've got a packet in descr */ + spider_net_pass_skb_up(descr, card, napi); descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; - return result; + return 1; + +bad_desc: + dev_kfree_skb_irq(descr->skb); + descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; + return 0; } /**
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:39:17
The invocation of the rx ring refill routine is haphazard; centralize and make its usage consistent. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:06.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:08.000000000 -0600@@ -968,8 +968,6 @@ spider_net_decode_one_descr(struct spide if (status == SPIDER_NET_DESCR_NOT_IN_USE) { /* not initialized yet, the ring must be empty */ spin_unlock_irqrestore(&chain->lock, flags); - spider_net_refill_rx_chain(card); - spider_net_enable_rxdmac(card); return 0; }
@@ -1058,6 +1056,7 @@ spider_net_poll(struct net_device *netde netdev->quota -= packets_done; *budget -= packets_done; spider_net_refill_rx_chain(card); + spider_net_enable_rxdmac(card); /* if all packets are in the stack, enable interrupts and return 0 */ /* if not, return 1 */
@@ -1197,11 +1196,9 @@ spider_net_set_mac(struct net_device *ne static void spider_net_handle_rxram_full(struct spider_net_card *card) { - int rc = 1; - while (rc) { - rc = spider_net_decode_one_descr(card, 0); - spider_net_refill_rx_chain(card); - } + while (spider_net_decode_one_descr(card, 0)); + + spider_net_refill_rx_chain(card); spider_net_enable_rxchtails(card); spider_net_enable_rxdmac(card); netif_rx_schedule(card->netdev);
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:40:11
Two distinct if() statements have the ame body. Merge the clauses. Also clean up punctuation, capitalization, etc. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:08.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:09.000000000 -0600@@ -936,15 +936,16 @@ spider_net_pass_skb_up(struct spider_net } /** - * spider_net_decode_one_descr - processes an rx descriptor + * spider_net_decode_one_descr - Processes an RX descriptor * @card: card structure * @napi: whether caller is in NAPI context * - * returns 1 if a packet has been sent to the stack, otherwise 0 + * Returns 1 if a packet has been sent to the stack, otherwise 0. * - * processes an rx descriptor by iommu-unmapping the data buffer and passing - * the packet up to the stack. This function is called in softirq - * context, e.g. either bottom half from interrupt or NAPI polling context + * Processes an RX descriptor by iommu-unmapping the data buffer + * and passing the packet up to the stack. This function is called + * in a softirq context, e.g. either bottom half from interrupt or + * NAPI polling context. */ static int spider_net_decode_one_descr(struct spider_net_card *card, int napi)
@@ -959,23 +960,18 @@ spider_net_decode_one_descr(struct spide status = spider_net_get_descr_status(descr); - if (status == SPIDER_NET_DESCR_CARDOWNED) { - /* nothing in the descriptor yet */ + /* Nothing in the descriptor yet, or ring is empty */ + if ( (status == SPIDER_NET_DESCR_CARDOWNED) || + (status == SPIDER_NET_DESCR_NOT_IN_USE) ) { spin_unlock_irqrestore(&chain->lock, flags); return 0; } - if (status == SPIDER_NET_DESCR_NOT_IN_USE) { - /* not initialized yet, the ring must be empty */ - spin_unlock_irqrestore(&chain->lock, flags); - return 0; - } - - /* descriptor definitively used -- move on tail */ + /* Descriptor definitively used -- move on tail. */ chain->tail = descr->next; spin_unlock_irqrestore(&chain->lock, flags); - /* unmap descriptor */ + /* Unmap descriptor. */ pci_unmap_single(card->pdev, descr->buf_addr, SPIDER_NET_MAX_FRAME, PCI_DMA_FROMDEVICE);
@@ -998,7 +994,7 @@ spider_net_decode_one_descr(struct spide goto bad_desc; } - /* The cases we'll throw away the packet immediately */ + /* The cases we'll throw away the packet immediately. */ if (descr->data_error & SPIDER_NET_DESTROY_RX_FLAGS) { if (netif_msg_rx_err(card)) pr_err("%s: error in received descriptor found, "
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:41:35
Remove unused variable; this makes code easier to read. Tweak commentary. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:09.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:11.000000000 -0600@@ -341,17 +341,16 @@ spider_net_free_rx_chain_contents(struct * @card: card structure * @descr: descriptor to re-init * - * return 0 on succes, <0 on failure + * Return 0 on succes, <0 on failure. * - * allocates a new rx skb, iommu-maps it and attaches it to the descriptor. - * Activate the descriptor state-wise + * Allocates a new rx skb, iommu-maps it and attaches it to the + * descriptor. Mark the descriptor as activated, ready-to-use. */ static int spider_net_prepare_rx_descr(struct spider_net_card *card, struct spider_net_descr *descr) { dma_addr_t buf; - int error = 0; int offset; int bufsize;
@@ -379,7 +378,7 @@ spider_net_prepare_rx_descr(struct spide (SPIDER_NET_RXBUF_ALIGN - 1); if (offset) skb_reserve(descr->skb, SPIDER_NET_RXBUF_ALIGN - offset); - /* io-mmu-map the skb */ + /* iommu-map the skb */ buf = pci_map_single(card->pdev, descr->skb->data, SPIDER_NET_MAX_FRAME, PCI_DMA_FROMDEVICE); descr->buf_addr = buf;
@@ -394,7 +393,7 @@ spider_net_prepare_rx_descr(struct spide SPIDER_NET_DMAC_NOINTR_COMPLETE; } - return error; + return 0; } /**
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:42:42
Tell the hardware the location of the rx ring tail. More punctuation cleanup. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:11.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:13.000000000 -0600@@ -457,10 +457,10 @@ spider_net_refill_rx_chain(struct spider } /** - * spider_net_alloc_rx_skbs - allocates rx skbs in rx descriptor chains + * spider_net_alloc_rx_skbs - Allocates rx skbs in rx descriptor chains * @card: card structure * - * returns 0 on success, <0 on failure + * Returns 0 on success, <0 on failure. */ static int spider_net_alloc_rx_skbs(struct spider_net_card *card)
@@ -471,17 +471,18 @@ spider_net_alloc_rx_skbs(struct spider_n result = -ENOMEM; chain = &card->rx_chain; - /* put at least one buffer into the chain. if this fails, - * we've got a problem. if not, spider_net_refill_rx_chain - * will do the rest at the end of this function */ + /* Put at least one buffer into the chain. if this fails, + * we've got a problem. If not, spider_net_refill_rx_chain + * will do the rest at the end of this function. */ if (spider_net_prepare_rx_descr(card, chain->head)) goto error; else chain->head = chain->head->next; - /* this will allocate the rest of the rx buffers; if not, it's - * business as usual later on */ + /* This will allocate the rest of the rx buffers; + * if not, it's business as usual later on. */ spider_net_refill_rx_chain(card); + spider_net_enable_rxchtails(card); spider_net_enable_rxdmac(card); return 0;
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:44:01
Re-enable irq's after emptying the RX ring; these had been previously turned off on reception of the rxram_full interrupt. More punctuation cleanup. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:13.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:15.000000000 -0600@@ -1182,12 +1182,11 @@ spider_net_set_mac(struct net_device *ne } /** - * spider_net_handle_rxram_full - cleans up RX ring upon RX RAM full interrupt + * spider_net_handle_rxram_full - Clean RX ring on RX RAM full interrupt * @card: card structure * - * spider_net_handle_rxram_full empties the RX ring so that spider can put - * more packets in it and empty its RX RAM. This is called in bottom half - * context + * Empty the RX ring so that the hardware can put more packets + * in it and empty its RX RAM. This is called in bottom half context. */ static void spider_net_handle_rxram_full(struct spider_net_card *card)
@@ -1198,6 +1197,7 @@ spider_net_handle_rxram_full(struct spid spider_net_enable_rxchtails(card); spider_net_enable_rxdmac(card); netif_rx_schedule(card->netdev); + spider_net_rx_irq_on(card); } /**
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:45:23
Add memory barrier to make sure that the rest of the RX descriptor state is flushed to memory before we tell the hardware that its ready to go. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 1 + 1 file changed, 1 insertion(+) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:15.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:16.000000000 -0600@@ -389,6 +389,7 @@ spider_net_prepare_rx_descr(struct spide card->spider_stats.rx_iommu_map_error++; descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; } else { + wmb(); descr->dmac_cmd_status = SPIDER_NET_DESCR_CARDOWNED | SPIDER_NET_DMAC_NOINTR_COMPLETE; }
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:46:32
Delete possible source of chain corruption; the hardware already knows the location of the tail, and writing it again is likely to mess it up. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 1 - 1 file changed, 1 deletion(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:16.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:18.000000000 -0600@@ -1195,7 +1195,6 @@ spider_net_handle_rxram_full(struct spid while (spider_net_decode_one_descr(card, 0)); spider_net_refill_rx_chain(card); - spider_net_enable_rxchtails(card); spider_net_enable_rxdmac(card); netif_rx_schedule(card->netdev); spider_net_rx_irq_on(card);
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:49:31
Add some debugging and error printing. The show_rx_chain() prints out the status of the rx chain, which shows that the status of the descriptors gets messed up after the second & subsequent RX ramfulls. Print out contents of bad packets if error occurs. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:18.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:03:20.000000000 -0600@@ -936,6 +936,34 @@ spider_net_pass_skb_up(struct spider_net card->netdev_stats.rx_bytes += skb->len; } +#ifdef DEBUG +static void show_rx_chain(struct spider_net_card *card) +{ + struct spider_net_descr_chain *chain = &card->rx_chain; + struct spider_net_descr *start= chain->tail; + struct spider_net_descr *descr= start; + int status; + + int cnt = 0; + int cstat = spider_net_get_descr_status(descr); + printk(KERN_INFO "RX chain tail at descr=%ld\n", + (start - card->descr) - card->num_tx_desc); + status = cstat; + do + { + status = spider_net_get_descr_status(descr); + if (cstat != status) { + printk(KERN_INFO "Have %d descrs with stat=x%08x\n", cnt, cstat); + cstat = status; + cnt = 0; + } + cnt ++; + descr = descr->next; + } while (descr != start); + printk(KERN_INFO "Last %d descrs with stat=x%08x\n", cnt, cstat); +} +#endif + /** * spider_net_decode_one_descr - Processes an RX descriptor * @card: card structure
@@ -1006,6 +1034,25 @@ spider_net_decode_one_descr(struct spide goto bad_desc; } + if (descr->dmac_cmd_status & 0xfefe) { + pr_err("%s: bad status, cmd_status=x%08x\n", + card->netdev->name, + descr->dmac_cmd_status); + pr_err("buf_addr=x%08x\n", descr->buf_addr); + pr_err("buf_size=x%08x\n", descr->buf_size); + pr_err("next_descr_addr=x%08x\n", descr->next_descr_addr); + pr_err("result_size=x%08x\n", descr->result_size); + pr_err("valid_size=x%08x\n", descr->valid_size); + pr_err("data_status=x%08x\n", descr->data_status); + pr_err("data_error=x%08x\n", descr->data_error); + pr_err("bus_addr=x%08x\n", descr->bus_addr); + pr_err("which=%ld\n", + (descr - card->descr) - card->num_tx_desc); + + card->spider_stats.rx_desc_error++; + goto bad_desc; + } + /* Ok, we've got a packet in descr */ spider_net_pass_skb_up(descr, card, napi); descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE;
From: Linas Vepstas <hidden>
Date: 2006-12-06 23:52:10
Make the hardware perceive the RX descriptor ring as a null-terminated linked list, instead of a circular ring. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de> ---- drivers/net/spider_net.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c ===================================================================
--- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-06 16:03:20.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-06 16:05:48.000000000 -0600@@ -389,9 +389,13 @@ spider_net_prepare_rx_descr(struct spide card->spider_stats.rx_iommu_map_error++; descr->dmac_cmd_status = SPIDER_NET_DESCR_NOT_IN_USE; } else { + descr->next_descr_addr = 0; wmb(); descr->dmac_cmd_status = SPIDER_NET_DESCR_CARDOWNED | SPIDER_NET_DMAC_NOINTR_COMPLETE; + + wmb(); + descr->prev->next_descr_addr = descr->bus_addr; } return 0;
@@ -1676,12 +1680,6 @@ spider_net_open(struct net_device *netde + card->num_tx_desc * sizeof(struct spider_net_descr), card->num_rx_desc); - descr = card->rx_chain.head; - do { - descr->next_descr_addr = descr->next->bus_addr; - descr = descr->next; - } while (descr != card->rx_chain.head); - /* allocate rx skbs */ if (spider_net_alloc_rx_skbs(card)) goto alloc_skbs_failed;
From: Andrew Morton <hidden>
Date: 2006-12-07 07:08:56
It worries me when a patch series gets resent a few hours later. Did anything change?
From: Jeff Garzik <hidden>
Date: 2006-12-07 10:09:31
Linas Vepstas wrote:
The RX packet handling can be called from several places, yet does not protect the rx ring structure. This patch places the ring buffer pointers under a lock. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de>
This is a HUGELY invasive patch. A sledgehammer. What /specifically/ are these "several places", and what other non-sledgehammer approaches were discarded before arriving at this one? Jeff
From: Christoph Hellwig <hch@infradead.org>
Date: 2006-12-07 10:12:07
On Wed, Dec 06, 2006 at 05:27:45PM -0600, Linas Vepstas wrote:
The current driver code performs 512 DMA mappings of a bunch of 32-byte structures. This is silly, as they are all in contiguous memory. Ths patch changes the code to DMA map the entie area with just one call.
This is still wrong. The descriptor array must be in dma_alloc_coherent memory, not a streaming mapping. (I also think I pointed this out a while ago when I made dma_alloc_coherent node-aware)
From: Linas Vepstas <hidden>
Date: 2006-12-07 17:16:47
On Wed, Dec 06, 2006 at 11:08:47PM -0800, Andrew Morton wrote:
It worries me when a patch series gets resent a few hours later. Did anything change?
I did not resend this patch series. However, I did receive a large number of MTA errors: [off-list ref]: mail forwarding loop for linuxppc-dev@ozlabs.org which might be related to what you saw. (I also cc'ed several lists, and delivry on one of the lists may have been delayed by a few hours??) --linas
From: Linas Vepstas <hidden>
Date: 2006-12-07 17:51:07
On Thu, Dec 07, 2006 at 05:09:20AM -0500, Jeff Garzik wrote:
Linas Vepstas wrote:quoted
The RX packet handling can be called from several places, yet does not protect the rx ring structure. This patch places the ring buffer pointers under a lock. Signed-off-by: Linas Vepstas <redacted> Cc: James K Lewis <redacted> Cc: Arnd Bergmann <arnd@arndb.de>This is a HUGELY invasive patch. A sledgehammer.
I am rather unlear what you perceive as being invasive, since the patch summary states: drivers/net/spider_net.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
What /specifically/ are these "several places",
spider_net_decode_one_descr() is called from spider_net_poll() (which is the netdev->poll callback) and also from spider_net_handle_rxram_full(). The rxramfull routine is called from a tasklet that is fired off after a "RX ram full" interrupt is receved. This interrupt is generated when the hardware runs out of space to store incoming packets. We are seeing this interrupt fire when the CPU is heavily loaded, and a lot of traffic is being fired at the device.
and what other non-sledgehammer approaches were discarded before arriving at this one?
Well, I'm not that good at kernel programming, so I guess I did not perceive this as a "sledgehammer." And alternative approach is to simply ignore the rxramfull interrupt entirely, and depend on poll() do all the work. I'll try this shortly. --linas
From: Stephen Rothwell <hidden>
Date: 2006-12-08 02:12:08
On Thu, 7 Dec 2006 11:16:30 -0600 linas@austin.ibm.com (Linas Vepstas) wrote:
On Wed, Dec 06, 2006 at 11:08:47PM -0800, Andrew Morton wrote:quoted
It worries me when a patch series gets resent a few hours later. Did anything change?I did not resend this patch series. However, I did receive a large number of MTA errors: [off-list ref]: mail forwarding loop for linuxppc-dev@ozlabs.org
Please report such things to postmaster@ozlabs.org and we may be able to do something about them ... If you do report them, please attach the full email with the error. -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2006-12-08 22:47:26
A spinlock is expensive in the fast path, which is why Jeff says it's invasive.
spider_net_decode_one_descr() is called from spider_net_poll() (which is the netdev->poll callback) and also from spider_net_handle_rxram_full(). The rxramfull routine is called from a tasklet that is fired off after a "RX ram full" interrupt is receved. This interrupt is generated when the hardware runs out of space to store incoming packets. We are seeing this interrupt fire when the CPU is heavily loaded, and a lot of traffic is being fired at the device.
How often does that interrupt happen in that case ? A better approach is to keep the fast path (ie. poll()) lockless, and in handle_rxram_full(), the slow path, protect against poll using netif_disable_poll(). Though that means using a work queue, not a tasklet, since it needs to schedule.
quoted
and what other non-sledgehammer approaches were discarded before arriving at this one?Well, I'm not that good at kernel programming, so I guess I did not perceive this as a "sledgehammer." And alternative approach is to simply ignore the rxramfull interrupt entirely, and depend on poll() do all the work. I'll try this shortly.
or you can schedule rx work from the rxramfull interrupt after setting a "something bad happened" flag. Then, poll can check this flag and do the right thing. Ben.
From: Linas Vepstas <hidden>
Date: 2006-12-11 21:07:19
On Sat, Dec 09, 2006 at 09:47:05AM +1100, Benjamin Herrenschmidt wrote:
A spinlock is expensive in the fast path, which is why Jeff says it's invasive.quoted
spider_net_decode_one_descr() is called from spider_net_poll() (which is the netdev->poll callback) and also from spider_net_handle_rxram_full(). The rxramfull routine is called from a tasklet that is fired off after a "RX ram full" interrupt is receved. This interrupt is generated when the hardware runs out of space to store incoming packets. We are seeing this interrupt fire when the CPU is heavily loaded, and a lot of traffic is being fired at the device.How often does that interrupt happen in that case ?
It is hard to reproduce; it is highly dependent on kernel version and network config. It seems to occur when the system is somehow loaded, and the tcp stack is unable to empty out the rx ring in a timely manner. Jim is able o trigger this trivially for some kernels, but not others.
A better approach is to keep the fast path (ie. poll()) lockless, and in handle_rxram_full(), the slow path, protect against poll using netif_disable_poll(). Though that means using a work queue, not a tasklet, since it needs to schedule.
Yes. Actually, I am thinking of treating this interrupt as if it were just another RX interrupt. What the original drivers seemed to want to do was to treat this as some sort of "high priority" rx interrupt, but there doesn't seem to be any real way of doing this, so it seems simpler just to rip out the tasklet and leave it at that.
or you can schedule rx work from the rxramfull interrupt after setting a "something bad happened" flag. Then, poll can check this flag and do the right thing.
Yes, exactly. --linas
From: Linas Vepstas <hidden>
Date: 2006-12-13 17:27:24
On Thu, Dec 07, 2006 at 10:11:51AM +0000, Christoph Hellwig wrote:
On Wed, Dec 06, 2006 at 05:27:45PM -0600, Linas Vepstas wrote:quoted
The current driver code performs 512 DMA mappings of a bunch of 32-byte structures. This is silly, as they are all in contiguous memory. Ths patch changes the code to DMA map the entie area with just one call.This is still wrong. The descriptor array must be in dma_alloc_coherent memory, not a streaming mapping. (I also think I pointed this out a while ago when I made dma_alloc_coherent node-aware)
Sorry, I missed this the first time. I'm splitting this off now; will resubmit shortly. --linas