netdev
[Top] [All Lists]

3c59x.c

To: netdev <netdev@xxxxxxxxxxx>
Subject: 3c59x.c
From: Andrew Morton <andrewm@xxxxxxxxxx>
Date: Sat, 25 Mar 2000 14:12:28 +0000
Sender: owner-netdev@xxxxxxxxxxx
Would anyone object if I did some work on this driver? (2.3.99)

Let me assume the answer is "no" and plunge on...

I have identified a stack of things which _could_ be done.  I've
broken them down into optional, desirable and essential.  I would
propose that the optional stuff gets noted in  a comment and the
desirable and essential things get implemented.

Would those in the know please review my notes below, correct any
misconceptions, add things I've missed?

There are a few questions in here as well.  They are marked with
"???".  Thanks.




[ A lot of the "vortex_foo" comments also apply to "boomerang_foo" ]

Optional
=======

- Debug code is _always_ compiled in, and debug level defaults to 1.

  This is OK, it would be better to make it easy for 'debug' to be
  literal "0".  This reduces the driver size by 1.5k.

  'debug' should be made always a literal constant in a non-modular
  compile because it can't be altered at runtime or boot.

- struct pci_id_info has a 'probe1' method.  It is never used.  Remove this.

???
- why do we independently kmalloc dev->priv?  Why not let
  init_etherdev() do it?

???
- Where does 'option' come from in vortex_probe1()?  Don't understand
  this.

- some printk's which don't use KERN_XXXX

- make ram_split[] static (shorter code).

- vortex_open(): this is dodgy:

    for (i = 2000; i >= 0 ; i--)
        if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
            break;

  Is 2000 'huge' enough to still work on a 3GHz CPU?

  Would prefer:

    for (i = 10; i >= 0 ; i--)
    {
        if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
            break;
        udelay(10);
    }

- priv.in_interrupt is not used.

- vortex_tx_timeout():

    for (j = 200; j >= 0 ; j--)
        if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
            break;

  CPU-speed dependent.


- vortex_error():

            for (i = 2000; i >= 0 ; i--)
                if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
                    break;

  (three places)

- vortex_start_xmit():

        if (inw(ioaddr + TxFree) > 1536) {
            netif_wake_queue(dev);

  But there is this:

        static const int mtu = 1500;

  use this 'knob' rather than "1536".

- boomerang_start_xmit():

    if (1) {

  huh?

- Inconsistent use of 'vortex_debug' and 'debug'

- vortex_interrupt():

  What does the stuff under

     if (status & DownComplete) {

  do?  Looks like transmitter shutdown handling?

- vortex_rx():

       while (inw(ioaddr + Wn7_MasterStatus) & 0x8000)
              ;

  while we're waiting for DMA to complete.  Is this efficient from a
  bus utilisation POV?

- [OT] Gack.  Can eth_type_trans() be sped up?

- vortex_rw():

        for (i = 200; i >= 0; i--)
            if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
                break;

  two places.

- mdio_read():

    for (i = 14; i >= 0; i--) {
        int dataval = (read_cmd&(1<<i)) ? MDIO_DATA_WRITE1 :
             MDIO_DATA_WRITE0;

 Could use

    for (i = 1 << 14; i; i >>= 1) {
        int dataval = (read_cmd&i) ? MDIO_DATA_WRITE1 :
             MDIO_DATA_WRITE0;

 Big deal.  These aren't in a fast path.

- mdio_write():

  Ditto .

- #if ! defined(final_version).  This seems to be cruft - it's
  always compiled in (there is no final version!)

Desirable
=========

- return value from init_etherdev() is not checked.

- return value from kmalloc() is not checked (two places)

- return from pci_alloc_consistent() not checked.

- return from request_region is unchecked.

- vortex_open():

    /* Use the now-standard shared IRQ implementation. */
    if (request_irq(dev->irq, &vortex_interrupt, SA_SHIRQ, dev->name, dev)) {
        return -EAGAIN;
    }

  This happens _after_ we've called init_timer()/add_timer, so vortex_timer()
  will end up getting called on a driver for which the open failed.  Use
  del_timer() here.

???
- vortex_open()

    skb = dev_alloc_skb(PKT_BUF_SZ);
    ...
    skb_reserve(skb, 2);    /* Align IP on 16 byte boundaries */

    Shouldn't that be PKT_BUF_SZ + 2?


Essential
=========

- Are there MOD_INC/DEC_USE_COUNT races?  What are the rules here?

- vortex_start_xmit() is racy.  It is not protected from h/w
  interrupts.  It needs a spin_lock_irqsave(priv->lock) and
  vortex_interrupt() needs a spin_lock(priv->lock);

  In vortex_start_xmit(), the spin_lock_irq() should happen
  immediately before the netif_stop_queue().

* vortex_start_xmit() calls netif_stop_queue() and then under some
  circumstances (non-DMA o/p and there is room in the Tx buffer) it
  calls netif_wake_queue().  Seems OK, as long as it's done under the
  spin_lock_irqsave().

- Ditto boomerang_start_xmit()

- vortex_tx_timeout() doesn't always call netif_wake_queue.  Is this
  OK?

- vortex_tx_timeout() fiddles with h/w and is not protected from
  vortex_interrupt() which also fiddles with h/w registers. 
  Use priv->lock for this case as well.

- boomerang_start_xmit() calls cli().  Remove this and wrap the
  entire function in spin_lock_irqsave(priv->lock)

- vortex_get_stats() needs a spinlock in update_stats().  (lock in
  dev.priv, call spinlock_init()).  Remove cli().

- set_rx_mode() probably needs a spinlock.  Safest to add it.

- vortex_interrupt() uses dev_kfree_skb_irq(), but vortex_interrupt()
  is called from elsewhere in non-IRQ context!  Use
  dev_kfree_skb_any().

???
- vortex_ioctl():

    switch(cmd) {
    case SIOCDEVPRIVATE:        /* Get the address of the PHY in use. */
        data[0] = phy;
    case SIOCDEVPRIVATE+1:      /* Read the specified MII register. */
        EL3WINDOW(4);
        data[3] = mdio_read(ioaddr, data[0] & 0x1f, data[1] & 0x1f);

  Do we mean to fall through the first case? This is either very
  devious or plain wrong.

- [OT] pci.c is racy.

- [OT] waah!  skeleton.c uses global cli()/sti()!

- [OT] I had a peek at eepro100.c.  The use of wait_for_cmd_done() is
  racy.



-- 
-akpm-

<Prev in Thread] Current Thread [Next in Thread>