[PATCH net v5 0/4] bnxt_en: Make RING FREE more robust

[PATCH net v5 0/4] bnxt_en: Make RING FREE more robust

[Joe Damato]

Greetings:

This is a follow up to the previous RFC (linked below).

On two production systems, I saw the following dmesg pattern:

  NETDEV WATCHDOG: transmit queue 0 timed out 6073 ms
  Resp cmpl intr err msg: 0x51                  x20
  hwrm_ring_free type 1 failed                  x12
  hwrm_ring_free type 2 failed                  x8
  AMD-Vi: IO_PAGE_FAULT  x3

This suggests that, for some currently unknown reason, TX completions stall
and the netdev watchdog fires. The driver asks FW to free the rings, this
times out, but the driver ignores the possible failure and frees ring memory.
Since the FW didn't respond to the ring free command, it is possible that the
FW is still DMAing to the memory which was freed.

Since the RFCv4, I found a third system that hit this repeatedly:

  NETDEV WATCHDOG: CPU: 35: transmit queue 0 timed out 5459 ms
  hwrm_ring_free type 1 failed. rc:fffffff0 err:0    x64
  hwrm_ring_free type 2 failed. rc:fffffff0 err:0    x32
  hwrm_ring_free type 0 failed. rc:fffffffb err:1    x64
  hwrm_ring_alloc type 1 failed. rc:ffffffe4 err:4
  hwrm_ring_free type 1 failed. rc:fffffff0 err:0    x44
  Abandoning msg {0x51 0x3e35} len: 0 due to firmware status: 0x408100
  nic open fail (rc: fffffffb)
  DMAR: [DMA Write NO_PASID] Request device [47:00.0] fault addr
  0xffffa07ef000 [fault reason 0x05] PTE Write access is not set

This case differs from the first two in a few ways, but this series would have
still helped:

 - The ring frees fail on the close path and again on the error path of
   the reopen (bnxt_init_chip()), and the device then writes to memory the
   driver has already unmapped. The series stops DMA on both paths.

 - The interface came back up later, the watchdog fired again, and the
   cycle started over. The host logged 79 DMAR write faults in about 20
   hours. Patch 4 breaks this loop by refusing to reopen after DMA has
   been stopped.

 - The firmware answers each RING_FREE about 5 seconds late, after the
   driver has given up, and finally reports an error in its health
   register. Patch 2 does not treat these late responses as success because
   their sequence ids do not match.

In this most recent case, I suspect the hardware is faulty, but the driver
shouldn't hand memory back to the system while the device could still write to
it.

In all of the above cases, the IOMMU caught the error. If the system had no
IOMMU or the IOMMU was in passthrough mode, data corruption or crash would
have occurred.

In the future, this could possibly be extended to make recovery automatic.

Thanks,
Joe

v5:
 - Switched from RFC to actual submission
 - Patch 4 updated to use an un-used bit to make backporting easier.

v4: https://lore.kernel.org/netdev/20260925174404.2789072-1-joe@dama.to/
  - No changes to patch 1 or 2
  - Patch 3: reworded the message logged when DMA is stopped, a rebind is
    what recovers the device, not a firmware reset. No functional change.
  - Patch 4 added which adds a new bit (BNXT_STATE_DMA_STOPPED) that is set
    when DMA is disabled. When this bit is set, the device requires user
    intervention to bring back up.

v3: https://lore.kernel.org/netdev/20260923210744.3406861-1-joe@dama.to/
  - No changes to patch 1
  - Patch 2: Don't poll for the valid bit as Michael suggested.
  - Patch 3: bnxt_hwrm_ring_free now returns -EIO instead of stopping the
    device, so the remaining resources can be freed and the remaining commands
    can be sent before stopping the device, as Michael suggested. Note the
    switch to using pci_clear_master in this patch instead of
    pci_disable_device. This was done so that the normal shutdown paths can
    call pci_disable_device without generating a warning.

v2: https://lore.kernel.org/netdev/20260922182405.1290749-1-joe@dama.to/
  - No changes to patch 1
  - Patch 2 from v1 dropped
  - Patch 2 in the v2 now checks the response and logs state before giving up
  - Patch 3 in the v2 disables the device to stop DMA before freeing ring
    memory

RFCv1: https://lore.kernel.org/netdev/20260917233218.1160001-1-joe@dama.to/


Joe Damato (4):
  bnxt_en: return the RING_FREE status to callers
  bnxt_en: check HWRM response if completion never arrives
  bnxt_en: stop DMA before releasing rings the firmware did not free
  bnxt_en: refuse to open a device with stopped DMA

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 122 ++++++++++++------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 +
 .../net/ethernet/broadcom/bnxt/bnxt_hwrm.c    |  33 ++++-
 3 files changed, 115 insertions(+), 41 deletions(-)


base-commit: 19a227903e65c7a9c179156485660a4e017575fe
-- 
2.53.0-Meta

[PATCH net v5 1/4] bnxt_en: return the RING_FREE status to callers

[Joe Damato]

hwrm_ring_free_send_msg() reports failure to its caller, returning -EIO
when the firmware rejects HWRM_RING_FREE or never answers it. All three
ring free helpers that send the command discard the value.

Return it instead. No caller acts on it yet, so there is no functional
change.

Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring")
Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 48 +++++++++++++----------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 33c4cacc8ffe..ea8529c78994 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7679,50 +7679,55 @@ static int hwrm_ring_free_send_msg(struct bnxt *bp,
 	return 0;
 }
 
-static void bnxt_hwrm_tx_ring_free(struct bnxt *bp,
-				   struct bnxt_tx_ring_info *txr,
-				   bool close_path)
+static int bnxt_hwrm_tx_ring_free(struct bnxt *bp,
+				  struct bnxt_tx_ring_info *txr,
+				  bool close_path)
 {
 	struct bnxt_ring_struct *ring = &txr->tx_ring_struct;
 	u32 cmpl_ring_id;
+	int rc;
 
 	if (ring->fw_ring_id == INVALID_HW_RING_ID)
-		return;
+		return 0;
 
 	cmpl_ring_id = close_path ? bnxt_cp_ring_for_tx(bp, txr) :
 		       INVALID_HW_RING_ID;
-	hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_TX,
-				cmpl_ring_id);
+	rc = hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_TX,
+				     cmpl_ring_id);
 	ring->fw_ring_id = INVALID_HW_RING_ID;
