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

[Théo Lebrun]

Replying to sashiko.dev reviews.
https://sashiko.dev/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com

> > @@ -3496,11 +3496,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]
> When macb_context_swap_end() unconditionally re-enables interrupts, sets MAC
> RX/TX back on (RE/TE in NCR), and calls netif_tx_start_all_queues(), does
> this override the phylink state machine by fully activating the MAC without
> checking netif_carrier_ok()?
>
> If the interface is administratively UP but the physical link is DOWN,
> macb_mac_link_down() is called by phylink, which disables MAC RX/TX (RE/TE
> bits) and stops the TX queues.
>
> If an MTU change is requested in this state, does this sequence bypass carrier
> state validation and activate the MAC while the physical link is still down?

Yes, agreed. Reported in [14/15] and discussed there.
https://lore.kernel.org/netdev/DKCVYZ69KGSM.2R1ELFSWRG5HM@xxxxxxxxxxx/

 - Adding netif_carrier_ok() check.
 - Grabbing bp->mac_cfg_lock from macb_hresp_error_task() which also
   edits the carrier state (so risks conflicting).
 - However we don't condition netif_tx_start_all_queues() on
   netif_carrier_ok() because macb_open() is the one doing the call,
   not link-up.

Thanks,

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