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

[netdev-bot+sashiko@kernel.org]

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