2002-01-12 21:14:03

by Manfred Spraul

[permalink] [raw]
Subject: [PATCH] Rx FIFO Overrun error found

--- 2.5/drivers/net/natsemi.c Fri Nov 23 20:35:23 2001
+++ build-2.5/drivers/net/natsemi.c Sat Jan 12 20:59:24 2002
@@ -101,6 +101,12 @@

version 1.0.13:
* ETHTOOL_[GS]EEPROM support (Tim Hockin)
+ version 1.0.14:
+ * OOM error handling.
+ * reinit_ring() instead of {drain,init}_ring().
+ * Rx status FIFO overrun bug fixed:
+ The nic sets that bit instead of IntrRxDone,
+ just call netdev_rx and hang is gone.

TODO:
* big endian support with CFG:BEM instead of cpu_to_le32
@@ -161,7 +167,7 @@
There are no ill effects from too-large receive rings. */
#define TX_RING_SIZE 16
#define TX_QUEUE_LEN 10 /* Limit ring entries actually used, min 4. */
-#define RX_RING_SIZE 64
+#define RX_RING_SIZE 32

/* Operational parameters that usually are not changed. */
/* Time in jiffies before concluding the transmitter is hung. */
@@ -612,6 +618,7 @@
unsigned int cur_rx, dirty_rx; /* Producer/consumer ring indices */
unsigned int cur_tx, dirty_tx;
unsigned int rx_buf_sz; /* Based on MTU+slack. */
+ unsigned int oom;
/* These values are keep track of the transceiver/media in use. */
unsigned int full_duplex;
/* Rx filter. */
@@ -640,9 +647,12 @@
static void netdev_timer(unsigned long data);
static void tx_timeout(struct net_device *dev);
static int alloc_ring(struct net_device *dev);
+static void refill_rx(struct net_device *dev);
static void init_ring(struct net_device *dev);
+static void drain_tx(struct net_device *dev);
static void drain_ring(struct net_device *dev);
static void free_ring(struct net_device *dev);
+static void reinit_ring(struct net_device *dev);
static void init_registers(struct net_device *dev);
static int start_tx(struct sk_buff *skb, struct net_device *dev);
static void intr_handler(int irq, void *dev_instance, struct pt_regs *regs);
@@ -1191,11 +1201,15 @@
}

/*
- * The frequency on this has been increased because of a nasty little problem.
+ * netdev_timer:
+ * Purpose:
+ * 1) check for link changes. Usually they are handled by the MII interrupt
+ * 2) check for sudden death of the NIC:
* It seems that a reference set for this chip went out with incorrect info,
* and there exist boards that aren't quite right. An unexpected voltage drop
* can cause the PHY to get itself in a weird state (basically reset..).
* NOTE: this only seems to affect revC chips.
+ * 3) check of death of the RX path due to OOM.
*/
static void netdev_timer(unsigned long data)
{
@@ -1213,17 +1227,22 @@
dev->name);
}

+ spin_lock_irq(&np->lock);
+
/* check for a nasty random phy-reset - use dspcfg as a flag */
writew(1, ioaddr+PGSEL);
dspcfg = readw(ioaddr+DSPCFG);
writew(0, ioaddr+PGSEL);
if (dspcfg != DSPCFG_VAL) {
if (!netif_queue_stopped(dev)) {
+ spin_unlock_irq(&np->lock);
printk(KERN_INFO
"%s: possible phy reset: re-initializing\n",
dev->name);
disable_irq(dev->irq);
spin_lock_irq(&np->lock);
+ natsemi_reset(dev);
+ reinit_ring(dev);
init_registers(dev);
spin_unlock_irq(&np->lock);
enable_irq(dev->irq);
@@ -1233,9 +1252,19 @@
}
} else {
/* init_registers() calls check_link() for the above case */
- spin_lock_irq(&np->lock);
check_link(dev);
- spin_unlock_irq(&np->lock);
+ }
+ spin_unlock_irq(&np->lock);
+ if (np->oom) {
+ disable_irq(dev->irq);
+ np->oom = 0;
+ refill_rx(dev);
+ enable_irq(dev->irq);
+ if (!np->oom) {
+ writel(RxOn, dev->base_addr + ChipCmd);
+ } else {
+ next_tick = 1;
+ }
}
mod_timer(&np->timer, jiffies + next_tick);
}
@@ -1277,8 +1306,7 @@
dump_ring(dev);

