[RFC net-next 0/2] bnxt_en: Recover from failed TX RING_FREE

[RFC net-next 0/2] bnxt_en: Recover from failed TX RING_FREE

[Joe Damato]

Greetings:

This series makes a failed TX HWRM_RING_FREE recoverable instead of silently
releasing ring memory the FW might still own to address a use after free I
noticed on a production system.

I'm posting this as an RFC because:
  - I am not sure if patch 2 is correct. Maybe Broadcom can let me know ?
  - I am not sure if this is a Fixes or not. I guess if it was always like
    this then this is net-next material?

The core issue is that bnxt_hwrm_tx_ring_free discards the return value of
hwrm_ring_free_send_msg and sets fw_ring_id = INVALID_HW_RING_ID
unconditionally. When a TX RING_FREE sent over a completion ring times out,
the driver logs:

  hwrm_ring_free type 1 failed. rc:fffffff0 err:0
  Resp cmpl intr err msg: 0x51

and then __bnxt_close_nic() calls bnxt_free_mem(), unmapping ring memory that
the FW was never told to release.

bnxt_queue_stop mentions something about this in a comment:

  "HWRM_RING_FREE completion is handled in NAPI to guarantee no more DMA
   on that ring after seeing the completion."

But... if the completion ring is dead, the completion never arrives.

This is reachable on production systems today:

  - TX completions stop
  - netdev watchdog fires
  - reset closes the device
  - every RING_FREE routed through that dead completion ring times out
  - ring memory freed by the driver but still in use by the FW

The result on an IOMMU host is IO_PAGE_FAULT or DMAR fault against freed
pages.

I tried to test the code in patch 2 on a BCM57504 with FW 235.1.208.0/pkg
235.1.208.0.

I hacked something together to inject a failure to test the reset paths on my
device. It seems like HWRM_RING_RESET ring_type=TX is accepted by thte FW and
the polled RING_FREE also succeeds, but in my testing the TX ring was idle. I
never tested a reset against a ring with descriptors in flight. Which leads me
to my questions.....

1. Does HWRM_RING_RESET with ring_type=TX cause the FW to abandon work already
outstanding on that ring and stop DMA ? If not .... then this code is wrong :(
and maybe see question (3) below.

2. Is a TX ring reset supposed to post a completion ring entry? In my testing
it seemed like the FW may have written success into the response DMA buffer
but never posted the entry, so the request times out with -EBUSY even though
it succeeded. Is that intentional? If so, maybe only polled transport mode
works on this FW?

3. Maybe the TX reset isn't necessary at all? Maybe instead the code should
retry the RING_FREE over polled transport and that's good enough? This depends
on the answer to question (1) above, but I guess it would simplify the code if
a polled RING_FREE is enough?

Thanks,
Joe

Joe Damato (2):
  bnxt_en: return status from bnxt_hwrm_tx_ring_free
  bnxt_en: recover a failed TX RING_FREE with a ring reset

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 55 ++++++++++++++++++++---
 1 file changed, 49 insertions(+), 6 deletions(-)


base-commit: 5ccdfb2c3203207deb17e7c5b0db8c7f475639a7
-- 
2.53.0-Meta

[RFC net-next 1/2] bnxt_en: return status from bnxt_hwrm_tx_ring_free

[Joe Damato]

hwrm_ring_free_send_msg() already reports failure to its caller,
returning -EIO when the firmware rejects HWRM_RING_FREE or never
answers it. bnxt_hwrm_tx_ring_free() discards that value.

Return it instead. No caller acts on it yet, so there is no functional
change; this only makes the failure observable so that the next patches
can recover from it.

Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index ca99f4b1a63c..ac7716dbf88d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7660,21 +7660,23 @@ 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,
-- 
2.53.0-Meta

[RFC net-next 2/2] bnxt_en: recover a failed TX RING_FREE with a ring reset

[Joe Damato]

When HWRM_RING_FREE is sent with a completion ring set and the completion
never arrives, hwrm_ring_free_send_msg() logs

  hwrm_ring_free type 1 failed. rc:fffffff0 err:0
  Resp cmpl intr err msg: 0x51

and returns -EIO. bnxt_hwrm_tx_ring_free() clears fw_ring_id anyway and
__bnxt_close_nic() calls bnxt_free_mem(), unmapping ring memory
the firmware has not been told to stop using.

On a host with an IOMMU the result is an IO_PAGE_FAULT or a DMAR fault
against the freed pages seconds later.

A TX ring whose completions have stopped will trigger the netdev
watchdog, the reset closes the device, and every RING_FREE routed
through that dead completion ring times out.

Try to recover from this by adding bnxt_tx_ring_reset_and_free and
bnxt_hwrm_tx_ring_reset.

The intent here is that bnxt_tx_ring_reset_and_free will try a TX ring reset
in polled mode (assuming the completion ring is dead) and, if that succeeds,
retry the RING_FREE in polled mode as well.

Bump tx_resets in this path. A non-zero ethtool tx_resets value on
a device that never tripped the netdev watchdog would identify a
firmware that stopped answering RING_FREE on a completion ring.

Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 41 +++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index ac7716dbf88d..c47f6f24dfa4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7660,6 +7660,44 @@ static int hwrm_ring_free_send_msg(struct bnxt *bp,
 	return 0;
 }
 
+static int bnxt_hwrm_tx_ring_reset(struct bnxt *bp,
+				   struct bnxt_tx_ring_info *txr)
+{
+	struct bnxt_ring_struct *ring = &txr->tx_ring_struct;
+	struct hwrm_ring_reset_input *req;
+	int rc;
+
+	rc = hwrm_req_init(bp, req, HWRM_RING_RESET);
+	if (rc)
+		return rc;
+
+	req->ring_type = RING_RESET_REQ_RING_TYPE_TX;
+	req->ring_id = cpu_to_le16(ring->fw_ring_id);
+	return hwrm_req_send_silent(bp, req);
+}
+
+static int bnxt_tx_ring_reset_and_free(struct bnxt *bp,
+				       struct bnxt_tx_ring_info *txr)
+{
+	struct bnxt_ring_struct *ring = &txr->tx_ring_struct;
+	struct bnxt_cp_ring_info *cpr;
+	int rc;
+
+	rc = bnxt_hwrm_tx_ring_reset(bp, txr);
+	if (rc) {
+		netdev_err(bp->dev,
+			   "TX ring %d reset failed after RING_FREE failed, rc: %d\n",
+			   txr->txq_index, rc);
+		return rc;
+	}
+
+	cpr = &txr->bnapi->cp_ring;
+	cpr->sw_stats->tx.tx_resets++;
+
+	return hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_TX,
+				       INVALID_HW_RING_ID);
+}
+
 static int bnxt_hwrm_tx_ring_free(struct bnxt *bp,
 				  struct bnxt_tx_ring_info *txr,
 				  bool close_path)
