[PATCH 0/6]: powerpc/cell spidernet ethernet patches

[PATCH 0/6]: powerpc/cell spidernet ethernet patches

[Linas Vepstas]

Please apply and forward upstream as appropriate.

Although these patches have not been baking in 
any -mm tree, they have been tested and are 
generally available as a part of the Cell SDK 2.0
overseen by Arnd Bergmann. (Arnd, if you want
to lend a voice of authority here, or to correct 
me, please do so...)

The following sequence of six patches implement a 
series of changes to the transmit side of the 
spidernet ethernet device driver, significantly 
improving performance for large packets.

This series of patches is almost identical to 
those previously mailed on 18-20 August, with one
critical change: NAPI polling is used instead of 
homegrown polling.

Although these patches improve things, I am not
satisfied with how this driver behaves, and so 
plan to do additional work next week. 

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>

--linas

[PATCH 1/6]: powerpc/cell spidernet burst alignment patch.

[Linas Vepstas]

This patch increases the Burst Address alignment from 64 to 1024 in the
Spidernet driver. This improves transmit performance for large packets.

From: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>

----
 drivers/net/spider_net.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h	2006-09-29 14:11:21.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h	2006-09-29 16:33:49.000000000 -0500
@@ -209,7 +209,7 @@ extern char spider_net_driver_name[];
 #define SPIDER_NET_DMA_RX_FEND_VALUE	0x00030003
 /* to set TX_DMA_EN */
 #define SPIDER_NET_TX_DMA_EN		0x80000000
-#define SPIDER_NET_GDTDCEIDIS		0x00000002
+#define SPIDER_NET_GDTDCEIDIS		0x00000302
 #define SPIDER_NET_DMA_TX_VALUE		SPIDER_NET_TX_DMA_EN | \
 					SPIDER_NET_GDTDCEIDIS
 #define SPIDER_NET_DMA_TX_FEND_VALUE	0x00030003

[PATCH 2/6]: powerpc/cell spidernet low watermark patch.

[Linas Vepstas]

Implement basic low-watermark support for the transmit queue.
Hardware low-watermarks allow a properly configured kernel
to continously stream data to a device and not have to handle 
any interrupts at all in doing so. Correct zero-interrupt
operation can be actually observed for this driver, when the 
socket buffer is made large enough.

The basic idea of a low-watermark interrupt is as follows.
The device driver queues up a bunch of packets for the hardware
to transmit, and then kicks the hardware to get it started.
As the hardware drains the queue of pending, untransmitted 
packets, the device driver will want to know when the queue
is almost empty, so that it can queue some more packets.

If the queue drains down to the low waterark, then an interrupt
will be generated. However, if the kernel/driver continues 
to add enough packets to keep the queue partially filled,
no interrupt will actually be generated, and the hardware 
can continue streaming packets indefinitely in this mode.

The impelmentation is done by setting the DESCR_TXDESFLG flag 
in one of the packets. When the hardware sees this flag, it will 
interrupt the device driver. Because this flag is on a fixed
packet, rather than at  fixed location in the queue, the
code below needs to move the flag as more packets are
queued up. This implementation attempts to keep the flag 
at about 1/4 from "empty".

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>

