[PATCH 0/16] Spidernet RX Misc cleanups and fixes

[PATCH 0/16] Spidernet RX Misc cleanups and fixes

[Linas Vepstas]

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

[PATCH 1/16] Spidernet DMA coalescing

[Linas Vepstas]

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 <linas@austin.ibm.com>
Acked-by: Joel Schopp <jschopp@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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];
 };
 

[PATCH 2/16] Spidernet add net_ratelimit to suppress long output

[Linas Vepstas]

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 <jklewis@us.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: Linas Vepstas <jlinas@austin.ibm.com>

----
 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"
 

[PATCH 3/16] Spidernet RX Locking

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;
 }
 

[PATCH 4/16] Spidernet Refactor RX refill

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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);

[PATCH 5/16] Spidernet RX skb mem leak

[Linas Vepstas]

One of the unlikely error branches has an skb memory leak.
Fix this by handling the error conditions consistently.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;
 	}
 

[PATCH 6/16] Spidernet another skb mem leak

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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:

[PATCH 7/16] Spidernet Cleanup return codes

[Linas Vepstas]

Simplify the somewhat convoluted use of return codes
in the rx buffre handling.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;
 }
 
 /**

[PATCH 8/16] Spidernet RX Refill

[Linas Vepstas]

The invocation of the rx ring refill routine is haphazard;
centralize and make its usage consistent.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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);

[PATCH 9/16] Spidernet Merge error branches

[Linas Vepstas]

Two distinct if() statements have the ame body. Merge the clauses.
Also clean up punctuation, capitalization, etc.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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, "

[PATCH 10/16] Spidernet Remove unused variable

[Linas Vepstas]

Remove unused variable; this makes code easier to read.
Tweak commentary.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;
 }
 
 /**

[PATCH 11/16] Spidernet RX Chain tail

[Linas Vepstas]

Tell the hardware the location of the rx ring tail.
More punctuation cleanup.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;
 

[PATCH 12/16] Spidernet Turn RX irq back on

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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);
 }
 
 /**

[PATCH 13/16] Spidernet Memory barrier

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;
 	}

[PATCH 14/16] Spidernet Avoid possible RX chain corruption

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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);

[PATCH 15/16] Spidernet RX Debugging printout

[Linas Vepstas]

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 <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;

[PATCH 16/16] Spidernet Rework RX linked list

[Linas Vepstas]

Make the hardware perceive the RX descriptor ring as a 
null-terminated linked list, instead of a circular ring.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: James K Lewis <jklewis@us.ibm.com>
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;

Re: [PATCH 1/16] Spidernet DMA coalescing

[Andrew Morton]

It worries me when a patch series gets resent a few hours later.

Did anything change?

Re: [PATCH 3/16] Spidernet RX Locking

[Jeff Garzik]

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 <linas@austin.ibm.com>
> Cc: James K Lewis <jklewis@us.ibm.com>
> 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

Re: [PATCH 1/16] Spidernet DMA coalescing

[Christoph Hellwig]

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)

Re: [PATCH 1/16] Spidernet DMA coalescing

[Linas Vepstas]

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:
   <linuxppc-dev@ozlabs.org>: 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

Re: [PATCH 3/16] Spidernet RX Locking

[Linas Vepstas]

On Thu, Dec 07, 2006 at 05:09:20AM -0500, Jeff Garzik wrote:
> 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 <linas@austin.ibm.com>
> >Cc: James K Lewis <jklewis@us.ibm.com>
> >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

Mail forwarding loop (was: Re: [PATCH 1/16] Spidernet DMA coalescing)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

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:
> >
> > 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:
>    <linuxppc-dev@ozlabs.org>: 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/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/16] Spidernet RX Locking

[Benjamin Herrenschmidt]

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.

> > 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.

Re: [PATCH 3/16] Spidernet RX Locking

[Linas Vepstas]

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.
> 
> > 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
> 

Re: [PATCH 1/16] Spidernet DMA coalescing

[Linas Vepstas]

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:
> > 
> > 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