Re: [Fwd: [RFT] NAPI for 8139too]

Re: [Fwd: [RFT] NAPI for 8139too]

[OGAWA Hirofumi]

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

Hi,

I also working for it (patches attached). So I have several comment.

Jeff Garzik <jgarzik@pobox.com> writes:

> +	dev->weight = 16;

just question: Is there basis for believing that this value is
rightness?  Although I use 64, I don't have a reason for having chosen
it at all.

>  	dev->stop = rtl8139_close;
>  	dev->get_stats = rtl8139_get_stats;
>  	dev->set_multicast_list = rtl8139_set_rx_mode;
> @@ -1879,24 +1877,28 @@
>  #endif
>  }
>  
> -static void rtl8139_rx_interrupt (struct net_device *dev,
> -				  struct rtl8139_private *tp, void *ioaddr)
> +/*
> + * NAPI poll routine.
> + */
> +static int rtl8139_poll(struct net_device *dev, int *budget)
>  {
> +	struct rtl8139_private *tp = dev->priv;
> +	void *ioaddr = tp->mmio_addr;
>  	unsigned char *rx_ring;
>  	u16 cur_rx;
> +	int rx, status;
> +	unsigned long flags;
>  
> -	assert (dev != NULL);
> -	assert (tp != NULL);
> -	assert (ioaddr != NULL);
> -
> +	spin_lock_irqsave(&tp->lock, flags);

I don't think lock of interrupt related path needed here. And this
thinks that it has a bad influence on TX path.

Instead of it, I think we need synchronize with ->tx_timeout because
->tx_timeout touch IntrMask.

> + rescan:
>  	rx_ring = tp->rx_ring;
>  	cur_rx = tp->cur_rx;
> -
[...]
>  			RTL_W16_F (IntrStatus, RxAckBits);
> +
> +		if (++rx >= dev->quota)

Shouldn't we use "min(dev->quota, *budget)" for this limit? Users can
change "budget" via sysctl.

> +	/* If received interrupt, then disable furthur interrupts
> +	 * and enable NAPI polling.
> +	 */
> +	if (netif_running (dev) && (status & RxAckBits)) {
> +		/* disable more receive interrupts */
> +		RTL_W16 (IntrMask, rtl8139_intr_mask & ~RxAckBits);
> +		netif_rx_schedule(dev);
> +	}

This style have problem on shutdowning interface path. Please see
8139too-napi-stop-fix.patch.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[-- Attachment #2: 8139too.tar.gz --]
[-- Type: application/octet-stream, Size: 5335 bytes --]

Re: [Fwd: [RFT] NAPI for 8139too]

[Stephen Hemminger]

On Sat, 18 Oct 2003 21:56:20 +0900
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:

> Hi,
> 
> I also working for it (patches attached). So I have several comment.
> 
> Jeff Garzik <jgarzik@pobox.com> writes:
> 
> > +	dev->weight = 16;
> 
> just question: Is there basis for believing that this value is
> rightness?  Although I use 64, I don't have a reason for having chosen
> it at all.

I chose 16 only because that was in the sample NAPI documentation,
but it probably doesn't matter a lot unless you have multiple NIC's using
NAPI in the same machine, which is not common since this chipset seems to be used
mostly in cheap boxes.

> 
> >  	dev->stop = rtl8139_close;
> >  	dev->get_stats = rtl8139_get_stats;
> >  	dev->set_multicast_list = rtl8139_set_rx_mode;
> > @@ -1879,24 +1877,28 @@
> >  #endif
> >  }
> >  
> > -static void rtl8139_rx_interrupt (struct net_device *dev,
> > -				  struct rtl8139_private *tp, void *ioaddr)
> > +/*
> > + * NAPI poll routine.
> > + */
> > +static int rtl8139_poll(struct net_device *dev, int *budget)
> >  {
> > +	struct rtl8139_private *tp = dev->priv;
> > +	void *ioaddr = tp->mmio_addr;
> >  	unsigned char *rx_ring;
> >  	u16 cur_rx;
> > +	int rx, status;
> > +	unsigned long flags;
> >  
> > -	assert (dev != NULL);
> > -	assert (tp != NULL);
> > -	assert (ioaddr != NULL);
> > -
> > +	spin_lock_irqsave(&tp->lock, flags);
> 
> I don't think lock of interrupt related path needed here. And this
> thinks that it has a bad influence on TX path.
> 
> Instead of it, I think we need synchronize with ->tx_timeout because
> ->tx_timeout touch IntrMask.

Locking in this version is more conservative than necessary, will think about
how to do it better.


> > + rescan:
> >  	rx_ring = tp->rx_ring;
> >  	cur_rx = tp->cur_rx;
> > -
> [...]
> >  			RTL_W16_F (IntrStatus, RxAckBits);
> > +
> > +		if (++rx >= dev->quota)
> 
> Shouldn't we use "min(dev->quota, *budget)" for this limit? Users can
> change "budget" via sysctl.

Once again, this not what the documentation, or some other drivers do.

> 
> > +	/* If received interrupt, then disable furthur interrupts
> > +	 * and enable NAPI polling.
> > +	 */
> > +	if (netif_running (dev) && (status & RxAckBits)) {
> > +		/* disable more receive interrupts */
> > +		RTL_W16 (IntrMask, rtl8139_intr_mask & ~RxAckBits);
> > +		netif_rx_schedule(dev);
> > +	}
> 
> This style have problem on shutdowning interface path. Please see
> 8139too-napi-stop-fix.patch.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 
> 

[RFT] merged 8139too NAPI

[Stephen Hemminger]

This version merges hirofumi's and my changes.  It includes:
	* fix DPRINTK() arguments that got out of date
	* likely/unlikely in interrupt path.
	* handle shared interrupts better.
	* need to block out bottom half in tx_timeout
	* exit receive loop if interface is brought down
	* avoid false activation of receive poll due to tx interrupts
	* don't need the local_irq_disable() in end of receive poll.

This patch is against 2.6.0-test8

--- linux-2.5/drivers/net/8139too.c	2003-10-20 11:49:13.000000000 -0700
+++ desktop-2.5/drivers/net/8139too.c	2003-10-20 12:06:12.000000000 -0700
@@ -92,7 +92,7 @@
 */
 
 #define DRV_NAME	"8139too"
-#define DRV_VERSION	"0.9.26"
+#define DRV_VERSION	"0.9.27"
 
 
 #include <linux/config.h>
@@ -159,9 +159,6 @@
 static int media[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 static int full_duplex[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 
-/* Maximum events (Rx packets, etc.) to handle at each interrupt. */
-static int max_interrupt_work = 20;
-
 /* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
@@ -572,6 +569,7 @@
 	signed char phys[4];		/* MII device addresses. */
 	char twistie, twist_row, twist_col;	/* Twister tune state. */
 	unsigned int default_port:4;	/* Last dev->if_port value. */
+	spinlock_t rx_lock;
 	spinlock_t lock;
 	chip_t chipset;
 	pid_t thr_pid;
@@ -589,13 +587,11 @@
 MODULE_LICENSE("GPL");
 
 MODULE_PARM (multicast_filter_limit, "i");
-MODULE_PARM (max_interrupt_work, "i");
 MODULE_PARM (media, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM (full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM (debug, "i");
 MODULE_PARM_DESC (debug, "8139too bitmapped message enable number");
 MODULE_PARM_DESC (multicast_filter_limit, "8139too maximum number of filtered multicast addresses");
-MODULE_PARM_DESC (max_interrupt_work, "8139too maximum events handled per interrupt");
 MODULE_PARM_DESC (media, "8139too: Bits 4+9: force full duplex, bit 5: 100Mbps");
 MODULE_PARM_DESC (full_duplex, "8139too: Force full duplex for board(s) (1)");
 
@@ -609,6 +605,7 @@
 static void rtl8139_init_ring (struct net_device *dev);
 static int rtl8139_start_xmit (struct sk_buff *skb,
 			       struct net_device *dev);
+static int rtl8139_poll(struct net_device *dev, int *budget);
 static irqreturn_t rtl8139_interrupt (int irq, void *dev_instance,
 			       struct pt_regs *regs);
 static int rtl8139_close (struct net_device *dev);
@@ -681,6 +678,10 @@
 	PCIErr | PCSTimeout | RxUnderrun | RxOverflow | RxFIFOOver |
 	TxErr | TxOK | RxErr | RxOK;
 
+static const u16 rtl8139_norx_intr_mask =
+	PCIErr | PCSTimeout | RxUnderrun |
+	TxErr | TxOK | RxErr ;
+
 #ifdef USE_BUF16K 
 static const unsigned int rtl8139_rx_config =
 	RxCfgRcv16K | RxNoWrap |
@@ -867,9 +868,7 @@
 
 match:
 	DPRINTK ("chipset id (%d) == index %d, '%s'\n",
-		tmp,
-		tp->chipset,
-		rtl_chip_info[tp->chipset].name);
+		 version, i, rtl_chip_info[i].name);
 
 	if (tp->chipset >= CH_8139B) {
 		u8 new_tmp8 = tmp8 = RTL_R8 (Config1);
@@ -963,6 +962,8 @@
 	/* The Rtl8139-specific entries in the device structure. */
 	dev->open = rtl8139_open;
 	dev->hard_start_xmit = rtl8139_start_xmit;
+	dev->poll = rtl8139_poll;
+	dev->weight = 64;
 	dev->stop = rtl8139_close;
 	dev->get_stats = rtl8139_get_stats;
 	dev->set_multicast_list = rtl8139_set_rx_mode;
@@ -985,6 +986,7 @@
 	/* note: tp->chipset set in rtl8139_init_board */
 	tp->drv_flags = board_info[ent->driver_data].hw_flags;
 	tp->mmio_addr = ioaddr;
+	spin_lock_init (&tp->rx_lock);
 	spin_lock_init (&tp->lock);
 	init_waitqueue_head (&tp->thr_wait);
 	init_completion (&tp->thr_exited);
@@ -1319,6 +1321,8 @@
 	rtl8139_init_ring (dev);
 	rtl8139_hw_start (dev);
 
+	netif_start_queue (dev);
+
 	DPRINTK ("%s: rtl8139_open() ioaddr %#lx IRQ %d"
 			" GP Pins %2.2x %s-duplex.\n",
 			dev->name, pci_resource_start (tp->pci_dev, 1),
@@ -1406,8 +1410,6 @@
 
 	/* Enable all known interrupts by setting the interrupt mask. */
 	RTL_W16 (IntrMask, rtl8139_intr_mask);
-
-	netif_start_queue (dev);
 }
 
 
@@ -1655,6 +1657,7 @@
 
 	tp->xstats.tx_timeouts++;
 
+	spin_lock_bh(&tp->rx_lock);
 	/* disable Tx ASAP, if not already */
 	tmp8 = RTL_R8 (ChipCmd);
 	if (tmp8 & CmdTxEnb)
@@ -1679,6 +1682,7 @@
 
 	/* ...and finally, reset everything */
 	rtl8139_hw_start (dev);
+	spin_unlock_bh(&tp->rx_lock);
 
 	netif_wake_queue (dev);
 }
@@ -1694,6 +1698,7 @@
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % NUM_TX_DESC;
 
+	/* Note: the chip doesn't have auto-pad! */
 	if (likely(len < TX_BUF_SIZE)) {
 		if (len < ETH_ZLEN)
 			memset(tp->tx_buf[entry], 0, ETH_ZLEN);
@@ -1705,7 +1710,6 @@
 		return 0;
 	}
 
-	/* Note: the chip doesn't have auto-pad! */
 	spin_lock_irq(&tp->lock);
 	RTL_W32_F (TxStatus0 + (entry * sizeof (u32)),
 		   tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
@@ -1791,8 +1795,7 @@
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
 		mb();
-		if (netif_queue_stopped (dev))
-			netif_wake_queue (dev);
+		netif_wake_queue (dev);
 	}
 }
 
@@ -1879,16 +1882,15 @@
 #endif
 }
 
-static void rtl8139_rx_interrupt (struct net_device *dev,
-				  struct rtl8139_private *tp, void *ioaddr)
+static inline int rtl8139_rx(struct net_device *dev, 
+			     struct rtl8139_private *tp,
+			     int budget)
 {
+	void *ioaddr = tp->mmio_addr;
+	int received = 0;
 	unsigned char *rx_ring;
 	u16 cur_rx;
 
-	assert (dev != NULL);
-	assert (tp != NULL);
-	assert (ioaddr != NULL);
-
 	rx_ring = tp->rx_ring;
 	cur_rx = tp->cur_rx;
 
@@ -1897,7 +1899,10 @@
 		 RTL_R16 (RxBufAddr),
 		 RTL_R16 (RxBufPtr), RTL_R8 (ChipCmd));
 
-	while ((RTL_R8 (ChipCmd) & RxBufEmpty) == 0) {
+rx_status_loop:
+	RTL_W16_F (IntrStatus, RxAckBits);
+
+	while (budget > 0 && (RTL_R8 (ChipCmd) & RxBufEmpty) == 0) {
 		int ring_offset = cur_rx % RX_BUF_LEN;
 		u32 rx_status;
 		unsigned int rx_size;
@@ -1929,21 +1934,27 @@
 		 * Theoretically, this should never happen
 		 * since EarlyRx is disabled.
 		 */
-		if (rx_size == 0xfff0) {
+		if (unlikely(rx_size == 0xfff0)) {
 			tp->xstats.early_rx++;
 			break;
 		}
 
+		/*
+		 * Shutting down, network.
+		 */
+		if (unlikely(!netif_running(dev)))
+			goto done;
+
 		/* If Rx err or invalid rx_size/rx_status received
 		 * (which happens if we get lost in the ring),
 		 * Rx process gets reset, so we abort any further
 		 * Rx processing.
 		 */
-		if ((rx_size > (MAX_ETH_FRAME_SIZE+4)) ||
-		    (rx_size < 8) ||
-		    (!(rx_status & RxStatusOK))) {
+		if (unlikely((rx_size > (MAX_ETH_FRAME_SIZE+4)) ||
+			     (rx_size < 8) ||
+			     (!(rx_status & RxStatusOK)))) {
 			rtl8139_rx_err (rx_status, dev, tp, ioaddr);
-			return;
+			return -1;
 		}
 
 		/* Malloc up new buffer, compatible with net-2e. */
@@ -1964,7 +1975,7 @@
 			skb_put (skb, pkt_size);
 
 			skb->protocol = eth_type_trans (skb, dev);
-			netif_rx (skb);
+			netif_receive_skb (skb);
 			dev->last_rx = jiffies;
 			tp->stats.rx_bytes += pkt_size;
 			tp->stats.rx_packets++;
@@ -1974,23 +1985,31 @@
 				dev->name);
 			tp->stats.rx_dropped++;
 		}
+		received++;
+		budget--;
 
 		cur_rx = (cur_rx + rx_size + 4 + 3) & ~3;
 		RTL_W16 (RxBufPtr, cur_rx - 16);
 
-		if (RTL_R16 (IntrStatus) & RxAckBits)
+		if (RTL_R16 (IntrStatus) & (RxFIFOOver | RxOverflow)) {
 			RTL_W16_F (IntrStatus, RxAckBits);
+			tp->stats.rx_errors++;
+		}
 	}
 
+	if (budget && (RTL_R16(IntrStatus) & RxAckBits))
+		goto rx_status_loop;
+ done:
+
 	DPRINTK ("%s: Done rtl8139_rx(), current %4.4x BufAddr %4.4x,"
 		 " free to %4.4x, Cmd %2.2x.\n", dev->name, cur_rx,
 		 RTL_R16 (RxBufAddr),
 		 RTL_R16 (RxBufPtr), RTL_R8 (ChipCmd));
 
 	tp->cur_rx = cur_rx;
+	return received;
 }
 
-
 static void rtl8139_weird_interrupt (struct net_device *dev,
 				     struct rtl8139_private *tp,
 				     void *ioaddr,
@@ -2013,13 +2032,11 @@
 		status &= ~RxUnderrun;
 	}
 
-	/* XXX along with rtl8139_rx_err, are we double-counting errors? */
-	if (status &
-	    (RxUnderrun | RxOverflow | RxErr | RxFIFOOver))
-		tp->stats.rx_errors++;
+	if (status & (RxUnderrun | RxErr))
+		tp->stats.rx_errors++;		/* race with rtl8139_rx_err */
 
 	if (status & PCSTimeout)
-		tp->stats.rx_length_errors++;
+		tp->stats.rx_length_errors++;	/* race with rtl8139_rx_err */
 	if (status & (RxUnderrun | RxFIFOOver))
 		tp->stats.rx_fifo_errors++;
 	if (status & PCIErr) {
@@ -2032,6 +2049,42 @@
 	}
 }
 
+static int rtl8139_poll(struct net_device *dev, int *budget)
+{
+	struct rtl8139_private *tp = dev->priv;
+	void *ioaddr = tp->mmio_addr;
+	u16 status;
+	int orig_budget = min(*budget, dev->quota);
+	int done = 1;
+
+	spin_lock(&tp->rx_lock);
+
+	status = RTL_R16(IntrStatus);
+
+	if (likely(status & RxAckBits)) {
+		int work_done;
+
+		work_done = rtl8139_rx(dev, tp, orig_budget);
+		if (likely(work_done > 0)) {
+			*budget -= work_done;
+			dev->quota -= work_done;
+			done = (work_done < orig_budget);
+		}
+	}
+
+	if (done) {
+		/*
+		 * This order is important
+		 * (see Documentation/networking/NAPI_HOWTO.txt)
+		 */
+		netif_rx_complete(dev);
+		RTL_W16_F(IntrMask, rtl8139_intr_mask);
+	}
+
+	spin_unlock(&tp->rx_lock);
+
+	return !done;
+}
 
 /* The interrupt handler does all of the Rx thread work and cleans up
    after the Tx thread. */
@@ -2040,76 +2093,56 @@
 {
 	struct net_device *dev = (struct net_device *) dev_instance;
 	struct rtl8139_private *tp = dev->priv;
-	int boguscnt = max_interrupt_work;
 	void *ioaddr = tp->mmio_addr;
-	int ackstat, status;
+	u16 status;
 	int link_changed = 0; /* avoid bogus "uninit" warning */
 	int handled = 0;
 
 	spin_lock (&tp->lock);
+	status = RTL_R16 (IntrStatus);
 
-	do {
-		status = RTL_R16 (IntrStatus);
-
-		/* h/w no longer present (hotplug?) or major error, bail */
-		if (status == 0xFFFF)
-			break;
-
-		if ((status &
-		     (PCIErr | PCSTimeout | RxUnderrun | RxOverflow |
-		      RxFIFOOver | TxErr | TxOK | RxErr | RxOK)) == 0)
-			break;
-
-		handled = 1;
-
-		/* Acknowledge all of the current interrupt sources ASAP, but
-		   an first get an additional status bit from CSCR. */
-		if (status & RxUnderrun)
-			link_changed = RTL_R16 (CSCR) & CSCR_LinkChangeBit;
-
-		/* The chip takes special action when we clear RxAckBits,
-		 * so we clear them later in rtl8139_rx_interrupt
+	/* shared irq? */
+	if (unlikely((status & rtl8139_intr_mask) == 0)) 
+		goto out;
+
+	handled = 1;
+
+	/* h/w no longer present (hotplug?) or major error, bail */
+	if (unlikely(status == 0xFFFF)) 
+		goto out;
+
+	/* Acknowledge all of the current interrupt sources ASAP, but
+	   an first get an additional status bit from CSCR. */
+	if (unlikely(status & RxUnderrun))
+		link_changed = RTL_R16 (CSCR) & CSCR_LinkChangeBit;
+
+	if (status & (RTL_R16 (IntrMask) & RxAckBits)) {
+		/* Disable furthur receive interrupts.
+		 * receive packets are processed by poll routine.
 		 */
-		ackstat = status & ~(RxAckBits | TxErr);
-		RTL_W16 (IntrStatus, ackstat);
-
-		DPRINTK ("%s: interrupt  status=%#4.4x ackstat=%#4.4x new intstat=%#4.4x.\n",
-			 dev->name, status, ackstat, RTL_R16 (IntrStatus));
-
-		if (netif_running (dev) && (status & RxAckBits))
-			rtl8139_rx_interrupt (dev, tp, ioaddr);
-
-		/* Check uncommon events with one test. */
-		if (status & (PCIErr | PCSTimeout | RxUnderrun | RxOverflow |
-		  	      RxFIFOOver | RxErr))
-			rtl8139_weird_interrupt (dev, tp, ioaddr,
-						 status, link_changed);
-
-		if (netif_running (dev) && (status & (TxOK | TxErr))) {
-			rtl8139_tx_interrupt (dev, tp, ioaddr);
-			if (status & TxErr)
-				RTL_W16 (IntrStatus, TxErr);
-		}
-
-		boguscnt--;
-	} while (boguscnt > 0);
+		RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
+		netif_rx_schedule (dev);
+	}
 
-	if (boguscnt <= 0) {
-		printk (KERN_WARNING "%s: Too much work at interrupt, "
-			"IntrStatus=0x%4.4x.\n", dev->name, status);
+	/* Check uncommon events with one test. */
+	if (unlikely(status & (PCIErr | PCSTimeout | RxUnderrun | RxOverflow)))
+		rtl8139_weird_interrupt (dev, tp, ioaddr,
+					 status, link_changed);
 
-		/* Clear all interrupt sources. */
-		RTL_W16 (IntrStatus, 0xffff);
+	if (netif_running (dev) && (status & (TxOK | TxErr))) {
+		rtl8139_tx_interrupt (dev, tp, ioaddr);
+		if (status & TxErr)
+			RTL_W16 (IntrStatus, TxErr);
 	}
 
+	RTL_W16_F (IntrStatus, status & ~(RxAckBits | TxErr));
+
+ out:
 	spin_unlock (&tp->lock);
 
-	DPRINTK ("%s: exiting interrupt, intr_status=%#4.4x.\n",
-		 dev->name, RTL_R16 (IntrStatus));
 	return IRQ_RETVAL(handled);
 }
 
-
 static int rtl8139_close (struct net_device *dev)
 {
 	struct rtl8139_private *tp = dev->priv;
@@ -2493,6 +2526,7 @@
 	pci_set_power_state (pdev, 0);
 	rtl8139_init_ring (dev);
 	rtl8139_hw_start (dev);
+	netif_start_queue (dev);
 	netif_device_attach (dev);
 	return 0;
 }

Re: [RFT] merged 8139too NAPI

[OGAWA Hirofumi]

Stephen Hemminger <shemminger@osdl.org> writes:

> +	spin_lock_bh(&tp->rx_lock);
>  	/* disable Tx ASAP, if not already */
>  	tmp8 = RTL_R8 (ChipCmd);
>  	if (tmp8 & CmdTxEnb)
> @@ -1679,6 +1682,7 @@
>  
>  	/* ...and finally, reset everything */
>  	rtl8139_hw_start (dev);
> +	spin_unlock_bh(&tp->rx_lock);

->tx_timeout is on timer softirq context. So softirq was disabled already.
Or Is this for future changes?

> +	/* Check uncommon events with one test. */
> +	if (unlikely(status & (PCIErr | PCSTimeout | RxUnderrun | RxOverflow)))
> +		rtl8139_weird_interrupt (dev, tp, ioaddr,
> +					 status, link_changed);

I couldn't find this on previous email, rtl8139_weird_interrupt was changing. 

	if (unlikely(status & (PCIErr|PCSTimeout|RxUnderrun|RxErr|RxFIFOOver)))
		rtl8139_weird_interrupt(dev, tp, ioaddr, status, link_changed);

It should check the above status.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCH] Updated 8139too with NAPI

[Stephen Hemminger]

Jeff, please review this for *after* 2.6.0 goes final.

This version of the 8139too driver uses NAPI for receive and a 64k window.
Some changes since the previous version are to handle suspend/resume of polling
receive, and to add a version netif_poll_disable that spins for use during
the transmit timeout processing.

--- 2.6.0-test9/drivers/net/8139too.c	2003-10-27 12:09:03.000000000 -0800
+++ linux-2.5/drivers/net/8139too.c	2003-10-28 10:34:51.000000000 -0800
@@ -92,7 +92,7 @@
 */
 
 #define DRV_NAME	"8139too"
-#define DRV_VERSION	"0.9.26"
+#define DRV_VERSION	"0.9.27"
 
 
 #include <linux/config.h>
@@ -123,12 +123,12 @@
 #define USE_IO_OPS 1
 #endif
 
-/* use a 16K rx ring buffer instead of the default 32K */
-#ifdef CONFIG_SH_DREAMCAST
+/* use a 16K rx ring buffer instead of the default 64K */
+#if defined(CONFIG_SH_DREAMCAST) || defined(CONFIG_EMBEDDED)
 #define USE_BUF16K 1
 #endif
 
-/* define to 1 to enable copious debugging info */
+/* define to 1 to some debug, 2 lots */
 #undef RTL8139_DEBUG
 
 /* define to 1 to disable lightweight runtime debugging checks */
@@ -145,11 +145,7 @@
 #ifdef RTL8139_NDEBUG
 #  define assert(expr) do {} while (0)
 #else
-#  define assert(expr) \
-        if(!(expr)) {					\
-        printk( "Assertion failed! %s,%s,%s,line=%d\n",	\
-        #expr,__FILE__,__FUNCTION__,__LINE__);		\
-        }
+#  define assert(expr) BUG_ON((expr) == 0)
 #endif
 
 
@@ -159,9 +155,6 @@
 static int media[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 static int full_duplex[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 
-/* Maximum events (Rx packets, etc.) to handle at each interrupt. */
-static int max_interrupt_work = 20;
-
 /* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
@@ -170,11 +163,15 @@ static int multicast_filter_limit = 32;
 static int debug = -1;
 
 /* Size of the in-memory receive ring. */
+/* 0==8K, 1==16K, 2==32K, 3==64K */
 #ifdef USE_BUF16K
-#define RX_BUF_LEN_IDX	1	/* 0==8K, 1==16K, 2==32K, 3==64K */
+#define RX_BUF_LEN_IDX	1
+#elif defined(USE_BUF32K)
+#define RX_BUF_LEN_IDX	2
 #else
-#define RX_BUF_LEN_IDX	2	/* 0==8K, 1==16K, 2==32K, 3==64K */
+#define RX_BUF_LEN_IDX	3
 #endif
+
 #define RX_BUF_LEN	(8192 << RX_BUF_LEN_IDX)
 #define RX_BUF_PAD	16
 #define RX_BUF_WRAP_PAD 2048 /* spare padding to handle lack of packet wrap */
@@ -589,13 +586,11 @@ MODULE_DESCRIPTION ("RealTek RTL-8139 Fa
 MODULE_LICENSE("GPL");
 
 MODULE_PARM (multicast_filter_limit, "i");
-MODULE_PARM (max_interrupt_work, "i");
 MODULE_PARM (media, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM (full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM (debug, "i");
 MODULE_PARM_DESC (debug, "8139too bitmapped message enable number");
 MODULE_PARM_DESC (multicast_filter_limit, "8139too maximum number of filtered multicast addresses");
-MODULE_PARM_DESC (max_interrupt_work, "8139too maximum events handled per interrupt");
 MODULE_PARM_DESC (media, "8139too: Bits 4+9: force full duplex, bit 5: 100Mbps");
 MODULE_PARM_DESC (full_duplex, "8139too: Force full duplex for board(s) (1)");
 
@@ -609,6 +604,7 @@ static void rtl8139_tx_timeout (struct n
 static void rtl8139_init_ring (struct net_device *dev);
 static int rtl8139_start_xmit (struct sk_buff *skb,
 			       struct net_device *dev);
+static int rtl8139_poll(struct net_device *dev, int *budget);
 static irqreturn_t rtl8139_interrupt (int irq, void *dev_instance,
 			       struct pt_regs *regs);
 static int rtl8139_close (struct net_device *dev);
@@ -681,16 +677,25 @@ static const u16 rtl8139_intr_mask =
 	PCIErr | PCSTimeout | RxUnderrun | RxOverflow | RxFIFOOver |
 	TxErr | TxOK | RxErr | RxOK;
 
+static const u16 rtl8139_norx_intr_mask =
+	PCIErr | PCSTimeout | RxUnderrun |
+	TxErr | TxOK | RxErr ;
+
 #ifdef USE_BUF16K 
 static const unsigned int rtl8139_rx_config =
 	RxCfgRcv16K | RxNoWrap |
 	(RX_FIFO_THRESH << RxCfgFIFOShift) |
 	(RX_DMA_BURST << RxCfgDMAShift);
-#else
+#elif defined(USE_BUF32K)
 static const unsigned int rtl8139_rx_config =
 	RxCfgRcv32K | RxNoWrap |
 	(RX_FIFO_THRESH << RxCfgFIFOShift) |
 	(RX_DMA_BURST << RxCfgDMAShift);
+#else
+static const unsigned int rtl8139_rx_config =
+	RxCfgRcv64K | RxNoWrap |
+	(RX_FIFO_THRESH << RxCfgFIFOShift) |
+	(RX_DMA_BURST << RxCfgDMAShift);
 #endif
 
 static const unsigned int rtl8139_tx_config =
@@ -867,9 +872,7 @@ static int __devinit rtl8139_init_board 
 
 match:
 	DPRINTK ("chipset id (%d) == index %d, '%s'\n",
-		tmp,
-		tp->chipset,
-		rtl_chip_info[tp->chipset].name);
+		 version, i, rtl_chip_info[i].name);
 
 	if (tp->chipset >= CH_8139B) {
 		u8 new_tmp8 = tmp8 = RTL_R8 (Config1);
@@ -963,6 +966,8 @@ static int __devinit rtl8139_init_one (s
 	/* The Rtl8139-specific entries in the device structure. */
 	dev->open = rtl8139_open;
 	dev->hard_start_xmit = rtl8139_start_xmit;
+	dev->poll = rtl8139_poll;
+	dev->weight = 64;
 	dev->stop = rtl8139_close;
 	dev->get_stats = rtl8139_get_stats;
 	dev->set_multicast_list = rtl8139_set_rx_mode;
@@ -1630,7 +1635,7 @@ static inline void rtl8139_start_thread(
 	}
 }
 
-static void rtl8139_tx_clear (struct rtl8139_private *tp)
+static inline void rtl8139_tx_clear (struct rtl8139_private *tp)
 {
 	tp->cur_tx = 0;
 	tp->dirty_tx = 0;
@@ -1639,6 +1644,15 @@ static void rtl8139_tx_clear (struct rtl
 }
 
 
+/* spin while poll is running
+ * could use netif_poll_disable but that sleeps.
+ */
+static inline void __netif_poll_disable(struct net_device *dev)
+{
+	while (test_and_set_bit(__LINK_STATE_RX_SCHED, &dev->state)) 
+		cpu_relax();
+}
+
 static void rtl8139_tx_timeout (struct net_device *dev)
 {
 	struct rtl8139_private *tp = dev->priv;
@@ -1663,6 +1677,8 @@ static void rtl8139_tx_timeout (struct n
 	/* Disable interrupts by clearing the interrupt mask. */
 	RTL_W16 (IntrMask, 0x0000);
 
+	__netif_poll_disable(dev);
+
 	/* Emit info to figure out what went wrong. */
 	printk (KERN_DEBUG "%s: Tx queue start entry %ld  dirty entry %ld.\n",
 		dev->name, tp->cur_tx, tp->dirty_tx);
@@ -1678,7 +1694,10 @@ static void rtl8139_tx_timeout (struct n
 	spin_unlock_irqrestore (&tp->lock, flags);
 
 	/* ...and finally, reset everything */
-	rtl8139_hw_start (dev);
+	if (netif_running(dev))
+		rtl8139_hw_start (dev);
+
+	netif_poll_enable (dev);
 
 	netif_wake_queue (dev);
 }
@@ -1694,6 +1713,7 @@ static int rtl8139_start_xmit (struct sk
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % NUM_TX_DESC;
 
+	/* Note: the chip doesn't have auto-pad! */
 	if (likely(len < TX_BUF_SIZE)) {
 		if (len < ETH_ZLEN)
 			memset(tp->tx_buf[entry], 0, ETH_ZLEN);
@@ -1705,7 +1725,6 @@ static int rtl8139_start_xmit (struct sk
 		return 0;
 	}
 
-	/* Note: the chip doesn't have auto-pad! */
 	spin_lock_irq(&tp->lock);
 	RTL_W32_F (TxStatus0 + (entry * sizeof (u32)),
 		   tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
@@ -1719,8 +1738,10 @@ static int rtl8139_start_xmit (struct sk
 		netif_stop_queue (dev);
 	spin_unlock_irq(&tp->lock);
 
+#if RTL8139_DEBUG > 2
 	DPRINTK ("%s: Queued Tx packet size %u to slot %d.\n",
 		 dev->name, len, entry);
+#endif
 
 	return 0;
 }
@@ -1791,8 +1812,7 @@ static void rtl8139_tx_interrupt (struct
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
 		mb();
-		if (netif_queue_stopped (dev))
-			netif_wake_queue (dev);
+		netif_wake_queue (dev);
 	}
 }
 
@@ -1879,59 +1899,50 @@ static void rtl8139_rx_err (u32 rx_statu
 #endif
 }
 
-static void rtl8139_rx_interrupt (struct net_device *dev,
-				  struct rtl8139_private *tp, void *ioaddr)
+static int rtl8139_rx(struct net_device *dev, struct rtl8139_private *tp,
+		      int budget)
 {
-	unsigned char *rx_ring;
-	u16 cur_rx;
-
-	assert (dev != NULL);
-	assert (tp != NULL);
-	assert (ioaddr != NULL);
-
-	rx_ring = tp->rx_ring;
-	cur_rx = tp->cur_rx;
+	void *ioaddr = tp->mmio_addr;
+	int received = 0;
+	const unsigned char *rx_ring = tp->rx_ring;
+	unsigned int cur_rx = tp->cur_rx;
+	unsigned int last = tp->cur_rx;
 
+#if RTL8139_DEBUG > 1
 	DPRINTK ("%s: In rtl8139_rx(), current %4.4x BufAddr %4.4x,"
 		 " free to %4.4x, Cmd %2.2x.\n", dev->name, cur_rx,
 		 RTL_R16 (RxBufAddr),
 		 RTL_R16 (RxBufPtr), RTL_R8 (ChipCmd));
+#endif
 
-	while ((RTL_R8 (ChipCmd) & RxBufEmpty) == 0) {
-		int ring_offset = cur_rx % RX_BUF_LEN;
+	while (netif_running(dev) && received < budget 
+	       && (RTL_R8 (ChipCmd) & RxBufEmpty) == 0) {
 		u32 rx_status;
 		unsigned int rx_size;
 		unsigned int pkt_size;
 		struct sk_buff *skb;
+		u16 status;
 
 		rmb();
 
 		/* read size+status of next frame from DMA ring buffer */
-		rx_status = le32_to_cpu (*(u32 *) (rx_ring + ring_offset));
+		rx_status = le32_to_cpu (*(u32 *) (rx_ring + cur_rx));
 		rx_size = rx_status >> 16;
 		pkt_size = rx_size - 4;
 
+#if RTL8139_DEBUG > 2
 		DPRINTK ("%s:  rtl8139_rx() status %4.4x, size %4.4x,"
 			 " cur %4.4x.\n", dev->name, rx_status,
 			 rx_size, cur_rx);
-#if RTL8139_DEBUG > 2
-		{
-			int i;
-			DPRINTK ("%s: Frame contents ", dev->name);
-			for (i = 0; i < 70; i++)
-				printk (" %2.2x",
-					rx_ring[ring_offset + i]);
-			printk (".\n");
-		}
 #endif
 
 		/* Packet copy from FIFO still in progress.
 		 * Theoretically, this should never happen
 		 * since EarlyRx is disabled.
 		 */
-		if (rx_size == 0xfff0) {
+		if (unlikely(rx_size == 0xfff0)) {
 			tp->xstats.early_rx++;
-			break;
+			goto done;
 		}
 
 		/* If Rx err or invalid rx_size/rx_status received
@@ -1939,55 +1950,69 @@ static void rtl8139_rx_interrupt (struct
 		 * Rx process gets reset, so we abort any further
 		 * Rx processing.
 		 */
-		if ((rx_size > (MAX_ETH_FRAME_SIZE+4)) ||
-		    (rx_size < 8) ||
-		    (!(rx_status & RxStatusOK))) {
+		if (unlikely((rx_size > (MAX_ETH_FRAME_SIZE+4)) ||
+			     (rx_size < 8) ||
+			     (!(rx_status & RxStatusOK)))) {
 			rtl8139_rx_err (rx_status, dev, tp, ioaddr);
-			return;
+			return -1;
 		}
 
 		/* Malloc up new buffer, compatible with net-2e. */
 		/* Omit the four octet CRC from the length. */
 
-		/* TODO: consider allocating skb's outside of
-		 * interrupt context, both to speed interrupt processing,
-		 * and also to reduce the chances of having to
-		 * drop packets here under memory pressure.
-		 */
-
 		skb = dev_alloc_skb (pkt_size + 2);
-		if (skb) {
+		if (likely(skb)) {
 			skb->dev = dev;
 			skb_reserve (skb, 2);	/* 16 byte align the IP fields. */
-
-			eth_copy_and_sum (skb, &rx_ring[ring_offset + 4], pkt_size, 0);
+			memcpy(skb->data, &rx_ring[cur_rx + 4], pkt_size);
 			skb_put (skb, pkt_size);
 
 			skb->protocol = eth_type_trans (skb, dev);
-			netif_rx (skb);
+
 			dev->last_rx = jiffies;
 			tp->stats.rx_bytes += pkt_size;
 			tp->stats.rx_packets++;
+
+			netif_receive_skb (skb);
 		} else {
-			printk (KERN_WARNING
-				"%s: Memory squeeze, dropping packet.\n",
-				dev->name);
+			if (net_ratelimit()) 
+				printk (KERN_WARNING
+					"%s: Memory squeeze, dropping packet.\n",
+					dev->name);
 			tp->stats.rx_dropped++;
 		}
+		received++;
 
-		cur_rx = (cur_rx + rx_size + 4 + 3) & ~3;
-		RTL_W16 (RxBufPtr, cur_rx - 16);
+		cur_rx = ((cur_rx + rx_size + 4 + 3) & ~3) % RX_BUF_LEN;
+		RTL_W16 (RxBufPtr, (u16) (cur_rx - 16));
 
-		if (RTL_R16 (IntrStatus) & RxAckBits)
+		if (last > cur_rx) 
+			printk(" crossing %x -> %x\n", last, cur_rx);
+		last = cur_rx;
+
+		/* Clear out errors and receive interrupts */
+		status = RTL_R16 (IntrStatus) & RxAckBits;
+		if (likely(status != 0)) {
+			if (unlikely(status & (RxFIFOOver | RxOverflow))) {
+				tp->stats.rx_errors++;
+				if (status & RxFIFOOver)
+					tp->stats.rx_fifo_errors++;
+			}
 			RTL_W16_F (IntrStatus, RxAckBits);
+		}
 	}
 
+ done:
+
+#if RTL8139_DEBUG > 1
 	DPRINTK ("%s: Done rtl8139_rx(), current %4.4x BufAddr %4.4x,"
 		 " free to %4.4x, Cmd %2.2x.\n", dev->name, cur_rx,
 		 RTL_R16 (RxBufAddr),
 		 RTL_R16 (RxBufPtr), RTL_R8 (ChipCmd));
+#endif
 
 	tp->cur_rx = cur_rx;
+	return received;
 }
 
 
@@ -2013,14 +2038,11 @@ static void rtl8139_weird_interrupt (str
 		status &= ~RxUnderrun;
 	}
 
-	/* XXX along with rtl8139_rx_err, are we double-counting errors? */
-	if (status &
-	    (RxUnderrun | RxOverflow | RxErr | RxFIFOOver))
+	if (status & (RxUnderrun | RxErr))
 		tp->stats.rx_errors++;
-
 	if (status & PCSTimeout)
-		tp->stats.rx_length_errors++;
-	if (status & (RxUnderrun | RxFIFOOver))
+		tp->stats.rx_length_errors++;	/* race with rtl8139_rx_err */
+	if (status & RxUnderrun)
 		tp->stats.rx_fifo_errors++;
 	if (status & PCIErr) {
 		u16 pci_cmd_status;
@@ -2032,6 +2054,35 @@ static void rtl8139_weird_interrupt (str
 	}
 }
 
+static int rtl8139_poll(struct net_device *dev, int *budget)
+{
+	struct rtl8139_private *tp = dev->priv;
+	void *ioaddr = tp->mmio_addr;
+	int orig_budget = min(*budget, dev->quota);
+	int done = 1;
+
+	if (likely(RTL_R16(IntrStatus) & RxAckBits)) {
+		int work_done;
+
+		work_done = rtl8139_rx(dev, tp, orig_budget);
+		if (likely(work_done > 0)) {
+			*budget -= work_done;
+			dev->quota -= work_done;
+			done = (work_done < orig_budget);
+		}
+	}
+
+	if (done) {
+		/*
+		 * This order is important
+		 * (see Documentation/networking/NAPI_HOWTO.txt)
+		 */
+		netif_rx_complete(dev);
+		RTL_W16_F(IntrMask, rtl8139_intr_mask);
+	}
+
+	return !done;
+}
 
 /* The interrupt handler does all of the Rx thread work and cleans up
    after the Tx thread. */
@@ -2040,68 +2091,53 @@ static irqreturn_t rtl8139_interrupt (in
 {
 	struct net_device *dev = (struct net_device *) dev_instance;
 	struct rtl8139_private *tp = dev->priv;
-	int boguscnt = max_interrupt_work;
 	void *ioaddr = tp->mmio_addr;
-	int ackstat, status;
+	u16 status, ackstat;
 	int link_changed = 0; /* avoid bogus "uninit" warning */
 	int handled = 0;
 
 	spin_lock (&tp->lock);
+	status = RTL_R16 (IntrStatus);
 
-	do {
-		status = RTL_R16 (IntrStatus);
-
-		/* h/w no longer present (hotplug?) or major error, bail */
-		if (status == 0xFFFF)
-			break;
+	/* shared irq? */
+	if (unlikely((status & rtl8139_intr_mask) == 0)) 
+		goto out;
 
-		if ((status &
-		     (PCIErr | PCSTimeout | RxUnderrun | RxOverflow |
-		      RxFIFOOver | TxErr | TxOK | RxErr | RxOK)) == 0)
-			break;
+	handled = 1;
 
-		handled = 1;
+	/* h/w no longer present (hotplug?) or major error, bail */
+	if (unlikely(status == 0xFFFF)) 
+		goto out;
 
-		/* Acknowledge all of the current interrupt sources ASAP, but
-		   an first get an additional status bit from CSCR. */
-		if (status & RxUnderrun)
-			link_changed = RTL_R16 (CSCR) & CSCR_LinkChangeBit;
+	/* Acknowledge all of the current interrupt sources ASAP, but
+	   an first get an additional status bit from CSCR. */
+	if (unlikely(status & RxUnderrun))
+		link_changed = RTL_R16 (CSCR) & CSCR_LinkChangeBit;
 
-		/* The chip takes special action when we clear RxAckBits,
-		 * so we clear them later in rtl8139_rx_interrupt
-		 */
-		ackstat = status & ~(RxAckBits | TxErr);
+	ackstat = status & ~(RxAckBits | TxErr);
+	if (ackstat)
 		RTL_W16 (IntrStatus, ackstat);
 
-		DPRINTK ("%s: interrupt  status=%#4.4x ackstat=%#4.4x new intstat=%#4.4x.\n",
-			 dev->name, status, ackstat, RTL_R16 (IntrStatus));
-
-		if (netif_running (dev) && (status & RxAckBits))
-			rtl8139_rx_interrupt (dev, tp, ioaddr);
-
-		/* Check uncommon events with one test. */
-		if (status & (PCIErr | PCSTimeout | RxUnderrun | RxOverflow |
-		  	      RxFIFOOver | RxErr))
-			rtl8139_weird_interrupt (dev, tp, ioaddr,
-						 status, link_changed);
-
-		if (netif_running (dev) && (status & (TxOK | TxErr))) {
-			rtl8139_tx_interrupt (dev, tp, ioaddr);
-			if (status & TxErr)
-				RTL_W16 (IntrStatus, TxErr);
+	/* Receive packets are processed by poll routine.
+	   If not running start it now. */
+	if (status & RxAckBits){
+		if (netif_rx_schedule_prep(dev)) {
+			RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
+			__netif_rx_schedule (dev);
 		}
-
-		boguscnt--;
-	} while (boguscnt > 0);
-
-	if (boguscnt <= 0) {
-		printk (KERN_WARNING "%s: Too much work at interrupt, "
-			"IntrStatus=0x%4.4x.\n", dev->name, status);
-
-		/* Clear all interrupt sources. */
-		RTL_W16 (IntrStatus, 0xffff);
 	}
 
+	/* Check uncommon events with one test. */
+	if (unlikely(status & (PCIErr | PCSTimeout | RxUnderrun | RxErr)))
+		rtl8139_weird_interrupt (dev, tp, ioaddr,
+					 status, link_changed);
+
+	if (netif_running (dev) && (status & (TxOK | TxErr))) {
+		rtl8139_tx_interrupt (dev, tp, ioaddr);
+		if (status & TxErr)
+			RTL_W16 (IntrStatus, TxErr);
+	}
+ out:
 	spin_unlock (&tp->lock);
 
 	DPRINTK ("%s: exiting interrupt, intr_status=%#4.4x.\n",
@@ -2445,9 +2481,11 @@ static void rtl8139_set_rx_mode (struct 
 	unsigned long flags;
 	struct rtl8139_private *tp = dev->priv;
 
+	netif_poll_disable (dev);
 	spin_lock_irqsave (&tp->lock, flags);
 	__set_rx_mode(dev);
 	spin_unlock_irqrestore (&tp->lock, flags);
+	netif_poll_enable (dev);
 }
 
 #ifdef CONFIG_PM
@@ -2463,6 +2501,7 @@ static int rtl8139_suspend (struct pci_d
 		return 0;
 
 	netif_device_detach (dev);
+	netif_poll_disable(dev);
 
 	spin_lock_irqsave (&tp->lock, flags);
 
@@ -2494,6 +2533,7 @@ static int rtl8139_resume (struct pci_de
 	rtl8139_init_ring (dev);
 	rtl8139_hw_start (dev);
 	netif_device_attach (dev);
+	netif_poll_enable (dev);
 	return 0;
 }
 

Re: [PATCH] Updated 8139too with NAPI

[Jeff Garzik]

I'd like to apply this to net-drivers-2.[45]-exp, but it doesn't apply 
cleanly.  Can you wait a bit, for my posting of those queues, and then 
resend?

Re: [PATCH] Updated 8139too with NAPI

[OGAWA Hirofumi]

Jeff Garzik <jgarzik@pobox.com> writes:

> I'd like to apply this to net-drivers-2.[45]-exp, but it doesn't apply
> cleanly.  Can you wait a bit, for my posting of those queues, and then
> resend?

Probably more tester/reviewer is always welcome, so I think it's good.


Stephen Hemminger <shemminger@osdl.org> writes:

> +	/* Receive packets are processed by poll routine.
> +	   If not running start it now. */
> +	if (status & RxAckBits){
> +		if (netif_rx_schedule_prep(dev)) {
> +			RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
> +			__netif_rx_schedule (dev);
>  		}

I think that this style is common issues of NIC drivers for "NAPI
support and intr-mask" handling. Looks like __netif_poll_disable()
also has the following problem.

For example,

	in ISR
		[...]
		if (status & RxAckBits){
			if (netif_rx_schedule_prep(dev)) {
				RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
				__netif_rx_schedule (dev);
			}
		}
		[...]

	in dev_close()
		[...]
		while (test_bit(__LINK_STATE_RX_SCHED, &dev->state)) {
			/* No hurry. */
			current->state = TASK_INTERRUPTIBLE;
			schedule_timeout(1);
		}
		  --------- (*)
		if (dev->stop)
			dev->stop(dev);
		[...]

If Rx interrupt happen on (*) point, ISR can't ack or disable.
Then it will start busy Rx interrupt loop. So if this happen, it's
seriously problem on UP system.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Updated 8139too with NAPI

[Stephen Hemminger]

Here is the latest update for 8139too with NAPI.

The changes from last time were to get rid of some of the netif_poll_disable calls
to avoid getting trapped by receive interrupt races.  It is okay for the interrupt
code to enable the poll routine to run even if we are just about to suspend or
change multicast list.

--- 2.6.0-test9/drivers/net/8139too.c	2003-10-27 12:09:03.000000000 -0800
+++ linux-2.5/drivers/net/8139too.c	2003-11-11 09:16:42.000000000 -0800
@@ -92,7 +92,7 @@
 */
 
 #define DRV_NAME	"8139too"
-#define DRV_VERSION	"0.9.26"
+#define DRV_VERSION	"0.9.27"
 
 
 #include <linux/config.h>
@@ -123,8 +123,8 @@
 #define USE_IO_OPS 1
 #endif
 
-/* use a 16K rx ring buffer instead of the default 32K */
-#ifdef CONFIG_SH_DREAMCAST
+/* use a 16K rx ring buffer instead of the default 64K */
+#if defined(CONFIG_SH_DREAMCAST) || defined(CONFIG_EMBEDDED)
 #define USE_BUF16K 1
 #endif
 
@@ -145,11 +145,7 @@
 #ifdef RTL8139_NDEBUG
 #  define assert(expr) do {} while (0)
 #else
-#  define assert(expr) \
-        if(!(expr)) {					\
-        printk( "Assertion failed! %s,%s,%s,line=%d\n",	\
-        #expr,__FILE__,__FUNCTION__,__LINE__);		\
-        }
+#  define assert(expr) BUG_ON((expr) == 0)
 #endif
 
 
@@ -159,9 +155,6 @@
 static int media[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 static int full_duplex[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 
-/* Maximum events (Rx packets, etc.) to handle at each interrupt. */
-static int max_interrupt_work = 20;
-
 /* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
@@ -170,11 +163,15 @@
 static int debug = -1;
 
 /* Size of the in-memory receive ring. */
+/* 0==8K, 1==16K, 2==32K, 3==64K */
 #ifdef USE_BUF16K
-#define RX_BUF_LEN_IDX	1	/* 0==8K, 1==16K, 2==32K, 3==64K */
+#define RX_BUF_LEN_IDX	1
+#elif defined(USE_BUF32K)
+#define RX_BUF_LEN_IDX	2
 #else
-#define RX_BUF_LEN_IDX	2	/* 0==8K, 1==16K, 2==32K, 3==64K */
+#define RX_BUF_LEN_IDX	3
 #endif
+
 #define RX_BUF_LEN	(8192 << RX_BUF_LEN_IDX)
 #define RX_BUF_PAD	16
 #define RX_BUF_WRAP_PAD 2048 /* spare padding to handle lack of packet wrap */
@@ -245,6 +242,7 @@
 	{0x1186, 0x1340, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
 	{0x13d1, 0xab06, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
 	{0x1259, 0xa117, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
+	{0x1259, 0xa11e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
 	{0x14ea, 0xab06, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
 	{0x14ea, 0xab07, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
 	{0x11db, 0x1234, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
@@ -589,13 +587,11 @@
 MODULE_LICENSE("GPL");
 
 MODULE_PARM (multicast_filter_limit, "i");
-MODULE_PARM (max_interrupt_work, "i");
 MODULE_PARM (media, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM (full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM (debug, "i");
 MODULE_PARM_DESC (debug, "8139too bitmapped message enable number");
 MODULE_PARM_DESC (multicast_filter_limit, "8139too maximum number of filtered multicast addresses");
-MODULE_PARM_DESC (max_interrupt_work, "8139too maximum events handled per interrupt");
 MODULE_PARM_DESC (media, "8139too: Bits 4+9: force full duplex, bit 5: 100Mbps");
 MODULE_PARM_DESC (full_duplex, "8139too: Force full duplex for board(s) (1)");
 
@@ -609,6 +605,7 @@
 static void rtl8139_init_ring (struct net_device *dev);
 static int rtl8139_start_xmit (struct sk_buff *skb,
 			       struct net_device *dev);
+static int rtl8139_poll(struct net_device *dev, int *budget);
 static irqreturn_t rtl8139_interrupt (int irq, void *dev_instance,
 			       struct pt_regs *regs);
 static int rtl8139_close (struct net_device *dev);
@@ -681,16 +678,25 @@
 	PCIErr | PCSTimeout | RxUnderrun | RxOverflow | RxFIFOOver |
 	TxErr | TxOK | RxErr | RxOK;
 
+static const u16 rtl8139_norx_intr_mask =
+	PCIErr | PCSTimeout | RxUnderrun |
+	TxErr | TxOK | RxErr ;
+
 #ifdef USE_BUF16K 
 static const unsigned int rtl8139_rx_config =
 	RxCfgRcv16K | RxNoWrap |
 	(RX_FIFO_THRESH << RxCfgFIFOShift) |
 	(RX_DMA_BURST << RxCfgDMAShift);
-#else
+#elif defined(USE_BUF32K)
 static const unsigned int rtl8139_rx_config =
 	RxCfgRcv32K | RxNoWrap |
 	(RX_FIFO_THRESH << RxCfgFIFOShift) |
 	(RX_DMA_BURST << RxCfgDMAShift);
+#else
+static const unsigned int rtl8139_rx_config =
+	RxCfgRcv64K | RxNoWrap |
+	(RX_FIFO_THRESH << RxCfgFIFOShift) |
+	(RX_DMA_BURST << RxCfgDMAShift);
 #endif
 
 static const unsigned int rtl8139_tx_config =
@@ -867,9 +873,7 @@
 
 match:
 	DPRINTK ("chipset id (%d) == index %d, '%s'\n",
-		tmp,
-		tp->chipset,
-		rtl_chip_info[tp->chipset].name);
+		 version, i, rtl_chip_info[i].name);
 
 	if (tp->chipset >= CH_8139B) {
 		u8 new_tmp8 = tmp8 = RTL_R8 (Config1);
@@ -963,6 +967,8 @@
 	/* The Rtl8139-specific entries in the device structure. */
 	dev->open = rtl8139_open;
 	dev->hard_start_xmit = rtl8139_start_xmit;
+	dev->poll = rtl8139_poll;
+	dev->weight = 64;
 	dev->stop = rtl8139_close;
 	dev->get_stats = rtl8139_get_stats;
 	dev->set_multicast_list = rtl8139_set_rx_mode;
@@ -1630,7 +1636,7 @@
 	}
 }
 
-static void rtl8139_tx_clear (struct rtl8139_private *tp)
+static inline void rtl8139_tx_clear (struct rtl8139_private *tp)
 {
 	tp->cur_tx = 0;
 	tp->dirty_tx = 0;
@@ -1639,6 +1645,15 @@
 }
 
 
+/* spin while poll is running
+ * could use netif_poll_disable but that sleeps.
+ */
+static inline void __netif_poll_disable(struct net_device *dev)
+{
+	while (test_and_set_bit(__LINK_STATE_RX_SCHED, &dev->state)) 
+		cpu_relax();
+}
+
 static void rtl8139_tx_timeout (struct net_device *dev)
 {
 	struct rtl8139_private *tp = dev->priv;
@@ -1663,6 +1678,8 @@
 	/* Disable interrupts by clearing the interrupt mask. */
 	RTL_W16 (IntrMask, 0x0000);
 
+	__netif_poll_disable(dev);
+
 	/* Emit info to figure out what went wrong. */
 	printk (KERN_DEBUG "%s: Tx queue start entry %ld  dirty entry %ld.\n",
 		dev->name, tp->cur_tx, tp->dirty_tx);
@@ -1678,9 +1695,11 @@
 	spin_unlock_irqrestore (&tp->lock, flags);
 
 	/* ...and finally, reset everything */
-	rtl8139_hw_start (dev);
-
-	netif_wake_queue (dev);
+	if (netif_running(dev)) {
+		netif_poll_enable (dev);
+		rtl8139_hw_start (dev);
+		netif_wake_queue (dev);
+	}
 }
 
 
@@ -1694,6 +1713,7 @@
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % NUM_TX_DESC;
 
+	/* Note: the chip doesn't have auto-pad! */
 	if (likely(len < TX_BUF_SIZE)) {
 		if (len < ETH_ZLEN)
 			memset(tp->tx_buf[entry], 0, ETH_ZLEN);
@@ -1705,7 +1725,6 @@
 		return 0;
 	}
 
-	/* Note: the chip doesn't have auto-pad! */
 	spin_lock_irq(&tp->lock);
 	RTL_W32_F (TxStatus0 + (entry * sizeof (u32)),
 		   tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
@@ -1791,8 +1810,7 @@
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
 		mb();
-		if (netif_queue_stopped (dev))
-			netif_wake_queue (dev);
+		netif_wake_queue (dev);
 	}
 }
 
@@ -1879,35 +1897,31 @@
 #endif
 }
 
-static void rtl8139_rx_interrupt (struct net_device *dev,
-				  struct rtl8139_private *tp, void *ioaddr)
+static int rtl8139_rx(struct net_device *dev, struct rtl8139_private *tp,
+		      int budget)
 {
-	unsigned char *rx_ring;
-	u16 cur_rx;
-
-	assert (dev != NULL);
-	assert (tp != NULL);
-	assert (ioaddr != NULL);
-
-	rx_ring = tp->rx_ring;
-	cur_rx = tp->cur_rx;
+	void *ioaddr = tp->mmio_addr;
+	int received = 0;
+	const unsigned char *rx_ring = tp->rx_ring;
+	unsigned int cur_rx = tp->cur_rx;
 
 	DPRINTK ("%s: In rtl8139_rx(), current %4.4x BufAddr %4.4x,"
 		 " free to %4.4x, Cmd %2.2x.\n", dev->name, cur_rx,
 		 RTL_R16 (RxBufAddr),
 		 RTL_R16 (RxBufPtr), RTL_R8 (ChipCmd));
 
-	while ((RTL_R8 (ChipCmd) & RxBufEmpty) == 0) {
-		int ring_offset = cur_rx % RX_BUF_LEN;
+	while (netif_running(dev) && received < budget 
+	       && (RTL_R8 (ChipCmd) & RxBufEmpty) == 0) {
 		u32 rx_status;
 		unsigned int rx_size;
 		unsigned int pkt_size;
 		struct sk_buff *skb;
+		u16 status;
 
 		rmb();
 
 		/* read size+status of next frame from DMA ring buffer */
-		rx_status = le32_to_cpu (*(u32 *) (rx_ring + ring_offset));
+		rx_status = le32_to_cpu (*(u32 *) (rx_ring + cur_rx));
 		rx_size = rx_status >> 16;
 		pkt_size = rx_size - 4;
 
@@ -1929,9 +1943,9 @@
 		 * Theoretically, this should never happen
 		 * since EarlyRx is disabled.
 		 */
-		if (rx_size == 0xfff0) {
+		if (unlikely(rx_size == 0xfff0)) {
 			tp->xstats.early_rx++;
-			break;
+			goto done;
 		}
 
 		/* If Rx err or invalid rx_size/rx_status received
@@ -1939,55 +1953,65 @@
 		 * Rx process gets reset, so we abort any further
 		 * Rx processing.
 		 */
-		if ((rx_size > (MAX_ETH_FRAME_SIZE+4)) ||
-		    (rx_size < 8) ||
-		    (!(rx_status & RxStatusOK))) {
+		if (unlikely((rx_size > (MAX_ETH_FRAME_SIZE+4)) ||
+			     (rx_size < 8) ||
+			     (!(rx_status & RxStatusOK)))) {
 			rtl8139_rx_err (rx_status, dev, tp, ioaddr);
-			return;
+			return -1;
 		}
 
 		/* Malloc up new buffer, compatible with net-2e. */
 		/* Omit the four octet CRC from the length. */
 
-		/* TODO: consider allocating skb's outside of
-		 * interrupt context, both to speed interrupt processing,
-		 * and also to reduce the chances of having to
-		 * drop packets here under memory pressure.
-		 */
-
 		skb = dev_alloc_skb (pkt_size + 2);
-		if (skb) {
+		if (likely(skb)) {
 			skb->dev = dev;
 			skb_reserve (skb, 2);	/* 16 byte align the IP fields. */
-
-			eth_copy_and_sum (skb, &rx_ring[ring_offset + 4], pkt_size, 0);
+			memcpy(skb->data, &rx_ring[cur_rx + 4], pkt_size);
 			skb_put (skb, pkt_size);
 
 			skb->protocol = eth_type_trans (skb, dev);
-			netif_rx (skb);
+
 			dev->last_rx = jiffies;
 			tp->stats.rx_bytes += pkt_size;
 			tp->stats.rx_packets++;
+
+			netif_receive_skb (skb);
 		} else {
-			printk (KERN_WARNING
-				"%s: Memory squeeze, dropping packet.\n",
-				dev->name);
+			if (net_ratelimit()) 
+				printk (KERN_WARNING
+					"%s: Memory squeeze, dropping packet.\n",
+					dev->name);
 			tp->stats.rx_dropped++;
 		}
+		received++;
 
-		cur_rx = (cur_rx + rx_size + 4 + 3) & ~3;
-		RTL_W16 (RxBufPtr, cur_rx - 16);
+		cur_rx = ((cur_rx + rx_size + 4 + 3) & ~3) % RX_BUF_LEN;
+		RTL_W16 (RxBufPtr, (u16) (cur_rx - 16));
 
-		if (RTL_R16 (IntrStatus) & RxAckBits)
+		/* Clear out errors and receive interrupts */
+		status = RTL_R16 (IntrStatus) & RxAckBits;
+		if (likely(status != 0)) {
+			if (unlikely(status & (RxFIFOOver | RxOverflow))) {
+				tp->stats.rx_errors++;
+				if (status & RxFIFOOver)
+					tp->stats.rx_fifo_errors++;
+			}
 			RTL_W16_F (IntrStatus, RxAckBits);
+		}
 	}
 
+ done:
+
+#if RTL8139_DEBUG > 1
 	DPRINTK ("%s: Done rtl8139_rx(), current %4.4x BufAddr %4.4x,"
 		 " free to %4.4x, Cmd %2.2x.\n", dev->name, cur_rx,
 		 RTL_R16 (RxBufAddr),
 		 RTL_R16 (RxBufPtr), RTL_R8 (ChipCmd));
+#endif
 
 	tp->cur_rx = cur_rx;
+	return received;
 }
 
 
@@ -2013,14 +2037,11 @@
 		status &= ~RxUnderrun;
 	}
 
-	/* XXX along with rtl8139_rx_err, are we double-counting errors? */
-	if (status &
-	    (RxUnderrun | RxOverflow | RxErr | RxFIFOOver))
+	if (status & (RxUnderrun | RxErr))
 		tp->stats.rx_errors++;
-
 	if (status & PCSTimeout)
-		tp->stats.rx_length_errors++;
-	if (status & (RxUnderrun | RxFIFOOver))
+		tp->stats.rx_length_errors++;	/* race with rtl8139_rx_err */
+	if (status & RxUnderrun)
 		tp->stats.rx_fifo_errors++;
 	if (status & PCIErr) {
 		u16 pci_cmd_status;
@@ -2032,6 +2053,35 @@
 	}
 }
 
+static int rtl8139_poll(struct net_device *dev, int *budget)
+{
+	struct rtl8139_private *tp = dev->priv;
+	void *ioaddr = tp->mmio_addr;
+	int orig_budget = min(*budget, dev->quota);
+	int done = 1;
+
+	if (likely(RTL_R16(IntrStatus) & RxAckBits)) {
+		int work_done;
+
+		work_done = rtl8139_rx(dev, tp, orig_budget);
+		if (likely(work_done > 0)) {
+			*budget -= work_done;
+			dev->quota -= work_done;
+			done = (work_done < orig_budget);
+		}
+	}
+
+	if (done) {
+		/*
+		 * This order is important
+		 * (see Documentation/networking/NAPI_HOWTO.txt)
+		 */
+		netif_rx_complete(dev);
+		RTL_W16_F(IntrMask, rtl8139_intr_mask);
+	}
+
+	return !done;
+}
 
 /* The interrupt handler does all of the Rx thread work and cleans up
    after the Tx thread. */
@@ -2040,68 +2090,59 @@
 {
 	struct net_device *dev = (struct net_device *) dev_instance;
 	struct rtl8139_private *tp = dev->priv;
-	int boguscnt = max_interrupt_work;
 	void *ioaddr = tp->mmio_addr;
-	int ackstat, status;
+	u16 status, ackstat;
 	int link_changed = 0; /* avoid bogus "uninit" warning */
 	int handled = 0;
 
 	spin_lock (&tp->lock);
+	status = RTL_R16 (IntrStatus);
 
-	do {
-		status = RTL_R16 (IntrStatus);
+	/* shared irq? */
+	if (unlikely((status & rtl8139_intr_mask) == 0)) 
+		goto out;
 
-		/* h/w no longer present (hotplug?) or major error, bail */
-		if (status == 0xFFFF)
-			break;
+	handled = 1;
 
-		if ((status &
-		     (PCIErr | PCSTimeout | RxUnderrun | RxOverflow |
-		      RxFIFOOver | TxErr | TxOK | RxErr | RxOK)) == 0)
-			break;
+	/* h/w no longer present (hotplug?) or major error, bail */
+	if (unlikely(status == 0xFFFF)) 
+		goto out;
 
-		handled = 1;
+	/* close possible race's with dev_close */
+	if (unlikely(!netif_running(dev))) {
+		RTL_W16 (IntrMask, 0);
+		goto out;
+	}
 
-		/* Acknowledge all of the current interrupt sources ASAP, but
-		   an first get an additional status bit from CSCR. */
-		if (status & RxUnderrun)
-			link_changed = RTL_R16 (CSCR) & CSCR_LinkChangeBit;
+	/* Acknowledge all of the current interrupt sources ASAP, but
+	   an first get an additional status bit from CSCR. */
+	if (unlikely(status & RxUnderrun))
+		link_changed = RTL_R16 (CSCR) & CSCR_LinkChangeBit;
 
-		/* The chip takes special action when we clear RxAckBits,
-		 * so we clear them later in rtl8139_rx_interrupt
-		 */
-		ackstat = status & ~(RxAckBits | TxErr);
+	ackstat = status & ~(RxAckBits | TxErr);
+	if (ackstat)
 		RTL_W16 (IntrStatus, ackstat);
 
-		DPRINTK ("%s: interrupt  status=%#4.4x ackstat=%#4.4x new intstat=%#4.4x.\n",
-			 dev->name, status, ackstat, RTL_R16 (IntrStatus));
-
-		if (netif_running (dev) && (status & RxAckBits))
-			rtl8139_rx_interrupt (dev, tp, ioaddr);
-
-		/* Check uncommon events with one test. */
-		if (status & (PCIErr | PCSTimeout | RxUnderrun | RxOverflow |
-		  	      RxFIFOOver | RxErr))
-			rtl8139_weird_interrupt (dev, tp, ioaddr,
-						 status, link_changed);
-
-		if (netif_running (dev) && (status & (TxOK | TxErr))) {
-			rtl8139_tx_interrupt (dev, tp, ioaddr);
-			if (status & TxErr)
-				RTL_W16 (IntrStatus, TxErr);
+	/* Receive packets are processed by poll routine.
+	   If not running start it now. */
+	if (status & RxAckBits){
+		if (netif_rx_schedule_prep(dev)) {
+			RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
+			__netif_rx_schedule (dev);
 		}
-
-		boguscnt--;
-	} while (boguscnt > 0);
-
-	if (boguscnt <= 0) {
-		printk (KERN_WARNING "%s: Too much work at interrupt, "
-			"IntrStatus=0x%4.4x.\n", dev->name, status);
-
-		/* Clear all interrupt sources. */
-		RTL_W16 (IntrStatus, 0xffff);
 	}
 
+	/* Check uncommon events with one test. */
+	if (unlikely(status & (PCIErr | PCSTimeout | RxUnderrun | RxErr)))
+		rtl8139_weird_interrupt (dev, tp, ioaddr,
+					 status, link_changed);
+
+	if (status & (TxOK | TxErr)) {
+		rtl8139_tx_interrupt (dev, tp, ioaddr);
+		if (status & TxErr)
+			RTL_W16 (IntrStatus, TxErr);
+	}
+ out:
 	spin_unlock (&tp->lock);
 
 	DPRINTK ("%s: exiting interrupt, intr_status=%#4.4x.\n",

Re: [PATCH] Updated 8139too with NAPI

[Jeff Garzik]

Stephen Hemminger wrote:
> Here is the latest update for 8139too with NAPI.
> 
> The changes from last time were to get rid of some of the netif_poll_disable calls
> to avoid getting trapped by receive interrupt races.  It is okay for the interrupt
> code to enable the poll routine to run even if we are just about to suspend or
> change multicast list.


I'm OK with the patch, but like last time, it doesn't apply cleanly to 
net-drivers-2.5-exp...  About to post an update, can you please rediff 
against that?

	Jeff

Re: [PATCH] Updated 8139too with NAPI

[OGAWA Hirofumi]

Stephen Hemminger <shemminger@osdl.org> writes:

> @@ -1663,6 +1678,8 @@
>  	/* Disable interrupts by clearing the interrupt mask. */
>  	RTL_W16 (IntrMask, 0x0000);
>  
> +	__netif_poll_disable(dev);

Rx-poll can enable interrupt. However, because it can happen on SMP
only, then reset of chips should be done sooner or later, so I don't
care so much.

> +		/*
> +		 * This order is important
> +		 * (see Documentation/networking/NAPI_HOWTO.txt)
> +		 */
> +		netif_rx_complete(dev);
> +		RTL_W16_F(IntrMask, rtl8139_intr_mask);

    netif_rx_complete(dev);
    -- interrupted --
	in rtl8139_interrupt(),
		if (netif_rx_schedule_prep(dev)) {
			RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
			__netif_rx_schedule (dev);
		}
    -- resume --
    /* enable interrupt, but rx-poll is already scheduling */
    RTL_W16_F(IntrMask, rtl8139_intr_mask);

So doesn't disable interrupt. Umm... this problem was things that my
patch fixed. Any objections?

		local_irq_disable();
		__netif_rx_complete(dev);
		RTL_W16_F(IntrMask, rtl8139_intr_mask);
		local_irq_enable();
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Updated 8139too with NAPI

[Jeff Garzik]

OGAWA Hirofumi wrote:
>     netif_rx_complete(dev);
>     -- interrupted --
> 	in rtl8139_interrupt(),
> 		if (netif_rx_schedule_prep(dev)) {
> 			RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
> 			__netif_rx_schedule (dev);
> 		}
>     -- resume --
>     /* enable interrupt, but rx-poll is already scheduling */
>     RTL_W16_F(IntrMask, rtl8139_intr_mask);


good point...