[PATCH] Fix SMP support on 3c527 net driver, take 2

[PATCH] Fix SMP support on 3c527 net driver, take 2

[Felipe W Damasio]

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

	Hi Richard,

	This is second version of the patch (against 2.6-test4) to fix SMP 
support on the 3c527 net driver, by removing cli/sti calls and 
replacing them with local locks.

	This one includes Manfred's idea of using wait_event instead of 
sleep_on, and releasing the lock to prevent the driver from going 
bezerk..it also uses spinlocks, since on some situations we are in 
bottom half context.

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

	Could you please see if this works for you?

	Manfred, could you take a look at this too?

	Comments are welcome so can get this fix right,

	Thanks.

Felipe

[-- Attachment #2: 3c527-cli_sti_removal.patch --]
[-- Type: text/plain, Size: 4454 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	Tue Sep  2 16:21:47 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/09/2"
 
 static const char *version =
 DRV_NAME ".c:v" DRV_VERSION " " DRV_RELDATE " Richard Proctor (rnp@netlink.co.nz)\n";
@@ -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>
@@ -157,11 +158,11 @@
 	volatile struct mc32_mailbox *rx_box;
 	volatile struct mc32_mailbox *tx_box;
 	volatile struct mc32_mailbox *exec_box;
-        volatile struct mc32_stats *stats;    /* Start of on-card statistics */
-        u16 tx_chain;           /* Transmit list start offset */
+	volatile struct mc32_stats *stats;    /* Start of on-card statistics */
+	u16 tx_chain;           /* Transmit list start offset */
 	u16 rx_chain;           /* Receive list start offset */
-        u16 tx_len;             /* Transmit list count */ 
-        u16 rx_len;             /* Receive list count */
+	u16 tx_len;             /* Transmit list count */ 
+	u16 rx_len;             /* Receive list count */
 
 	u32 base;
 	u16 exec_pending;
@@ -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. */
@@ -197,7 +199,6 @@
 	{ 0x0000, NULL }
 };
 
-
 /* Macros for ring index manipulations */ 
 static inline u16 next_rx(u16 rx) { return (rx+1)&(RX_RING_LEN-1); };
 static inline u16 prev_rx(u16 rx) { return (rx-1)&(RX_RING_LEN-1); };
@@ -536,7 +537,7 @@
  *	command register. This tells us nothing about the completion
  *	status of any pending commands and takes very little time at all.
  */
- 
+
 static void mc32_ready_poll(struct net_device *dev)
 {
 	int ioaddr = dev->base_addr;
@@ -579,6 +580,24 @@
 	return 0;
 }
 
+/**
+ *	wait_pending - sleep until a field reaches a certain value 
+ *	@lp: m32_local structure describing the target card
+ *
+ *	The caller must acquire lp->lock before calling this function, it
+ *	temporarily drops the lock when it sleeps (SMP-safe).
+ */
+
+static inline void wait_pending(struct mc32_local *lp)
+{
+	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 +638,12 @@
 	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);
+	while (lp->exec_pending)
+		wait_pending(lp);
 		
 	/*
 	 *	Issue mine
@@ -634,7 +651,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 +662,11 @@
 	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);
+	while (lp->exec_pending != 2)
+		wait_pending (lp);
 	lp->exec_pending=0;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	
 	if(lp->exec_box->mbox&(1<<13))
 		ret = -1;
@@ -735,15 +750,14 @@
 	outb(HOST_CMD_SUSPND_RX, ioaddr+HOST_CMD);			
 	mc32_ready_poll(dev); 
 	outb(HOST_CMD_SUSPND_TX, ioaddr+HOST_CMD);	
-		
-	save_flags(flags);
-	cli();
-		
+
+	spin_lock_irqsave (&lp->lock, flags);
+
 	while(lp->xceiver_state!=HALTED) 
 		sleep_on(&lp->event); 
-		
-	restore_flags(flags);	
-} 
+
+	spin_unlock_irqrestore (&lp->lock, flags);
+}
 
 
 /**
@@ -1062,12 +1076,11 @@
 
 	netif_stop_queue(dev);
 
-	save_flags(flags);
-	cli();
-		
+	spin_lock_irqsave (&lp->lock, flags);
+
 	if(atomic_read(&lp->tx_count)==0)
 	{
-		restore_flags(flags);
+		spin_unlock_irqrestore (&lp->lock, flags);
 		return 1;
 	}
 
@@ -1098,7 +1111,7 @@
 		
 	p->control     &= ~CONTROL_EOL;     /* Clear EOL on p */ 
 out:	
-	restore_flags(flags);
+	spin_unlock_irqrestore (&lp->lock, flags);
 
 	netif_wake_queue(dev);
 	return 0;

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

[Felipe W Damasio]

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

	Hi Richard,

Felipe W Damasio wrote:
>     This is second version of the patch (against 2.6-test4) to fix SMP 
> support on the 3c527 net driver, by removing cli/sti calls and replacing 
> them with local locks.
> 
>     This one includes Manfred's idea of using wait_event instead of 
> sleep_on, and releasing the lock to prevent the driver from going 
> bezerk..it also uses spinlocks, since on some situations we are in 
> bottom half context.

	Please try this patch instead.

	This one holds the device lock before doing "finish_wait", which 
seems to be the Right Way to do it.

	Thanks.

Felipe

[-- Attachment #2: 3c527-cli_sti_removal.patch --]
[-- Type: text/plain, Size: 4454 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	Wed Sep  3 10:39:37 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/09/2"
 
 static const char *version =
 DRV_NAME ".c:v" DRV_VERSION " " DRV_RELDATE " Richard Proctor (rnp@netlink.co.nz)\n";
@@ -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>
@@ -157,11 +158,11 @@
 	volatile struct mc32_mailbox *rx_box;
 	volatile struct mc32_mailbox *tx_box;
 	volatile struct mc32_mailbox *exec_box;
-        volatile struct mc32_stats *stats;    /* Start of on-card statistics */
-        u16 tx_chain;           /* Transmit list start offset */
+	volatile struct mc32_stats *stats;    /* Start of on-card statistics */
+	u16 tx_chain;           /* Transmit list start offset */
 	u16 rx_chain;           /* Receive list start offset */
-        u16 tx_len;             /* Transmit list count */ 
-        u16 rx_len;             /* Receive list count */
+	u16 tx_len;             /* Transmit list count */ 
+	u16 rx_len;             /* Receive list count */
 
 	u32 base;
 	u16 exec_pending;
@@ -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. */
@@ -197,7 +199,6 @@
 	{ 0x0000, NULL }
 };
 
-
 /* Macros for ring index manipulations */ 
 static inline u16 next_rx(u16 rx) { return (rx+1)&(RX_RING_LEN-1); };
 static inline u16 prev_rx(u16 rx) { return (rx-1)&(RX_RING_LEN-1); };
@@ -536,7 +537,7 @@
  *	command register. This tells us nothing about the completion
  *	status of any pending commands and takes very little time at all.
  */
- 
+
 static void mc32_ready_poll(struct net_device *dev)
 {
 	int ioaddr = dev->base_addr;
@@ -579,6 +580,24 @@
 	return 0;
 }
 
+/**
+ *	wait_pending - sleep until a field reaches a certain value 
+ *	@lp: m32_local structure describing the target card
+ *
+ *	The caller must acquire lp->lock before calling this function, it
+ *	temporarily drops the lock when it sleeps (SMP-safe).
+ */
+
+static inline void wait_pending(struct mc32_local *lp)
+{
+	DEFINE_WAIT(wait);
+
+	prepare_to_wait(&lp->event, &wait, TASK_UNINTERRUPTIBLE);
+	spin_unlock_irq(&lp->lock);
+	schedule();
+	spin_lock_irq(&lp->lock);
+	finish_wait(&lp->event, &wait);
+}
 
 /**
  *	mc32_command	-	send a command and sleep until completion
@@ -619,14 +638,12 @@
 	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);
+	while (lp->exec_pending)
+		wait_pending(lp);
 		
 	/*
 	 *	Issue mine
@@ -634,7 +651,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 +662,11 @@
 	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);
+	while (lp->exec_pending != 2)
+		wait_pending (lp);
 	lp->exec_pending=0;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	
 	if(lp->exec_box->mbox&(1<<13))
 		ret = -1;
@@ -735,15 +750,14 @@
 	outb(HOST_CMD_SUSPND_RX, ioaddr+HOST_CMD);			
 	mc32_ready_poll(dev); 
 	outb(HOST_CMD_SUSPND_TX, ioaddr+HOST_CMD);	
-		
-	save_flags(flags);
-	cli();
-		
+
+	spin_lock_irqsave (&lp->lock, flags);
+
 	while(lp->xceiver_state!=HALTED) 
 		sleep_on(&lp->event); 
-		
-	restore_flags(flags);	
-} 
+
+	spin_unlock_irqrestore (&lp->lock, flags);
+}
 
 
 /**
@@ -1062,12 +1076,11 @@
 
 	netif_stop_queue(dev);
 
-	save_flags(flags);
-	cli();
-		
+	spin_lock_irqsave (&lp->lock, flags);
+
 	if(atomic_read(&lp->tx_count)==0)
 	{
-		restore_flags(flags);
+		spin_unlock_irqrestore (&lp->lock, flags);
 		return 1;
 	}
 
@@ -1098,7 +1111,7 @@
 		
 	p->control     &= ~CONTROL_EOL;     /* Clear EOL on p */ 
 out:	
-	restore_flags(flags);
+	spin_unlock_irqrestore (&lp->lock, flags);
 
 	netif_wake_queue(dev);
 	return 0;

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

[Richard Procter]

Felipe, 

Just had a quick look at it. A couple of thoughts:        

* mc32_interrupt doesn't acquire the spinlock, rendering the critical
  sections un-exclusive on SMP. 

* sleep_on's remain in mc32_halt_transceiver and mc32_close. I think this 
  could lead to deadlock on UP --- eg. thread sleeps with spinlock held,
  subsequent interrupt then waits in vain. Might be less deadly on SMP?
  (are interrupt handlers reentrant on SMP?) 
 
* Just noticed an old error in mc32_close --- sleep_on is called in
  without first disabling interrupts. 

I've had a go at testing it, but ran into troubles with the ibmmca.c
driver carking on an uninitialised spinlock. Hopefully, I'll find some
quality time in the weekend to get things going. 

best, 
Richard. 

On Wed, 3 Sep 2003, Felipe W Damasio wrote:

> 	Hi Richard,
> 
> 	Please try this patch instead.
> 
> 	This one holds the device lock before doing "finish_wait", which 
> seems to be the Right Way to do it.
> 
> 	Thanks.
> 
> Felipe
> 

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

[Felipe W Damasio]

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

	Hi Richard,

Richard Procter wrote:
> * mc32_interrupt doesn't acquire the spinlock, rendering the critical
>   sections un-exclusive on SMP. 

	Hmm, you're right here...didn't notice this.

	But I'm not sure on how we should go here: We go get the hold lock 
right after 'status=inb(ioaddr+HOST_CMD)', so we can treat the 
interrupt safely.

	Although, there are some places that we do "wake_up (lp->event)", 
which I *think* will sleep (which is wrong), or will trigger the 
finish of wait_pending (which will try to re-acquire the lcok and will 
deadlock, which is also wrong).

	So, yes, we could release the lock right before we wake_up, but I'm 
not sure if this is the right fix either.

	Furthermore, if we acquire the lock after the "while (inb..)" 
statement, we must release the lock, and re-acquire so we can treat 
tx_ring/rx_ring safely as well...which is damn ugly and probably has 
many races.

	Maybe we could get the lock before 'while'?

	Ideas?

> * sleep_on's remain in mc32_halt_transceiver and mc32_close. I think this 
>   could lead to deadlock on UP --- eg. thread sleeps with spinlock held,
>   subsequent interrupt then waits in vain. Might be less deadly on SMP?
>   (are interrupt handlers reentrant on SMP?) 

	Neh, that was my fault.

	I just didn't switch to wait_pending...this is an easy fix.

> * Just noticed an old error in mc32_close --- sleep_on is called in
>   without first disabling interrupts. 

	This is fixed since we have to hold the device lock before calling 
wait_pending, so it should be fixed too.

> I've had a go at testing it, but ran into troubles with the ibmmca.c
> driver carking on an uninitialised spinlock. Hopefully, I'll find some
> quality time in the weekend to get things going. 

	Great.

	If all works, then we can push to Linus/Andrew/Jeff Garzik for merging.

	Attached is the new patch which fixes the latter 2 problems.

	Feedback welcome.

	Cheers,

Felipe

[-- Attachment #2: 3c527-cli_sti_removal.patch --]
[-- Type: text/plain, Size: 5502 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	Thu Sep  4 09:50:03 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/09/2"
 
 static const char *version =
 DRV_NAME ".c:v" DRV_VERSION " " DRV_RELDATE " Richard Proctor (rnp@netlink.co.nz)\n";
@@ -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>
@@ -157,11 +158,11 @@
 	volatile struct mc32_mailbox *rx_box;
 	volatile struct mc32_mailbox *tx_box;
 	volatile struct mc32_mailbox *exec_box;
-        volatile struct mc32_stats *stats;    /* Start of on-card statistics */
-        u16 tx_chain;           /* Transmit list start offset */
+	volatile struct mc32_stats *stats;    /* Start of on-card statistics */
+	u16 tx_chain;           /* Transmit list start offset */
 	u16 rx_chain;           /* Receive list start offset */
-        u16 tx_len;             /* Transmit list count */ 
-        u16 rx_len;             /* Receive list count */
+	u16 tx_len;             /* Transmit list count */ 
+	u16 rx_len;             /* Receive list count */
 
 	u32 base;
 	u16 exec_pending;
@@ -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. */
@@ -197,7 +199,6 @@
 	{ 0x0000, NULL }
 };
 
-
 /* Macros for ring index manipulations */ 
 static inline u16 next_rx(u16 rx) { return (rx+1)&(RX_RING_LEN-1); };
 static inline u16 prev_rx(u16 rx) { return (rx-1)&(RX_RING_LEN-1); };
@@ -536,7 +537,7 @@
  *	command register. This tells us nothing about the completion
  *	status of any pending commands and takes very little time at all.
  */
- 
+
 static void mc32_ready_poll(struct net_device *dev)
 {
 	int ioaddr = dev->base_addr;
@@ -579,6 +580,24 @@
 	return 0;
 }
 
+/**
+ *	wait_pending - sleep until a field reaches a certain value 
+ *	@lp: m32_local structure describing the target card
+ *
+ *	The caller must acquire lp->lock before calling this function, it
+ *	temporarily drops the lock when it sleeps (SMP-safe).
+ */
+
+static inline void wait_pending(struct mc32_local *lp)
+{
+	DEFINE_WAIT(wait);
+
+	prepare_to_wait(&lp->event, &wait, TASK_UNINTERRUPTIBLE);
+	spin_unlock_irq(&lp->lock);
+	schedule();
+	spin_lock_irq(&lp->lock);
+	finish_wait(&lp->event, &wait);
+}
 
 /**
  *	mc32_command	-	send a command and sleep until completion
@@ -619,14 +638,12 @@
 	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);
+	while (lp->exec_pending)
+		wait_pending(lp);
 		
 	/*
 	 *	Issue mine
@@ -634,7 +651,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 +662,11 @@
 	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);
+	while (lp->exec_pending != 2)
+		wait_pending (lp);
 	lp->exec_pending=0;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	
 	if(lp->exec_box->mbox&(1<<13))
 		ret = -1;
@@ -735,15 +750,14 @@
 	outb(HOST_CMD_SUSPND_RX, ioaddr+HOST_CMD);			
 	mc32_ready_poll(dev); 
 	outb(HOST_CMD_SUSPND_TX, ioaddr+HOST_CMD);	
-		
-	save_flags(flags);
-	cli();
-		
+
+	spin_lock_irqsave (&lp->lock, flags);
+
 	while(lp->xceiver_state!=HALTED) 
-		sleep_on(&lp->event); 
-		
-	restore_flags(flags);	
-} 
+		wait_pending (lp); 
+
+	spin_unlock_irqrestore (&lp->lock, flags);
+}
 
 
 /**
@@ -1062,12 +1076,11 @@
 
 	netif_stop_queue(dev);
 
-	save_flags(flags);
-	cli();
-		
+	spin_lock_irqsave (&lp->lock, flags);
+
 	if(atomic_read(&lp->tx_count)==0)
 	{
-		restore_flags(flags);
+		spin_unlock_irqrestore (&lp->lock, flags);
 		return 1;
 	}
 
@@ -1098,7 +1111,7 @@
 		
 	p->control     &= ~CONTROL_EOL;     /* Clear EOL on p */ 
 out:	
-	restore_flags(flags);
+	spin_unlock_irqrestore (&lp->lock, flags);
 
 	netif_wake_queue(dev);
 	return 0;
@@ -1386,7 +1399,7 @@
 			case 3: /* Halt */
 			case 4: /* Abort */
 				lp->xceiver_state |= TX_HALTED; 
-				wake_up(&lp->event);
+				wake_up(&lp->event); 
 				break;
 			default:
 				printk("%s: strange tx ack %d\n", dev->name, status&7);
@@ -1435,8 +1448,8 @@
 				   
 				if (lp->mc_reload_wait) 
 					mc32_reset_multicast_list(dev);
-				else 
-					wake_up(&lp->event);			       
+				else
+					wake_up(&lp->event);
 			}
 		}
 		if(status&2)
@@ -1450,7 +1463,6 @@
 		}
 	}
 