@@ -7675,6 +7713,9 @@ static int bnxt_hwrm_tx_ring_free(struct bnxt *bp,
 		       INVALID_HW_RING_ID;
 	rc = hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_TX,
 				     cmpl_ring_id);
+	if (rc && cmpl_ring_id != INVALID_HW_RING_ID)
+		rc = bnxt_tx_ring_reset_and_free(bp, txr);
+
 	ring->fw_ring_id = INVALID_HW_RING_ID;
 	return rc;
 }
-- 
2.53.0-Meta

Re: [RFC net-next 0/2] bnxt_en: Recover from failed TX RING_FREE

[Michael Chan]

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

On Thu, Sep 17, 2026 at 4:32 PM Joe Damato <joe@dama.to> wrote:

> But... if the completion ring is dead, the completion never arrives.
>
> This is reachable on production systems today:
>
>   - TX completions stop
>   - netdev watchdog fires
>   - reset closes the device
>   - every RING_FREE routed through that dead completion ring times out
>   - ring memory freed by the driver but still in use by the FW
>
> The result on an IOMMU host is IO_PAGE_FAULT or DMAR fault against freed
> pages.

If the FW did not receive the HWRM_RING_FREE command from the driver,
there is no guarantee that DMA will stop.  I agree that the driver is
not very robust in handling this in the close path.  We just continue
after the timeout hoping that the FW actually received it but couldn't
respond.

In contrast, the error recovery path for fatal errors is a little more
robust.  For example, in bnxt_fw_fatal_close(), we call
pci_disable_device() to stop DMA from the device.

We probably should do something similar in the close path when
HWRM_RING_FREE is not responding at all.  Maybe even FLR.

>
> I tried to test the code in patch 2 on a BCM57504 with FW 235.1.208.0/pkg
> 235.1.208.0.
>
> I hacked something together to inject a failure to test the reset paths on my
> device. It seems like HWRM_RING_RESET ring_type=TX is accepted by thte FW and
> the polled RING_FREE also succeeds, but in my testing the TX ring was idle. I
> never tested a reset against a ring with descriptors in flight. Which leads me
> to my questions.....
>
> 1. Does HWRM_RING_RESET with ring_type=TX cause the FW to abandon work already
> outstanding on that ring and stop DMA ? If not .... then this code is wrong :(
> and maybe see question (3) below.

HWRM_RING_RESET is not supported on BCM5750X and BCM5760X.  Even if
the FW accepts it, it probably does nothing internally.

>
> 3. Maybe the TX reset isn't necessary at all? Maybe instead the code should
> retry the RING_FREE over polled transport and that's good enough? This depends
> on the answer to question (1) above, but I guess it would simplify the code if
> a polled RING_FREE is enough?
>

If interrupt is not working and the HWRM_RING_FREE doesn't complete
using the interrupt path, it will timeout using the same FW polling
timeout.  In other words, if FW received it but just couldn't post the
completion on the completion ring and interrupt, the wait should be
long enough for it to complete.   One additional thing we can do that
we're currently not doing is to check for a valid response length and
check the valid bit anyway on interrupt timeout.  If these look valid,
the FW has accepted the HWRM_RING_FREE but just couldn't interrupt.
If these are not valid, sending a second HWRM_RING_FREE should yield
the same results.

Thanks for looking into this.

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