Re: [PATCH net-next 10/11] net: macb: use context swapping in .set_ringparam()

[Nicolai Buchwitz]

On 1.4.2026 18:39, Théo Lebrun wrote:
> ethtool_ops.set_ringparam() is implemented using the primitive close /
> update ring size / reopen sequence. Under memory pressure this does not
> fly: we free our buffers at close and cannot reallocate new ones at
> open. Also, it triggers a slow PHY reinit.
>
> Instead, exploit the new context mechanism and improve our sequence to:
>  - allocate a new context (including buffers) first
>  - if it fails, early return without any impact to the interface
>  - stop interface
>  - update global state (bp, netdev, etc)
>  - pass buffer pointers to the hardware
>  - start interface
>  - free old context.
>
> The HW disable sequence is inspired by macb_reset_hw() but avoids
> (1) setting NCR bit CLRSTAT and (2) clearing register PBUFRXCUT.
>
> The HW re-enable sequence is inspired by macb_mac_link_up(), skipping
> over register writes which would be redundant (because values have not
> changed).
>
> The generic context swapping parts are isolated into helper functions
> macb_context_swap_start|end(), reusable by other operations 
> (change_mtu,
> set_channels, etc).
>
> Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 89 
> +++++++++++++++++++++++++++++---
>  1 file changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
> b/drivers/net/ethernet/cadence/macb_main.c
> index 42b19b969f3e..543356554c11 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2905,6 +2905,76 @@ static struct macb_context 
> *macb_context_alloc(struct macb *bp,
>  	return ctx;
>  }
>
> +static void macb_context_swap_start(struct macb *bp)
> +{
> +	struct macb_queue *queue;
> +	unsigned int q;
> +	u32 ctrl;
> +
> +	/* Disable software Tx, disable HW Tx/Rx and disable NAPI. */
> +
> +	netif_tx_disable(bp->netdev);
> +
> +	ctrl = macb_readl(bp, NCR);
> +	macb_writel(bp, NCR, ctrl & ~(MACB_BIT(RE) | MACB_BIT(TE)));
> +
> +	macb_writel(bp, TSR, -1);
> +	macb_writel(bp, RSR, -1);
> +
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		queue_writel(queue, IDR, -1);
> +		queue_readl(queue, ISR);
> +		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +			queue_writel(queue, ISR, -1);
> +	}
> +
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		napi_disable(&queue->napi_rx);
> +		napi_disable(&queue->napi_tx);
> +	}

tx_error_task, hresp_err_bh_work, and tx_lpi_work all dereference
bp->ctx and could race with the pointer swap in swap_end.
macb_close() cancels at least tx_lpi_work here. Should these be
flushed too?

> +}
> +
> +static void macb_context_swap_end(struct macb *bp,
> +				  struct macb_context *new_ctx)
> +{
> +	struct macb_context *old_ctx;
> +	struct macb_queue *queue;
> +	unsigned int q;
> +	u32 ctrl;
> +
> +	/* 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);
> +	}
> +
> +	if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
> +		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));
> +
> +	netif_tx_start_all_queues(bp->netdev);
> +
> +	/* Free old context. */
> +
> +	macb_free_consistent(old_ctx);

1. kfree(old_ctx) is missing. The context struct itself leaks on
   every swap.

2. macb_close() calls netdev_tx_reset_queue() for each queue.
   Shouldn't the swap do the same? BQL accounting will be stale
   after switching to a fresh context.

3. macb_configure_dma() is not called after the swap. For
   set_ringparam this is probably fine since rx_buffer_size
   does not change, but this becomes a problem in patch 11.

> [...]