[PATCH net-next V3] net: axienet: Clear stale AXI DMA TX/RX status before re-enabling interrupts

[PATCH net-next V3] net: axienet: Clear stale AXI DMA TX/RX status before re-enabling interrupts

[Kusuma Vasana]

The AXI DMA interrupt line is level-sensitive: it asserts whenever
the IOC (XAXIDMA_IRQ_IOC_MASK) or DELAY (XAXIDMA_IRQ_DELAY_MASK) bits
in the status register (XAXIDMA_TX_SR_OFFSET / XAXIDMA_RX_SR_OFFSET)
are set and their corresponding enable bits in the control register
are active.

During TX/RX, interrupts are disabled in the control register while
NAPI runs. Completions arriving in this window cause hardware to latch
IOC/DELAY into the status register regardless of the control register
mask state. After NAPI completion, re-enabling interrupts immediately
re-asserts the IRQ line due to these stale status register bits, even
when no new work is pending.

This results in a stale interrupt and redundant NAPI poll cycle with
no new work pending, causing unnecessary CPU processing.

In the initial driver, the status register was cleared after polling
all packets, which naturally consumed any status accumulated during
processing. In later versions of the driver, status register clearing
was moved to the ISR before polling begins, leaving no mechanism to
clear status bits that arrive during the NAPI poll window.

Clear the status register IOC/DELAY bits before re-enabling interrupts
and recheck the next BD to catch race-window completions. Both are
skipped on budget exhaustion. On the RX side, both are additionally
skipped on refill failure to leave the level-sensitive IRQ armed so the
poll is rescheduled on the next hardware completion.

Signed-off-by: Kusuma Vasana <kusuma.vasana@amd.com>
---
Targeting net-next with no Fixes tag: the extra poll cycle is a
performance inefficiency, not a functional defect.

Changes in v3:
- Updated the commit description.
- Restricted TX/RX status-register clear and completion recheck to the
  NAPI completion path, with RX only when refill succeeds.
- Rechecked descriptor completion after status-register clear to handle
  race-window completions.

Changes in v2:
-Added net-next prefix in the subject
-Updated the commit description

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

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index fcf517069d16..4816870e8502 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1018,6 +1018,16 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
 			netif_wake_queue(ndev);
 	}
 
