RE: e100 "Ferguson" release
From: Feldman, Scott <hidden>
Date: 2003-08-05 03:45:08
New one: http://sf.net/projects/e1000, e100-3.0.0_dev12.tar.gz
Comments:
Thanks Jeff!
* (API) Does the out-of-tx-resources condition in e100_xmit_frame ever really happen? I am under the impression that returning non-zero in ->hard_start_xmit results in the packet sometimes being requeued and sometimes dropped. I prefer to guarantee a more-steady state, by simply dropping the packet unconditionally, when this uncommon condition occurs. So, I would a) mark the failure condition with unlikely(), and b) if the condition occurs, simply drop the packet (tx_dropped++, kfree skb), and return zero.
Stop the queue also?
if(unlikely(e100_exec_cb(nic, skb, e100_xmit_prepare) == -ENOMEM)) {
netif_stop_queue(netdev);
nic->net_stats.tx_dropped++;
dev_kfree_skb(skb);
return 0;
}
Added some more likely/unlikely's in the perf paths.
* (minor) for completeness, you should limit the PCI class in the pci_device_id table to PCI_CLASS_NETWORK_ETHERNET. There are one-in-a-million cases where this matters, but it's usually a BIOS bug. Still, it's there in pci_device_id table, and it's an easy change, so might as well use it.
OK
* (style) your struct config definition is terribly clever. perhaps too clever, making it unreadable? Not a specific complaint, mind you, just something that caught my eye.
Then the driver would be perfect. We can't have that. ;-)
* (minor) in tg3, my own benchmarks and experiments showed it helped to explictly use ____cacheline_aligned markers when defining certain sections of members in struct tg3 (or struct nic, in e100's case). You already clearly pay attention to member layout WRT cache effects, but if you have a clear dividing line, or lines, in struct nic you can use _____cacheline_aligned for even greater benefit. At a minimum test it with a cpu-usage-measuring benchmark like ttcp, though, of course :)
OK
* (extremely minor) some people (like me :)) consider dead reads like the readb() call in e100_write_flush
OK
* (major?) Aren't there some clunky e100 adapters that don't do MMIO? Do we care?
Not that I'm aware of. Current e100 doesn't support them if they're out there.
* I would love to see feedback from people testing this driver on ppc64 and sparc64, particularly.
Me too. Things seem to work on ppc (Mac) and ia64.
* (style, minor) My eyes would prefer functions like e100_hw_reset to have a few more blank lines in them, spreading code+comment blocks out a bit.
OK
* (extremely minor) one wonders if you really need the write flush in mdio_ctrl. If the flush is removed, the same net effect appears to occur.
Good catch.
* (boring but needed) convert all the magic numbers in e100_configure into constants, or at least add comments describing the magic numbers. If the value is just one bit, you might simply append "/* true */", for example. The general idea is to make the "member name =
value" list a little bit more readable to somebody who doesn't know
the
hardware, and struct config, intimately.
That _was_ boring.
* IIRC Donald's MII phy scanning code scans MII phy ids like this: 1..31,0. Or maybe 1..31, and then 0 iff no MII phys were found. In general I would prefer to follow his eepro100.c probe order. Some phys need this because they will report on both phy id #0 (which is magical) and phy id #(non-zero). Donald would know more than me,
here. [kernel] eepro100 gets the ID from the eeprom, so no scanning there. Current e100 goes 1, 0..31, which is what we've always done, IIRC.
* Is it easy to support MII phy interrupts? It would be nice to get a callback that was handled immediately, on phys that do support such interrupts.
I don't see those being passed through and handled by the MAC.
* do we care about spinlocks around the update_stats and get_stats code?
Not sure. update_stats runs in a timer callback. Can get_stats jump in?
* (bugs) in e100_up, you should undo mod_timer [major] and netif_start_queue [minor], if request_irq fails. And maybe stop the receiver, too?
OK
* for all constants 0xffffffff (and others as well if you so choose), prefer the C99 suffix to a cast. This is particularly relevant in pci_set_dma_mask calls, where one should be using 0xffffffffULL, but applies to other constants as well.
I didn't see any other constant casts besides the pci_set_dma_mask call. That one is fixed.
* (potential races) in e100_probe, you want to call register_netdev as basically the last operation that can fail, if possible. Particularly, you need to move the PCI API operations above register_netdev. Remember, register_netdev winds up calling /sbin/hotplug, which in turn calls programs that will want to start using the interface. So you need to have everything set up by that point, really.
OK (nice catch).
* in e100_probe, "if(nic->csr == 0UL) {" should really just test for
NULL, because ioremap is defined to return a pointer...OK
* (minor) use a netif_msg_xxx wrapper/constant in e100_init_module test?
Can't - don't have nic->msg_enable allocated yet. :( -scott