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

[Donald Becker]

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				[e-mail]
Scyld Computing Corporation		http://www.scyld.com
410 Severn Ave. Suite 210		Second Generation Beowulf Clusters
Annapolis MD 21403			410-990-9993