Bogus, broken changes: was Req. for testing: new 8139too updates

[Jeff Garzik]

Donald Becker wrote:
> 
> On Thu, 30 Mar 2000, Jeff Garzik wrote:
> 
> > Attached is a patch against 2.3.99-pre4-pre1 development kernel, which
> > updates the 8139too network driver to be smarter about autonegotiation
> > and media handling, LED handling, and a lot of other changes.
> 
>  #include <linux/etherdevice.h>
> +#include <linux/delay.h>
>  #include <asm/io.h>
> 
> Do not use delay.h unless you absolutely cannot avoid it.  I removed
> everyplace I could for good reasons.

Why?  (interested)


>  /* A few user-configurable values. */
> +/* media options */
> +static int media[] = {-1, -1, -1, -1, -1, -1, -1, -1};
> +
> 
> This is a new, inconsistent parameter name.
> You are creating a new interface, when a defined one already exists.

Ok, I'll change it to 'option'.


>  #define RX_BUF_LEN (8192 << RX_BUF_LEN_IDX)
> +#define RX_BUF_PAD 16
> +#define RX_BUF_TOT_LEN (RX_BUF_LEN + RX_BUF_PAD)

> This should not be in the "user adjustable compile-time setting".
> It is a hardware detail and is not tunable.

Maybe it should be moved, but I don't like the "+ 16" in the code...


> -/* The following settings are log_2(bytes)-4:  0 == 16 bytes .. 6==1024. */
> +/* The following settings are log_2(bytes)-4:  0 == 16 bytes .. 6==1024, 7==end of packet. */
>  #define RX_FIFO_THRESH 4       /* Rx buffer level before first PCI xfer.  */
> -#define RX_DMA_BURST   4       /* Maximum PCI burst, '4' is 256 bytes */
> -#define TX_DMA_BURST   4       /* Calculate as 16<<val. */
> +#define RX_DMA_BURST   7       /* Maximum PCI burst, '7' is unlimited */
> +#define TX_DMA_BURST   4       /* Maximum PCI burst, '4' is 256 */
> 
> Please keep to 80 column lines.  You removed the calculation comment.
> And increasing the burst size on the pre-PCI-2.1 chips might lead to
> problems.
> 
> +       /* Soft reset the chip. */
> +       RTL_W8 (ChipCmd, (RTL_R8 (ChipCmd) & ChipCmdClear) | CmdReset);
> 
> This retriggers autonegotiation.  This is very bad in some environments.
> For instance, if you are talking to a CISCO switch you won't be able to
> communicate for at least a minute.  This will look exactly like a driver
> bug.
> 
> +       RTL_W16_F (FIFOTMS, RTL_R16 (FIFOTMS) & ~(1<<7));
> +       RTL_W16_F (NWayAdvert, RTL_R16 (NWayAdvert) |
> +                 (1<<13)|(1<<8)|(1<<7)|(1<<6)|(1<<5)|(1<<0));
> +       RTL_W16_F (BasicModeCtrl, RTL_R16 (BasicModeCtrl) |
> +               (1<<13)|(1<<12)|(1<<9)|(1<<8));
> +       RTL_W8_F (MediaStatus, RTL_R8 (MediaStatus) | (1<<7) | (1<<6));
> 
> More horrible crap that won't possibly work with the older chips.

It is test code, which will definitely look different should this become
useful.  So I agree it is ugly.  :)

Remember the RTL-8129 is not and will never be supported by this
driver.  With that in mind, can you actually name a single bit from the
code above which will fail/break on older RTL-8139 chips?


> This also
> breaks 'mii-diag'.

Known problem, will be fixed before release (if any).


> (Hey, you flamed me just weeks ago about magic constants.
> Here you are doing something far worse -- these are independent bits that
> are commonly modified.

<g> I was pretty much planning to take it in the short from you over
this code.  :)

Yeah it will get cleaned up before release.  For RTL8139B and RTL8139A
chips at least (I don't have docs for plain RTL8139), these register
settings are basically ensuring that all media options are enabled when
autonegotiation starts (which has been found to not always be the case
with these chips).

It is -test code- and -known wrong- for a release driver.  A release
driver should read the EEPROM media settings and default to those, while
allowing mii-diag/ethtool/module options to change the media settings. 
A release driver should never force NWay settings as the current test
code does, without checking options at all...

Thanks,

	Jeff





-- 
Jeff Garzik              | Tact is the ability to tell a man 
Building 1024            | he has an open mind when he has a
MandrakeSoft, Inc.       | hole in his head.  (-random fortune)
 | To unsubscribe, send mail to Majordomo@cesdis.gsfc.nasa.gov, and within the
 |  body of the mail, include only the text:
 |   unsubscribe this-list-name youraddress@wherever.org
 | You will be unsubscribed as speedily as possible.