[PATCH 0/9] sky2 driver patches

[PATCH 0/9] sky2 driver patches

[Stephen Hemminger]

The patches are ordered in importance.  The first six should go
into 2.6.31, the rest can wait for 2.6.32.


-- 

[PATCH 1/9] sky2: turn off pause during shutdown

[Stephen Hemminger]

[-- Attachment #1: sky2-gmac-shut.patch --]
[-- Type: text/plain, Size: 564 bytes --]

This unblocks the chip if it is stuck in pause cycle during
shutdown.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2009-06-17 10:29:17.292748274 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:50.630937431 -0700
@@ -1808,7 +1808,8 @@ static int sky2_down(struct net_device *
 
 	synchronize_irq(hw->pdev->irq);
 
-	sky2_gmac_reset(hw, port);
+	/* Force flow control off */
+	sky2_write8(hw, SK_REG(port, GMAC_CTRL), GMC_PAUSE_OFF);
 
 	/* Stop transmitter */
 	sky2_write32(hw, Q_ADDR(txqaddr[port], Q_CSR), BMU_STOP);

-- 

[PATCH 2/9] sky2: more receive shutdown

[Stephen Hemminger]

[-- Attachment #1: sky2-shut1.patch --]
[-- Type: text/plain, Size: 704 bytes --]

Reset more parts of the receive path when device is take offline.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2009-06-17 10:29:50.630937431 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:52.005685817 -0700
@@ -1151,7 +1151,14 @@ stopped:
 
 	/* reset the Rx prefetch unit */
 	sky2_write32(hw, Y2_QADDR(rxq, PREF_UNIT_CTRL), PREF_UNIT_RST_SET);
-	mmiowb();
+
+	/* Reset the RAM Buffer receive queue */
+	sky2_write8(hw, RB_ADDR(rxq, RB_CTRL), RB_RST_SET);
+
+	/* Reset Rx MAC FIFO */
+	sky2_write8(hw, SK_REG(sky2->port, RX_GMF_CTRL_T), GMF_RST_SET);
+
+	sky2_read8(hw, B0_CTST);
 }
 
 /* Clean out receive buffer area, assumes receiver hardware stopped */

-- 

[PATCH 3/9] sky2: PCI irq issues

[Stephen Hemminger]

[-- Attachment #1: sky2-irqpost.patch --]
[-- Type: text/plain, Size: 715 bytes --]

Add some read's to avoid any PCI posting issues when controlling
irq's.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2009-06-17 10:29:52.005685817 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:53.382997969 -0700
@@ -1495,6 +1495,7 @@ static int sky2_up(struct net_device *de
 	imask = sky2_read32(hw, B0_IMSK);
 	imask |= portirq_msk[port];
 	sky2_write32(hw, B0_IMSK, imask);
+	sky2_read32(hw, B0_IMSK);
 
 	sky2_set_multicast(dev);
 
@@ -1812,6 +1813,7 @@ static int sky2_down(struct net_device *
 	imask = sky2_read32(hw, B0_IMSK);
 	imask &= ~portirq_msk[port];
 	sky2_write32(hw, B0_IMSK, imask);
+	sky2_read32(hw, B0_IMSK);
 
 	synchronize_irq(hw->pdev->irq);
 

-- 

[PATCH 4/9] sky2: fix shutdown synchronization

[Stephen Hemminger]

[-- Attachment #1: sky2-rxsync-shut.patch --]
[-- Type: text/plain, Size: 1586 bytes --]

The logic in sky2_down was incorrect. Receiver could report status
after rx_stop was called.

The steps need to be:
   * stop new frames from being transmitted
   * shut off transmit/receive logic
   * synchronize with NAPI to process status info about transmitter
     and receiver

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2009-06-17 10:29:53.382997969 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:55.361810536 -0700
@@ -1815,8 +1815,6 @@ static int sky2_down(struct net_device *
 	sky2_write32(hw, B0_IMSK, imask);
 	sky2_read32(hw, B0_IMSK);
 
-	synchronize_irq(hw->pdev->irq);
-
 	/* Force flow control off */
 	sky2_write8(hw, SK_REG(port, GMAC_CTRL), GMC_PAUSE_OFF);
 
@@ -1831,9 +1829,6 @@ static int sky2_down(struct net_device *
 	ctrl &= ~(GM_GPCR_TX_ENA | GM_GPCR_RX_ENA);
 	gma_write16(hw, port, GM_GP_CTRL, ctrl);
 
-	/* Make sure no packets are pending */
-	napi_synchronize(&hw->napi);
-
 	sky2_write8(hw, SK_REG(port, GPHY_CTRL), GPC_RST_SET);
 
 	/* Workaround shared GMAC reset */
@@ -1864,6 +1859,15 @@ static int sky2_down(struct net_device *
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
 	sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);
 
+	/* Force any delayed status interrrupt and NAPI */
+	sky2_write32(hw, STAT_LEV_TIMER_CNT, 0);
+	sky2_write32(hw, STAT_TX_TIMER_CNT, 0);
+	sky2_write32(hw, STAT_ISR_TIMER_CNT, 0);
+	sky2_read8(hw, STAT_ISR_TIMER_CTRL);
+
+	synchronize_irq(hw->pdev->irq);
+	napi_synchronize(&hw->napi);
+
 	sky2_phy_power_down(hw, port);
 
 	/* turn off LED's */

-- 

[PATCH 5/9] sky2: receive counter update

[Stephen Hemminger]

[-- Attachment #1: sky2-rxupdate.patch --]
[-- Type: text/plain, Size: 1987 bytes --]

Since it is likely that there are multiple packets received per
interrupt, only update the receive counters once after all
packets are processed.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2009-06-17 10:29:55.361810536 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:56.510685400 -0700
@@ -2357,11 +2357,25 @@ static inline void sky2_tx_done(struct n
 	}
 }
 
+static inline void sky2_rx_done(struct sky2_hw *hw, unsigned port,
+				unsigned packets, unsigned bytes)
+{
+	if (packets) {
+		struct net_device *dev = hw->dev[port];
+
+		dev->stats.rx_packets += packets;
+		dev->stats.rx_bytes += bytes;
+		dev->last_rx = jiffies;
+		sky2_rx_update(netdev_priv(dev), rxqaddr[port]);
+	}
+}
+
 /* Process status response ring */
 static int sky2_status_intr(struct sky2_hw *hw, int to_do, u16 idx)
 {
 	int work_done = 0;
-	unsigned rx[2] = { 0, 0 };
+	unsigned int total_bytes[2] = { 0 };
+	unsigned int total_packets[2] = { 0 };
 
 	rmb();
 	do {
@@ -2388,7 +2402,8 @@ static int sky2_status_intr(struct sky2_
 		le->opcode = 0;
 		switch (opcode & ~HW_OWNER) {
 		case OP_RXSTAT:
-			++rx[port];
+			total_packets[port]++;
+			total_bytes[port] += length;
 			skb = sky2_receive(dev, length, status);
 			if (unlikely(!skb)) {
 				dev->stats.rx_dropped++;
@@ -2406,9 +2421,6 @@ static int sky2_status_intr(struct sky2_
 			}
 
 			skb->protocol = eth_type_trans(skb, dev);
-			dev->stats.rx_packets++;
-			dev->stats.rx_bytes += skb->len;
-			dev->last_rx = jiffies;
 
 #ifdef SKY2_VLAN_TAG_USED
 			if (sky2->vlgrp && (status & GMR_FS_VLAN)) {
@@ -2487,11 +2499,8 @@ static int sky2_status_intr(struct sky2_
 	sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
 
 exit_loop:
-	if (rx[0])
-		sky2_rx_update(netdev_priv(hw->dev[0]), Q_R1);
-
-	if (rx[1])
-		sky2_rx_update(netdev_priv(hw->dev[1]), Q_R2);
+	sky2_rx_done(hw, 0, total_packets[0], total_bytes[0]);
+	sky2_rx_done(hw, 1, total_packets[1], total_bytes[1]);
 
 	return work_done;
 }

-- 

[PATCH 6/9] sky2: reduce default transmit ring

[Stephen Hemminger]

[-- Attachment #1: sky2-txring.patch --]
[-- Type: text/plain, Size: 809 bytes --]

Reduce the size of the driver transmit ring to reduce latency
and allow qdisc to do better rate control.  Also make it
obvious what the minimum transmit ring allowed is and why.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2009-06-17 10:29:56.510685400 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:57.400008862 -0700
@@ -65,9 +65,9 @@
 #define RX_DEF_PENDING		RX_MAX_PENDING
 
 #define TX_RING_SIZE		512
-#define TX_DEF_PENDING		(TX_RING_SIZE - 1)
-#define TX_MIN_PENDING		64
+#define TX_DEF_PENDING		128
 #define MAX_SKB_TX_LE		(4 + (sizeof(dma_addr_t)/sizeof(u32))*MAX_SKB_FRAGS)
+#define TX_MIN_PENDING		(MAX_SKB_TX_LE+1)
 
 #define STATUS_RING_SIZE	2048	/* 2 ports * (TX + 2*RX) */
 #define STATUS_LE_BYTES		(STATUS_RING_SIZE*sizeof(struct sky2_status_le))

-- 

[PATCH 7/9] sky2: skb recycling

[Stephen Hemminger]

[-- Attachment #1: sky2-recycle.patch --]
[-- Type: text/plain, Size: 3026 bytes --]

This patch implements skb recycling. It reclaims transmitted skb's
for use in the receive ring.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2009-06-17 10:29:57.400008862 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:58.250810500 -0700
@@ -1176,6 +1176,7 @@ static void sky2_rx_clean(struct sky2_po
 			re->skb = NULL;
 		}
 	}
+	skb_queue_purge(&sky2->rx_recycle);
 }
 
 /* Basic MII support */
@@ -1252,6 +1253,12 @@ static void sky2_vlan_rx_register(struct
 }
 #endif
 
+/* Amount of required worst case padding in rx buffer */
+static inline unsigned sky2_rx_pad(const struct sky2_hw *hw)
+{
+	return (hw->flags & SKY2_HW_RAM_BUFFER) ? 8 : 2;
+}
+
 /*
  * Allocate an skb for receiving. If the MTU is large enough
  * make the skb non-linear with a fragment list of pages.
@@ -1261,6 +1268,13 @@ static struct sk_buff *sky2_rx_alloc(str
 	struct sk_buff *skb;
 	int i;
 
+	skb = __skb_dequeue(&sky2->rx_recycle);
+	if (!skb)
+		skb = netdev_alloc_skb(sky2->netdev, sky2->rx_data_size
+				       + sky2_rx_pad(sky2->hw));
+	if (!skb)
+		goto nomem;
+
 	if (sky2->hw->flags & SKY2_HW_RAM_BUFFER) {
 		unsigned char *start;
 		/*
@@ -1269,18 +1283,10 @@ static struct sk_buff *sky2_rx_alloc(str
 		 * The buffer returned from netdev_alloc_skb is
 		 * aligned except if slab debugging is enabled.
 		 */
-		skb = netdev_alloc_skb(sky2->netdev, sky2->rx_data_size + 8);
-		if (!skb)
-			goto nomem;
 		start = PTR_ALIGN(skb->data, 8);
 		skb_reserve(skb, start - skb->data);
-	} else {
-		skb = netdev_alloc_skb(sky2->netdev,
-				       sky2->rx_data_size + NET_IP_ALIGN);
-		if (!skb)
-			goto nomem;
+	} else
 		skb_reserve(skb, NET_IP_ALIGN);
-	}
 
 	for (i = 0; i < sky2->rx_nfrags; i++) {
 		struct page *page = alloc_page(GFP_ATOMIC);
@@ -1357,6 +1363,8 @@ static int sky2_rx_start(struct sky2_por
 
 	sky2->rx_data_size = size;
 
+	skb_queue_head_init(&sky2->rx_recycle);
+
 	/* Fill Rx ring */
 	for (i = 0; i < sky2->rx_pending; i++) {
 		re = sky2->rx_ring + i;
@@ -1764,14 +1772,22 @@ static void sky2_tx_complete(struct sky2
 		}
 
 		if (le->ctrl & EOP) {
+			struct sk_buff *skb = re->skb;
+
 			if (unlikely(netif_msg_tx_done(sky2)))
 				printk(KERN_DEBUG "%s: tx done %u\n",
 				       dev->name, idx);
 
 			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += re->skb->len;
+			dev->stats.tx_bytes += skb->len;
+
+			if (skb_queue_len(&sky2->rx_recycle) < sky2->rx_pending
+			    && skb_recycle_check(skb, sky2->rx_data_size
+						 + sky2_rx_pad(sky2->hw)))
+				__skb_queue_head(&sky2->rx_recycle, skb);
+			else
+				dev_kfree_skb_any(skb);
 
-			dev_kfree_skb_any(re->skb);
 			sky2->tx_next = RING_NEXT(idx, TX_RING_SIZE);
 		}
 	}
--- a/drivers/net/sky2.h	2009-06-17 10:29:17.200758602 -0700
+++ b/drivers/net/sky2.h	2009-06-17 10:29:58.252811835 -0700
@@ -2028,6 +2028,7 @@ struct sky2_port {
 	u16		     rx_pending;
 	u16		     rx_data_size;
 	u16		     rx_nfrags;
+	struct sk_buff_head  rx_recycle;
 
 #ifdef SKY2_VLAN_TAG_USED
 	u16		     rx_tag;

-- 

[PATCH 8/9] sky2: add GRO support

[Stephen Hemminger]

[-- Attachment #1: sky2-gro.patch --]
[-- Type: text/plain, Size: 1407 bytes --]

Add support for generic receive offload.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2009-06-17 10:29:58.250810500 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:29:59.249757381 -0700
@@ -2373,6 +2373,26 @@ static inline void sky2_tx_done(struct n
 	}
 }
 
+static inline void sky2_skb_rx(const struct sky2_port *sky2,
+ 			       u32 status, struct sk_buff *skb)
+{
+#ifdef SKY2_VLAN_TAG_USED
+	u16 vlan_tag = be16_to_cpu(sky2->rx_tag);
+	if (sky2->vlgrp && (status & GMR_FS_VLAN)) {
+		if (skb->ip_summed == CHECKSUM_NONE)
+			vlan_hwaccel_receive_skb(skb, sky2->vlgrp, vlan_tag);
+		else
+			vlan_gro_receive(&sky2->hw->napi, sky2->vlgrp,
+					 vlan_tag, skb);
+ 		return;
+ 	}
+#endif
+ 	if (skb->ip_summed == CHECKSUM_NONE)
+ 		netif_receive_skb(skb);
+ 	else
+ 		napi_gro_receive(&sky2->hw->napi, skb);
+}
+
 static inline void sky2_rx_done(struct sky2_hw *hw, unsigned port,
 				unsigned packets, unsigned bytes)
 {
@@ -2438,14 +2458,7 @@ static int sky2_status_intr(struct sky2_
 
 			skb->protocol = eth_type_trans(skb, dev);
 
-#ifdef SKY2_VLAN_TAG_USED
-			if (sky2->vlgrp && (status & GMR_FS_VLAN)) {
-				vlan_hwaccel_receive_skb(skb,
-							 sky2->vlgrp,
-							 be16_to_cpu(sky2->rx_tag));
-			} else
-#endif
-				netif_receive_skb(skb);
+			sky2_skb_rx(sky2, status, skb);
 
 			/* Stop after net poll weight */
 			if (++work_done >= to_do)

-- 

[PATCH 9/9] sky2: version 1.23

[Stephen Hemminger]

[-- Attachment #1: sky2-vers.patch --]
[-- Type: text/plain, Size: 359 bytes --]

Version bump.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2009-06-17 10:29:59.249757381 -0700
+++ b/drivers/net/sky2.c	2009-06-17 10:30:00.319061538 -0700
@@ -50,7 +50,7 @@
 #include "sky2.h"
 
 #define DRV_NAME		"sky2"
-#define DRV_VERSION		"1.22"
+#define DRV_VERSION		"1.23"
 #define PFX			DRV_NAME " "
 
 /*

-- 

Re: [PATCH 1/9] sky2: turn off pause during shutdown

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:32 -0700

> This unblocks the chip if it is stuck in pause cycle during
> shutdown.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 2/9] sky2: more receive shutdown

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:33 -0700

> Reset more parts of the receive path when device is take offline.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 3/9] sky2: PCI irq issues

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:34 -0700

> Add some read's to avoid any PCI posting issues when controlling
> irq's.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 4/9] sky2: fix shutdown synchronization

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:35 -0700

> The logic in sky2_down was incorrect. Receiver could report status
> after rx_stop was called.
> 
> The steps need to be:
>    * stop new frames from being transmitted
>    * shut off transmit/receive logic
>    * synchronize with NAPI to process status info about transmitter
>      and receiver
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 5/9] sky2: receive counter update

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:36 -0700

> Since it is likely that there are multiple packets received per
> interrupt, only update the receive counters once after all
> packets are processed.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 6/9] sky2: reduce default transmit ring

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:37 -0700

> Reduce the size of the driver transmit ring to reduce latency
> and allow qdisc to do better rate control.  Also make it
> obvious what the minimum transmit ring allowed is and why.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 7/9] sky2: skb recycling

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:38 -0700

> This patch implements skb recycling. It reclaims transmitted skb's
> for use in the receive ring.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 8/9] sky2: add GRO support

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:39 -0700

> Add support for generic receive offload.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

This patch was hand edited in some way, as if you took hunks from a
*.rej file and directly copied them into the source yet did not remove
the leading spaces that are added to *.rej files by patch.

So there were a ton of "space before tab" errors emmited by GIT.  I
fixed these up for you.

Re: [PATCH 9/9] sky2: version 1.23

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Jun 2009 10:30:40 -0700

> Version bump.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH 7/9] sky2: skb recycling

[Brandeburg, Jesse]

On Wed, 17 Jun 2009, Stephen Hemminger wrote:

> This patch implements skb recycling. It reclaims transmitted skb's
> for use in the receive ring.

Was sky2 able to show any gain in forwarding performance due to this 
patch?  Was it significant?  What kind of machine was used to test?

Re: [PATCH 7/9] sky2: skb recycling

[Stephen Hemminger]

On Thu, 18 Jun 2009 14:13:20 -0700 (Pacific Daylight Time)
"Brandeburg, Jesse" <jesse.brandeburg@intel.com> wrote:

> 
> 
> On Wed, 17 Jun 2009, Stephen Hemminger wrote:
> 
> > This patch implements skb recycling. It reclaims transmitted skb's
> > for use in the receive ring.
> 
> Was sky2 able to show any gain in forwarding performance due to this 
> patch?  Was it significant?  What kind of machine was used to test?

no not really, sky2 has other issue which hurt performance.


-- 

Re: [PATCH 4/9] sky2: fix shutdown synchronization

[Mike McCormack]

Hi Steven,

After applying your complete patch series, I'm still getting crashes in 
sky2_poll, and I can make them go away by adding an msleep(1) before 
these lines in sky2_down.

+	synchronize_irq(hw->pdev->irq);
+	napi_synchronize(&hw->napi);
+
  	sky2_phy_power_down(hw, port)

thanks,

Mike

Re: [PATCH 4/9] sky2: fix shutdown synchronization

[Stephen Hemminger]

On Fri, 19 Jun 2009 08:25:03 +0900
Mike McCormack <mikem@ring3k.org> wrote:

> 
> Hi Steven,
> 
> After applying your complete patch series, I'm still getting crashes in 
> sky2_poll, and I can make them go away by adding an msleep(1) before 
> these lines in sky2_down.
> 

adding msleep adds delay so interrupt and packets can clear, that is okay,
but would rather have something deterministic?  perhaps it needs to poll
irq status register or napi status.

-- 

Re: [PATCH 4/9] sky2: fix shutdown synchronization

[Mike McCormack]

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

Stephen Hemminger wrote:
> On Fri, 19 Jun 2009 08:25:03 +0900
> Mike McCormack <mikem@ring3k.org> wrote:
> 
>> Hi Steven,
>>
>> After applying your complete patch series, I'm still getting crashes in 
>> sky2_poll, and I can make them go away by adding an msleep(1) before 
>> these lines in sky2_down.
>>
> 
> adding msleep adds delay so interrupt and packets can clear, that is okay,
> but would rather have something deterministic?  perhaps it needs to poll
> irq status register or napi status.

Well, having a check for NULL rx_ring in sky2_poll functions is a little 
more deterministic, but I know you don't like that :-)

Another possibility is to try reordering the shutdown, I can have a go 
at experimenting with that.

I have found two other potential problems:

* there appears to be a race in sky2_poll (patch attached)

* sky2_up can cause receiver hangs due to a similar race to the one in 
sky2_down (I have some code which stops that from happening and will try 
put this top of your changes and push a patch out today for your inspection)

thanks,

Mike

[-- Attachment #2: 0001-Fix-a-race-condition-in-sky2_poll.patch --]
[-- Type: text/x-patch, Size: 1537 bytes --]

>From e9a7e8c1863a998428046bf7eaf7951379645822 Mon Sep 17 00:00:00 2001
From: Mike McCormack <mikem@ring3k.org>
Date: Thu, 18 Jun 2009 20:37:41 +0900
Subject: [PATCH] Fix a race condition in sky2_poll

Clear interrupt only when the status buffer is fully drained,
Make sure to clear interrupt when work_done == work_limit
 and the buffer is drained.
---
 drivers/net/sky2.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 7681d28..ca1e9e5 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -2524,9 +2524,6 @@ static int sky2_status_intr(struct sky2_hw *hw, int to_do, u16 idx)
 		}
 	} while (hw->st_idx != idx);
 
-	/* Fully processed status ring so clear irq */
-	sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
-
 exit_loop:
 	sky2_rx_done(hw, 0, total_packets[0], total_bytes[0]);
 	sky2_rx_done(hw, 1, total_packets[1], total_bytes[1]);
@@ -2779,9 +2776,16 @@ static int sky2_poll(struct napi_struct *napi, int work_limit)
 	if (status & Y2_IS_IRQ_PHY2)
 		sky2_phy_intr(hw, 1);
 
-	while ((idx = sky2_read16(hw, STAT_PUT_IDX)) != hw->st_idx) {
+	idx = sky2_read16(hw, STAT_PUT_IDX);
+	while (idx != hw->st_idx) {
 		work_done += sky2_status_intr(hw, work_limit - work_done, idx);
 
+		/* If we fully processed the status ring, clear the irq */
+		idx = sky2_read16(hw, STAT_PUT_IDX);
+		if (idx == hw->st_idx) {
+			sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
+			break;
+		}
 		if (work_done >= work_limit)
 			goto done;
 	}
-- 
1.5.6.5

Re: [PATCH 2/9] sky2: more receive shutdown

[Graham Murray]

Stephen Hemminger <shemminger@vyatta.com> writes:

> Reset more parts of the receive path when device is take offline.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> --- a/drivers/net/sky2.c	2009-06-17 10:29:50.630937431 -0700
> +++ b/drivers/net/sky2.c	2009-06-17 10:29:52.005685817 -0700
> @@ -1151,7 +1151,14 @@ stopped:
>  
>  	/* reset the Rx prefetch unit */
>  	sky2_write32(hw, Y2_QADDR(rxq, PREF_UNIT_CTRL), PREF_UNIT_RST_SET);
> -	mmiowb();
> +
> +	/* Reset the RAM Buffer receive queue */
> +	sky2_write8(hw, RB_ADDR(rxq, RB_CTRL), RB_RST_SET);
> +
> +	/* Reset Rx MAC FIFO */
> +	sky2_write8(hw, SK_REG(sky2->port, RX_GMF_CTRL_T), GMF_RST_SET);
> +
> +	sky2_read8(hw, B0_CTST);
>  }
>  
>  /* Clean out receive buffer area, assumes receiver hardware stopped */

With this this patch applied, the receive on my network card does not
work from boot. Git bisect identified this as the 'bad' commit and after
reverting it all works properly.

Here are the entries for sky2 and eth0 from the system log for a boot
with the patch enabled:-

Jun 20 06:38:25 newton sky2 driver version 1.23
Jun 20 06:38:25 newton sky2 0000:04:00.0: PCI INT A -> GSI 19 (level, low) -> IRQ 19
Jun 20 06:38:25 newton sky2 0000:04:00.0: setting latency timer to 64
Jun 20 06:38:25 newton sky2 0000:04:00.0: Yukon-2 EC chip revision 2
Jun 20 06:38:25 newton sky2 0000:04:00.0: irq 30 for MSI/MSI-X
Jun 20 06:38:25 newton sky2 eth0: addr 00:1b:fc:65:82:93
Jun 20 06:38:25 newton sky2 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
Jun 20 06:38:25 newton sky2 0000:03:00.0: setting latency timer to 64
Jun 20 06:38:25 newton sky2 0000:03:00.0: Yukon-2 EC chip revision 2
Jun 20 06:38:25 newton sky2 0000:03:00.0: irq 31 for MSI/MSI-X
Jun 20 06:38:25 newton sky2 eth1: addr 00:1b:fc:65:7c:be
Jun 20 06:38:25 newton sky2 eth0: enabling interface
Jun 20 06:38:25 newton ADDRCONF(NETDEV_UP): eth0: link is not ready
Jun 20 06:38:28 newton sky2 eth0: Link is up at 100 Mbps, full duplex, flow control both
Jun 20 06:38:28 newton ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

lspci -v
03:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8053 PCI-E Gigabit Ethernet Controller (rev 20)
        Subsystem: ASUSTeK Computer Inc. Marvell 88E8053 Gigabit Ethernet controller PCIe (Asus)
        Flags: bus master, fast devsel, latency 0, IRQ 31
        Memory at ff7fc000 (64-bit, non-prefetchable) [size=16K]
        I/O ports at a800 [size=256]
        Expansion ROM at ff7c0000 [disabled] [size=128K]
        Capabilities: [48] Power Management version 2
        Capabilities: [50] Vital Product Data
        Capabilities: [5c] MSI: Mask- 64bit+ Count=1/2 Enable+
        Capabilities: [e0] Express Legacy Endpoint, MSI 00
        Capabilities: [100] Advanced Error Reporting
        Kernel driver in use: sky2
        Kernel modules: sky2

Re: [PATCH 2/9] sky2: more receive shutdown

[Graham Murray]

Graham Murray <gmurray@webwayone.co.uk> writes:

> With this this patch applied, the receive on my network card does not
> work from boot. Git bisect identified this as the 'bad' commit and after
> reverting it all works properly.

I should add that there is no panic, BUG, oops etc, nor any other
printk messages, the interface just does not work.