2000-10-05 16:19:38

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] tlan timer fix on tytso's list.

As tigran mentions, you should not explicitly zero static vars.

You need to ask yourself if TLAN will -ever- be used in a hotplug
situation, ie. do you find TLAN hardware on CardBus or PCMCIA cards?

If no, you should:


If yes, you have some small bugs...

* tlan_remove_one should be __devexit. Plain 'ole __exit drops the code
from the static kernel image... which is very bad if you have a hotplug
TLAN which gets removed during the lifetime of the kernel.

* return value from pci_module_init and TLan_EisaProbe is never checked.
If you don't care about the return value, just remove 'rc' var.

* tlan_init_one is marked __devinit and TLan_probe1 is marked __init.
If you have hotplug devices, and you insert one after booting,
TLan_probe1 has already been dropped from the kernel image. Oops!

* You do not need to memset(,0,) for TLanPrivateInfo. init_etherdev
does this for you.

* if pci_enable_device fails, you do not clean up. memory leak.

* does TLan_EisaProbe work? It looks like request_region may be called
twice for the same I/O region, once in TLan_EisaProbe, and once in
TLan_Init (via TLan_probe1).

* The following change reverts a bugfix! You should return the
request_irq error return as the current code does:

if ( err ) {
printk(KERN_ERR "TLAN: Cannot open %s because IRQ %d is already in use.\n", dev->name, dev->irq );
- return err;
+ return -EAGAIN;

* You should call netif_wake_queue in TLan_tx_timeout, not
netif_start_queue. (some other net drivers need updating for this, too)
Alarm clock
You have new mail in /var/spool/mail/jgarzik unnecesary change:
-void TLan_PhyMonitor( struct net_device *dev )
+void TLan_PhyMonitor( struct net_device *data )
+ struct net_device *dev = (struct net_device *)data;
TLanPrivateInfo *priv = (TLanPrivateInfo *) dev->priv;