net: macb: manage Rx BNA error to avoid race condition

[yseraf]

In case of BNA error (Buffer Not Available), the GMAC sets the BNA bit and
triggers a RCOMP interrupt. At the next end of frame received, if the BNA
condition is not corrected, only the BNA bit is set but no more RCOMP
interrupts are generated. A RCOMP interrupt is guaranteed if the DMA queue
base address is written while Rx is disabled fixing the race condition.

Signed-off-by: Yannick Lanz [email protected]

EDIT: The commit has changed since this original one.

[yseraf]

On the SAMA5D4, after some time (weeks to months), the netif becomes unusable. When the problem appears, there is no more IRQ from the MACB periph nor NET_RX softIRQ but the rx_resource_errors counter is incrementing on each packet.

Following the SAMA5D4's datasheet, this error counter is incrementing when a BNA condition is set.
What is also specified in the datasheet is that a BNA condition generates a RCOMP interrupt with the BNA bit set in the RSR register. What is not explained is (confirmed by our tests) that a second BNA condition without having moved the Rx ring queue pointer (if condition has not been cleared) doesn't generate an interrupt.

In some condition, the BNA bit is cleared but not the condition and the polling is letting place to the IRQ. In this case, there is no way to receive packets anymore (no more IRQ nor softIRQ).

[yseraf]

Also we don't know if it the same for the SAMA5D2/3 or the other CPUs using this peripheral. In all cases, using a high intensity traffic generator (like PacketSender) on the SAMA5D4 makes the netif unusable after some minutes.
To finish, clearing completely the Rx ring in case of error is not elegant (and we can lost up to 512 packets) but that's an easy way to remove the race condition and our other attempts has all ended with race conditions.
We would ideally need more information regarding the internal behavior of the MACB to improve the fix.

[yseraf]

The patch has been modified to free only a slot (drop the packet) and remove the BNA condition without dropping the whole Rx ring.
Also, since the interrupts are disabled and not masked, we need to test reception and BNA condition after having enabled again the Rx IRQ and leaving the polling mode otherwise, we will let stale a incoming packet until the next reception (not really a problem) or enter a deadlock in case of BNA condition (no more polling, no more interrupt).

The netif can supports 30Mbps of UDP traffic (not tested more) with this patch without any issue.

[yseraf]

Anybody there ? Am I supposed to submit my patch somewhere else ?

[claudiubeznea]

Hi @hilt0n! What is the packet size you are using? I guess that you are sending traffic at line rate. Did you try to add MACB_CAPS_NEEDS_RSTONUBR as capability for sama5d4_config? See:

static const struct macb_config sama5d4_config = {
.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_NEEDS_RSTONUBR,
.dma_burst_length = 4,
.clk_init = macb_clk_init,
.init = macb_init,
.usrio = &macb_default_usrio,
};

If yes, is the behavior the same? Is the performance lower than what you achieve with this patch?
Could you please share the performance results before your chances on the kernel you are using?

Thank you,
Claudiu

[yseraf]

Hi @claudiubeznea,

Thank you for the support. You are right regarding packet count, 30Mbit/s doesn't mean nothing.
I send 30k packets/second in UDP (packet size of about 20-30 Bytes).

I just tested MACB_CAPS_NEEDS_RSTONUBR flag and it doesn't fix the issue.
Following my tests and memories, the BNA error doesn't set the flag UBR but generate RCOMP interrupt with the BNA flag set (at least on the SAMA5D4).

The datasheet seems to be a lot of copy/paste from other CPU with previous (maybe?) version of the peripheral.
The support of Microchip didn't help since they just gave a brief of the datasheet.

Regarding performances, I tested with iperf3 with base options:

Base

Client -> Server

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   111 MBytes  93.0 Mbits/sec   48             sender
[  5]   0.00-10.00  sec   110 MBytes  92.6 Mbits/sec                  receiver

Server -> Client

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.01  sec   112 MBytes  93.5 Mbits/sec   85             sender
[  5]   0.00-10.01  sec   111 MBytes  93.0 Mbits/sec                  receiver

Fix

Client -> Server

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.01  sec   111 MBytes  93.1 Mbits/sec   88             sender
[  5]   0.00-10.01  sec   111 MBytes  92.7 Mbits/sec                  receiver

Server -> Client

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.01  sec   113 MBytes  94.4 Mbits/sec   20             sender
[  5]   0.00-10.01  sec   112 MBytes  93.9 Mbits/sec                  receiver

The results are really similar, do you have any proposal regarding more pertinent parameters for the tests ?

EDIT: The kernel is the 4.19.78 (linux-at91) and updated results

[claudiubeznea]

Please contact the authors of f027418. According to commit tags they are:
Co-developed-by: Scott McNutt [email protected]
Signed-off-by: Scott McNutt [email protected]
Signed-off-by: Robert Hancock [email protected]

This commit is integrated in Linux mainline and we cherry picked it with all the mainline contribution tags and authorship. So, what is not mentioned here? You patch is not integrated here thus it is left behind.

If you have questions about why you were not mentioned please contact the original authors and contributors of f027418.

Note that it is also possible that other people were facing the same issue that you encountered here and published it to mainline (you didn't do this, you only published it here, this is not Linux mainline is vendor tree). I see their solution is more simple and elegant and significant differs (at least in lines of code) from your solution. Anyway, if you have authorship questions please contact them. They are the authors.

And before accusing:
"You visibly don't really understand how OSS work... When you use the work of someone else, you have to mention it otherwise, this is stolen...
My work on this patch:f027418"
please teach your self about how mainline Linux contributions work. The commit you mentioned is a commit integrated in Linux mainline that we cherry picked from mainline with all the mainline contribution. It is the way Linux stable cherry picking works (except probably we didn't mentioned here the original commit ID from Torvalds' tree). Note that patch " net: macb: Fix lost RX packet wakeup race in NAPI receive " has author and the rest of tags as they are in public Linux kernel tree.

Have a good day!

[yseraf]

You are right, my apologies. I over reacted that's why I deleted my post.

I think I was more annoyed by atmel/microchip not taking this patch (which makes the ethernet driver just working) into account.
Happy to see that sama5 has finally a working ethernet driver integrated into the mainline.

Regarding source of bug patch proposal, I'm a bit surprised this is not an ascending processus. I though it was more a down to up approach like silicon manufacturers (like mchp) pushing bug fixes to arm mainline pushing then to kernel mainline.
Not sure for the next time how to do it...