Re: [PATCH net-next v3 06/15] net: macb: allocate tieoff descriptor once across device lifetime

[Théo Lebrun]

On Mon Jul 13, 2026 at 2:00 PM CEST, Théo Lebrun wrote:
> On Thu Jul 2, 2026 at 12:54 PM CEST, Nicolai Buchwitz wrote:
>> On 1.7.2026 17:59, Théo Lebrun wrote:
>>> The tieoff descriptor is a RX DMA descriptor ring of size one. It gets
>>> configured onto queues for Wake-on-LAN during system-wide suspend when
>>> hardware does not support disabling individual queues
>>> (MACB_CAPS_QUEUE_DISABLE).
>>> 
>>> MACB/GEM driver allocates it alongside the main RX ring
>>> inside macb_alloc() at open. Free is done by macb_free() at close.
>>> 
>>> Change to allocate once at probe and free on probe failure or device
>>> removal. This makes the tieoff descriptor lifetime much longer,
>>> avoiding repeating coherent buffer allocation on each open/close cycle.
>>> 
>>> Main benefit: we dissociate its lifetime from the main ring's lifetime.
>>> That way there is less work to be doing on resources (re)alloc. This
>>> currently happens on close/open, but will soon also happen on context
>>> swap operations (set_ringparam, change_mtu, set_channels, etc).
>>> 
>>> Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>>> ---
>>>  drivers/net/ethernet/cadence/macb_main.c | 75 
>>> +++++++++++++++++---------------
>>>  1 file changed, 41 insertions(+), 34 deletions(-)
>>> 
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>>> b/drivers/net/ethernet/cadence/macb_main.c
>>> index 8b52122bc134..951a7f080225 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>
>>> [...]
>>
>>>  static void macb_init_rings(struct macb *bp)
>>> @@ -2832,8 +2801,6 @@ static void macb_init_rings(struct macb *bp)
>>>  	bp->queues[0].tx_head = 0;
>>>  	bp->queues[0].tx_tail = 0;
>>>  	desc->ctrl |= MACB_BIT(TX_WRAP);
>>> -
>>> -	macb_init_tieoff(bp);
>>>  }
>>> 
>>>  static void macb_reset_hw(struct macb *bp)
>>> @@ -5518,6 +5485,38 @@ static int eyeq5_init(struct platform_device 
>>> *pdev)
>>>  	return ret;
>>>  }
>>> 
>>> +static int macb_alloc_tieoff(struct macb *bp)
>>> +{
>>> +	/* Tieoff is a workaround in case HW cannot disable queues, for PM. 
>>> */
>>> +	if (bp->caps & MACB_CAPS_QUEUE_DISABLE)
>>> +		return 0;
>>
>> Before, the tieoff was allocated in macb_alloc(), which the at91ether 
>> path
>> never called. Now it's allocated from macb_probe() for all variants, 
>> gated only
>> on MACB_CAPS_QUEUE_DISABLE, so EMAC gets a coherent descriptor it never 
>> uses.
>>
>> Add MACB_CAPS_MACB_IS_EMAC to the if statement?
>
> Clearly. That EMAC distinction keeps being annoying.

Well well well. Under AT91 with WOL active there is nothing preventing
the tieoff from being used in macb_suspend(). Meaning the tieoff is
being used zero-initialised. Not advisable.

So I withdraw my previous comment: we won't shield macb_alloc_tieoff()
from IS_EMAC and maybe it will fix a bug.

Thanks,

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