[PATCH] Spidernet - remove ETH_ZLEN check in earlier patch

[PATCH] Spidernet - remove ETH_ZLEN check in earlier patch

[Jim Lewis]

Subject: Spidernet - remove ETH_ZLEN check in earlier patch
From: James K Lewis <jklewis@us.ibm.com>

In an earlier patch, code was added to pad packets that were less that
ETH_ZLEN (60) bytes using the skb_pad function. This has caused hangs
when accessing certain NFS mounted file systems. This patch removes the
check and solves the NFS problem. The driver, with this patch, has been
tested extensively. Please apply.


Signed-off-by: James K Lewis <jklewis@us.ibm.com>


---
 drivers/net/spider_net.c |   17 ++++-------------
 drivers/net/spider_net.h |    2 +-
 2 files changed, 5 insertions(+), 14 deletions(-)

Index: linux-2.6.18/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18.orig/drivers/net/spider_net.c	2006-11-04
13:04:32.000000000 -0600
+++ linux-2.6.18/drivers/net/spider_net.c	2006-11-04 13:10:00.000000000
-0600
@@ -610,20 +610,12 @@ spider_net_prepare_tx_descr(struct spide
 	struct spider_net_descr *descr;
 	dma_addr_t buf;
 	unsigned long flags;
-	int length;
 
-	length = skb->len;
-	if (length < ETH_ZLEN) {
-		if (skb_pad(skb, ETH_ZLEN-length))
-			return 0;
-		length = ETH_ZLEN;
-	}
-
-	buf = pci_map_single(card->pdev, skb->data, length, PCI_DMA_TODEVICE);
+	buf = pci_map_single(card->pdev, skb->data, skb->len,
PCI_DMA_TODEVICE);
 	if (pci_dma_mapping_error(buf)) {
 		if (netif_msg_tx_err(card) && net_ratelimit())
 			pr_err("could not iommu-map packet (%p, %i). "
-				  "Dropping packet\n", skb->data, length);
+				  "Dropping packet\n", skb->data, skb->len);
 		card->spider_stats.tx_iommu_map_error++;
 		return -ENOMEM;
 	}
