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

[netdev-bot+sashiko@kernel.org]

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 does not cover what its commit message claims.
- [Low] The author identity "taozijin <taozj888@163.com>" does not match
  the Signed-off-by trailer "Zijin Tao <taozj888@163.com>".

--

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

This patch wraps four netdev_err() calls in the macb driver with
net_ratelimit(). Two are in gem_rx(), one is the RX queue corruption
message in macb_rx(), and one is the HRESP DMA bus error in
macb_interrupt_misc(). The goal is to stop console floods at boot when
many bad or oversized frames arrive.

> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Cc: stable@vger.kernel.org
>
> Signed-off-by: Zijin Tao <taozj888@163.com>

[Severity: Low]
This isn't a bug, but the name in the Author line does not match the
name in the Signed-off-by. The email address is the same:

  Author: taozijin <taozj888@163.com>
  Signed-off-by: Zijin Tao <taozj888@163.com>

checkpatch.pl reports this as FROM_SIGN_OFF_MISMATCH.

Also, a blank line separates the Fixes: and Cc: tags from the
Signed-off-by, so they don't form a single trailer block.

> 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

[ ... ]

> @@ -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: High]
The commit message says:

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

Does the patch match that description? The only non-RX print it
converts is this HRESP one in macb_interrupt_misc(). That is a DMA bus
error interrupt, not the transmit data path. No TX data path print
seems to be rate limited.

Several prints of the same kind in macb_main.c look like they are still
unlimited. at91ether_interrupt() runs in hard IRQ context and still
does:

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

at91ether_open() enables ISR_ROVR. A line rate flood from a link peer
would cause exactly this RX overrun, which is the scenario in the commit
message. macb_interrupt_misc() handles the same bit by only incrementing
hw_stats rx_overruns.

gem_rx_refill() runs at the end of every gem_rx() poll and still has:

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

Under memory pressure, would this log again on every poll, right next
to the gem_rx() prints that are now rate limited?

On the TX side, these still print without a limit:

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

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

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

  macb_tx_error_task():
	netdev_err(bp->netdev,
		   "BUG: TX buffers exhausted mid-frame\n");

The comment in macb_tx_error_task() says that last print is meant to be
loud, so it may be fine to leave as is.

For the two paths that return NETDEV_TX_BUSY, won't the qdisc requeue
the same skb and print the message again on every retry?

This is tagged for stable. Should the remaining RX, IRQ and TX data path
sites be converted too? Or should the commit message only describe the
RX and HRESP prints this patch changes?

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