--- 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 ([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;
--- 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 ([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.
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
>
--- 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 ([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 */
--- 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 ([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 */
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.
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
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;
}
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