[RFC net v2 0/3] bnxt_en: Make RING FREE more robust

[RFC net v2 0/3] bnxt_en: Make RING FREE more robust

[Joe Damato]

Greetings:

This is a follow up to the previous RFC (linked below), which takes a slightly
different approach, is now targeted at net instead of net-next, and has a new
subject line :)

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.

This series tries to prevent this by:

 - Returning and checking ring free command return values
 - Examining the FW response if the ring free command times out. It is
   possible that, for some reason, the FW did complete the ring free but was
   unable to respond with an IRQ. This seems unlikely given what appears to be
   a use after free in dmesg, but worth logging just in case.
 - Lastly, disable the device to stop DMA before the driver frees ring memory,
   which should prevent any possible use after free.

Sending this as an RFC so that the Broadcom folks have some time to take a
look and test as needed.

Thanks,
Joe

v2:
  - 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 (3):
  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

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 69 +++++++++++------
 .../net/ethernet/broadcom/bnxt/bnxt_hwrm.c    | 75 ++++++++++++++-----
 2 files changed, 102 insertions(+), 42 deletions(-)


base-commit: 17741334d00bf5ebd37f8c1c36bc9c146a351deb
-- 
2.53.0-Meta

[RFC net v2 1/3] 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 d7728d0c5b6e..a7f6facca7b4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7660,50 +7660,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;
@@ -7711,14 +7716,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

[RFC net v2 2/3] 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 appears then the firmware completed the command and only the
notification was lost. Fall through to the normal error_code handling in
that case.

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    | 75 ++++++++++++++-----
 1 file changed, 57 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
index 5bfabdca7d0e..c494abb71c51 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
@@ -456,6 +456,30 @@ static bool hwrm_wait_must_abort(struct bnxt *bp, u32 req_type, u32 *fw_status)
 	return *fw_status && !BNXT_FW_IS_HEALTHY(*fw_status);
 }
 
+/* Wait for the firmware to set the valid byte at the end of the response.
+ * Returns the number of usec spent waiting; a return of
+ * HWRM_VALID_BIT_DELAY_USEC or more means the byte never appeared.
+ */
+static int hwrm_wait_for_valid(u8 *valid)
+{
+	int j;
+
+	for (j = 0; j < HWRM_VALID_BIT_DELAY_USEC; ) {
+		/* make sure we read from updated DMA memory */
+		dma_rmb();
+		if (*valid)
+			break;
+		if (j < 10) {
+			udelay(1);
+			j++;
+		} else {
+			usleep_range(20, 30);
+			j += 20;
+		}
+	}
+	return j;
+}
+
 static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
 {
 	u32 doorbell_offset = BNXT_GRCPF_REG_CHIMP_COMM_TRIGGER;
@@ -582,12 +606,39 @@ 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;
+			bool completed = false;
+			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.
+			 */
+			len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
+			if (len &&
+			    READ_ONCE(ctx->resp->seq_id) == ctx->req->seq_id) {
+				valid = (u8 *)ctx->resp + len - 1;
+				completed = hwrm_wait_for_valid(valid) <
+					    HWRM_VALID_BIT_DELAY_USEC;
+				valid_byte = *valid;
+			}
+			if (!completed) {
+				hwrm_err(bp, ctx,
+					 "Resp cmpl intr err msg: 0x%x len:%d valid:0x%x seq:0x%x/0x%x\n",
+					 req_type, len, valid_byte,
+					 le16_to_cpu(READ_ONCE(ctx->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));
+			valid = ((u8 *)ctx->resp) + len - 1;
 		}
-		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 */
 		int j;
@@ -647,19 +698,7 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
 
 		/* Last byte of resp contains valid bit */
 		valid = ((u8 *)ctx->resp) + len - 1;
-		for (j = 0; j < HWRM_VALID_BIT_DELAY_USEC; ) {
-			/* make sure we read from updated DMA memory */
-			dma_rmb();
-			if (*valid)
-				break;
-			if (j < 10) {
-				udelay(1);
-				j++;
-			} else {
-				usleep_range(20, 30);
-				j += 20;
-			}
-		}
+		j = hwrm_wait_for_valid(valid);
 
 		if (j >= HWRM_VALID_BIT_DELAY_USEC) {
 			hwrm_err(bp, ctx, "Error (timeout: %u) msg {0x%x 0x%x} len:%d v:%d\n",
-- 
2.53.0-Meta

[RFC net v2 3/3] 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, if there are any, disable
the device before the close path releases memory.

The device stays unusable until a firmware reset.

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 | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index a7f6facca7b4..574532d7047b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7756,6 +7756,7 @@ static void bnxt_clear_one_cp_ring(struct bnxt *bp, struct bnxt_cp_ring_info *cp
 
 static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
 {
+	int stuck = 0;
 	u32 type;
 	int i;
 
@@ -7763,12 +7764,15 @@ static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path)
 		return;
 
 	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
@@ -7798,6 +7802,17 @@ 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;
+
+	/* FW never acknowledged freeing these rings, so it may still be
+	 * DMAing to them. Stop the device before handing memory back.
+	 */
+	netdev_err(bp->dev,
+		   "Firmware did not free %d ring(s); disabling DMA before releasing ring memory. A firmware reset is required.\n",
+		   stuck);
+	pci_disable_device(bp->pdev);
 }
 
 static int __bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max,
-- 
2.53.0-Meta

Re: [RFC net v2 2/3] bnxt_en: check HWRM response if completion never arrives

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

On Tue, Sep 22, 2026 at 11:24 AM Joe Damato <joe@dama.to> wrote:

> @@ -582,12 +606,39 @@ 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;
> +                       bool completed = false;
> +                       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.
> +                        */
> +                       len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
> +                       if (len &&
> +                           READ_ONCE(ctx->resp->seq_id) == ctx->req->seq_id) {
> +                               valid = (u8 *)ctx->resp + len - 1;
> +                               completed = hwrm_wait_for_valid(valid) <
> +                                           HWRM_VALID_BIT_DELAY_USEC;

I feel that it is not necessary to wait for the valid bit in this
case.  We have already waited several seconds for the interrupt and it
never came.  By now, the whole response should be valid if we got it.

This differs from the polling case below which polls for a non-zero
length.  As soon as we see the length, we need to wait a bit for the
valid bit which is at the end of the message.

> +                               valid_byte = *valid;
> +                       }
> +                       if (!completed) {
> +                               hwrm_err(bp, ctx,
> +                                        "Resp cmpl intr err msg: 0x%x len:%d valid:0x%x seq:0x%x/0x%x\n",
> +                                        req_type, len, valid_byte,
> +                                        le16_to_cpu(READ_ONCE(ctx->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));
> +                       valid = ((u8 *)ctx->resp) + len - 1;
>                 }
> -               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 */
>                 int j;

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

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

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Tue, Sep 22, 2026 at 11:24 AM Joe Damato <joe@dama.to> wrote:

> @@ -7798,6 +7802,17 @@ 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;
> +
> +       /* FW never acknowledged freeing these rings, so it may still be
> +        * DMAing to them. Stop the device before handing memory back.
> +        */
> +       netdev_err(bp->dev,
> +                  "Firmware did not free %d ring(s); disabling DMA before releasing ring memory. A firmware reset is required.\n",
> +                  stuck);
> +       pci_disable_device(bp->pdev);

I think it may be better to return error and let the caller decide
what actions to take.  For example, bnxt_hwrm_resource_free() might
decide to finish all the remaining steps to free all the FW resources
before taking any actions.  Disabling DMA now will prevent us from
sending any remaining messages to the FW to complete the shutdown.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]