[ 2nd transmission. Microsoft mailer "helpfully"
reformatted the patch in the last one... :-( ]
Greetings,
We have identified two related bugs in the e100 driver and we request
that they be repaired in the official Intel version of the driver.
Both bugs are related to manipulation of the MDI control register.
The first problem is that the Ready bit is being ignored when
writing to the Control register; we noticed this because the Linux
bonding driver would occasionally come to the spurious conclusion
that the link was down when querying Link State. It turned out
that by failing to wait for a previous command to complete it was
selecting what was essentially a random register in the MDI register
set. When we added code that waits for the Ready bit (as shown in
the patch file below) all such problems ceased.
The second problem is that, although access to the MDI registers
involves multiple steps which must not be intermixed, nothing was
defending against two or more threads attempting simultaneous access.
The most obvious situation where such interference could occur
involves the watchdog versus ioctl paths, but there are probably
others, so we recommend the locking shown in our patch file.
Thanks,
--Michael O'Donnell, Stratus Technologies, Maynard, MA USA
Signed-off-by: Michael O'Donnell <Michael.ODonnell at stratus dot com>
--- drivers/net/e100.c.orig 2006-01-06 10:28:13.000000000 -0500
+++ drivers/net/e100.c 2006-01-06 10:51:57.000000000 -0500
@@ -125,20 +125,24 @@
* not supported (hardware limitation).
*
* MagicPacket(tm) WoL support is enabled/disabled via ethtool.
*
* Thanks to JC ([email protected]) for helping with
* testing/troubleshooting the development driver.
*
* TODO:
* o several entry points race with dev->close
* o check for tx-no-resources/stop Q races with tx clean/wake Q
+ *
+ * FIXES:
+ * 2005/12/02 - Michael O'Donnell <Michael.ODonnell at stratus dot com>
+ * - Stratus87247: protect MDI control register manipulations
*/
#include <linux/config.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/kernel.h>
#include <linux/types.h>
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/init.h>
@@ -572,20 +577,21 @@ struct nic {
u32 rx_fc_pause;
u32 rx_fc_unsupported;
u32 rx_tco_frames;
u32 rx_over_length_errors;
u8 rev_id;
u16 leds;
u16 eeprom_wc;
u16 eeprom[256];
u32 pm_state[16];
+ spinlock_t mdio_lock;
};
static inline void e100_write_flush(struct nic *nic)
{
/* Flush previous PCI writes through intermediate bridges
* by doing a benign read */
(void)readb(&nic->csr->scb.status);
}
static inline void e100_enable_irq(struct nic *nic)
@@ -869,29 +894,49 @@ static inline int e100_exec_cb(struct ni
err_unlock:
spin_unlock_irqrestore(&nic->cb_lock, flags);
return err;
}
static u16 mdio_ctrl(struct nic *nic, u32 addr, u32 dir, u32 reg, u16
data)
{
u32 data_out = 0;
unsigned int i;
+ unsigned long flags;
+
+ /*
+ * Stratus87247: we shouldn't be writing the MDI control
+ * register until the Ready bit shows True. Also, since
+ * manipulation of the MDI control registers is a multi-step
+ * procedure it should be done under lock.
+ */
+ spin_lock_irqsave( &(nic->mdio_lock), flags );
+ for( i = 100; i; --i ) {
+ if( readl( &(nic->csr->mdi_ctrl) ) & mdi_ready ) {
+ break;
+ }
+ udelay( 20 );
+ }
+ if( unlikely( !i ) ) {
+ printk( "e100.mdio_ctrl(%s)won't go Ready\n",
nic->netdev->name );
+ spin_unlock_irqrestore( &(nic->mdio_lock), flags );
+ return( 0 ); /* No way to indicate timeout
error */
+ }
writel((reg << 16) | (addr << 21) | dir | data,
&nic->csr->mdi_ctrl);
for(i = 0; i < 100; i++) {
udelay(20);
if((data_out = readl(&nic->csr->mdi_ctrl)) & mdi_ready)
break;
}
-
+ spin_unlock_irqrestore( &(nic->mdio_lock), flags );
DPRINTK(HW, DEBUG,
"%s:addr=%d, reg=%d, data_in=0x%04X, data_out=0x%04X\n",
dir == mdi_read ? "READ" : "WRITE", addr, reg, data,
data_out);
return (u16)data_out;
}
static int mdio_read(struct net_device *netdev, int addr, int reg)
{
return mdio_ctrl(netdev_priv(netdev), addr, mdi_read, reg, 0);
}
@@ -2306,20 +2435,21 @@ static int __devinit e100_probe(struct p
if(ent->driver_data)
nic->flags |= ich;
else
nic->flags &= ~ich;
e100_get_defaults(nic);
/* locks must be initialized before calling hw_reset */
spin_lock_init(&nic->cb_lock);
spin_lock_init(&nic->cmd_lock);
+ spin_lock_init(&nic->mdio_lock);
/* Reset the device before pci_set_master() in case device is in
some
* funky state and has an interrupt pending - hint: we don't
have the
* interrupt handler registered yet. */
e100_hw_reset(nic);
pci_set_master(pdev);
init_timer(&nic->watchdog);
nic->watchdog.function = e100_watchdog;
On 1/6/06, ODonnell, Michael <[email protected]> wrote:
> [ 2nd transmission. Microsoft mailer "helpfully"
> reformatted the patch in the last one... :-( ]
>
> Greetings,
>
> We have identified two related bugs in the e100 driver and we request
> that they be repaired in the official Intel version of the driver.
>
> Both bugs are related to manipulation of the MDI control register.
>
> The first problem is that the Ready bit is being ignored when
> writing to the Control register; we noticed this because the Linux
> bonding driver would occasionally come to the spurious conclusion
> that the link was down when querying Link State. It turned out
> that by failing to wait for a previous command to complete it was
> selecting what was essentially a random register in the MDI register
> set. When we added code that waits for the Ready bit (as shown in
> the patch file below) all such problems ceased.
damn, you know I had seen this on one machine only, and the machine
had other problems, so i thought it wasn't e100. I can't quite figure
out why we haven't seen this more often given how long the bug appears
to have existed.
> The second problem is that, although access to the MDI registers
> involves multiple steps which must not be intermixed, nothing was
> defending against two or more threads attempting simultaneous access.
> The most obvious situation where such interference could occur
> involves the watchdog versus ioctl paths, but there are probably
> others, so we recommend the locking shown in our patch file.
Agreed, but once again I am simply amazed this has been there so long.
I think these are both good patches and I'll ack this and absorb it
for our next release. It will be a bit before its completely through
our process but its okay with me if this goes into the kernel now.
Jesse