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

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

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

> commit 34a47bcab06bd390622de5b8628e1ca4b22fa12e
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
> Subject: net: macb: use context swapping in .set_ringparam()
>
> This commit modifies the ethtool set_ringparam implementation to use a
> new context swapping mechanism instead of the previous close and reopen
> sequence. It introduces macb_context_swap_start() and
> macb_context_swap_end() helpers to handle the transition smoothly, along
> with a new mac_cfg_lock to serialize changes against phylink MAC callbacks.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 6d79663e42bfe..55c97f2035e76 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -814,6 +831,10 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
> >  	unsigned int q;
> >  	u32 ctrl;
> >  
> > +	mutex_lock(&bp->mac_cfg_lock);
> > +
> > +	bp->link_up = false;
> > +
> >  	if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC))
> >  		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> >  			queue_writel(queue, IDR,
> > @@ -824,6 +845,8 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
> >  	macb_writel(bp, NCR, ctrl);
>
> [Severity: High]
> This is a pre-existing issue, but does this read-modify-write of NCR need
> protection to avoid dropping hardware configuration?

Indeed. Unrelated but will do.

> >  	netif_tx_stop_all_queues(netdev);
> > +
> > +	mutex_unlock(&bp->mac_cfg_lock);
> >  }
>
> [ ... ]
>
> > @@ -3105,6 +3135,134 @@ static void macb_configure_dma(struct macb *bp)
> >  	}
> >  }
> >  
> > +static void macb_context_swap_start(struct macb *bp)
> > +{
> > +	struct macb_queue *queue;
> > +	unsigned long flags;
> > +	unsigned int q;
> > +	u32 ctrl;
> > +
> > +	mutex_lock(&bp->mac_cfg_lock);
> > +
> > +	/* We cannot mask IRQs because they'll get re-armed by BH. So instead we
> > +	 * signal to IRQ handler it shouldn't drive BH features and should
> > +	 * self-disarm.
> > +	 */
> > +	spin_lock_irqsave(&bp->lock, flags);
> > +	bp->ctx_swap = true;
> > +	spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +	/* Drain BH features. HW is still active and usable at this point but
> > +	 * IRQs are being ignored.
> > +	 */
> > +
> > +	cancel_work_sync(&bp->hresp_err_bh_work);
> > +
> > +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> > +		/* Must be done before NAPI is disabled. */
> > +		cancel_work_sync(&queue->tx_error_task);
>
> [Severity: High]
> This is a pre-existing issue, but since we are carefully cancelling worker
> threads here during context swap, should we also be cancelling them during
> macb_close() and macb_suspend()?

Yes I'll integrate all that for next revision. See this email where I
went into more details.
https://lore.kernel.org/netdev/DLCLQPVD8B9N.NFKWDS6SH0QN@bootlin.com/

[...]
> > +
> > +		napi_disable(&queue->napi_rx);
> > +		napi_disable(&queue->napi_tx);
> > +	}
> > +
> > +	/* Must be done after napi_tx is disabled. */
> > +	cancel_delayed_work_sync(&bp->tx_lpi_work);
> > +
> > +	/* Can finally disable software Tx; need to wait until napi_tx and
> > +	 * tx_error_task cannot be scheduled as either might wakeup Tx.
> > +	 */
> > +	netif_tx_disable(bp->netdev);
>
> [Severity: High]
> Does this sequence leave the interface vulnerable to a false TX timeout?
>
> When macb_context_swap_start() stops the software TX queues via
> netif_tx_disable(), it does not suspend the watchdog via netif_carrier_off()
> or update the queue's trans_start time.

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.

