Below is a patch from Denis Vlasenko <[email protected]> which
removes cli/sti in the ewrk3 driver. It tests out fine here with SMP & preempt.
Applies against 2.5.34 + ewrk3-ethtool patch. Also applies without ethtool patch
with some offsets.
(Denis, I took the liberty of forwarding this to Jeff since it works fine for me
and the driver is pretty much useless without it. Scream if you don't want it
applied...)
--Adam
--- linux-2.5.43-mm1/drivers/net/ewrk3.c.orig Fri Oct 18 21:55:44 2002
+++ linux-2.5.43-mm1/drivers/net/ewrk3.c Fri Oct 18 22:02:41 2002
@@ -940,6 +940,7 @@
spin_unlock(&lp->hw_lock);
}
+/* Called with lp->hw_lock held */
static int ewrk3_rx(struct net_device *dev)
{
struct ewrk3_private *lp = (struct ewrk3_private *) dev->priv;
@@ -1065,8 +1066,9 @@
}
/*
- ** Buffer sent - check for TX buffer errors.
- */
+** Buffer sent - check for TX buffer errors.
+** Called with lp->hw_lock held
+*/
static int ewrk3_tx(struct net_device *dev)
{
struct ewrk3_private *lp = (struct ewrk3_private *) dev->priv;
@@ -1830,6 +1832,7 @@
u_long iobase = dev->base_addr;
int i, j, status = 0;
u_char csr;
+ unsigned long flags;
union ewrk3_addr {
u_char addr[HASH_TABLE_LEN * ETH_ALEN];
u_short val[(HASH_TABLE_LEN * ETH_ALEN) >> 1];
@@ -1953,19 +1956,26 @@
}
break;
- case EWRK3_GET_STATS: /* Get the driver statistics */
- cli();
- ioc->len = sizeof(lp->pktStats);
- if (copy_to_user(ioc->data, &lp->pktStats, ioc->len))
- status = -EFAULT;
- sti();
+ case EWRK3_GET_STATS: { /* Get the driver statistics */
+ typeof(lp->pktStats) *tmp_stats =
+ kmalloc(sizeof(lp->pktStats), GFP_KERNEL);
+ if (!tmp_stats) return -ENOMEM;
+ spin_lock_irqsave(&lp->hw_lock, flags);
+ memcpy(tmp_stats, &lp->pktStats, sizeof(lp->pktStats));
+ spin_unlock_irqrestore(&lp->hw_lock, flags);
+
+ ioc->len = sizeof(lp->pktStats);
+ if (copy_to_user(ioc->data, tmp_stats, sizeof(lp->pktStats)))
+ status = -EFAULT;
+ kfree(tmp_stats);
break;
+ }
case EWRK3_CLR_STATS: /* Zero out the driver statistics */
if (capable(CAP_NET_ADMIN)) {
- cli();
+ spin_lock_irqsave(&lp->hw_lock, flags);
memset(&lp->pktStats, 0, sizeof(lp->pktStats));
- sti();
+ spin_unlock_irqrestore(&lp->hw_lock,flags);
} else {
status = -EPERM;
}
> Applies against 2.5.34 + ewrk3-ethtool patch. Also applies without ethtool patch
^^
2.5.43, of course.
--Adam
On 19 October 2002 02:13, Adam Kropelin wrote:
> Below is a patch from Denis Vlasenko
> <[email protected]> which removes cli/sti in the
> ewrk3 driver. It tests out fine here with SMP & preempt.
Thanks. SMP & preempt testing is most important.
> Applies against 2.5.34 + ewrk3-ethtool patch. Also applies without
> ethtool patch with some offsets.
>
> (Denis, I took the liberty of forwarding this to Jeff since it works
> fine for me and the driver is pretty much useless without it. Scream
> if you don't want it applied...)
No problem.
I just want drivers to be _thoroughly_ tested while we fix them since
a flaky driver can spoil all efforts spent on making bulletproof core
kernel.
What methods do folks use to break NIC drivers?
It would be nice to have crash-my-nic toolset :-)
BTW, I wanted to learn about pktgen, but there's no pktgen in 2.5
(don't know why).
--
vda
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.751 -> 1.752
# drivers/net/ewrk3.c 1.15 -> 1.16
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/10/21 [email protected] 1.752
# Remove cli/sti from ewrk3 net driver.
# Also, comment out ETHTOOL_PHYS_ID until its sleeping is fixed.
#
# Originally by Denis Vlasenko, then via Adam Kropelin, and
# finally cleaned up by me.
# --------------------------------------------
#
diff -Nru a/drivers/net/ewrk3.c b/drivers/net/ewrk3.c
--- a/drivers/net/ewrk3.c Mon Oct 21 20:35:04 2002
+++ b/drivers/net/ewrk3.c Mon Oct 21 20:35:04 2002
@@ -262,20 +262,22 @@
#define EWRK3_PKT_BIN_SZ 128 /* Should be >=100 unless you
increase EWRK3_PKT_STAT_SZ */
+struct ewrk3_stats {
+ u32 bins[EWRK3_PKT_STAT_SZ];
+ u32 unicast;
+ u32 multicast;
+ u32 broadcast;
+ u32 excessive_collisions;
+ u32 tx_underruns;
+ u32 excessive_underruns;
+};
+
struct ewrk3_private {
char adapter_name[80]; /* Name exported to /proc/ioports */
u_long shmem_base; /* Shared memory start address */
u_long shmem_length; /* Shared memory window length */
struct net_device_stats stats; /* Public stats */
- struct {
- u32 bins[EWRK3_PKT_STAT_SZ]; /* Private stats counters */
- u32 unicast;
- u32 multicast;
- u32 broadcast;
- u32 excessive_collisions;
- u32 tx_underruns;
- u32 excessive_underruns;
- } pktStats;
+ struct ewrk3_stats pktStats; /* Private stats counters */
u_char irq_mask; /* Adapter IRQ mask bits */
u_char mPage; /* Maximum 2kB Page number */
u_char lemac; /* Chip rev. level */
@@ -940,6 +942,7 @@
spin_unlock(&lp->hw_lock);
}
+/* Called with lp->hw_lock held */
static int ewrk3_rx(struct net_device *dev)
{
struct ewrk3_private *lp = (struct ewrk3_private *) dev->priv;
@@ -1065,8 +1068,9 @@
}
/*
- ** Buffer sent - check for TX buffer errors.
- */
+** Buffer sent - check for TX buffer errors.
+** Called with lp->hw_lock held
+*/
static int ewrk3_tx(struct net_device *dev)
{
struct ewrk3_private *lp = (struct ewrk3_private *) dev->priv;
@@ -1760,6 +1764,7 @@
return 0;
}
+#ifdef BROKEN
/* Blink LED for identification */
case ETHTOOL_PHYS_ID: {
struct ethtool_value edata;
@@ -1813,6 +1818,7 @@
spin_unlock_irqrestore(&lp->hw_lock, flags);
return ret;
}
+#endif /* BROKEN */
}
@@ -1830,6 +1836,7 @@
u_long iobase = dev->base_addr;
int i, j, status = 0;
u_char csr;
+ unsigned long flags;
union ewrk3_addr {
u_char addr[HASH_TABLE_LEN * ETH_ALEN];
u_short val[(HASH_TABLE_LEN * ETH_ALEN) >> 1];
@@ -1953,19 +1960,26 @@
}
break;
- case EWRK3_GET_STATS: /* Get the driver statistics */
- cli();
- ioc->len = sizeof(lp->pktStats);
- if (copy_to_user(ioc->data, &lp->pktStats, ioc->len))
- status = -EFAULT;
- sti();
+ case EWRK3_GET_STATS: { /* Get the driver statistics */
+ struct ewrk3_stats *tmp_stats =
+ kmalloc(sizeof(lp->pktStats), GFP_KERNEL);
+ if (!tmp_stats) return -ENOMEM;
+ spin_lock_irqsave(&lp->hw_lock, flags);
+ memcpy(tmp_stats, &lp->pktStats, sizeof(lp->pktStats));
+ spin_unlock_irqrestore(&lp->hw_lock, flags);
+
+ ioc->len = sizeof(lp->pktStats);
+ if (copy_to_user(ioc->data, tmp_stats, sizeof(lp->pktStats)))
+ status = -EFAULT;
+ kfree(tmp_stats);
break;
+ }
case EWRK3_CLR_STATS: /* Zero out the driver statistics */
if (capable(CAP_NET_ADMIN)) {
- cli();
+ spin_lock_irqsave(&lp->hw_lock, flags);
memset(&lp->pktStats, 0, sizeof(lp->pktStats));
- sti();
+ spin_unlock_irqrestore(&lp->hw_lock,flags);
} else {
status = -EPERM;
}
Adam Kropelin wrote:
> On Mon, Oct 21, 2002 at 08:36:17PM -0400, Jeff Garzik wrote:
>
>>Adam Kropelin wrote:
>>
>>>Below is a patch from Denis Vlasenko <[email protected]>
>>>which
>>>removes cli/sti in the ewrk3 driver. It tests out fine here with SMP &
>>>preempt.
>>
>>Applied and then cleaned up... ETHTOOL_PHYS_ID needs to use
>>schedule_timeout(), and using typeof should be avoided.
>
>
> The patch below (against yours) should fix ETHTOOL_PHYS_ID again. Let me
> know if I got it wrong. Was my prior use of
> wait_event_interruptible_tiumeout actually broken or rather
> just a lot more complicated than it needed to be?
broken in 2.4 -and- more complicated than it needed to be in 2.5.x ;-)
> I didn't find any use of typeof. If you point me to it I'll fix that up,
> too.
I fixed it up... grep your original patch. typeof() was used because
lp->pktStats was an anonymous structure with no defined typename. I
moved the struct out and gave it a name.
> Thanks for bearing with me. I'm climbing the clue-ladder as fast as I
> can... ;)
hehe, thanks for doing the patches and the testing... :)
> --- linux-2.5.44/drivers/net/ewrk3.c Mon Oct 21 22:01:01 2002
> +++ linux-2.5.44/drivers/net/ewrk3.c.new Mon Oct 21 21:58:13 2002
quick review says it looks good, I'll make another pass over it with my
brain switched on, and apply or comment as it warrants :)
Jeff
On Mon, Oct 21, 2002 at 08:36:17PM -0400, Jeff Garzik wrote:
> Adam Kropelin wrote:
> >Below is a patch from Denis Vlasenko <[email protected]>
> >which
> >removes cli/sti in the ewrk3 driver. It tests out fine here with SMP &
> >preempt.
>
> Applied and then cleaned up... ETHTOOL_PHYS_ID needs to use
> schedule_timeout(), and using typeof should be avoided.
The patch below (against yours) should fix ETHTOOL_PHYS_ID again. Let me
know if I got it wrong. Was my prior use of
wait_event_interruptible_tiumeout actually broken or rather
just a lot more complicated than it needed to be?
I didn't find any use of typeof. If you point me to it I'll fix that up,
too.
Thanks for bearing with me. I'm climbing the clue-ladder as fast as I
can... ;)
--Adam
--- linux-2.5.44/drivers/net/ewrk3.c Mon Oct 21 22:01:01 2002
+++ linux-2.5.44/drivers/net/ewrk3.c.new Mon Oct 21 21:58:13 2002
@@ -1759,23 +1759,18 @@
return 0;
}
-#ifdef BROKEN
/* Blink LED for identification */
case ETHTOOL_PHYS_ID: {
struct ethtool_value edata;
u_long flags;
- long delay, ret;
+ long ret=0;
u_char cr;
int count;
- wait_queue_head_t wait;
-
- init_waitqueue_head(&wait);
if (copy_from_user(&edata, useraddr, sizeof(edata)))
return -EFAULT;
/* Toggle LED 4x per second */
- delay = HZ >> 2;
count = edata.data << 2;
spin_lock_irqsave(&lp->hw_lock, flags);
@@ -1796,24 +1791,21 @@
/* Wait a little while */
spin_unlock_irqrestore(&lp->hw_lock, flags);
- ret = delay;
- __wait_event_interruptible_timeout(wait, 0, ret);
+ set_current_state(TASK_INTERRUPTIBLE);
+ ret = schedule_timeout(HZ>>2);
spin_lock_irqsave(&lp->hw_lock, flags);
/* Exit if we got a signal */
- if (ret == -ERESTARTSYS)
- goto out;
+ if (ret)
+ break;
}
- ret = 0;
-out:
lp->led_mask = CR_LED;
cr = inb(EWRK3_CR);
outb(cr & ~CR_LED, EWRK3_CR);
spin_unlock_irqrestore(&lp->hw_lock, flags);
- return ret;
+ return ret ? -ERESTARTSYS : 0;
}
-#endif /* BROKEN */
}
Adam Kropelin wrote:
>On Mon, Oct 21, 2002 at 08:36:17PM -0400, Jeff Garzik wrote:
>
>
>>Adam Kropelin wrote:
>>
>>
>>>Below is a patch from Denis Vlasenko <[email protected]>
>>>which
>>>removes cli/sti in the ewrk3 driver. It tests out fine here with SMP &
>>>preempt.
>>>
>>>
>>Applied and then cleaned up... ETHTOOL_PHYS_ID needs to use
>>schedule_timeout(), and using typeof should be avoided.
>>
>>
>
>The patch below (against yours) should fix ETHTOOL_PHYS_ID again. Let me
>know if I got it wrong. Was my prior use of
>wait_event_interruptible_tiumeout actually broken or rather
>just a lot more complicated than it needed to be?
>
>I didn't find any use of typeof. If you point me to it I'll fix that up,
>too.
>
>Thanks for bearing with me. I'm climbing the clue-ladder as fast as I
>can... ;)
>
>--Adam
>
>--- linux-2.5.44/drivers/net/ewrk3.c Mon Oct 21 22:01:01 2002
>+++ linux-2.5.44/drivers/net/ewrk3.c.new Mon Oct 21 21:58:13 2002
>@@ -1759,23 +1759,18 @@
> return 0;
> }
>
>-#ifdef BROKEN
> /* Blink LED for identification */
> case ETHTOOL_PHYS_ID: {
> struct ethtool_value edata;
> u_long flags;
>- long delay, ret;
>+ long ret=0;
> u_char cr;
> int count;
>- wait_queue_head_t wait;
>-
>- init_waitqueue_head(&wait);
>
> if (copy_from_user(&edata, useraddr, sizeof(edata)))
> return -EFAULT;
>
> /* Toggle LED 4x per second */
>- delay = HZ >> 2;
> count = edata.data << 2;
>
> spin_lock_irqsave(&lp->hw_lock, flags);
>@@ -1796,24 +1791,21 @@
>
> /* Wait a little while */
> spin_unlock_irqrestore(&lp->hw_lock, flags);
>- ret = delay;
>- __wait_event_interruptible_timeout(wait, 0, ret);
>+ set_current_state(TASK_INTERRUPTIBLE);
>+ ret = schedule_timeout(HZ>>2);
>
>
close -- if schedule_timeout() returns greater than zero, that number is
the remaining jiffies that schedule_timeout _should_ have slept, but did
not. Ideally you need to call it in a loop, that decrements a variable
based on schedule_timeout return code.
On Fri, Oct 25, 2002 at 06:45:04PM -0400, Jeff Garzik wrote:
> Adam Kropelin wrote:
> >
> > /* Wait a little while */
> > spin_unlock_irqrestore(&lp->hw_lock, flags);
> >- ret = delay;
> >- __wait_event_interruptible_timeout(wait, 0, ret);
> >+ set_current_state(TASK_INTERRUPTIBLE);
> >+ ret = schedule_timeout(HZ>>2);
> >
>
> close -- if schedule_timeout() returns greater than zero, that number is
> the remaining jiffies that schedule_timeout _should_ have slept, but did
> not. Ideally you need to call it in a loop, that decrements a variable
> based on schedule_timeout return code.
My assumption was that the only case in which schedule_timeout() would
return without completing the sleep is if the process was delivered a
signal. So I break the loop immediately in that case. I presume from
your explanation that schedule_timeout() may return for some other reason
(out of curiousity, what?)...and in that case I need to check for a
pending signal, exit if there is one, otherwise schedule_timeout() for
the remaining time. Am I getting warmer?
--Adam
On Fri, Oct 25, 2002 at 06:45:04PM -0400, Jeff Garzik wrote:
> Adam Kropelin wrote:
> > spin_unlock_irqrestore(&lp->hw_lock, flags);
> >- ret = delay;
> >- __wait_event_interruptible_timeout(wait, 0, ret);
> >+ set_current_state(TASK_INTERRUPTIBLE);
> >+ ret = schedule_timeout(HZ>>2);
>
> close -- if schedule_timeout() returns greater than zero, that number is
> the remaining jiffies that schedule_timeout _should_ have slept, but did
> not. Ideally you need to call it in a loop, that decrements a variable
> based on schedule_timeout return code.
Try this one on for size...
--Adam
--- linux-2.5.45-virgin/drivers/net/ewrk3.c Wed Oct 30 22:55:10 2002
+++ linux-2.5.45/drivers/net/ewrk3.c Fri Nov 1 17:15:13 2002
@@ -1759,23 +1759,18 @@
return 0;
}
-#ifdef BROKEN
/* Blink LED for identification */
case ETHTOOL_PHYS_ID: {
struct ethtool_value edata;
u_long flags;
- long delay, ret;
+ long ret=0;
u_char cr;
int count;
- wait_queue_head_t wait;
-
- init_waitqueue_head(&wait);
if (copy_from_user(&edata, useraddr, sizeof(edata)))
return -EFAULT;
/* Toggle LED 4x per second */
- delay = HZ >> 2;
count = edata.data << 2;
spin_lock_irqsave(&lp->hw_lock, flags);
@@ -1796,24 +1791,24 @@
/* Wait a little while */
spin_unlock_irqrestore(&lp->hw_lock, flags);
- ret = delay;
- __wait_event_interruptible_timeout(wait, 0, ret);
+ ret = HZ>>2;
+ while(ret && !signal_pending(current)) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ ret = schedule_timeout(ret);
+ }
spin_lock_irqsave(&lp->hw_lock, flags);
/* Exit if we got a signal */
- if (ret == -ERESTARTSYS)
- goto out;
+ if (ret)
+ break;
}
- ret = 0;
-out:
lp->led_mask = CR_LED;
cr = inb(EWRK3_CR);
outb(cr & ~CR_LED, EWRK3_CR);
spin_unlock_irqrestore(&lp->hw_lock, flags);
- return ret;
+ return ret ? -ERESTARTSYS : 0;
}
-#endif /* BROKEN */
}