Re: [RFC net v3 3/3] bnxt_en: stop DMA before releasing rings the firmware did not free

[Joe Damato <joe@dama.to>]

On Wed, Sep 23, 2026 at 11:26:47PM -0700, Michael Chan wrote:
> On Wed, Sep 23, 2026 at 2:07 PM Joe Damato <joe@dama.to> wrote:
> 
> > @@ -7798,6 +7802,19 @@ static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
> >                         bp->grp_info[i].cp_fw_ring_id = INVALID_HW_RING_ID;
> >                 }
> >         }
> > +
> > +       if (!stuck)
> > +               return 0;
> > +
> > +       netdev_err(bp->dev, "Firmware did not free %d ring(s)\n", stuck);
> > +       return -EIO;
> > +}
> > +
> > +static void bnxt_stop_dma(struct bnxt *bp)
> > +{
> > +       netdev_err(bp->dev,
> > +                  "Disabling DMA before releasing ring memory, a firmware reset is required\n");
> > +       pci_clear_master(bp->pdev);
> >  }
> >
> Thanks for the patches.  The general scheme to more properly handle
> RING_FREE timeout to prevent possible memory corruption is correct.
> However, the corrective action here feels incomplete.  We should at
> least set some bp->state flags so that at the next open, we would know
> that something was wrong and could take further actions (e.g. FLR,
> etc)  to try to bring it back.

Thanks for the review. That makes sense. Were you thinking of trying to bring
the device back up automatically or requiring user intervention?

The automatic path is probably somewhat tricky, it might look something like
(psuedo code):

  #define BNXT_STATE_DMA_STOPPED  10

  /* bnxt_stop_dma */
  set_bit(BNXT_STATE_DMA_STOPPED, &bp->state);
  pci_clear_master(bp->pdev);

  /* bnxt_open, bnxt_open_nic, and probably bnxt_half_open_nic */
  if (test_bit(BNXT_STATE_DMA_STOPPED, ...
     bnxt_recover_stopped_dma(...)

  /* pseudo code */
  bnxt_recover_stopped_dma(...) {
     pcie_flr();
     pci_set_master();
     bnxt_fw_init_one();
     clear_bit(BNXT_STATE_DMA_STOPPED, ...
  }

That's just a basic outline; I suspect the details will be much trickier and
would probably take a few revisions to get right.

Instead, if you are OK with leaving the device disabled until a user rebinds
it, then I'd define a new bit (BNXT_STATE_DMA_STOPPED), set it in
bnxt_stop_dma, and then return ENODEV from the open paths. Simpler, but
requires manual intervention when this occurs.

I am guessing you mean the former, but wanted to check before I started to try
to write that code.