Re: e100 "Ferguson" release

[Jeff Garzik <jgarzik@xxxxxxxxx>]

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