Hi,
drivers/net/ni5010.c:
This patch replaces cli/sti with spinlocks. Compiles fine though
untested.
Vinay
--- linux-2.6.0-test4/drivers/net/ni5010.c 2003-07-15 17:22:18.000000000 +0530
+++ linux-2.6.0-test4-nvk/drivers/net/ni5010.c 2003-08-24 20:29:35.000000000 +0530
@@ -96,6 +96,7 @@
struct net_device_stats stats;
int o_pkt_size;
int i_pkt_size;
+ spinlock_t tx_lock;
};
/* Index to functions, as function prototypes. */
@@ -280,11 +281,16 @@
/* DMA is not supported (yet?), so no use detecting it */
if (dev->priv == NULL) {
+ struct ni5010_local* lp;
+
dev->priv = kmalloc(sizeof(struct ni5010_local), GFP_KERNEL|GFP_DMA);
if (dev->priv == NULL) {
printk(KERN_WARNING "%s: Failed to allocate private memory\n", dev->name);
return -ENOMEM;
}
+
+ lp = (struct ni5010_local*)dev->priv;
+ spin_lock_init(&lp->tx_lock);
}
PRINTK2((KERN_DEBUG "%s: I/O #10 passed!\n", dev->name));
@@ -693,8 +699,7 @@
buf_offs = NI5010_BUFSIZE - length - pad;
lp->o_pkt_size = length + pad;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lp->tx_lock, flags);
outb(0, EDLC_RMASK); /* Mask all receive interrupts */
outb(0, IE_MMODE); /* Put Xmit buffer on system bus */
@@ -712,7 +717,7 @@
outb(MM_EN_XMT | MM_MUX, IE_MMODE); /* Begin transmission */
outb(XM_ALL, EDLC_XMASK); /* Cause interrupt after completion or fail */
- restore_flags(flags);
+ spin_unlock_irqrestore(&lp->tx_lock, flags);
netif_wake_queue(dev);
Vinay wrote:
>drivers/net/ni5010.c:
>This patch replaces cli/sti with spinlocks. Compiles fine though
>untested.
>
>
I don't have the hardware either, but the patch seems to be wrong:
The cli() call was in hardware_send_packet(). I assume that this
function should be synchronized against concurrent interrupts - both
functions access the hardware. Due to the dev_xmit_lock, the networking
core already guarantees that hardware_send_packet is never reentered.
cli() provided protection against concurrent interrupts on other cpus,
spin_lock_irqsave() doesn't.
Could you add a spin_lock()/spin_unlock() into ni5010_interrupt?
--
Manfred
Hi,
On Sun, 2003-08-24 at 22:01, Manfred Spraul wrote:
> Vinay wrote:
>
> >drivers/net/ni5010.c:
> >This patch replaces cli/sti with spinlocks. Compiles fine though
> >untested.
> >
> >
> I don't have the hardware either, but the patch seems to be wrong:
> The cli() call was in hardware_send_packet(). I assume that this
> function should be synchronized against concurrent interrupts - both
> functions access the hardware. Due to the dev_xmit_lock, the networking
> core already guarantees that hardware_send_packet is never reentered.
> cli() provided protection against concurrent interrupts on other cpus,
> spin_lock_irqsave() doesn't.
>
> Could you add a spin_lock()/spin_unlock() into ni5010_interrupt?
Thanks for the suggestions. I have updated the patch accordingly. Hope
the locking is sufficient.
Thanks
vinay
--- linux-2.6.0-test4/drivers/net/ni5010.c 2003-07-15 17:22:18.000000000 +0530
+++ linux-2.6.0-test4-nvk/drivers/net/ni5010.c 2003-08-25 16:56:02.000000000 +0530
@@ -96,6 +96,7 @@
struct net_device_stats stats;
int o_pkt_size;
int i_pkt_size;
+ spinlock_t lock;
};
/* Index to functions, as function prototypes. */
@@ -280,11 +281,16 @@
/* DMA is not supported (yet?), so no use detecting it */
if (dev->priv == NULL) {
+ struct ni5010_local* lp;
+
dev->priv = kmalloc(sizeof(struct ni5010_local), GFP_KERNEL|GFP_DMA);
if (dev->priv == NULL) {
printk(KERN_WARNING "%s: Failed to allocate private memory\n", dev->name);
return -ENOMEM;
}
+
+ lp = (struct ni5010_local*)dev->priv;
+ spin_lock_init(&lp->lock);
}
PRINTK2((KERN_DEBUG "%s: I/O #10 passed!\n", dev->name));
@@ -463,6 +469,7 @@
ioaddr = dev->base_addr;
lp = (struct ni5010_local *)dev->priv;
+ spin_lock(&lp->lock);
status = inb(IE_ISTAT);
PRINTK3((KERN_DEBUG "%s: IE_ISTAT = %#02x\n", dev->name, status));
@@ -479,6 +486,7 @@
if (!xmit_was_error)
reset_receiver(dev);
+ spin_unlock(&lp->lock);
return IRQ_HANDLED;
}
@@ -693,8 +701,7 @@
buf_offs = NI5010_BUFSIZE - length - pad;
lp->o_pkt_size = length + pad;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lp->lock, flags);
outb(0, EDLC_RMASK); /* Mask all receive interrupts */
outb(0, IE_MMODE); /* Put Xmit buffer on system bus */
@@ -712,7 +719,7 @@
outb(MM_EN_XMT | MM_MUX, IE_MMODE); /* Begin transmission */
outb(XM_ALL, EDLC_XMASK); /* Cause interrupt after completion or fail */
- restore_flags(flags);
+ spin_unlock_irqrestore(&lp->lock, flags);
netif_wake_queue(dev);