natsemi_reset(dev);
- drain_ring(dev);
- init_ring(dev);
+ reinit_ring(dev);
init_registers(dev);
} else {
printk(KERN_WARNING
@@ -1305,15 +1333,53 @@
return 0;
}

+static void refill_rx(struct net_device *dev)
+{
+ struct netdev_private *np = dev->priv;
+
+ /* Refill the Rx ring buffers. */
+ for (; np->cur_rx - np->dirty_rx > 0; np->dirty_rx++) {
+ struct sk_buff *skb;
+ int entry = np->dirty_rx % RX_RING_SIZE;
+ if (np->rx_skbuff[entry] == NULL) {
+ skb = dev_alloc_skb(np->rx_buf_sz);
+ np->rx_skbuff[entry] = skb;
+ if (skb == NULL)
+ break; /* Better luck next round. */
+ skb->dev = dev; /* Mark as being used by this device. */
+ np->rx_dma[entry] = pci_map_single(np->pci_dev,
+ skb->data, skb->len, PCI_DMA_FROMDEVICE);
+ np->rx_ring[entry].addr = cpu_to_le32(np->rx_dma[entry]);
+ }
+ np->rx_ring[entry].cmd_status =
+ cpu_to_le32(np->rx_buf_sz);
+ }
+ if (np->cur_rx - np->dirty_tx == RX_RING_SIZE) {
+ if (debug > 2)
+ printk(KERN_INFO "%s: going OOM.\n", dev->name);
+ np->oom = 1;
+ }
+}
+
/* Initialize the Rx and Tx rings, along with various 'dev' bits. */
static void init_ring(struct net_device *dev)
{
struct netdev_private *np = dev->priv;
int i;

- np->cur_rx = np->cur_tx = 0;
- np->dirty_rx = np->dirty_tx = 0;
+ /* 1) TX ring */
+ np->dirty_tx = np->cur_tx = 0;
+ for (i = 0; i < TX_RING_SIZE; i++) {
+ np->tx_skbuff[i] = NULL;
+ np->tx_ring[i].next_desc = cpu_to_le32(np->ring_dma
+ +sizeof(struct netdev_desc)
+ *((i+1)%TX_RING_SIZE+RX_RING_SIZE));
+ np->tx_ring[i].cmd_status = 0;
+ }

+ /* 2) RX ring */
+ np->dirty_rx = 0;
+ np->cur_rx = RX_RING_SIZE;
np->rx_buf_sz = (dev->mtu <= 1500 ? PKT_BUF_SZ : dev->mtu + 32);
np->rx_head_desc = &np->rx_ring[0];

@@ -1328,29 +1394,25 @@
np->rx_ring[i].cmd_status = cpu_to_le32(DescOwn);
np->rx_skbuff[i] = NULL;
}
+ refill_rx(dev);
+ dump_ring(dev);
+}

- /* Fill in the Rx buffers. Handle allocation failure gracefully. */
- for (i = 0; i < RX_RING_SIZE; i++) {
- struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz);
- np->rx_skbuff[i] = skb;
- if (skb == NULL)
- break;
- skb->dev = dev; /* Mark as being used by this device. */
- np->rx_dma[i] = pci_map_single(np->pci_dev,
- skb->data, skb->len, PCI_DMA_FROMDEVICE);
- np->rx_ring[i].addr = cpu_to_le32(np->rx_dma[i]);
- np->rx_ring[i].cmd_status = cpu_to_le32(np->rx_buf_sz);
- }
- np->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
+static void drain_tx(struct net_device *dev)
+{
+ struct netdev_private *np = dev->priv;
+ int i;

for (i = 0; i < TX_RING_SIZE; i++) {
+ if (np->tx_skbuff[i]) {
+ pci_unmap_single(np->pci_dev,
+ np->rx_dma[i],
+ np->rx_skbuff[i]->len,
+ PCI_DMA_TODEVICE);
+ dev_kfree_skb(np->tx_skbuff[i]);
+ }
np->tx_skbuff[i] = NULL;
- np->tx_ring[i].next_desc = cpu_to_le32(np->ring_dma
- +sizeof(struct netdev_desc)
- *((i+1)%TX_RING_SIZE+RX_RING_SIZE));
- np->tx_ring[i].cmd_status = 0;
}
- dump_ring(dev);
}

