Andrew Morton escreveu:
On Fri, 08 Dec 2006 18:17:06 -0200
Cesar Eduardo Barros [EMAIL PROTECTED] wrote:
From: Cesar Eduardo Barros [EMAIL PROTECTED]
+} 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).
Signed-off-by: Cesar Eduardo Barros [EMAIL PROTECTED]
---
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
--- linux-2.6.19-2.0b/drivers/net/sc92031.c 2006-12-09 17:53:22.0
-0200
+++ linux-2.6.19/drivers/net/sc92031.c 2006-12-09 17:55:20.0 -0200
@@ -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_tintr_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