+	return rc;
 }
 
-static void bnxt_hwrm_rx_ring_free(struct bnxt *bp,
-				   struct bnxt_rx_ring_info *rxr,
-				   bool close_path)
+static int bnxt_hwrm_rx_ring_free(struct bnxt *bp,
+				  struct bnxt_rx_ring_info *rxr,
+				  bool close_path)
 {
 	struct bnxt_ring_struct *ring = &rxr->rx_ring_struct;
 	u32 grp_idx = rxr->bnapi->index;
 	u32 cmpl_ring_id;
+	int rc;
 
 	if (ring->fw_ring_id == INVALID_HW_RING_ID)
-		return;
+		return 0;
 
 	cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr);
-	hwrm_ring_free_send_msg(bp, ring,
-				RING_FREE_REQ_RING_TYPE_RX,
-				close_path ? cmpl_ring_id :
-				INVALID_HW_RING_ID);
+	rc = hwrm_ring_free_send_msg(bp, ring,
+				     RING_FREE_REQ_RING_TYPE_RX,
+				     close_path ? cmpl_ring_id :
+				     INVALID_HW_RING_ID);
 	ring->fw_ring_id = INVALID_HW_RING_ID;
 	bp->grp_info[grp_idx].rx_fw_ring_id = INVALID_HW_RING_ID;
+	return rc;
 }
 
-static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
-				       struct bnxt_rx_ring_info *rxr,
-				       bool close_path)
+static int bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
+				      struct bnxt_rx_ring_info *rxr,
+				      bool close_path)
 {
 	struct bnxt_ring_struct *ring = &rxr->rx_agg_ring_struct;
 	u32 grp_idx = rxr->bnapi->index;
 	u32 type, cmpl_ring_id;
+	int rc;
 
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
 		type = RING_FREE_REQ_RING_TYPE_RX_AGG;
@@ -7730,14 +7735,15 @@ static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
 		type = RING_FREE_REQ_RING_TYPE_RX;
 
 	if (ring->fw_ring_id == INVALID_HW_RING_ID)
-		return;
+		return 0;
 
 	cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr);
-	hwrm_ring_free_send_msg(bp, ring, type,
-				close_path ? cmpl_ring_id :
-				INVALID_HW_RING_ID);
+	rc = hwrm_ring_free_send_msg(bp, ring, type,
+				     close_path ? cmpl_ring_id :
+				     INVALID_HW_RING_ID);
 	ring->fw_ring_id = INVALID_HW_RING_ID;
 	bp->grp_info[grp_idx].agg_fw_ring_id = INVALID_HW_RING_ID;
+	return rc;
 }
 
 static void bnxt_hwrm_cp_ring_free(struct bnxt *bp,
-- 
2.53.0-Meta

[PATCH net v5 2/4] bnxt_en: check HWRM response if completion never arrives

[Joe Damato]

When a command is sent over a completion ring, __hwrm_send() waits for
NAPI to consume the completion and gives up if it never arrives, without
looking at the response.

If a completion is not posted within the timeout, check the response
before giving up. If resp_len is set, the sequence id matches, and the
valid byte is set then the firmware completed the command and only the
notification was lost. Fall through to the normal error_code handling in
that case.

Several seconds are spent waiting for the completion, so a response that
was written at all is complete by the time the wait gives up. There is no
need to poll for the valid byte here the way the polling path below has
to, where the poll is for a non-zero length and the valid byte at the end
of the message may still be on its way.

Log the response state on both paths so there is more data when this rare
event occurs.

Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring")
Signed-off-by: Joe Damato <joe@dama.to>
---
 .../net/ethernet/broadcom/bnxt/bnxt_hwrm.c    | 33 ++++++++++++++++---
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
index 5bfabdca7d0e..4feba90f0bf6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
@@ -582,11 +582,36 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
 		}
 
 		if (READ_ONCE(token->state) != BNXT_HWRM_COMPLETE) {
-			hwrm_err(bp, ctx, "Resp cmpl intr err msg: 0x%x\n",
-				 req_type);
-			goto exit;
+			__le16 resp_seq_id;
+			u8 valid_byte = 0;
+
+			/* The completion ring entry was not delivered for
+			 * some reason. It might be possible that the command
+			 * was carried out even without a completion being
+			 * posted. Check the response before giving up and log
+			 * the state.
+			 */
+			dma_rmb();
+			resp_seq_id = READ_ONCE(ctx->resp->seq_id);
+			len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
+			if (len && resp_seq_id == ctx->req->seq_id)
+				valid_byte = *((u8 *)ctx->resp + len - 1);
+
+			if (!valid_byte) {
+				hwrm_err(bp, ctx,
+					 "Resp cmpl intr err msg: 0x%x len:%d seq:0x%x/0x%x\n",
+					 req_type, len,
+					 le16_to_cpu(resp_seq_id),
+					 le16_to_cpu(ctx->req->seq_id));
+				goto exit;
+			}
+			netdev_warn(bp->dev,
+				    "Resp cmpl intr not delivered, msg: 0x%x completed anyway (len:%d valid:0x%x err:0x%x)\n",
+				    req_type, len, valid_byte,
+				    le16_to_cpu(ctx->resp->error_code));
+		} else {
+			len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
 		}
