Re: e100 "Ferguson" release

Feldman, Scott wrote:

quoted

* (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;
}

Yes.  I would also printk(KERN_ERR "we have a bug!") or somesuch, like several other drivers do, too.

quoted

* 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.

hmmm.  I prefer the phy scanning to checking eeprom, since it reduces the chance of eeprom screwups.  However, I still think there's some issue related to phy id #0.  Oh well, fine for now, I guess.

quoted

* 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?

Well, the ->get_stats only returns a pointer to the stats, which are then accessed in an unlocked manner.  Since the net stats are unsigned longs, asynchronously reading and updating them isn't a big deal in practice.

quoted

* (minor) use a netif_msg_xxx wrapper/constant in e100_init_module test?

Can't - don't have nic->msg_enable allocated yet.  :(

You could always use "(1 << debug) - 1"... :)  I dunno if it's worth worrying about.

	Jeff