static void drain_ring(struct net_device *dev)
@@ -1371,16 +1433,29 @@
}
np->rx_skbuff[i] = NULL;
}
- for (i = 0; i < TX_RING_SIZE; i++) {
- if (np->tx_skbuff[i]) {
- pci_unmap_single(np->pci_dev,
- np->rx_dma[i],
- np->rx_skbuff[i]->len,
- PCI_DMA_TODEVICE);
- dev_kfree_skb(np->tx_skbuff[i]);
- }
- np->tx_skbuff[i] = NULL;
- }
+ drain_tx(dev);
+}
+
+static void reinit_ring(struct net_device *dev)
+{
+ struct netdev_private *np = dev->priv;
+ int i;
+
+ /* drain TX ring */
+ drain_tx(dev);
+ np->dirty_tx = np->cur_tx = 0;
+ for (i=0;i<TX_RING_SIZE;i++)
+ np->tx_ring[i].cmd_status = 0;
+
+ /* RX Ring */
+ np->dirty_rx = 0;
+ np->cur_rx = RX_RING_SIZE;
+ np->rx_head_desc = &np->rx_ring[0];
+ /* Initialize all Rx descriptors. */
+ for (i = 0; i < RX_RING_SIZE; i++)
+ np->rx_ring[i].cmd_status = cpu_to_le32(DescOwn);
+
+ refill_rx(dev);
}

static void free_ring(struct net_device *dev)
@@ -1508,7 +1583,7 @@
if (intr_status == 0)
break;

- if (intr_status & (IntrRxDone | IntrRxIntr))
+ if (intr_status & (IntrRxDone | IntrRxIntr | RxStatusFIFOOver))
netdev_rx(dev);

if (intr_status & (IntrTxDone | IntrTxIntr | IntrTxIdle | IntrTxErr) ) {
@@ -1610,26 +1685,14 @@
desc_status = le32_to_cpu(np->rx_head_desc->cmd_status);
}

- /* Refill the Rx ring buffers. */
- for (; np->cur_rx - np->dirty_rx > 0; np->dirty_rx++) {
- struct sk_buff *skb;
- entry = np->dirty_rx % RX_RING_SIZE;
- if (np->rx_skbuff[entry] == NULL) {
- skb = dev_alloc_skb(np->rx_buf_sz);
- np->rx_skbuff[entry] = skb;
- if (skb == NULL)
- break; /* Better luck next round. */
- skb->dev = dev; /* Mark as being used by this device. */
- np->rx_dma[entry] = pci_map_single(np->pci_dev,
- skb->data, skb->len, PCI_DMA_FROMDEVICE);
- np->rx_ring[entry].addr = cpu_to_le32(np->rx_dma[entry]);
- }
- np->rx_ring[entry].cmd_status =
- cpu_to_le32(np->rx_buf_sz);
- }
+ refill_rx(dev);

/* Restart Rx engine if stopped. */
- writel(RxOn, dev->base_addr + ChipCmd);
+ if (np->oom)
+ mod_timer(&np->timer, jiffies + 1);
+ else
+ writel(RxOn, dev->base_addr + ChipCmd);
+
}

static void netdev_error(struct net_device *dev, int intr_status)
@@ -2331,7 +2394,8 @@
/* enable the WOL interrupt.
* Could be used to send a netlink message.
*/
- writel(readl(ioaddr + IntrMask) | WOLPkt, ioaddr + IntrMask);
+ writel(WOLPkt, ioaddr + IntrMask);
+ writel(1, ioaddr + IntrEnable);
}
}

@@ -2341,7 +2405,6 @@
struct netdev_private *np = dev->priv;

netif_stop_queue(dev);
- netif_carrier_off(dev);

if (debug > 1) {
printk(KERN_DEBUG "%s: Shutting down ethercard, status was %4.4x.\n",
@@ -2350,13 +2413,17 @@
dev->name, np->cur_tx, np->dirty_tx, np->cur_rx, np->dirty_rx);
}

