[PATCH 0/5] sky2: version 1.2

[PATCH 0/5] sky2: version 1.2

[Stephen Hemminger]

Update to sky2 driver. Mostly fixes to try and handle users
stuck with edge-triggered interrupts. Also, some minor cleanups.

Patches apply onto 1.1 version in 2.6.17-rc2

--

[PATCH 1/5] sky2: reschedule if irq still pending

[Stephen Hemminger]

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

This is a workaround for the case edge-triggered irq's. Several users
seem to have broken configurations sharing edge-triggered irq's. To avoid
losing IRQ's, reshedule if more work arrives.

The changes to netdevice.h are to extract the part that puts device
back in list into separate inline.

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


--- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:48:44.000000000 -0700
+++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:48:47.000000000 -0700
@@ -2093,6 +2093,7 @@
 	int work_done = 0;
 	u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
 
+ restart_poll:
 	if (unlikely(status & ~Y2_IS_STAT_BMU)) {
 		if (status & Y2_IS_HW_ERR)
 			sky2_hw_intr(hw);
@@ -2123,7 +2124,7 @@
 	}
 
 	if (status & Y2_IS_STAT_BMU) {
-		work_done = sky2_status_intr(hw, work_limit);
+		work_done += sky2_status_intr(hw, work_limit - work_done);
 		*budget -= work_done;
 		dev0->quota -= work_done;
 
@@ -2133,9 +2134,22 @@
 		sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
 	}
 
-	netif_rx_complete(dev0);
+	local_irq_disable();
+	__netif_rx_complete(dev0);
 
 	status = sky2_read32(hw, B0_Y2_SP_LISR);
+
+	if (unlikely(status)) {
+		/* More work pending, try and keep going */
+		if (__netif_rx_schedule_prep(dev0)) {
+			__netif_rx_reschedule(dev0, work_done);
+			status = sky2_read32(hw, B0_Y2_SP_EISR);
+			local_irq_enable();
+			goto restart_poll;
+		}
+	}
+
+	local_irq_enable();
 	return 0;
 }
 
@@ -2153,8 +2167,6 @@
 	prefetch(&hw->st_le[hw->st_idx]);
 	if (likely(__netif_rx_schedule_prep(dev0)))
 		__netif_rx_schedule(dev0);
-	else
-		printk(KERN_DEBUG PFX "irq race detected\n");
 
 	return IRQ_HANDLED;
 }
--- sky2-2.6.17.orig/include/linux/netdevice.h	2006-04-25 10:48:44.000000000 -0700
+++ sky2-2.6.17/include/linux/netdevice.h	2006-04-25 10:48:47.000000000 -0700
@@ -829,19 +829,21 @@
 		__netif_rx_schedule(dev);
 }
 
-/* Try to reschedule poll. Called by dev->poll() after netif_rx_complete().
- * Do not inline this?
- */
+
+static inline void  __netif_rx_reschedule(struct net_device *dev, int undo)
+{
+	dev->quota += undo;
+	list_add_tail(&dev->poll_list, &__get_cpu_var(softnet_data).poll_list);
+	__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+}
+
+/* Try to reschedule poll. Called by dev->poll() after netif_rx_complete(). */
 static inline int netif_rx_reschedule(struct net_device *dev, int undo)
 {
 	if (netif_rx_schedule_prep(dev)) {
 		unsigned long flags;
-
-		dev->quota += undo;
-
 		local_irq_save(flags);
-		list_add_tail(&dev->poll_list, &__get_cpu_var(softnet_data).poll_list);
-		__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+		__netif_rx_reschedule(dev, undo);
 		local_irq_restore(flags);
 		return 1;
 	}

--

Re: [PATCH 1/5] sky2: reschedule if irq still pending

[Jeff Garzik]

Stephen Hemminger wrote:
> This is a workaround for the case edge-triggered irq's. Several users
> seem to have broken configurations sharing edge-triggered irq's. To avoid
> losing IRQ's, reshedule if more work arrives.
> 
> The changes to netdevice.h are to extract the part that puts device
> back in list into separate inline.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

applied 1-5 to #upstream-fixes

[PATCH 2/5] sky2: add fake idle irq timer

[Stephen Hemminger]

[-- Attachment #1: sky2-idle-timer.patch --]
[-- Type: text/plain, Size: 1943 bytes --]

Add an fake NAPI schedule once a second. This is an attempt to work around
for broken configurations with edge-triggered interrupts.

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

--- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:48:47.000000000 -0700
+++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:53:32.000000000 -0700
@@ -2086,6 +2086,20 @@
 	}
 }
 
