Re: [PATCH net v5 4/4] bnxt_en: refuse to open a device with stopped DMA

[Joe Damato <joe@dama.to>]

On Sat, Nov 07, 2026 at 07:58:53PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] This patch blocks driver recovery after a full device reset.
> - [Medium] bnxt_en: missing BNXT_STATE_DMA_STOPPED check in
>   BNXT_NO_FW_ACCESS()
> 
> --
> 
> bnxt_en: refuse to open a device with stopped DMA
> 
> This adds a BNXT_STATE_DMA_STOPPED bit. bnxt_stop_dma() sets it after
> the firmware fails to free rings and bus mastering is cleared. While the
> bit is set, bnxt_open(), bnxt_open_nic() and bnxt_half_open_nic() refuse
> to open the device, so the user has to rebind the driver.
> 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index b5ef86bf92bf5..eb885fd74064f 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -7833,9 +7833,20 @@ static void bnxt_stop_dma(struct bnxt *bp)
> >  {
> >  	netdev_err(bp->dev,
> >  		   "Disabling DMA before releasing ring memory, the driver must be rebound to recover\n");
> > +	set_bit(BNXT_STATE_DMA_STOPPED, &bp->state);
> >  	pci_clear_master(bp->pdev);
> >  }
> 
> [Severity: Medium]
> Once bnxt_stop_dma() has cleared bus mastering, can any later HWRM
> command complete? The firmware writes HWRM responses into host memory
> by DMA. With BNXT_FW_CAP_SHORT_CMD it also fetches the request by DMA.
> With the master bit cleared, __hwrm_send() should just poll until
> each command times out.
> 
> BNXT_NO_FW_ACCESS() only tests BNXT_STATE_FW_FATAL_COND and
> pci_channel_offline(). The new BNXT_STATE_DMA_STOPPED bit is never
> consulted there, so nothing stops these commands from being sent.
> 
> After the failed ring free, __bnxt_close_nic() continues into
> bnxt_free_mem(), which calls bnxt_free_ntp_fltrs() and
> bnxt_free_l2_filters(). bnxt_close() then sends
> bnxt_hwrm_shutdown_link() and bnxt_hwrm_if_change(). The netif_close()
> teardown after a failed bnxt_open_nic() goes through the same path.
> 
> Suppose the user follows the log message and rebinds the driver.
> bnxt_remove_one() still sends bnxt_hwrm_func_drv_unrgtr() and the rest
> of the teardown commands, and each of them waits for its full timeout
> too.
> 
> Most of this runs under rtnl_lock and the netdev instance lock. Every
> stalled command lengthens how long those locks are held. Close and
> unbind of a device that has already failed can therefore take a long
> time.
> 
> Should BNXT_NO_FW_ACCESS() also test BNXT_STATE_DMA_STOPPED, so that
> __hwrm_send() fails these commands immediately? If so, the bit would
> have to be cleared wherever bus mastering is turned back on:
> bnxt_fw_reset_task(), bnxt_resume() and bnxt_io_slot_reset().
> Otherwise the HWRM commands those paths send right after
> pci_set_master() would be refused as well.

I think this is legit and BNXT_NO_FW_ACCESS should test
BNXT_STATE_DMA_STOPPED. So, I'll do that in the next version.

> [ ... ]
> 
> > @@ -13505,6 +13521,10 @@ static int bnxt_open(struct net_device *dev)
> >  	struct bnxt *bp = netdev_priv(dev);
> >  	int rc;
> >  
> > +	rc = bnxt_check_dma_stopped(bp);
> > +	if (rc)
> > +		return rc;
> > +
> >  	if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) {
> >  		rc = bnxt_reinit_after_abort(bp);
> >  		if (rc) {
> 
> [Severity: Medium]
> Nothing in the driver clears BNXT_STATE_DMA_STOPPED. That includes the
> paths that fully reset the device and call pci_set_master() again.
> 
> This check also runs before bnxt_reinit_after_abort() and
> bnxt_hwrm_if_change(). Could that make the firmware reset, resume and AER
> recovery paths fail every time?
> 
> In a non-fatal firmware reset, bnxt_fw_reset_close()->__bnxt_close_nic()
> still sends HWRM_RING_FREE. Ring free is only skipped for FW_FATAL_COND
> or when the PCI channel is offline. If ring free fails there:
> 
> __bnxt_close_nic()
>     if (bnxt_shutdown_nic(bp, irq_re_init))
>         bnxt_stop_dma(bp);
> 
> bnxt_fw_reset_task() then turns bus mastering back on and waits for the
> firmware to come up, but the open is refused:
> 
> bnxt_fw_reset_task()
>     pci_set_master(bp->pdev);
>     ...
>     case BNXT_FW_RESET_STATE_OPENING:
>         ...
>         rc = bnxt_open(bp->dev);
>         if (rc) {
>             netdev_err(bp->dev, "bnxt_open() failed during FW reset\n");
>             bnxt_fw_reset_abort(bp, rc);
> 
> Suspend/resume looks similar. bnxt_suspend()->bnxt_close() can set the
> bit, and then bnxt_resume() does:
> 
> bnxt_resume()
>     pci_set_master(bp->pdev);
>     if (bnxt_hwrm_ver_get(bp)) {
>     ...
>     rc = bnxt_hwrm_func_reset(bp);
>     ...
>     if (netif_running(dev)) {
>         rc = bnxt_open(dev);
> 
> A rebind runs the same pci_set_master() plus HWRM_FUNC_RESET sequence, in
> bnxt_init_board() and bnxt_fw_init_one_p1(). Even so, bnxt_open() fails
> here and the netdev stays detached.
> 
> AER has the same problem. Suppose an earlier failed close left the bit
> set. bnxt_io_slot_reset() hard resets the slot, calls pci_set_master()
> and then bnxt_hwrm_func_reset(). After that, bnxt_io_resume()->bnxt_open()
> and any later ifup are still refused.
> 
> With only the preceding commit "bnxt_en: stop DMA before releasing rings
> the firmware did not free", these paths turned bus mastering back on and
> bnxt_open() succeeded.
> 
> The fatal firmware error path already handles a similar case.
> bnxt_fw_fatal_close() calls pci_disable_device(), and the reset task
> later calls pci_set_master() and reopens the device.
> 
> The commit message says "the device needs manual intervention by the
> user to become usable again". Does that still hold once the firmware or
> the function has been reset?
> 
> Should the bit be cleared, or the check skipped, after a firmware reset,
> HWRM_FUNC_RESET or slot reset completes?

IMHO, this behavior is intentional. If ring free fails for some reason the
device is intentionally left unrecoverable without manual intervention.

It's not clear to me if other paths should be able to clear the bit and
re-enable DMA.

So, I think I'm going to disagree with Sashiko on this one.

pw-bot: cr