Re: [RFR] gianfar ethernet driver

[Jeff Garzik <jgarzik@xxxxxxxxx>]

Andy Fleming wrote:
> > > 7) ethtool_ops should not be kmalloc'd.
> > > +       dev_ethtool_ops =
> > > +               (struct ethtool_ops *)kmalloc(sizeof(struct 
> > > ethtool_ops),
> > > +                                             GFP_KERNEL);
>
>
> Is there a particular reason?  The reason I did it this way is because 
> the driver supports a 10/100 controller which does not have the RMON 
> statistics, and therefore should not read from that register space.  So 
> I needed to use different functions to fill in the statistics lists.

Just switch out one static pointer for another, once you know the 
hardware you're dealing with.  You _must_ have that knowledge anyway, 
before calling register_netdev(), otherwise you're got a race in 
initialization versus interface-up.


> > > 8) I recommend enabling _both_ hardware interrupt coalescing and NAPI,
> > > at the same time.  Several other Linux net drivers need to be changed to
> > > do this, as well
>
>
> Ok... but this is possible with the driver as it is.  Interrupt 
> Coalescing is runtime configurable, and NAPI is a compile-time option.  
> NAPI can sometimes hurt performance, and so we'd like to have the 
> ability to disable it for some deployments.  I guess I'm not sure what 
> change this suggestion was meant to cause.

Default hardware mitigation to on in all cases, and preferably NAPI as well.

If you see cases that hurt performance, that wants investigating.


> > > 14) surely gfar_set_mac_address() needs some sort of synchronization?
>
>
> I'm not sure why.  It only gets called during gfar_open(), and 
> startup_gfar() gets called after it.

Incorrect.  Set-mac-address can be called when the interface is up and 
active, such as by the bonding driver.


> > > 15) gfar_change_mtu() synchronization appears absent?
>
>
> I'm not exactly sure what kind of synchronization is expected here.  
> stop_gfar() and startup_gfar() do modify the appropriate hardware values.

Same conditions (and response) as set-mac-address.  These can be called 
while you're actively DMAing packets.


> > > 23) gfar_set_multi() does not appear to take into account huge
> > > multi-cast lists.  Drivers usually handle large dev->mc_count by falling
> > > back to ALLMULTI behavior.
>
>
> Actually, it does.  The bits which are set represent hash table values. 
>  Essentially, the MAC addr is converted to an 8-bit CRC.  This value 
> then serves as an index to the 256-bit hash table.  If the list is 
> large, then certain bits may be written twice, but if all the bits are 
> set, then the behavior is the same as in the ALLMULTI behavior -- every 
> multicast packet arrives.  It would be a mistake, IMHO, to cut it off 
> after N entries, as several of those entries could overlap in the 
> bitmap.  Clearly, the less bits which are set, the more effective the 
> hardware filter, so checking each address will, I think, always be a 
> performance win or tie (on the filtering side) over ALLMULTI

ok


> > > 24) setting IFF_MULTICAST is suspicious at best, possibly wrong:
> > > +       dev->mtu = 1500;
> > > +       dev->set_multicast_list = gfar_set_multi;
> > > +       dev->flags |= IFF_MULTICAST;
> > > +
>
>
> I'll believe you, but is there a reason for this?  I guess it's already 
> set, by default, so easy fix.

follow the other drivers, and behave predictably :)


> > > 28) I see you support register dump (good!), now please send the
> > > register-pretty-print patch to the ethtool package :)
>
>
> Heh.  Well, I haven't written that, yet.  There's actually a slight 
> problem, in that the 85xx registers are 32-bit, and refuse to be 
> accessed as bytes.  I'll be sure to look at that in the near...ish future.

Nothing wrong with accessing registers as 32-bit quantities.  That 
attribute is common to a lot of NICs.


