Skip to contentSkip to conversation

pcnet32: NAPI implementation.

PATCH In review Don Fry 2 participants 4 messages Jun 29, 2006 – Jun 30, 2006

Subsystems (2)
lore
Share & tools
lore

Don Fry

author
#0 lore raw

PATCH 8/9 pcnet32: NAPI implementation.

Implement NAPI changes to pcnet32 driver. Compile default is off. Listed as experimental.

Len and Don both worked on a NAPI implementation and have both tested these changes.

An e1000 blasting short packets to the pcnet32 will lockup Don's system until the receive storm stops. Without NAPI Len's system watchdog would expire causing the system to reboot. With NAPI the system will stay operational.

Tested ia32 and ppc64. Tested '970A, '971, '972, '973, '975, '976, and '978.

The Kconfig changes came from Len. Don is to blame for all the others.

drivers/net/Kconfig renamed from drivers/net/orig.Kconfig +17 -0
@@ -1300,6 +1300,23 @@ config PCNET32
 	  <file:Documentation/networking/net-modules.txt>. The module
 	  will be called pcnet32.
 
+config PCNET32_NAPI
+	bool "Use RX polling (NAPI) (EXPERIMENTAL)"
+	depends on PCNET32 && EXPERIMENTAL
+	help
+	  NAPI is a new driver API designed to reduce CPU and interrupt load
+	  when the driver is receiving lots of packets from the card. It is
+	  still somewhat experimental and thus not yet enabled by default.
+
+	  If your estimated Rx load is 10kpps or more, or if the card will be
+	  deployed on potentially unfriendly networks (e.g. in a firewall),
+	  then say Y here.
+
+	  See <file:Documentation/networking/NAPI_HOWTO.txt> for more
+	  information.
+
+	  If in doubt, say N.
+
 config AMD8111_ETH
 	tristate "AMD 8111 (new PCI lance) support"
 	depends on NET_PCI && PCI
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +346 -293

726 lines not on this page · raw message

Signature · 2 lines
Don Fry
[email protected]
#1 lore raw

Nit below.

Don Fry <[email protected]> :
[...]

Quoted 2 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +0 -0

[...]

Quoted 3 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +3 -0
+#ifdef CONFIG_PCNET32_NAPI
+static int pcnet32_poll(struct net_device *dev, int *budget)
+{

[...]

drivers/net/pcnet32.c static int pcnet32_suspend(struct net_de · line 1402

Quoted 3 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +3 -0
+	netif_rx_complete(dev);
+
+	spin_lock_irqsave(&lp->lock, flags);

No need to save/restore (it's true in pcnet32_{get_regs/suspend} too).

drivers/net/pcnet32.c static int pcnet32_suspend(struct net_de · line 1410

Quoted 8 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +8 -0
+
+	/* clear interrupt masks */
+	val = lp->a.read_csr(ioaddr, CSR3);
+	val &= 0x00ff;
+	lp->a.write_csr(ioaddr, CSR3, val);
+
+	/* Set interrupt enable. */
+	lp->a.write_csr(ioaddr, CSR0, CSR0_INTEN);

Insert mmiowb():

Quoted 2 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +2 -0
+
+	spin_unlock_irqrestore(&lp->lock, flags);

[...]

drivers/net/pcnet32.c pcnet32_interrupt(int irq, void *dev_id, · line 2617

Quoted 13 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +7 -2
@@ -2413,183 +2608,41 @@ pcnet32_interrupt(int irq, void *dev_id,
 				       dev->name, csr0);
 			/* unlike for the lance, there is no restart needed */
 		}
-
-		if (must_restart) {
+#ifdef CONFIG_PCNET32_NAPI
+		if (netif_rx_schedule_prep(dev)) {
+			u16 val;
+			/* set interrupt masks */
+			val = lp->a.read_csr(ioaddr, CSR3);
+			val |= 0x5f00;
+			lp->a.write_csr(ioaddr, CSR3, val);

Insert mmiowb();

Signature · 1 line
Ueimor
author
#2 lore raw
Quoted 11 lines · Francois Romieu

Nit below.

Don Fry <[email protected]> :
[...]

Quoted 2 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +0 -0

[...]

Quoted 3 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +3 -0
+#ifdef CONFIG_PCNET32_NAPI
+static int pcnet32_poll(struct net_device *dev, int *budget)
+{

[...]

Quoted 5 lines

drivers/net/pcnet32.c static int pcnet32_suspend(struct net_de · line 1402

Quoted 3 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +3 -0
+	netif_rx_complete(dev);
+
+	spin_lock_irqsave(&lp->lock, flags);

No need to save/restore (it's true in pcnet32_{get_regs/suspend} too).

This lock is taken by the interrupt handler and my reading of spinlocks.txt says I do need to use spin_lock_irqsave unless I misunderstand. The only spin_lock() is in the interrupt handler itself, all others are spin_lock_irqsave.

Quoted 11 lines

drivers/net/pcnet32.c static int pcnet32_suspend(struct net_de · line 1410

Quoted 9 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +8 -0
+
+	/* clear interrupt masks */
+	val = lp->a.read_csr(ioaddr, CSR3);
+	val &= 0x00ff;
+	lp->a.write_csr(ioaddr, CSR3, val);
+
+	/* Set interrupt enable. */
+	lp->a.write_csr(ioaddr, CSR0, CSR0_INTEN);

Insert mmiowb():

Is this required in addition to the two outw() in write_csr? There are rmb() and wmb() in the places that need them, but no mmiowb() anywhere. What are the factors for when mmiowb needs to be inserted?

Quoted 23 lines
Quoted 3 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +2 -0
+
+	spin_unlock_irqrestore(&lp->lock, flags);

[...]

drivers/net/pcnet32.c pcnet32_interrupt(int irq, void *dev_id, · line 2617

Quoted 13 lines
drivers/net/pcnet32.c renamed from drivers/net/purge.pcnet32.c +7 -2
@@ -2413,183 +2608,41 @@ pcnet32_interrupt(int irq, void *dev_id,
 				       dev->name, csr0);
 			/* unlike for the lance, there is no restart needed */
 		}
-
-		if (must_restart) {
+#ifdef CONFIG_PCNET32_NAPI
+		if (netif_rx_schedule_prep(dev)) {
+			u16 val;
+			/* set interrupt masks */
+			val = lp->a.read_csr(ioaddr, CSR3);
+			val |= 0x5f00;
+			lp->a.write_csr(ioaddr, CSR3, val);

Insert mmiowb();

Signature · 1 line
Ueimor
Signature · 2 lines
Don Fry
[email protected]
#3 lore raw

Don Fry <[email protected]> :
[...]

Quoted 6 lines
Quoted 1 line

No need to save/restore (it's true in pcnet32_{get_regs/suspend} too).

This lock is taken by the interrupt handler and my reading of spinlocks.txt says I do need to use spin_lock_irqsave unless I misunderstand. The only spin_lock() is in the interrupt handler itself, all others are spin_lock_irqsave.

The poll() function is issued in an irq enabled context. The state to be saved (in flags) is known thus the code can simply disable and enable the irq.

[...]

Quoted 5 lines
Quoted 1 line

Insert mmiowb():

Is this required in addition to the two outw() in write_csr? There are rmb() and wmb() in the places that need them, but no mmiowb() anywhere. What are the factors for when mmiowb needs to be inserted?

They are explained in Documentation/memory-barriers.txt.

I doubt that many user would be hurt if the mmiowb() are not added :o)

Signature · 1 line
Ueimor