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

[Daniel Machon]

Hi Kusuma,

A few comments:

> 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.
> 
> Signed-off-by: Kusuma Vasana <kusuma.vasana@xxxxxxx>
> ---
> Changes in v4:
> - Read back SR after W1C so the clear is posted before the BD recheck.
> - Document that poll may return @budget on that recheck.
> - Drop the incorrect "leave IRQ armed" wording for RX refill failure.
> 
> 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 | 29 +++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index fcf517069d16..4d8169fc39cd 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -986,7 +986,8 @@ axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device *ndev)
>   * @napi:      Pointer to NAPI structure.
>   * @budget:    Max number of TX packets to process.
>   *
> - * Return: Number of TX packets processed.
> + * Return: Number of TX packets processed, or @budget to continue polling
> + * after a post-clear completion recheck.
>   *
>   * This function is invoked from the NAPI processing to notify the completion
>   * of transmit operation. It clears fields in the corresponding Tx BDs and
> @@ -1018,6 +1019,17 @@ static int axienet_tx_poll(struct napi_struct *napi, int budget)
>                         netif_wake_queue(ndev);
>         }
> 
> +       /* Clear stale IOC/DELAY bits, flush the clear, 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);
> +               axienet_dma_in32(lp, XAXIDMA_TX_SR_OFFSET);
> +               if (lp->tx_bd_v[lp->tx_bd_ci].status & XAXIDMA_BD_STS_COMPLETE_MASK)
> +                       return budget;
> +       }
> +

The patch does not apply on my tree.

This part was reworked in f9d58127380a ("net: axienet: do not report TX
completions as NAPI work") to not return budget-sized work, it now just returns
0.

>         if (packets < budget && napi_complete_done(napi, packets)) {
>                 /* Re-enable TX completion interrupts. This should
>                  * cause an immediate interrupt if any TX packets are
> @@ -1204,7 +1216,8 @@ static void axienet_dma_rx_cb(void *data, const struct dmaengine_result *result)
>   * @napi:      Pointer to NAPI structure.
>   * @budget:    Max number of RX packets to process.
>   *
> - * Return: Number of RX packets processed.
> + * Return: Number of RX packets processed, or @budget to continue polling
> + * after a post-clear completion recheck.
>   */
>  static int axienet_rx_poll(struct napi_struct *napi, int budget)
>  {
> @@ -1301,6 +1314,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, flush the clear, and re-check for
> +        * race-window completions. Skipped on budget exhaustion and on
> +        * refill failure (cur_p->skb == NULL).
> +        */
> +       if (packets < budget && cur_p->skb) {
> +               axienet_dma_out32(lp, XAXIDMA_RX_SR_OFFSET,
> +                                 XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_DELAY_MASK);
> +               axienet_dma_in32(lp, XAXIDMA_RX_SR_OFFSET);
> +               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)) {
>

I think the RX side looks good.

>struct dim_sample sample = {
> --
> 2.43.0
> 
> 

/Daniel