-		len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
 		valid = ((u8 *)ctx->resp) + len - 1;
 	} else {
 		__le16 seen_out_of_seq = ctx->req->seq_id; /* will never see */
-- 
2.53.0-Meta

[PATCH net v5 3/4] bnxt_en: stop DMA before releasing rings the firmware did not free

[Joe Damato]

When HWRM_RING_FREE is not answered, bnxt_hwrm_ring_free() clears
fw_ring_id and __bnxt_close_nic() goes on to call bnxt_free_mem(), which
unmaps the ring memory and the RX buffers that the FW may still be
using.

This is reachable in production. On a BCM57504 the first sign is the TX
watchdog; the close that follows times out a subset of its RING_FREEs and
the driver releases those rings anyway:

  05:30:12  NETDEV WATCHDOG: transmit queue 0 timed out 6073 ms
  05:30:12  Resp cmpl intr err msg: 0x51                  x20
  05:30:12  hwrm_ring_free type 1 failed                  x12
  05:30:12  hwrm_ring_free type 2 failed                  x8
  05:30:12  AMD-Vi: IO_PAGE_FAULT  x3

Count the rings the firmware did not free and report that to the caller
so it can decide what to do. bnxt_hwrm_resource_free() still frees the
remaining firmware resources before returning the error, so the shutdown
can send every message it needs to before the device is stopped.

The device stays unusable until the driver is rebound.

Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring")
Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 48 +++++++++++++++++------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index ea8529c78994..b5ef86bf92bf 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7773,21 +7773,25 @@ static void bnxt_clear_one_cp_ring(struct bnxt *bp, struct bnxt_cp_ring_info *cp
 			memset(cpr->cp_desc_ring[i], 0, size);
 }
 
-static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
+static int bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
 {
+	int stuck = 0;
 	u32 type;
 	int i;
 
 	if (!bp->bnapi)
-		return;
+		return 0;
 
 	for (i = 0; i < bp->tx_nr_rings; i++)
-		bnxt_hwrm_tx_ring_free(bp, &bp->tx_ring[i], close_path);
+		if (bnxt_hwrm_tx_ring_free(bp, &bp->tx_ring[i], close_path))
+			stuck++;
 
 	bnxt_cancel_dim(bp);
 	for (i = 0; i < bp->rx_nr_rings; i++) {
-		bnxt_hwrm_rx_ring_free(bp, &bp->rx_ring[i], close_path);
-		bnxt_hwrm_rx_agg_ring_free(bp, &bp->rx_ring[i], close_path);
+		if (bnxt_hwrm_rx_ring_free(bp, &bp->rx_ring[i], close_path))
+			stuck++;
+		if (bnxt_hwrm_rx_agg_ring_free(bp, &bp->rx_ring[i], close_path))
+			stuck++;
 	}
 
 	/* The completion rings are about to be freed.  After that the
@@ -7817,6 +7821,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, the driver must be rebound to recover\n");
+	pci_clear_master(bp->pdev);
 }
 
 static int __bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
@@ -10851,16 +10868,19 @@ static void bnxt_clear_vnic(struct bnxt *bp)
 		bnxt_hwrm_vnic_ctx_free(bp);
 }
 
-static void bnxt_hwrm_resource_free(struct bnxt *bp, bool close_path,
-				    bool irq_re_init)
+static int bnxt_hwrm_resource_free(struct bnxt *bp, bool close_path,
+				   bool irq_re_init)
 {
+	int rc;
+
 	bnxt_clear_vnic(bp);
-	bnxt_hwrm_ring_free(bp, close_path);
+	rc = bnxt_hwrm_ring_free(bp, close_path);
 	bnxt_hwrm_ring_grp_free(bp);
 	if (irq_re_init) {
 		bnxt_hwrm_stat_ctx_free(bp);
 		bnxt_hwrm_free_tunnel_ports(bp);
 	}
+	return rc;
 }
 
 static int bnxt_hwrm_set_br_mode(struct bnxt *bp, u16 br_mode)
@@ -11382,15 +11402,15 @@ static int bnxt_init_chip(struct bnxt *bp, bool irq_re_init)
 	return 0;
 
 err_out:
-	bnxt_hwrm_resource_free(bp, 0, true);
+	if (bnxt_hwrm_resource_free(bp, 0, true))
+		bnxt_stop_dma(bp);
 
 	return rc;
 }
 
 static int bnxt_shutdown_nic(struct bnxt *bp, bool irq_re_init)
 {
-	bnxt_hwrm_resource_free(bp, 1, irq_re_init);
-	return 0;
+	return bnxt_hwrm_resource_free(bp, 1, irq_re_init);
 }
 
 static int bnxt_init_nic(struct bnxt *bp, bool irq_re_init)
@@ -13461,7 +13481,8 @@ int bnxt_half_open_nic(struct bnxt *bp)
  */
 void bnxt_half_close_nic(struct bnxt *bp)
 {
-	bnxt_hwrm_resource_free(bp, false, true);
+	if (bnxt_hwrm_resource_free(bp, false, true))
+		bnxt_stop_dma(bp);
 	bnxt_del_napi(bp);
 	bnxt_free_skbs(bp);
 	bnxt_free_mem(bp, true);
@@ -13540,7 +13561,8 @@ static void __bnxt_close_nic(struct bnxt *bp, bool irq_re_init,
 	if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp))
 		bnxt_clear_rss_ctxs(bp);
 	/* Flush rings and disable interrupts */
-	bnxt_shutdown_nic(bp, irq_re_init);
+	if (bnxt_shutdown_nic(bp, irq_re_init))
+		bnxt_stop_dma(bp);
 
 	/* TODO CHIMP_FW: Link/PHY related cleanup if (link_re_init) */
 
-- 
2.53.0-Meta

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

[Joe Damato]

Add BNXT_STATE_DMA_STOPPED and set it in bnxt_stop_dma() to signal that
the device had DMA disabled. When opening the device later, check this
bit and exit with an error.

This is a very rare case, but when it happens the device needs manual
intervention by the user to become usable again.

Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring")
Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 26 ++++++++++++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  1 +
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b5ef86bf92bf..eb885fd74064 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);
 }
 