+ /* Disable interrupts, and flush posted writes */
+ writel(0, ioaddr + IntrEnable);
+ readl(ioaddr + IntrEnable);
+ free_irq(dev->irq, dev);
del_timer_sync(&np->timer);
-
- disable_irq(dev->irq);
+ /* Interrupt disabled, interrupt handler released,
+ * queue stopped, timer deleted. All async codepaths
+ * that access the driver are disabled.
+ */
spin_lock_irq(&np->lock);

- /* Disable and clear interrupts */
- writel(0, ioaddr + IntrEnable);
readl(ioaddr + IntrMask);
readw(ioaddr + MIntrStatus);

@@ -2369,17 +2436,6 @@
__get_stats(dev);
spin_unlock_irq(&np->lock);

- /* race: shared irq and as most nics the DP83815
- * reports _all_ interrupt conditions in IntrStatus, even
- * disabled ones.
- * packet received after disable_irq, but before stop_rxtx
- * --> race. intr_handler would restart the rx process.
- * netif_device_{de,a}tach around {enable,free}_irq.
- */
- netif_device_detach(dev);
- enable_irq(dev->irq);
- free_irq(dev->irq, dev);
- netif_device_attach(dev);
/* clear the carrier last - an interrupt could reenable it otherwise */
netif_carrier_off(dev);

@@ -2387,7 +2443,7 @@
drain_ring(dev);
free_ring(dev);

