Realtek 8139 problem on 486.

Hello all,

I'm observing a problem with Realtek 8139 cards on a couple of 486 boxes. The respective driver is 8139too. It starts operation successfully, obtains an ip address via dhcp, replies to pings steadily, but some subsequent communication fails apparently. At least, nfsroot is unusable (it gets stuck in "... not responding, still trying" forever), and also iperf3 -c xxx when run against a neighbour box on a lan prints 2-3 lines with some reasonable 7Mbit/s rate, then just prints 0s and subsequently throws a panic about output queue full or some such.

My kernel is 4.14.221 at the moment, but I can compile another if necessary.
I've already tried the "#define RTL8139_DEBUG 3" and "8139TOO_PIO=y" and "#define RX_DMA_BURST 4" and "#define TX_DMA_BURST 4" (in case there is a PCI burst issue, as mentioned somewhere) and nothing changed whatsoever.

Some additional notes:
- the problem is 100% reproducable;
- replacing this 8139 card with some entirely different 8139-based card changes nothing;
- if I replace this 8139 with a (just random) intel Pro/1000 card, everything seem to work fine;
- if I insert this 8139 into some other 486 motherboard (with a different chipset), everything seem to work fine again;
- etherboot and pxelinux work fine.

I'm willing to do some debugging but unfortunately I'm not anywhere familiar with this driver and network controllers in general, therefore I'm asking for some hints/advice first.

Thank you,

Regards,
Nikolai

This driver hasn't seen functional changes for ages. Any previous kernel version that works fine so that you could bisect? It will be hard to find any developer who has test hw, especially as your issue seems to be system-dependent.
Please provide a full dmesg log, maybe it provides a hint.

You write: "reasonable 7Mbit/s rate"
So you operate the systems with a 10Mbit hub? Or any other reason why you consider 7Mbps reasonable?

Heiner

I have a few active 80486 test systems (the 5.12.7 kernel works fine on 80486) that I might be able to help test with too.

- Matthew

Hi all,

30.05.2021 0:44, tedheadster:

Unfortunately I don't have any proven-to-work-well previous kernel for this specific hardware config, and dmesg does not show anything even slightly usefull.

However, I've just installed Debian Woody (kernel 2.2.20, gcc 2.95) and got something interesting. There are two interfaces shown in ifconfig:

eth0 Link encap:Ethernet HWaddr 00:11:6B:32:85:74

          inet addr:192.168.0.3  Bcast:192.168.0.255  Mask:255.255.255.0
          ......
          Interrupt:9 Base address:0x6000

eth1 Link encap:Ethernet HWaddr 00:11:6B:32:85:74

          inet addr:192.168.0.4  Bcast:192.168.0.255  Mask:255.255.255.0
          ......
          Interrupt:9 Base address:0x8000

whereas actually only one physical network card present. One can notice HWaddr and Irq is the same, although base addr is not.

lspci says:
00:0d.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8139 (rev 10)

Now as soon as ip configuration is done preperly, I can ifup any of them successfully.
And, eth0 shows some familiar misbehaviour: I can do ssh, but scp large file fails with subsequent communication breakdown. On the other hand, eth1 works fine, scp and iperf3 (both as a client and as a server) show quite regular operation. I'm puzzled a bit. But at least it looks like a possible starting point.

Thank you,

Regards,
Nikolai

Hi all,

30.05.2021 3:49, Nikolai Zhubr:
[...]

Ok, sorted out this.
eth0 == rtl8139.c:v1.07 5/6/99
eth1 == 8139too Fast Ethernet driver 0.9.18-pre4

So, 8139too 0.9.18-pre4 as of kernel 2.2.20 is the one that apparently works correctly here. I feel the bisect is going to be massive.

Thank you,

Regards,
Nikolai

Hi all,

30.05.2021 13:36, Nikolai Zhubr:

It appears the problem arrived between kernel 2.6.2 and 2.6.3, that is respectively 8139too versions 0.9.26 and 0.9.27. Unmodified kernel 2.6.2 works fine, unmodified kernel 2.6.3 shows reproducable connectivity issues.

The diff is not small, I'm not sure I can dig through. Any hints/ideas greatly appreciated.

Thank you,

Regards,
Nikolai

This is apparently when NAPI was introduced into the driver.

One thing I noticed here was the handling for shared IRQs changing in the process. Do you happen to have shared IRQs, and if so, can you change it so this card has an IRQ that is not shared with any other device?

Arnd

Hi all,

30.05.2021 23:54, Arnd Bergmann:
[...]

I think this IRQ is not shared:

# cat /proc/interrupts
           CPU0
  0:     322570          XT-PIC  timer
  1:          8          XT-PIC  i8042
  2:          0          XT-PIC  cascade
  9:        896          XT-PIC  eth0
 14:      18000          XT-PIC  ide0
NMI:          0
ERR:          0

# dmesg | grep -i irq
eth0: RealTek RTL8139 at 0xc4800000, 00:11:6b:32:85:74, IRQ 9
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
serio: i8042 AUX port at 0x60,0x64 irq 12
serio: i8042 KBD port at 0x60,0x64 irq 1

However indeed, it seems a problem was introduced with a rework of interrupt handling (rtl8139_interrupt) in 2.6.3, because I have already pushed all other differences from 2.6.3 to 2.6.2 and it still keeps working fine. My resulting minimized diff is still ~300 lines, it is too big and complicated to be usefull to post here as is.

Thank you,

Regards,
Nikolai

Hi all,

31.05.2021 2:17, Nikolai Zhubr:
[...]

Some more input.

I was able to minimize the problematic diff to basically one screenfull, it is quite comprehencable now, and I'm including it below. It is the change in status/event handling due to a switch to NAPI that intruduced the problem. Now, in some more detailed tests, I observe that _receiving_ still works fine. It is _sending_ that suffers, and apparently, only when trying to send a lot at a time. In such case I see these warnings:

NETDEV WATCHDOG: eth0: transmit timed out
eth0: link up, 100Mbps, full-duplex, lpa 0xC5E1

It looks like the queue of tx frames somehow gets messed up.

The essential diff fragment:
================================================
dev->open = rtl8139_open; dev->hard_start_xmit = rtl8139_start_xmit;
+ dev->poll = rtl8139_poll;
dev->weight = 64;
dev->stop = rtl8139_close; dev->get_stats = rtl8139_get_stats;
@@ -2015,7 +2010,7 @@
tp->stats.rx_bytes += pkt_size; tp->stats.rx_packets++;

