Message-ID: <38E88BC7.437850F9@uow.edu.au>
Date:   Mon, 03 Apr 2000 12:17:11 +0000
From:   Andrew Morton <andrewm@uow.edu.au>
MIME-Version: 1.0
To:     netdev <netdev@oss.sgi.com>
Subject: 3c59x redux
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: owner-netdev@oss.sgi.com
Precedence: bulk

Guys,

I cannot _believe_ the amount of email I'm generating over this.  Still,
it has been useful for me and, I hope, for the Linux 3com driver. 
Thanks for your patience and assistance.

Here is my new list of proposed changes.  Please review.  Please note
the very last point - I think it's a buglet in the existing driver.


- For static compilation, make 'debug' a constant.

- For MODULE compilation, make debug a variable, but put in a
  comment pointing out the code size and cache footprint
  advantages of making it a constant.

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

- use init_etherdev() to allocate dev->priv (and don't kfree
  it).  Check that the address returned from init_etherdev() is
  16-byte aligned.  If it isn't, force an oops.  This will keep
  everyone happy and honest :-)

- make ram_split[] static (shorter code).

- priv.in_interrupt is not used.

- boomerang_start_xmit():

    if (1) {

- Inconsistent use of 'vortex_debug' and 'debug'

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

- handle error return from

	init_etherdev()
	kmalloc()
	pci_alloc_consistent()
	request_region()

- 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.  Move the init_timer()/add_timer() code to
  after this return.

- "For later cards which can handle "porky packets", emulate
  the HAVE_CHANGE_MTU stuff in hamachi.c.  This allows the MTU to
  be changed via an ioctl."

  Put a FIXME comment in the code for this at this time.

Locking issues
==============

The philosophy at this time is "no locks".  The justification is
below.  If things start going wierd then the "big driver lock"
should go in.  (benchmarks?)


- No spinlock is needed in
  vortex_start_xmit/boomerang_start_xmit because:

  1: The Tx and Rx handling in the ISR are separate.  The
     h/w is safe with this and an Rx interrupt during
     vortex_start)xmit() is OK.

  2: netcore prevents hard_start_xmit from being reentered.

- No spinlock needed on vortex_tx_timeout() for the same
  reasons.  tx_timeout() is serialised wrt hard_start_xmit by the
  netcore layer.

- vortex_get_stats() fiddles with the hardware a lot.  It needs
  exclusive access.  But there are no serialisation guarantees
  for get_stats() so we'll do a global cli() in here (it already
  has one) rather than clog the fast paths with a spinlock.

- set_rx_mode() (aka set_multicast_list) has no protection, but
  it's a single outw().  No locking needed here at all.

- vortex_interrupt() uses dev_kfree_skb_irq(), but
  vortex_interrupt() is called from elsewhere in non-IRQ context!
  Alexey says this is OK.

- vortex_ioctl() fiddles with h/w.  It calls mdio_write() which
  is very stateful.  It sets the register window pointer which
  is also stateful. Use cli().

- in vortex_interrupt():

	if (status & TxAvailable) { 

  This looks scary, because it's a vortex-only thing.  But the
  3c905 guarantees that this bit is zero.  Add a comment to
  clarify this.  (Or split the ISR into vortex and boomerang).

- Donald's concern about the new netif_wake_queue() in
  vortex_interrupt().

  Not much we can do about this?

  I overestimated (I said ~100 insns).  In fact netif_wake_queue
  is ~25 insns, ~5 memory hits worst case.

- Don has extra cards in his pci_tbl.  Pinch these.

- Nail 'dev=0' at end of vortex_scan()

- Fix the MOD_INC/DEC_USE_COUNT as per Tim Waugh's report:

	drivers/net/3c59x.c:vortex_attach -- wrong
	drivers/net/3c59x.c:vortex_open -- calls request_irq further up

Still to resolve
================

- cli() is "reliable death of serial interfaces".  What did
  Alexey mean by this? Do we actually drop Rx chars on serial
  ports? Hope not.

- I'm confused.  If 'option=1', we set vp->bus_master, but this
  is only for vortex.  A consequence of this is that the DMADone
  interrupt gets enabled.  But for 905, this bit (bit 8 in the
  interrupt reg) is 'linkEvent'!

  So we should clear vp->bus_master if it's a 905, and arrange
  for this test:

	if (status & DMADone) {
		if (inw(ioaddr + Wn7_MasterStatus) & 0x1000) {

  to not occur at all for 905's.

  We need to split up the ISR - what does Don think?


-- 
-akpm-