Urban,

About the suggestion to make via_rhine_error handle more interrupts,

enum intr_status_bits {
        IntrRxDone=0x0001, IntrRxErr=0x0004, IntrRxEmpty=0x0020,
        IntrTxDone=0x0002, IntrTxAbort=0x0008, IntrTxUnderrun=0x0010,
        IntrPCIErr=0x0040,
        IntrStatsMax=0x0080, IntrRxEarly=0x0100, IntrMIIChange=0x0200,
        IntrRxOverflow=0x0400, IntrRxDropped=0x0800, IntrRxNoBuf=0x1000,
        IntrTxAborted=0x2000, IntrLinkChange=0x4000,
        IntrRxWakeUp=0x8000,
        IntrNormalSummary=0x0003, IntrAbnormalSummary=0xC260,
};

RxEarly, RxOverflow, RxNoBuf are not handled
(which brings up another question - how should they be handled and where?? It doesn't seem to me that those should end up in error, sending CmdTxDemand. )

RxErr, RxWakeUp, RxDropped, RxEmpty call via_rhine_rx TxAbort, TxUnderrun,PCIErr, StatsMax, MIIChange call via_rhine_error TxAborted calls via_rgine_tx
The others don't look like errors.

Martin Eriksson,
The reason my message said PCI Error and not unhandled
is because it specifies a specific interrupt - IntrPCIErr. (basically the onle one that's left unhandled that can call via_rhine_error)

*blink* I had not noticed that.

All drivers actually need to handle RxNoBufs and RxOverflow, assuming they have similar meaning to what I'm familiar with on other chips. The chip may recover transparently, but one should be at least aware of them.

RxEarly you very likely do -not- want to handle...

Jeff

Sorry, I forgot the subject line for last mail Message has no subject. Garzik's reply is Re: Your Mail.

As for the Interrupts:
Actually, RxNoBuf is handled by calling via_rhine_rx
but not enabled when setting interrupt mask. My patch will fix that.

However, RxOverflow is never handled at all and neither is RxEarly. Nor are they enabled when setting interrupt mask (patch enables).

How should RxOverflow be handled?
Should I call via_rhine_rx, like other errors do? - add IntrRxOverflow (and
possibly RxEarly)

if (intr_status & (IntrRxDone | IntrRxErr | IntrRxDropped |
IntrRxWakeUp | IntrRxEmpty | IntrRxNoBuf))
via_rhine_rx(dev);

How should PCIErr be handled?
Other drivers say:

        /* Hmmmmm, it's not clear how to recover from PCI faults. */

The public docs don't say.

You should probably add them to the list of reasons to call via_rhine_error, and let them be caught by the "wicked" error rule. A reason to use a term that doesn't try to be specific is that we don't really know what is causing the event.

Before your patch IntrTxUnderrun would raise the tx threshold AND issue a CmdTxDemand. After it will only do the first. Do you know that this is an improvement?

Does merging this error handling change into the main kernel really help you test other things for your timeout problem? Can't you just include this bit in your other work?

/Urban

That is precisely the problem.
4 missing interrupts in the interrupt mask, 1 additional interrupt which Jeff Garzik says all drivers should handle and the via-rhine doesn't. 2 interrupts handled quite differently in the linuxfet driver. 1 wicked error message of which I've seen 3 different versions and none of them makes much sense to me.

It is lack of documentation.
The tech sheets do not explain what causes the interrupts in detail, how to handle the interrupts in detail. They rather provide the bit address of the interrupt which is good to know, but not enough. What resources did Donald Becker use when he was writing this?

Perhaps I should ask him.
Or do you know a good document on such things?

Well, most Rx errors are handled in via_rhine_rx.
The "Wicked" error rule doesn't do anything besides a CmdTxDemand. Is this correct handling for, let's say RxOverflow? ..Again, facing the overwhelming lack of documentation :) (see above paragraph)

Yes, we do know what's causing the event.
We know exactly which interrupt.
We just don't know how to handle it.

Ok not really.
Apparently this change is controversial and I'll leave it out of my patch until decided what to do. I would just like the kernel driver to be less buggy and I wanted to contribute.

Plus, I've been making too many changes to my copy. It would be easier to work over a clean kernel driver. I would have less things to keep track of that have been changed.

---------------------------
While we're talking about bugs, here's a (possibly ignorant) question:
Do Rhine-III's have a place in the following? :

#ifdef USE_MEM
static void __devinit enable_mmio(long ioaddr, int chip_id)

{
        int n;
        if (chip_id == VT86C100A) {
                /* More recent docs say that this bit is reserved ... */
                n = inb(ioaddr + ConfigA) | 0x20;
                outb(n, ioaddr + ConfigA);
        } else if (chip_id == VT6102) {
                n = inb(ioaddr + ConfigD) | 0x80;
                outb(n, ioaddr + ConfigD);
        }
}

#endif

-----------------------------

Well, apart from things which are not precisely clear how to fix, would you like to me to submit any portions of my patch for inclusion at all? A little warning: as far as testing goes - my card stalls without the fixes, and it stalls with the fixes :) Yet, they seem simple enough to offer for inclusion.

He's the one who wrote:
"Recovery for other fault sources not known."

I have a feeling he didn't have any other sources, except that he had written a whole bunch of other (similar) drivers before.

Some things you can try to guess:
RxEarly - An interrupt is sent when the chip begins to receive data. if
the driver wants to do something when that happens then it should enable it. The current driver doesn't so it shouldn't be enabled.
Or something completely different.

One option is not to do anything.

Another to catch the event and call via_rhine_error on it. That will make it print the "wicked" message and do a CmdTxDemand. Does it hurt to make a CmdTxDemand when you don't really need it?

If you have a reason to think it hurts, then separate the events that you do CmdTxDemand on:

	if (intr_status & ~IntrRxOverflow)
		writew(CmdTxDemand ...);

(which will still do a CmdTxDemand in all cases it used to do one, but
not if the interrupt was just a IntrRxOverflow. I think.)

And the same for the other interrupt bits that weren't enabled.

Nothing prevents you from having more than one copy of the driver in your local tree(s). Since the driver is just one file that works ok, make a copy of it when you have a version you "like". Then you can look back later and compare.

Probably. You need to check if their docs also enable MMIO by flipping that bit, then you could change the "chip_id == VT6102" part to be just an else.

The wait_for_reset thing is clearly a bug, and the cur_tx-1 is probably right too because you want the number it was queued as. So yes, send them to Jeff.

/Urban