[PATCH net v3] net: axienet: bound TX completion cleanup by the NAPI budget

[PATCH net v3] net: axienet: bound TX completion cleanup by the NAPI budget

[Sagi Maimon]

axienet_tx_poll() passes lp->tx_bd_num to axienet_free_tx_chain() as
@nr_bds, and @budget is only forwarded to napi_consume_skb() as its
bulk-free hint.  Nothing limits the cleanup loop to the NAPI budget, so
the number of packets returned is bounded by the TX ring size rather
than by the budget, and the poll can report more work than it was
given:

  eth0: NAPI poll function axienet_tx_poll+0x0/0x180 [xilinx_emac]
        returned 96, exceeding its budget of 64.

Returning more than the budget breaks the NAPI contract.  It also makes
the "packets < budget" test in axienet_tx_poll() false, so
napi_complete_done() is skipped and TX completion interrupts are not
re-enabled on that pass.  NAPI reschedules the poll, so this recovers,
but the accounting is wrong either way.

In steady state fewer descriptors complete per poll than the budget
allows, which is why this is rarely observed.  Triggering it needs more
than @budget completions outstanding at once - for example when TX
completion interrupts have not been taken for a while and a full ring is
reclaimed in one go.

Stop the loop once the budget is spent.  cur_p->skb is only set on a
packet's last descriptor, so breaking there never leaves a packet
half-freed.  The check is skipped on the @force path, which cleans up
after a DMA mapping failure.

A budget of 0 is a separate case.  netpoll calls napi->poll() with a
budget of 0 to reclaim the TX path only, and expects no work to be
reported.  Treat 0 as no limit in the cleanup loop so the ring is still
drained, and have axienet_tx_poll() report no work for it.  Returning
the reclaimed count would trip the WARN_ONCE() in poll_one_napi(),
which the unbounded loop could already do before this change.

Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes
with this change applied.  Neither an over-budget poll nor the netpoll
path was exercised in that test.

Fixes: 9e2bc267e780 ("net: axienet: Use NAPI for TX completion path")
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---

Notes:
    Changes in v3:
    - Report no work for a budget of 0: axienet_tx_poll() now returns
      "budget ? packets : 0", so netpoll cannot trip the WARN_ONCE() in
      poll_one_napi() (Sashiko).
    - Reword the @budget kernel-doc and the in-loop comment to cover both
      uses of a budget of 0 (Sashiko).
    - Drop the wrong claim that a budget of 0 matters to the @force callers
      (Sashiko).
    - Add a hardware test note, and the Assisted-by: tag that v1 and v2
      omitted.
    - v2: https://lore.kernel.org/netdev/20260917115657.20697-1-maimon.sagi@gmail.com/
    
    Changes in v2:
    - Treat a budget of 0 as no limit, so that netpoll still drains the TX
      ring; v1 reclaimed nothing for it (Sashiko).
    - v1: https://lore.kernel.org/netdev/20260914114821.55503-1-maimon.sagi@gmail.com/

 .../net/ethernet/xilinx/xilinx_axienet_main.c | 22 +++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 782f903d318f..7fd77f8cb57c 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -772,7 +772,11 @@ static int axienet_device_reset(struct net_device *ndev)
  * @force:	Whether to clean descriptors even if not complete
  * @sizep:	Pointer to a u32 accumulating the total byte count of
  *		completed packets (using skb->len). Ignored if NULL.
- * @budget:	NAPI budget (use 0 when not called from NAPI poll)
+ * @budget:	NAPI budget, or 0 when not called from NAPI poll; also passed
+ *		to napi_consume_skb(). When @force is false, cleanup stops once
+ *		@budget completed packets have been freed. A budget of 0 means
+ *		no limit: netpoll polls with it to drain the TX ring, and
+ *		axienet_tx_poll() then reports no work.
  *
  * Would either be called after a successful transmit operation, or after
  * there was an error when setting up the chain.