+	/* Clear stale IOC/DELAY bits and re-check for race-window completions.
+	 * Skipped on budget exhaustion.
+	 */
+	if (packets < budget) {
+		axienet_dma_out32(lp, XAXIDMA_TX_SR_OFFSET,
+				  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
+		if (lp->tx_bd_v[lp->tx_bd_ci].status & XAXIDMA_BD_STS_COMPLETE_MASK)
+			return budget;
+	}
+
 	if (packets < budget && napi_complete_done(napi, packets)) {
 		/* Re-enable TX completion interrupts. This should
 		 * cause an immediate interrupt if any TX packets are
@@ -1301,6 +1311,18 @@ static int axienet_rx_poll(struct napi_struct *napi, int budget)
 	if (tail_p)
 		axienet_dma_out_addr(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p);
 
+	/* Clear stale IOC/DELAY bits and re-check for race-window completions.
+	 * Skipped on budget exhaustion and on refill failure (cur_p->skb ==
+	 * NULL) to leave the level-sensitive IRQ armed so the poll is
+	 * rescheduled on the next hardware completion.
+	 */
+	if (packets < budget && cur_p->skb) {
+		axienet_dma_out32(lp, XAXIDMA_RX_SR_OFFSET,
+				  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
+		if (cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)
+			return budget;
+	}
+
 	if (packets < budget && napi_complete_done(napi, packets)) {
 		if (READ_ONCE(lp->rx_dim_enabled)) {
 			struct dim_sample sample = {
-- 
2.43.0

Re: [PATCH net-next V3] net: axienet: Clear stale AXI DMA TX/RX status before re-enabling interrupts

[netdev-bot+sashiko]

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

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

- [High] Unordered write-1-to-clear of the AXI DMA status register
  followed by a descriptor read can silently swallow a DMA completion.
- [Low] Both new blocks can `return budget` while `packets` is less than
  budget (possibly zero), which contradicts the unchanged kernel-doc of…
- [Low] The new RX comment at
  drivers/net/ethernet/xilinx/xilinx_axienet_main.c:1314-1318 states…

--

net: axienet: Clear stale AXI DMA TX/RX status before re-enabling interrupts

This adds a write-1-to-clear of the IOC/DELAY bits in the AXI DMA TX/RX
status registers at the end of axienet_tx_poll() and axienet_rx_poll(),
followed by a re-check of the next buffer descriptor, in order to avoid
one redundant NAPI poll caused by status latched while interrupts were
masked.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..ae43b9744cb90 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1018,6 +1018,16 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
>  			netif_wake_queue(ndev);
>  	}
>  
> +	/* Clear stale IOC/DELAY bits and re-check for race-window completions.
> +	 * Skipped on budget exhaustion.
> +	 */
> +	if (packets < budget) {
> +		axienet_dma_out32(lp, XAXIDMA_TX_SR_OFFSET,
> +				  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
> +		if (lp->tx_bd_v[lp->tx_bd_ci].status & XAXIDMA_BD_STS_COMPLETE_MASK)
> +			return budget;
> +	}

[Severity: High]
Can this sequence lose a DMA completion?  The status-register clear is a
posted write with nothing after it to flush or order it:

drivers/net/ethernet/xilinx/xilinx_axienet.h:
static inline void axienet_dma_out32(struct axienet_local *lp,
				     off_t reg, u32 value)
{
	iowrite32(value, lp->dma_regs + reg);
}

The descriptor status that follows is a plain load from
dma_alloc_coherent() memory, so on arm/arm64 the Device-nGnRE store is not
ordered against the subsequent Normal-memory load (iowrite32() only emits
__iowmb() before the store).

That allows this interleaving:

  CPU				DMA engine
  ----				----------
  write IOC|DELAY to SR		(write still in flight)
  read tx_bd_v[tx_bd_ci].status
    -> not COMPLETE
				descriptor writeback, latch IOC/DELAY
  (queued W1C lands, wipes IOC/DELAY)
  napi_complete_done()
  write lp->tx_dma_cr

after which IOC/DELAY are enabled in the control register but the status
register is clean, so the level-sensitive line stays de-asserted while a
COMPLETE descriptor sits in the ring.

Before this patch there was no such window: the status word was acked only
in axienet_tx_irq()/axienet_rx_irq() before NAPI ran:

	axienet_dma_out32(lp, XAXIDMA_TX_SR_OFFSET, status);

so anything the hardware latched during the poll window kept the line
asserted and produced exactly the extra poll this patch removes.

The RX hunk below has the same ack-then-scan shape, and it is issued right
after the XAXIDMA_RX_TDESC_OFFSET doorbell, i.e. when the engine is most
likely to complete a descriptor concurrently.  On TX the skb is not freed
and netdev_completed_queue()/BQL accounting stays skewed until some later
completion; note that axienet_netdev_ops has no .ndo_tx_timeout, and
netdev_watchdog_up() returns early without one, so there is no software
watchdog fallback.  On RX the frame is delivered late, or on an idle link
not until the peer retransmits.

Would reading back XAXIDMA_TX_SR_OFFSET / XAXIDMA_RX_SR_OFFSET (or
otherwise ordering the clear ahead of the descriptor scan) close this?

[Severity: Low]
This return can hand back budget while packets is smaller than budget, and
possibly zero.  The kernel-doc above axienet_tx_poll() still says:

 * Return: Number of TX packets processed.

and the one above axienet_rx_poll() says:

 * Return: Number of RX packets processed.

The commit message describes the change only as

    "recheck the next BD to catch race-window completions"

without mentioning that the poll now returns a sentinel work value.
__napi_poll() treats work == weight specially:

net/core/dev.c:__napi_poll() {
	...
	if (likely(work < weight))
		return work;
	...
	gro_flush_normal(&n->gro, HZ >= 1000);
	...
	*repoll = true;
}

and net_rx_action() charges the returned value against netdev_budget with
budget -= napi_poll(n, &repoll), so a poll that reclaimed nothing consumes
a full weight of the softirq budget.  On RX the same path also skips the
net_dim() sample in the napi_complete_done() branch.

Returning budget does look like the required idiom here, since returning a
smaller value without calling napi_complete_done() would leave the instance
scheduled but off the poll list.  Could the kernel-doc and changelog be
updated to describe the sentinel return instead?

>  	if (packets < budget && napi_complete_done(napi, packets)) {
>  		/* Re-enable TX completion interrupts. This should
>  		 * cause an immediate interrupt if any TX packets are
> @@ -1301,6 +1311,18 @@ static int axienet_rx_poll(struct napi_struct *napi, int budget)
>  	if (tail_p)
>  		axienet_dma_out_addr(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p);
>  
> +	/* Clear stale IOC/DELAY bits and re-check for race-window completions.
> +	 * Skipped on budget exhaustion and on refill failure (cur_p->skb ==
> +	 * NULL) to leave the level-sensitive IRQ armed so the poll is
> +	 * rescheduled on the next hardware completion.
> +	 */

[Severity: Low]
Is the "leave the level-sensitive IRQ armed so the poll is rescheduled on
the next hardware completion" part of this comment accurate?

axienet_rx_irq() already writes back the whole status word before NAPI is
scheduled:

	axienet_dma_out32(lp, XAXIDMA_RX_SR_OFFSET, status);

so unless the hardware latched a new IOC/DELAY during the poll window there
is no bit left to keep the line asserted, and the skip is a no-op rather
than something that arms the interrupt.

When a bit was latched during the window, the following block is not gated
on cur_p->skb:

	if (packets < budget && napi_complete_done(napi, packets)) {
		...
		axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);

so the line asserts an interrupt immediately, not "on the next hardware
completion".

The RX tail was also only advanced to the last successfully refilled
descriptor:

	if (tail_p)
		axienet_dma_out_addr(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p);

so a further completion past the stalled slot is not guaranteed either.
Could the comment (and the matching sentence in the commit message) be
reworded to match what the code does?

> +	if (packets < budget && cur_p->skb) {
> +		axienet_dma_out32(lp, XAXIDMA_RX_SR_OFFSET,
> +				  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
> +		if (cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)
> +			return budget;
> +	}
>  
>  	if (packets < budget && napi_complete_done(napi, packets)) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153243.1903501-1-kusuma.vasana%40amd.com