Re: [PATCH net-next 06/11] net: macb: introduce macb_context struct for buffer management

Théo Lebrun
2026-04-02T14:11:28Z

On Thu Apr 2, 2026 at 1:22 PM CEST, Nicolai Buchwitz wrote:
> On 1.4.2026 18:39, Théo Lebrun wrote:
>> Whenever an operation requires buffer realloc, we close the interface,
>> update parameters and reopen. To improve reliability under memory
>> pressure, we should rather alloc new buffers, reconfigure HW and free
>> old buffers. This requires MACB to support having multiple "contexts"
>> in parallel.
>>
>> Introduce this concept by adding the macb_context struct, which owns
>> all
>> queue buffers and the parameters associated. We do not yet support
>> multiple contexts in parallel, because all functions access bp->ctx
>> (the currently active context) directly.
>>
>> Steps:
>>
>> - Introduce `struct macb_context` and its children `struct macb_rxq`
>> and `struct macb_txq`. Context fields are stolen from `struct macb`
>> and rxq/txq fields are from `struct macb_queue`.
>>
>> Making it two separate structs per queue simplifies accesses: we
>> grab
>> a txq/rxq local variable and access fields like txq->head instead of
>> queue->tx_head. It also anecdotally improves data locality.
>>
>> - macb_init_dflt() does not set bp->ctx->{rx,tx}_ring_size to default
>> values as ctx is not allocated yet. Instead, introduce
>> bp->configured_{rx,tx}_ring_size which get updated on user requests.
>>
>> - macb_open() starts by allocating bp->ctx. It gets freed in the
>> open error codepath or by macb_close().
>>
>> - Guided by compile errors, update all codepaths. Most diff is
>> changing
>> `queue->tx_*` to `txq->*` and `queue->rx_*` to `rxq->*`, with a new
>> local variable. Also rx_buffer_size / rx_ring_size / tx_ring_size
>> move from bp to bp->ctx.
>>
>> Introduce two helpers macb_tx|rx() functions to convert macb_queue
>> pointers.
>>
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>> drivers/net/ethernet/cadence/macb.h | 49 ++--
>> drivers/net/ethernet/cadence/macb_main.c | 442
>> ++++++++++++++++++-------------
>> 2 files changed, 296 insertions(+), 195 deletions(-)
>>
>
>> [...]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index d5023fdc0756..0f63d9b89c11 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
>> [...]
>
>> @@ -3596,14 +3677,15 @@ static void macb_get_regs(struct net_device
>> *netdev, struct ethtool_regs *regs,
>> void *p)
>> {
>> struct macb *bp = netdev_priv(netdev);
>> + struct macb_txq *txq = &bp->ctx->txq[0];
>
> bp->ctx is NULL when the interface is down. This will crash if
> ethtool -d is called while the interface is not running. Same
> issue below in macb_get_ringparam().

Agreed. Will check if context is alive and use default values otherwise.
Something like this for V2:

static void macb_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
void *p)
{
dma_addr_t tx_dma_tail = 0, tx_dma_head = 0;
struct macb *bp = netdev_priv(netdev);
unsigned int tail = 0, head = 0;
struct macb_txq *txq;
u32 *regs_buff = p;

regs->version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1))
| MACB_GREGS_VERSION;

if (bp->ctx) {
txq = &bp->ctx->txq[0];
tail = macb_tx_ring_wrap(bp->ctx, txq->tail);
head = macb_tx_ring_wrap(bp->ctx, txq->head);
tx_dma_tail = macb_tx_dma(&bp->queues[0], tail);
tx_dma_head = macb_tx_dma(&bp->queues[0], head);
}

regs_buff[0] = macb_readl(bp, NCR);
regs_buff[1] = macb_or_gem_readl(bp, NCFGR);
regs_buff[2] = macb_readl(bp, NSR);
regs_buff[3] = macb_readl(bp, TSR);
regs_buff[4] = macb_readl(bp, RBQP);
regs_buff[5] = macb_readl(bp, TBQP);
regs_buff[6] = macb_readl(bp, RSR);
regs_buff[7] = macb_readl(bp, IMR);

regs_buff[8] = tail;
regs_buff[9] = head;
regs_buff[10] = tx_dma_tail;
regs_buff[11] = tx_dma_head;

if (!(bp->caps & MACB_CAPS_USRIO_DISABLED))
regs_buff[12] = macb_or_gem_readl(bp, USRIO);
if (macb_is_gem(bp->caps))
regs_buff[13] = gem_readl(bp, DMACFG);
}

>
>> unsigned int tail, head;
>> u32 *regs_buff = p;
>>
>> regs->version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1))
>> | MACB_GREGS_VERSION;
>>
>> - tail = macb_tx_ring_wrap(bp, bp->queues[0].tx_tail);
>> - head = macb_tx_ring_wrap(bp, bp->queues[0].tx_head);
>> + tail = macb_tx_ring_wrap(bp, txq->tail);
>> + head = macb_tx_ring_wrap(bp, txq->head);
>>
>> regs_buff[0] = macb_readl(bp, NCR);
>> regs_buff[1] = macb_or_gem_readl(bp, NCFGR);
>> @@ -3682,8 +3764,8 @@ static void macb_get_ringparam(struct net_device
>> *netdev,
>> ring->rx_max_pending = MAX_RX_RING_SIZE;
>> ring->tx_max_pending = MAX_TX_RING_SIZE;
>>
>> - ring->rx_pending = bp->rx_ring_size;
>> - ring->tx_pending = bp->tx_ring_size;
>> + ring->rx_pending = bp->ctx->rx_ring_size;
>> + ring->tx_pending = bp->ctx->tx_ring_size;
>
> Same NULL ctx issue as above. This one could just read from
> bp->configured_{rx,tx}_ring_size instead.

Agreed with the fix, easy one.

Thanks!

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