Re: e100 "Ferguson" release

[Jeff Garzik <jgarzik@pobox.com>]

Feldman, Scott wrote:
>>* (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.


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


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


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