Fix SMP support on 3c527 net driver

PATCH Manfred Spraul 2 participants 3 messages Aug 31, 2003 Network drivers

lore

Share & tools

lore

Manfred Spraul

author

2003-08-31 08:28 UTC #0 lore raw

Quoted 3 lines · Felipe

Also, the down/up function doesn't seem to be used in interrupt context, so I think it will work.

[snip]

Quoted 11 lines

static int mc32_send_packet(struct sk_buff *skb, struct net_device *dev)
{
	struct mc32_local *lp = (struct mc32_local *)dev->priv;
(quoted diff) +0 -1
-	unsigned long flags;
	volatile struct skb_header *p, *np;

	netif_stop_queue(dev);

Review

Quoted 3 lines

(quoted diff) +1 -2

-	save_flags(flags);
-	cli();
+	down(&lp->mc32_sem);

No, that's wrong. mc32_send_packet is the hard_start_xmit function, called from bottom half context, with the dev_xmit_lock spinlock held. Additionally, you must replace the sleep_on calls with wait_event, or an open-coded wait queue: sleep_on is racy, it only works with cli().

IMHO the right way to fix cli() is
- add a single spinlock to the driver or the device structure. Do not forget the spin_lock_init().
- replace cli/sti with spin_lock_irqsave/spin_unlock_irqsave.
- Additionally acquire the spinlock in every interrupt handler (cli() stops all interrupts, spinlocks only stop interrupt on the current cpu).
- check if there were recursive cli() calls. Fix them.
- replace all sleep_on calls with wait queue calls.
- check if there are any kmalloc or schedule calls in the area now under the spinlock, and reorganize the code.

And please add a changelog entry that code was converted without testing.

--
Manfred

Felipe W Damasio

2003-08-31 16:46 UTC #1 lore raw

Hi Manfred,

Quoted 1 line · Manfred Spraul

Additionally, you must replace the sleep_on calls with wait_event, or an open-coded wait queue: sleep_on is racy, it only works with cli().

Oh, I didn't no that..

Quoted 4 lines

IMHO the right way to fix cli() is
- add a single spinlock to the driver or the device structure. Do not forget the spin_lock_init().
- replace cli/sti with spin_lock_irqsave/spin_unlock_irqsave.

Yes.

Quoted 6 lines

- Additionally acquire the spinlock in every interrupt handler (cli() stops all interrupts, spinlocks only stop interrupt on the current cpu).
- check if there were recursive cli() calls. Fix them.
- replace all sleep_on calls with wait queue calls.
- check if there are any kmalloc or schedule calls in the area now under the spinlock, and reorganize the code.

But doesn't wait_queue call schedule()?

Quoted 1 line

And please add a changelog entry that code was converted without testing.

	Ok.

	Thanks for your review, will work on those.

Felipe

Signature · 2 lines

It's most certainly GNU/Linux, not Linux. Read more at
http://www.gnu.org/gnu/why-gnu-linux.html

Manfred Spraul

author

2003-08-31 17:31 UTC #2 lore raw

Quoted 25 lines · Felipe W Damasio

Hi Manfred,
Quoted 1 line · Manfred Spraul
Additionally, you must replace the sleep_on calls with wait_event, or an open-coded wait queue: sleep_on is racy, it only works with cli().
Oh, I didn't no that..
Quoted 4 lines
IMHO the right way to fix cli() is
- add a single spinlock to the driver or the device structure. Do not forget the spin_lock_init().
- replace cli/sti with spin_lock_irqsave/spin_unlock_irqsave.
Yes.
Quoted 6 lines
- Additionally acquire the spinlock in every interrupt handler (cli() stops all interrupts, spinlocks only stop interrupt on the current cpu).
- check if there were recursive cli() calls. Fix them.
- replace all sleep_on calls with wait queue calls.
- check if there are any kmalloc or schedule calls in the area now under the spinlock, and reorganize the code.

Quoted 1 line

But doesn't wait_queue call schedule()?

Yes, it does. You need a larger change to fix that. Something like the attached patch.

--
Manfred

drivers/net/3c527.c +30 ~~-13~~

@@ -100,6 +100,7 @@
 #include <linux/string.h>
 #include <linux/wait.h>
 #include <linux/ethtool.h>
+#include <linux/spinlock.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -179,6 +180,7 @@
 	u16 tx_ring_head;       /* index to tx en-queue end */
 
 	u16 rx_ring_tail;       /* index to rx de-queue end */ 
+	spinlock_t lock;
 };
 
 /* The station (ethernet) address prefix, used for a sanity check. */

@@ -579,6 +581,27 @@
 	return 0;
 }
 
+/**
+ *	wait_exec_pending - sleep until exec_pending reaches a certain value
+ *	@lp: m32_local structure describing the target card
+ *	@value: value to wait for
+ *
+ *	The caller must acquire lp->lock before calling this function, it
+ *	temporarily drops the lock when it sleeps.
+ */
+
+static void wait_exec_pending(struct mc32_local *lp, int value)
+{
+	while (lp->exec_pending != value) {
+	        DEFINE_WAIT(wait);
+
+	        prepare_to_wait(&lp->event, &wait, TASK_UNINTERRUPTIBLE);
+		spin_unlock_irq(&lp->lock);
+	        schedule();
+	        finish_wait(&lp->event, &wait);
+		spin_lock_irq(&lp->lock);
+	}
+}
 
 /**
  *	mc32_command	-	send a command and sleep until completion

@@ -619,14 +642,11 @@
 	int ret = 0;
 	
 	/*
-	 *	Wait for a command
+	 *	Wait until there are no more pending commands
 	 */
 	 
-	save_flags(flags);
-	cli();
-	 
-	while(lp->exec_pending)
-		sleep_on(&lp->event);
+	spin_lock_irqsave(&lp->lock, flags);
+	wait_exec_pending(lp, 0);
 		
 	/*
 	 *	Issue mine

@@ -634,7 +654,7 @@
 
 	lp->exec_pending=1;
 	
-	restore_flags(flags);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	
 	lp->exec_box->mbox=0;
 	lp->exec_box->mbox=cmd;

@@ -645,13 +665,10 @@
 	while(!(inb(ioaddr+HOST_STATUS)&HOST_STATUS_CRR));
 	outb(1<<6, ioaddr+HOST_CMD);	
 
-	save_flags(flags);
-	cli();
-
-	while(lp->exec_pending!=2)
-		sleep_on(&lp->event);
+	spin_lock_irqsave(&lp->lock, flags);
+	wait_exec_pending(lp, 2);
 	lp->exec_pending=0;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	
 	if(lp->exec_box->mbox&(1<<13))
 		ret = -1;