[PATCH net v4] net: macb: rate limit netdev error info print in the data path

[PATCH net v4] net: macb: rate limit netdev error info print in the data path

[taozj888]

From: taozijin <taozj888@163.com>

Now the MACB ethernet driver print the netdev error information
directly by netdev_err(), which would lead to a large number of
error information print if there was a significant number of
error or just jumbo packets exceeding the MTU received when booting.
For example, it would print a large number of:

macb PHYT0036:00 eth0: not whole frame pointed by descriptor
macb PHYT0036:00 eth0: not whole frame pointed by descriptor
...

in gem_rx() by received a large number of packets without
RX_EOF flag set, especially with unknown packet
type that would penetrate the hardware offload for the IP packets.

The unlimited prints here would greatly bother and delay
the system booting process unless the source stop sending
packets since they occupy the console output bandwidth and
other processes have to wait for the completion of printing those
messages.

So rate limit the netdev error information print in the receive
and transmit data path.

Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
Cc: stable@vger.kernel.org

Signed-off-by: Zijin Tao <taozj888@163.com>
---
Changes since v1:

v4:

- Update the commit message for the target fix id to 89e5785fc8a6
as suggested by Nicolai Buchwitz<nb@tipi-net.de>
- Update the patch with the changelog added in the patch itself

v3:

- add this change log since v2 missing it
- drop the blank line between the assignment sentence and the commit
  info

v2:

- update the commit message with the reason why the unlimit prints would
  bother/delay booting process
- update the commit message with the Fix commit id provided which is
  suggested by Théo Lebrun<theo.lebrun@bootlin.com>
- update the commit message with what packet type would trigger the
  error printings more easily.

v1:

- initial commit

 drivers/net/ethernet/cadence/macb_main.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8e5c034dc3a4..ca60960bec36 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1617,16 +1617,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
 		count++;
 
 		if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF))) {
-			netdev_err(bp->netdev,
-				   "not whole frame pointed by descriptor\n");
+			if (net_ratelimit())
+				netdev_err(bp->netdev, "not whole frame pointed by descriptor\n");
 			bp->netdev->stats.rx_dropped++;
 			queue->stats.rx_dropped++;
 			break;
 		}
 		skb = queue->rx_skbuff[entry];
 		if (unlikely(!skb)) {
-			netdev_err(bp->netdev,
-				   "inconsistent Rx descriptor chain\n");
+			if (net_ratelimit())
+				netdev_err(bp->netdev, "inconsistent Rx descriptor chain\n");
 			bp->netdev->stats.rx_dropped++;
 			queue->stats.rx_dropped++;
 			break;
@@ -1829,7 +1829,8 @@ static int macb_rx(struct macb_queue *queue, struct napi_struct *napi,
 		unsigned long flags;
 		u32 ctrl;
 
-		netdev_err(bp->netdev, "RX queue corruption: reset it\n");
+		if (net_ratelimit())
+			netdev_err(bp->netdev, "RX queue corruption: reset it\n");
 
 		spin_lock_irqsave(&bp->lock, flags);
 
@@ -2102,7 +2103,8 @@ static int macb_interrupt_misc(struct macb_queue *queue, u32 status)
 
 	if (status & MACB_BIT(HRESP)) {
 		queue_work(system_bh_wq, &bp->hresp_err_bh_work);
-		netdev_err(netdev, "DMA bus error: HRESP not OK\n");
+		if (net_ratelimit())
+			netdev_err(netdev, "DMA bus error: HRESP not OK\n");
 		macb_queue_isr_clear(bp, queue, MACB_BIT(HRESP));
 	}
 
-- 
2.34.1

Re:Re: [PATCH net v4] net: macb: rate limit netdev error info print in the data path

[taozj888]

Hi Theo:


Please see my comments inline.


Thanks,


Zijin Tao





At 2026-10-07 17:33:03, "Théo Lebrun" <theo.lebrun@bootlin.com> wrote:
>Hello taozj888,
>
>I was reviewing your patch but you sent V4 at the same time!
>
>On Wed Oct 7, 2026 at 7:32 AM CEST,  wrote:
>> From: taozijin <taozj888@163.com>
>
>We need full names in kernel patches, you have that in your
>Signed-off-by.
>
>> Now the MACB ethernet driver print the netdev error information
>> directly by netdev_err(), which would lead to a large number of
>> error information print if there was a significant number of
>> error or just jumbo packets exceeding the MTU received when booting.
>> For example, it would print a large number of:
>>
>> macb PHYT0036:00 eth0: not whole frame pointed by descriptor
>> macb PHYT0036:00 eth0: not whole frame pointed by descriptor
>> ...
>>
>> in gem_rx() by received a large number of packets without
>> RX_EOF flag set, especially with unknown packet
>> type that would penetrate the hardware offload for the IP packets.
>>
>> The unlimited prints here would greatly bother and delay
>> the system booting process unless the source stop sending
>> packets since they occupy the console output bandwidth and
>> other processes have to wait for the completion of printing those
>> messages.
>>
>> So rate limit the netdev error information print in the receive
>> and transmit data path.
>>
>> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
>> Cc: stable@vger.kernel.org
>
>What happened? This doesn't match your discussion with Nicolai.

>https://lore.kernel.org/netdev/3e1515e162d084e2200eeaa6074bfbaf@tipi-net.de/


@Theo, in the link that you provided, Nicolai suggested to use the commit id 
"4df95131ea80", and the original text is quoted below:
"

>>>> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
>>> 
>>> IMHO the correct tag is 4df95131ea80 ("net/macb: change RX path for
>>> GEM")?
>>> At least the gem_rx() messages were introduced here.
>> 
>> The patch used to touch macb_start_xmit(), which explains why I told
>> Zijin to target the initial commit on previous revision.
>> 
> OK, I will still use the initial commit indicated by Theo.

Please re-read Théo's reply: he only suggested the initial
because v1 also touched macb_start_xmit(). v2 doesn't anymore,
89e5785fc8a6 is no longer the right target. Please use:

Fixes: 4df95131ea80 ("net/macb: change RX path for GEM")
"
So I changed the commit id from "89e5785fc8a6" to "4df95131ea80"
in the v4 patch, and somebody suggested to set the changelog just
in the patch below its "---" part if there is only just 1 patch, but
not in the cover-letter, so I added the changlog in the v4 patch.


If you didn't agree the changes in the v4 patch I would like to just abandon
it, or I will send a v5 patch to restore to the contents of the v3 patch with
the changelog added in the patch itself.


For the autorship, I would like to use the "Zijin Tao <taozj888@163.com>" for all
parts in the patch (for the signed-off-by and From).

>
>TLDR, because those threads are a mess:
> - why drop start_xmit diff?
> - fix your patch autorship
> - check your changelog!
> - check your fixes!
>
>Thanks,
>
>--
>Théo Lebrun, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com

Re: [PATCH net v4] net: macb: rate limit netdev error info print in the data path

[Théo Lebrun]

Hello Zijin,

 - I'll stop replying inline because you seem to be having issues with
   your mailer. Please fix it to ease the discussion.

   See here how your email looks:
   https://lore.kernel.org/netdev/63731290.3f0c.1a1198fe993.Coremail.taozj888@163.com/

 - Your V4 commit message says "Fixes: 89e5785fc8a6" but that is wrong
   with the current patch, it shold use 4df95131ea80. I don't
   understand why you are talking about the changelog at the same time,
   this is unrelated.

 - You skipped over explaining why your dropped the start_xmit diff.

I'll have to stop reviewing if you skip over parts of the messages.
I gave you a TLDR and you skipped over the first bulletpoint.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net v4] net: macb: rate limit netdev error info print in the data path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.
- [Low] macb: mismatch between commit message and implementation for TX
  rate limiting

--

net: macb: rate limit netdev error info print in the data path

This commit wraps the netdev_err() calls in gem_rx(), macb_rx() and
macb_interrupt_misc() with net_ratelimit(). The goal is to stop a flood of
bad RX descriptors from filling the console and delaying boot. It is tagged
for stable as a fix for the original Atmel MACB driver.

> So rate limit the netdev error information print in the receive
> and transmit data path.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 8e5c034dc3a48..ca60960bec362 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1617,16 +1617,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  		count++;
>  
>  		if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF))) {
> -			netdev_err(bp->netdev,
> -				   "not whole frame pointed by descriptor\n");
> +			if (net_ratelimit())
> +				netdev_err(bp->netdev, "not whole frame pointed by descriptor\n");
>  			bp->netdev->stats.rx_dropped++;
>  			queue->stats.rx_dropped++;
>  			break;
>  		}
>  		skb = queue->rx_skbuff[entry];
>  		if (unlikely(!skb)) {
> -			netdev_err(bp->netdev,
> -				   "inconsistent Rx descriptor chain\n");
> +			if (net_ratelimit())
> +				netdev_err(bp->netdev, "inconsistent Rx descriptor chain\n");
>  			bp->netdev->stats.rx_dropped++;
>  			queue->stats.rx_dropped++;
>  			break;

[Severity: High]
The commit message says prints in both the receive and transmit data
paths are rate limited, but this patch doesn't change any TX path. Are
there other per-packet or per-interrupt netdev_err() calls in
macb_main.c that can still flood the console in the same way?

An empty rx_skbuff[] entry usually means a refill failed. gem_rx() calls
gem_rx_refill() at the end of every NAPI poll, and the print there is
still unconditional:

gem_rx_refill() {
    ...
			skb = netdev_alloc_skb(bp->netdev, bp->rx_buffer_size);
			if (unlikely(!skb)) {
				netdev_err(bp->netdev,
					   "Unable to allocate sk_buff\n");
				break;
			}
    ...
}

Under memory pressure, won't this print on every poll of every queue?

On the TX side, macb_start_xmit() returns NETDEV_TX_BUSY after a print
that has no rate limit:

macb_start_xmit() {
    ...
		if (skb_headlen(skb) < hdrlen) {
			netdev_err(bp->netdev, "Error - LSO headers fragmented!!!\n");
			/* if this is required, would need to copy to single buffer */
			return NETDEV_TX_BUSY;
		}
    ...
}

The qdisc requeues the same skb and tries again. Can this message keep
repeating for as long as that skb stays at the head of the queue?

macb_tx_map() also prints once for every packet whose DMA mapping fails:

macb_tx_map() {
    ...
dma_error:
	netdev_err(bp->netdev, "TX DMA map failed\n");
    ...
}

at91ether_start_xmit() has the same pattern:

			netdev_err(netdev, "%s: DMA mapping error\n", __func__);
			return NETDEV_TX_OK;

at91ether_interrupt() prints from hardirq context on every RX overrun:

	if (intstatus & MACB_BIT(ISR_ROVR))
		netdev_err(netdev, "ROVR error\n");

ISR_ROVR is enabled in IER for at91ether. The driver has only
AT91ETHER_MAX_RX_DESCR (9) RX descriptors and handles RX inside the IRQ
handler. Could a remote sender flooding the link trigger this print in
the way the commit message describes for gem_rx()?

For comparison, the ISR_ROVR handling in macb_interrupt_misc() only
increments a counter.

Since this is going to stable as a fix for console flooding, should
these sites be rate limited as well?

[ ... ]

> @@ -2102,7 +2103,8 @@ static int macb_interrupt_misc(struct macb_queue *queue, u32 status)
>  
>  	if (status & MACB_BIT(HRESP)) {
>  		queue_work(system_bh_wq, &bp->hresp_err_bh_work);
> -		netdev_err(netdev, "DMA bus error: HRESP not OK\n");
> +		if (net_ratelimit())
> +			netdev_err(netdev, "DMA bus error: HRESP not OK\n");
>  		macb_queue_isr_clear(bp, queue, MACB_BIT(HRESP));
>  	}
>  

[Severity: Low]
The commit message says this rate limits the error prints in the
receive and transmit data paths, but none of the sites changed here
are in the TX path. gem_rx() and macb_rx() are RX. The HRESP print in
macb_interrupt_misc() reports a DMA bus error from the misc interrupt
handler.

The TX side still has unconditional prints that can fire once per
packet. macb_tx_map() prints "TX DMA map failed" every time a mapping
fails. macb_start_xmit() prints
"Error - LSO headers fragmented!!!" before returning NETDEV_TX_BUSY,
so the qdisc requeues the same skb and hits the print again on the
next attempt. This patch does not touch either site.

Should these TX sites be rate limited as well? Or should the commit
message be reworded so it only claims the RX and interrupt paths?
Since this is tagged for stable, the description needs to match what
the backport actually changes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007053211.572073-1-taozj888%40163.com