-
 	/*
 	 *	Process the transmit and receive rings 
          */
@@ -1492,6 +1504,8 @@
 	struct mc32_local *lp = (struct mc32_local *)dev->priv;
 
 	int ioaddr = dev->base_addr;
+	unsigned long flags;
+
 	u8 regs;
 	u16 one=1;
 	
@@ -1509,9 +1523,11 @@
 	mc32_halt_transceiver(dev); 
 	
 	/* Catch any waiting commands */
-	
+
+	spin_lock_irqsave(&lp->lock, flags);
 	while(lp->exec_pending==1)
-		sleep_on(&lp->event);
+		wait_pending(lp);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	       
 	/* Ok the card is now stopping */	
 	

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

[Alan Cox]

On Iau, 2003-09-04 at 13:52, Felipe W Damasio wrote:
> 	But I'm not sure on how we should go here: We go get the hold lock 
> right after 'status=inb(ioaddr+HOST_CMD)', so we can treat the 
> interrupt safely.
> 
> 	Although, there are some places that we do "wake_up (lp->event)", 

wake_up will not sleep, its safe from interrupts and with locks held.

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

[Felipe W Damasio]

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



Alan Cox wrote:
> On Iau, 2003-09-04 at 13:52, Felipe W Damasio wrote:
> 
>>	But I'm not sure on how we should go here: We go get the hold lock 
>>right after 'status=inb(ioaddr+HOST_CMD)', so we can treat the 
>>interrupt safely.
>>
>>	Although, there are some places that we do "wake_up (lp->event)", 
> 
> 
> wake_up will not sleep, its safe from interrupts and with locks held.

	So I think we can use something like the attached patch, then..which 