@@ -788,6 +792,16 @@ static int axienet_free_tx_chain(struct axienet_local *lp, u32 first_bd,
 	dma_addr_t phys;
 
 	for (i = 0; i < nr_bds; i++) {
+		/* A NAPI poll must not return more than its budget.  Stop on a
+		 * packet boundary once it is spent - cur_p->skb is only set on
+		 * a packet's last descriptor, so no packet is left half-freed.
+		 * A zero budget means no limit: netpoll polls with a budget of
+		 * 0 to reclaim the TX path, so the ring must still be drained;
+		 * axienet_tx_poll() reports no work to it.
+		 */
+		if (!force && budget && packets >= budget)
+			break;
+
 		cur_p = &lp->tx_bd_v[(first_bd + i) % lp->tx_bd_num];
 		status = cur_p->status;
 
@@ -1027,7 +1041,11 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
 		axienet_dma_out32(lp, XAXIDMA_TX_CR_OFFSET, lp->tx_dma_cr);
 		spin_unlock_irq(&lp->tx_cr_lock);
 	}
-	return packets;
+
+	/* netpoll polls with a budget of 0 to reclaim the TX path and expects
+	 * no work to be reported; see poll_one_napi().
+	 */
+	return budget ? packets : 0;
 }
 
 /**

base-commit: 879e280b8486d4612ad1aa050d6fada2dd80cf1c
-- 
2.47.0

Re: [PATCH net v3] net: axienet: bound TX completion cleanup by the NAPI budget

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new budget break in axienet_free_tx_chain() (line 801)
  leaves completed descriptors in lp->tx_bd_v.
- [Low] Wrong Fixes: tag.
- [Low] The kernel-doc for axienet_tx_poll() no longer matches its
  behaviour.

--

net: axienet: bound TX completion cleanup by the NAPI budget

This patch stops the TX completion loop in axienet_free_tx_chain() once
the NAPI budget has been used up. The check is skipped on the force path.
A budget of 0 is treated as no limit, and axienet_tx_poll() now returns
0 for a budget of 0, so the netpoll WARN_ONCE() in poll_one_napi() is not
triggered.

> Fixes: 9e2bc267e780 ("net: axienet: Use NAPI for TX completion path")

[Severity: Low]
Is this the right commit for the Fixes: tag?

In 9e2bc267e780, axienet_tx_poll() passed budget as nr_bds, and the
helper returned i, the number of descriptors walked:

    axienet_free_tx_chain(lp, lp->tx_bd_ci, budget, false, &size, budget)

That return value is bounded by budget. With a netpoll budget of 0 the
loop runs zero times, so poll_one_napi() could not warn either.

The call that passes lp->tx_bd_num and returns a packet count comes from
5a6caa2cfabb ("net: xilinx: axienet: Fix packet counting"). git blame on
the baseline points there:

    packets = axienet_free_tx_chain(lp, lp->tx_bd_ci, lp->tx_bd_num, false,
                                    &size, budget);

Should the tag be the following instead?

    Fixes: 5a6caa2cfabb ("net: xilinx: axienet: Fix packet counting")

The sentence "which the unbounded loop could already do before this
change" also describes the loop added by 5a6caa2cfabb.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 782f903d318f3..7fd77f8cb57c6 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

[ ... ]

> @@ -788,6 +792,16 @@ static int axienet_free_tx_chain(struct axienet_local *lp, u32 first_bd,
>  	dma_addr_t phys;
>  
>  	for (i = 0; i < nr_bds; i++) {
> +		/* A NAPI poll must not return more than its budget.  Stop on a
> +		 * packet boundary once it is spent - cur_p->skb is only set on
> +		 * a packet's last descriptor, so no packet is left half-freed.
> +		 * A zero budget means no limit: netpoll polls with a budget of
> +		 * 0 to reclaim the TX path, so the ring must still be drained;
> +		 * axienet_tx_poll() reports no work to it.
> +		 */
> +		if (!force && budget && packets >= budget)
> +			break;

[Severity: Medium]
Can this break leak the TX skbs and DMA mappings still in the ring when
the interface is brought down?

Completed descriptors left behind after the break still have cur_p->skb
set and a live dma_map_single() mapping. The commit message relies on
NAPI rescheduling the poll to reclaim them. That does not happen while
napi_disable() is pending.

axienet_stop() calls napi_disable(&lp->napi_tx) while TX DMA is still
running. If the TX poll finds more than budget completions, it returns
budget, and __napi_poll() takes this branch:

net/core/dev.c:__napi_poll() {
    ...
	if (unlikely(napi_disable_pending(n))) {
		napi_complete(n);
		return work;
	}
    ...
}

Nothing polls again after that, and axienet_stop() continues:

axienet_stop()
  napi_disable(&lp->napi_tx)
  axienet_dma_stop(lp)
  free_irq(lp->tx_irq, ndev)
  axienet_dma_bd_release(ndev)
    dma_free_coherent(lp->dev,
                      sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
                      lp->tx_bd_v, lp->tx_bd_p)

axienet_dma_bd_release() does not unmap the TX buffers or free
tx_bd_v[i].skb. The only TX ring skb cleanup loop is in
axienet_dma_err_handler(), and the normal stop path does not run it. The
remaining skbs are never freed. Their sk_wmem_alloc charge is never
released, and their streaming DMA mappings are leaked.

Before this patch, a poll running during napi_disable() drained every
descriptor completed by then. ndo_stop already had no TX reclaim for
descriptors still in flight, so that gap predates this patch. The budget
break adds a new way to hit it.

Would it work to drain the ring without the budget bound and clamp only
the return value? Another option is to reclaim all TX descriptors in
axienet_stop() or axienet_dma_bd_release().

