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

[netdev-bot+sashiko@kernel.org]

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.

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106191423.4194081-1-joe%40dama.to