+static int bnxt_check_dma_stopped(struct bnxt *bp)
+{
+	if (!test_bit(BNXT_STATE_DMA_STOPPED, &bp->state))
+		return 0;
+
+	netdev_err(bp->dev,
+		   "DMA was disabled after the firmware failed to free rings, rebind the driver to recover\n");
+	return -ENODEV;
+}
+
 static int __bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
 			     bool shared);
 static int bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
@@ -13426,9 +13437,10 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 
 int bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 {
-	int rc = 0;
+	int rc;
 
-	if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state))
+	rc = bnxt_check_dma_stopped(bp);
+	if (!rc && test_bit(BNXT_STATE_ABORT_ERR, &bp->state))
 		rc = -EIO;
 	if (!rc)
 		rc = __bnxt_open_nic(bp, irq_re_init, link_re_init);
@@ -13445,7 +13457,11 @@ int bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
  */
 int bnxt_half_open_nic(struct bnxt *bp)
 {
-	int rc = 0;
+	int rc;
+
+	rc = bnxt_check_dma_stopped(bp);
+	if (rc)
+		goto half_open_err;
 
 	if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) {
 		netdev_err(bp->dev, "A previous firmware reset has not completed, aborting half open\n");
@@ -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) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 41d05efd6307..7e65b61b6bf8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2474,6 +2474,7 @@ struct bnxt {
 #define BNXT_STATE_FW_NON_FATAL_COND	13
 #define BNXT_STATE_FW_ACTIVATE_RESET	14
 #define BNXT_STATE_HALF_OPEN		15	/* For offline ethtool tests */
+#define BNXT_STATE_DMA_STOPPED		16
 
 #define BNXT_NO_FW_ACCESS(bp)					\
 	(test_bit(BNXT_STATE_FW_FATAL_COND, &(bp)->state) ||	\
-- 
2.53.0-Meta

Re: [PATCH net v5 1/4] bnxt_en: return the RING_FREE status to callers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · 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).

- [High] The patch makes bnxt_hwrm_rx_ring_free(),
  bnxt_hwrm_rx_agg_ring_free() and bnxt_hwrm_tx_ring_free() return the…
- [Medium] The commit message says "All three ring free helpers that send
  the command discard the value", but four helpers send HWRM_RING_FREE,…

--

bnxt_en: return the RING_FREE status to callers

This commit changes bnxt_hwrm_tx_ring_free(), bnxt_hwrm_rx_ring_free()
and bnxt_hwrm_rx_agg_ring_free() so they return the status from
hwrm_ring_free_send_msg() instead of discarding it. No caller acts on
the returned value yet.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 33c4cacc8ffe3..ea8529c789947 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -7679,50 +7679,55 @@ static int hwrm_ring_free_send_msg(struct bnxt *bp,

[ ... ]

> -static void bnxt_hwrm_rx_ring_free(struct bnxt *bp,
> -				   struct bnxt_rx_ring_info *rxr,
> -				   bool close_path)
> +static int bnxt_hwrm_rx_ring_free(struct bnxt *bp,
> +				  struct bnxt_rx_ring_info *rxr,
> +				  bool close_path)
>  {
>  	struct bnxt_ring_struct *ring = &rxr->rx_ring_struct;
>  	u32 grp_idx = rxr->bnapi->index;
>  	u32 cmpl_ring_id;
> +	int rc;
>  
>  	if (ring->fw_ring_id == INVALID_HW_RING_ID)
> -		return;
> +		return 0;
>  
>  	cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr);
> -	hwrm_ring_free_send_msg(bp, ring,
> -				RING_FREE_REQ_RING_TYPE_RX,
> -				close_path ? cmpl_ring_id :
> -				INVALID_HW_RING_ID);
> +	rc = hwrm_ring_free_send_msg(bp, ring,
> +				     RING_FREE_REQ_RING_TYPE_RX,
> +				     close_path ? cmpl_ring_id :
> +				     INVALID_HW_RING_ID);
>  	ring->fw_ring_id = INVALID_HW_RING_ID;
>  	bp->grp_info[grp_idx].rx_fw_ring_id = INVALID_HW_RING_ID;
> +	return rc;
>  }

[Severity: High]
The per-queue callers of these helpers still ignore the returned status,
and none of the later patches in the series seems to change them. Can
the queue restart path still release rings that the firmware did not
free?

In bnxt_queue_stop():

	bnxt_hwrm_rx_ring_free(bp, rxr, false);
	bnxt_hwrm_rx_agg_ring_free(bp, rxr, false);
	...
	memcpy(qmem, rxr, sizeof(*rxr));
	bnxt_init_rx_ring_struct(bp, qmem);

	return 0;

If the firmware rejects HWRM_RING_FREE or it times out, ndo_queue_stop
still returns 0. netdev_rx_queue_reconfig() then calls
ndo_queue_mem_free() on the old queue memory.

bnxt_queue_mem_free() then frees the RX buffers, destroys the page pools
and frees the RX and AGG descriptor rings. The firmware ring may still be
live at that point.

With tph_mode set, bnxt_tx_queue_stop() does the same thing for TX:

		bnxt_hwrm_tx_ring_free(bp, txr, true);
		bnxt_hwrm_cp_ring_free(bp, txr->tx_cpr);
		bnxt_free_one_tx_ring_skbs(bp, txr, txr->txq_index);

The helpers also clear ring->fw_ring_id even when rc is non-zero. After
that, the stale firmware ring is no longer visible to the stuck ring
accounting that the later patch "bnxt_en: stop DMA before releasing
rings the firmware did not free" adds to bnxt_hwrm_ring_free(). Would a
later close then skip bnxt_stop_dma() for that ring?

This path can be reached through netdev_rx_queue_restart(), either from
devmem TCP or io_uring zcrx memory provider binding, or from the bnxt
TPH IRQ affinity work.

> -static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
> -				       struct bnxt_rx_ring_info *rxr,
> -				       bool close_path)
> +static int bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
> +				      struct bnxt_rx_ring_info *rxr,
> +				      bool close_path)

[ ... ]