get the lock before the while loop on mc32_interrupt and releases it 
after processing the interrupts.

	Richard, could you please test it?

	Thanks

Felipe

[-- Attachment #2: 3c527-cli_sti_removal.patch --]
[-- Type: text/plain, Size: 6062 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	Thu Sep  4 10:24:40 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/09/2"
 
 static const char *version =
 DRV_NAME ".c:v" DRV_VERSION " " DRV_RELDATE " Richard Proctor (rnp@netlink.co.nz)\n";
@@ -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>
@@ -157,11 +158,11 @@
 	volatile struct mc32_mailbox *rx_box;
 	volatile struct mc32_mailbox *tx_box;
 	volatile struct mc32_mailbox *exec_box;
-        volatile struct mc32_stats *stats;    /* Start of on-card statistics */
-        u16 tx_chain;           /* Transmit list start offset */
+	volatile struct mc32_stats *stats;    /* Start of on-card statistics */
+	u16 tx_chain;           /* Transmit list start offset */
 	u16 rx_chain;           /* Receive list start offset */
-        u16 tx_len;             /* Transmit list count */ 
-        u16 rx_len;             /* Receive list count */
+	u16 tx_len;             /* Transmit list count */ 
+	u16 rx_len;             /* Receive list count */
 
 	u32 base;
 	u16 exec_pending;
@@ -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. */
@@ -197,7 +199,6 @@
 	{ 0x0000, NULL }
 };
 
-
 /* Macros for ring index manipulations */ 
 static inline u16 next_rx(u16 rx) { return (rx+1)&(RX_RING_LEN-1); };
 static inline u16 prev_rx(u16 rx) { return (rx-1)&(RX_RING_LEN-1); };
@@ -536,7 +537,7 @@
  *	command register. This tells us nothing about the completion
  *	status of any pending commands and takes very little time at all.
  */
- 
+
 static void mc32_ready_poll(struct net_device *dev)
 {
 	int ioaddr = dev->base_addr;
@@ -579,6 +580,24 @@
 	return 0;
 }
 
+/**
+ *	wait_pending - sleep until a field reaches a certain value 
+ *	@lp: m32_local structure describing the target card
+ *
+ *	The caller must acquire lp->lock before calling this function, it
+ *	temporarily drops the lock when it sleeps (SMP-safe).
+ */
+
+static inline void wait_pending(struct mc32_local *lp)
+{
+	DEFINE_WAIT(wait);
+
+	prepare_to_wait(&lp->event, &wait, TASK_UNINTERRUPTIBLE);
+	spin_unlock_irq(&lp->lock);
+	schedule();
+	spin_lock_irq(&lp->lock);
+	finish_wait(&lp->event, &wait);
+}
 
 /**
  *	mc32_command	-	send a command and sleep until completion
@@ -619,14 +638,12 @@
 	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);
+	while (lp->exec_pending)
+		wait_pending(lp);
 		
 	/*
 	 *	Issue mine
@@ -634,7 +651,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 +662,11 @@
 	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);
+	while (lp->exec_pending != 2)
+		wait_pending (lp);
 	lp->exec_pending=0;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	
 	if(lp->exec_box->mbox&(1<<13))
 		ret = -1;
@@ -735,15 +750,14 @@
 	outb(HOST_CMD_SUSPND_RX, ioaddr+HOST_CMD);			
 	mc32_ready_poll(dev); 
 	outb(HOST_CMD_SUSPND_TX, ioaddr+HOST_CMD);	
-		
-	save_flags(flags);
-	cli();
-		
+
+	spin_lock_irqsave (&lp->lock, flags);
+
 	while(lp->xceiver_state!=HALTED) 
-		sleep_on(&lp->event); 
-		
-	restore_flags(flags);	
-} 
+		wait_pending (lp); 
+
+	spin_unlock_irqrestore (&lp->lock, flags);
+}
 
 
 /**
@@ -1062,12 +1076,11 @@
 
 	netif_stop_queue(dev);
 
-	save_flags(flags);
-	cli();
-		
+	spin_lock_irqsave (&lp->lock, flags);
+
 	if(atomic_read(&lp->tx_count)==0)
 	{
-		restore_flags(flags);
+		spin_unlock_irqrestore (&lp->lock, flags);
 		return 1;
 	}
 
@@ -1098,7 +1111,7 @@
 		
 	p->control     &= ~CONTROL_EOL;     /* Clear EOL on p */ 
 out:	
-	restore_flags(flags);
+	spin_unlock_irqrestore (&lp->lock, flags);
 
 	netif_wake_queue(dev);
 	return 0;
@@ -1354,6 +1367,7 @@
 	int ioaddr, status, boguscount = 0;
 	int rx_event = 0;
 	int tx_event = 0; 
+	unsigned long flags;
 	
 	if (dev == NULL) {
 		printk(KERN_WARNING "%s: irq %d for unknown device.\n", cardname, irq);
@@ -1365,6 +1379,8 @@
 
 	/* See whats cooking */
 
+	spin_lock_irqsave(&lp->lock, flags);
+	
 	while((inb(ioaddr+HOST_STATUS)&HOST_STATUS_CWR) && boguscount++<2000)
 	{
 		status=inb(ioaddr+HOST_CMD);
@@ -1386,7 +1402,7 @@
 			case 3: /* Halt */
 			case 4: /* Abort */
 				lp->xceiver_state |= TX_HALTED; 
-				wake_up(&lp->event);
+				wake_up(&lp->event); 
 				break;
 			default:
 				printk("%s: strange tx ack %d\n", dev->name, status&7);
@@ -1435,8 +1451,8 @@
 				   
 				if (lp->mc_reload_wait) 
 					mc32_reset_multicast_list(dev);
-				else 
-					wake_up(&lp->event);			       
+				else
+					wake_up(&lp->event);
 			}
 		}
 		if(status&2)
@@ -1450,7 +1466,6 @@
 		}
 	}
 
