* Re: [PATCH] Fix SMP support on 3c527 net driver
@ 2003-08-31 8:28 Manfred Spraul
2003-08-31 16:50 ` Felipe W Damasio
0 siblings, 1 reply; 4+ messages in thread
From: Manfred Spraul @ 2003-08-31 8:28 UTC (permalink / raw)
To: Felipe W Damasio; +Cc: linux-kernel, rnp
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix SMP support on 3c527 net driver
2003-08-31 8:28 [PATCH] Fix SMP support on 3c527 net driver Manfred Spraul
@ 2003-08-31 16:50 ` Felipe W Damasio
2003-08-31 17:30 ` Manfred Spraul
0 siblings, 1 reply; 4+ messages in thread
From: Felipe W Damasio @ 2003-08-31 16:50 UTC (permalink / raw)
To: Manfred Spraul; +Cc: linux-kernel, rnp
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix SMP support on 3c527 net driver
2003-08-31 16:50 ` Felipe W Damasio
@ 2003-08-31 17:30 ` Manfred Spraul
0 siblings, 0 replies; 4+ messages in thread
From: Manfred Spraul @ 2003-08-31 17:30 UTC (permalink / raw)
To: Felipe W Damasio; +Cc: linux-kernel, rnp
[-- 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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Fix SMP support on 3c527 net driver
@ 2003-08-31 3:06 Felipe W Damasio
0 siblings, 0 replies; 4+ messages in thread
From: Felipe W Damasio @ 2003-08-31 3:06 UTC (permalink / raw)
To: rnp; +Cc: linux-kernel
[-- 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;
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2003-08-31 17:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-08-31 8:28 [PATCH] Fix SMP support on 3c527 net driver Manfred Spraul
2003-08-31 16:50 ` Felipe W Damasio
2003-08-31 17:30 ` Manfred Spraul
-- strict thread matches above, loose matches on Subject: below --
2003-08-31 3:06 Felipe W Damasio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).