> > +
> > +	/* Now that everything is stopped, clear DQL. */
>
> [ ... ]
>
> > +static void macb_context_swap_end(struct macb *bp,
> > +				  struct macb_context *new_ctx)
> > +{
>
> [ ... ]
>
> > +	/* Free old context. */
> > +
> > +	macb_free(old_ctx);
> > +	kfree(old_ctx);
>
> [Severity: Critical]
> This is a pre-existing issue, but could this initialization sequence lead
> to severe memory corruption if we are under memory pressure?

Yes, that's the whole point.

>
> macb_init_buffers() is called earlier in this function to swap to the new
> context. When initializing DMA rings, gem_init_rings() calls
> gem_init_rx_ring(), which leaves the newly allocated descriptors completely
> zeroed, and then calls gem_rx_refill().
>
> If netdev_alloc_skb() fails due to memory pressure, gem_rx_refill() aborts
> early:
>
>     if (unlikely(!skb)) {
>         netdev_err(bp->netdev, "Unable to allocate sk_buff\n");
>         break;
>     }

That's the main issue: gem_rx_refill() should propagate the error and
make the context swap fail. This patch exists but was part of the XDP
series. I'll take it along the ride as it's needed to make context swap
reasonable.

[...]

> > @@ -3829,9 +3987,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 (bp->caps & MACB_CAPS_MACB_IS_EMAC)
> >  		return -EOPNOTSUPP;
> > @@ -3853,16 +4012,20 @@ static int macb_set_ringparam(struct net_device *netdev,
> >  		return 0;
> >  	}
> >  
> > -	if (netif_running(bp->netdev)) {
> > -		reset = 1;
> > -		macb_close(bp->netdev);
> > +	if (running) {
> > +		new_ctx = macb_context_alloc(bp, netdev->mtu,
> > +					     new_rx_size, new_tx_size);
>
> [Severity: High]
> Can this allocation path lead to a kernel panic and memory leak on failure?
>
> When macb_context_alloc() is called here under memory pressure, it calls
> macb_alloc().
>
> In macb_alloc(), if the RX DMA allocation fails (or boundary check fails),
> it jumps to the error path after allocating tx:
>
> drivers/net/ethernet/cadence/macb_main.c:macb_alloc() {
>     ...
>     if (!rx || upper_32_bits(rx_dma) != upper_32_bits(rx_dma + size - 1))
>         goto out_err;
>     ...
> out_err:
>     macb_free(ctx);
> }
>
> At this point, txq->ring is still NULL. macb_free() will unconditionally call:
>
>     dma_free_coherent(dev, size, txq->ring, txq->ring_dma);
>
> Will this trigger a panic on architectures where dma_free_coherent() lacks
> tolerance for a NULL virtual address, and permanently leak the successfully
> allocated TX DMA ring?

I don't see where dma_free_coherent() "lacks tolerance for a NULL
virtual address". That would be surprising.

However we do have a leak yes, which I plan on fixing (still unrelated
to context swapping).

>
> > +		if (IS_ERR(new_ctx))
> > +			return PTR_ERR(new_ctx);
> > +
> > +		macb_context_swap_start(bp);
> >  	}

---

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

> commit c12dbce3061c66a490797075684d70f4e544b34f
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: use context swapping in .set_ringparam()
>
> This replaces the close / resize / reopen sequence in
> ethtool_ops.set_ringparam() with a context swap: a new context (rings and
> buffers) is allocated first, the datapath is quiesced via a new bp->ctx_swap
> flag plus NAPI/work draining, the new context is handed to the hardware, and
> the old context is freed afterwards. A new mac_cfg_lock mutex serialises the
> swap against the phylink MAC callbacks, and a redundant bp->link_up boolean
> tracks link state under that mutex.
>
> A few questions below, first on the changelog and then inline in the diff.
>
> [Severity: Low]
> [Finding: main-1-0]
> [Sources: opus-5]
> The changelog describes the new mutex and the redundant link state boolean:
>
>     Introduce a new locking primitive (mac_cfg_lock mutex) to serialise swap
>     with phylink MAC callbacks. [...] so we use a redundant bp->link_up
>     boolean that is mac_cfg_lock protected.
>
> but the second new state flag, bp->ctx_swap, is not mentioned anywhere, and
> neither is the change of macb_interrupt() semantics that goes with it. While
> a swap is in progress the handler treats every pending cause as if the device
> were down (IDR -1, ISR clear, break), so MACB_INT_MISC_FLAGS handling and
> HRESP DMA bus errors are acknowledged and discarded for the duration of the
> window.
>
> Could the changelog say a word about bp->ctx_swap and about the interrupt
> handler behaviour during the swap, since correctness of the sequence depends
> on both?

Good point, this is missing!

>
> > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > index ed9bd58d2e26..35564f2ed662 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
>
> [ ... ]
>
> > @@ -1415,6 +1417,12 @@ struct macb {
> >  	struct delayed_work	tx_lpi_work;
> >  	u32			tx_lpi_timer;
> >  
> > +	/* ISR must not drive NAPI & BH mechanisms. Protected by bp->lock. */
> > +	bool			ctx_swap;
> > +
> > +	/* Redundant to netif_carrier_ok(), but set under bp->mac_cfg_lock. */
> > +	bool			link_up;
> > +
> >  	u32	rx_intr_mask;
> >  
> >  	struct macb_pm_data pm_data;
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 6d79663e42bf..55c97f2035e7 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
> > @@ -2199,8 +2227,10 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> >  	}
> >  
> >  	while (status) {
> > -		/* close possible race with dev_close */
> > -		if (unlikely(!netif_running(netdev))) {
> > +		/* close possible race with dev_close, and with context-swap
> > +		 * teardown
> > +		 */
> > +		if (unlikely(!netif_running(netdev) || bp->ctx_swap)) {
> >  			queue_writel(queue, IDR, -1);
> >  			macb_queue_isr_clear(bp, queue, -1);
> >  			break;
>
> [Severity: Medium]
> [Finding: main-5-8]
> [Sources: opus-5]
> bp->ctx_swap is set on entry to the swap region in macb_context_swap_start()
> and cleared on exit in macb_context_swap_end(), purely to keep the interrupt
> handler away from state the swap is mutating. Is a boolean the right
> mechanism here rather than masking the per-queue IRQs?
>
> The driver already owns queue->irq, so disable_irq()/enable_irq(), or IMR
> masking plus synchronize_irq(), would enforce the exclusion instead of
> relying on the handler observing the flag.

No the masking+synchronize_irq() doesn't work. If we start context
swapping by masking interrupts, NAPI/BH is still active and might
re-arm IRQs. See how napi_rx/napi_tx/tx_error_task/hresp_task all can
enable/disable IRQs, and they are not yet disabled/cancelled.

So then we might think "we'll disable NAPI/BH features first then
disable IRQs". But that doesn't fly either! Notice how IRQs schedule
the NAPI/BH features and we can't have that if they are disabled. Eg
napi_disable on an already disabled NAPI is a deadlock.

|               |   enable/disable   |           schedule            |
|               |----|-------|-------|------|----|----|--------|-----|
|               |IRQs|napi_tx|napi_rx|tx_lpi|napi|napi|tx_error|hresp|
| Context       |    |       |       | task | rx | tx |  task  |task |
|===============|====|=======|=======|======|====|====|========|=====|
| open          | X  |   X   |   X   |      |    |    |        |     |
| link_up       | X  |       |       |      |    |    |        |     |
| link_down     | X  |       |       |      |    |    |        |     |
| close         | X  |   X   |   X   |      |    |    |        |     |
| enable_tx_lpi |    |       |       |  X   |    |    |        |     |
| swap          | X  |   X   |   X   |  X   |    |    |        |     |
| suspend       | X  |   X   |   X   |      |    |    |        |     |
| resume        | X  |   X   |   X   |      |    |    |        |     |
|---------------|----|-------|-------|------|----|----|--------|-----|
| irq & netpoll | X  |       |       |      | X  | X  |   X    | X   |
|---------------|----|-------|-------|------|----|----|--------|-----|
| napi_rx       | X  |       |       |      | X  |    |        |     |
| napi_tx       | X  |       |       |  X   |    | X  |        |     |
|---------------|----|-------|-------|------|----|----|--------|-----|
| tx_error_task | X  |   X   |       |      |    |    |        |     |
| hresp task    | X  |       |       |      |    |    |        |     |

The reason disable_irq()/enable_irq() doesn't work is more long term. We
also need the same "shutdown IRQs atomically" mechanism for the suspend
WOL path. In that case we need to shutdown all but one IRQ. There's the
same thing for when the interface is down. We'll use this "quiesce
IRQs" mechanism as well, but might want to keep some activated.

We're not the first ones using such a mechanism. Check out tg3 for
example and its tg3_irq_quiesce() function.
https://elixir.bootlin.com/linux/v7.2.2/source/drivers/net/ethernet/broadcom/tg3.c#L7488

[...]

>
> > @@ -3105,6 +3135,134 @@ static void macb_configure_dma(struct macb *bp)
> >  	}
> >  }
> >  
> > +static void macb_context_swap_start(struct macb *bp)
> > +{
>
> [ ... ]
>
> > +	/* Must be done after napi_tx is disabled. */
> > +	cancel_delayed_work_sync(&bp->tx_lpi_work);
> > +
> > +	/* Can finally disable software Tx; need to wait until napi_tx and
> > +	 * tx_error_task cannot be scheduled as either might wakeup Tx.
> > +	 */
> > +	netif_tx_disable(bp->netdev);
>
> [Severity: High]
> [Finding: gpt-5-6-sol-5-11]
> [Sources: gpt-5-6-sol, opus-5]
> This stops all TX queues while netif_running() and the carrier both stay set,
> since this is an ethtool op and phylink is deliberately not stopped. Does
> that leave the netdev TX watchdog free to run during the swap?

Yes! See above (grep for "^Well actually") where I explained how we fix
this TX watchdog race. And we explicitely don't touch phylink because
we don't want any PHY retrain. Gotta go fast.

[...]

>
> > +	spin_unlock_irqrestore(&bp->lock, flags);
> > +}
> > +
> > +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);
>
> [Severity: Medium]
> [Finding: main-5-9]
> [Sources: opus-5]
> bp->ctx is published here with a plain store, with no bp->lock, no
> WRITE_ONCE() and no barrier, and old_ctx is dma-freed and kfree()d at the
> end of this function after mac_cfg_lock has been dropped and after NAPI,
> software TX and hardware RX/TX have been restarted.

Yes that's a mistake. Should be WRITE_ONCE() followed by mb().
All codepaths that assign bp->ctx should look like that.

Btw, I learnt something reading a good chunk of
Documentation/memory-barriers.txt: "Memory operations issued before the
ACQUIRE may be completed after the ACQUIRE operation has completed". So
here we do

   bp->ctx = ...;
   spin_lock(...);
   relaxed_writel(...);

And the spin_lock isn't implicitely introducing a mandatory mb. TIL.

> Every other non-rtnl reader of bp->ctx runs from hardirq, softirq or work
> context under bp->lock or queue->tx_ptr_lock, for example
> macb_tx_lpi_work_fn() -> macb_tx_all_queues_idle():
>
>     for (q = 0; q < bp->num_queues; ++q) {
>         txq = &bp->ctx->txq[q];
>
> so the safety of the swap rests entirely on the drain list in
> macb_context_swap_start() being exhaustive. The macb_tx_timeout() ->
> macb_tx_restart() path noted above is one reader that is not on it.
>
> Would doing the pointer swap under bp->lock (taken a few lines below
> anyway), and freeing old_ctx only after a synchronisation point, make that
> contract enforced rather than assumed?

No let's not do this. It is "taken a few lines below anyway" but
inbetween there's napi_enable() and it mustn't be done in bp->lock
critical section and the sequence ordering must stay as-is.

And doing the ctx write in a bp->lock critical section doesn't help
anything if in the same section we also write some MMIO registers
(taking the same example as below).

>
> > +
> > +	/* 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);
> > +
> > +	if (bp->link_up) {
> > +		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));
> > +	}
>
> [Severity: Medium]
> [Finding: gpt-5-6-sol-7-14]
> [Sources: gpt-5-6-sol, opus-5]
> The descriptors for new_ctx were written earlier, in macb_context_alloc() ->
> mog_init_rings(), and the ring base registers were handed over just above by
> macb_init_buffers(); RE|TE are then enabled here. Is a wmb() missing between
> the descriptor stores and those register writes?

Covered above.

[...]

> > +	spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +	if (bp->link_up) {
> > +		netif_tx_wake_all_queues(bp->netdev);
> > +
> > +		if (bp->eee_active)
> > +			macb_txp_lpi_initial_defer(bp);
> > +	}
>
> [Severity: Medium]
> [Finding: main-2-3]
> [Sources: opus-5]
> bp->link_up is documented in macb.h as "Redundant to netif_carrier_ok(), but
> set under bp->mac_cfg_lock", and it gates both IER/RE|TE re-arming and the
> TX wake here. Is macb_hresp_error_task() able to break that redundancy?

hresp_error is not even taking bp->lock to writel to the hardware. It
races against every other context in MACB. One we get in, the right
primitives will be there to fix this, as it is almost a context swap
operation (just with old_ctx free before new_ctx alloc).

[...]

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