Re: [PATCH] net: macb: add TX stall timeout callback to recover from lost TSTART write

[Andrea della Porta <andrea.porta@suse.com>]

Hi Theo,

On 11:45 Fri 12 Jun     , Théo Lebrun wrote:
> Hello Andrea,
> 
> On Fri Jun 12, 2026 at 11:01 AM CEST, Andrea della Porta wrote:
> > From: Lukasz Raczylo <lukasz@raczylo.com>
> >
> > The MACB found in the Raspberry Pi RP1 suffers from sporadic stalls on
> > the TX queue.
> > While the exact root cause is not yet fully understood, it is likely
> > related to a hardware issue where a TSTART write to the NCR register
> > is missed, preventing the transmission from being kicked off.
> >
> > Implement a timeout callback to handle TX queue stalls, triggering the
> > existing restart mechanism to recover.
> >
> > Link: https://lore.kernel.org/all/20260514215459.36109-1-lukasz@raczylo.com/
> > Fixes: dc110d1b23564 ("net: cadence: macb: Add support for Raspberry Pi RP1 ethernet controller")
> > Signed-off-by: Lukasz Raczylo <lukasz@raczylo.com>
> > Co-developed-by: Steffen Jaeckel <sjaeckel@suse.de>
> > Signed-off-by: Steffen Jaeckel <sjaeckel@suse.de>
> > Co-developed-by: Andrea della Porta <andrea.porta@suse.com>
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index a12aa21244e83..615da65d5d68d 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -4522,6 +4522,16 @@ static int macb_setup_tc(struct net_device *dev, enum tc_setup_type type,
> >  	}
> >  }
> >  
> > +static void macb_tx_timeout(struct net_device *dev, unsigned int q)
> > +{
> > +	struct macb *bp = netdev_priv(dev);
> > +
> > +	if (net_ratelimit())
> > +		netdev_err(dev, "TX stall detected, re-kicking TSTART\n");
> 
> Is this standard? It looks odd.

I've found it used in other drivers, it's the closest I had found that
limit the rate for net related output. As Nicolai suggested, on timeout
a message is already printed by the core, so I will drop those two lines.

> 
> > +	dev->stats.tx_errors++;
> 
> I am surprised by this. `tx_errors` would ideally be one per packet that
> didn't get sent. Here we increment it once per queue that stalled.
> 
> I have a series to address stats issue (and use netdev_stat_ops API).
> It is a follow-up to this:
> https://lore.kernel.org/netdev/20260428-macb-drop-tx-v2-0-647f5199d8df@bootlin.com/
> 
> Also this is per-device shared data and we access it without
> synchronisation.
> 
> Let's drop this increment.

Agreed.

Thanks,
Andrea

> 
> > +	macb_tx_restart(&bp->queues[q]);
> > +}
> 
> Regards,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>