Re: [PATCH] VIA Rhine stalls: TxAbort handling

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Ivan G.]

On Monday 13 May 2002 09:53 pm, you wrote:
> I don't know how many time-out problems exist in via-rhine.c, but the patch
> below fixes at least one of them. It works for my VT6102 based card. I hope
> some of you can confirm the fix (or let me know what other chips I broke).

My card behaves as unpredictably as always.
Sometimes it works fine for relatively long periods of time (2mins+ :) )
and sometimes it stalls dead on the first 100K.

So....
I've learned not to trust that card regarding anything..
Let's look at the code instead.

> - Recover gracefully from TxAbort (the actual fix)

Some time ago, I asked Becker about this same piece of code in the Linuxfet 
driver pertaining to IntrTxUnderrun. They recover in such a way for both 
Underrun and Abort. And here's what he said:


<QUOTE>
> if (txstatus & 0x0800) {
>              /* uderrun happen */
>               np->tx_ring[entry].tx_status = cpu_to_le32(DescOwn);
>               writel(virt_to_bus(&np->tx_ring[entry]), ioaddr + TxRingPtr);


This is backing up the Tx pointer to retransmit the failed packet.
My original driver did not do this -- I believe that it's semantically
wrong.

I write my drivers to always fail the current transmit attempt on Tx
Abort.  Ethernet uses dropped packets on severe overload (16 collision
abort) to provide flow control to the protocol layers.  If you back up
and attempt to retransmit the same packet you defeat this element of the
protocol.  It's an atypical case, but Ethernet works where Aloha fails
because it gets the rare cases right.
</QUOTE>

I don't know if he's right or wrong. 
I have tried the code. I got different results with different combinations of 
recovery from Underrun and Abort. I think I was able to get some improvement 
on the stall issue with one of them, but then speed went down, as you 
acknowledge. A friend also verified that some time ago:

<QUOTE>
with patch:
transmit and receive speeds are slow but steady, no stalling

without patch:
transmit speeds are fast but have occasional stalls... overall, its probably 
the same speed as with the patch
</QUOTE>

So, given what Becker said and the completely random behavior of my card,
I decided to stay away from the abort handling. You can figure out if it's 
the proper thing to do.


Speaking of VIA Rhine Cards....

THIS:
      if (chip_id == VT86C100A) {
                /* More recent docs say that this bit is reserved ... */
                n = inb(ioaddr + ConfigA) | 0x20;
                outb(n, ioaddr + ConfigA);

is not right, as the comment also explains...
So what should be done here instead??

Ivan G, tested with a Rhine-I card (VT86C100A)

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Ivan G.]

One other thing I forgot to mention....
interrupts logged by your patch:
0008 (abort) (LOTS)
0009 (abort, rxdone)
001a (underrun, abort, txdone)

[PATCH] VIA Rhine stalls: TxAbort handling

[Roger Luethi]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

I don't know how many time-out problems exist in via-rhine.c, but the patch
below fixes at least one of them. It works for my VT6102 based card. I hope
some of you can confirm the fix (or let me know what other chips I broke).

Here's the error message the patch addresses:
Transmit timed out, status 0000, PHY status 782d, resetting...

The patch is slightly experimental and contains some changes and clean-ups
that are not directly related to the time out problem.

Patch description:
- Recover gracefully from TxAbort (the actual fix)
- Explicitly pick a backoff algorithm (alternative "fix")
- Remove full_duplex, duplex_lock, and advertising from netdev_private
- Make use of MII register names somewhat more consistent
- Update comment regarding config information at 0x78
- Move comment on *_desc_status where it belongs
- More comment details

Some information on stalling:

The time outs happened because the driver's handling of TxAbort was
incomplete: The transfer didn't get restarted. My initial fix was to
prevent TxAbort -- clearing bit 1 of ConfigD did the trick for me. The
stalling effect was gone.

The simple reason: The AMD backoff algorithm always triggered TxAborts,
the others didn't.

However, once I had the driver recover from TxAbort without waiting for the
time out reset, the AMD solution provided over 20% higher throughput than
the DEC algorithm. YMMV, depending on the specific setup. I'd vote for a
module parameter. For now, I hardcoded AMD: it's what the eeprom picks when
reloaded. Also, every other algorithm masked the TxAbort problem (by not
triggering any).

Incidentally, the VIA drivers clear bits 0-3 and set bit 3 of TxConfig for
all chips (the latter supposedly deals with backoff algorithms, too, but
it didn't seem to make a measurable difference on my system).

Patch is against 2.4.19-pre8. Feedback welcome.

Roger

[-- Attachment #2: via-rhine.c.patch --]
[-- Type: text/plain, Size: 4503 bytes --]

--- via-rhine.c.org	Sun May 12 21:53:41 2002
+++ via-rhine.c	Tue May 14 05:33:06 2002
@@ -224,7 +224,8 @@
 Boards with this chip are functional only in a bus-master PCI slot.
 
 Many operational settings are loaded from the EEPROM to the Config word at
-offset 0x78.  This driver assumes that they are correct.
+offset 0x78. For most of these settings, this driver assumes that they are
+correct.
 If this driver is compiled to use PCI memory space operations the EEPROM
 must be configured to enable memory ops.
 
@@ -376,6 +377,11 @@
 	StickyHW=0x83, WOLcrClr=0xA4, WOLcgClr=0xA7, PwrcsrClr=0xAC,
 };
 
+/* Bits in ConfigD (select backoff algorithm (Ethernet capture effect)) */
+enum backoff_bits {
+	BackOpt=0x01, BackAMD=0x02, BackDEC=0x04, BackRandom=0x08
+};
+
 #ifdef USE_MEM
 /* Registers we check that mmio and reg are the same. */
 int mmio_verify_registers[] = {
@@ -424,11 +430,11 @@
 	u32 next_desc;
 };
 
-/* Bits in *_desc.status */
 enum rx_status_bits {
 	RxOK=0x8000, RxWholePkt=0x0300, RxErr=0x008F
 };
 
+/* Bits in *_desc.status */
 enum desc_status_bits {
 	DescOwn=0x80000000, DescEndPacket=0x4000, DescIntr=0x1000,
 };
@@ -476,13 +482,10 @@
 	u16 chip_cmd;						/* Current setting for ChipCmd */
 
 	/* These values are keep track of the transceiver/media in use. */
-	unsigned int full_duplex:1;			/* Full-duplex operation requested. */
-	unsigned int duplex_lock:1;
 	unsigned int default_port:4;		/* Last dev->if_port value. */
 	u8 tx_thresh, rx_thresh;
 
 	/* MII transceiver section. */
-	u16 advertising;					/* NWay media advertisement */
 	unsigned char phys[MAX_MII_CNT];			/* MII device addresses. */
 	unsigned int mii_cnt;			/* number of MIIs found, but only the first one is used */
 	u16 mii_status;						/* last read MII status */
@@ -693,6 +696,9 @@
 		writeb(readb(ioaddr + ConfigA) & 0xFE, ioaddr + ConfigA);
 	}
 
+	/* Select backoff algorithm */
+	writeb(readb(ioaddr + ConfigD) & (0xF0 | BackAMD), ioaddr + ConfigD);
+
 	dev->irq = pdev->irq;
 
 	np = dev->priv;
@@ -784,7 +790,7 @@
 				   (option & 0x300 ? 100 : 10),
 				   (option & 0x220 ? "full" : "half"));
 			if (np->mii_cnt)
