Fix SMP support on 3c527 net driver, take 2
PATCH 1/1 Fix SMP support on 3c527 net driver, take 2
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 ofsleep_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
drivers/net/3c527.c +46 -33
@@ -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 ([email protected])\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 ;
Hi Richard,
Quoted 5 lines · Felipe W Damasio
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_eventinstead ofsleep_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
drivers/net/3c527.c +46 -33
@@ -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 ([email protected])\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 ;
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.
Quoted 11 lines · Felipe W Damasio
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
Hi Richard,
Quoted 2 lines · Richard Procter
*
mc32_interruptdoesn'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?
Quoted 4 lines
* sleep_on's remain in
mc32_halt_transceiverandmc32_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.
Quoted 2 lines
* Just noticed an old error in
mc32_close---sleep_onis called in without first disabling interrupts.
This is fixed since we have to hold the device lock before callingwait_pending, so it should be fixed too.
Quoted 3 lines
I've had a go at testing it, but ran into troubles with the
ibmmca.cdriver 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
drivers/net/3c527.c +56 -40
@@ -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 ([email protected])\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 */
Quoted 5 lines · Felipe W Damasio
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.
Quoted 9 lines · Alan Cox
Quoted 4 lines · Felipe W Damasio
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_upwill 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
drivers/net/3c527.c +61 -40
@@ -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 ([email protected])\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 */
Hi,
Quoted 4 lines · Felipe W Damasio
So I think we can use something like the attached patch, then..which
get the lock before the while loop onmc32_interruptand 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
Quoted 1 line · Felipe W Damasio
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;
}Hi Richard,
Quoted 3 lines · Richard Procter
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 ;)
Quoted 6 lines
That said, over the weekend I realised that the need to unroll
wait_eventwas 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.
Quoted 4 lines
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. :)
Quoted 3 lines
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