Re: [PATCH net-next v4 11/15] net: macb: change function signatures to take contexts

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

On Sun Jul 19, 2026 at 12:46 PM CEST, Nicolai Buchwitz wrote:
> On 17.7.2026 21:48, Théo Lebrun wrote:
>> For parallel MACB context to start become a reality, many functions 
>> need
>> to stop operating on bp->ctx (the currently active context) and instead
>> work on a context they get passed. That context might be
>> (1) the new one that is getting allocated and initialised, or,
>> (2) the old one to be freed.
>> 
>> To reduce bug surface area, taint those functions to *only* take a
>> context `struct macb_context *ctx` and no `struct macb *bp`. That way,
>> no bug of using `bp->ctx` instead of `ctx` will ever occur.
>> 
>> We also convert functions that take a `struct macb_queue *queue` to
>> instead take `struct macb_context *ctx, unsigned int q`, with q
>> indexing ctx->txq[] and ctx->rxq[].
>> 
>> Full list:
>> 
>>    macb_adj_dma_desc_idx()
>>    macb_tx_ring_wrap()
>>    macb_tx_desc()
>>    macb_rx_ring_wrap()
>>    macb_rx_desc()
>>    macb_get_addr()
>>    gem_rx_refill()
>>    macb_init_rx_ring()
>>    gem_free_rx_buffers()
>>    macb_free_rx_buffers()
>>    macb_tx_ring_size_per_queue()
>>    macb_rx_ring_size_per_queue()
>>    macb_free()
>>    gem_alloc_rx_buffers()
>>    macb_alloc_rx_buffers()
>>    macb_alloc()
>>    gem_init_rx_ring()
>>    gem_init_rings()
>>    macb_init_rings()
>> 
>> Note about gem_rx_refill(): it ends with a netdev_vdbg() that prints 
>> the
>> queue pointer. Change to print the queue index because we do not have
>> access to the queue anymore.
>> 
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.h      |   7 +-
>>  drivers/net/ethernet/cadence/macb_main.c | 398 
>> ++++++++++++++++---------------
>>  2 files changed, 215 insertions(+), 190 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb.h 
>> b/drivers/net/ethernet/cadence/macb.h
>> index c551d7db8ebe..ac2f2d8065d7 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -1196,11 +1196,12 @@ static const struct gem_statistic 
>> queue_statistics[] = {
>> 
>>  struct macb;
>>  struct macb_queue;
>> +struct macb_context;
>> 
>>  struct macb_or_gem_ops {
>> -	int	(*mog_alloc_rx_buffers)(struct macb *bp);
>> -	void	(*mog_free_rx_buffers)(struct macb *bp);
>> -	void	(*mog_init_rings)(struct macb *bp);
>> +	int	(*mog_alloc_rx_buffers)(struct macb_context *ctx);
>> +	void	(*mog_free_rx_buffers)(struct macb_context *ctx);
>> +	void	(*mog_init_rings)(struct macb_context *ctx);
>>  	int	(*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
>>  			  int budget);
>>  };
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 7574418d5094..d396a307310b 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
>> [...]
>
>> @@ -5083,7 +5107,7 @@ static int at91ether_start(struct macb *bp)
>> 
>>  	addr = rxq->buffers_dma;
>>  	for (i = 0; i < AT91ETHER_MAX_RX_DESCR; i++) {
>> -		desc = macb_rx_desc(queue, i);
>> +		desc = macb_rx_desc(bp->ctx, 0, i);
>
> AFAIU at91ether_open() doesn't set bp->ctx->info at this point in the
> series, so with CONFIG_MACB_USE_HWSTAMP=y this should oops on ifup:
>
>      macb_rx_desc()
>        macb_adj_dma_desc_idx()
>          macb_dma_ptp(ctx->info)   -> NULL deref
>
> The next patch adds the missing assignment to at91ether_open(), so
> only bisection is affected. Maybe move that line here or into patch 9?

Yes, IDK why/how I landed on this.

⟩ # This looks much more sensible.
⟩ git log -L:at91ether_open:drivers/net/ethernet/cadence/macb_main.c net-next/main..macb-context

net: macb: unify device pointer naming convention
net: macb: unify variable naming convention in at91ether functions
=> variable renames

net: macb: introduce macb_context struct for buffer management
=> adds allocation with `bp->ctx = kzalloc_obj(...)` and cleanup on error

net: macb: make `struct macb` subset reachable from macb_context struct
=> adds `bp->ctx->info = &bp->info;` when introducing info

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com