-				mdio_write(dev, np->phys[0], 0,
+				mdio_write(dev, np->phys[0], MII_BMCR,
 						   ((option & 0x300) ? 0x2000 : 0) |  /* 100mbps? */
 						   ((option & 0x220) ? 0x0100 : 0));  /* Full duplex? */
 		}
@@ -968,9 +974,9 @@
 		writeb(dev->dev_addr[i], ioaddr + StationAddr + i);
 
 	/* Initialize other registers. */
-	writew(0x0006, ioaddr + PCIBusConfig);	/* Tune configuration??? */
-	/* Configure the FIFO thresholds. */
-	writeb(0x20, ioaddr + TxConfig);	/* Initial threshold 32 bytes */
+	writew(0x0006, ioaddr + PCIBusConfig);	/* Store & forward */
+	/* Configure initial FIFO thresholds. */
+	writeb(0x20, ioaddr + TxConfig);
 	np->tx_thresh = 0x20;
 	np->rx_thresh = 0x60;			/* Written in via_rhine_set_rx_mode(). */
 
@@ -1029,13 +1035,13 @@
 
 	if (phy_id == np->phys[0]) {
 		switch (regnum) {
-		case 0:							/* Is user forcing speed/duplex? */
+		case MII_BMCR:					/* Is user forcing speed/duplex? */
 			if (value & 0x9000)			/* Autonegotiation. */
 				np->mii_if.duplex_lock = 0;
 			else
 				np->mii_if.full_duplex = (value & 0x0100) ? 1 : 0;
 			break;
-		case 4:
+		case MII_ADVERTISE:
 			np->mii_if.advertising = value;
 			break;
 		}
@@ -1325,7 +1331,12 @@
 			np->stats.tx_errors++;
 			if (txstatus & 0x0400) np->stats.tx_carrier_errors++;
 			if (txstatus & 0x0200) np->stats.tx_window_errors++;
-			if (txstatus & 0x0100) np->stats.tx_aborted_errors++;
+			if (txstatus & 0x0100) {
+				np->stats.tx_aborted_errors++;
+				np->tx_ring[entry].tx_status = cpu_to_le32(DescOwn);
+				writel(virt_to_bus(&np->tx_ring[entry]), dev->base_addr + TxRingPtr);
+				break; /* Keep the skb for retry */
+			}
 			if (txstatus & 0x0080) np->stats.tx_heartbeat_errors++;
 			if (txstatus & 0x0002) np->stats.tx_fifo_errors++;
 			/* Transmitter restarted in 'abnormal' handler. */
@@ -1492,8 +1503,11 @@
 		clear_tally_counters(ioaddr);
 	}
 	if (intr_status & IntrTxAbort) {
-		/* Stats counted in Tx-done handler, just restart Tx. */
-		writew(CmdTxDemand | np->chip_cmd, dev->base_addr + ChipCmd);
+		/* Prepared in via_rhine_tx(), now kick-start Tx */
+		writew(CmdTxOn | np->chip_cmd, dev->base_addr + ChipCmd);
+		writeb(CmdTxDemand | readb(dev->base_addr + ChipCmd) , dev->base_addr + ChipCmd);
+		printk(KERN_ERR "%s: ABORT %4.4x.\n",
+			   dev->name, intr_status);
 	}
 	if (intr_status & IntrTxUnderrun) {
 		if (np->tx_thresh < 0xE0)

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Jeff Garzik]

I'm going to apply the non-controversial parts of this patch...

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Roger Luethi]

On Tue, 14 May 2002 10:11:37 -0400, Jeff Garzik wrote:
> I'm going to apply the non-controversial parts of this patch...

Would this be a good place to find out what has made it in?

http://gkernel.bkbits.net:8080/net-drivers-2.5/hist/drivers/net/via-rhine.c

Roger

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Urban Widmark]

On Mon, 13 May 2002, Ivan G. wrote:

> Speaking of VIA Rhine Cards....
> 
> THIS:
>       if (chip_id == VT86C100A) {
>                 /* More recent docs say that this bit is reserved ... */
>                 n = inb(ioaddr + ConfigA) | 0x20;
>                 outb(n, ioaddr + ConfigA);
> 
> is not right, as the comment also explains...
> So what should be done here instead??

What makes you think there are other options?

The older VT86C100A docs say the bit controls if memory mapped io is
enabled, the newer says it is reserved. That doesn't mean that the bit
doesn't control what it used to, only that it is no longer documented.
(seems unlikely that they actually changed the hardware)

Why remove it from the docs? Perhaps the chip has bugs with mmio.

It works (for me at least), but it is experimental esp. on the VT86C100A.
When you have seen stalls did you run with or without mmio?

/Urban

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Urban Widmark]

On Tue, 14 May 2002, Roger Luethi wrote:

> The simple reason: The AMD backoff algorithm always triggered TxAborts,
> the others didn't.
> 
> However, once I had the driver recover from TxAbort without waiting for the
> time out reset, the AMD solution provided over 20% higher throughput than
> the DEC algorithm. YMMV, depending on the specific setup. I'd vote for a
> module parameter. For now, I hardcoded AMD: it's what the eeprom picks when
> reloaded. Also, every other algorithm masked the TxAbort problem (by not
> triggering any).

