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

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

[Zijin Tao]

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: 4df95131ea80 ("net/macb: change RX path for GEM")
Cc: stable@vger.kernel.org
Signed-off-by: Zijin Tao <taozj888@163.com>
---

Changes since v1:

v5:

- Update the commit message for the target fix id to 4df95131ea80
which was indicated by Theo <theo.lebrun@bootlin.com> and Nicolai
<nb@tipi-net.de>
- Update the user name in the "From" part to Zijin Tao which is
the same as that of "Signed-off-by"

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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b8234ac4b602..c32d48d03008 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));
 	}
 
@@ -2511,7 +2513,8 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 		else
 			hdrlen = skb_tcp_all_headers(skb);
 		if (skb_headlen(skb) < hdrlen) {
-			netdev_err(bp->netdev, "Error - LSO headers fragmented!!!\n");
+			if (net_ratelimit())
+				netdev_err(bp->netdev, "Error - LSO headers fragmented!!!\n");
 			/* if this is required, would need to copy to single buffer */
 			return NETDEV_TX_BUSY;
 		}
-- 
2.34.1

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

[netdev-bot+sashiko]

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

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

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).

- [Medium] The commit message says the patch will "rate limit the netdev
  error information print in the receive and transmit data path" of the…

--

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

This commit wraps five netdev_err() calls in gem_rx(), macb_rx(),
macb_interrupt_misc() and macb_start_xmit() with net_ratelimit(). The goal
is to stop the console from flooding when many bad or oversized frames
arrive, for example during boot.

> 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 261a7e87520a4..9f31ad7bf8e13 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -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));
>  	}
>  
> @@ -2505,7 +2507,8 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
>  		else
>  			hdrlen = skb_tcp_all_headers(skb);
>  		if (skb_headlen(skb) < hdrlen) {
> -			netdev_err(bp->netdev, "Error - LSO headers fragmented!!!\n");
> +			if (net_ratelimit())
> +				netdev_err(bp->netdev, "Error - LSO headers fragmented!!!\n");
>  			/* if this is required, would need to copy to single buffer */
>  			return NETDEV_TX_BUSY;
>  		}

[Severity: Medium]

The commit message says the patch rate limits error prints "in the receive
and transmit data path". Are there other data path prints in macb_main.c
that need the same change? Several netdev_err() calls on these paths still
have no rate limit.

gem_rx_refill(), which runs at the end of every gem_rx() poll:

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

macb_tx_map(), called from macb_start_xmit():

	netdev_err(bp->netdev, "BUG! empty skb!\n");
	...
dma_error:
	netdev_err(bp->netdev, "TX DMA map failed\n");

at91ether_start_xmit(), the at91rm9200 counterpart of macb_start_xmit():

	netdev_err(netdev, "%s: DMA mapping error\n", __func__);
	...
	} else {
		netdev_err(netdev, "%s called, but device is busy!\n",
			   __func__);
		return NETDEV_TX_BUSY;

This path returns NETDEV_TX_BUSY, so the qdisc requeues and retries.
Wouldn't the message then print again on every retry?

at91ether_interrupt(), the counterpart of macb_interrupt_misc():

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

This prints from hardirq context on every receive overrun, and an inbound
packet flood can cause overruns. Can this produce the same console flood
the commit message describes? The HRESP message in macb_interrupt_misc()
is now rate limited, but this one is not.

Should these sites be converted too? Alternatively, the commit message
could be narrowed to the sites this patch actually changes.

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