Re: [PATCH net v2] net: nixge: stop IRQ producers before draining DMA error tasklet

[Daniel Machon]

Hi,

> The TX and RX IRQ handlers can reschedule dma_err_tasklet after
> tasklet_kill(), allowing it to access rings freed by nixge_stop().
> 
> Free both IRQs before killing the tasklet. Since pending recovery can
> restart DMA, mask channel interrupts and wait for a DMA reset to finish
> before releasing the rings. Clearing RUNSTOP alone does not ensure that
> outstanding DMA accesses have completed.
> 
> If reset times out, retain the DMA buffers and reject subsequent opens
> to prevent those buffers from being freed or their pointers overwritten.

The subject only covers the IRQ/tasklet reorder. Whats described here is really
two separate fixes - should this be two patches?

> 
> Fixes: 492caffa8a1a ("net: ethernet: nixge: Add support for National Instruments XGE netdev")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
> Changes in v2:
> - Wait for DMA reset completion; retain buffers and reject reopen on timeout.
> 
> Based on net commit 6dc989ea46b96ce170840174b4a38c4a387fb005.
> No hardware testing was performed.
> 
>  drivers/net/ethernet/ni/nixge.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 230d5ff..878ca67 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -185,6 +185,7 @@ struct nixge_priv {
>         void __iomem *dma_regs;
> 
>         struct tasklet_struct dma_err_tasklet;
> +       bool dma_stop_failed;
> 
>         int tx_irq;
>         int rx_irq;
> @@ -391,7 +392,7 @@ out:
>         return -ENOMEM;
>  }
> 
> -static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
> +static int __nixge_device_reset(struct nixge_priv *priv, off_t offset)
>  {
>         u32 status;
>         int err;
> @@ -407,6 +408,8 @@ static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
>                                      1000);
>         if (err)
>                 netdev_err(priv->ndev, "%s: DMA reset timeout!\n", __func__);
> +
> +       return err;
>  }
> 
>  static void nixge_device_reset(struct net_device *ndev)
> @@ -869,6 +872,10 @@ static int nixge_open(struct net_device *ndev)
>         struct phy_device *phy;
>         int ret;
> 
> +       /* A failed stop retained buffers which DMA may still be using. */
> +       if (priv->dma_stop_failed)
> +               return -EIO;
> +
>         nixge_device_reset(ndev);
> 
>         phy = of_phy_connect(ndev, priv->phy_node,
> @@ -911,6 +918,7 @@ static int nixge_stop(struct net_device *ndev)
>  {
>         struct nixge_priv *priv = netdev_priv(ndev);
>         u32 cr;
> +       int ret;

nit: RXT

> 
>         netif_stop_queue(ndev);
>         napi_disable(&priv->napi);
> @@ -920,6 +928,7 @@ static int nixge_stop(struct net_device *ndev)
>                 phy_disconnect(ndev->phydev);
>         }
> 
> +       /* Stop DMA while the completion IRQ handlers are still installed. */
>         cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
>         nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
>                             cr & (~XAXIDMA_CR_RUNSTOP_MASK));
> @@ -927,11 +936,29 @@ static int nixge_stop(struct net_device *ndev)
>         nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
>                             cr & (~XAXIDMA_CR_RUNSTOP_MASK));
> 
> -       tasklet_kill(&priv->dma_err_tasklet);
> -
> +       /* Remove both producers before draining the error tasklet. */
>         free_irq(priv->tx_irq, ndev);
>         free_irq(priv->rx_irq, ndev);
> 
> +       tasklet_kill(&priv->dma_err_tasklet);
> +
> +       /* Error recovery may have restarted DMA and enabled interrupts. */
> +       cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> +       nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
> +                           cr & ~(XAXIDMA_CR_RUNSTOP_MASK | XAXIDMA_IRQ_ALL_MASK));
> +       cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET);
> +       nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
> +                           cr & ~(XAXIDMA_CR_RUNSTOP_MASK | XAXIDMA_IRQ_ALL_MASK));
> +
> +       /* Either channel's reset quiesces the entire AXI DMA engine. */
> +       ret = __nixge_device_reset(priv, XAXIDMA_TX_CR_OFFSET);
> +       if (ret) {
> +               /* Retain DMA buffers and prevent a subsequent open replacing them. */
> +               priv->dma_stop_failed = true;
> +               netdev_err(ndev, "DMA stop failed; retaining DMA buffers\n");
> +               return ret;
> +       }
> +
>         nixge_hw_dma_bd_release(ndev);
> 
>         return 0;
> --
> 2.53.0
> 
>