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

[Sagi Maimon]

On Tue, Oct 6, 2026 at 2:41 AM Joe Damato <joe@xxxxxxx> wrote:
>
> 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>

Thanks for the review, Joe.
Agreed on the helper.  As on v2, I kept the fix minimal for net, and
will send the helper to net-next once this has reached it, with both
axienet_dma_bd_release() and axienet_dma_err_handler() using it.
Thanks,
Sagi