Re: [PATCH net-next v7 17/17] net: macb: use context swapping in .ndo_change_mtu()

[Théo Lebrun]

Replying to sashiko
https://sashiko.dev/#/patchset/20260803-macb-context-v7-0-4d7d4af04849%40bootlin.com

> commit 86a1d8b3d0caecfd721045c06709c6a2234de503
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: use context swapping in .ndo_change_mtu()
>
> This commit implements ndo_change_mtu() for MACB by swapping contexts.
> It allocates a new context, reconfigures the hardware, and frees the
> old context to resist memory pressure and speed up the process.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 9093fb2c789f5..df9594e515df4 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3531,11 +3531,29 @@ static int macb_close(struct net_device *netdev)
> >  
>
> [...]
>
> >  static int macb_change_mtu(struct net_device *netdev, int new_mtu)
> >  {
> > -	if (netif_running(netdev))
> > -		return -EBUSY;
> > +	struct macb *bp = netdev_priv(netdev);
> > +	bool running = netif_running(netdev);
> > +	struct macb_context *new_ctx;
> > +
> > +	if (running) {
> > +		/* Context swapping is not supported for AT91. */
> > +		if (bp->caps & MACB_CAPS_MACB_IS_EMAC)
> > +			return -EBUSY;
> > +
> > +		new_ctx = macb_context_alloc(bp, new_mtu,
> > +					     bp->configured_rx_ring_size,
> > +					     bp->configured_tx_ring_size);
>
> [...]
>
> > +		if (IS_ERR(new_ctx))
> > +			return PTR_ERR(new_ctx);
> > +
> > +		macb_context_swap_start(bp);
> > +	}
> >  
> >  	WRITE_ONCE(netdev->mtu, new_mtu);
> >  
> > +	if (running)
> > +		macb_context_swap_end(bp, new_ctx);
>
> [Severity: High]
> Does macb_context_swap_end() unconditionally wake the TX queues even if the
> link is down?
>
> If the interface is UP but the link is DOWN (e.g., carrier loss),
> macb_mac_link_down() stops the TX queues. However, when ndo_change_mtu() is
> called and executes macb_context_swap_end():
>
> macb_context_swap_end() {
>     if (bp->link_up && bp->eee_active)
>         macb_txp_lpi_initial_defer(bp);
>     
>     netif_tx_start_all_queues(bp->netdev);
> }
>
> By calling netif_tx_start_all_queues() without checking the bp->link_up
> state, the TX queues are awakened. Can this cause the networking stack to
> push packets to the driver, filling up the TX ring and eventually triggering
> a NETDEV_WATCHDOG TX timeout since the hardware transmitter remains
> disabled?

Argh! It's not the first time Sashiko mentions this
netif_tx_start_all_queues() call during swap.

It didn't bother me because we have the same in macb_open(). So if our
open implementation leaves tx queues active even before link-up then I
don't see why it's wrong to have our swap implementation do the same.

Looking more and more into it, the only difference I could find is that
we cannot have a qdisc attached at open, but we might have one at swap
even while link is down (with the net/core/link_watch.c mechanism).

Let's please Sashiko, and do two changes:
 - condition the netif_tx_start_all_queues() call
 - actually replace it by a netif_tx_wake_all_queues()

I'm adding the second point because IIUC it is needed to trigger
immediately if there were pending packets in the qdisc, else it could
wait for the next packet to arrive.

Thanks,

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