- netif_rx (skb);
+ netif_receive_skb (skb);
} else {
if (net_ratelimit())
printk (KERN_WARNING
@@ -2138,10 +2133,8 @@
u16 status, ackstat;
int link_changed = 0; /* avoid bogus "uninit" warning */ int handled = 0;
- int boguscnt = max_interrupt_work;

spin_lock (&tp->lock);
- do {
status = RTL_R16 (IntrStatus);

/* shared irq? */
@@ -2169,8 +2162,14 @@
if (ackstat)
RTL_W16 (IntrStatus, ackstat);

- if (netif_running (dev) && (status & RxAckBits))
- rtl8139_rx (dev, tp, 1000000000);
+ /* Receive packets are processed by poll routine.
+ If not running start it now. */
+ if (status & RxAckBits){
+ if (netif_rx_schedule_prep(dev)) {
+ RTL_W16_F (IntrMask, rtl8139_norx_intr_mask);
+ __netif_rx_schedule (dev);
+ }
+ }

/* Check uncommon events with one test. */
if (unlikely(status & (PCIErr | PCSTimeout | RxUnderrun | RxErr)))
@@ -2182,16 +2181,6 @@
if (status & TxErr)
RTL_W16 (IntrStatus, TxErr);
}
- boguscnt--;
- } while (boguscnt > 0);
-
================================================

Thank you,

Regards,
Nikolai

Hi all,

Some more results follow. I'll report on all suggestions here in one go for brevity.

Just tested with "_F" added to all of them, did not help.

I get the idea in general, but not sure how exactly you proposed to move rtl8139_tx_interrupt() and adjust the RTL_W16 (IntrStatus, ackstat). But meanwhile, I tried a dumb thing instead, and it worked!
I've put back The Loop:
---------------------------
+ int boguscnt = 20;

        spin_lock (&tp->lock);
+       do {
        status = RTL_R16 (IntrStatus);

        /* shared irq? */
@@ -2181,6 +2183,8 @@
                if (status & TxErr)
                        RTL_W16 (IntrStatus, TxErr);
        }

+ boguscnt--;
+ } while (boguscnt > 0);
out:
---------------------------
With this added, connection works fine again. Of course it is silly, but hopefully it gives a path for a real fix.

If I understand correctly this means packet scheduler type. In more recent kernels I typically have CONFIG_DEFAULT_NET_SCH="fq_codel", now in 2.6.3 no explicite scheduler is enabled, so it must be some fast fifo. But as the sympthoms were basically identical in e.g. 2.6.3 and 4.14, I suppose it is unlikely to be the cause.

Ok, the changes compiled fine, but unfortunately made no noticable difference.

Thank you,

Regards,
Nikolai

What was discussed here 16 yrs ago should sound familiar to you.
https://www.mail-archive.com/[email protected]/msg92234.html
"It was an option in my BIOS PCI level/edge settings as I posted." You could check whether you have same/similar option in your BIOS and play with it.

So it appears that the interrupt is lost if new TX events come in after the status register is read, and that checking it again manages to make that race harder to hit, but maybe not reliably.

The best idea I have for a proper fix would be to move the TX processing into the poll function as well, making sure that by the end of that function the driver is either still in napi polling mode, or both RX and TX interrupts are enabled and acked.

Arnd

Hi all,

01.06.2021 10:20, Arnd Bergmann:
[...]

Yes indeed, this motherboard does have such an option, and it defaulted to "Edge", which apparently is not what PCI device normally expects. Changing it to "Level" made unmodified kernel 2.6.4 work fine.
And 8259A.pl comfirms this, too.

Before:
# ./8259A.pl
irq 0: 00, edge
irq 1: 00, edge
irq 2: 00, edge
irq 3: 00, edge
irq 4: 00, edge
irq 5: 00, edge
irq 6: 00, edge
irq 7: 00, edge
irq 8: 00, edge
irq 9: 00, edge
irq 10: 00, edge
irq 11: 00, edge
irq 12: 00, edge
irq 13: 00, edge
irq 14: 00, edge
irq 15: 00, edge

After:
# ./8259A.pl
irq 0: 00, edge
irq 1: 00, edge
irq 2: 00, edge
irq 3: 00, edge
irq 4: 00, edge
irq 5: 00, edge
irq 6: 00, edge
irq 7: 00, edge
irq 8: 06, edge
irq 9: 06, level
irq 10: 06, level
irq 11: 06, edge
irq 12: 06, edge
irq 13: 06, edge
irq 14: 06, edge
irq 15: 06, edge

It looks like incorrect IRQ triggering mode makes 2 or more IRQs merge into one, kind of. However, if I understand this 8139 operation logic correctly, the possible max number of signaled events in one go is limited by the number of tx/rx descriptors and can not grow beyond it while inside the interrupt handler in any case. If so, using the loop would seem not that bad, and the limit would be certainly not 20 but max(NUM_TX_DESC, CONFIG_8139_RXBUF_IDX) == 4.

This one is too complicated for me to implement myself, so I'll have to wait if someone does this.

Alternatively, maybe it is possible to explicitely request level mode from 8259 at the driver startup?

Thank you,

Regards,
Nikolai

Great, so you have a solution for your problem, as i would expect that this setting also makes 8139too in newer kernels usable for you.

This hardware is so old that it's not very likely someone spends effort on this.

I have doubts that it is possible to overrule what the BIOS/ACPI communicate to the kernel.

Hi all,

01.06.2021 14:42, Heiner Kallweit:
[...]

Well, not exactly, although I'm definitely getting close to some sort of satisfactory solution, and I appreciate all the helpfull replies.

Relying on this BIOS setting is still not good, because yet another motherboard I've just tested today does not have it (or at least in no evident form anyway).

Now I'd like to ask, is quality reliable fix still wanted in mainline or rather not? Because I'll personally do my best to create/find a good fix anyway, but if it is of zero interest for mainline, I'll probably not invest much time into communicating it. My understanding was that default rule is "if broken go fix it" but due to the age of both code and hardware, maybe it is considered frozen or some such (I'm just not aware really).

Thank you,

Regards,
Nikolai

Driver 8139too has no maintainer. And you refer to "mainline" like to a number of developers who are paid by somebody to maintain all drivers in the kernel. That's not the case in general. You provided valuable input, and if you'd contribute to improving 8139too and submit patches for fixing the issue you're facing, this would be much appreciated.

Heiner

Hi all,

02.06.2021 0:48, Heiner Kallweit:

Ups, indeed. I just looked at the header and supposed it has. Sorry. (I do not touch kernel development much, usually)

Ok, it is a bit more clear now.
I'll do more testing/searching/reading and probably come up with something then.

Thank you,

Regards,
Nikolai

It was apparently maintained by Jeff Garzik until 2007, but he already considered
this driver outdated back then, and later stopped maintaining drivers altogether.

I thought about the issue a bit more. The idea of the level interrupt handler is that it
gets called again as long as any of the bits in 'IntrStatus' are set, so the handler
can just read the register, write ack or mask the bits it sees and then process the events it got. If another tx-complete even comes in before the end of the handler,
it gets entered again and everything works.

If the irqchip is set to edge mode, this breaks down, and the loop can work around
it by making it much more likely that all bits are cleared or masked at the end of the handler, but there is no guarantee for this: If any interrupt comes in
between reading the IntrStatus register and finishing the handler, you never get an interrupt again.

