Kent E Yoder wrote:
> This patch fixes several bugs and works around known hardware problems
> which conspired to lock up the system randomly. Its somewhat large,
> therefore available at:
http://oss.software.ibm.com/developer/opensource/linux/patches/tr/lanstreamer-0.5.1.patch.gz
>
> * Interrupt function rearranged
> * PCI Configuration changed
> * Tx descriptors had to be reduced to 1 (!)
> * Send/Receive operations are nearly serialized
Marcelo, please do not apply this patch...
Sorry for the delay, review:
1) (in code, not in your patch) prefer kernel-standard types like u32 or
u16 to __u32 and __u16
2) poor formatting:
> +#if STREAMER_IOCTL
> + dev->do_ioctl = &streamer_ioctl;
> +#else
> dev->do_ioctl = NULL;
> +#endif
3) I don't like this playing around with magic numbers. pci_set_master
and pci_enable_device and pci_disable_device twiddle PCI_COMMAND bits,
too. Use constants from pci.h make it clear what bits you are clearing,
and what bits you are setting.
> + pci_write_config_byte (pdev, PCI_CACHE_LINE_SIZE, cls);
> + pci_read_config_word (pdev, PCI_COMMAND, &pcr);
> +
> + /* Turn off Fast B2B enable */
> + pcr &= 0xfdff;
> + /* Turn on SERR# enable and others */
> + pcr |= 0x0157;
> +
> + pci_write_config_word (pdev, PCI_COMMAND, pcr);
> + pci_read_config_word (pdev, PCI_COMMAND, &pcr);
4) Your code appears to -always- set cache line size to zero. Is that a
hardware bug? Look at acenic.c to see a better example of setting PCI
cache line size.
5) what is the purpose of the spin_lock in streamer_open, if open is
serialized? it worries me that the previous streamer_open code disabled
interrupts and the new one does not, but replaces with a non-irq-saving
lock that appears superfluous.
6) udelay(1) after brand new spin_lock in streamer_interrupt is
suspicious
7) disabling interrupts by zeroing NIC intr mask, in interrupt handler,
is general not needed. why was this added? interrupt handlers are not
re-entered so this is not a worry.
8) the while loop in the interrupt looks like it could go on for quite a
while under heavy load, starving out a lot of other kernel code. it
needs a work limit at the very least...
9) disabling interrupts at the beginning of each TX is wrong. you
probably want spin_lock_irqsave at critical parts of the xmit.
10) udelay(100) is likely wrong and a sign of a race (perhaps #9, above,
fixes this)
11) replacing save_flags/cli with normal spin_lock in streamer_close is
suspicious and likely wrong. See issue #5 about streamer_open
serialization. Have you read Documentation/networking/netdevices.txt ?
12) formatting of streamer_ioctl is grossly different from the rest of
the code
13) SIOCDEVPRIVATE ioctls are going away in 2.5. (you can implement
include/linux/ethtool.h SIOCETHTOOL interface for lot of the
functionality, though)
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed Jan 23 2002 - 21:00:20 EST