-
 	/*
 	 *	Process the transmit and receive rings 
          */
@@ -1461,6 +1476,8 @@
 	if(rx_event) 
 		mc32_rx_ring(dev);
 
+	spin_unlock_irqrestore(&lp->lock, flags);
+
 	return IRQ_HANDLED;
 }
 
@@ -1492,6 +1509,8 @@
 	struct mc32_local *lp = (struct mc32_local *)dev->priv;
 
 	int ioaddr = dev->base_addr;
+	unsigned long flags;
+
 	u8 regs;
 	u16 one=1;
 	
@@ -1509,9 +1528,11 @@
 	mc32_halt_transceiver(dev); 
 	
 	/* Catch any waiting commands */
-	
+
+	spin_lock_irqsave(&lp->lock, flags);
 	while(lp->exec_pending==1)
-		sleep_on(&lp->event);
+		wait_pending(lp);
+	spin_unlock_irqrestore(&lp->lock, flags);
 	       
 	/* Ok the card is now stopping */	
 	

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

[Felipe W Damasio]

	Hi,

Felipe W Damasio wrote:
>     So I think we can use something like the attached patch, then..which 
> get the lock before the while loop on mc32_interrupt and releases it 
> after processing the interrupts.
> 
>     Richard, could you please test it?

	Richard, did you test the driver with this last patch?

	If it fixes the SMP support correctly, then we can push it to Linus 
