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

[Théo Lebrun]

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260731-macb-context-v6-0-49d5a1439d48%40bootlin.com

> commit 99f5236ffcf1506bce8a08773b0456e50c57986e
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: move printk() calls out of bp->lock critical section
>
> The two netdev_err() calls in macb_tx_error_task() are deferred until after
> the spin_unlock_irqrestore(&bp->lock, flags), using the existing
> halt_timeout flag plus a new buggy_driver flag. The stated goal is to
> guarantee that netpoll can never be entered while bp->lock is held.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 987fafb2bf40d..65d5ff8a5e230 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -1358,9 +1358,6 @@ static void macb_tx_error_task(struct work_struct *work)
> >  			 * since it's the only one written back by the hardware
> >  			 */
> >  			if (!(ctrl & MACB_BIT(TX_BUF_EXHAUSTED))) {
> > -				netdev_vdbg(bp->netdev, "txerr skb %u (data %p) TX complete\n",
> > -					    macb_tx_ring_wrap(ctx, tail),
> > -					    skb->data);
> >  				bp->netdev->stats.tx_packets++;
> >  				queue->stats.tx_packets++;
> >  				packets++;
>
> [Severity: Low]
> [Finding: main-1-1]
> [Sources: opus-5]
> This isn't a bug, but the commit message only describes deferring the
> netdev_err() calls, while this hunk deletes the per-descriptor
> netdev_vdbg() trace outright rather than moving it. Its arguments are
> per-descriptor (macb_tx_ring_wrap(ctx, tail) and skb->data) and cannot be
> reproduced after the loop, so VERBOSE_DEBUG builds lose that TX error
> recovery trace. Could the commit message mention this removal?

See below.

>
> > @@ -1374,8 +1371,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);
> >  		}
>
> [Severity: Low]
> [Finding: main-1-1]
> [Sources: opus-5]
> Same finding as above: this also converts a per-descriptor message into one
> aggregated message per invocation, so the number of affected descriptors is
> no longer visible. Worth a line in the commit message as well?

Yes! Mentioned in cover letter but not enough in commit message.

> > @@ -1412,6 +1408,13 @@ 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);
> > +
> > +	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");
> > +
> >  	napi_enable(&queue->napi_tx);
> >  }
>
> [Severity: High]
> [Finding: main-1-0]
> [Sources: opus-5]
> The commit message says the goal is "to ensure we can never netpoll under
> bp->lock's reign". Is that invariant actually established, given the
> netdev_err() still present in macb_interrupt_misc()?
>
> 	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));
> 	}
>
> macb_interrupt() holds the lock across the whole status loop, including that
> call:
>
> 	spin_lock(&bp->lock);
> 	while (status) {
> 		...
> 		if (unlikely(status & MACB_INT_MISC_FLAGS))
> 			if (macb_interrupt_misc(queue, status))
> 				break;
> 		status = queue_readl(queue, ISR);
> 	}
> 	spin_unlock(&bp->lock);
>
> [...]

Argh, yes. I had seen this printk() following the last review iteration
but I couldn't think of a way to land in the interrupt handler while
netpoll was doing its job. But actually it's the other way around: we
might be in the IRQ handler and call our printk that triggers netpoll
mode, re-entering into macb_interrupt_misc().

Let's drop that netdev_err() in macb_interrupt_misc(). I've checked
again, it's the only non-debug printk I could find potentially in a
bp->lock section.

If you have debug prints active AND you land in this race, you have your
hands in the engine anyway. This isn't hurting any real driver user. So
I explicitely ignore them.

(Note: I will re-add the netdev_vdbg() in macb_tx_error_task().)

Thanks,

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