eepro100.c: configuration bug

Michael Barthelow m.barthelow@f5.com
Mon Mar 29 16:52:29 1999


This message is in MIME format. Since your mail reader does not understand
this format, some or all of this message may not be legible.

------_=_NextPart_001_01BE7A2E.6D036500
Content-Type: text/plain;
	charset="iso-8859-1"

> There shouldn't be collisions with a FDX connection -- I 
> assume you mean CRC errors in that case.

I meant frame alignment errors, which I think could happen as the result of
an interrupted (truncated) transmit.

> 
> > The problem is that bit 7 of configuration byte 5 is being 
> > used as "DMA Maximum Byte Count Disable" but in fact, asserting 
> > this bit *enables* the DMA Maximum Byte Counters. 
> > With the default configuration of config byte 4 = 0 and config 
> > byte 5 = 0x80 we have the worst possible configuration: we've 
> > enabled the Maximum byte counters and set the counters to zero.
> 
> As you note below, it's *not* enabled by default.
> 
> > So in the 1.06 driver, line 377 should change from:
> > 22, 0x08, 0, 0,  0, 0x80, 0x32, 0x03,  1, 
> >
> > to:
> > 22, 0x08, 0, 0,  0, 0, 0x32, 0x03,  1, 
> 
> This table is always copied before use and the value overwritten.

I apologize for describing your method of allowing a user override of
the txdmacount to automatically enable the counters as a bug. 
I misinterpreted the code. 

I guess my objections are reduced to saying that I think that a default
configuration table ought to represent as much as possible a valid setup,
and not depend on later manipulation to work correctly (i.e. byte 5 should
be 0)

mb


> 
> > This problem may not show up in all systems because of 
> > another bug in the driver. In lines 32-34 we have:
> 
> Not a bug, but the intended behavior.
> 
> > /* Tx/Rx DMA burst length, 0-127, 0 == no preemption, 
> tx==128 -> disabled. */
> > static int txdmacount = 128;
> ...
> >                  config_cmd_data[5] = txdmacount + 0x80; 
> > 
> > I think the 0x80 is added here thinking to disable the DMA 
> > maximum byte counters. But since txdmacount has been 
> > initialized to 0x80, bit 7 carries out of the byte and we 
> > are left with zero, which does indeed disable the byte counters!
> 
> Yes!  That's the way it's supposed to work.
> 
> This is intended to make it easy to set the value -- documentation is
> correct.
>    The burst length limit is disabled by default.
>    A value in the range 1-127 sets the burst limit.
>    A zero set the length limit to no pre-emption
>      (which is valid, but not a good idea).
> 
> > >From the Intel programmer's manual:
> > 
> > "It is recommended that software does not enable the DMA 
> > maximum byte counters. Enabling the counters and placing 
> > relatively low values in them could result in shorter bursts 
> > on the PCI bus. This could reduce system performance and lead 
> > to transmit underruns or receive overruns. If the counters are 
> > enabled, then it is suggested to use fairly large values in the 
> > counters (in other words, values greater than 50 decimal"
> > 
> > I believe the collisions and crc errors are caused by 
> > transmit underruns, because with zero value counters, the 
> > Tx & Rx threads are thrashing in contending for the bus.
> 
> 

------_=_NextPart_001_01BE7A2E.6D036500
Content-Type: text/html;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">



RE: eepro100.c: configuration bug



> There shouldn't be collisions with a FDX = connection -- I
> assume you mean CRC errors in that case.

I meant frame alignment errors, which I think could = happen as the result of an interrupted (truncated) transmit.

>
> > The problem is that bit 7 of configuration = byte 5 is being
> > used as "DMA Maximum Byte Count = Disable" but in fact, asserting
> > this bit *enables* the DMA Maximum Byte = Counters.
> > With the default configuration of config = byte 4 =3D 0 and config
> > byte 5 =3D 0x80 we have the worst possible = configuration: we've
> > enabled the Maximum byte counters and set = the counters to zero.
>
> As you note below, it's *not* enabled by = default.
>
> > So in the 1.06 driver, line 377 should = change from:
> > 22, 0x08, 0, 0,  0, 0x80, 0x32, = 0x03,  1,
> >
> > to:
> > 22, 0x08, 0, 0,  0, 0, 0x32, = 0x03,  1,
>
> This table is always copied before use and the = value overwritten.

I apologize for describing your method of allowing a = user override of
the txdmacount to automatically enable the counters = as a bug.
I misinterpreted the code.

I guess my objections are reduced to saying that I = think that a default configuration table ought to represent as much as = possible a valid setup, and not depend on later manipulation to work = correctly (i.e. byte 5 should be 0)

mb


>
> > This problem may not show up in all = systems because of
> > another bug in the driver. In lines 32-34 = we have:
>
> Not a bug, but the intended behavior.
>
> > /* Tx/Rx DMA burst length, 0-127, 0 =3D=3D = no preemption,
> tx=3D=3D128 -> disabled. */
> > static int txdmacount =3D 128;
> ...
> = >           &n= bsp;      config_cmd_data[5] =3D txdmacount + = 0x80;
> >
> > I think the 0x80 is added here thinking to = disable the DMA
> > maximum byte counters. But since = txdmacount has been
> > initialized to 0x80, bit 7 carries out of = the byte and we
> > are left with zero, which does indeed = disable the byte counters!
>
> Yes!  That's the way it's supposed to = work.
>
> This is intended to make it easy to set the = value -- documentation is
> correct.
>    The burst length limit is = disabled by default.
>    A value in the range 1-127 = sets the burst limit.
>    A zero set the length limit = to no pre-emption
>      (which is valid, = but not a good idea).
>
> > >From the Intel programmer's = manual:
> >
> > "It is recommended that software does = not enable the DMA
> > maximum byte counters. Enabling the = counters and placing
> > relatively low values in them could result = in shorter bursts
> > on the PCI bus. This could reduce system = performance and lead
> > to transmit underruns or receive overruns. = If the counters are
> > enabled, then it is suggested to use = fairly large values in the
> > counters (in other words, values greater = than 50 decimal"
> >
> > I believe the collisions and crc errors = are caused by
> > transmit underruns, because with zero = value counters, the
> > Tx & Rx threads are thrashing in = contending for the bus.
>
>

------_=_NextPart_001_01BE7A2E.6D036500--