I think the easiest workaround to address this reliably would be to move all the irq processing into the poll function. This way the interrupt is completely masked in the device until the poll handler finishes, and unmasking it while there
are pending events would reliably trigger a new irq regardless of level or edge mode. Something like the untested change at https://pastebin.com/MhBJDt6Z . I don't know of other drivers that do it like this though, so I'm not sure if this
causes a different set of problems.

Arnd

Hi Arnd,

02.06.2021 12:12, Arnd Bergmann:
[...]

I started applying your patch (trying to morph it a little bit so as to shove in a minimally invasive manner into 4.14) and then noticed that it probably won't work as intended. If I'm not mistaken this rx poll thing is only active within kind of "rx bursts", so it is not guaranteed to be continually running all the time when there is no or little rx input. I'd suppose some new additional work/thread would have to be introduced in order for such approach to be reliably implemented.

Meanwhile, beside the lost tx irq issue, I've apparently identified rx overrun issue. According to tinymembench, the raw RAM performance of this system is roughly around 15-30 Mbytes/s at best, so it is close to 100Mbit wire speed. Tracing NFS over UDP operation (client side) I've found that of 2 full-sized incoming NFS/UDP packets the second one will always be lost, approved by rapid increase of iface err counter. More specifically, I've found that a couple of packets sized 1500+700 can still be successfully accepted, but no way 1500+1500. Apparently 8139 has very little ram builtin so it needs that packets can go into main ram fast enough. It appeared though that just adding rsize=1024 allows NFS work quite well, with only ocasional small pauses. Also, apparenly TCP/IP somehow recovers/autotunes iteself automatically, so it just works fine. I suppose this overrun problem can not be fixed in a general form (other than forcing a downgrade of link speed to 10 Mbit), as AFAIK there are no provisions in ethernet to request e.g. extra delays between packets. What might be usefull though is dropping some line to dmesg suggesting to somehow limit the incoming flow. Such hint in dmesg would have saved me quite some time.

Anyway, for now I got it working quite well (with a re-added busy loop and rsize=1024). I'm going to look at the elcr_set_level_irq approach later, but it looks quite complicated. If there is something else I can test while at it, please let me know.

Thank you,

Regards,
Nikolai

The basic idea of the napi poll function is to do batch processing as much as possible and avoid excessive interrupts when the system is already busy processing the received data.

However, it should not lead to any missed interrupts with my patch:
at any point in time, you have either all hardware interrupts enabled, or you are in napi polling mode and are guaranteed to call the poll function within a relatively short timespan. If you have no pending rx events, processing should be pretty much instantaneous, it just gets pushed from the irq handler to immediately following the irq handler. If there is a constant stream of incoming data, it gets moved into softirqd context, which may be delayed when there is another thread running.

Arnd

08.06.2021 10:44, Arnd Bergmann:
[...]

For this to work, napi_complete should likely be called with some different condition instead? E.g.:

- if (work_done < budget) {
+ if ((work_done < budget) && !status) {

Otherwise polling would possibly be shut down before all non-rx events are cleared?
For some reason such 'corrected' version does not work here though (Communication fails completely). Probably I'm still missing something.

Thank you,

Regards,
Nikolai

The idea was that all non-rx events that were pending at the start of the function have been Acked at this point, by writing to the IntrMask register before processing the particular event. If the same kind of event arrives after the Ack, then opening in the mask should immediately trigger the interrupt handler, which reactivates the poll function.

If you instead want to re-arm the poll function every time that 'status' was non-zero, you would have to also return 'budget' to the caller, like

+ if (status)
+ work_done = budget; /* pretend RX is still busy */

        if (work_done < budget) {

I think that should also work, but it seems more expensive since it would always go back into the poll function after a non-RX event, rather than only going back into the irq handler if a new event has just arrived.

Arnd

08.06.2021 23:45, Arnd Bergmann:

Ok, it works, indeed. The overall bitrate seems lower somewhat.
I'll re-test and benchmark some few variants (e.g. with and without busy loop) and report my findings.

Thank you,

Regards,
Nikolai

If it's only a bit slower, that is not surprising, I'd expect it to use fewer CPU
cycles though, as it avoids the expensive polling.

There are a couple of things you could do to make it faster without reducing reliability, but I wouldn't recommend major surgery on this driver, I was just going for the simplest change that would make it work right with broken IRQ settings.

You could play around a little with the order in which you process events:
doing RX first would help free up buffer space in the card earlier, possibly alternating between TX and RX one buffer at a time, or processing both in a loop until the budget runs out would also help.

Arnd

Hi Arnd,

09.06.2021 10:09, Arnd Bergmann:
[...]

I've modified your patch so as to quickly test several approaches within a single file by just switching some conditional defines. My diff against 4.14 is here:
https://pastebin.com/mgpLPciE

The tests were performed using a simple shell script:
https://pastebin.com/Vfr8JC3X

Each cell in the resulting table shows:
- tcp sender/receiver (Mbit/s) as reported by iperf3 (total)
- udp sender/receiver (Mbit/s) as reported by iperf3 (total)
- accumulated cpu utilization during tcp+upd test.

The first line in the table essentially corresponds to a standard unmodified kernel. The second line corresponds to your initially proposed approach.

All tests run with the same physical instance of 8139D card against the same server.

(The table best viewed in monospace font)

+-------------------+-------------+-----------+-----------+
| #Defines          ; i486dx2/66  ; Pentium3/ ; PentiumE/ |
|                   ; (Edge IRQ)  ;  1200     ; Dual 2600 |
+-------------------+-------------+-----------+-----------+
| TX_WORK_IN_IRQ 1  ;             ; tcp 86/86 ; tcp 94/94 |
| TX_WORK_IN_POLL 0 ;  (fails)    ; udp 96/96 ; udp 96/96 |
| LOOP_IN_IRQ 0     ;             ; cpu 59%   ; cpu 15%   |
| LOOP_IN_POLL 0    ;             ;           ;           |
+-------------------+-------------+-----------+-----------+
| TX_WORK_IN_IRQ 0  ; tcp 9.4/9.1 ; tcp 88/88 ; tcp 95/94 |
| TX_WORK_IN_POLL 1 ; udp 5.5/5.5 ; udp 96/96 ; udp 96/96 |
| LOOP_IN_IRQ 0     ; cpu 98%     ; cpu 55%   ; cpu 19%   |
| LOOP_IN_POLL 0    ;             ;           ;           |
+-------------------+-------------+-----------+-----------+
| TX_WORK_IN_IRQ 0  ; tcp 9.0/8.7 ; tcp 87/87 ; tcp 95/94 |
| TX_WORK_IN_POLL 1 ; udp 5.8/5.8 ; udp 96/96 ; udp 96/96 |
| LOOP_IN_IRQ 0     ; cpu 98%     ; cpu 58%   ; cpu 20%   |
| LOOP_IN_POLL 1    ;             ;           ;           |
+-------------------+-------------+-----------+-----------+
| TX_WORK_IN_IRQ 1  ; tcp 7.3/7.3 ; tcp 87/86 ; tcp 94/94 |
| TX_WORK_IN_POLL 0 ; udp 6.2/6.2 ; udp 96/96 ; udp 96/96 |
| LOOP_IN_IRQ 1     ; cpu 99%     ; cpu 57%   ; cpu 17%   |
| LOOP_IN_POLL 0    ;             ;           ;           |
+-------------------+-------------+-----------+-----------+
| TX_WORK_IN_IRQ 1  ; tcp 6.5/6.5 ; tcp 88/88 ; tcp 94/94 |
| TX_WORK_IN_POLL 1 ; udp 6.1/6.1 ; udp 96/96 ; udp 96/96 |
| LOOP_IN_IRQ 1     ; cpu 99%     ; cpu 55%   ; cpu 16%   |
| LOOP_IN_POLL 1    ;             ;           ;           |
+-------------------+-------------+-----------+-----------+
| TX_WORK_IN_IRQ 1  ; tcp 5.7/5.7 ; tcp 87/87 ; tcp 95/94 |
| TX_WORK_IN_POLL 1 ; udp 6.1/6.1 ; udp 96/96 ; udp 96/96 |
| LOOP_IN_IRQ 1     ; cpu 98%     ; cpu 56%   ; cpu 15%   |
| LOOP_IN_POLL 0    ;             ;           ;           |
+-------------------+-------------+-----------+-----------+

Hopefully this helps to choose the most benefical approach.

Thank you,

Regards,
Nikolai

I think several variants can just be eliminated without looking at the numbers:

- doing the TX work in the irq handler (with the loop) but not in the poll function is incorrect with the edge interupts, as it has the same race as before, you just make it much harder to hit

- doing the tx work in both the irq handler and the poll function is probably not helpful, you just do extra work

- calling the tx cleanup loop in a second loop is not helpful if you don't do anything interesting after finding that all TX frames are done.

For best performance I would suggest restructuring the poll function from your current

  while (boguscnt--) {
       handle_rare_events();
       while (tx_pending())
             handle_one_tx();
  }
  while (rx_pending && work_done < budged)
         work_done += handle_one_rx();

to something like

   handle_rare_events();
   do {
      if (rx_pending())
          work_done += handle_one_rx();
      if (tx_pending())
          work_done += handle_one_tx();
   } while ((tx_pending || rx_pending) && work_done < budget)

This way, you can catch the most events in one poll function if new work comes in while you are processing the pending events.

Or, to keep the change simpler, keep the inner loop in the tx and rx processing, doing all rx events before moving on to processing all tx events, but then looping back to try both again, until either the budget runs out or no further events are pending.

Arnd

13.06.2021 1:41, Arnd Bergmann:

Ok, made a new version: https://pastebin.com/3FUUrg7C
It is much simpler and is very close to your patch now.

All previous conditional defines are eliminated along with unnecessary code fragments, and here is TUNE8139_BIG_LOOP to introduce a top-level loop in poll function as you suggested above. But apparently it works well both with and without this loop. At least my testing did not show any substantial difference in performance. Therefore I think it could be completely removed for the sake of simplicity.

One problem though is the kernel now always throws a traceback shortly after communication start:
https://pastebin.com/VhwQ8wsU
According to system.map it likely points to __local_bh_endble_ip() and there is one WARN_ON_ONCE() in it indeed, but I have no idea what it is and how to fix it.

Yet another thing is that tp->rx_lock and tp->lock are now used within poll function in a way that possibly suggests one of them could be eliminated.

Thank you,

Regards,
Nikolai

Ok, simpler is better in that case.

There must be some call to spin_unlock_bh() or local_bh_enabled() or similar, which is not allowed when interrupts are disabled with spin_lock_irqsave().

I don't see where exactly happens, but since nothing interesting is now executed in hardirq context, I assume you can change tp->lock from being called with _irq or _irqsave to using the _bh version that just blocks the poll and start_xmit functions from happening, not the hardirq.

Agreed. I didn't want to change too much in my proposal, but I'm sure these can be merged into a single lock, or possibly even eliminated entirely.

Arnd

It's an issue in x86 platform code, not the 8139too driver. Any option
card wired to PCI in this system somehow could suffer from it. Depending on how you look at it you may or may not qualify it as a bug though, and any solution can be considered a workaround (for a BIOS misfeature) rather than a bug fix.

The question is IMHO legitimate, and I can't speak for x86 platform
maintainers. If I were one, I'd accept a reasonable workaround and it does not appear to me it would be a very complex one to address this case:
basically a PCI quirk to set "this southbridge has ELCR at the usual location" (if indeed it does; you don't want to blindly poke at random port I/O locations in generic code), and then a tweak to pirq_enable_irq' or pcibios_lookup_irq' as Arnd has suggested.

Maciej

I think it would be good though to reinstate the driver workaround in some way, regardless of whether the x86 platform code gets changed or not.

From the old linux-2.6.2 code it appears that someone had intentionally added the loop as a hack to make it work on a broken or misconfigured BIOS. It's hard to know if that was indeed the intention, but it's clear that the driver change in 2.6.3 broke something that worked (most of the time) without fixing it in a better way.

(adding the x86 maintainers to Cc, the thread is archived at
https://lore.kernel.org/netdev/[email protected]/)

Changing the x86 platform code as well would clearly help avoid similar issues with other PCI cards on these broken platforms, but doing it correctly seems hard for a couple of reasons.

It sounds like it would have been a good idea 20 years ago when the broken i486 platforms were still fairly common, but now we don't even know whether the code was intentional or not. I don't remember a lot of the specifics of pre-APIC x86, but I do remember IRQ configuration as a common problem without a single good solution.

There is a realistic chance that other combinations of broken hardware and drivers rely on the x86 PCI code doing exactly what it does today. If overriding the BIOS setting breaks something that works today, nothing is gained, because the next person running into an i486 PCI specific bug is unlikely to be as persistent and competent as Nikolai in tracking down the root cause.

Doing a narrower change to a specific chipset, motherboard or BIOS would be less risky, but would likely miss similar cases. Any x86 specific change would clearly also miss similar problems that may or may not exist on other architectures.

Arnd

Why?

The usual fix the symptom not the root cause approach. So, no.

Correct. Any legacy PCI interrupt #A..#D must be level triggered. Edge is simply wrong for those. But of course that's wishful thinking. See below.

Vs. ELCR: it's documented to be 0x4d0, 0x4d1 and the kernel has it hard coded. So that's really the least of my worries.

Yes. It still is because even today BIOS tinkerers get it wrong.

Legacy PCI interrupts are specified as being level triggered because legacy PCI requires to share interrupts and you cannot share edge triggered interrupts reliably. PCI got a lot of things wrong, but this part is completely correct.

From experience I know that 8139 depends on that and I debugged that myself many years ago on an ARM system which had the trigger type configuration wrong.

Let me clarify why this breaks first. The interesting things to look at are:

- the device internal interrupt condition which in this case are package received or transmitted and status change

- the interrupt line

Let's look at the timing diagram of simple RX events:

           ___________________               _____...

RX ___| |_____________|

            ___________________               ____...

INTA ____| |_____________|

              _ INT handler _____ RETI _        __...

Kernel ______| |______|

Trivial case which works with both edge and level. Now let me add TX complete for the level triggered case:

           ___________________              

RX ___| |_____________

                             _______________

TX _____________________|

            ________________________________

INTA ____|

_ INT handle RX ___ RETI_ __ INT handle TX
Kernel ______| |_|

Works as expected. Now the same with edge mode on the i8259 side:

           ___________________              

RX ___| |_____________

                             _______________

TX _____________________|

            ________________________________

INTA ____|

_ INT handle RX ___ RETI_
Kernel ______| |_____ <- FAIL

Edge mode loses the TX interrupt when it arrives before the RX condition is cleared because INTA will not go low.

The driver cannot do anything about it reliably because all of this is asynchronous. The loop approach is a bandaid and kinda works, but can it work reliably? No. Aside of that it's curing the symptom which is the wrong approach.

The only reasonable solution to this is to enforce level even if that BIOS switch says otherwise.

Yes, sigh. The reason why this BIOS switch exists, which should have never existed, is that during the transition from EISA which was edge triggered to PCI some card manufacturers just changed the bus interface of their cards but completely missed the edge -> level change in hardware either by stupidity or intentionally so they did not have to make any changes to the rest of the hardware and to drivers.

The predominant OS'es at that time of course did not have an easy way to add a quirk to fix this, so the way out was to add the chicken bit option to the BIOS which then either tells the OS the trigger mode for INTA..INTD and just sets up ELCR before booting the OS. I have faint memories that I had to use such a BIOS switch long time ago to get some add-on PCI card working.

So the right thing to do is to leave 8139 as is and add a PCI quirk which enforces level trigger type for 8139 in legacy PCI interrupt mode.

Alternatively we can just emit a noisy warning when a legacy PCI interrupt is configured as edge by the BIOS and tell people to toggle that switch if stuff does not work. Though that might be futile because not all BIOSes have these toggle options.

Thanks,

tglx

Right, that is what I concluded as well.

I think the hack that Nikolai implemented based on my suggestion should work around this reliably though: All events (RX, TX, and others) are now masked in the hardirq handler but processed and acked inside the napi poll function. After it has processed all events, the device unmasks all events, so if a new event is already pending at this time, the IRQ line gets raised and will correctly be processed in both edge and level mode.

Am I missing something here?

Obviously it's just a hack and it's still going to fail if the edge interrupt is actually shared with another device, but I don't see any downside in changing the driver that way.

So you would prefer a quirk specific to this PCI device on misconfigured machines? That would at least completely avoid the risk of breaking setups that rely on an intentional misconfiguration, since it only changes setups that are definitely not working.

On the other hand, it wouldn't help with non-x86 machines that might have the same issue, or on x86 machines with a different PCI card in a misconfigured slot, unless there we maintain a list of PCI IDs that are known to not work with a broken BIOS configuration.

That sounds like a good idea regardless of any other outcome.

Arnd

Umm, I have been fairly sure it is actually EISA that has introduced
switching between edge- and level-triggered interrupts on a line-by-line basis for ISA option compatibility (previously ISA and MCA were globally edge-triggered and level-triggered respectively via a bit in ICW1 as per classic 8259A configuration modes) and at least Intel documentation[1][2] confirms that ELCR was already present in plain EISA chipsets, so that you could set IRQ lines used by ISA and EISA cards for edge-triggering and level-triggering respectively.

One peculiarity here is that level-triggered interrupts are necessarily
active-low to permit sharing, but ISA edge-triggered interrupts are active-high. Otherwise due to the oddity of how edge-triggered 8259A interrupts work we could actually safely reprogram all but the 8254 timer for level triggering and things would still work just fine (pretty much what MCA did with a hardware hack for the timer).

So while there may have been odd cases of ISA circuitry repurposed for
PCI (and then in a broken manner), they must have been exceedingly rare in the first place, and likely hardly any have survived, as their usability must have been marginal.

For EISA I reckon such repurposing wouldn't be an issue as the
manufacturer would simply require the interrupt line to be programmed by the EISA BIOS for the edge-triggered mode via the accompanying ECU configuration file for the piece of hardware in question. In fact some genuine ISA hardware, such as the 3Com 3c509b Ethernet card, did support configuring as EISA hardware.

I think it makes little sense really to add a quirk based on option card
hardware ID, as one could plug any PCI card into a slot whose interrupt line to be used has been incorrectly configured in the BIOS. So all PCI cards would have to have such a quirk defined.

As I noted in a previous message I think the only sensible approach is to
have the quirk based on the southbridge hardware ID, or in the absence of one -- the northbridge ID (I have now double-checked and it's the Aries chipset that does not make its southbridge component, the 82426EX ISA Bridge (IB), visible in the PCI configuration space).

If this system uses the IB or the 82378ZB SIO southbridge, then we can
handle it with full confidence however, as they have been documented, and they both have the ELCR register[3][4][5]. Note that the earlier version of the SIO southbridge, the 82378IB doesn't have the ELCR register[6][7], and it could only handle PCI interrupts with board-specific external logic (probably a design erratum/oversight, hence a later correction[8]).

The northbridge ID for Aries is 8086:0486 and the southbridge ID for SIO
is 8086:0484. The 82378IB and 82378ZB SIO revisions can be told apart by the configuration-space Revision Identification Register, bits 3:0: 0x0 for the 82378IB w/o ELCR, 0x3 for the 82378ZB with ELCR[9].

A warning surely won't hurt, but TBH I'd just reprogram any incorrectly
configured PCI interrupt line unconditionally where the ELCR is available. The chance someone uses a broken PCI card that drives its interrupt line as edge-triggered and is actually handled by a Linux driver both at a time is IMO nil. Such a card requires special provisions hardly any system provides and has anyone here seen a report of such a beast?

NB copies of documents referred available upon request, except for
290473-004, which I only have in the hardcopy form (though see
<https://archive.org/details/bitsavers_intelpentirocessorsandRelatedComponents_64170750>).

References:

[1] "82357 Integrated System Peripheral (ISP)", Intel Corporation, Order
Number: 290253-006, March, 1994, Section 5.14.7 "Edge and Level Triggered Modes", p. 44

[2] "Multiprocessor Specification", Version 1.1, Intel Corporation, Order
Number: 242016-003, September 1994, Section 5.3.2 "Level-triggered Interrupt Support", p. 5-7

[3] "82420EX PCIset Data Sheet, 82425EX PCI System Controller (PSC)

    and 82426EX ISA Bridge (IB)", Intel Corporation, Order Number: 
    290488-004, December 1995, Section 4.11.2 "Edge and Level Interrupt
    Triggered Mode", p. 111

[4] "82378 System I/O (SIO)", Intel Corporation, Order Number: 290473-004, December 1994, Section 5.8.1 "Edge and Level Triggered Modes"

[5] "82378ZB System I/O (SIO) and 82379AB System I/O APIC (SIO.A)", Intel Corporation, Order Number: 290571-001, March 1996, Section 4.8.1. "Edge and Level Triggered Modes", p. 101

[6] "82378IB System I/O (SIO)", Intel Corporation, Order Number:

    290473-002, April 1993, Section 5.8.7.7 "Edge and Level Triggered 
    Modes"

[7] "82378 System I/O (SIO)", Intel Corporation, Order Number: 290473-004, December 1994, Section 4.4.1 "ICW1--Initialization Command Word 1 Register"

[8] "82378IB to 82378ZB Errata Fix and Feature Enhancement Conversion

    FOL933002-01", 
    <https://web.archive.org/web/19990421045433/http://support.intel.com/support/chipsets/420/8511.htm>

[9] "82378 System I/O (SIO)", Intel Corporation, Order Number: 290473-004, December 1994, Section 4.1.5 "RID--Revision Identification Register"

Maciej

I looked some more through the git history and found at least one time that the per-chipset ELCR fixup came up for discussion[1], and this appears to have resulted in generalizing an ALI specific fixup into common code into common code[2], so we should already be doing exactly this in many cases. If Nikolai can boot the system with debugging enabled for arch/x86/pci/irq.c, we should be able to see exactly which code path is his in his case, and why it doesn't go through setting that register at the moment.

I also found an slightly more recent discussion, from where it seems that the authoritative decision when it came up in the past was that edge triggered interrupts are supposed to work as long as they are not shared [3][4].

Arnd

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://repo.or.cz/davej-history.git/commitdiff/e3576079d9e2#patch63
[3] https://yarchive.net/comp/linux/edge_triggered_interrupts.html
[4] https://lore.kernel.org/lkml/[email protected]/

Sadly Linus's rule applies both ways: if a device has been designed with
level-triggered interrupts in mind, there may be no race-free way to ensure an active-to-inactive-to-active transition has happened on its IRQ line as the driver acknowledges handling in the relevant device's CSR.

The rule of thumb is to acknowledge early in the handler, and to work
around broken configurations it may be desirable to also briefly mask all the interrupt sources with the device so as to make sure it deasserts its IRQ line even if another interrupt has already been queued. OTOH if IRQ sharing is to be supported a device absolutely has to have an interrupt mask register, as the system cannot rely on masking at the interrupt controller if multiple devices are to be handled with a single line. I suspect many of our drivers do not do such precautionary masking though.

Is there a mask register with the 8139?

NB I find it amazing how people can break such relatively simple
concepts, like not getting PCI IRQ steering right in early designs or messing up edge and level triggering. I mean wired interrupts are not rocket science. Sigh...

Maciej

Ah, it seems that the Cc list got dropped in a different sub-thread, see
https://pastebin.com/3FUUrg7C for a version of my patch that Nikolai tested successfully. This one goes further by completely masking all interrupts between the hardirq handler and the subsequent napi_complete_done(). This may not be the most efficient way of doing it, but it seems good enough according to his measurements, and it is a relatively safe and straightforward change.

Arnd

From: Maciej W. Rozycki

...

Typically you need to:
1) stop the chip driving IRQ low.
2) process all the completed RX and TX entries.
3) clear the chip's interrupt pending bits (often write to clear).
4) check for completed RX/TX entries, back to 2 if found.
5) enable driving IRQ.

The loop (4) is needed because of the timing window between
(2) and (3).
You can swap (2) and (3) over - but then you get an additional interrupt if packets arrive during processing - which is common.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

21.06.2021 14:22, Arnd Bergmann:
[...]

Here is my dmesg with debugging (hopefully) added to irq.c:

https://pastebin.com/tnC2rRDM

This is unmodified kernel, except for 8139too.c.
I've now realized maybe some usefull config option(s) related to pci irq handling are not enabled because due to size considerations it was initially configured as very minimalistic with just some really necessary features enabled (although ACPI, PCI_BIOS, PCI, EISA, ISA are enabled)

Just rechecked it again with the 8259A.pl script:

# 8259A.pl
irq 0: 00, edge
irq 1: 00, edge
irq 2: 00, edge
irq 3: 00, edge
irq 4: 00, edge
irq 5: 00, edge
irq 6: 00, edge
irq 7: 00, edge
irq 8: 00, edge
irq 9: 00, edge
irq 10: 00, edge
irq 11: 00, edge
irq 12: 00, edge
irq 13: 00, edge
irq 14: 00, edge
irq 15: 00, edge

Thank you,

Regards,
Nikolai

Ok, so it's getting roughly through the right code path:

        dev_dbg(&dev->dev, "PCI INT %c -> PIRQ %02x, mask %04x, excl %04x",
                'A' + pin - 1, pirq, mask, pirq_table->exclusive_irqs);
...
        dev_dbg(&dev->dev, "PCI INT %c -> newirq %d", 'A' + pin - 1, newirq);

        /* Check if it is hardcoded */
        if ((pirq & 0xf0) == 0xf0) {
                irq = pirq & 0xf;
                msg = "hardcoded";
        } else if (r->get && (irq = r->get(pirq_router_dev, dev, pirq)) && \
        ((!(pci_probe & PCI_USE_PIRQ_MASK)) || ((1 << irq) & mask))) {
                msg = "found";
                elcr_set_level_irq(irq);
        } else if (newirq && r->set &&
                (dev->class >> 8) != PCI_CLASS_DISPLAY_VGA) {
                if (r->set(pirq_router_dev, dev, pirq, newirq)) {
                        elcr_set_level_irq(newirq);
                        msg = "assigned";
                        irq = newirq;
                }
        }

        if (!irq) {
                if (newirq && mask == (1 << newirq)) {
                        msg = "guessed";
                        irq = newirq;
                } else {
                        dev_dbg(&dev->dev, "can't route interrupt\n");
                        return 0;
                }
        }
        dev_info(&dev->dev, "%s PCI INT %c -> IRQ %d\n", msg, 'A' +

pin - 1, irq);

with the corresponding output from that one being

[    0.761546] 8139too 0000:00:0d.0: runtime IRQ mapping not provided by arch
[    0.761546] 8139too: 8139too Fast Ethernet driver 0.9.28
[    0.761546] 8139too 0000:00:0d.0: PCI INT A -> PIRQ 02, mask 1eb8, excl 0001
[    0.765817] 8139too 0000:00:0d.0: PCI INT A -> newirq 9
[    0.765817] 8139too 0000:00:0d.0: can't route interrupt
[    0.777546] 8139too 0000:00:0d.0 eth0: RealTek RTL8139 at

0xc4804000, 00:11:6b:32:85:74, IRQ 9

From what I can tell, this means that there is no chipset specific get/set function,
the irq is not hardcoded but the 'mask' value in the irq_info table (0x1eb8) lists a number of options.

In this case, the function gives up with the "can't route interrupt\n" output and does not call elcr_set_level_irq(newirq). Adding the call to elcr_set_level_irq() in that code path as well probably makes it set the irq to level mode:

@@ -984,6 +984,7 @@ static int pcibios_lookup_irq(struct pci_dev *dev,

int assign)

                        irq = newirq;
                } else {
                        dev_dbg(&dev->dev, "can't route interrupt\n");
+                       elcr_set_level_irq(newirq);
                        return 0;
                }
        }