+/* If idle then force a fake soft NAPI poll once a second
+ * to work around cases where sharing an edge triggered interrupt.
+ */
+static void sky2_idle(unsigned long arg)
+{
+	struct net_device *dev = (struct net_device *) arg;
+
+	local_irq_disable();
+	if (__netif_rx_schedule_prep(dev))
+		__netif_rx_schedule(dev);
+	local_irq_enable();
+}
+
+
 static int sky2_poll(struct net_device *dev0, int *budget)
 {
 	struct sky2_hw *hw = ((struct sky2_port *) netdev_priv(dev0))->hw;
@@ -2134,6 +2148,8 @@
 		sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
 	}
 
+	mod_timer(&hw->idle_timer, jiffies + HZ);
+
 	local_irq_disable();
 	__netif_rx_complete(dev0);
 
@@ -3288,6 +3304,8 @@
 
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 
+	setup_timer(&hw->idle_timer, sky2_idle, (unsigned long) dev);
+
 	pci_set_drvdata(pdev, hw);
 
 	return 0;
@@ -3323,13 +3341,15 @@
 	if (!hw)
 		return;
 
+	del_timer_sync(&hw->idle_timer);
+
+	sky2_write32(hw, B0_IMSK, 0);
 	dev0 = hw->dev[0];
 	dev1 = hw->dev[1];
 	if (dev1)
 		unregister_netdev(dev1);
 	unregister_netdev(dev0);
 
-	sky2_write32(hw, B0_IMSK, 0);
 	sky2_set_power_state(hw, PCI_D3hot);
 	sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
 	sky2_write8(hw, B0_CTST, CS_RST_SET);
--- sky2-2.6.17.orig/drivers/net/sky2.h	2006-04-25 10:48:42.000000000 -0700
+++ sky2-2.6.17/drivers/net/sky2.h	2006-04-25 10:51:33.000000000 -0700
@@ -1880,6 +1880,8 @@
 	struct sky2_status_le *st_le;
 	u32		     st_idx;
 	dma_addr_t   	     st_dma;
+
+	struct timer_list    idle_timer;
 	int		     msi_detected;
 	wait_queue_head_t    msi_wait;
 };

--

Re: [PATCH 2/5] sky2: add fake idle irq timer

[Francois Romieu]

Stephen Hemminger <shemminger@osdl.org> :
[...]
> --- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:48:47.000000000 -0700
> +++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:53:32.000000000 -0700
> @@ -2086,6 +2086,20 @@
>  	}
>  }
>  
> +/* If idle then force a fake soft NAPI poll once a second
> + * to work around cases where sharing an edge triggered interrupt.
> + */
> +static void sky2_idle(unsigned long arg)
> +{
> +	struct net_device *dev = (struct net_device *) arg;
> +
> +	local_irq_disable();
> +	if (__netif_rx_schedule_prep(dev))
> +		__netif_rx_schedule(dev);
> +	local_irq_enable();
> +}
> +
> +
>  static int sky2_poll(struct net_device *dev0, int *budget)
>  {
>  	struct sky2_hw *hw = ((struct sky2_port *) netdev_priv(dev0))->hw;
> @@ -2134,6 +2148,8 @@
>  		sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
>  	}
>  
> +	mod_timer(&hw->idle_timer, jiffies + HZ);
> +
>  	local_irq_disable();
>  	__netif_rx_complete(dev0);


Any objection against moving mod_timer() from sky2_poll() to sky2_idle()
so as to keep poll() path unmodified ?

-- 
Ueimor

Re: [PATCH 2/5] sky2: add fake idle irq timer

[Stephen Hemminger]

On Tue, 25 Apr 2006 23:23:29 +0200
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Stephen Hemminger <shemminger@osdl.org> :
> [...]
> > --- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:48:47.000000000 -0700
> > +++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:53:32.000000000 -0700
> > @@ -2086,6 +2086,20 @@
> >  	}
> >  }
> >  
> > +/* If idle then force a fake soft NAPI poll once a second
> > + * to work around cases where sharing an edge triggered interrupt.
> > + */
> > +static void sky2_idle(unsigned long arg)
> > +{
> > +	struct net_device *dev = (struct net_device *) arg;
> > +
> > +	local_irq_disable();
> > +	if (__netif_rx_schedule_prep(dev))
> > +		__netif_rx_schedule(dev);
> > +	local_irq_enable();
> > +}
> > +
> > +
> >  static int sky2_poll(struct net_device *dev0, int *budget)
> >  {
> >  	struct sky2_hw *hw = ((struct sky2_port *) netdev_priv(dev0))->hw;
> > @@ -2134,6 +2148,8 @@
> >  		sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
> >  	}
> >  
> > +	mod_timer(&hw->idle_timer, jiffies + HZ);
> > +
> >  	local_irq_disable();
> >  	__netif_rx_complete(dev0);
> 
> 
> Any objection against moving mod_timer() from sky2_poll() to sky2_idle()
> so as to keep poll() path unmodified ?
> 

