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

Théo Lebrun
2026-04-02T16:31:53Z

On Thu Apr 2, 2026 at 1:29 PM CEST, Nicolai Buchwitz wrote:
> 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@bootlin.com>
>> ---
>> 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?

This is a large topic! While trying to find a solution as part of this
series I am noticing many race conditions. With this context series we
worsen some (by introducing bp->ctx NULL ptr dereference).

Let's start by identifying all schedule-able contexts involved:
- #1 any request from userspace, too many callbacks to list
- #2 NAPI softirq or kthread context, macb_{rx,tx}_poll()
- #3 bp->hresp_err_bh_work / macb_hresp_error_task()
- #4 bp->tx_lpi_work / macb_tx_lpi_work_fn()
- #5 queue->tx_error_task / macb_tx_error_task()
- #6 IRQ context, macb_interrupt()

Some race conditions:

- #1 macb_close() doesn't cancel & wait upon #3 hresp_err_bh_work.
They could race, especially as #3 doesn't grab bp->lock. One race
example: #3 BP HRESP starts the interface after it has been closed
and buffers freed. RBQP/TBQP are not reset so MACB would occur
memory corruption on Rx and transmit memory content.

- #1 macb_close() doesn't cancel & wait upon #5 tx_error_task. #5 does
grab bp->lock but that doesn't make it much safer. One race example:
same as above, restart of interface with ghost ring buffers.

- #3 hresp_err_bh_work could collide with anything as it does no
locking, especially #1 (xmit for example) or #2 (NAPI). It is less
likely to collide with #6 IRQ because it starts by disabling those
but there is a possibility of the IRQ having already triggered and
macb_interrupt() already running in parallel of
macb_hresp_error_task().

- #5 queue->tx_error_task writes to Tx head/tail inside bp->lock.
#1 macb_start_xmit() modifies those too, but inside
queue->tx_ptr_lock. Oops. There probably are other places modifying
head/tail or any other Tx queue value without queue->tx_ptr_lock.

- #5 macb_tx_error_task() tries to gently disable TX but if it
times-out then it uses the global switch (TE field in NCR
register). That sounds racy with #2 NAPI that doesn't grab bp->lock
and would probably break if the interface is shutdown under its
feet.

I don't see much more. To fix all that, someone ought to exhaustively go
through all tasks (#1-6 above) & all shared data and reason one by one.
Who will be that someone? ;-) But that sounds pretty unrelated to the
series at hand, no?

I'd agree that some locking of bp->lock around the swap operation would
improve the series, and I'll add that in V2 for sure!

>
>> +}
>> +
>> +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.

Agreed.

> 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.

I explicitely left that out as I thought DQL would benefit from keeping
past context of the traffic. But indeed as we start afresh from a new
set of buffers we should reset DQL. fbnic, pointed out as an good
example by Jakub recently, does that.

>
> 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.

Indeed, I had missed it took bp->ctx->rx_buffer_size as a parameter.
Will fix.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering