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

[Théo Lebrun]

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

> commit 6e5157b79656fb39cc93533094703e19ecdcef41
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: use context swapping in .ndo_change_mtu()
>
> This converts macb_change_mtu() from returning -EBUSY on a running
> interface into a context swap: allocate a new context, quiesce and
> reconfigure the hardware, then free the old context. AT91 EMAC keeps the
> -EBUSY behaviour via a MACB_CAPS_MACB_IS_EMAC check.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 55c97f2035e7..3160e815a89a 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3530,11 +3530,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);
>
> [Severity: Medium]
> [Finding: gpt-5-6-sol-4-5]
> [Sources: gpt-5-6-sol]
>
> This isn't a bug introduced by this patch, but can this error path leak the
> coherent Tx ring?  The same structure already exists at the series baseline
> in macb_alloc_consistent()/macb_free_consistent() and was reachable through
> macb_open(); this hunk adds a second, easily repeatable trigger from
> userspace.

Yes there is a bug in macb_alloc() and it'll be fixed.

[...]

>
> > +
> > +		macb_context_swap_start(bp);
> > +	}
> >  
> >  	WRITE_ONCE(netdev->mtu, new_mtu);
> >  
> > +	if (running)
> > +		macb_context_swap_end(bp, new_ctx);
> > +
>
> [Severity: High]
> [Finding: main-5-6]
> [Sources: opus-5]
>
> Is the netdev Tx watchdog also covered by the swap window?

Yes. Talked about in reply to [PATCH net-next v9 16/17]:
https://lore.kernel.org/all/DLCTKEROUJCX.12G2JSI26LQXH@xxxxxxxxxxx/

Copy/pasting here:

   Well actually netif_tx_disable() does reset dev_queue->trans_start of
   all queues. So a race does exist but it is pretty slim (not the full
   context swap): either before this netif_tx_disable() line or if context
   swap lasts more than 5s.

   I'll protect this by (1) grabbing bp->lock inside macb_tx_restart() and
   (2) checking the state of our boolean flag to know if context swap is
   ongoing.

[...]

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