If traffic is moving, then I want the timer to keep getting rescheduled
farther out.

Re: [PATCH 2/5] sky2: add fake idle irq timer

[Francois Romieu]

Stephen Hemminger <shemminger@osdl.org> :
[...]
> > Any objection against moving mod_timer() from sky2_poll() to sky2_idle()
> > so as to keep poll() path unmodified ?
> > 
> 
> If traffic is moving, then I want the timer to keep getting rescheduled
> farther out.

If my version of the driver is not stale, the timer will not be
rescheduled when work_done >= work_limit. I.e. the optimization
tends to vanish when the load goes up. Before this point is reached,
the long path of mod_timer() can be taken up to HZ per second.

I have no idea which balance works best.

-- 
Ueimor

Re: [PATCH 2/5] sky2: add fake idle irq timer

[Stephen Hemminger]

On Wed, 26 Apr 2006 00:39:00 +0200
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Stephen Hemminger <shemminger@osdl.org> :
> [...]
> > > Any objection against moving mod_timer() from sky2_poll() to sky2_idle()
> > > so as to keep poll() path unmodified ?
> > > 
> > 
> > If traffic is moving, then I want the timer to keep getting rescheduled
> > farther out.
> 
> If my version of the driver is not stale, the timer will not be
> rescheduled when work_done >= work_limit.

I am trying to work around possible lost IRQ's, not netdev scheduler
screw up's.  If workdone >= work_limit, then it will already be
called back later when it return's 1.

[PATCH 3/5] sky2: use ALIGN() macro

[Stephen Hemminger]

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

The ALIGN() macro in kernel.h does the same math that the
sky2 driver was using for padding.

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

--- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:47:03.000000000 -0700
+++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:47:28.000000000 -0700
@@ -925,8 +925,7 @@
 	skb = alloc_skb(size + RX_SKB_ALIGN, gfp_mask);
 	if (likely(skb)) {
 		unsigned long p	= (unsigned long) skb->data;
-		skb_reserve(skb,
-			((p + RX_SKB_ALIGN - 1) & ~(RX_SKB_ALIGN - 1)) - p);
+		skb_reserve(skb, ALIGN(p, RX_SKB_ALIGN) - p);
 	}
 
 	return skb;
@@ -1686,13 +1685,12 @@
 }
 
 
-#define roundup(x, y)   ((((x)+((y)-1))/(y))*(y))
 /* Want receive buffer size to be multiple of 64 bits
  * and incl room for vlan and truncation
  */
 static inline unsigned sky2_buf_size(int mtu)
 {
-	return roundup(mtu + ETH_HLEN + VLAN_HLEN, 8) + 8;
+	return ALIGN(mtu + ETH_HLEN + VLAN_HLEN, 8) + 8;
 }
 
 static int sky2_change_mtu(struct net_device *dev, int new_mtu)

--

[PATCH 4/5] sky2: reset function can be devinit

[Stephen Hemminger]

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

The sky2_reset function only called from sky2_probe.
Maybe the compiler was smart enough to figure this out already.

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


--- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:53:37.000000000 -0700
+++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:54:57.000000000 -0700
@@ -2219,7 +2219,7 @@
 }
 
 
-static int sky2_reset(struct sky2_hw *hw)
+static int __devinit sky2_reset(struct sky2_hw *hw)
 {
 	u16 status;
 	u8 t8, pmd_type;

--

[PATCH 5/5] sky2: version 1.2

[Stephen Hemminger]

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

Update to version 1.2

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


--- sky2-2.6.17.orig/drivers/net/sky2.c	2006-04-25 10:54:57.000000000 -0700
+++ sky2-2.6.17/drivers/net/sky2.c	2006-04-25 10:55:51.000000000 -0700
@@ -51,7 +51,7 @@
 #include "sky2.h"
 
 #define DRV_NAME		"sky2"
-#define DRV_VERSION		"1.1"
+#define DRV_VERSION		"1.2"
 #define PFX			DRV_NAME " "
 
 /*

--

Re: [PATCH 0/5] sky2: version 1.2

[Bertrand Jacquin]

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

Le Tue, 25 Apr 2006 10:58:49 -0700, Stephen Hemminger <shemminger@osdl.org> m'a avoué:

> Update to sky2 driver. Mostly fixes to try and handle users
> stuck with edge-triggered interrupts. Also, some minor cleanups.
> 
> Patches apply onto 1.1 version in 2.6.17-rc2

Hi,

I've just testing patches and I get still the same bug.

-- 
/* Beber : beber (AT) gna (DOT) org
 * http://guybrush.ath.cx, irc://irc.freenode.net/#{e.fr,gentoofr}
 * Guybrush @ Melee */

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