Message-ID: <38E87D8A.E86A9F5B@uow.edu.au>
Date:   Mon, 03 Apr 2000 11:16:26 +0000
From:   Andrew Morton <andrewm@uow.edu.au>
MIME-Version: 1.0
To:     netdev <netdev@oss.sgi.com>
Subject: Re: coverage
References: <38E73398.E1727365@uow.edu.au>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: owner-netdev@oss.sgi.com
Precedence: bulk

Andrew Morton wrote:
> 
> My approach to the first two was to put a 5 mSec delay in
> boomerang_rx(), decrease max_interrupt_work to 5 and ping flood it.
> 
> This produces some pretty ugly results.

OK, found it.  It could be interpreted as a silicon (or specification)
bug.  But it can be fixed in the driver.

vortex_interrupt()
{
	...
	if (status & RxComplete)
		vortex_rx(dev);
	...
}

In the 3c905, rxComplete indicates that the rx FIFO has one or
more full packets.  But it is _internally_ acknowledged when
the upload engine has transferred the packet to memory.  So
there is a small window during which that bit will be set.

But the bit shouldn't be visible to the processor because
it's being masked off in the Indication Enable register.
"Indications disabled with this command do not cause an
interrupt to the host, nor are they visible in the
IntStatus register".

However I am very occasionally seeing this bit set when
processing updateStats interrupts.  Off we go to vortex_rx
and the kernel is destroyed secretly and fatally!

When describing this bit in the IntStatus reg the 905 spec
says "This bit is automatically acked by the upload engine
as it uploads packets. Drivers should disable this interrupt
and mask this bit when reading IntStatus".

But the initialisation code in vortex_open() only does half
the job:

vp->status_enable = SetStatusEnb | HostError|IntReq|StatsFull|TxComplete|
       (vp->full_bus_master_tx ? DownComplete : TxAvailable) |
       (vp->full_bus_master_rx ? UpComplete : RxComplete) |
       (vp->bus_master ? DMADone : 0);
vp->intr_enable = SetIntrEnb | IntLatch | TxAvailable | RxComplete |
        StatsFull | HostError | TxComplete | IntReq
        | (vp->bus_master ? DMADone : 0) | UpComplete | DownComplete;

Note that it is setting clearing RxComplete in the
IndicationEnable reg but setting RxComplete in the
InterruptEnable reg.  This appears to be confusing
the chip.

When I cleared RxComplete in the InterruptEnable reg the
problem went away.  So there's the fix:

         vp->intr_enable = SetIntrEnb | IntLatch | TxAvailable |
		(vp->full_bus_master_rx ? 0 : RxComplete) |
                StatsFull | HostError | TxComplete | IntReq
                | (vp->bus_master ? DMADone : 0) | UpComplete | DownComplete;

I think we should be treating TxAvailable in the same way in
this statement.

Don, it perturbs me that we are testing RxComplete and TxAvailable in
the 905 ISR.  And vice versa for 59x's, of course.  I think it would
be better from a cleanliness and performance (esp. cache footprint) POV
to break out a separate boomerang_interrupt.  What do you think?

--
-akpm-