> @@ -7730,14 +7735,15 @@ static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
>  		type = RING_FREE_REQ_RING_TYPE_RX;
>  
>  	if (ring->fw_ring_id == INVALID_HW_RING_ID)
> -		return;
> +		return 0;
>  
>  	cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr);
> -	hwrm_ring_free_send_msg(bp, ring, type,
> -				close_path ? cmpl_ring_id :
> -				INVALID_HW_RING_ID);
> +	rc = hwrm_ring_free_send_msg(bp, ring, type,
> +				     close_path ? cmpl_ring_id :
> +				     INVALID_HW_RING_ID);
>  	ring->fw_ring_id = INVALID_HW_RING_ID;
>  	bp->grp_info[grp_idx].agg_fw_ring_id = INVALID_HW_RING_ID;
> +	return rc;
>  }
>  
>  static void bnxt_hwrm_cp_ring_free(struct bnxt *bp,

[Severity: Medium]
The commit message says:

    All three ring free helpers that send the command discard the value.

Is that count right? bnxt_hwrm_cp_ring_free() also sends HWRM_RING_FREE,
and it still returns void:

	hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_L2_CMPL,
				INVALID_HW_RING_ID);
	ring->fw_ring_id = INVALID_HW_RING_ID;

The inline NQ/L2_CMPL free in bnxt_hwrm_ring_free() also drops its
status:

		if (ring->fw_ring_id != INVALID_HW_RING_ID) {
			hwrm_ring_free_send_msg(bp, ring, type,
						INVALID_HW_RING_ID);

By the end of the series, bnxt_hwrm_ring_free() counts stuck rings only
for TX, RX and AGG failures. If a CP or NQ RING_FREE fails, would
bnxt_stop_dma() be skipped, with __bnxt_close_nic() going on to free
that ring's memory in bnxt_free_mem()?

Ring 0 of the CP/NQ set is also registered as the firmware async event
completion ring by bnxt_hwrm_set_async_event_cr() in
bnxt_hwrm_ring_alloc(). The close path does not move that registration.

It is not certain that firmware keeps writing to an unfreed CP/NQ ring
after the TX and RX rings are gone. Async event completions are the
likely source if it does.

On the per-queue paths, bnxt_tx_queue_stop() and bnxt_queue_stop() (with
tph_mode) call the converted helpers right next to
bnxt_hwrm_cp_ring_free(). The CP ring free status is not available to
those callers either.

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

Re: [PATCH net v5 2/4] bnxt_en: check HWRM response if completion never arrives

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 2

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] The new fallback for a missing completion is in the generic
  completion-ring wait branch of __hwrm_send()…
- [Medium] bnxt_hwrm_rx_ring_reset()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c:14467) sends…
- [Medium] In the new fallback for a missing completion in __hwrm_send(),
  dma_rmb() runs once, before the loads of seq_id, resp_len and the…
- [Low] The commit message says the valid byte need not be polled because
  'Several seconds are spent waiting for the completion, so a response…
- [Low] BNXT_HWRM_CTX_SILENT is documented in bnxt_hwrm.h as 'squelch
  firmware errors', and hwrm_err() turns messages into netdev_dbg when…

--

bnxt_en: check HWRM response if completion never arrives

This changes __hwrm_send() so that when a command sent over a completion
ring never has its completion consumed by NAPI, the DMA response is
checked before giving up. If resp_len is non-zero, the sequence id
matches and the valid byte is set, the command is treated as completed
and falls through to the normal error_code handling.

> Several seconds are spent waiting for the completion, so a response that
> was written at all is complete by the time the wait gives up. There is no
> need to poll for the valid byte here the way the polling path below has
> to, where the poll is for a non-zero length and the valid byte at the end
> of the message may still be on its way.

[Severity: Low]
Is the "several seconds" part accurate? __hwrm_req_init() sets the
default timeout like this:

    ctx->timeout = bp->hwrm_cmd_timeout ?: DFLT_HWRM_CMD_TIMEOUT;

DFLT_HWRM_CMD_TIMEOUT is defined as 500 in bnxt.h, and
bp->hwrm_cmd_timeout comes from the firmware's def_req_timeout. None of
the completion ring callers (ring free, ring reset, selftest irq) seem to
raise it.

The wait loop can also end with no timeout at all when
bnxt_free_hwrm_resources() cancels the token:

    WRITE_ONCE(token->state, BNXT_HWRM_CANCELLED);

That also enters the new != BNXT_HWRM_COMPLETE branch. And if firmware
finishes right at the timeout boundary, the response DMA may still be in
flight when it is checked.

