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

[Nicolai Buchwitz]

Hi Théo

On July 15, 2026 3:48:18 PM GMT+02:00, "Théo Lebrun" <theo.lebrun@xxxxxxxxxxx> wrote:
>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.

Sounds reasonable.

Reviewed-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>

Thanks,
Nicolai

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