@@ -633,7 +625,7 @@ spider_net_prepare_tx_descr(struct spide
 	card->tx_chain.head = descr->next;
 
 	descr->buf_addr = buf;
-	descr->buf_size = length;
+	descr->buf_size = skb->len;
 	descr->next_descr_addr = 0;
 	descr->skb = skb;
 	descr->data_status = 0;
@@ -768,8 +760,7 @@ spider_net_release_tx_chain(struct spide
 
 		/* unmap the skb */
 		if (skb) {
-			int len = skb->len < ETH_ZLEN ? ETH_ZLEN : skb->len;
-			pci_unmap_single(card->pdev, buf_addr, len, PCI_DMA_TODEVICE);
+			pci_unmap_single(card->pdev, buf_addr, skb->len, PCI_DMA_TODEVICE);
 			dev_kfree_skb(skb);
 		}
 	}
Index: linux-2.6.18/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18.orig/drivers/net/spider_net.h	2006-11-04
13:03:58.000000000 -0600
+++ linux-2.6.18/drivers/net/spider_net.h	2006-11-04 13:06:11.000000000
-0600
@@ -24,7 +24,7 @@
 #ifndef _SPIDER_NET_H
 #define _SPIDER_NET_H
 
-#define VERSION "1.1 A"
+#define VERSION "1.5 A"
 
 #include "sungem_phy.h"
 

Re: [PATCH] Spidernet - remove ETH_ZLEN check in earlier patch

[Stephen Hemminger]

On Fri, 10 Nov 2006 11:50:46 -0600
Jim Lewis <jim@jklewis.com> wrote:

> Subject: Spidernet - remove ETH_ZLEN check in earlier patch
> From: James K Lewis <jklewis@us.ibm.com>
> 
> In an earlier patch, code was added to pad packets that were less that
> ETH_ZLEN (60) bytes using the skb_pad function. This has caused hangs
> when accessing certain NFS mounted file systems. This patch removes the
> check and solves the NFS problem. The driver, with this patch, has been
> tested extensively. Please apply.
> 
> 
> Signed-off-by: James K Lewis <jklewis@us.ibm.com>

Does the hardware do padding for you? The padding is important for the Ethernet
spec, and for security reasons (random memory leakage).

-- 
Stephen Hemminger <shemminger@osdl.org>

[PATCH] spidernet: fix transmit routine.

[Stephen Hemminger]

Try this patch instead (I don't have the hardware to even build).


Fix spider_net transmit routine:
 1. use skb_padto properly
 2. don't return -ENOMEM. Only valid returns from device transmit
    routine are NETDEV_TX_OK, BUSY, LOCKED

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
---
 drivers/net/spider_net.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/spider_net.c b/drivers/net/spider_net.c
index 418138d..cd7f13c 100644
--- a/drivers/net/spider_net.c
+++ b/drivers/net/spider_net.c
@@ -644,22 +644,18 @@ spider_net_prepare_tx_descr(struct spide
 	struct spider_net_descr *descr;
 	dma_addr_t buf;
 	unsigned long flags;
-	int length;
 
-	length = skb->len;
-	if (length < ETH_ZLEN) {
-		if (skb_pad(skb, ETH_ZLEN-length))
-			return 0;
-		length = ETH_ZLEN;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
 
-	buf = pci_map_single(card->pdev, skb->data, length, PCI_DMA_TODEVICE);
+	buf = pci_map_single(card->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
 	if (pci_dma_mapping_error(buf)) {
 		if (netif_msg_tx_err(card) && net_ratelimit())
 			pr_err("could not iommu-map packet (%p, %i). "
-				  "Dropping packet\n", skb->data, length);
+				  "Dropping packet\n", skb->data, skb->len);
 		card->spider_stats.tx_iommu_map_error++;
-		return -ENOMEM;
+		dev_kfree_skb(skb);
+		return NETDEV_TX_OK;
 	}
 
 	spin_lock_irqsave(&card->tx_chain.lock, flags);
@@ -667,7 +663,7 @@ spider_net_prepare_tx_descr(struct spide
 	card->tx_chain.head = descr->next;
 
 	descr->buf_addr = buf;
-	descr->buf_size = length;
+	descr->buf_size = skb->len;
 	descr->next_descr_addr = 0;
 	descr->skb = skb;
 	descr->data_status = 0;
@@ -690,7 +686,7 @@ spider_net_prepare_tx_descr(struct spide
 	descr->prev->next_descr_addr = descr->bus_addr;
 
 	card->netdev->trans_start = jiffies; /* set netdev watchdog timer */
-	return 0;
+	return NETDEV_TX_OK;
 }
 
 static int
-- 
1.4.1

Re: [PATCH] spidernet: fix transmit routine.

[Stephen Hemminger]

On Fri, 10 Nov 2006 13:10:25 -0600
James K Lewis <jklewis@us.ibm.com> wrote:

> Stephen,
> 
>   I had already tried that previously, the fail still occurred. Note that 
> the skb_pad (or skb_padto) never actually fails. There is just something 
> about calling it that causes the hang. Note too that I believe your patch 
> is incorrect, you would still have to pci_unmap_single the correct length 
> (skb_pad does not actually change skb->len which just seems plain wrong to 
> me).

Yeah, that was a bad choice not to set skb->len to start with. Too late
now given how it might affect other drivers.

>   I checked, and none of our "mainstream" PPC ethernet drivers (e100, 
> e1000, pcnet32, tg3) have this check in them. When I have time I may add 
> the pad to pcnet32 (for example) to see if it fails.

The hardware for all these drivers does padding.


> Jim Lewis
> Advisory Software Engineer
> IBM Linux Technology Center
> 512-838-7754
> 
>