A partly written response still reads valid_byte == 0 and takes the error
exit, so the code fails safe. Could the justification be reworded, though?

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> index 5bfabdca7d0e2..4feba90f0bf68 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> @@ -582,11 +582,36 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
>  		}
>  
>  		if (READ_ONCE(token->state) != BNXT_HWRM_COMPLETE) {
> -			hwrm_err(bp, ctx, "Resp cmpl intr err msg: 0x%x\n",
> -				 req_type);
> -			goto exit;
> +			__le16 resp_seq_id;
> +			u8 valid_byte = 0;
> +
> +			/* The completion ring entry was not delivered for
> +			 * some reason. It might be possible that the command
> +			 * was carried out even without a completion being
> +			 * posted. Check the response before giving up and log
> +			 * the state.
> +			 */
> +			dma_rmb();
> +			resp_seq_id = READ_ONCE(ctx->resp->seq_id);
> +			len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
> +			if (len && resp_seq_id == ctx->req->seq_id)
> +				valid_byte = *((u8 *)ctx->resp + len - 1);

[Severity: Medium]
Is the dma_rmb() in the right place here? It runs once, before the
seq_id, resp_len and valid byte loads. Nothing orders the loads of the
rest of the response after the valid byte is seen.

Those later reads include ctx->resp->error_code in the netdev_warn()
below. They also include the common tail of __hwrm_send():

    *valid = 0;
    rc = le16_to_cpu(ctx->resp->error_code);

and any response fields that callers read after hwrm_req_send() returns.

Firmware writes the valid byte last. The usual consumer pattern is to
see the valid byte, then dma_rmb(), then read the payload. The address of
the valid byte load depends on len, but the error_code load depends on
nothing.

On weakly ordered CPUs such as arm64 or POWER, could the error_code load
complete before the valid byte load while firmware is still writing the
response at the timeout boundary? The response buffer starts zeroed, so a
stale error_code would read as 0 and a failed command would look like a
success.

For HWRM_RING_FREE, would that hide a failure from the error handling
added later in the series, which stops DMA only when ring free fails?

The polling path has the same gap after its valid loop. This patch adds
a new consumer that relies on the completeness argument in the commit
message.

> +
> +			if (!valid_byte) {
> +				hwrm_err(bp, ctx,
> +					 "Resp cmpl intr err msg: 0x%x len:%d seq:0x%x/0x%x\n",
> +					 req_type, len,
> +					 le16_to_cpu(resp_seq_id),
> +					 le16_to_cpu(ctx->req->seq_id));
> +				goto exit;
> +			}

[Severity: Medium]
This fallback is in the generic completion ring branch, so it covers
every request that sets cmpl_ring, not only HWRM_RING_FREE. Does it change
the result of the ethtool interrupt self test?

bnxt_hwrm_selftest_irq() sends HWRM_SELFTEST_IRQ with the completion ring
set:

    req->cmpl_ring = cpu_to_le16(cmpl_ring);
    return hwrm_req_send(bp, req);

The test catches a broken interrupt path only because hwrm_req_send()
fails when NAPI never consumes the HWRM_DONE.

Firmware still writes the DMA response for the command. If the interrupt
or completion never reaches NAPI, this path now falls through and returns
error_code, which is 0:

ethtool -t <dev> offline
  bnxt_self_test()
    bnxt_test_irq()
      bnxt_hwrm_selftest_irq()
        __hwrm_send()

bnxt_self_test() marks the test failed only here:

    if (rc || bnxt_test_irq(bp)) {
        buf[BNXT_IRQ_TEST_IDX] = 1;
        etest->flags |= ETH_TEST_FL_FAILED;
    }

Would Interrupt_test then pass on a device whose interrupt delivery is
broken, with only a netdev_warn in dmesg? The commit message mentions
only the ring free case.

[Severity: Medium]
bnxt_hwrm_rx_ring_reset() raises a similar question. It sends
HWRM_RING_RESET with cmpl_ring set to the RX ring's own completion ring,
using hwrm_req_send_silent().

Before this patch, a lost completion returned -EBUSY. bnxt_rx_ring_reset()
then fell back to bnxt_reset_task(), which also re-initializes NAPI and
IRQs:

    rc = bnxt_hwrm_rx_ring_reset(bp, i);
    if (rc) {
        ...
        bnxt_reset_task(bp, true);
        break;
    }
    bnxt_free_one_rx_ring_skbs(bp, rxr);
    ...
    rxr->bnapi->in_reset = false;

Now the same case can return 0 and take the in-place recovery path.

Commit 74608fc98d28 sent the response through the ring's own completion
ring for ordering: "The response will be the last entry in the ring and
we can free the ring after getting the response." When NAPI consumes
HWRM_DONE, that shows the earlier RX completions on the ring were
discarded while in_reset was set.

Without that, can unconsumed completions stay on the cp ring and be
processed by bnxt_rx_pkt() after in_reset is cleared and the buffers are
replaced?

If the completion was lost because the ring's interrupt or NAPI is stuck,
would the in-place path leave the ring broken, where the global reset
would have recovered it?

What happens once stale entries are processed depends on runtime state,
so this one is less certain.

> +			netdev_warn(bp->dev,
> +				    "Resp cmpl intr not delivered, msg: 0x%x completed anyway (len:%d valid:0x%x err:0x%x)\n",
> +				    req_type, len, valid_byte,
> +				    le16_to_cpu(ctx->resp->error_code));

[Severity: Low]
Should this respect BNXT_HWRM_CTX_SILENT? The flag is documented as
"squelch firmware errors". The failure side of this branch uses
hwrm_err(), which drops to netdev_dbg for silent contexts.

This netdev_warn() always prints the firmware error_code. The common tail
of __hwrm_send() suppresses the same error_code for silent callers:

    if (rc == HWRM_ERR_CODE_BUSY && !(ctx->flags & BNXT_HWRM_CTX_SILENT))
        netdev_warn(bp->dev, "FW returned busy, hwrm req_type 0x%x\n",
                    req_type);
    else if (rc && rc != HWRM_ERR_CODE_PF_UNAVAILABLE)
        hwrm_err(bp, ctx, "hwrm req_type 0x%x seq id 0x%x error 0x%x\n",
                 req_type, le16_to_cpu(ctx->req->seq_id), rc);

bnxt_hwrm_rx_ring_reset() uses hwrm_req_send_silent() and goes through
this path. A lost completion there would now log a warning that includes
the firmware error code.

[ ... ]

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

Re: [PATCH net v5 3/4] bnxt_en: stop DMA before releasing rings the firmware did not free

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 4 · 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).

- [High] The fix leaves out completion/NQ rings and stat contexts.
- [High] The queue-restart path still frees ring memory after a RING_FREE
  failure.
- [Medium] bnxt_stop_dma() calls pci_clear_master() on the whole PCI
  function without quiescing anything else that uses it.
- [Medium] The trigger for an irreversible bus-master disable is broader
  than the commit justifies.
- [Medium] Bus mastering can be turned back on while stale firmware rings
  may still exist.
- [Medium] RING_FREE failures that only happen because a firmware reset
  is in progress are counted as stuck rings, and in the final series…

--

bnxt_en: stop DMA before releasing rings the firmware did not free