No idea if doing this is a good idea though, in particular I have no clue about whether this is a common scenario or not.

Arnd

22.06.2021 16:22, Arnd Bergmann:
[...]

Yes, it does indeed:

[    0.765886] 8139too 0000:00:0d.0: can't route interrupt
[    0.765886] PCI: setting IRQ 9 as level-triggered
[    0.781734] 8139too 0000:00:0d.0 eth0: RealTek RTL8139 at 0xc4804000, 00:11:6b:32:85:74, IRQ 9

And also here:

# 8259A.pl
irq 0: 00, edge
irq 1: 00, edge
irq 2: 00, edge
irq 3: 00, edge
irq 4: 00, edge
irq 5: 00, edge
irq 6: 00, edge
irq 7: 00, edge
irq 8: 02, edge
irq 9: 02, level
irq 10: 02, edge
irq 11: 02, edge
irq 12: 02, edge
irq 13: 02, edge
irq 14: 02, edge
irq 15: 02, edge

Now connection also works fine with unmodified 8139too driver. The percentage of low-level errors stays very small:

RX packets:13953 errors:1 dropped:2 overruns:1 frame:0 TX packets:37346 errors:0 dropped:0 overruns:13 carrier:0

This fix looks really nice. Maybe it is right thing to do.

Thank you,

