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

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

quoted

@@ -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@bootlin.com/ (local)

 - 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