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

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

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
https://bootlin.com