for merging on mainline.

	Thanks,

Felipe

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

[Richard Procter]

On Mon, 8 Sep 2003, Felipe W Damasio wrote:

> 	Richard, did you test the driver with this last patch?

Hey Felipe, 

I've had a good look over your revised patch, and it looks fine to me. I
didn't manage to get an MCA kernel booting to test it, but I'm not sure if
it would have added a lot, especially as I don't have an SMP MCA machine.

That said, over the weekend I realised that the need to unroll wait_event
was a consequence of using the same queue to perform two quite distinct
functions: serialising the issuing of commands, and waiting for the card
to complete command execution. This forces us to use a private variable to
indicate which situation has occured. That's ok on UP, but requires us to
jump through hoops to use it safely on SMP with spinlocks. 

I've rewritten things using completions (== semaphores?), and it's both
cleaner and (unexpectedly) smaller (see example below). I'm in the process
of convincing myself it all works; should have something out there by the
end of the week.

If there's a merge deadline coming up, please feel free to submit the
patch, otherwise I'd like to hold off for a couple of days and see where
we stand then.

best, 
Richard. 

Object size of the function below: 
  -- original sti/cli driver: 238 bytes. 
  -- with spinlocks + inlined wait_event unrolling: 1833 bytes (770% of original)
  -- using completions: 190 bytes (80% of original, but with
     three completions structs per card instead of a lock + wait_queue_head)