This commit makes bnxt_hwrm_ring_free() count TX, RX and RX AGG rings
whose HWRM_RING_FREE failed and return -EIO when any failed. The error
goes up through bnxt_hwrm_resource_free() to the close, half-close and
init_chip error paths. Those paths then call a new bnxt_stop_dma() helper,
which clears PCI bus mastering before ring memory is released.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index ea8529c789947..b5ef86bf92bf5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -7773,21 +7773,25 @@ static void bnxt_clear_one_cp_ring(struct bnxt *bp, struct bnxt_cp_ring_info *cp
>  			memset(cpr->cp_desc_ring[i], 0, size);
>  }
>  
> -static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
> +static int bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
>  {
> +	int stuck = 0;
>  	u32 type;
>  	int i;
>  
>  	if (!bp->bnapi)
> -		return;
> +		return 0;
>  
>  	for (i = 0; i < bp->tx_nr_rings; i++)
> -		bnxt_hwrm_tx_ring_free(bp, &bp->tx_ring[i], close_path);
> +		if (bnxt_hwrm_tx_ring_free(bp, &bp->tx_ring[i], close_path))
> +			stuck++;

[Severity: Medium]
Does every non-zero return here really mean the firmware did not free the
ring? hwrm_ring_free_send_msg() returns the same -EIO for several
different cases:

hwrm_ring_free_send_msg() {
	...
	if (rc || error_code) {
		netdev_err(bp->dev, "hwrm_ring_free type %d failed. rc:%x err:%x\n",
			   ring_type, rc, error_code);
		return -EIO;
	}
	...
}

A timeout ends up here. So does a firmware reply with a non-zero
error_code, for example a rejection of a ring ID the firmware no longer
tracks. Local failures from __hwrm_send() also end up here: -EBUSY when
BNXT_NO_FW_ACCESS() becomes true, or -ENOMEM from token acquisition.

The commit message only makes the case for "When HWRM_RING_FREE is not
answered". An explicit error reply means the firmware processed the
command.

Before this patch such a reply was only logged and the device could be
reopened. Now it leads to bnxt_stop_dma() and the driver has to be
rebound. Should stuck only count the case where no response arrived?

>  
>  	bnxt_cancel_dim(bp);
>  	for (i = 0; i < bp->rx_nr_rings; i++) {
> -		bnxt_hwrm_rx_ring_free(bp, &bp->rx_ring[i], close_path);
> -		bnxt_hwrm_rx_agg_ring_free(bp, &bp->rx_ring[i], close_path);
> +		if (bnxt_hwrm_rx_ring_free(bp, &bp->rx_ring[i], close_path))
> +			stuck++;
> +		if (bnxt_hwrm_rx_agg_ring_free(bp, &bp->rx_ring[i], close_path))
> +			stuck++;
>  	}

[Severity: High]
Earlier in the series these helpers were changed to return the RING_FREE
status, but the queue restart path still ignores it. Can that path free
ring memory after an unanswered RING_FREE?

bnxt_queue_stop() {
	...
	bnxt_hwrm_rx_ring_free(bp, rxr, false);
	bnxt_hwrm_rx_agg_ring_free(bp, rxr, false);
	...
	if (bp->tph_mode) {
		bnxt_hwrm_cp_ring_free(bp, rxr->rx_cpr);
		bnxt_clear_one_cp_ring(bp, rxr->rx_cpr);
	}
	...
	return 0;
}

With TPH, bnxt_tx_queue_stop() does the same for TX, then unmaps the TX
buffers straight away:

		bnxt_hwrm_tx_ring_free(bp, txr, true);
		bnxt_hwrm_cp_ring_free(bp, txr->tx_cpr);
		bnxt_free_one_tx_ring_skbs(bp, txr, txr->txq_index);
		bnxt_clear_one_cp_ring(bp, txr->tx_cpr);

bnxt_queue_stop() returns 0, so netdev_rx_queue_restart() treats the stop
as successful and calls ndo_queue_mem_free(old_mem). That frees the old
RX descriptor rings and the page_pool buffers the firmware may still be
writing to:

netdev_rx_queue_restart()
  bnxt_queue_stop()
    bnxt_hwrm_rx_ring_free()    <- -EIO, ignored
  ndo_queue_mem_free(old_mem)

This path can be reached from bnxt_tph_update() on an IRQ affinity
change, from devmem dmabuf bind/unbind, or from io_uring zcrx
registration.

Neither this patch nor the later "bnxt_en: refuse to open a device with
stopped DMA" changes these paths. Should they be handled the same way?

>  
>  	/* The completion rings are about to be freed.  After that the
> @@ -7817,6 +7821,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;

[Severity: High]
Only the TX, RX and RX AGG frees are counted in stuck. Can the firmware
still own completion or NQ ring memory when this returns 0?

bnxt_hwrm_cp_ring_free() still returns void, and it drops the result for
the L2_CMPL rings in cp_ring_arr:

bnxt_hwrm_cp_ring_free() {
	...
	hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_L2_CMPL,
				INVALID_HW_RING_ID);
	ring->fw_ring_id = INVALID_HW_RING_ID;
}

The NQ/L2_CMPL free in the loop just above also ignores its return value:

		if (ring->fw_ring_id != INVALID_HW_RING_ID) {
			hwrm_ring_free_send_msg(bp, ring, type,
						INVALID_HW_RING_ID);
			ring->fw_ring_id = INVALID_HW_RING_ID;

bnxt_hwrm_stat_ctx_free() has the same problem. It ignores each
hwrm_req_send() result and sets hw_stats_ctx_id to INVALID_STATS_CTX_ID
anyway, and bnxt_hwrm_resource_free() takes rc only from
bnxt_hwrm_ring_free().

The commit message says only a subset of RING_FREEs time out. Suppose
that subset is only CP/NQ rings or STAT_CTX_FREE. Then bnxt_stop_dma() is
skipped, and __bnxt_close_nic()/bnxt_half_close_nic() go on to
bnxt_free_mem(). That frees the completion/NQ descriptor pages through
bnxt_free_cp_rings() and bnxt_free_all_cp_arrays(). With irq_re_init it
also frees the hw_stats DMA buffers through bnxt_free_ring_stats().

The device writes to these buffers even with no traffic. It writes
completion and NQ entries, async events on cp ring 0 (registered by
bnxt_hwrm_set_async_event_cr() in bnxt_hwrm_ring_alloc()) and periodic
stats.

Doesn't that leave the IO_PAGE_FAULT case from the commit message still
reachable? The later "bnxt_en: refuse to open a device with stopped DMA"
does not change this.

> +
> +	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, the driver must be rebound to recover\n");
> +	pci_clear_master(bp->pdev);

[Severity: Medium]
The commit message says:

  "bnxt_hwrm_resource_free() still frees the remaining firmware
  resources before returning the error, so the shutdown can send every
  message it needs to before the device is stopped."

Is that accurate? The firmware DMA-writes HWRM responses into host
memory:

__hwrm_req_init() {
	...
	ctx->req->resp_addr = cpu_to_le64(dma_handle + BNXT_HWRM_RESP_OFFSET);
	...
}

Once bus mastering is cleared, every later HWRM can only time out, and
bnxt_close() still sends messages after the close:

bnxt_close() {
	...
	bnxt_close_nic(bp, true, true);
	bnxt_hwrm_shutdown_link(bp);
	bnxt_hwrm_if_change(bp, false);
	...
}

bnxt_fw_reset_close() likewise sends HWRM_FUNC_DRV_UNRGTR through
bnxt_hwrm_func_drv_unrgtr() after __bnxt_close_nic().

pci_clear_master() also acts on the whole PCI function, which the bnxt_re
auxiliary device shares. The normal close and TX timeout reset paths
(bnxt_close(), and bnxt_reset() -> bnxt_reset_task()) never call
bnxt_ulp_stop().

The driver's existing paths that disable the device all call
bnxt_ulp_stop() first: bnxt_fw_exception()/bnxt_fw_reset() before
bnxt_fw_reset_close(), bnxt_io_error_detected() and bnxt_suspend().

Would bnxt_re lose DMA, MSI-X delivery and its HWRM channel here without
being notified?

A separate concern, an immediate reopen through bnxt_reset_task() ->
bnxt_open_nic(), is fixed later in the series by "bnxt_en: refuse to
open a device with stopped DMA". The ULP and post-stop HWRM issues are
still present at the end of the series.

[Severity: Medium]
Can bus mastering be turned back on while the firmware still has the
stuck rings? No caller of pci_set_master() in the driver checks for the
stopped state first. For example:

bnxt_resume() {
	...
	pci_set_master(bp->pdev);
	if (bnxt_hwrm_ver_get(bp)) {
		rc = -ENODEV;
		goto resume_exit;
	}
	rc = bnxt_hwrm_func_reset(bp);
	if (rc) {
		rc = -EBUSY;
		goto resume_exit;
	}
	...
}

VER_GET or FUNC_RESET may well fail if the firmware has already stopped
answering RING_FREE. Resume then exits with bus mastering on and nothing
reset.

If the function kept its ring context across suspend (D3hot without an
internal reset), could the stuck RX rings then DMA into pages that
bnxt_free_mem() has already released?

bnxt_fw_reset_task() also calls pci_set_master() unconditionally in its
ENABLE_DEV state.

The later "bnxt_en: refuse to open a device with stopped DMA" only blocks
bnxt_open*(). So "the driver must be rebound to recover" holds for open,
but not for bus mastering being re-enabled.

>  }
>  
>  static int __bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,

[ ... ]

> @@ -13540,7 +13561,8 @@ static void __bnxt_close_nic(struct bnxt *bp, bool irq_re_init,
>  	if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp))
>  		bnxt_clear_rss_ctxs(bp);
>  	/* Flush rings and disable interrupts */
> -	bnxt_shutdown_nic(bp, irq_re_init);
> +	if (bnxt_shutdown_nic(bp, irq_re_init))
> +		bnxt_stop_dma(bp);

[Severity: Medium]
What happens when the only reason these RING_FREEs fail is a firmware
reset in progress?

NAPI and bnxt_timer are still running here, because bnxt_disable_napi()
and timer_delete_sync() come after bnxt_shutdown_nic(). The async
RESET_NOTIFY handler can set BNXT_STATE_FW_FATAL_COND after
hwrm_ring_free_send_msg() has passed its BNXT_NO_FW_ACCESS() check.
__hwrm_send() then returns -EBUSY.

__hwrm_send() also abandons the wait if the firmware health register
reports not healthy:

__hwrm_send() {
	...
				if (hwrm_wait_must_abort(bp, req_type, &sts)) {
					hwrm_err(bp, ctx, "Resp cmpl intr abandoning msg: 0x%x due to firmware status: 0x%x\n",
						 req_type, sts);
					goto exit;
	...
}

bnxt_close_nic() already notes that "Some firmware messages may fail as
we proceed to close" when BNXT_STATE_IN_FW_RESET is set. With this
patch, each such failure becomes -EIO, stuck++ and then bnxt_stop_dma().

That includes the FW reset close path itself:

bnxt_fw_reset_task()
  bnxt_fw_reset_close()
    __bnxt_close_nic()
      bnxt_shutdown_nic()    <- -EIO
      bnxt_stop_dma()

After the later "bnxt_en: refuse to open a device with stopped DMA",
bnxt_stop_dma() also sets BNXT_STATE_DMA_STOPPED, and nothing clears it.

The reset still re-enables bus mastering in ENABLE_DEV. But in the
OPENING state, bnxt_open() then fails in bnxt_check_dma_stopped() with
-ENODEV, and bnxt_fw_reset_abort() runs.

If a close races with a reset, bnxt_close_nic() sets ABORT_ERR. A later
bnxt_open() is then refused before it reaches bnxt_reinit_after_abort(),
which doesn't call pci_set_master() anyway.

Before this patch a coordinated FW reset could recover the device. Is it
intended that a rebind is now the only way to recover?

>  
>  	/* TODO CHIMP_FW: Link/PHY related cleanup if (link_re_init) */
>

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

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

[netdev-bot+sashiko]

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