----
 drivers/net/spider_net.c |   56 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/net/spider_net.h |    6 +++--
 2 files changed, 55 insertions(+), 7 deletions(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c	2006-09-29 14:11:20.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c	2006-09-29 16:33:46.000000000 -0500
@@ -703,6 +703,39 @@ spider_net_release_tx_descr(struct spide
 	dev_kfree_skb_any(skb);
 }
 
+static void
+spider_net_set_low_watermark(struct spider_net_card *card)
+{
+	int status;
+	int cnt=0;
+	int i;
+	struct spider_net_descr *descr = card->tx_chain.tail;
+
+	/* Measure the length of the queue. */
+	while (descr != card->tx_chain.head) {
+		status = descr->dmac_cmd_status & SPIDER_NET_DESCR_NOT_IN_USE;
+		if (status == SPIDER_NET_DESCR_NOT_IN_USE)
+			break;
+		descr = descr->next;
+		cnt++;
+	}
+	if (cnt == 0)
+		return;
+
+	/* Set low-watermark 3/4th's of the way into the queue. */
+	descr = card->tx_chain.tail;
+	cnt = (cnt*3)/4;
+	for (i=0;i<cnt; i++)
+		descr = descr->next;
+
+	/* Set the new watermark, clear the old wtermark */
+	descr->dmac_cmd_status |= SPIDER_NET_DESCR_TXDESFLG;
+	if (card->low_watermark && card->low_watermark != descr)
+		card->low_watermark->dmac_cmd_status =
+		     card->low_watermark->dmac_cmd_status & ~SPIDER_NET_DESCR_TXDESFLG;
+	card->low_watermark = descr;
+}
+
 /**
  * spider_net_release_tx_chain - processes sent tx descriptors
  * @card: adapter structure
@@ -720,6 +753,7 @@ spider_net_release_tx_chain(struct spide
 {
 	struct spider_net_descr_chain *chain = &card->tx_chain;
 	int status;
+	int rc=0;
 
 	spider_net_read_reg(card, SPIDER_NET_GDTDMACCNTR);
 
@@ -732,8 +766,10 @@ spider_net_release_tx_chain(struct spide
 			break;
 
 		case SPIDER_NET_DESCR_CARDOWNED:
-			if (!brutal)
-				return 1;
+			if (!brutal) {
+				rc = 1;
+				goto done;
+			}
 			/* fallthrough, if we release the descriptors
 			 * brutally (then we don't care about
 			 * SPIDER_NET_DESCR_CARDOWNED) */
@@ -750,12 +786,15 @@ spider_net_release_tx_chain(struct spide
 
 		default:
 			card->netdev_stats.tx_dropped++;
-			return 1;
+			rc = 1;
+			goto done;
 		}
 		spider_net_release_tx_descr(card);
 	}
-
-	return 0;
+done:
+	if (rc == 1)
+		spider_net_set_low_watermark(card);
+	return rc;
 }
 
 /**
@@ -1460,6 +1499,10 @@ spider_net_interrupt(int irq, void *ptr,
 		spider_net_rx_irq_off(card);
 		netif_rx_schedule(netdev);
 	}
+	if (status_reg & SPIDER_NET_TXINT ) {
+		spider_net_cleanup_tx_ring(card);
+		netif_wake_queue(netdev);
+	}
 
 	if (status_reg & SPIDER_NET_ERRINT )
 		spider_net_handle_error_irq(card, status_reg);
@@ -1621,6 +1664,9 @@ spider_net_open(struct net_device *netde
 	if (spider_net_init_chain(card, &card->tx_chain, card->descr,
 			PCI_DMA_TODEVICE, card->tx_desc))
 		goto alloc_tx_failed;
+
+	card->low_watermark = NULL;
+
 	if (spider_net_init_chain(card, &card->rx_chain,
 			card->descr + card->rx_desc,
 			PCI_DMA_FROMDEVICE, card->rx_desc))
Index: linux-2.6.18-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h	2006-09-29 14:47:50.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h	2006-09-29 16:33:43.000000000 -0500
@@ -47,7 +47,7 @@ extern char spider_net_driver_name[];
 #define SPIDER_NET_TX_DESCRIPTORS_MIN		16
 #define SPIDER_NET_TX_DESCRIPTORS_MAX		512
 
-#define SPIDER_NET_TX_TIMER			20
+#define SPIDER_NET_TX_TIMER			(HZ/5)
 
 #define SPIDER_NET_RX_CSUM_DEFAULT		1
 
@@ -209,7 +209,7 @@ extern char spider_net_driver_name[];
 #define SPIDER_NET_DMA_RX_FEND_VALUE	0x00030003
 /* to set TX_DMA_EN */
 #define SPIDER_NET_TX_DMA_EN		0x80000000
-#define SPIDER_NET_GDTDCEIDIS		0x00000302
+#define SPIDER_NET_GDTDCEIDIS		0x00000300
 #define SPIDER_NET_DMA_TX_VALUE		SPIDER_NET_TX_DMA_EN | \
 					SPIDER_NET_GDTDCEIDIS
 #define SPIDER_NET_DMA_TX_FEND_VALUE	0x00030003
