On Sat, 25 Mar 2000, Andrew Morton wrote:
> To: netdev <netdev@xxxxxxxxxxx>
> Subject: 3c59x.c
>
> Would anyone object if I did some work on this driver? (2.3.99)
You should bring this up on the linux-vortex mailing list.
If you do not read the linux-vortex mailing list, you should not even think
about modifying the driver until you understand more about it.
> - 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.
This is highly useful information when tracking down problems.
Most problems are not in the driver structure, but problems with the system
(IRQs, PCI bus, and the like) or the media selection.
> - struct pci_id_info has a 'probe1' method. It is never used. Remove this.
This is because it was incompletely converted for 2.3.99.
Rule: It's not reasonable to halfway convert something and walk away.
> - why do we independently kmalloc dev->priv? Why not let
> init_etherdev() do it?
Because of alignment requirements. Some descriptors must be longword
aligned, and they perform much better when cache line aligned. You must
understand caching systems before touching this. It's OK to take slight
performance hits on rarely used systems like the Sparc, but you should
throughly understand what the PCI bus implementation on the adapter is doing.
> ???
> - Where does 'option' come from in vortex_probe1()? Don't understand
> this.
LILO parameters.
> - some printk's which don't use KERN_XXXX
You only put that at the beginning of the emitted line!
I see only one place where this is incorrect, on line 848, and that should
only show up for certain CardBus adapters. (Hmmm, that line came from a
*later* driver version than 99H!)
> - vortex_open(): this is dodgy:
No, it's not.
> for (i = 2000; i >= 0 ; i--)
> if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
> break;
>
> Is 2000 'huge' enough to still work on a 3GHz CPU?
Yes. These are based on PCI bus transactions. This is not a CPU timing loop.
> Would prefer:
>
> for (i = 10; i >= 0 ; i--)
> {
> if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
> break;
> udelay(10);
> }
No. This is wrong, and potentially very, very bad.
Do a profile on how many times this loop executes. It is typically zero or
one times.
Regarding the similar reset code, I explicitly changed 3Com drivers back
after Alan changed them to use udelay(). The udelay() code was broken at
one point -- it didn't take into account running faster because of cache
alignment. Alan blamed me for using the wrong timing info, and left a note
about my "bug" in the driver. Neither I nor 3Com could reproduce the
problem. Later the udelay() fix was very quietly slipped into place.
> - priv.in_interrupt is not used.
The code for this lock was eliminated in recent 2.3.* changes. But whoever
did that change did only a minimal, hackish job of updating the driver.
> - vortex_tx_timeout():
>
> for (j = 200; j >= 0 ; j--)
> if ( ! (inw(ioaddr + EL3_STATUS) & CmdInProgress))
> break;
>
> CPU-speed dependent.
No, it's not. See above.
You *must* understand what is going on before you touch the code.
> - 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".
Noooooo. No.
> - boomerang_start_xmit():
>
> if (1) {
>
> huh?
Hackish driver update to 2.3.*. Not my fault. Look at the original 99H
source, or better, the 99M source code to understand why the original code
made sense.
> - vortex_interrupt():
> What does the stuff under
> if (status & DownComplete) {
> do? Looks like transmitter shutdown handling?
No. Think "DownloadComplete", except I used the names defined in the 3Com
manual when they didn't conflict with some other convention.
> - vortex_rx():
> while (inw(ioaddr + Wn7_MasterStatus) & 0x8000)
> ;
>
> while we're waiting for DMA to complete. Is this efficient from a
> bus utilisation POV?
Usually no. This is a bad loop, and violates my usual coding standards.
But in this special case, for the 3c590 series only, we are waiting on a
single PCI bus burst transaction that we just triggered. The typical loop
iteration count is zero, but has no easily calculated upper bound.
> - [OT] Gack. Can eth_type_trans() be sped up?
Gackkkk! It *is* horrible, and a cache pig to boot. Not my fault, and
it certainly should be part of netif_rx() not a separate call in the driver.
It was put in by the people that do PPP and didn't care about performance.
> - mdio_read():
..
> - mdio_write():
[[ Pointless code change. Don't touch touch that code. It take me a long
time to get the serial EEPROM code and the MDIO code just right. Once I
did, and verified the cases on an o-scope, I copied that code verbatim
across the various drivers. If you think it's easy, write a duplicate
function from scratch, just reading the datasheet. ]]
> ???
> - 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?
No. PKT_BUF_SZ is 1536, not 1514/1518. It ends on a cache line boundary.
Drivers that handle "porky packets" (and the recent 3Com cards have the
hardware support to do this) use vp->rx_buf_sz instead of a constant.
See the Hamachi driver for an example.
> Essential
> =========
>
> - Are there MOD_INC/DEC_USE_COUNT races? What are the rules here?
The locking for this is handled outside the drivers, as it should be.
> - 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);
The vortex_start_xmit() is for the 3c590 series only, not the later hardware.
The hardware only needs to be protected against simultaneous transmit
attempts, not a receive that happens in the middle of transferring a packet
to the card's (large!) FIFO.
Putting a spin lock there will waste a *lot* of spin time, since the old
3c590 isn't especially fast, especially if we are using PIO mode.
> * 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().
These macros are only OK if they are trivial locking functions. When they
are more complex functions every card without a large Tx queue will suffer
badly.
> - vortex_tx_timeout() doesn't always call netif_wake_queue. Is this
> OK?
It depends on the Tx scheme in use.
> - 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.
Yes.
All MII drivers use this code (and some without MII even fake the registers).
You should understand how this driver relates to other drivers, the
diagnostics code, and the programs that read MII registers.
> - [OT] waah! skeleton.c uses global cli()/sti()!
I've had pci-skeleton.c for a long time, but it apparently wasn't worthy.
It is now incorrect for 2.3.*.
At least parts of the ancient skeleton.c have been updated with the new
interface.
> - [OT] I had a peek at eepro100.c. The use of wait_for_cmd_done() is
> racy.
It's not quite as it appears. Most run-time commands are instantly accepted
or slow the next PCI response. The only commands that take visible time are
the initialization.
Remember that locks are not free. They cost a lot of time, and measuring
that time usually changes the timing to understate the cost.
Donald Becker
Scyld Computing Corporation, becker@xxxxxxxxx
|