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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

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

> commit 9ae1e293afffec7abe7929599ec74c61a5616ac7
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: use context swapping in .ndo_change_mtu()
>
> This commit uses the newly introduced context buffer management to implement
> .ndo_change_mtu() as a context swap. This resists memory pressure by failing
> without closing the interface and speeds up the process by avoiding PHY
> reinitialization. AT91 EMAC is refused with -EBUSY.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index c4818216ab702..30efe46ee0d3e 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3507,11 +3507,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);
>
> [Severity: High]
> Can this sequence cause a deadlock?
>
> When calling macb_context_swap_start() here, it acquires bp->mac_cfg_lock and
> then waits synchronously for bp->hresp_err_bh_work to finish via
> cancel_work_sync():
>
> [...]

Solved because macb_hresp_error_task() doesn't grab mac_cfg_lock
anymore.

> [Severity: High]
> Does this fast context swap wait for the hardware RX engine to become
> completely idle before freeing the old buffers?
>
> Looking at macb_context_swap_start(), it clears the Receive Enable (RE) bit in
> the NCR register to stop the RX engine, but doesn't appear to poll or wait for
> the hardware to finish its current DMA operations.
>
> Then, when we reach macb_context_swap_end() below, the old memory and SKBs are
> freed immediately. If the hardware is still in the process of finishing a
> large packet reception, could it continue writing data into the now-freed skb
> buffers and corrupt memory?

The documented shutdown sequence is

   writel(readl(NCR) & ~RE, NCR);

I just re-checked the manual.
My guess is the LLM got that idea from this comment:

   static void macb_reset_hw(struct macb *bp)
   {
      u32 ctrl = macb_readl(bp, NCR);

      /* Disable RX and TX (XXX: Should we halt the transmission
       * more gracefully?)
       */
      ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));

      // ...
   }

We should drop it, but again no commit budget is available here.

>
> > +	}
> >  
> >  	WRITE_ONCE(netdev->mtu, new_mtu);
> >  
> > +	if (running)
> > +		macb_context_swap_end(bp, new_ctx);
>
> [Severity: Medium]
> Does this unconditionally wake the TX queues regardless of the link state?
>
> The driver correctly halts and stops TX queues in macb_mac_link_down() to
> prevent queuing to disabled hardware. However, calling macb_context_swap_end()
> here finishes with:
>
> macb_context_swap_end() {
>         ...
>         netif_tx_start_all_queues(bp->netdev);
> }
>
> If the link is currently down, doesn't this improperly clear the XOFF state
> set by the link-down handler, allowing packets to be enqueued while the
> transmitter is disabled in hardware?

Second time the LLM reports this. netif_tx_start_all_queues() is done at
open. So it means from open to link-up we have the same issue.

If we don't do that at swap_end, even if we are currently link-down,
then the upcoming link-up will not start the queues, so they will stay
off.

>
> [Severity: Medium]
> Does the context swap sequence fail to restore the Energy Efficient Ethernet
> (LPI) state?
>
> macb_context_swap_start() explicitly cancels the delayed work bp->tx_lpi_work
> to prevent it from firing during the swap.
>
> However, macb_context_swap_end() doesn't seem to contain any logic to restart
> the timer or re-enable LPI. If the interface was idle and bp->eee_active is
> true, will this leave the hardware in a higher power state indefinitely until
> a new packet is transmitted?

Correct. Fixed. See
https://lore.kernel.org/all/DKFJUAD77C4T.38O5CM12TVT5M@bootlin.com/

---

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

> commit cc76a66faa22e273620a90e8d48f9af6c96113e2
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: use context swapping in .ndo_change_mtu()
>
> This converts macb_change_mtu() from refusing MTU changes on a running
> interface into a context swap: allocate a new context, reconfigure the
> hardware via macb_context_swap_start()/macb_context_swap_end(), then free
> the old context. AT91 EMAC devices are meant to be refused with -EBUSY.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index c4818216ab702..30efe46ee0d3e 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3507,11 +3507,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;
>
> [Severity: Low]
> [Finding: fable-5-1-1]
> [Sources: fable-5, opus-5]
> Can this branch ever be reached?  MACB_CAPS_MACB_IS_EMAC is only set by
> emac_config, whose .init is at91ether_init(), and at91ether_init()
> installs at91ether_netdev_ops:
>
> [...]

Correct. I'll keep it anyway. It costs nothing and we might want to
unify ndo in the future.

For example I just noticed that ndo_tx_timeout (which got contributed
recently) hasn't landed for EMAC, even though it could make sense.

And I like seeing easily when reading the code that there is no chance
we ever land in swap_start/end from EMAC.

> That also makes this part of the commit message inaccurate:
>
> 	AT91 EMAC is handled differently as their buffer management is separate
> 	and they don't do NAPI. We refuse them (-EBUSY) to avoid implementing
> 	context swapping for them.
>
> Nothing is actually refused here.  Note the identical guard in
> macb_set_ringparam() is reachable, because at91ether_init() does install
> the shared macb_ethtool_ops, which likely explains the copy.

I'll fix the commit message however.

> > +
> > +		macb_context_swap_start(bp);
>
> [Severity: High]
> [Finding: fable-5-5-9]
> [Sources: fable-5, opus-5]
> Can this deadlock against the HRESP work item?
>
> [...]

Fixed because macb_hresp_error_task() doesn't grab the mutex anymore.

> [Severity: High]
> [Finding: main-5-7]
> [Sources: opus-5]
> Related to the above, is it valid for macb_hresp_error_task() to take a
> mutex at all?  The work is dispatched on a BH workqueue:
>
> [...]

Fixed because macb_hresp_error_task() doesn't grab the mutex anymore.

>
> >  
> >  	WRITE_ONCE(netdev->mtu, new_mtu);
> >  
> > +	if (running)
> > +		macb_context_swap_end(bp, new_ctx);
>
> [Severity: High]
> [Finding: fable-5-5-10]
> [Sources: fable-5]
> Can the carrier test in macb_context_swap_end() leave RX/TX disabled for
> good?  The re-enable is conditional:
>
> 	if (netif_carrier_ok(bp->netdev)) {
> 		... queue_writel(queue, IER, ...);
> 		macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
> 	}
>
> [...]

Fixed. Have our own redundant bp->link_up that lives under
bp->mac_cfg_lock's protection. See
https://lore.kernel.org/all/DKFJUAD77C4T.38O5CM12TVT5M@bootlin.com/

Thanks,

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