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

Théo Lebrun
2026-04-03T09:04:01Z

On Thu Apr 2, 2026 at 6:31 PM CEST, Théo Lebrun wrote:
> 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!

After some sleep, I feel like my message was a bit rough. To clarify
what I plan for V2:
- grab bp->lock on swap to protect us against some of #1 userspace and
all of #6 IRQ.
- disabling #2 NAPI on swap is already done
- disable all three BH features on swap

That will not fix everything listed above.

On top, we should:
- check/revise our locking strategy for almost all codepaths,
- check all BH features are disabled and blocked upon in the right
codepaths,
- in many bp->lock critical section, we should early exit if !bp->ctx.

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