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

[PATCH net-next V2] 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. When the driver was converted to NAPI, 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.

Fix this by unconditionally clearing interrupt status bits before
re-enabling interrupts in the NAPI poll handlers.

Signed-off-by: Kusuma Vasana <kusuma.vasana@amd.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes in V2 :

-Added net-next prefix in the subject
-Updated the commit description

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

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index fcf517069d16..29050c8d04e2 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1018,6 +1018,13 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
 			netif_wake_queue(ndev);
 	}
 
+	/* Clear stale IOC/DELAY bits that may have latched during the
+	 * poll window to prevent a stale interrupt when there is no
+	 * work pending.
+	 */
+	axienet_dma_out32(lp, XAXIDMA_TX_SR_OFFSET,
+			  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
+
 	if (packets < budget && napi_complete_done(napi, packets)) {
 		/* Re-enable TX completion interrupts. This should
 		 * cause an immediate interrupt if any TX packets are
@@ -1293,6 +1300,13 @@ static int axienet_rx_poll(struct napi_struct *napi, int budget)
 		cur_p = &lp->rx_bd_v[lp->rx_bd_ci];
 	}
 
+	/* Clear stale IOC/DELAY bits that may have latched during the
+	 * poll window to prevent a stale interrupt when there is no
+	 * work pending.
+	 */
+	axienet_dma_out32(lp, XAXIDMA_RX_SR_OFFSET,
+			  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
+
 	u64_stats_update_begin(&lp->rx_stat_sync);
 	u64_stats_add(&lp->rx_packets, packets);
 	u64_stats_add(&lp->rx_bytes, size);
-- 
2.43.0

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: axienet: Clear stale AXI DMA TX/RX status before re-enabling interrupts

This patch clears the AXI DMA IOC/DELAY status bits at the end of the TX
and RX NAPI poll handlers to avoid a redundant stale interrupt caused by
IOC/DELAY bits latched into the status register while interrupts were
masked in the control register during NAPI. The clear is performed
unconditionally, before the napi_complete_done() / CR re-enable step.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index fcf517069d16..29050c8d04e2 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1018,6 +1018,13 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
>  			netif_wake_queue(ndev);
>  	}
>  
> +	/* Clear stale IOC/DELAY bits that may have latched during the
> +	 * poll window to prevent a stale interrupt when there is no
> +	 * work pending.
> +	 */
> +	axienet_dma_out32(lp, XAXIDMA_TX_SR_OFFSET,
> +			  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
> +
>  	if (packets < budget && napi_complete_done(napi, packets)) {
>  		/* Re-enable TX completion interrupts. This should
>  		 * cause an immediate interrupt if any TX packets are

[High]
Can this sequence race with a late completion arriving after the poll
loop has exited but before the SR write?

The AXI DMA engine writes cur_p->status and latches SR.IOC asynchronously
from CPU execution. The poll loop's exit condition reads cur_p->status,
and between that read and the unconditional SR IOC/DELAY clear that this
hunk adds, the hardware can set COMPLETE_MASK on the next BD and latch
SR.IOC for it. That legitimate notification would then be wiped by the
new W1C write.

Since napi_schedule() is only called from the ISR, and the ISR only fires
when the IRQ line asserts, napi_complete_done() will succeed here even
though a new completion has appeared. The subsequent CR re-enable would
then observe SR.IOC=0, so the level-sensitive line does not re-assert,
and the completed BD is stranded until the next unrelated completion or
DELAY-timer expiry moves things forward.

Before this patch, any SR bit latched during the poll window (whether
stale or from a late completion) re-asserted the IRQ line after CR
re-enable, which the changelog frames purely as an unnecessary stale
interrupt:

    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.

Did that same behavior also serve as the recovery path for a completion
that lands in the SR-clear/CR-reenable window? If so, would it be worth
either re-checking cur_p->status after the SR clear and skipping
napi_complete_done() when new work has appeared, or performing the CR
re-enable before the SR clear so a late latch still asserts the line?

NAPIF_STATE_MISSED does not appear to help here because it requires a
napi_schedule() call, which only the ISR performs.

For configurations where axienet_calc_cr() does arm the DELAY timer
(coalesce count > 1), the stranded BD is recovered when DELAY expires:

    /* Only set interrupt delay timer if not generating an interrupt on
     * every packet
     */

Under DIM with low traffic the coalesce count can settle at 1, in which
case DELAY is not armed and there is no bounded recovery for the last
BD of a burst until another packet arrives. Is that concern valid, or is
there another mechanism that ensures the last BD is drained?

> @@ -1293,6 +1300,13 @@ static int axienet_rx_poll(struct napi_struct *napi, int budget)
>  		cur_p = &lp->rx_bd_v[lp->rx_bd_ci];
>  	}
>  
> +	/* Clear stale IOC/DELAY bits that may have latched during the
> +	 * poll window to prevent a stale interrupt when there is no
> +	 * work pending.
> +	 */
> +	axienet_dma_out32(lp, XAXIDMA_RX_SR_OFFSET,
> +			  XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
> +

The same question applies to the RX path: a completion arriving after
the while loop's cur_p->status check but before this SR write would have
its SR.IOC latch cleared, and the subsequent CR re-enable would then not
re-assert the line. Is there a reason the RX path is not exposed to the
same race?
-- 
pw-bot: cr