The backoff algorithm bits have different names (and possibly different
meaning) for the vt86c100a. My vt86c100a eeprom sets all backoff bits to
0000, but my vt6102 sets it to 0010. Since the eeprom is reloaded when the
driver opens, why force it to "amd"?

A module parameter would be nice for testing.

Ivan, have you tried playing with these bits?

Donalds suggestion is that the TxAborts is simply too much collisions.
Perhaps the eeprom selection of backoff algorithm isn't working well in
your environment.

/Urban

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Ivan G.]

> I'll take that down as "The patch didn't break anything" <g>. Thanks.

:) Some nice day this card will work.
I know it. It's too bad I don't have time to mess with it right now.

> What I have seen: switching from eeprom default (AMD/MBA backoff on my
> card) to something else (as VIA does) slows things down, but the TxAborts
> are gone. Did you try different backoff algorithms?

The slowdown I was talking about was actually with the new abort/underrun 
handling - I had tried it by myself before your patch. That's the what that 
quote was about. I think I handled both Abort and Underrun like that.
I'll try that new patch that you're making to retest.

> Also, you may want to try if the backoff bit in TxConfig makes a difference
> for you (may be different with your chip, after all). It's not a one-liner
> like ConfigD, though, since TxConfig gets overwritten in several places.
> On a side note, I'm not particularly happy about the way we stomp all over
> TxConfig when we set the threshold. IMO we should leave the lower 5 bits
> alone.

No, I haven't messed with those bits, to answer Urban and your question.
I've only tried your patch which forces the backoff algortihm to AMD.
Tests sound like a good idea. I'll try something out when I have time - been 
busy lately.

> The only data sheet I've seen for the VT86C100A agrees with the code, not
> the comment, so apparantly I don't have access to those more recent docs.
> This code is only used if you enable MMIO, though, which may not be a good
> idea if you already have problems with the driver.

On Urban's question,  I test without MMIO so this is not a related issue. I 
was merely curious since I don't feel comfortable trusting something which
A) does not match any of the other Rhine-based cards (2's and 3's)
B) says RESERVED in the docs which I have.

Funny, I was going to send you a link to the newer docs, but I ran into the 
older ones which I had never seen before. Yes, they do agree with the current 
code. Hmm. Perhaps we should ask VIA why it was changed...

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Roger Luethi]

On Mon, 13 May 2002 17:51:22 -0600, Ivan G. wrote:
> My card behaves as unpredictably as always.

I'll take that down as "The patch didn't break anything" <g>. Thanks.

> Sometimes it works fine for relatively long periods of time (2mins+ :) )
> and sometimes it stalls dead on the first 100K.

You have a different chip. I don't have a VT86C100A to play with. Too bad.

> > - Recover gracefully from TxAbort (the actual fix)
> 
> Some time ago, I asked Becker about this same piece of code in the Linuxfet 
> driver pertaining to IntrTxUnderrun. They recover in such a way for both 
> Underrun and Abort. And here's what he said:

Yeah, I noticed VIA does that for underrun, too. I have never seen one,
though. So for everything I know the driver might work for underruns as it
is. Note that for TxAbort, the LK driver already tries to restart Tx,
whereas for TxUnderrun, it just increases the threshold. What I am trying
to improve is that the restart after TxAbort works.

> <QUOTE>
> This is backing up the Tx pointer to retransmit the failed packet.
> My original driver did not do this -- I believe that it's semantically
> wrong.
> </QUOTE>

I tend to agree with Donald Becker here. I am working on an updated patch
which drops the aborted frame.

> recovery from Underrun and Abort. I think I was able to get some improvement 
> on the stall issue with one of them, but then speed went down, as you 
> acknowledge.

What I have seen: switching from eeprom default (AMD/MBA backoff on my
card) to something else (as VIA does) slows things down, but the TxAborts
are gone. Did you try different backoff algorithms?

Also, you may want to try if the backoff bit in TxConfig makes a difference
for you (may be different with your chip, after all). It's not a one-liner
like ConfigD, though, since TxConfig gets overwritten in several places.
On a side note, I'm not particularly happy about the way we stomp all over
TxConfig when we set the threshold. IMO we should leave the lower 5 bits
alone.

