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
Please apply.
Thanks,
Kent <[email protected]>
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
Thanks for your points, I will take a look at these when I can...
Kent
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
2) Fixed.
3) Yeah, this was bad... Fixed.
4, 6, 7, 9 & 10) These fall into the category of hardware workaround. The
lanstreamer hardware is essentially broken (with respect to its hardware
specification) and these were a few of the things which made it work.
For #6, the udelay(1) had more to do with the following write() than
with spin_lock(). When that delay was not there, the write failed
randomly. The same with the udelay(10) at the end of the interrupt
function...
For #7, 9 & 10, zeroing out interrupts in the interrupt function and in
the xmit function and the delay have no software function at all, but it
made the hardware happy. Without these, the card would lock up the box
under heavy load.
5 & 11) Nope, I had not read Doc/networking/netdevices.txt, so I have a
question. What does being inside rtnl_lock() imply other than the sleep
issues?
The calls to cli() and save_flags() were wrong from the beginning. They
were imported by the last maintainer since this driver is a modified
version of the olympic token ring driver. The current spin_lock() and
spin_unlock() calls protect the srb_queued variable. If we were to set it
to one and then get interrupted before we actually write() to the srb, the
interrupt function will try to service whatever junk happens to be in the
srb. If going into the interrupt function is covered by rtnl_lock(), this
would be covered, but I guess its not (?)....
8) Yeah, we didn't see any problems with this in test (probably due to
lanstreamer's fairly low bandwidth), but you are right. Fixed.
12) Fixed.
13) Hmm.. Guess I'll need to learn about that... eventually :^). Fixed,
for now...
Kent <[email protected]>
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
> For #6, the udelay(1) had more to do with the following write() than
> with spin_lock(). When that delay was not there, the write failed
> randomly. The same with the udelay(10) at the end of the interrupt
> function...
That smells of covering up a race rather than fixing something. Another
thing you may be doing perhaps is hiding PCI posting effects ?
Alan
Sorry, this mail was sent acidentally... but since its out here...
> 5 & 11) Nope, I had not read Doc/networking/netdevices.txt, so I have a
question. What does being inside rtnl_lock() imply other than the sleep
> issues?
>
> The calls to cli() and save_flags() were wrong from the beginning.
They were imported by the last maintainer since this driver is a modified
version > of the olympic token ring driver. The current spin_lock() and
spin_unlock() calls protect the srb_queued variable. If we were to set it
to one and then > get interrupted before we actually write() to the srb,
the interrupt function will try to service whatever junk happens to be in
the srb. If going into the
> interrupt function is covered by rtnl_lock(), this would be covered, but
I guess its not (?)....
actually I should be using spin_lock_irqsave() in open() and close()
since the lock is taken inside the interrupt function, no?
Kent
> actually I should be using spin_lock_irqsave() in open() and close()
> since the lock is taken inside the interrupt function, no?
Correct - which might explain some of your other delays curing lockups
> > actually I should be using spin_lock_irqsave() in open() and close()
> > since the lock is taken inside the interrupt function, no?
>
> Correct - which might explain some of your other delays curing lockups
Hmmm.. i wish it did. Unfortunately the only non irq saved calls to
spin_lock are in the open and close functions, my lock call in xmit is irq
saved. These should be changed obviously, but I haven't seen the box lock
up during open or close...
Kent
>
>> For #6, the udelay(1) had more to do with the following write() than
>> with spin_lock(). When that delay was not there, the write failed
>> randomly. The same with the udelay(10) at the end of the interrupt
>> function...
>
>That smells of covering up a race rather than fixing something. Another
>thing you may be doing perhaps is hiding PCI posting effects ?
Ok, I thought of one thing that might make things clearer here: when I
say "the write failed", I mean that we saw the write go out on the PCI bus
and then the box locked up. We were looking at it on a PCI bus analyzer.
That, and it wasn't just this write, or just writes in general, it really
seemed random.
BTW, I don't know what PCI posting effects are...
Kent
> BTW, I don't know what PCI posting effects are...
Ok given
writel(foo, dev->reg);
udelay(5);
writel(bar, dev->reg);
The pci bridge is at liberty to delay the first write until the second or a
read from that device comes along (and wants to do so to merge bursts). It
tends to bite people
- When they do a write to clear the IRQ status and don't do
a read so they keep handling lots of phantom level triggered
interrupts.
- When there is a delay (reset is common) that has to be observed
- At the end of a DMA transfer when people unmap stuff early
and the "stop the DMA" command got delayed
Alan
On Fri, 18 Jan 2002, Alan Cox wrote:
> > BTW, I don't know what PCI posting effects are...
>
> Ok given
>
> writel(foo, dev->reg);
> udelay(5);
> writel(bar, dev->reg);
>
> The pci bridge is at liberty to delay the first write until the second or a
> read from that device comes along (and wants to do so to merge bursts). It
> tends to bite people
>
> - When they do a write to clear the IRQ status and don't do
> a read so they keep handling lots of phantom level triggered
> interrupts.
Not only (when the write is intended to clear some interrupt condition).
As the actual clear of the interrupt condition is delayed, then it may
just also clear a sub-sequent condition and this condition may be missed
by the driver interrupt routine. Due to this a race, interrupt stall can
occur.
> - When there is a delay (reset is common) that has to be observed
>
> - At the end of a DMA transfer when people unmap stuff early
> and the "stop the DMA" command got delayed
G?rard.
On Fri, Jan 18, 2002 at 05:02:57PM -0600 or sometime in the same epoch, Kent E Yoder scribbled:
> Ok, I thought of one thing that might make things clearer here: when I
> say "the write failed", I mean that we saw the write go out on the PCI bus
> and then the box locked up. We were looking at it on a PCI bus analyzer.
> That, and it wasn't just this write, or just writes in general, it really
> seemed random.
>
Kent,
We had this on olympic for certain high end IBM boxen. Spent forever
trying to trap it as I couldn't emulate the behaviour on my test
boxes. We weren't getting correct values from pci reads/write and we
were running out of buffers as they weren't getting flushed. The
machine wouldn't lock but the adapter would stop tx/rx.
Turned out the pci bridge on the machine itself was causing the
problems. Tweaking the pci bus fixed the problem.
--
Mike Phillips
Linux Token Ring Project
http://www.linuxtr.net
mailto: [email protected]
Mike,
Did you tweak the card's PCI config area to fix this problem, or
elsewhere?
Kent
> Kent,
>
> We had this on olympic for certain high end IBM boxen. Spent forever
> trying to trap it as I couldn't emulate the behaviour on my test
> boxes. We weren't getting correct values from pci reads/write and we
> were running out of buffers as they weren't getting flushed. The
> machine wouldn't lock but the adapter would stop tx/rx.
>
> Turned out the pci bridge on the machine itself was causing the
> problems. Tweaking the pci bus fixed the problem.
On Mon, Jan 21, 2002 at 10:44:20AM -0600 or sometime in the same epoch, Kent E Yoder scribbled:
> Did you tweak the card's PCI config area to fix this problem, or
> elsewhere?
>
Kent,
Nope, the tweak was on the machine itself, something to do with the
pci bus itself. Was weird, would only show up under heavy load. Doing
multiple simulaneous ftp's of large files (ISO images) would create the
problem, but doing a single ftp transfer of the same file wouldn't
create the problem.
I honestly can't remember what the exact fix was now, this was a
couple of years ago.
--
Mike Phillips
Linux Token Ring Project
http://www.linuxtr.net
mailto: [email protected]
I think the delays in the driver *were* just working around PCI posting
effects. I tested by removing all the delays and instead putting
something like:
writew(val, addr);
(void) read(addr);
instead, to flush the PCI cache. Things seem to be happy.
Is this the best way to make sure the PCI cache is flushed for writes that
need to happen immediately? I don't see many other drivers doing it...
Kent
> > BTW, I don't know what PCI posting effects are...
>
> Ok given
>
> writel(foo, dev->reg);
> udelay(5);
> writel(bar, dev->reg);
>
> The pci bridge is at liberty to delay the first write until the second or
a
> read from that device comes along (and wants to do so to merge bursts).
It
> tends to bite people
>
> - When they do a write to clear the IRQ
status and don't do
> a read so they keep handling lots of
phantom level triggered
> interrupts.
>
> - When there is a delay (reset is common)
that has to be observed
>
> - At the end of a DMA transfer when people
unmap stuff early
> and the "stop the DMA" command got
delayed
>
> Alan
On Wed, Jan 30, 2002 at 01:27:29PM -0600, Kent E Yoder wrote:
> Is this the best way to make sure the PCI cache is flushed for writes that
> need to happen immediately?
Pretty much...
> I don't see many other drivers doing it...
Some drivers don't need or do things in other ways, some need it and
don't have it...
If you wanted to audit and test long driver delays in various random
the world will cheer your name, I'm sure ;-) ;-)
Jeff
On Wed, Jan 30, 2002 at 01:27:29PM -0600, Kent E Yoder wrote:
> I think the delays in the driver *were* just working around PCI posting
> effects. I tested by removing all the delays and instead putting
> something like:
> writew(val, addr);
> (void) read(addr);
>
> instead, to flush the PCI cache. Things seem to be happy.
>
> Is this the best way to make sure the PCI cache is flushed for writes that
> need to happen immediately? I don't see many other drivers doing it...
Wouldn't creating a flush_and_writew() or similar be an idea here?
/David Weinehall
_ _
// David Weinehall <[email protected]> /> Northern lights wander \\
// Maintainer of the v2.0 kernel // Dance across the winter sky //
\> http://www.acc.umu.se/~tao/ </ Full colour fire </
> > Is this the best way to make sure the PCI cache is flushed for writes that
> > need to happen immediately? I don't see many other drivers doing it...
>
> Wouldn't creating a flush_and_writew() or similar be an idea here?
You can't write a generic function for it. The PCI rules are all quite
simple and well documented, unfortunately it a book nobody has 8)
> effects. I tested by removing all the delays and instead putting
> something like:
> writew(val, addr);
> (void) read(addr);
>
> instead, to flush the PCI cache. Things seem to be happy.
>
> Is this the best way to make sure the PCI cache is flushed for writes that
> need to happen immediately? I don't see many other drivers doing it...
In many cases its not needed. Devices tend to be structured to avoid having
to force pci writes - that by definition is a CPU stall and loses you marks
in the benchmarketing and the pretty zdnet graphs.
There other thing it can also hide is pci write merging bugs (which some
devices have). For example at least one ethernet chip fails if you do
writew(mac_addr, ioport+foo);
writew(mac_addr+2, ioport+foo+2);
writew(mac_addr+4, ioport+foo+4);
because the writes get merged and a chip bug causes a bus hang on the 32bit
write of the mac address. In that case its not the posting being masked but
the merging.
Alan
On Wed, Jan 30, 2002 at 01:27:29PM -0600 or sometime in the same epoch, Kent E Yoder scribbled:
> I think the delays in the driver *were* just working around PCI posting
> effects. I tested by removing all the delays and instead putting
> something like:
> writew(val, addr);
> (void) read(addr);
Yep, I has a similar problem when developing olympic, I would write
twice to the same location in very quick succession. This was using
one of the built in locations that the chipset would binary OR to the
current value. So theoretically the second write should have been
OR'ed with the first write, but the second write was being done before
the first write was committed across the pci bus, resulting in only
the second value being stored.
In the end, I rewrote it to combine into a single write.
--
Mike Phillips
Linux Token Ring Project
http://www.linuxtr.net
mailto: [email protected]
> > effects. I tested by removing all the delays and instead putting
> > something like:
> > writew(val, addr);
> > (void) readw(addr);
Ok, now I'm curious about something...
If the readw() above is needed here (it definitely fixes *something*),
what purpose does the volatile below serve?
io.h:122:#define writew(b,addr) (*(volatile unsigned short *)
__io_virt(addr) = (b))X
Is this a sort of "go do this now" command to flush it from the CPU to the
PCI bus, while the readw() makes sure its flushed out of the PCI cache?
> >
> > instead, to flush the PCI cache. Things seem to be happy.
> >
> > Is this the best way to make sure the PCI cache is flushed for writes
that
> > need to happen immediately? I don't see many other drivers doing
it...
Another question is, if the PCI bus is caching like this, how does it
handle adapters which write to one address and read from another for the
same variable? I'm guessing it flushes all writes on a read? This is
exactly what lanstreamer does, and I'm thinking this may have caused
problems before.
Thanks,
Kent
> handle adapters which write to one address and read from another for the
> same variable? I'm guessing it flushes all writes on a read? This is
> exactly what lanstreamer does, and I'm thinking this may have caused
> problems before.
You probably want to get the actual documentation and read it - there are
a set of guarantees that I/O's do not pass one another. A read will not pass
a write for example. When you think about PCI as a message passing system
in both directions its generally a lot better for your head 8). The guarantees
are also defined in terms of PCI functions rather than a single port
Alan
> what purpose does the volatile below serve?
>
> io.h:122:#define writew(b,addr) (*(volatile unsigned short *)
> __io_virt(addr) = (b))X
>
> Is this a sort of "go do this now" command to flush it from the CPU to the
> PCI bus, while the readw() makes sure its flushed out of the PCI cache?
The compiler isnt obliged to actually generate the assignment in order
otherwise, so given
writew(1, foo+2);
writew(2, foo);
it might have the urge to reverse them - perhaps to optimise in using
postincrement modes
volatile says "stuff happens here beyond the compilers direct knowledge of
events".
Simple example
i=0;
while(i<100000) i++;
can be optimised to i=100000 if i is not volatile
Alan