netdev
[Top] [All Lists]

Re: [PATCH] 2.4.20-pre sundance.c update

To: Jeff Garzik <jgarzik@xxxxxxxxxxxxxxxx>
Subject: Re: [PATCH] 2.4.20-pre sundance.c update
From: Donald Becker <becker@xxxxxxxxx>
Date: Thu, 19 Sep 2002 09:23:58 -0400 (EDT)
Cc: netdev@xxxxxxxxxxx, Jason Lunz <lunz@xxxxxxxxxxxx>, Richard Gooch <rgooch@xxxxxxxxxxxxxxx>, "Patrick R. McManus" <mcmanus@xxxxxxxxxxxx>, Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>
In-reply-to: <3D89519C.1020809@xxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
On Thu, 19 Sep 2002, Jeff Garzik wrote:

> Attached is the patch I have against 2.4.20-pre-latest, to start fixing 
> from as a baseline.

diff -Nru a/drivers/net/sundance.c b/drivers/net/sundance.c
> --- a/drivers/net/sundance.c  Thu Sep 19 00:22:26 2002
> +++ b/drivers/net/sundance.c  Thu Sep 19 00:22:26 2002
...
>  /* Maximum events (Rx packets, etc.) to handle at each interrupt. */
> -static int max_interrupt_work = 30;
> +static int max_interrupt_work = 0;

Please don't change the semantics of module parameters.  All of my PCI
network drivers have used this name for years with the same semantics.
Arbitrarily changing is A Bad Thing.

> static int mtu;

Get rid of this.  See the MTU setting code in my driver release.

>     bonding and packet priority, and more than 128 requires modifying the
>     Tx error recovery.
>     Large receive rings merely waste memory. */
> -#define TX_RING_SIZE 16
> -#define TX_QUEUE_LEN 10              /* Limit ring entries actually used.  */
> -#define RX_RING_SIZE 32
> +#define TX_RING_SIZE 64
> +#define TX_QUEUE_LEN (TX_RING_SIZE - 1) /* Limit ring entries actually used. 
>  */

Did you miss reading the comment?  A Tx ring size of 64 is a bad idea.
Increasing the Rx ring size is fine, considering that most current
machines have plenty of memory and the kernel hasn't always been good
about keeping skbuffs available.

+MODULE_PARM(flowctrl, "i");

Flow control should be negotiated, not set.

> -     TxThreshold = 0x3c,
> +     TxStartThresh = 0x3c,
> +     RxEarlyThresh = 0x3e,

[[ Many other name changes omitted ]]

Why change the symbolic names for the registers?

>       /* Frequently used values: keep some adjacent for cache effect. */
>       spinlock_t lock;
> +     spinlock_t rx_lock;                                     /* Group with 
> Tx control cache line. */

Uhmmm, again, read the comment.  This comment was originally with the
'txlock' variable

        } else if (dev->mc_count) {
                struct dev_mc_list *mclist;
-               memset(mc_filter, 0, sizeof(mc_filter));
+               int bit;
+               int index;
+               int crc;
+               memset (mc_filter, 0, sizeof (mc_filter));
                for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
-                        i++, mclist = mclist->next) {
-                       set_bit(ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x3f,
-                                       mc_filter);
+                    i++, mclist = mclist->next) {
+                       crc = ether_crc_le (ETH_ALEN, mclist->dmi_addr);
+                       for (index=0, bit=0; bit < 6; bit++, crc <<= 1)
+                               if (crc & 0x80000000) index |= 1 << bit;
+                       mc_filter[index/16] |= (1 << (index % 16));
                }

Uhhmmm, perhaps calling a function to do the wrong-endian CRC and then
bit-reversing the result is not an improved way to do this?  That's why
there needs to be two Ethernet CRC functions -- so that you don't
need to bit bit-reverse the result.


-- 
Donald Becker                           becker@xxxxxxxxx
Scyld Computing Corporation             http://www.scyld.com
410 Severn Ave. Suite 210               Second Generation Beowulf Clusters
Annapolis MD 21403                      410-990-9993


<Prev in Thread] Current Thread [Next in Thread>