> A friend also verified that some time ago:
>
> <QUOTE>
> with patch:
> transmit and receive speeds are slow but steady, no stalling
> 
> without patch:
> transmit speeds are fast but have occasional stalls... overall, its probably 
> the same speed as with the patch
> </QUOTE>

That quote would be immensely more helpful if we knew what patch exactly
he's talking about <g>.

> Speaking of VIA Rhine Cards....
> 
> THIS:
>       if (chip_id == VT86C100A) {
>                 /* More recent docs say that this bit is reserved ... */
>                 n = inb(ioaddr + ConfigA) | 0x20;
>                 outb(n, ioaddr + ConfigA);
> 
> is not right, as the comment also explains...
> So what should be done here instead??

The only data sheet I've seen for the VT86C100A agrees with the code, not
the comment, so apparantly I don't have access to those more recent docs.
This code is only used if you enable MMIO, though, which may not be a good
idea if you already have problems with the driver.

> One other thing I forgot to mention....
> interrupts logged by your patch:
> 0008 (abort) (LOTS)
> 0009 (abort, rxdone)
> 001a (underrun, abort, txdone)

That's all perfectly normal. Except for the underrun, that I have never
seen myself.

If your chip behaves anything like mine, you are _supposed_ to see lots of
aborts with BackAMD selected. I intentionally picked the backoff algorithm
which produces TxAborts (it's the eeprom default anyway, though). If you
change BackAMD to say BackRandom and there are still as many aborts as
before, we have found another difference between our systems.

Roger

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Roger Luethi]

On Tue, 14 May 2002 19:47:02 +0200, Urban Widmark wrote:
> The backoff algorithm bits have different names (and possibly different
> meaning) for the vt86c100a.

Not according to my data sheet. You may have been confused by the names I
picked: BackAMD should really be MBA. My upcoming patch will change the
names, i.e. AMD becomes MBA.

> My vt86c100a eeprom sets all backoff bits to 0000, but my vt6102 sets it
> to 0010. Since the eeprom is reloaded when the driver opens, why force it
> to "amd"?

You just answered your question. I did it because on my system that is the
way to trigger aborts and I suspected some cards might come up with a
different default value. VIA is clearing that bit in their driver so I
wouldn't be too surprised if even some newer cards did it, too.

> A module parameter would be nice for testing.

For testing the current solution should do. A parameter would make sense if
we come to the conclusion that it would be beneficial for regular users to
make a selection. It just might be. Maybe somebody who is more opinionated
that me would like to step forward and make that call!?

> Ivan, have you tried playing with these bits?

Have _you_?

> Donalds suggestion is that the TxAborts is simply too much collisions.
> Perhaps the eeprom selection of backoff algorithm isn't working well in
> your environment.

No, it works just great. The problem is that the unmodified driver breaks
down as soon as a TxAbort happens. From my limited experience, MBA seems to
be rather aggressive which is a good thing in some situations (e.g. if
nobody cares that you're monopolizing the network).

Roger

Re: [PATCH] VIA Rhine stalls: TxAbort handling

[Roger Luethi]

On Wed, 15 May 2002 15:52:35 -0600, Ivan G. wrote:
> The slowdown I was talking about was actually with the new abort/underrun 
> handling - I had tried it by myself before your patch. That's the what that 
> quote was about. I think I handled both Abort and Underrun like that.
> I'll try that new patch that you're making to retest.

How a different abort handling could cause a slow down is beyond me,
especially if you used the old code where the aborted frame got
reactivated. With the vanilla code, you were bound to stall on the first tx
error, which should certainly decrease troughput by a fair bit. And unless
a tx error occurs, both versions (of the code) behave identically. I'm
afraid I don't understand what's going on with the VT86C100A.

> On Urban's question,  I test without MMIO so this is not a related issue. I 
> was merely curious since I don't feel comfortable trusting something which
> A) does not match any of the other Rhine-based cards (2's and 3's)
> B) says RESERVED in the docs which I have.
> 
> Funny, I was going to send you a link to the newer docs, but I ran into the 
> older ones which I had never seen before. Yes, they do agree with the current 
> code. Hmm. Perhaps we should ask VIA why it was changed...

I'd take the docs with a grain of salt, especially the VT86C100A data sheet
(the one I have, anyway) contains so blatant mistakes it's downright
confusing.

Roger