Regards,
Nikolai

Ok, nice!

I'll leave that up to Thomas and Maciej to decide, they should have the best idea of why the x86 pci-irq code looks the way it does today and what the possible risk with my patch is.

As I said before, I still think we should also merge the 8139 driver patch, probably without that loop. It's not great, but I'm much more confident I understand what that does and that the patched version is better than the current code.

Arnd

Ah, so this is the SiS 85C496/497 chipset; another one that does not have
its southbridge visible in the PCI configuration space, perhaps because it doesn't put the southbridge on PCI in the first place, and instead it maps its configuration registers in the upper half of the northbridge's space. Oh, the joys of early attempts!

It does PCI interrupt steering, it has the ELCR, but we don't have a PIRQ
router implemented for it. I have a datasheet, so this should be fairly trivial to do, and hopefully things will then work automagically, no need for hacks.

It's very late tonight here, so let me come back with something tomorrow.

Maciej

Hi Maciej,

23.06.2021 4:04, Maciej W. Rozycki:
[...]

I see your patches for this chipset have now arrived and I'll test them as soon as I get to that box. Meanwhile I've captured a similar log from another (and actually this one is my main concern because it can not be replaced easily):

https://pastebin.com/NVfRcMww

Something is clearly different here, 8259A.pl reports all irqs are edge no matter what. BIOS setup only offers some strange "PCI IDE IRQ mode" (Edge/Level) and apparently it has no effect anyway. (A modified version of 8139too works fine though)

