[9fans] ether8139.c fix

[9fans] ether8139.c fix

[geoff]

Line 494, in rtl8139receive(), reads

				memmove(bp->rp, p, l);
but should be
				memmove(bp->wp, p, l);

jmk has vetted this change.

Re: [9fans] ether8139.c fix

[Don]

> Line 494, in rtl8139receive(), reads
> 
> 				memmove(bp->rp, p, l);
> but should be
> 				memmove(bp->wp, p, l);
> 
> jmk has vetted this change.
Good deal, but, which bug does this fix fix?
Don (north_)
http://www.7f.no-ip.com/

Re: [9fans] ether8139.c fix

[jmk]

OK. I think we all got confused about this. Turns out I thought
the fix posted by geoff@collyer.net was referring to the 2nd memmove in
the receive code, where the real problem lay and which I had already
fixed some time before. My apologies for not taking time to verify
the fix was what I thought it was.

Anyway, in conjunction with Presotto I've tested the following which
completely replaces the code in rtl8139receive() for handling an OK packet:


		/*
		 * Receive Completed OK.
		 * Very simplistic; there are ways this could be done
		 * without copying, but the juice probably isn't worth
		 * the squeeze.
		 * The packet length includes a 4 byte CRC on the end.
		 */
		capr = (capr+4) % ctlr->rblen;
		p = ctlr->rbstart+capr;
		capr = (capr+length) % ctlr->rblen;

		if((bp = iallocb(length)) != nil){
			if(p+length >= ctlr->rbstart+ctlr->rblen){
				l = ctlr->rbstart+ctlr->rblen - p;
				memmove(bp->wp, p, l);
				bp->wp += l;
				length -= l;
				p = ctlr->rbstart;
			}
			if(length > 0){
				memmove(bp->wp, p, length);
				bp->wp += length;
			}
			bp->wp -= 4;
			etheriq(edev, bp, 1);
		}

		capr = ROUNDUP(capr, 4);
		csr16w(ctlr, Capr, capr-16);

Re: [9fans] ether8139.c fix

[Boyd Roberts]

> This fix should be a noop.  Though the change should be made for
> clarity, after the bp = iallocb(length), both bp->rp and
> bp->wp point to the same spot.

Had me puzzled too until I realised it had to be a ring buffer.

Re: [9fans] ether8139.c fix

[presotto]

<geoff@collyer.net> a crit dans le message news:<20020304000219.1ABC919999@mail.cse.psu.edu>...
> Line 494, in rtl8139receive(), reads
> 
> 				memmove(bp->rp, p, l);
> but should be
> 				memmove(bp->wp, p, l);
> 
> jmk has vetted this change.

This fix should be a noop.  Though the change should be made for
clarity, after the bp = iallocb(length), both bp->rp and
bp->wp point to the same spot.

However, looking at the code, if the packet wraps around the
buffer, and if the crc straddles the end of the ring buffer, the
code is just wrong.  It would be better as:

		bp = iallocb(length);
		if(p+length >= ctlr->rbstart+ctlr->rblen){
			l = ctlr->rbstart+ctlr->rblen - p;
			if(bp != nil){
				memmove(bp->rp, p, l);
				bp->wp += l;
			}
			length -= l;
			p = ctlr->rbstart;
		}
		if(length > 0 && bp != nil){
			memmove(bp->wp, p, length);
			bp->wp += length;
		}
		if(bp != nil){
			bp->wp -= 4;

One could rewrite the if-chain to be a bit more efficient, but
you should get the idea.

Of course, I'm assuming that the device allows the CRC to straddle
the end of the buffer.  If not, the code is fine as is.  However,
looking at memmove.s, it'll fault if the length is negative, which
I believe is the observed behaviour.

RE: [9fans] ether8139.c fix

[Jason Gurtz]

> There could be other bugs in the 8139 driver, I don't yet
> have any 8139s to test it on.

I have an edimax card with with the RTL8139A.  I'd be glad to donate to
the cause if you want to email your shipping info to me.  If anyone is
so inclined (not sure how different the chips are) I also have some
older pci 10Mbit 10base-T/10base-2 combo cards with the RTL8029 chip
(unknown manufacturer).

Cheers,

~Jason

--

Re: [9fans] ether8139.c fix

[jmk]

I'd have to dig the card back out of the box I stored it in but I'm pretty
sure it was an 8139C. There's a comment at the beginning of the driver:

	/*
	 * Realtek 8139 (but not the 8129).
	 * Error recovery for the various over/under -flow conditions
	 * may need work.
	 */

I never saw any errors at all on my card. Perhaps someone who has a
problem could track it down. The various free *nix drivers all have
workarounds for odd chip bugs, I would have looked more closely at them
if I'd had a problem to track down.

Re: [9fans] ether8139.c fix

[Plan9]

Well,

We updated and tested with the following cards :
	CARD				CHIPSET
	-----------------		--------
	RTL8139B			RTL8139B
	D-Link DFE-538 TX		DL10038 (alias RTL8139B)
	Compaq 10/100 PCI WOL	EMPX??? (alias RTL8139B)

But it seems like the issues are still here. While transfering a big amount of data or during normal ops (but a long time after booting) the machine unexpectedely crashes by suddenly rebooting.

We have clearly isolated the issue by testing the same PC/pccpud/kfs with an elnk3.

Maybe you did it for 8139A ?

<geoff@collyer.net> a crit dans le message news:<20020304000219.1ABC919999@mail.cse.psu.edu>...
> Line 494, in rtl8139receive(), reads
> 
> 				memmove(bp->rp, p, l);
> but should be
> 				memmove(bp->wp, p, l);
> 
> jmk has vetted this change.

Re: [9fans] ether8139.c fix

[geoff]

The old code was just wrong; it was obviously a typo.

There could be other bugs in the 8139 driver, I don't yet have any
8139s to test it on.

Re: [9fans] ether8139.c fix

[jmk]

On Tue Mar  5 05:08:09 EST 2002, north_@www.7f.no-ip.com wrote:
> > Line 494, in rtl8139receive(), reads
> > 
> > 				memmove(bp->rp, p, l);
> > but should be
> > 				memmove(bp->wp, p, l);
> > 
> > jmk has vetted this change.
> Good deal, but, which bug does this fix fix?
> Don (north_)
> http://www.7f.no-ip.com/

it fixes corrupting every packet which causes wrap-around
of the 64Kb receive buffer. it was fixed a long time ago but
i didn't realise the driver had been released without the
fix.