static int mc32_command(struct net_device *dev, u16 cmd, void *data, int len)
{
	struct mc32_local *lp = (struct mc32_local *)dev->priv;
	int ioaddr = dev->base_addr;
	int ret = 0;
	
	/* (Initially complete) */ 

	wait_for_completion(&lp->current_command); 
	
	/*
	 *     My Turn 
	 */

	lp->exec_nonblocking=0; 
	lp->exec_box->mbox=0;
	lp->exec_box->mbox=cmd;
	memcpy((void *)lp->exec_box->data, data, len);
	barrier();	/* the memcpy forgot the volatile so be sure */

	while(!(inb(ioaddr+HOST_STATUS)&HOST_STATUS_CRR));
	outb(1<<6, ioaddr+HOST_CMD);		

	wait_for_completion(&lp->execution); 
	
	if(lp->exec_box->mbox&(1<<13))
	  ret = -1;

	/* ** on SMP, we could starve the mc_reload wait as 
	 * other threads waiting for completion could block the reload.
	 * a problem? solutions? 
	 */ 

	complete(&lp->current_command); 
	
	/*
	 *	A multicast set got blocked - try it now
	 */
		
	if(lp->mc_reload_wait)
	{
		mc32_reset_multicast_list(dev);
	}

	return ret;
}

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