@@ -349,6 +349,7 @@ enum spider_net_int2_status {
 #define SPIDER_NET_DESCR_FORCE_END		0x50000000 /* used in rx and tx */
 #define SPIDER_NET_DESCR_CARDOWNED		0xA0000000 /* used in rx and tx */
 #define SPIDER_NET_DESCR_NOT_IN_USE		0xF0000000
+#define SPIDER_NET_DESCR_TXDESFLG		0x00800000
 
 struct spider_net_descr {
 	/* as defined by the hardware */
@@ -433,6 +434,7 @@ struct spider_net_card {
 
 	struct spider_net_descr_chain tx_chain;
 	struct spider_net_descr_chain rx_chain;
+	struct spider_net_descr *low_watermark;
 
 	struct net_device_stats netdev_stats;
 

[PATCH 3/6]: powerpc/cell spidernet stop error printing patch.

[Linas Vepstas]

Turn off mis-interpretation of the queue-empty interrupt
status bit as an error. This bit is set as a part of 
the previous low-watermark patch.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>

----
 drivers/net/spider_net.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c	2006-09-29 15:01:55.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c	2006-09-29 16:33:43.000000000 -0500
@@ -1282,12 +1282,15 @@ spider_net_handle_error_irq(struct spide
 	case SPIDER_NET_PHYINT:
 	case SPIDER_NET_GMAC2INT:
 	case SPIDER_NET_GMAC1INT:
-	case SPIDER_NET_GIPSINT:
 	case SPIDER_NET_GFIFOINT:
 	case SPIDER_NET_DMACINT:
 	case SPIDER_NET_GSYSINT:
 		break; */
 
+	case SPIDER_NET_GIPSINT:
+		show_error = 0;
+		break;
+
 	case SPIDER_NET_GPWOPCMPINT:
 		/* PHY write operation completed */
 		show_error = 0;
@@ -1346,9 +1349,10 @@ spider_net_handle_error_irq(struct spide
 	case SPIDER_NET_GDTDCEINT:
 		/* chain end. If a descriptor should be sent, kick off
 		 * tx dma
-		if (card->tx_chain.tail == card->tx_chain.head)
+		if (card->tx_chain.tail != card->tx_chain.head)
 			spider_net_kick_tx_dma(card);
-		show_error = 0; */
+		*/
+		show_error = 0;
 		break;
 
 	/* case SPIDER_NET_G1TMCNTINT: not used. print a message */
@@ -1462,8 +1466,9 @@ spider_net_handle_error_irq(struct spide
 	}
 
 	if ((show_error) && (netif_msg_intr(card)))
-		pr_err("Got error interrupt, GHIINT0STS = 0x%08x, "
+		pr_err("Got error interrupt on %s, GHIINT0STS = 0x%08x, "
 		       "GHIINT1STS = 0x%08x, GHIINT2STS = 0x%08x\n",
+		       card->netdev->name,
 		       status_reg, error_reg1, error_reg2);
 
 	/* clear interrupt sources */

[PATCH 4/6]: powerpc/cell spidernet ethtool -i version number info.

[Linas Vepstas]

This patch adds version information as reported by 
ethtool -i to the Spidernet driver.

From: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>

----
 drivers/net/spider_net.c         |    3 +++
 drivers/net/spider_net.h         |    2 ++
 drivers/net/spider_net_ethtool.c |    2 +-
 3 files changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c	2006-09-29 15:05:05.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c	2006-09-29 16:33:39.000000000 -0500
@@ -55,6 +55,7 @@ MODULE_AUTHOR("Utz Bacher <utz.bacher@de
 	      "<Jens.Osterkamp@de.ibm.com>");
 MODULE_DESCRIPTION("Spider Southbridge Gigabit Ethernet driver");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(VERSION);
 
 static int rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_DEFAULT;
 static int tx_descriptors = SPIDER_NET_TX_DESCRIPTORS_DEFAULT;
@@ -2303,6 +2304,8 @@ static struct pci_driver spider_net_driv
  */
 static int __init spider_net_init(void)
 {
+	printk("spidernet Version %s.\n",VERSION);
+
 	if (rx_descriptors < SPIDER_NET_RX_DESCRIPTORS_MIN) {
 		rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_MIN;
 		pr_info("adjusting rx descriptors to %i.\n", rx_descriptors);
Index: linux-2.6.18-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h	2006-09-29 15:01:55.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h	2006-09-29 15:18:12.000000000 -0500
@@ -24,6 +24,8 @@
 #ifndef _SPIDER_NET_H
 #define _SPIDER_NET_H
 
+#define VERSION "1.1 A"
+
 #include "sungem_phy.h"
 
 extern int spider_net_stop(struct net_device *netdev);
Index: linux-2.6.18-mm2/drivers/net/spider_net_ethtool.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net_ethtool.c	2006-09-29 14:11:18.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net_ethtool.c	2006-09-29 15:18:12.000000000 -0500
@@ -76,7 +76,7 @@ spider_net_ethtool_get_drvinfo(struct ne
 	/* clear and fill out info */
 	memset(drvinfo, 0, sizeof(struct ethtool_drvinfo));
 	strncpy(drvinfo->driver, spider_net_driver_name, 32);
-	strncpy(drvinfo->version, "0.1", 32);
+	strncpy(drvinfo->version, VERSION, 32);
 	strcpy(drvinfo->fw_version, "no information");
 	strncpy(drvinfo->bus_info, pci_name(card->pdev), 32);
 }

[PATCH 5/6]: powerpc/cell spidernet ethtool -i version number

[Linas Vepstas]

Jim, as the official maintainer, you should explicitly ack this patch.
--linas

This patch moves transmit queue cleanup code out of the 
interrupt context, and into the NAPI polling routine.

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 |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c	2006-09-29 17:01:39.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c	2006-09-29 17:39:06.000000000 -0500
@@ -887,9 +887,10 @@ out:
  * spider_net_cleanup_tx_ring - cleans up the TX ring
  * @card: card structure
  *
- * spider_net_cleanup_tx_ring is called by the tx_timer (as we don't use
- * interrupts to cleanup our TX ring) and returns sent packets to the stack
- * by freeing them
+ * spider_net_cleanup_tx_ring is called by either the tx_timer
+ * or from the NAPI polling routine.
+ * This routine releases resources associted with transmitted
+ * packets, including updating the queue tail pointer.
  */
 static void
 spider_net_cleanup_tx_ring(struct spider_net_card *card)
@@ -1093,6 +1094,7 @@ spider_net_poll(struct net_device *netde
 	int packets_to_do, packets_done = 0;
 	int no_more_packets = 0;
 
+	spider_net_cleanup_tx_ring(card);
 	packets_to_do = min(*budget, netdev->quota);
 
 	while (packets_to_do) {
@@ -1505,10 +1507,8 @@ spider_net_interrupt(int irq, void *ptr,
 		spider_net_rx_irq_off(card);
 		netif_rx_schedule(netdev);
 	}
-	if (status_reg & SPIDER_NET_TXINT ) {
-		spider_net_cleanup_tx_ring(card);
-		netif_wake_queue(netdev);
-	}
+	if (status_reg & SPIDER_NET_TXINT)
+		netif_rx_schedule(netdev);
 
 	if (status_reg & SPIDER_NET_ERRINT )
 		spider_net_handle_error_irq(card, status_reg);

[PATCH 6/6]: powerpc/cell spidernet refine locking

[Linas Vepstas]

The transmit side of the spider ethernet driver currently
places locks around some very large chunks of code. This
results in a fair amount of lock contention is some cases. 
This patch makes the locks much more fine-grained, protecting
only the cirtical sections. One lock is used to protect 
three locations: the queue head and tail pointers, and the 
queue low-watermark location.


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 |   77 ++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 43 deletions(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c	2006-09-29 17:39:06.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c	2006-09-29 17:41:19.000000000 -0500
@@ -646,8 +646,9 @@ static int
 spider_net_prepare_tx_descr(struct spider_net_card *card,
 			    struct sk_buff *skb)
 {
-	struct spider_net_descr *descr = card->tx_chain.head;
+	struct spider_net_descr *descr;
 	dma_addr_t buf;
+	unsigned long flags;
 
 	buf = pci_map_single(card->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
 	if (pci_dma_mapping_error(buf)) {
@@ -658,6 +659,10 @@ spider_net_prepare_tx_descr(struct spide
 		return -ENOMEM;
 	}
 
+	spin_lock_irqsave(&card->tx_chain.lock, flags);
+	descr = card->tx_chain.head;
+	card->tx_chain.head = card->tx_chain.head->next;
+
 	descr->buf_addr = buf;
 	descr->buf_size = skb->len;
 	descr->next_descr_addr = 0;
@@ -666,6 +671,8 @@ spider_net_prepare_tx_descr(struct spide
 
 	descr->dmac_cmd_status =
 			SPIDER_NET_DESCR_CARDOWNED | SPIDER_NET_DMAC_NOCS;
+	spin_unlock_irqrestore(&card->tx_chain.lock, flags);
+
 	if (skb->protocol == htons(ETH_P_IP))
 		switch (skb->nh.iph->protocol) {
 		case IPPROTO_TCP:
@@ -676,37 +683,16 @@ spider_net_prepare_tx_descr(struct spide
 			break;
 		}
 
+	/* Chain the bus address, so that the DMA engine finds this descr. */
 	descr->prev->next_descr_addr = descr->bus_addr;
 
 	return 0;
 }
 
-/**
- * spider_net_release_tx_descr - processes a used tx descriptor
- * @card: card structure
- * @descr: descriptor to release
- *
- * releases a used tx descriptor (unmapping, freeing of skb)
- */
-static inline void
-spider_net_release_tx_descr(struct spider_net_card *card)
-{
-	struct spider_net_descr *descr = card->tx_chain.tail;
-	struct sk_buff *skb;
-
-	card->tx_chain.tail = card->tx_chain.tail->next;
-	descr->dmac_cmd_status |= SPIDER_NET_DESCR_NOT_IN_USE;
-
-	/* unmap the skb */
-	skb = descr->skb;
-	pci_unmap_single(card->pdev, descr->buf_addr, skb->len,
-			PCI_DMA_TODEVICE);
-	dev_kfree_skb_any(skb);
-}
-
 static void
 spider_net_set_low_watermark(struct spider_net_card *card)
 {
+	unsigned long flags;
 	int status;
 	int cnt=0;
 	int i;
@@ -730,11 +716,13 @@ spider_net_set_low_watermark(struct spid
 		descr = descr->next;
 
 	/* Set the new watermark, clear the old wtermark */
+	spin_lock_irqsave(&card->tx_chain.lock, flags);
 	descr->dmac_cmd_status |= SPIDER_NET_DESCR_TXDESFLG;
 	if (card->low_watermark && card->low_watermark != descr)
 		card->low_watermark->dmac_cmd_status =
 		     card->low_watermark->dmac_cmd_status & ~SPIDER_NET_DESCR_TXDESFLG;
 	card->low_watermark = descr;
+	spin_unlock_irqrestore(&card->tx_chain.lock, flags);
 }
 
 /**
@@ -753,22 +741,30 @@ static int
 spider_net_release_tx_chain(struct spider_net_card *card, int brutal)
 {
 	struct spider_net_descr_chain *chain = &card->tx_chain;
+	struct spider_net_descr *descr;
+	struct sk_buff *skb;
+	u32 buf_addr;
+	unsigned long flags;
 	int status;
 	int rc=0;
 
 	spider_net_read_reg(card, SPIDER_NET_GDTDMACCNTR);
 
 	while (chain->tail != chain->head) {
-		status = spider_net_get_descr_status(chain->tail);
+		spin_lock_irqsave(&chain->lock, flags);
+		descr = chain->tail;
+
+		status = spider_net_get_descr_status(descr);
 		switch (status) {
 		case SPIDER_NET_DESCR_COMPLETE:
 			card->netdev_stats.tx_packets++;
-			card->netdev_stats.tx_bytes += chain->tail->skb->len;
+			card->netdev_stats.tx_bytes += descr->skb->len;
 			break;
 
 		case SPIDER_NET_DESCR_CARDOWNED:
 			if (!brutal) {
 				rc = 1;
+				spin_unlock_irqrestore(&chain->lock, flags);
 				goto done;
 			}
 			/* fallthrough, if we release the descriptors
@@ -788,9 +784,19 @@ spider_net_release_tx_chain(struct spide
 		default:
 			card->netdev_stats.tx_dropped++;
 			rc = 1;
+			spin_unlock_irqrestore(&chain->lock, flags);
 			goto done;
 		}
-		spider_net_release_tx_descr(card);
+
+		chain->tail = chain->tail->next;
+		descr->dmac_cmd_status |= SPIDER_NET_DESCR_NOT_IN_USE;
+		skb = descr->skb;
+		buf_addr = descr->buf_addr;
+		spin_unlock_irqrestore(&chain->lock, flags);
+
+		/* unmap the skb */
+		pci_unmap_single(card->pdev, buf_addr, skb->len, PCI_DMA_TODEVICE);
+		dev_kfree_skb_any(skb);
 	}
 done:
 	if (rc == 1)
@@ -847,11 +853,8 @@ spider_net_xmit(struct sk_buff *skb, str
 	struct spider_net_card *card = netdev_priv(netdev);
 	struct spider_net_descr_chain *chain = &card->tx_chain;
 	struct spider_net_descr *descr = chain->head;
-	unsigned long flags;
 	int result;
 
-	spin_lock_irqsave(&chain->lock, flags);
-
 	spider_net_release_tx_chain(card, 0);
 
 	if (chain->head->next == chain->tail->prev) {
@@ -873,12 +876,9 @@ spider_net_xmit(struct sk_buff *skb, str
 	}
 
 	result = NETDEV_TX_OK;
-
 	spider_net_kick_tx_dma(card);
-	card->tx_chain.head = card->tx_chain.head->next;
 
 out:
-	spin_unlock_irqrestore(&chain->lock, flags);
 	netif_wake_queue(netdev);
 	return result;
 }
@@ -895,15 +895,9 @@ out:
 static void
 spider_net_cleanup_tx_ring(struct spider_net_card *card)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&card->tx_chain.lock, flags);
-
 	if ((spider_net_release_tx_chain(card, 0) != 0) &&
 	    (card->netdev->flags & IFF_UP))
 		spider_net_kick_tx_dma(card);
-
-	spin_unlock_irqrestore(&card->tx_chain.lock, flags);
 }
 
 /**
@@ -1930,10 +1924,7 @@ spider_net_stop(struct net_device *netde
 	spider_net_disable_rxdmac(card);
 
 	/* release chains */
-	if (spin_trylock(&card->tx_chain.lock)) {
-		spider_net_release_tx_chain(card, 1);
-		spin_unlock(&card->tx_chain.lock);
-	}
+	spider_net_release_tx_chain(card, 1);
 
 	spider_net_free_chain(card, &card->tx_chain);
 	spider_net_free_chain(card, &card->rx_chain);

Re: [PATCH 6/6]: powerpc/cell spidernet refine locking

[Andrew Morton]

On Fri, 29 Sep 2006 18:29:11 -0500
linas@austin.ibm.com (Linas Vepstas) wrote:

> The transmit side of the spider ethernet driver currently
> places locks around some very large chunks of code. This
> results in a fair amount of lock contention is some cases. 
> This patch makes the locks much more fine-grained, protecting
> only the cirtical sections. One lock is used to protect 
> three locations: the queue head and tail pointers, and the 
> queue low-watermark location.

You have spider_net_set_low_watermark() walking the tx_chain outside
tx_chain.lock.  Are you sure about that?

Re: [PATCH 1/6]: powerpc/cell spidernet burst alignment patch.

[Arnd Bergmann]

Am Saturday 30 September 2006 01:15 schrieb Linas Vepstas:
> This patch increases the Burst Address alignment from 64 to 1024 in the
> Spidernet driver. This improves transmit performance for large packets.
>
> From: James K Lewis <jklewis@us.ibm.com>
> Signed-off-by: James K Lewis <jklewis@us.ibm.com>
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 2/6]: powerpc/cell spidernet low watermark patch.

[Arnd Bergmann]

Am Saturday 30 September 2006 01:17 schrieb Linas Vepstas:
> Implement basic low-watermark support for the transmit queue.
> Hardware low-watermarks allow a properly configured kernel
> to continously stream data to a device and not have to handle
> any interrupts at all in doing so. Correct zero-interrupt
> operation can be actually observed for this driver, when the
> socket buffer is made large enough.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 3/6]: powerpc/cell spidernet stop error printing patch.

[Arnd Bergmann]

Am Saturday 30 September 2006 01:19 schrieb Linas Vepstas:
> Turn off mis-interpretation of the queue-empty interrupt
> status bit as an error. This bit is set as a part of
> the previous low-watermark patch.
>
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> Signed-off-by: James K Lewis <jklewis@us.ibm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 4/6]: powerpc/cell spidernet ethtool -i version number info.

[Arnd Bergmann]

Am Saturday 30 September 2006 01:21 schrieb Linas Vepstas:
> This patch adds version information as reported by
> ethtool -i to the Spidernet driver.
>
> From: James K Lewis <jklewis@us.ibm.com>
> Signed-off-by: James K Lewis <jklewis@us.ibm.com>
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>

Same comment as last time this was submitted:

> @@ -2303,6 +2304,8 @@ static struct pci_driver spider_net_driv
>   */
>  static int __init spider_net_init(void)
>  {
> +       printk("spidernet Version %s.\n",VERSION);
> +
>         if (rx_descriptors < SPIDER_NET_RX_DESCRIPTORS_MIN) {
>                 rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_MIN;
>                 pr_info("adjusting rx descriptors to %i.\n",

This is missing a printk level and a space character. More importantly,
you should not print the presence of the driver to the syslog, but
the presence of a device driven by it.

	Arnd <><

Re: [PATCH 5/6]: powerpc/cell spidernet ethtool -i version number

[Arnd Bergmann]

Am Saturday 30 September 2006 01:26 schrieb Linas Vepstas:
> This patch moves transmit queue cleanup code out of the
> interrupt context, and into the NAPI polling routine.
>
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> Cc: James K Lewis <jklewis@us.ibm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>

The subject of this mail should be "spidernet: use NAPI poll for
TX interrupts", otherwise

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 0/6]: powerpc/cell spidernet ethernet patches

[Arnd Bergmann]

Am Saturday 30 September 2006 01:05 schrieb Linas Vepstas:
> Although these patches have not been baking in
> any -mm tree, they have been tested and are
> generally available as a part of the Cell SDK 2.0
> overseen by Arnd Bergmann. (Arnd, if you want
> to lend a voice of authority here, or to correct
> me, please do so...)
>
> The following sequence of six patches implement a
> series of changes to the transmit side of the
> spidernet ethernet device driver, significantly
> improving performance for large packets.
>
> This series of patches is almost identical to
> those previously mailed on 18-20 August, with one
> critical change: NAPI polling is used instead of
> homegrown polling.
>
> Although these patches improve things, I am not
> satisfied with how this driver behaves, and so
> plan to do additional work next week.
>

I'm not sure if I have missed a patch in here, but I
don't see anything reintroducing the 'netif_stop_queue'
that is missing from the transmit path.

Do you have a extra patch for that?

	Arnd <><

Re: [PATCH 0/6]: powerpc/cell spidernet ethernet patches

[Linas Vepstas]

On Sat, Sep 30, 2006 at 12:40:00PM +0200, Arnd Bergmann wrote:
> Am Saturday 30 September 2006 01:05 schrieb Linas Vepstas:
> >
> > Although these patches improve things, I am not
> > satisfied with how this driver behaves, and so
> > plan to do additional work next week.
> >
> 
> I'm not sure if I have missed a patch in here, but I
> don't see anything reintroducing the 'netif_stop_queue'
> that is missing from the transmit path.
> 
> Do you have a extra patch for that?

Unfinished.  There are several ways in which the current 
spider-net driver doesn't do things the way Greg KH's, etal 
book on device drivers recommends. I was planning on combing 
through these this week.

--linas

Re: [PATCH 0/6]: powerpc/cell spidernet ethernet patches

[Arnd Bergmann]

On Monday 02 October 2006 18:27, Linas Vepstas wrote:
> > 
> > I'm not sure if I have missed a patch in here, but I
> > don't see anything reintroducing the 'netif_stop_queue'
> > that is missing from the transmit path.
> > 
> > Do you have a extra patch for that?
> 
> Unfinished.  There are several ways in which the current 
> spider-net driver doesn't do things the way Greg KH's, etal 
> book on device drivers recommends. I was planning on combing 
> through these this week.

Ok, that's good. However, removing the netif_stop_queue
was an obvious oversight that happened during the cleanup
last year.

Putting that one line back in should be a really safe fix for
the problem of overly high system load we sometimes see.

	Arnd <><

Re: [PATCH 4/6]: powerpc/cell spidernet ethtool -i version number info.

[Linas Vepstas]

On Sat, Sep 30, 2006 at 12:33:45PM +0200, Arnd Bergmann wrote:
> Am Saturday 30 September 2006 01:21 schrieb Linas Vepstas:
> >
> >  static int __init spider_net_init(void)
> >  {
> > +       printk("spidernet Version %s.\n",VERSION);
> > +
> 
> This is missing a printk level and a space character. 

Revised patch below.

> More importantly,
> you should not print the presence of the driver to the syslog, but
> the presence of a device driven by it.

Most device drivers seem to print version numbers in the module_init 
routines (i.e. right before calling pci_register_driver), rather than 
in the probe routines; so this patch follows this convention.

--linas

This patch adds version information as reported by
ethtool -i to the Spidernet driver.

From: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: James K Lewis <jklewis@us.ibm.com>
Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>


----
 drivers/net/spider_net.c         |    3 +++
 drivers/net/spider_net.h         |    2 ++
 drivers/net/spider_net_ethtool.c |    2 +-
 3 files changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c	2006-09-29 16:44:17.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c	2006-10-02 12:00:15.000000000 -0500
@@ -55,6 +55,7 @@ MODULE_AUTHOR("Utz Bacher <utz.bacher@de
 	      "<Jens.Osterkamp@de.ibm.com>");
 MODULE_DESCRIPTION("Spider Southbridge Gigabit Ethernet driver");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(VERSION);
 
 static int rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_DEFAULT;
 static int tx_descriptors = SPIDER_NET_TX_DESCRIPTORS_DEFAULT;
@@ -2303,6 +2304,8 @@ static struct pci_driver spider_net_driv
  */
 static int __init spider_net_init(void)
 {
+	printk(KERN_INFO "Spidernet version %s.\n", VERSION);
+
 	if (rx_descriptors < SPIDER_NET_RX_DESCRIPTORS_MIN) {
 		rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_MIN;
 		pr_info("adjusting rx descriptors to %i.\n", rx_descriptors);
Index: linux-2.6.18-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h	2006-09-29 16:44:17.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h	2006-10-02 11:57:32.000000000 -0500
@@ -24,6 +24,8 @@
 #ifndef _SPIDER_NET_H
 #define _SPIDER_NET_H
 
+#define VERSION "1.1 A"
+
 #include "sungem_phy.h"
 
 extern int spider_net_stop(struct net_device *netdev);
Index: linux-2.6.18-mm2/drivers/net/spider_net_ethtool.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net_ethtool.c	2006-09-29 16:33:43.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net_ethtool.c	2006-09-29 16:44:18.000000000 -0500
@@ -76,7 +76,7 @@ spider_net_ethtool_get_drvinfo(struct ne
 	/* clear and fill out info */
 	memset(drvinfo, 0, sizeof(struct ethtool_drvinfo));
 	strncpy(drvinfo->driver, spider_net_driver_name, 32);
-	strncpy(drvinfo->version, "0.1", 32);
+	strncpy(drvinfo->version, VERSION, 32);
 	strcpy(drvinfo->fw_version, "no information");
 	strncpy(drvinfo->bus_info, pci_name(card->pdev), 32);
 }

Re: [PATCH 0/6]: powerpc/cell spidernet ethernet patches

[Linas Vepstas]

On Mon, Oct 02, 2006 at 06:50:39PM +0200, Arnd Bergmann wrote:
> On Monday 02 October 2006 18:27, Linas Vepstas wrote:
> > > 
> > > I'm not sure if I have missed a patch in here, but I
> > > don't see anything reintroducing the 'netif_stop_queue'
> > > that is missing from the transmit path.
> > > 
> > > Do you have a extra patch for that?
> > 
> > Unfinished.  There are several ways in which the current 
> > spider-net driver doesn't do things the way Greg KH's, etal 
> > book on device drivers recommends. I was planning on combing 
> > through these this week.
> 
> Ok, that's good. However, removing the netif_stop_queue
> was an obvious oversight that happened during the cleanup
> last year.
> 
> Putting that one line back in should be a really safe fix for
> the problem of overly high system load we sometimes see.

Hmm. I have a patch from 5 weeks ago that seems to insert a bunch 
of these. I'm not sure why it hadn't been mailed before, I'll 
test and post as soon as I can.

--linas

Re: [PATCH 6/6]: powerpc/cell spidernet refine locking

[Linas Vepstas]

On Fri, Sep 29, 2006 at 07:47:52PM -0700, Andrew Morton wrote:
> On Fri, 29 Sep 2006 18:29:11 -0500
> linas@austin.ibm.com (Linas Vepstas) wrote:
> 
> > The transmit side of the spider ethernet driver currently
> > places locks around some very large chunks of code. This
> > results in a fair amount of lock contention is some cases. 
> > This patch makes the locks much more fine-grained, protecting
> > only the cirtical sections. One lock is used to protect 
> > three locations: the queue head and tail pointers, and the 
> > queue low-watermark location.
> 
> You have spider_net_set_low_watermark() walking the tx_chain outside
> tx_chain.lock.  Are you sure about that?

Yes. Its making an approximate count of the queue length, and I figured
that if its approximate to begin with, an unlocked version should be
just fine.

--linas