Skip to contentSkip to conversation

2.6 driver for Silan SC92031 (second try)

PATCH In review Cesar Eduardo Barros 2 participants 3 messages Dec 8, 2006 – Dec 9, 2006

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 2.6 driver for Silan SC92031 (second try)

From: Cesar Eduardo Barros <[email protected]>

This is a driver for the Silan SC92031/Rsltek 8139D NIC chip.

This chip is found on at least one counterfeit Encore ENL832-TX-RENT NIC [1], which came with a mini-CD with the 2.4 driver. A slightly older version of the driver was found at [2]. The main difference between them is that the newer one has a small bugfix in the RX path, a lot of gratuitous renaming of functions, all the printable strings changed to show as a "Rsltek 8139D" [sic], and a PCI ID of 8139 instead of 2031. The driver on this patch is a rewrite of the vendor drivers (based mostly on the older one).

This patch is against 2.6.19; it is currently being used on a 2.6.17 [3] kernel on one of my boxes (Ubuntu x86-64 vendor kernel, with the driver compiled as a out-of-tree module), and compiles on 2.6.19 without any errors or warnings.

Changes from the previous patch sent to netdev:
- Use MMIO instead of PIO
- Changed TX bounce buffers allocation
- Use skb_copy_and_csum_dev
- Several small bug fixes
- Tested for more than just a few minutes each time

[1] See http://www.encore-usa.com/faq.php under ENL832-TX-RENT for more information
[2] Look for SL_LINUX.ZIP (which is really a .tar.gz) at

    http://broadbandforum.in/dataone_Intex_LAN_cardlinux-t4207-s15.html

[3] To compile on 2.6.17, simply add back the last argument to the interrupt handler in two places, and copy the boolean declarations from 2.6.19

---

drivers/net/Kconfig +12 -0
diff -uprN -X linux-2.6.19.orig/Documentation/dontdiff linux-2.6.19.orig/drivers/net/Kconfig linux-2.6.19/drivers/net/Kconfig
@@ -1777,6 +1777,18 @@ config LAN_SAA9730
 	  workstations.
 	  See <http://www.semiconductors.philips.com/pip/SAA9730_flyer_1>.
 
+config SC92031
+	tristate "Silan SC92031 PCI Fast Ethernet Adapter driver (EXPERIMENTAL)"
+	depends on NET_PCI && PCI && EXPERIMENTAL
+	select CRC32
+	---help---
+	  This is a driver for the Fast Ethernet PCI network cards based on
+	  the Silan SC92031 chip (sometimes also called Rsltek 8139D). If you
+	  have one of these, say Y here.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called sc92031.  This is recommended.
+
 config NET_POCKET
 	bool "Pocket and portable adapters"
 	depends on NET_ETHERNET && PARPORT
drivers/net/Makefile +1 -0
diff -uprN -X linux-2.6.19.orig/Documentation/dontdiff linux-2.6.19.orig/drivers/net/Makefile linux-2.6.19/drivers/net/Makefile
@@ -160,6 +160,7 @@ obj-$(CONFIG_APRICOT) += 82596.o
 obj-$(CONFIG_LASI_82596) += lasi_82596.o
 obj-$(CONFIG_MVME16x_NET) += 82596.o
 obj-$(CONFIG_BVME6000_NET) += 82596.o
+obj-$(CONFIG_SC92031) += sc92031.o
 
 # This is also a 82596 and should probably be merged
 obj-$(CONFIG_LP486E) += lp486e.o
drivers/net/sc92031.c +1583 -0

1,583 lines not on this page · raw message

Signature · 3 lines
Cesar Eduardo Barros
[email protected]
[email protected]
#1 lore raw

drivers/net/Kconfig config LAN_SAA9730

Quoted 13 lines · On Fri, 08 Dec 2006 18:17:06 -0200 Cesar Eduardo Barros <[email protected]>…

From: Cesar Eduardo Barros <[email protected]>

This is a driver for the Silan SC92031/Rsltek 8139D NIC chip.

...

drivers/net/Kconfig +4 -0
+config SC92031
+	depends on NET_PCI && PCI && EXPERIMENTAL
+	select CRC32
 
...
 