[Felipe W Damasio]

	Hi Richard,

Richard Procter wrote:
> I've had a good look over your revised patch, and it looks fine to me. I
> didn't manage to get an MCA kernel booting to test it, but I'm not sure if
> it would have added a lot, especially as I don't have an SMP MCA machine.

	That's closer the a proper test box than me, since I don't even have 
an that NIC ;)

> That said, over the weekend I realised that the need to unroll wait_event
> was a consequence of using the same queue to perform two quite distinct
> functions: serialising the issuing of commands, and waiting for the card
> to complete command execution. This forces us to use a private variable to
> indicate which situation has occured. That's ok on UP, but requires us to
> jump through hoops to use it safely on SMP with spinlocks. 

	True, but since the patch uses a per-device lock, aren't we safe (and 
relatively scaleable) on SMP too?

	Using wait_event as "prepare_for_wait" and all that IMHO is needed 
because wait_event calls schedule, and we can't do that with our 
device lock held..hence the prepare_to_wait/spin_unlock/schedule stuff 
on mc32::wait_pending.

	I don't know if using 2 different locks is worth it, since we may 
starve mc_reload on SMP...but since the ammount of code dropped, it's 
worth testing to see if we scale better than the inline wait_pending 
stuff.

> I've rewritten things using completions (== semaphores?), and it's both
> cleaner and (unexpectedly) smaller (see example below). I'm in the process
> of convincing myself it all works; should have something out there by the
> end of the week.

	Great!

	Please let me know when you have something...I'm really enjoying 
helping to fix this bug. :)

> If there's a merge deadline coming up, please feel free to submit the
> patch, otherwise I'd like to hold off for a couple of days and see where
> we stand then.

	There isn't.

	Maybe we won't make 2.6.0-test5, but there's no need to rush things. 
Let's get this right.

	Kind Regards,

Felipe