tulip bug, alternative fix, #3 (tested)
Wolfgang Walter
wolfgang.walter@stusta.mhn.de
Sun Dec 12 17:31:40 1999
On Sun, Dec 12, 1999 at 09:44:14PM +0100, Stephen R. van den Berg wrote:
> Wolfgang Walter wrote:
> >I sent description of the bugs to this list and to Donald Becker. As I haven't
> >got an answer from him, yet, I don't know if he is already working on a fix or
> >what he thinks about this patch. Maybe he's already working on a better fix.
>
> Well, I looked at the patch. Haven't yet doublechecked the exact
> IO-commands, but, a casual comparison of what you did and past discussions
> on the list reveals that you simply disable the "Too much work during
> interrupt" compensations. Donald remarked that this is not a good idea.
Yes, too. I also changed the way the driver tries to detect too-much-work.
The new check does not check interrupts, but the number of packets sent and
received (and it avoids to mix them).
The to-much-work-handling had serveral problems. Especially it acknowledged
interrupts and disabled the last one occured. This was the reason for
permanent tx-hangs (probably tx-interrupts could be acknowlegded without
beeing handled).
I think the too-much-work should work rather acceptable now:
1. if to much tx-work has to be done in the interrupt-handler the tx-ring
you have less cpu-time to send
2. if you get to much packets sent rx-ring gets full and receiving is
suspended.
If this is not enough for receiving we may leave the card longer in
rx-suspend-state: we do not handle received-packets for a certain
amount of time so the rx-ring remains full and the card in rx-suspend
mode.
> I'm not sure if he's right (didn't have time to check the logic yet), but
> simply skipping it seems almost a bit too easy.
>
> Comparing his patches for v0.91u with the ones you supplied, I come up
> with a merged version (still suitable for standalone operation with
> 2.2.x) that takes the INTR_MITIGATION support into account (I'm not even
> sure if that can make a difference in this lockup case).
>
> Oh yes, incidentally, your patches seem relative to a different 0.91g version
Its the one of 2.2.14pre4.
> than the one I have (and picked up from Donald's website). Tiny differences,
> but enough to screw up the diffs/patch.
>
> If people could review this merged version (even test it perhaps?), and
> tell me if I made a mess out of it or not? This patch is relative to
> the v0.91g which I got from Donald's website.
>
> --- tulip.c.91g Sat Dec 11 19:37:43 1999
> +++ ../tulip.c Sun Dec 12 06:27:12 1999
> @@ -18,7 +18,7 @@
> */
>
> #define SMP_CHECK
> -static const char version[] = "tulip.c:v0.91g 7/16/99 becker@cesdis.gsfc.nasa.gov\n";
> +static const char version[] = "tulip.c:v0.91gB 7/16/99 becker@cesdis.gsfc.nasa.gov\n";
>
> /* A few user-configurable values. */
>
> @@ -51,6 +51,7 @@
> bonding and packet priority.
> There are no ill effects from too-large receive rings. */
> #define TX_RING_SIZE 16
> +#define TX_QUEUE_LEN 10
> #define RX_RING_SIZE 32
>
> /* Set the copy breakpoint for the copy-only-tiny-buffer Rx structure. */
> @@ -361,7 +362,7 @@
> HAS_MII=1, HAS_MEDIA_TABLE=2, CSR12_IN_SROM=4, ALWAYS_CHECK_MII=8,
> HAS_PWRDWN=0x10, MC_HASH_ONLY=0x20, /* Hash-only multicast filter. */
> HAS_PNICNWAY=0x80, HAS_NWAY143=0x40, /* Uses internal NWay xcvr. */
> - HAS_8023X=0x100,
> + HAS_INTR_MITIGATION=0x100, HAS_8023X=0x400,
> };
> static struct tulip_chip_table {
> char *chip_name;
> @@ -375,7 +376,8 @@
> { "Digital DS21140 Tulip", 128, 0x0001ebef,
> HAS_MII | HAS_MEDIA_TABLE | CSR12_IN_SROM, tulip_timer },
> { "Digital DS21143 Tulip", 128, 0x0801fbff,
> - HAS_MII | HAS_MEDIA_TABLE | ALWAYS_CHECK_MII | HAS_PWRDWN | HAS_NWAY143,
> + HAS_MII | HAS_MEDIA_TABLE | ALWAYS_CHECK_MII | HAS_PWRDWN | HAS_NWAY143
> + | HAS_INTR_MITIGATION,
> t21142_timer },
> { "Lite-On 82c168 PNIC", 256, 0x0001ebef,
> HAS_MII | HAS_PNICNWAY, pnic_timer },
> @@ -433,7 +435,7 @@
>
> /* The bits in the CSR5 status registers, mostly interrupt sources. */
> enum status_bits {
> - TimerInt=0x800, TPLnkFail=0x1000, TPLnkPass=0x10,
> + TimerInt=0x800, SytemError=0x2000, TPLnkFail=0x1000, TPLnkPass=0x10,
> NormalIntr=0x10000, AbnormalIntr=0x8000,
> RxJabber=0x200, RxDied=0x100, RxNoBuf=0x80, RxIntr=0x40,
> TxFIFOUnderflow=0x20, TxJabber=0x08, TxNoBuf=0x04, TxDied=0x02, TxIntr=0x01,
> @@ -530,6 +532,9 @@
> int cur_index; /* Current media index. */
> int saved_if_port;
> unsigned char pci_bus, pci_devfn;
> + int ttimer;
> + int susp_rx;
> + unsigned long nir;
> int pad0, pad1; /* Used for 8-byte alignment */
> };
>
> @@ -548,6 +553,7 @@
> static void tulip_tx_timeout(struct device *dev);
> static void tulip_init_ring(struct device *dev);
> static int tulip_start_xmit(struct sk_buff *skb, struct device *dev);
> +static int tulip_refill_rx(struct device *dev);
> static int tulip_rx(struct device *dev);
> static void tulip_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
> static int tulip_close(struct device *dev);
> @@ -2484,6 +2490,9 @@
> tp->tx_full = 0;
> tp->cur_rx = tp->cur_tx = 0;
> tp->dirty_rx = tp->dirty_tx = 0;
> + tp->susp_rx = 0;
> + tp->ttimer = 0;
> + tp->nir = 0;
>
> for (i = 0; i < RX_RING_SIZE; i++) {
> tp->rx_ring[i].status = 0x00000000;
> @@ -2545,11 +2554,11 @@
> tp->tx_skbuff[entry] = skb;
> tp->tx_ring[entry].buffer1 = virt_to_bus(skb->data);
>
> - if (tp->cur_tx - tp->dirty_tx < TX_RING_SIZE/2) {/* Typical path */
> + if (tp->cur_tx - tp->dirty_tx < TX_QUEUE_LEN/2) {/* Typical path */
> flag = 0x60000000; /* No interrupt */
> - } else if (tp->cur_tx - tp->dirty_tx == TX_RING_SIZE/2) {
> + } else if (tp->cur_tx - tp->dirty_tx == TX_QUEUE_LEN/2) {
> flag = 0xe0000000; /* Tx-done intr. */
> - } else if (tp->cur_tx - tp->dirty_tx < TX_RING_SIZE - 2) {
> + } else if (tp->cur_tx - tp->dirty_tx < TX_QUEUE_LEN - 2) {
> flag = 0x60000000; /* No Tx-done intr. */
> } else { /* Leave room for set_rx_mode() to fill entries. */
> tp->tx_full = 1;
> @@ -2578,7 +2587,15 @@
> struct device *dev = (struct device *)dev_instance;
> struct tulip_private *tp = (struct tulip_private *)dev->priv;
> long ioaddr = dev->base_addr;
> - int csr5, work_budget = max_interrupt_work;
> + int csr5;
> + int entry;
> + int missed;
> + int rx = 0;
> + int tx = 0;
> + int oi = 0;
> + int maxrx = RX_RING_SIZE;
> + int maxtx = TX_RING_SIZE;
> + int maxoi = TX_RING_SIZE;
>
> #if defined(__i386__) && defined(SMP_CHECK)
> if (test_and_set_bit(0, (void*)&dev->interrupt)) {
> @@ -2596,6 +2613,8 @@
> dev->interrupt = 1;
> #endif
>
> + tp->nir++;
> +
> do {
> csr5 = inl(ioaddr + CSR5);
> /* Acknowledge all of the current interrupt sources ASAP. */
> @@ -2608,10 +2627,12 @@
> if ((csr5 & (NormalIntr|AbnormalIntr)) == 0)
> break;
>
> - if (csr5 & (RxIntr | RxNoBuf))
> - work_budget -= tulip_rx(dev);
> + if (csr5 & (RxIntr | RxNoBuf)) {
> + rx += tulip_rx(dev);
> + tulip_refill_rx(dev);
> + }
>
> - if (csr5 & (TxNoBuf | TxDied | TxIntr)) {
> + if (csr5 & (TxNoBuf | TxDied | TxIntr | TimerInt)) {
> unsigned int dirty_tx;
>
> for (dirty_tx = tp->dirty_tx; tp->cur_tx - dirty_tx > 0;
> @@ -2656,6 +2677,7 @@
> /* Free the original skb. */
> dev_free_skb(tp->tx_skbuff[entry]);
> tp->tx_skbuff[entry] = 0;
> + tx++;
> }
>
> #ifndef final_version
> @@ -2667,7 +2689,7 @@
> #endif
>
> if (tp->tx_full && dev->tbusy
> - && tp->cur_tx - dirty_tx < TX_RING_SIZE - 2) {
> + && tp->cur_tx - dirty_tx < TX_QUEUE_LEN - 4) {
> /* The ring is no longer full, clear tbusy. */
> tp->tx_full = 0;
> dev->tbusy = 0;
> @@ -2698,38 +2720,77 @@
> /* Restart the transmit process. */
> outl(tp->csr6 | 0x0002, ioaddr + CSR6);
> outl(tp->csr6 | 0x2002, ioaddr + CSR6);
> + outl(0, ioaddr + CSR1);
> }
> if (csr5 & RxDied) { /* Missed a Rx frame. */
> tp->stats.rx_errors++;
> tp->stats.rx_missed_errors += inl(ioaddr + CSR8) & 0xffff;
> outl(tp->csr6 | 0x2002, ioaddr + CSR6);
> }
> - if (csr5 & TimerInt) {
> - if (tulip_debug > 2)
> - printk(KERN_ERR "%s: Re-enabling interrupts, %8.8x.\n",
> - dev->name, csr5);
> - outl(tulip_tbl[tp->chip_id].valid_intrs, ioaddr + CSR7);
> - }
> if (csr5 & (TPLnkPass | TPLnkFail | 0x08000000)) {
> if (tp->link_change)
> (tp->link_change)(dev, csr5);
> }
> + if (csr5 & SytemError) {
> + printk(KERN_ERR "%s: (%lu) System Error occured\n", dev->name, tp->nir);
> + }
> /* Clear all error sources, included undocumented ones! */
> outl(0x0800f7ba, ioaddr + CSR5);
> + oi++;
> }
> - if (--work_budget < 0) {
> + if (csr5 & TimerInt) {
> +#if 0
> + if (tulip_debug > 2)
> + printk(KERN_ERR "%s: Re-enabling interrupts, %8.8x.\n",
> + dev->name, csr5);
> + outl(tulip_tbl[tp->chip_id].valid_intrs, ioaddr + CSR7);
> +#endif
> + tp->ttimer = 0;
> + oi++;
> + }
> + if (tx > maxtx || rx > maxrx || oi > maxoi) {
> +#if 0
> if (tulip_debug > 1)
> +#endif
> printk(KERN_WARNING "%s: Too much work during an interrupt, "
> - "csr5=0x%8.8x.\n", dev->name, csr5);
> + "csr5=0x%8.8x. (%lu) (%d,%d,%d)\n", dev->name, csr5, tp->nir, tx, rx, oi);
> /* Acknowledge all interrupt sources. */
> +#if 1
> outl(0x8001ffff, ioaddr + CSR5);
> - /* Clear all interrupting sources, set timer to re-enable. */
> - outl(((~csr5) & 0x0001ebef) | 0x0800, ioaddr + CSR7);
> - outl(12, ioaddr + CSR11);
> + if (tp->flags & HAS_INTR_MITIGATION)
> + outl(0x45240000, ioaddr + CSR11);
> + else {
> + /* Mask all interrupting sources, set timer to e-enable. */
> + outl(((~csr5) & 0x0001ebef) | AbnormalIntr | TimerInt,
> + ioaddr + CSR7);
> + outl(0x0012, ioaddr + CSR11);
> + }
> + tp->ttimer = 1;
> +#endif
This will not work. You must remove the else-tree.
1. You switch of interrupts and never switch them on again:
2. Switching of interrupts was the problem because some people observed.
HAS_INTR_MITIGATION is probably a good method.
> break;
> }
> } while (1);
>
> + tulip_refill_rx(dev);
> +
> + /* check if we card is in suspend mode */
> + entry = tp->dirty_rx % RX_RING_SIZE;
> + if (tp->rx_skbuff[entry] == NULL) {
> + printk(KERN_WARNING "%s: in rx suspend mode: (%lu) (tp->cur_rx = %u, ttimer = %d, rx = %d) go/stay in suspend mode\n", dev->name, tp->nir, tp->cur_rx, tp->ttimer, rx);
> + if (tp->ttimer == 0 || (inl(ioaddr + CSR11) & 0xffff) == 0) {
> + printk(KERN_WARNING "%s: in rx suspend mode: (%lu) set timer\n", dev->name, tp->nir);
> + outl(tulip_tbl[tp->chip_id].valid_intrs | TimerInt,
> + ioaddr + CSR7);
> + outl(TimerInt, ioaddr + CSR5);
> + outl(12, ioaddr + CSR11);
> + tp->ttimer = 1;
> + }
> + }
> +
> + if ((missed = inl(ioaddr + CSR8) & 0x1ffff)) {
> + tp->stats.rx_dropped += missed & 0x10000 ? 0x10000 : missed;
> + }
> +
> if (tulip_debug > 3)
> printk(KERN_DEBUG "%s: exiting interrupt, csr5=%#4.4x.\n",
> dev->name, inl(ioaddr + CSR5));
> @@ -2742,13 +2803,36 @@
> return;
> }
>
> +static int tulip_refill_rx(struct device *dev)
> +{
> + struct tulip_private *tp = (struct tulip_private *)dev->priv;
> + int entry;
> + int refilled = 0;
> +
> + /* Refill the Rx ring buffers. */
> + for (; tp->cur_rx - tp->dirty_rx > 0; tp->dirty_rx++) {
> + entry = tp->dirty_rx % RX_RING_SIZE;
> + if (tp->rx_skbuff[entry] == NULL) {
> + struct sk_buff *skb;
> + skb = tp->rx_skbuff[entry] = dev_alloc_skb(PKT_BUF_SZ);
> + if (skb == NULL)
> + break;
> + skb->dev = dev; /* Mark as being used by this device. */
> + tp->rx_ring[entry].buffer1 = virt_to_bus(skb->tail);
> + refilled++;
> + }
> + tp->rx_ring[entry].status = cpu_to_le32(DescOwned);
> + }
> + return refilled;
> +}
> +
> static int
> tulip_rx(struct device *dev)
> {
> struct tulip_private *tp = (struct tulip_private *)dev->priv;
> int entry = tp->cur_rx % RX_RING_SIZE;
> int rx_work_limit = tp->dirty_rx + RX_RING_SIZE - tp->cur_rx;
> - int work_done = 0;
> + int received = 0;
>
> if (tulip_debug > 4)
> printk(KERN_DEBUG " In tulip_rx(), entry %d %8.8x.\n", entry,
> @@ -2810,7 +2894,6 @@
> memcpy(skb_put(skb, pkt_len),
> bus_to_virt(tp->rx_ring[entry].buffer1), pkt_len);
> #endif
> - work_done++;
> } else { /* Pass up the skb already on the Rx ring. */
> char *temp = skb_put(skb = tp->rx_skbuff[entry], pkt_len);
> tp->rx_skbuff[entry] = NULL;
> @@ -2830,25 +2913,11 @@
> tp->stats.rx_bytes += pkt_len;
> #endif
> }
> + received++;
> entry = (++tp->cur_rx) % RX_RING_SIZE;
> }
>
> - /* Refill the Rx ring buffers. */
> - for (; tp->cur_rx - tp->dirty_rx > 0; tp->dirty_rx++) {
> - entry = tp->dirty_rx % RX_RING_SIZE;
> - if (tp->rx_skbuff[entry] == NULL) {
> - struct sk_buff *skb;
> - skb = tp->rx_skbuff[entry] = dev_alloc_skb(PKT_BUF_SZ);
> - if (skb == NULL)
> - break;
> - skb->dev = dev; /* Mark as being used by this device. */
> - tp->rx_ring[entry].buffer1 = virt_to_bus(skb->tail);
> - work_done++;
> - }
> - tp->rx_ring[entry].status = DescOwned;
> - }
> -
> - return work_done;
> + return received;
> }
>
> static int
> --
> Sincerely, srb@cuci.nl
> Stephen R. van den Berg (AKA BuGless).
>
> Climate is what you expect. Weather is what you get.
Greetings,
Wolfgang Walter
--
Veni, Vidi, VISA:
I came, I saw, I did a little shopping.