Re: [PATCH] Fix SMP support on 3c527 net driver

Re: [PATCH] Fix SMP support on 3c527 net driver

[Manfred Spraul]

Felipe wrote:

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

> static int mc32_send_packet(struct sk_buff *skb, struct net_device *dev)
> {
> 	struct mc32_local *lp = (struct mc32_local *)dev->priv;
>-	unsigned long flags;
> 
> 	volatile struct skb_header *p, *np;
> 
> 	netif_stop_queue(dev);
> 
>-	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

Re: [PATCH] Fix SMP support on 3c527 net driver

[Felipe W Damasio]

	Hi Manfred,

Manfred Spraul wrote:
> 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..

> 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.

> - 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()?

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

	Ok.

	Thanks for your review, will work on those.

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

Re: [PATCH] Fix SMP support on 3c527 net driver

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

Felipe W Damasio wrote:

>     Hi Manfred,
>
> Manfred Spraul wrote:
>
>> 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..
>
>> 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.
>
>> - 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()?

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

--
    Manfred

[-- Attachment #2: patch-3c527 --]
[-- Type: text/plain, Size: 2159 bytes --]

--- 2.6/drivers/net/3c527.c	2003-06-17 06:20:03.000000000 +0200
+++ build-2.6/drivers/net/3c527.c	2003-08-31 19:26:37.000000000 +0200
@@ -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;

[PATCH] Fix SMP support on 3c527 net driver

[Felipe W Damasio]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

	Hi Richard,

	Patch against 2.6.0-test4

	This is a first try to fix SMP support on the 3c527 net driver, by 
removing cli/sti and replacing them with proper locking.

	Since the critical section that used cli/save_flags to serialize its 
access had "sleep_on" in it, I added a per-device semaphore to it, and 
used this lock instead. Also, the down/up function doesn't seem to be 
used in interrupt context, so I think it will work.

	Compile fine, but I don't have the hardware to test it.

	Please review this patch and consider applying if it looks good,

	Thanks.

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

[-- Attachment #2: 3c527-smp.patch --]
[-- Type: text/plain, Size: 2773 bytes --]

--- linux-2.6.0-test4/drivers/net/3c527.c	Fri Aug 22 20:56:34 2003
+++ linux-2.6.0-test4-fwd/drivers/net/3c527.c	Sat Aug 30 23:57:25 2003
@@ -17,8 +17,8 @@
  */
 
 #define DRV_NAME		"3c527"
-#define DRV_VERSION		"0.6a"
-#define DRV_RELDATE		"2001/11/17"
+#define DRV_VERSION		"0.6b"
+#define DRV_RELDATE		"2003/08/31"
 
 static const char *version =
 DRV_NAME ".c:v" DRV_VERSION " " DRV_RELDATE " Richard Proctor (rnp@netlink.co.nz)\n";
@@ -174,6 +174,8 @@
 
 	struct mc32_ring_desc tx_ring[TX_RING_LEN];	/* Host Transmit ring */
 	struct mc32_ring_desc rx_ring[RX_RING_LEN];	/* Host Receive ring */
+	
+	struct semaphore mc32_sem;
 
 	u16 tx_ring_tail;       /* index to tx de-queue end */
 	u16 tx_ring_head;       /* index to tx en-queue end */
@@ -615,16 +617,14 @@
 {
 	struct mc32_local *lp = (struct mc32_local *)dev->priv;
 	int ioaddr = dev->base_addr;
-	unsigned long flags;
 	int ret = 0;
 	
 	/*
 	 *	Wait for a command
 	 */
 	 
-	save_flags(flags);
-	cli();
-	 
+	down(&lp->mc32_sem);
+	
 	while(lp->exec_pending)
 		sleep_on(&lp->event);
 		
@@ -634,7 +634,7 @@
 
 	lp->exec_pending=1;
 	
-	restore_flags(flags);
+	up(&lp->mc32_sem);
 	
 	lp->exec_box->mbox=0;
 	lp->exec_box->mbox=cmd;
@@ -645,13 +645,12 @@
 	while(!(inb(ioaddr+HOST_STATUS)&HOST_STATUS_CRR));
 	outb(1<<6, ioaddr+HOST_CMD);	
 
-	save_flags(flags);
-	cli();
+	down(&lp->mc32_sem);
 
 	while(lp->exec_pending!=2)
 		sleep_on(&lp->event);
 	lp->exec_pending=0;
-	restore_flags(flags);
+	up(&lp->mc32_sem);
 	
 	if(lp->exec_box->mbox&(1<<13))
 		ret = -1;
@@ -725,7 +724,6 @@
 {
 	struct mc32_local *lp = (struct mc32_local *)dev->priv;
 	int ioaddr = dev->base_addr;
-	unsigned long flags;
 
 	mc32_ready_poll(dev);	
 
@@ -735,14 +733,13 @@
 	outb(HOST_CMD_SUSPND_RX, ioaddr+HOST_CMD);			
 	mc32_ready_poll(dev); 
 	outb(HOST_CMD_SUSPND_TX, ioaddr+HOST_CMD);	
-		
-	save_flags(flags);
-	cli();
+	
+	down(&lp->mc32_sem);
 		
 	while(lp->xceiver_state!=HALTED) 
 		sleep_on(&lp->event); 
 		
-	restore_flags(flags);	
+	up(&lp->mc32_sem);
 } 
 
 
@@ -1008,6 +1005,7 @@
 		return -ENOBUFS;
 	}
 
+	init_MUTEX(&lp->mc32_sem);
 	lp->desired_state = RUNNING; 
 	
 	/* And finally, set the ball rolling... */
@@ -1056,18 +1054,16 @@
 static int mc32_send_packet(struct sk_buff *skb, struct net_device *dev)
 {
 	struct mc32_local *lp = (struct mc32_local *)dev->priv;
-	unsigned long flags;
 
 	volatile struct skb_header *p, *np;
 
 	netif_stop_queue(dev);
 
-	save_flags(flags);
-	cli();
+	down(&lp->mc32_sem);
 		
 	if(atomic_read(&lp->tx_count)==0)
 	{
-		restore_flags(flags);
+		up(&lp->mc32_sem);
 		return 1;
 	}
 
@@ -1098,7 +1094,7 @@
 		
 	p->control     &= ~CONTROL_EOL;     /* Clear EOL on p */ 
 out:	
-	restore_flags(flags);
+	up(&lp->mc32_sem);
 
 	netif_wake_queue(dev);
 	return 0;