> > > 31) infinite loops, particularly on 1-bits, are discouraged:
> > > +       /* Wait for the transaction to finish */
> > > +       while (gfar_read(&regbase->miimind) & (MIIMIND_NOTVALID |
> > > MIIMIND_BUSY))
> > > +               cpu_relax();
>
>
> What is the suggested method for waiting for the bus to be free?  Should 
> I timeout after some time, and bring the driver down?

it's really up to you, as it depends on the implementation platforms.

The most simple is to include a counter that counts down to zero, and 
starts some absurdly large number like 100000.


> > > 33) merge-stopper:  mii_parse_sr().  never wait for autonegotiation to
> > > complete.  it is an asynchronous operation that could exceed 30 seconds.
>
>
> Hmm...
>
> > > 34) merge-stopper:  dm9161_wait().  same thing as #33.
>
>
> This may be a problem.  That function is there to work around an issue 
> in the PHY, wherein if you try to configure it before it has come up all 
> the way, it refuses to bring the link up.  We've sworn at this code many 
> times, but there has, as of yet, not been a good suggestion as to how we 
> can ensure that the 9161 is  ready before we configure it.

I interpret that as a driver bug :)

As common sense, regardless of phy bugs, you should not be trying to 
configure the MAC _or_ the phy in the middle of autonegotiation. 
Presumeably you are using a combination of netif_stop_queue(), 
netif_carrier_off(), and netif_device_detach() to achieve this.


> > > 35) liberally borrow code and ideas from Ben H's sungem_phy.c.
> > > Eventually we want to move to a generic phy module.
>
>
> Heh.  ironically, I stole liberally from the ibm_emac driver, which now 
> looks exactly like the sungem_phy code for phy handling.  A generic phy 
> module would be a good thing, and I'm even interested in helping with 
> code and/or suggestions.  If nothing else, I'll jump on the new generic 
> phy module bandwagon!

Cool.  This item #35 is more of a long term "think about it" type of 
request.  Please do not hesitate to think of ways that you could share 
code with sungem_phy.c, and/or make them both use the same API, and 
submit patches along those lines :)

It's not enough to just write a driver, help change Linux for the better :)


>  From Jamal:
>
> > 1) The check (in gfar_start_xmit()):
> >
> > Should happen much sooner - i.e before the skb is touched.
>
>
> I'm not sure I agree here.  What I am doing is detecting the full state 
> before an skb needs to be rejected.  I am testing the NEXT descriptor to 
> see if it is ready (if not, dirty_tx will be pointing to it).  This way, 
> I always handle any skb that is passed to gfar_start_xmit()

See my comments to jamal as well:  if you guarantee that you always have 
room on the DMA ring for an additional skb, that check should never be 
needed.

Some extremely cautious/paranoid programmers will add a check, with a 
printk noting it's a BUG (i.e. impossible) condition, such as

        if (unlikely(... no more descriptors ...)) {
                printk(KERN_ERR "%s: BUG: no room for TX\n", dev->name);
                netif_stop_queue();
                spin_unlock_irqrestore();
                return 1;
        }

        ... queue TX to hardware ...

        if (no more descriptors)
                netif_stop_queue()

        spin_unlock_irqrestore()


> > 2) Also its pretty scary if you are doing:
> > txbdp->status |= TXBD_INTERRUPT for every packet.
> > Look at other drivers, they try to do this every few packets;
> > or enable tx mitigation to slow the rate  of those interupts.
>
>
> I don't believe this is as dire as you think.  This bit only indicates 
> that, if the conditions are right, an interrupt will be generated once 
> that descriptor is processed, and its data sent.  Conditions which 
> mitigate that are:
> 1) coalescing is on, and the timer and counter have not triggered the 
> interrupt yet
> 2) NAPI is enabled, and so interrupts are disabled after the first 
> packet arrives
> 3) NAPI is disabled, but the driver is currently handling a previous 
> interrupt, so the interrupts are disabled for now.

Even at 10/100 speeds, you really don't want to be generating one 
interrupt per Tx...

        Jeff