[PATCH 0/5] skge update

[PATCH 0/5] skge update

[Stephen Hemminger]

This set fixes several races and performance problems related
to bug reports.


-- 
VGER BF report: H 0.437515

[PATCH 1/5] skge: use netdev_alloc_skb

[Stephen Hemminger]

[-- Attachment #1: skge-netdev-alloc.patch --]
[-- Type: text/plain, Size: 3331 bytes --]

Change to use new netdev_alloc_skb interface for 2.6.18.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- skge-2.6.orig/drivers/net/skge.c
+++ skge-2.6/drivers/net/skge.c
@@ -818,8 +818,9 @@ static void skge_rx_clean(struct skge_po
 /* Allocate buffers for receive ring
  * For receive:  to_clean is next received frame.
  */
-static int skge_rx_fill(struct skge_port *skge)
+static int skge_rx_fill(struct net_device *dev)
 {
+	struct skge_port *skge = netdev_priv(dev);
 	struct skge_ring *ring = &skge->rx_ring;
 	struct skge_element *e;
 
@@ -827,8 +828,8 @@ static int skge_rx_fill(struct skge_port
 	do {
 		struct sk_buff *skb;
 
-		skb = __dev_alloc_skb(skge->rx_buf_size + NET_IP_ALIGN,
-				      GFP_KERNEL);
+		skb = __netdev_alloc_skb(dev, skge->rx_buf_size + NET_IP_ALIGN,
+					 GFP_KERNEL);
 		if (!skb)
 			return -ENOMEM;
 
@@ -2179,7 +2180,7 @@ static int skge_up(struct net_device *de
 	if (err)
 		goto free_pci_mem;
 
-	err = skge_rx_fill(skge);
+	err = skge_rx_fill(dev);
 	if (err)
 		goto free_rx_ring;
 
@@ -2585,16 +2586,17 @@ static inline int bad_phy_status(const s
 /* Get receive buffer from descriptor.
  * Handles copy of small buffers and reallocation failures
  */
-static inline struct sk_buff *skge_rx_get(struct skge_port *skge,
-					  struct skge_element *e,
-					  u32 control, u32 status, u16 csum)
+static struct sk_buff *skge_rx_get(struct net_device *dev,
+				   struct skge_element *e,
+				   u32 control, u32 status, u16 csum)
 {
+	struct skge_port *skge = netdev_priv(dev);
 	struct sk_buff *skb;
 	u16 len = control & BMU_BBC;
 
 	if (unlikely(netif_msg_rx_status(skge)))
 		printk(KERN_DEBUG PFX "%s: rx slot %td status 0x%x len %d\n",
-		       skge->netdev->name, e - skge->rx_ring.start,
+		       dev->name, e - skge->rx_ring.start,
 		       status, len);
 
 	if (len > skge->rx_buf_size)
@@ -2610,7 +2612,7 @@ static inline struct sk_buff *skge_rx_ge
 		goto error;
 
 	if (len < RX_COPY_THRESHOLD) {
-		skb = dev_alloc_skb(len + 2);
+		skb = netdev_alloc_skb(dev, len + 2);
 		if (!skb)
 			goto resubmit;
 
@@ -2625,7 +2627,7 @@ static inline struct sk_buff *skge_rx_ge
 		skge_rx_reuse(e, skge->rx_buf_size);
 	} else {
 		struct sk_buff *nskb;
-		nskb = dev_alloc_skb(skge->rx_buf_size + NET_IP_ALIGN);
+		nskb = netdev_alloc_skb(dev, skge->rx_buf_size + NET_IP_ALIGN);
 		if (!nskb)
 			goto resubmit;
 
@@ -2640,20 +2642,19 @@ static inline struct sk_buff *skge_rx_ge
 	}
 
 	skb_put(skb, len);
-	skb->dev = skge->netdev;
 	if (skge->rx_csum) {
 		skb->csum = csum;
 		skb->ip_summed = CHECKSUM_HW;
 	}
 
-	skb->protocol = eth_type_trans(skb, skge->netdev);
+	skb->protocol = eth_type_trans(skb, dev);
 
 	return skb;
 error:
 
 	if (netif_msg_rx_err(skge))
 		printk(KERN_DEBUG PFX "%s: rx err, slot %td control 0x%x status 0x%x\n",
-		       skge->netdev->name, e - skge->rx_ring.start,
+		       dev->name, e - skge->rx_ring.start,
 		       control, status);
 
 	if (skge->hw->chip_id == CHIP_ID_GENESIS) {
@@ -2723,7 +2724,7 @@ static int skge_poll(struct net_device *
 		if (control & BMU_OWN)
 			break;
 
-		skb = skge_rx_get(skge, e, control, rd->status, rd->csum2);
+		skb = skge_rx_get(dev, e, control, rd->status, rd->csum2);
 		if (likely(skb)) {
 			dev->last_rx = jiffies;
 			netif_receive_skb(skb);

--


-- 
VGER BF report: H 0.0016278

[PATCH 2/5] skge: irq lock race

[Stephen Hemminger]

[-- Attachment #1: skge-irq-lock-order.patch --]
[-- Type: text/plain, Size: 1061 bytes --]

The driver needs to access the IRQ status inside of lock to avoid
races with other places changing IRQ mask etc. This may be related
to some of the SMP bugs reported against skge in kernel bugzilla.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- netdev-2.6.orig/drivers/net/skge.c
+++ netdev-2.6/drivers/net/skge.c
@@ -2891,13 +2891,15 @@ static irqreturn_t skge_intr(int irq, vo
 {
 	struct skge_hw *hw = dev_id;
 	u32 status;
+	int handled = 0;
 
+	spin_lock(&hw->hw_lock);
 	/* Reading this register masks IRQ */
 	status = skge_read32(hw, B0_SP_ISRC);
 	if (status == 0)
-		return IRQ_NONE;
+		goto out;
 
-	spin_lock(&hw->hw_lock);
+	handled = 1;
 	status &= hw->intr_mask;
 	if (status & IS_EXT_REG) {
 		hw->intr_mask &= ~IS_EXT_REG;
@@ -2959,9 +2961,10 @@ static irqreturn_t skge_intr(int irq, vo
 
 	skge_write32(hw, B0_IMSK, hw->intr_mask);
 	skge_read32(hw, B0_IMSK);
+out:
 	spin_unlock(&hw->hw_lock);
 
-	return IRQ_HANDLED;
+	return IRQ_RETVAL(handled);
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER

--


-- 
VGER BF report: H 0.00723675

[PATCH 3/5] skge: use NAPI for transmit complete

[Stephen Hemminger]

[-- Attachment #1: skge-tx-napi.patch --]
[-- Type: text/plain, Size: 7090 bytes --]

The skge driver has much better performance if transmit done is handled in
NAPI softirq. Change from doing transmit locking in driver (LLTX) and
use device lock.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


--- netdev-2.6.orig/drivers/net/skge.c
+++ netdev-2.6/drivers/net/skge.c
@@ -91,7 +91,7 @@ MODULE_DEVICE_TABLE(pci, skge_id_table);
 static int skge_up(struct net_device *dev);
 static int skge_down(struct net_device *dev);
 static void skge_phy_reset(struct skge_port *skge);
-static void skge_tx_clean(struct skge_port *skge);
+static void skge_tx_clean(struct net_device *dev);
 static int xm_phy_write(struct skge_hw *hw, int port, u16 reg, u16 val);
 static int gm_phy_write(struct skge_hw *hw, int port, u16 reg, u16 val);
 static void genesis_get_stats(struct skge_port *skge, u64 *data);
@@ -105,6 +105,7 @@ static const int txqaddr[] = { Q_XA1, Q_
 static const int rxqaddr[] = { Q_R1, Q_R2 };
 static const u32 rxirqmask[] = { IS_R1_F, IS_R2_F };
 static const u32 txirqmask[] = { IS_XA1_F, IS_XA2_F };
+static const u32 irqmask[] = { IS_R1_F|IS_XA1_F, IS_R2_F|IS_XA2_F };
 
 static int skge_get_regs_len(struct net_device *dev)
 {
@@ -2283,7 +2284,7 @@ static int skge_down(struct net_device *
 	skge_led(skge, LED_MODE_OFF);
 
 	netif_poll_disable(dev);
-	skge_tx_clean(skge);
+	skge_tx_clean(dev);
 	skge_rx_clean(skge);
 
 	kfree(skge->rx_ring.start);
@@ -2308,25 +2309,12 @@ static int skge_xmit_frame(struct sk_buf
 	int i;
 	u32 control, len;
 	u64 map;
-	unsigned long flags;
 
 	if (skb_padto(skb, ETH_ZLEN))
 		return NETDEV_TX_OK;
 
-	if (!spin_trylock_irqsave(&skge->tx_lock, flags))
-		/* Collision - tell upper layer to requeue */
-		return NETDEV_TX_LOCKED;
-
-	if (unlikely(skge_avail(&skge->tx_ring) < skb_shinfo(skb)->nr_frags + 1)) {
-		if (!netif_queue_stopped(dev)) {
-			netif_stop_queue(dev);
-
-			printk(KERN_WARNING PFX "%s: ring full when queue awake!\n",
-			       dev->name);
-		}
-		spin_unlock_irqrestore(&skge->tx_lock, flags);
+	if (unlikely(skge_avail(&skge->tx_ring) < skb_shinfo(skb)->nr_frags + 1))
 		return NETDEV_TX_BUSY;
-	}
 
 	e = skge->tx_ring.to_use;
 	td = e->desc;
@@ -2401,8 +2389,6 @@ static int skge_xmit_frame(struct sk_buf
 		netif_stop_queue(dev);
 	}
 
-	spin_unlock_irqrestore(&skge->tx_lock, flags);
-
 	dev->trans_start = jiffies;
 
 	return NETDEV_TX_OK;
@@ -2432,18 +2418,18 @@ static void skge_tx_free(struct skge_por
 			printk(KERN_DEBUG PFX "%s: tx done slot %td\n",
 			       skge->netdev->name, e - skge->tx_ring.start);
 
-		dev_kfree_skb_any(e->skb);
+		dev_kfree_skb(e->skb);
 	}
 	e->skb = NULL;
 }
 
 /* Free all buffers in transmit ring */
-static void skge_tx_clean(struct skge_port *skge)
+static void skge_tx_clean(struct net_device *dev)
 {
+	struct skge_port *skge = netdev_priv(dev);
 	struct skge_element *e;
-	unsigned long flags;
 
-	spin_lock_irqsave(&skge->tx_lock, flags);
+	netif_tx_lock_bh(dev);
 	for (e = skge->tx_ring.to_clean; e != skge->tx_ring.to_use; e = e->next) {
 		struct skge_tx_desc *td = e->desc;
 		skge_tx_free(skge, e, td->control);
@@ -2451,8 +2437,8 @@ static void skge_tx_clean(struct skge_po
 	}
 
 	skge->tx_ring.to_clean = e;
-	netif_wake_queue(skge->netdev);
-	spin_unlock_irqrestore(&skge->tx_lock, flags);
+	netif_wake_queue(dev);
+	netif_tx_unlock_bh(dev);
 }
 
 static void skge_tx_timeout(struct net_device *dev)
@@ -2463,7 +2449,7 @@ static void skge_tx_timeout(struct net_d
 		printk(KERN_DEBUG PFX "%s: tx timeout\n", dev->name);
 
 	skge_write8(skge->hw, Q_ADDR(txqaddr[skge->port], Q_CSR), CSR_STOP);
-	skge_tx_clean(skge);
+	skge_tx_clean(dev);
 }
 
 static int skge_change_mtu(struct net_device *dev, int new_mtu)
@@ -2679,15 +2665,15 @@ resubmit:
 }
 
 /* Free all buffers in Tx ring which are no longer owned by device */
-static void skge_txirq(struct net_device *dev)
+static void skge_tx_done(struct net_device *dev)
 {
 	struct skge_port *skge = netdev_priv(dev);
 	struct skge_ring *ring = &skge->tx_ring;
 	struct skge_element *e;
 
-	rmb();
+	skge_write8(skge->hw, Q_ADDR(txqaddr[skge->port], Q_CSR), CSR_IRQ_CL_F);
 
-	spin_lock(&skge->tx_lock);
+	netif_tx_lock(dev);
 	for (e = ring->to_clean; e != ring->to_use; e = e->next) {
 		struct skge_tx_desc *td = e->desc;
 
@@ -2698,11 +2684,10 @@ static void skge_txirq(struct net_device
 	}
 	skge->tx_ring.to_clean = e;
 
-	if (netif_queue_stopped(skge->netdev)
-	    && skge_avail(&skge->tx_ring) > TX_LOW_WATER)
-		netif_wake_queue(skge->netdev);
+	if (skge_avail(&skge->tx_ring) > TX_LOW_WATER)
+		netif_wake_queue(dev);
 
-	spin_unlock(&skge->tx_lock);
+	netif_tx_unlock(dev);
 }
 
 static int skge_poll(struct net_device *dev, int *budget)
@@ -2714,6 +2699,10 @@ static int skge_poll(struct net_device *
 	int to_do = min(dev->quota, *budget);
 	int work_done = 0;
 
+	skge_tx_done(dev);
+
+	skge_write8(hw, Q_ADDR(rxqaddr[skge->port], Q_CSR), CSR_IRQ_CL_F);
+
 	for (e = ring->to_clean; prefetch(e->next), work_done < to_do; e = e->next) {
 		struct skge_rx_desc *rd = e->desc;
 		struct sk_buff *skb;
@@ -2744,10 +2733,9 @@ static int skge_poll(struct net_device *
 	if (work_done >=  to_do)
 		return 1; /* not done */
 
-	netif_rx_complete(dev);
-
 	spin_lock_irq(&hw->hw_lock);
-	hw->intr_mask |= rxirqmask[skge->port];
+	__netif_rx_complete(dev);
+	hw->intr_mask |= irqmask[skge->port];
   	skge_write32(hw, B0_IMSK, hw->intr_mask);
 	skge_read32(hw, B0_IMSK);
 	spin_unlock_irq(&hw->hw_lock);
@@ -2906,14 +2894,8 @@ static irqreturn_t skge_intr(int irq, vo
 		schedule_work(&hw->phy_work);
 	}
 
-	if (status & IS_XA1_F) {
-		skge_write8(hw, Q_ADDR(Q_XA1, Q_CSR), CSR_IRQ_CL_F);
-		skge_txirq(hw->dev[0]);
-	}
-
-	if (status & IS_R1_F) {
-		skge_write8(hw, Q_ADDR(Q_R1, Q_CSR), CSR_IRQ_CL_F);
-		hw->intr_mask &= ~IS_R1_F;
+	if (status & (IS_XA1_F|IS_R1_F)) {
+		hw->intr_mask &= ~(IS_XA1_F|IS_R1_F);
 		netif_rx_schedule(hw->dev[0]);
 	}
 
@@ -2932,14 +2914,8 @@ static irqreturn_t skge_intr(int irq, vo
 		skge_mac_intr(hw, 0);
 
 	if (hw->dev[1]) {
-		if (status & IS_XA2_F) {
-			skge_write8(hw, Q_ADDR(Q_XA2, Q_CSR), CSR_IRQ_CL_F);
-			skge_txirq(hw->dev[1]);
-		}
-
-		if (status & IS_R2_F) {
-			skge_write8(hw, Q_ADDR(Q_R2, Q_CSR), CSR_IRQ_CL_F);
-			hw->intr_mask &= ~IS_R2_F;
+		if (status & (IS_XA2_F|IS_R2_F)) {
+			hw->intr_mask &= ~(IS_XA2_F|IS_R2_F);
 			netif_rx_schedule(hw->dev[1]);
 		}
 
@@ -3228,7 +3204,7 @@ static struct net_device *skge_devinit(s
 	dev->poll_controller = skge_netpoll;
 #endif
 	dev->irq = hw->pdev->irq;
-	dev->features = NETIF_F_LLTX;
+
 	if (highmem)
 		dev->features |= NETIF_F_HIGHDMA;
 
@@ -3250,8 +3226,6 @@ static struct net_device *skge_devinit(s
 
 	skge->port = port;
 
-	spin_lock_init(&skge->tx_lock);
-
 	if (hw->chip_id != CHIP_ID_GENESIS) {
 		dev->features |= NETIF_F_IP_CSUM | NETIF_F_SG;
 		skge->rx_csum = 1;
--- netdev-2.6.orig/drivers/net/skge.h
+++ netdev-2.6/drivers/net/skge.h
@@ -2417,7 +2417,6 @@ struct skge_port {
 	struct net_device    *netdev;
 	int		     port;
 
-	spinlock_t	     tx_lock;
 	struct skge_ring     tx_ring;
 	struct skge_ring     rx_ring;
 

--


-- 
VGER BF report: H 0.0797428

[PATCH 4/5] skge: version 1.8

[Stephen Hemminger]

[-- Attachment #1: skge-1.8.patch --]
[-- Type: text/plain, Size: 417 bytes --]

Because of the NAPI and other SMP fixes, let's call
this a version.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- skge-2.6.orig/drivers/net/skge.c
+++ skge-2.6/drivers/net/skge.c
@@ -43,7 +43,7 @@
 #include "skge.h"
 
 #define DRV_NAME		"skge"
-#define DRV_VERSION		"1.7"
+#define DRV_VERSION		"1.8"
 #define PFX			DRV_NAME " "
 
 #define DEFAULT_TX_RING_SIZE	128

--


-- 
VGER BF report: H 0.0441835

[PATCH 5/5] skge: pci_post bugs

[Stephen Hemminger]

[-- Attachment #1: skge-pci-post.patch --]
[-- Type: text/plain, Size: 1348 bytes --]

There are several places where this driver needs to ensure that 
PCI accesses have completed before releasing locks. These maybe related
to the problem (not verified):
	http://bugzilla.kernel.org/show_bug.cgi?id=6142

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


--- netdev-2.6.orig/drivers/net/skge.c
+++ netdev-2.6/drivers/net/skge.c
@@ -2747,7 +2747,7 @@ static int skge_poll(struct net_device *
 	spin_lock_irq(&hw->hw_lock);
 	hw->intr_mask |= rxirqmask[skge->port];
   	skge_write32(hw, B0_IMSK, hw->intr_mask);
-	mmiowb();
+	skge_read32(hw, B0_IMSK);
 	spin_unlock_irq(&hw->hw_lock);
 
 	return 0;
@@ -2881,6 +2881,7 @@ static void skge_extirq(void *arg)
 	spin_lock_irq(&hw->hw_lock);
 	hw->intr_mask |= IS_EXT_REG;
 	skge_write32(hw, B0_IMSK, hw->intr_mask);
+	skge_read32(hw, B0_IMSK);
 	spin_unlock_irq(&hw->hw_lock);
 }
 
@@ -2955,6 +2956,7 @@ static irqreturn_t skge_intr(int irq, vo
 		skge_error_irq(hw);
 
 	skge_write32(hw, B0_IMSK, hw->intr_mask);
+	skge_read32(hw, B0_IMSK);
 	spin_unlock(&hw->hw_lock);
 
 	return IRQ_HANDLED;
@@ -3424,6 +3426,7 @@ static void __devexit skge_remove(struct
 	spin_lock_irq(&hw->hw_lock);
 	hw->intr_mask = 0;
 	skge_write32(hw, B0_IMSK, 0);
+	skge_read32(hw, B0_IMSK);
 	spin_unlock_irq(&hw->hw_lock);
 
 	skge_write16(hw, B0_LED, LED_STAT_OFF);

--


-- 
VGER BF report: H 0.178292

Re: [PATCH 2/5] skge: irq lock race

[Jeff Garzik]

Stephen Hemminger wrote:
> The driver needs to access the IRQ status inside of lock to avoid
> races with other places changing IRQ mask etc. This may be related
> to some of the SMP bugs reported against skge in kernel bugzilla.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> --- netdev-2.6.orig/drivers/net/skge.c
> +++ netdev-2.6/drivers/net/skge.c
> @@ -2891,13 +2891,15 @@ static irqreturn_t skge_intr(int irq, vo
>  {
>  	struct skge_hw *hw = dev_id;
>  	u32 status;
> +	int handled = 0;
>  
> +	spin_lock(&hw->hw_lock);
>  	/* Reading this register masks IRQ */
>  	status = skge_read32(hw, B0_SP_ISRC);
>  	if (status == 0)
> -		return IRQ_NONE;
> +		goto out;

You also need to check for device/hardware fault here (status==0xffffffff).

	Jeff

Re: [PATCH 1/5] skge: use netdev_alloc_skb

[Jeff Garzik]

applied patches 1-4.  patch #5 does not apply:

[jgarzik@pretzel netdev-2.6]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt
5 patch(es) to process.

Applying 'skge: use netdev_alloc_skb'

Wrote tree 5933a42766864a3117611ec1c999758e9f8a2f38
Committed: c54f9765daafe8493dba837b3d70e97432cd876a

Applying 'skge: irq lock race'

Wrote tree d896de1c7306f0f03c9781cb054bbcd5cf0a9817
Committed: 29365c900963d4986b74a0dadea46872bf283d76

Applying 'skge: use NAPI for transmit complete'

Wrote tree 84d8d7355f8c14f3abd57e8e313ce39ed09e3d05
Committed: 513f533e3f161c5a59555677d35e8ae28037bd89

Applying 'skge: version 1.8'

Wrote tree f17e223a4b80c22e82c2a8543806aebad4e6e77c
Committed: f6aa1693671fa9ea661fd30f003820d129fe0628

Applying 'skge: pci_post bugs'

error: patch failed: drivers/net/skge.c:2747
error: drivers/net/skge.c: patch does not apply

[PATCH] skge: heck for PCI hotplug during IRQ

[Stephen Hemminger]

Check if IRQ came from hardware fault (hotplug).

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

---

 drivers/net/skge.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

7e83b6245b11158875757ab346ca6d0954ecb95e
diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 3f1b72e..fba8b74 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -2884,7 +2884,7 @@ static irqreturn_t skge_intr(int irq, vo
 	spin_lock(&hw->hw_lock);
 	/* Reading this register masks IRQ */
 	status = skge_read32(hw, B0_SP_ISRC);
-	if (status == 0)
+	if (status == 0 || status == ~0)
 		goto out;
 
 	handled = 1;
-- 
1.2.4

Re: [PATCH] skge: heck for PCI hotplug during IRQ

[Jeff Garzik]

applied