+	} while(unlikely(cmpxchg(&priv->intr_status,

You'll have the arm maintainer after you with a pointy stick.

cmpxchg is only available on certain architectures. It would be acceptable to make this driver depend on X86 (or something). Better to rewrite this code so it doesn't use cmpxchg.

#2 lore raw

Andrew Morton escreveu:

Quoted 5 lines
Quoted 2 lines · On Fri, 08 Dec 2006 18:17:06 -0200 Cesar Eduardo Barros <[email protected]>…

From: Cesar Eduardo Barros <[email protected]>

Quoted 7 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	} while(unlikely(cmpxchg(&priv->intr_status,

You'll have the arm maintainer after you with a pointy stick.

cmpxchg is only available on certain architectures. It would be acceptable to make this driver depend on X86 (or something). Better to rewrite this code so it doesn't use cmpxchg.

Fixed. As long as I make sure the interrupt and the tasklet never run at the same time, I do not need to protect intr_status against concurrent modification.

Also fixed a potential bug where the tasklet could end up permanently disabled.

Apply on top of the previous (2.0b) patch (if you prefer a merged patch, just ask and I'll make one).

It has received the same testing as the previous one, only for less time (I'm using it right now).

---

drivers/net/sc92031.c +54 -20
diff -uprN -X linux-2.6.19.orig/Documentation/dontdiff linux-2.6.19-2.0b/drivers/net/sc92031.c linux-2.6.19/drivers/net/sc92031.c
@@ -34,7 +34,7 @@
 
 #define SC92031_NAME "sc92031"
 #define SC92031_DESCRIPTION "Silan SC92031 PCI Fast Ethernet Adapter driver"
-#define SC92031_VERSION "2.0b"
+#define SC92031_VERSION "2.0c"
 
 /* BAR 0 is MMIO, BAR 1 is PIO */
 #ifndef SC92031_USE_BAR
@@ -261,6 +261,12 @@ enum PMConfigBits {
  * Use mmiowb() before unlocking if the hardware was written to.
  */
 
+/* Locking rules for the interrupt:
+ * - the interrupt and the tasklet never run at the same time
+ * - neither run between sc92031_disable_interrupts and
+ *   sc92031_enable_interrupt
+ */
+
 struct sc92031_priv {
 	spinlock_t		lock;
 	/* iomap.h cookie */
@@ -288,6 +294,7 @@ struct sc92031_priv {
 
 	/* copies of some hardware registers */
 	u32			intr_status;
+	atomic_t		intr_mask;
 	u32			rx_config;
 	u32			tx_config;
 	u32			pm_config;
@@ -304,6 +311,13 @@ struct sc92031_priv {
 	struct net_device_stats	stats;
 };
 
+/* I don't know which registers can be safely read; however, I can guess
+ * MAC0 is one of them. */
+static inline void _sc92031_dummy_read(void __iomem *port_base)
+{
+	ioread32(port_base + MAC0);
+}
+
 static u32 _sc92031_mii_wait(void __iomem *port_base)
 {
 	u32 mii_status;
@@ -348,12 +362,18 @@ static void sc92031_disable_interrupts(s
 	struct sc92031_priv *priv = netdev_priv(dev);
 	void __iomem *port_base = priv->port_base;
 
-	tasklet_disable(&priv->tasklet);
+	/* tell the tasklet/interrupt not to enable interrupts */
+	atomic_set(&priv->intr_mask, 0);
+	wmb();
 
+	/* stop interrupts */
 	iowrite32(0, port_base + IntrMask);
+	_sc92031_dummy_read(port_base);
 	mmiowb();
 
+	/* wait for any concurrent interrupt/tasklet to finish */
 	synchronize_irq(dev->irq);
+	tasklet_disable(&priv->tasklet);
 }
 
 static void sc92031_enable_interrupts(struct net_device *dev)
@@ -363,6 +383,9 @@ static void sc92031_enable_interrupts(st
 
 	tasklet_enable(&priv->tasklet);
 
+	atomic_set(&priv->intr_mask, IntrBits);
+	wmb();
+
 	iowrite32(IntrBits, port_base + IntrMask);
 	mmiowb();
 }
@@ -608,6 +631,7 @@ static void _sc92031_reset(struct net_de
 
 	/* clear old register values */
 	priv->intr_status = 0;
+	atomic_set(&priv->intr_mask, 0);
 	priv->rx_config = 0;
 	priv->tx_config = 0;
 	priv->mc_flags = 0;
@@ -824,9 +848,9 @@ static void sc92031_tasklet(unsigned lon
 	struct net_device *dev = (struct net_device *)data;
 	struct sc92031_priv *priv = netdev_priv(dev);
 	void __iomem *port_base = priv->port_base;
-	u32 intr_status;
+	u32 intr_status, intr_mask;
 
-	intr_status = xchg(&priv->intr_status, 0);
+	intr_status = priv->intr_status;
 
 	spin_lock(&priv->lock);
 
@@ -851,7 +875,10 @@ static void sc92031_tasklet(unsigned lon
 		_sc92031_link_tasklet(dev);
 
 out:
-	iowrite32(IntrBits, port_base + IntrMask);
+	intr_mask = atomic_read(&priv->intr_mask);
+	rmb();
+
+	iowrite32(intr_mask, port_base + IntrMask);
 	mmiowb();
 
 	spin_unlock(&priv->lock);
@@ -862,29 +889,33 @@ static irqreturn_t sc92031_interrupt(int
 	struct net_device *dev = dev_id;
 	struct sc92031_priv *priv = netdev_priv(dev);
 	void __iomem *port_base = priv->port_base;
-	u32 intr_status, old_intr_status, new_intr_status;
+	u32 intr_status, intr_mask;
+
+	/* mask interrupts before clearing IntrStatus */
+	iowrite32(0, port_base + IntrMask);
+	_sc92031_dummy_read(port_base);
 
 	intr_status = ioread32(port_base + IntrStatus);
 	if (unlikely(intr_status == 0xffffffff))
-		return IRQ_NONE;
+		return IRQ_NONE;	// hardware has gone missing
 
 	intr_status &= IntrBits;
 	if (!intr_status)
-		return IRQ_NONE;
-
-	do {
-		old_intr_status = priv->intr_status;
-		new_intr_status = old_intr_status | intr_status;
-	} while(unlikely(cmpxchg(&priv->intr_status,
-					old_intr_status, new_intr_status)
-			!= old_intr_status));
-
-	iowrite32(0, port_base + IntrMask);
-	mmiowb();
+		goto out_none;
 
+	priv->intr_status = intr_status;
 	tasklet_schedule(&priv->tasklet);
 
 	return IRQ_HANDLED;
+
+out_none:
+	intr_mask = atomic_read(&priv->intr_mask);
+	rmb();
+
+	iowrite32(intr_mask, port_base + IntrMask);
+	mmiowb();
+
+	return IRQ_NONE;
 }
 
 static struct net_device_stats *sc92031_get_stats(struct net_device *dev)
@@ -1007,7 +1038,7 @@ static int sc92031_open(struct net_devic
 
 	priv->pm_config = 0;
 
-	sc92031_disable_interrupts(dev);
+	/* Interrupts already disabled by sc92031_stop or sc92031_probe */
 	spin_lock(&priv->lock);
 
 	_sc92031_reset(dev);
@@ -1442,6 +1473,9 @@ static int __devinit sc92031_probe(struc
 	priv->port_base = port_base;
 	priv->pdev = pdev;
 	tasklet_init(&priv->tasklet, sc92031_tasklet, (unsigned long)dev);
+	/* Fudge tasklet count so the call to sc92031_enable_interrupts at
+	 * sc92031_open will work correctly */
+	tasklet_disable_nosync(&priv->tasklet);
 
 	/* PCI PM Wakeup */
 	iowrite32((~PM_LongWF & ~PM_LWPTN) | PM_Enable, port_base + PMConfig);
@@ -1527,7 +1561,7 @@ static int sc92031_resume(struct pci_dev
 	if (!netif_running(dev))
 		goto out;
 
-	sc92031_disable_interrupts(dev);
+	/* Interrupts already disabled by sc92031_suspend */
 	spin_lock(&priv->lock);
 
 	_sc92031_reset(dev);
Signature · 3 lines
Cesar Eduardo Barros
[email protected]
[email protected]