- {
+ {
u32 wol = readl(ioaddr + WOLCmd) & WakeOptsSummary;
if (wol) {
/* restart the NIC in WOL mode.


Attachments:
patch-natsemi (10.41 kB)

2002-01-12 21:39:06

by Tim Hockin

[permalink] [raw]
Subject: Re: [PATCH] Rx FIFO Overrun error found

> if (dspcfg != DSPCFG_VAL) {
> if (!netif_queue_stopped(dev)) {
> + spin_unlock_irq(&np->lock);
> printk(KERN_INFO
> "%s: possible phy reset: re-initializing\n",
> dev->name);
> disable_irq(dev->irq);
> spin_lock_irq(&np->lock);
> + natsemi_reset(dev);
> + reinit_ring(dev);
> init_registers(dev);
> spin_unlock_irq(&np->lock);
> enable_irq(dev->irq);

I'm not sure you want or need a natsemi_reset here - I'll need to check my
notes on this when I get back to work. Can I ask why this change was made?
This is a very hard case to reproduce, so I'm not very comfortable changing
the codepath :) We've had National looking at this buggy behavior for
months now, with little result.

> /* enable the WOL interrupt.
> * Could be used to send a netlink message.
> */
> - writel(readl(ioaddr + IntrMask) | WOLPkt, ioaddr + IntrMask);
> + writel(WOLPkt, ioaddr + IntrMask);
> + writel(1, ioaddr + IntrEnable);

is this intended to blow away the other bits in IntrMask? Keep in mind
that Wake-On-Phy requires the PHY interrupt enabled, but I don't know if it
needs it on in intrmask or just in the Phy intr reg.

There are a few changes in here I want to double check, but all my
test-setup and notes for natsemi are at work - I may have more comments
next week.

Tim

2002-01-12 21:57:36

by Manfred Spraul

[permalink] [raw]
Subject: Re: [PATCH] Rx FIFO Overrun error found

Tim Hockin wrote:
>
> > if (dspcfg != DSPCFG_VAL) {
> > if (!netif_queue_stopped(dev)) {
> > + spin_unlock_irq(&np->lock);
> > printk(KERN_INFO
> > "%s: possible phy reset: re-initializing\n",
> > dev->name);
> > disable_irq(dev->irq);
> > spin_lock_irq(&np->lock);
> > + natsemi_reset(dev);
> > + reinit_ring(dev);
> > init_registers(dev);
> > spin_unlock_irq(&np->lock);
> > enable_irq(dev->irq);
>
> I'm not sure you want or need a natsemi_reset here - I'll need to check my
> notes on this when I get back to work. Can I ask why this change was made?
> This is a very hard case to reproduce, so I'm not very comfortable changing
> the codepath :) We've had National looking at this buggy behavior for
> months now, with little result.
>

init_registers() reinitialized the rx and tx ring pointers as seen by
the nic. You called it without init_ring(), i.e. the ring pointers as
seen by the driver didn't match the values used by the nic. It probably
only worked by chance. And changing the ring pointers while the nic is
in the middle of a data transfers just asks for trouble.
I agree that natsemi_stop_rxtx() instead of reset would be enough.

> > /* enable the WOL interrupt.
> > * Could be used to send a netlink message.
> > */
> > - writel(readl(ioaddr + IntrMask) | WOLPkt, ioaddr + IntrMask);
> > + writel(WOLPkt, ioaddr + IntrMask);
> > + writel(1, ioaddr + IntrEnable);
>
> is this intended to blow away the other bits in IntrMask? Keep in mind
> that Wake-On-Phy requires the PHY interrupt enabled, but I don't know if it
> needs it on in intrmask or just in the Phy intr reg.
>

Yes, they are always 0. Actually the code is (and was) never executed.
Everyone calls enable_wol_mode(,0).

--
Manfred

2002-01-14 00:55:18

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] Rx FIFO Overrun error found

Manfred Spraul wrote:
>
> Hi all,
>
> I think I found the reason for the Rx status FIFO overrun nic hang:
> If the Rx fifo overflows, the nic sets RxStatusFIFOOver _instead_ of
> IntrRxIntr.
> Thus netdev_rx is never called, the rx ring is never refilled, nic
> hangs.
>
> The attached patch adds proper OOM handling to natsemi.c.
> I've also copied the simpler netdev_close locking from ns83820.c.

Two comments, sis900.c uses the check (rx-overrun | rx-err | rx-ok), did
you test that combination? (s/RxStatusFIFOOver/IntrRxOverrun/)

and it would be preferred to separate "add oom handling" and "fix nic
hang" patches.

--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery

2002-01-14 17:30:12

by Manfred Spraul

[permalink] [raw]
Subject: Re: [PATCH] Rx FIFO Overrun error found

--- 2.5/drivers/net/natsemi.c Fri Nov 23 20:35:23 2001
+++ build-2.5/drivers/net/natsemi.c Mon Jan 14 16:56:22 2002
@@ -1508,7 +1508,7 @@
if (intr_status == 0)
break;

- if (intr_status & (IntrRxDone | IntrRxIntr))
+ if (intr_status & (IntrRxDone | IntrRxIntr | RxStatusFIFOOver | IntrRxErr | IntrRxOverrun ))
netdev_rx(dev);

if (intr_status & (IntrTxDone | IntrTxIntr | IntrTxIdle | IntrTxErr) ) {


Attachments:
patch-natsemi-fifoonly (418.00 B)

2002-01-14 18:54:22

by Torrey Hoffman

[permalink] [raw]
Subject: RE: [PATCH] Rx FIFO Overrun error found


Manfred Spraul wrote, and my ears perked up:
...
> Attached is the patch against the nic hang. Now all rx error bits
> trigger netdev_rx - it doesn't hurt and could catch further hardware
> oddities.

hello natsemi-users ...

We've been having difficult-to-reproduce problems with IP
multicast receive on our natsemi hardware, could this be
related?

Our application receives an IP multicast stream (MPEG-2
video) at about 4 Mb/sec, and sometimes, randomly,
multicast packets just stop showing up at the app layer.
This typically happens after hours of no problems.
(Kernel is 2.4.16 with low-latency patch)

When this happens, tcpdump on the same machine doesn't
see the multicast packets either, but: TCP connections
like FTP still work fine, and other machines on the same
hub still see the multicast traffic, so we are sure the
packets are on the wire.

Our app detects the unexpected loss of the stream and
repeatedly does multicast joins to try to get it back,
but this does not seem to help. However, switching to a
different IP multicast address works - if we change to
a different channel and then back again, everything
will work again... until next time.

I wonder if the multicast hash table is getting corrupted
somehow...

Maybe I should force the multicast hash table to be
rebuilt on any rx error?

Advice welcome, even suggestions on where to put printk
statements...

Torrey

2002-02-08 10:38:56

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] Rx FIFO Overrun error found

patch (finally) applied, to 2.4 and 2.5
--
Jeff Garzik | "I went through my candy like hot oatmeal
Building 1024 | through an internally-buttered weasel."
MandrakeSoft | - goats.com