[PATCH net-next] net: macb: move printk() calls out of bp->lock critical section

[PATCH net-next] net: macb: move printk() calls out of bp->lock critical section

[Théo Lebrun]

printk() call while bp->lock is acquired might be a bad idea:

 - It grows the spinlock atomic section.

 - If netconsole is active on the same interface (and we run on the
   queue's CPU), we risk a deadlock because macb_poll_controller()
   calls macb_interrupt() which grab bp->lock if an IRQ is pending.

Three messages are changed:

 - In macb_tx_error_task(), defer netdev_err("halt tx timed out") call
   to after the critical section. Update the message to highlight it
   occurred in the past. Inherit the buffer exhaustion boolean variable
   name from the old code comment.

 - In macb_tx_error_task(), defer the netdev_err("TX buffers exhausted
   mid-frame") call out of the loop. This also means it goes from
   1-per-error to 1-per-task-invocation.

 - In IRQ handling, move HRESP error printing out of
   macb_interrupt_misc() into macb_interrupt(). Again, it means we
   dedup error reporting if status is read multiple times in a row with
   HRESP bit set. This is fine as from past instances I've seen, this
   message spams our log if it occurs.

Notice we *ignore* debug printks; if you are debugging MACB maybe don't
use netconsole...

The netconsole deadlock is theoretical & never reproduced.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
This patch used to be part of context swapping V9.
It got moved out as there aren't any dependency or relationship.

Technically it is a fix, in practice I'm happy for it to go through
net-next/main for more testing and it is a theoretical bugfix (as usual
nowadays). Decided after seeing Jakub taking a similar patch into
net-next this morning:

> Since Linus is pushing back on our number of Fixes let's go for
> net-next with similar fixes until the merge window
https://lore.kernel.org/netdev/20260929184531.36ac3dc3@kernel.org/

Changes since context swapping v9:
- Rebase onto latest net-next/main (47a144672573).
- Send patch standalone.
- Simplify the commit message (it was a long and partially wrong
  rambling before). Also mention that it is a theoretical bugfix.
- Link to v9: https://patch.msgid.link/20260812-macb-context-v9-0-7ddbf5f715e0@bootlin.com
---
 drivers/net/ethernet/cadence/macb_main.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 20fe30789834..6725f8ac7606 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1282,6 +1282,7 @@ static void macb_tx_error_task(struct work_struct *work)
 	struct macb_tx_skb *tx_skb;
 	struct macb_dma_desc *desc;
 	bool halt_timeout = false;
+	bool buggy_driver = false;
 	struct sk_buff *skb;
 	unsigned long flags;
 	unsigned int tail;
@@ -1308,7 +1309,6 @@ static void macb_tx_error_task(struct work_struct *work)
 	 * macb/gem must be halted to write TBQP register
 	 */
 	if (macb_halt_tx(bp)) {
-		netdev_err(bp->netdev, "BUG: halt tx timed out\n");
 		macb_writel(bp, NCR, macb_readl(bp, NCR) & (~MACB_BIT(TE)));
 		halt_timeout = true;
 	}
@@ -1353,8 +1353,7 @@ static void macb_tx_error_task(struct work_struct *work)
 			 * those. Statistics are updated by hardware.
 			 */
 			if (ctrl & MACB_BIT(TX_BUF_EXHAUSTED))
-				netdev_err(bp->netdev,
-					   "BUG: TX buffers exhausted mid-frame\n");
+				buggy_driver = true;
 
 			desc->ctrl = ctrl | MACB_BIT(TX_USED);
 		}
@@ -1391,7 +1390,14 @@ static void macb_tx_error_task(struct work_struct *work)
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
 	spin_unlock_irqrestore(&bp->lock, flags);
+
 	napi_enable(&queue->napi_tx);
+
+	if (halt_timeout)
+		netdev_err(bp->netdev, "BUG: halt tx timed out, we ignored it\n");
+
+	if (buggy_driver)
+		netdev_err(bp->netdev, "BUG: TX buffers exhausted mid-frame\n");
 }
 
 static bool ptp_one_step_sync(struct sk_buff *skb)
@@ -2143,11 +2149,8 @@ static void gem_wol_interrupt(struct macb_queue *queue, u32 status)
 static int macb_interrupt_misc(struct macb_queue *queue, u32 status)
 {
 	struct macb *bp = queue->bp;
-	struct net_device *netdev;
 	u32 ctrl;
 
-	netdev = bp->netdev;
-
 	if (unlikely(status & (MACB_TX_ERR_FLAGS))) {
 		queue_writel(queue, IDR, MACB_TX_INT_FLAGS);
 		schedule_work(&queue->tx_error_task);
@@ -2187,7 +2190,6 @@ 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");
 		macb_queue_isr_clear(bp, queue, MACB_BIT(HRESP));
 	}
 
@@ -2207,6 +2209,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 	struct macb_queue *queue = dev_id;
 	struct macb *bp = queue->bp;
 	struct net_device *netdev = bp->netdev;
+	bool hresp_err = false;
 	u32 status;
 
 	status = queue_readl(queue, ISR);
@@ -2253,15 +2256,22 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			napi_schedule_irqoff(&queue->napi_tx);
 		}
 
-		if (unlikely(status & MACB_INT_MISC_FLAGS))
+		if (unlikely(status & MACB_INT_MISC_FLAGS)) {
 			if (macb_interrupt_misc(queue, status))
 				break;
 
+			if (status & MACB_BIT(HRESP))
+				hresp_err = true;
+		}
+
 		status = queue_readl(queue, ISR);
 	}
 
 	spin_unlock(&bp->lock);
 
+	if (hresp_err)
+		netdev_err(netdev, "DMA bus error: HRESP not OK\n");
+
 	return IRQ_HANDLED;
 }
 

---
base-commit: 068b5854d6cf588826bd876172652e7c1469d4e6
change-id: 20260930-macb-irq-a87653182a47

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

Re: [PATCH net-next] net: macb: move printk() calls out of bp->lock critical section

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next] net: macb: move printk() calls out of bp->lock critical section

[Théo Lebrun]

Hello netdev bot,

On Wed Sep 30, 2026 at 8:38 PM CEST,  wrote:
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

This is mentioned two fold; in the commit message:

   The netconsole deadlock is theoretical & never reproduced.

And in the cover letter (folded below the '--' line):

   Technically it is a fix, in practice I'm happy for it to go through
   net-next/main for more testing and it is a theoretical bugfix (as usual
   nowadays). Decided after seeing Jakub taking a similar patch into
   net-next this morning:
   [...]

>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

I do most of my MACB tests on EyeQ5.

> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

I love this newly introduced message!
But here it might have been a false positive (?).

 - Can it read what's below the '--' line?

 - Maybe it could trust the driver maintainers, especially regarding the
   second question about having access to real hardware?

I looked around to see if this infra was open-source but I couldn't find
it. Only found the https://github.com/linux-netdev/nipa repo but that's
not it.

Thanks,

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