8390.c: preemption and disable_irq

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
@@ -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;

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.