[PATCH] 8390.c: preemption and disable_irq

[PATCH] 8390.c: preemption and disable_irq

[Robert Love]

Jeff,

Alan pointed out to me that drivers/net/8390.c calls disable_irq() while
non-atomic.  This leads to the possibility that the kernel is preempted
while the IRQ line is disabled.

This is not a bug but obviously not optimal.  The solution is simply to
disable preemption prior to calling disable_irq().

Patch against 2.5.40 is attached.  Look OK?

	Robert Love

P.S. Note there is a much simpler solution of merely flipping the
spin_lock() and disable_irq() calls - the spin_lock() will provide the
atomicity without the explicit preempt_disable().  I do not think this
is safe, however, since the IRQ handler could deadlock if it grabbed
this lock (which I assume it does).

diff -urN linux-2.5.40/drivers/net/8390.c linux/drivers/net/8390.c
--- linux-2.5.40/drivers/net/8390.c	Tue Oct  1 03:06:17 2002
+++ linux/drivers/net/8390.c	Tue Oct  1 17:49:11 2002
@@ -243,7 +243,8 @@
 	}
 
 	/* Ugly but a reset can be slow, yet must be protected */
-		
+
+	preempt_disable();
 	disable_irq_nosync(dev->irq);
 	spin_lock(&ei_local->page_lock);
 		
@@ -253,6 +254,7 @@
 		
 	spin_unlock(&ei_local->page_lock);
 	enable_irq(dev->irq);
+	preempt_enable();
 	netif_wake_queue(dev);
 }
     
@@ -286,9 +288,9 @@
 	/*
 	 *	Slow phase with lock held.
 	 */
-	 
+
+	preempt_disable();
 	disable_irq_nosync(dev->irq);
-	
 	spin_lock(&ei_local->page_lock);
 	
 	ei_local->irqlock = 1;
@@ -331,6 +333,7 @@
 		outb_p(ENISR_ALL, e8390_base + EN0_IMR);
 		spin_unlock(&ei_local->page_lock);
 		enable_irq(dev->irq);
+		preempt_enable();
 		ei_local->stat.tx_errors++;
 		return 1;
 	}
@@ -387,6 +390,7 @@
 	
 	spin_unlock(&ei_local->page_lock);
 	enable_irq(dev->irq);
+	preempt_enable();
 
 	dev_kfree_skb (skb);
 	ei_local->stat.tx_bytes += send_length;

Re: [PATCH] 8390.c: preemption and disable_irq

[Alan Cox]

On Tue, 2002-10-01 at 22:58, Robert Love wrote:
> P.S. Note there is a much simpler solution of merely flipping the
> spin_lock() and disable_irq() calls - the spin_lock() will provide the
> atomicity without the explicit preempt_disable().  I do not think this
> is safe, however, since the IRQ handler could deadlock if it grabbed
> this lock (which I assume it does).


The actual game the driver is playing goes like this

ne2k-pci hardware is often very slow (some of it is isa chips nailed to
crappy pci/isa glue). 8390's also have the fun property that the
registers are windowed and you can't clear a pending IRQ without
flipping windows. 

When we do a transmit we want to do the packet upload with most
interrupts enabled (or you drop serial interrupts and life sucks a lot).
We take the lock with the irq. We switch window (safely under the lock)
and we turn interrupts off on the card. We then release the lock. 

At this point you might think all is safe. However IRQ delivery on x86
is asynchronous so an IRQ can be in flight after we turn it off but not
yet at the CPU. To cover this case we disable that IRQ line, then take
the lock - possibly waiting briefly for an IRQ handler to clear on
another CPU, but not where one can begin to occur and deadlock.

The end result of this little pile of tricks is that you can run ne2k
cards and a modem on a PC at the same time. I think you want to
disable pre-empt before we take the lock with irqsave - to cover the
moment we have the irq disabled against pre-emption and packet loss too
- ie move the pre-empt disable further backwards.