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