Re: [PATCH net v3] net: axienet: free outstanding TX buffers in axienet_dma_bd_release()

[Joe Damato]

On Sun, Oct 04, 2026 at 11:37:59AM +0300, Sagi Maimon wrote:
> axienet_dma_bd_release() walks the RX ring to unmap and free every
> receive buffer before releasing it, but frees the TX descriptor ring
> with dma_free_coherent() alone.  Any descriptor that
> axienet_free_tx_chain() had not yet reclaimed still holds its skb and
> its streaming DMA mapping, and both are lost.
> 
> axienet_stop() disables TX NAPI and stops the DMA engine before calling
> it, so nothing reclaims those descriptors afterwards.  Bringing the
> interface down while frames are in flight therefore leaks up to
> lp->tx_bd_num skbs and mappings each time.
> 
> Walk the TX ring the way axienet_dma_err_handler() already does: unmap
> every descriptor whose cntrl is still set - axienet_free_tx_chain()
> clears it on reclaim - and free any skb still attached, as a drop.
> This relies on axienet_stop() having stopped the DMA engine first, as
> the RX walk in the same function already does.
 
[...]

> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> Assisted-by: LLM sparse
> Signed-off-by: Sagi Maimon <maimon.sagi@xxxxxxxxx>
> ---
> 
> Notes:
>     Changes in v3:
>     - Clear tx_bd_v and rx_bd_v after freeing the rings.  v2 only caught a
>       NULL tx_bd_v from a first open; after a close followed by a failed
>       reset the walk would have read the freed ring (Sashiko).
>     - Reword the comment on the skb free: a descriptor can complete after
>       TX NAPI was disabled, so "never transmitted" was not always true
>       (Sashiko).
>     - Say in the commit message which hardware the test ran on.
>     - Kept Jacob's Reviewed-by, as the changes are small; please say if
>       that is not OK.
>     - The hardware test is v1's.  The changes since only affect the
>       failed-reset paths, which it did not exercise, and how the freed skbs
>       are accounted.
>     - v2: https://lore.kernel.org/netdev/20260930133851.663023-1-maimon.sagi@xxxxxxxxx/

[...]

> 
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 26 ++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 09443623a3e2..c88c671f8b2d 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -186,11 +186,34 @@ static void axienet_dma_bd_release(struct net_device *ndev)

[...]

> +	lp->tx_bd_v = NULL;
>  
>  	if (!lp->rx_bd_v)
>  		return;
> @@ -221,6 +244,7 @@ static void axienet_dma_bd_release(struct net_device *ndev)
>  			  sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
>  			  lp->rx_bd_v,
>  			  lp->rx_bd_p);
> +	lp->rx_bd_v = NULL;
>  }

The added null writes makes me think that centralizing this code and using it
from both axienet_dma_err_handler and axienet_dma_bd_release (instead of
repeating it) is a good idea like I mentioned in the last post.

The code seems right tho even tho I don't like duplicating the logic.

Reviewed-by: Joe Damato <joe@xxxxxxx>