[ ... ]

> @@ -1027,7 +1041,11 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
>  		axienet_dma_out32(lp, XAXIDMA_TX_CR_OFFSET, lp->tx_dma_cr);
>  		spin_unlock_irq(&lp->tx_cr_lock);
>  	}
> -	return packets;
> +
> +	/* netpoll polls with a budget of 0 to reclaim the TX path and expects
> +	 * no work to be reported; see poll_one_napi().
> +	 */
> +	return budget ? packets : 0;
>  }

[Severity: Low]
Should the kernel-doc above axienet_tx_poll() be updated too? It still
says:

 * @budget:	Max number of TX packets to process.
 *
 * Return: Number of TX packets processed.

With this patch a budget of 0 means no limit. For that case the function
returns 0 even after it has reclaimed packets and passed them to
netdev_completed_queue() and the u64 stats.

The new @budget text for axienet_free_tx_chain() also contradicts itself.
It first says 0 is used "when not called from NAPI poll", then says
"netpoll polls with it". The netpoll call is a NAPI ->poll() call, since
poll_one_napi() calls napi->poll(napi, 0).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924135052.185129-1-maimon.sagi%40gmail.com

Re: [PATCH net v3] net: axienet: bound TX completion cleanup by the NAPI budget

[Paolo Abeni]

On 9/24/26 15:50, Sagi Maimon wrote:
> axienet_tx_poll() passes lp->tx_bd_num to axienet_free_tx_chain() as
> @nr_bds, and @budget is only forwarded to napi_consume_skb() as its
> bulk-free hint.  Nothing limits the cleanup loop to the NAPI budget, so
> the number of packets returned is bounded by the TX ring size rather
> than by the budget, and the poll can report more work than it was
> given:
> 
>    eth0: NAPI poll function axienet_tx_poll+0x0/0x180 [xilinx_emac]
>          returned 96, exceeding its budget of 64.
> 
> Returning more than the budget breaks the NAPI contract.  It also makes
> the "packets < budget" test in axienet_tx_poll() false, so
> napi_complete_done() is skipped and TX completion interrupts are not
> re-enabled on that pass.  NAPI reschedules the poll, so this recovers,
> but the accounting is wrong either way.
> 
> In steady state fewer descriptors complete per poll than the budget
> allows, which is why this is rarely observed.  Triggering it needs more
> than @budget completions outstanding at once - for example when TX
> completion interrupts have not been taken for a while and a full ring is
> reclaimed in one go.
> 
> Stop the loop once the budget is spent.  

Napi can process as much TX descriptor as available, even above `budget`
see:

https://elixir.bootlin.com/linux/v7.2.8/source/Documentation/networking/napi.rst#L68

The solution would be capping axienet_tx_poll() return value to `budget`.

/P

Re: [PATCH net v3] net: axienet: bound TX completion cleanup by the NAPI budget

[Sagi Maimon]

On Tue, Sep 29, 2026 at 4:22 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 9/24/26 15:50, Sagi Maimon wrote:
> > axienet_tx_poll() passes lp->tx_bd_num to axienet_free_tx_chain() as
> > @nr_bds, and @budget is only forwarded to napi_consume_skb() as its
> > bulk-free hint.  Nothing limits the cleanup loop to the NAPI budget, so
> > the number of packets returned is bounded by the TX ring size rather
> > than by the budget, and the poll can report more work than it was
> > given:
> >
> >    eth0: NAPI poll function axienet_tx_poll+0x0/0x180 [xilinx_emac]
> >          returned 96, exceeding its budget of 64.
> >
> > Returning more than the budget breaks the NAPI contract.  It also makes
> > the "packets < budget" test in axienet_tx_poll() false, so
> > napi_complete_done() is skipped and TX completion interrupts are not
> > re-enabled on that pass.  NAPI reschedules the poll, so this recovers,
> > but the accounting is wrong either way.
> >
> > In steady state fewer descriptors complete per poll than the budget
> > allows, which is why this is rarely observed.  Triggering it needs more
> > than @budget completions outstanding at once - for example when TX
> > completion interrupts have not been taken for a while and a full ring is
> > reclaimed in one go.
> >
> > Stop the loop once the budget is spent.
>
> Napi can process as much TX descriptor as available, even above `budget`
> see:
>
> https://elixir.bootlin.com/linux/v7.2.8/source/Documentation/networking/napi.rst#L68
>
> The solution would be capping axienet_tx_poll() return value to `budget`.

Agreed, and v4 does exactly that: the ring is still reclaimed in full,
and axienet_tx_poll() returns min(packets, budget), which also gives 0
for netpoll's budget of 0.  It is posted as "net: axienet: cap the TX
poll return value at the NAPI budget".

Thanks,
Sagi

pw-bot: cr

>
> /P
>