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

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

Sashiko replies.

Both sashiko.dev and sashiko-netdev has comments on this patch.
https://sashiko.dev/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com?patch=8011

---

sashiko.dev:

> > +static void macb_context_swap_end(struct macb *bp,
> > +              struct macb_context *new_ctx)
> > +{
> [ ... ]
> > +   ctrl = macb_readl(bp, NCR);
> > +   macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
> > +
> > +   spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +   mutex_unlock(&bp->mac_cfg_lock);
> > +
> > +   netif_tx_start_all_queues(bp->netdev);
>
> [Severity: High]
> Does unconditionally enabling the MAC receiver, transmitter, and TX queues
> here risk overriding phylink's link-down state?
>
> If a user changes ring parameters via ethtool on an administratively up
> interface that currently has no physical carrier, could this forcefully
> enable the hardware and violate the phylink state machine contract?

Yes. We need to check for that using netif_carrier_ok(). We already have
our bp->mac_cfg_lock mutex delaying the conflicting phylink ops.

Note: we'll add the mutex lock in macb_hresp_error_task() which toggles
the carrier state.

>
> > +
> > +   /* Free old context. */
> > +
> > +   macb_free(old_ctx);
> > +   kfree(old_ctx);
> > +}

---

sashiko-netdev:

> > +static void macb_context_swap_end(struct macb *bp,
> > +              struct macb_context *new_ctx)
> > +{
> > +   struct macb_context *old_ctx;
> > +   struct macb_queue *queue;
> > +   unsigned long flags;
> > +   unsigned int q;
> > +   u32 ctrl;
> > +
> > +   lockdep_assert_held(&bp->mac_cfg_lock);
> > +
> > +   /* Swap contexts & give buffer pointers to HW. */
> > +
> > +   old_ctx = bp->ctx;
> > +   bp->ctx = new_ctx;
> > +   macb_init_buffers(bp);
> > +
> > +   /* Start NAPI, HW Tx/Rx and software Tx. */
> > +
> > +   for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> > +      napi_enable(&queue->napi_rx);
> > +      napi_enable(&queue->napi_tx);
> > +   }
> > +
> > +   spin_lock_irqsave(&bp->lock, flags);
> > +
> > +   /* Re-arm normal interrupt processing before enabling IRQs. */
> > +   bp->ctx_swap = false;
> > +
> > +   macb_configure_dma(bp);
> > +
> > +   for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> > +      queue_writel(queue, IER,
> > +              bp->rx_intr_mask |
> > +              MACB_TX_INT_FLAGS |
> > +              MACB_BIT(HRESP));
> > +   }
> > +
> > +   ctrl = macb_readl(bp, NCR);
> > +   macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
> > +
> > +   spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +   mutex_unlock(&bp->mac_cfg_lock);
> > +
> > +   netif_tx_start_all_queues(bp->netdev);
>
> [Medium]
> Can there be a window between mutex_unlock(&bp->mac_cfg_lock) and
> netif_tx_start_all_queues(bp->netdev) where phylink_resolve runs
> macb_mac_link_down(), which now also takes mac_cfg_lock and calls
> netif_tx_stop_all_queues()?
>
> If mac_link_down runs in that window:
>
>     static void macb_mac_link_down(...)
>     {
>         mutex_lock(&bp->mac_cfg_lock);
>         ...
>         netif_tx_stop_all_queues(netdev);
>         mutex_unlock(&bp->mac_cfg_lock);
>     }
>
> then the subsequent netif_tx_start_all_queues() in macb_context_swap_end()
> would clear __QUEUE_STATE_DRV_XOFF and undo the stop that mac_link_down
> just performed.
>
> For comparison, macb_mac_link_up() calls netif_tx_wake_all_queues() while
> still holding mac_cfg_lock, and macb_mac_link_down() calls
> netif_tx_stop_all_queues() while still holding it. Would moving the
> netif_tx_start_all_queues() call above mutex_unlock() here match that
> pattern and close the window?

Agreed. netif_tx_start_all_queues() will move one line up.

>
> > +
> > +   /* Free old context. */
> > +
> > +   macb_free(old_ctx);
> > +   kfree(old_ctx);
> > +}
>
> > @@ -3816,9 +3953,10 @@ static int macb_set_ringparam(struct net_device *netdev,
> >                 struct kernel_ethtool_ringparam *kernel_ring,
> >                 struct netlink_ext_ack *extack)
> >  {
> > +   unsigned int new_rx_size, new_tx_size;
> >     struct macb *bp = netdev_priv(netdev);
> > -   u32 new_rx_size, new_tx_size;
> > -   unsigned int reset = 0;
> > +   bool running = netif_running(netdev);
> > +   struct macb_context *new_ctx;
> >
> >     if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
> >        return -EINVAL;
> > @@ -3837,16 +3975,24 @@ static int macb_set_ringparam(struct net_device *netdev,
> >        return 0;
> >     }
> >
> > -   if (netif_running(bp->netdev)) {
> > -      reset = 1;
> > -      macb_close(bp->netdev);
> > +   if (running) {
> > +      /* Context swapping is not supported for AT91. */
> > +      if (bp->caps & MACB_CAPS_MACB_IS_EMAC)
> > +         return -EBUSY;
>
> [Medium]
> Before this change, ethtool --set-ring on an administratively up AT91
> EMAC interface went through the macb_close()/macb_open() path and
> succeeded. After this change AT91 EMAC returns -EBUSY when the interface
> is up, so users must now bring the link down, adjust ring sizes, and
> bring it back up.
>
> Should this user-visible behavior change be called out in the commit
> message, or would it be preferable to keep the previous close/reopen
> sequence as an AT91-only fallback so existing management scripts continue
> to work?

No we don't want the fallback.
Yes I'll mention this in the commit message.

Thanks,

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