Messages in this thread | | | Date | Thu, 17 Jan 2002 05:57:06 -0500 | From | Jeff Garzik <> | Subject | Re: [PATCH] IBM Lanstreamer bugfixes |
| |
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/
| |