Thank you,

Regards,
Nikolai

Hi Nikolai,

Such an option was common in BIOS configuration of the time, due to odd
arrangements with early PCI ATA interfaces, such as being equipped with an ISA stub board with a connecting cable to route IRQ14/15 from an ISA slot.

Anyway, this the ALi M1489/M1487 chipset and no datasheet is easily
available (perhaps Nvidia, which bought the chipset part of ALi back in 2006, has a copy, but I wouldn't bet on it). A product brief is available that says the chipset supports PCI steering, but we can only guess how it works.

Please try the debug patch I sent with the previous message and maybe
it'll dump the PIRQ router, which may or may not help. Choosing the CONFIG_PCI_GOBIOS option may also reveal something along with said patch.

Maciej

I have actually tracked a datasheet down now, so I'll see what I can do.
The southbridge has a separate edge/level trigger mode control register for PIRQ lines in addition to the usual ELCR register, so it will require extra handling and the wording is not as clear as with Intel documentation (understandably), so it may require further experimentation.

Maciej

Hello Maciej,

15.07.2021 2:32, Maciej W. Rozycki:

Thank you very much for investigating this! Tomorrow I'll likely be able to pull this motherboard out and arrange it for whatever testing might be necessary. However I'll be in a trip for a couple of weeks soon so please excuse some possible delay in my replies. I'm definitely willing to make all efforts to get it supported.

Regards,
Nikolai

Hello Nikolai,

I have preliminary code ready for submission, so I only need to wrap it
up for posting. However it requires some additional infrastructure shared with other parts of the kernel, so it will be a small patch series again, and due to code dependencies I'll include support for another PIRQ router as well as some clean-ups, because they all need to be applied in order. With that in place we should have complete coverage for Intel chipsets.

Moreover based on the ALi M1533 datasheet I have also tracked down I have
noticed we have bugs in the handling for the other ALi chipsets, which I seem unable to satisfactorily resolve without a copy of the ALi M1563 datasheet I don't have. I've noticed Nvidia does continue having at least some ALi-related stuff online, so chances are they have retained the ALi legacy internally. I have therefore requested a copy of the M1563 datasheet and will likely extend the patch series accordingly, whether I do or do not receive the document.

Stay tuned.

Maciej

22.06.2021 22:26, Arnd Bergmann:
[...]

Yes, the 'poll' approach apparently works stable and does not cause any measurable performance decrease. But it would need some carefull cleanup/review, especially WRT locking. Now that all real event handling work is happening in the poll function, it still has to be protected against the (potentially also long-running) reset function which in current design can be called e.g. from a different thread due to tx timeout, and this does not look good, but it is a bit beyond my capability to arrange it better. Besides, the idea was to keep the fix simple and avoid a massive rework...

Thank you,

Regards,
Nikolai

The simplest fix is not always the right one.

I've now posted a small patch series that adds a PCI IRQ router for the
SiS85C497 device. Would you please try it with your system along with the debug patch included below and send me the resulting bootstap log?

Maciej

---
 arch/x86/include/asm/pci_x86.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-macro-ide/arch/x86/include/asm/pci_x86.h
===================================================================
@@ -7,7 +7,7 @@
 
 #include <linux/ioport.h>
 
-#undef DEBUG
+#define DEBUG 1
 
 #ifdef DEBUG
 #define DBG(fmt, ...) printk(fmt, ##__VA_ARGS__)

Right, I forgot you saw that one WARN_ON trigger. If you enable KALLSYMS and BUGVERBOSE, it should become obvious what that one is about.

Since the calls are just moved from hardirq to softirq context, it should be possible to do this without changing the locking state of any bit, by just making sure that irqs are disabled during the code that was part of the hardirq handler.

If you remove the loop, you can move the spin_lock_irqsave(&tp->lock) back down after the rx handler.

Anything on top of that can be done as a cleanup or simplification.

Sure, but in this case, a fairly simple change can help make this driver work on any machine that for whatever reason treats the IRQ as edge triggered.

Thanks a lot, that should help avoid the same problem on this chipset with other drivers.

Arnd

24.06.2021 11:28, Arnd Bergmann:

Here is new log with a better backtrace:

https://pastebin.com/DP3dSE4w

The respective diff against regular 8139too.c is here:

https://pastebin.com/v8kA7ZmX

(Basically the same as you proposed initially, just slight difference might be as a remainder of my various previous testing)

Thank you,

Regards,
Nikolai

Ok, I think the generic code changed a bit, but it does confirm that the problem is doing the RX handing with IRQs disabled.

The simplest workaround would be to just move the "spin_lock_irqsave(&tp->lock, flags);" a few lines down, below the rx processing. This keeps the locking rules exactly how they were before your patch, and anything beyond that could be done as a follow-up cleanup, if someone wants to think this through more.

Arnd

Hello Arnd,

03.07.2021 12:10, Arnd Bergmann:

Indeed, moving spin_lock_irqsave below rtl8139_rx eliminated the warn_on message apparently, here is a new log:

https://pastebin.com/dVFNVEu4

and here is my resulting diff:

https://pastebin.com/CzNzsUPu

My usual tests run fine. However I still see 2 issues:

1. I do not understand all this locking thing enough to do a good cleanup myself, and it looks like it needs one;
2. It looks like in case of incorrect (edge) triggering mode, the "poll approach" with no loop added in the poll function would still allow a race window, as explained in following outline (from some previous mails):

22.06.2021 14:12, David Laight:

So in terms of such outline, the "poll approach" now implements 1, 2, 3, 5 but still misses 4, and my understanding is that it is therefore still not a complete solution for the broken triggering case (Although practically, the time window might be too small for the race effect to be ever observable) From my previous testing I know that such a loop does not affect the perfomance too much anyway, so it seems quite safe to add it. Maybe I've missunderstood something though.

Thank you,

Regards,
Nikolai

Ok, great, the patch looks good to me, and I think we should just merge that, in addition to Maciej's bugfix for the platform.

A lot of drivers need one. With your latest patch, I'm confident enough that it's not getting worse here and given the age of this device I don't think the cleanup is required. It's probably possible to squeeze out a little more performance from this device on SMP systems by improving it, but this would still be hopeless to compete against a $5 gigabit ethernet card.

The latest version of your patch already does what David explained as the alternative: the 'ack' (step 3) happens before processing the interrupts (2), so you don't need step 4 for correctness. You had that in the previous version of the patch that had the loop, and since you have experimentally shown that it makes no significant difference to performance, I'd rather leave it out for simplicity.

If another event becomes pending after the Ack but before the napi_complete_done(), then we get an interrupt and call napi_schedule() again.

For some reason, the TxErr bit is only cleared after tx processing, so we could miss an error event, but that seems fine as the tx errors are not handled in any way other than counting them, which is already unreliable if multiple transmits fail before the interrupt comes.

Arnd

From: Nikolai Zhubr

...

The timing window (4) happens.
The next receive packet will usually clear it - but that might require a timeout. I'm sure I got a trip to Sweden out of it.

I also think Linux requires the tx-reap be done in a timely manner. If that is only done in response to an end of tx interrupt the delay could be substantial.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Umm, any 486 system surely predates ACPI by several years; indeed I have
a dual Pentium MMX system from 1997 that does not have ACPI either and uses an MP Table to communicate interrupt line configuration. I am fairly sure an old UP 486 system communicates nothing.

Since the inception of EISA the usual way for x86 systems to switch
between classic edge-triggered and level-triggered interrupts on a line by line basis has been the ELCR, a 16-bit mainboard register at 0x4d0 in the port I/O space. We do have support for that register, however used in certain circumstances only, clearly not yours. Classic 8259A chips could only switch trigger modes globally across all interrupt lines.

You might be able to add a quirk based on your chipset's vendor/device ID
though, which would call `elcr_set_level_irq' for interrupt lines claimed by PCI devices. You'd have to match on the southbridge's ID I imagine, if any (ISTR at least one Intel chipset did not have a southbridge visible on PCI), as it's where the 8259A cores along with any ELCR reside.

It would be the right thing to do IMO; those early PCI systems often got
these things wrong, and the selection for the trigger mode shouldn't have been there in the BIOS setup in the first place (the manufacturer couldn't obviously figure it out how to do this correctly by just scanning PCI, so they shifted the burden onto the end user; though I have to admit odd hw used to be made too, e.g. I remember seeing a PCI PATA option card with an extra cable and a tiny PCB stub to be plugged into the upper part of a spare ISA slot to get IRQ 14/15 routed from there).

HTH,

Maciej

Hi all,

01.06.2021 20:44, Maciej W. Rozycki:
[...]

I'm looking at this comment in arch/x86/kernel/acpi/boot.c:

	/*
	 * Make sure all (legacy) PCI IRQs are set as level-triggered.
	 */

Doesn't it target exactly the case in question? If so, why it does not actually work?

By legacy they likely mean non-ACPI IRQs, so for 486 it's just all of them. So I'd suppose, if the kernel readily knows a particular IRQ is assigned to PCI bus (I'm almost sure it does) shouldn't it already take care of proper triggering mode automatically? Because then there would be no need to add workarounds to individual drivers.

Thank you,

Regards,
Nikolai

I think this means non-MSI interrupts

Without ACPI, this code is never called, instead of acpi_pci_irq_enable(), you use pirq_enable_irq()/pcibios_lookup_irq(). There are already some hardware specific quirks in there, it's possible this could be changed to address your case as well, but it already looks a bit fragile.

Arnd

Issue could be related to rx and tx processing now potentially running in parallel. I only have access to the current 8139too source code, hopefully the following works on the old version:

In the end of rtl8139_start_xmit() there's
if ((tp->cur_tx - NUM_TX_DESC) == tp->dirty_tx)

		netif_stop_queue (dev);

Try changing this to

if (tp->cur_tx - NUM_TX_DESC == tp->dirty_tx) {
	smp_wmb();
	netif_stop_queue (dev);
	smp_mb__after_atomic();       /* if this doesn't exist yet, use mb() */
	if (tp->cur_tx - NUM_TX_DESC != tp->dirty_tx)
		netif_start_queue(dev);
}

And at the end of rtl8139_tx_interrupt() change

	if (tp->dirty_tx != dirty_tx) {
		tp->dirty_tx = dirty_tx;
		mb();
		netif_wake_queue (dev);
	}

to

	if (tp->dirty_tx != dirty_tx) {
		tp->dirty_tx = dirty_tx;
		mb();
		if (netif_queue_stopped(dev) && tp->cur_tx - NUM_TX_DESC != tp->dirty_tx)
			netif_wake_queue (dev);
	}

What's your qdisc? Recently there was a bug related to the lockless pfifo_fast qdisc