Thread (5 messages) flat view 5 messages, 1 author, 1d ago
WARM1d

Revision v1 of 19 in this series.

Revisions (19)
  1. v1 [diff vs current]
  2. v1 [diff vs current]
  3. v1 [diff vs current]
  4. v1 [diff vs current]
  5. v1 [diff vs current]
  6. v1 [diff vs current]
  7. v1 [diff vs current]
  8. v1 [diff vs current]
  9. v1 [diff vs current]
  10. v1 [diff vs current]
  11. v1 [diff vs current]
  12. v1 [diff vs current]
  13. v1 [diff vs current]
  14. v1 [diff vs current]
  15. v1 [diff vs current]
  16. v1 [diff vs current]
  17. v2 [diff vs current]
  18. v1 [diff vs current]
  19. v1 current

[PATCH net 1/4] bnxt_en: Fix ring accounting underflow when rings are constrained

From: Michael Chan <michael.chan@broadcom.com>
Date: 2026-08-31 02:44:34
Subsystem: broadcom bnxt_en 50 gigabit ethernet driver, networking drivers, the rest · Maintainers: Michael Chan, Pavan Chebbi, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds

When __bnxt_reserve_rings() reserves fewer TX rings than requested,
and an XDP program is attached, bnxt_adj_tx_rings() blindly subtracts
bp->tx_nr_rings_xdp from bp->tx_nr_rings, potentially causing the
result to be negative (large value).  The large value will propagate
and cause unpredictable failures.

bnxt_adj_tx_rings() should scale down the TX rings for XDP and TCs
evenly when there is a shortage of TX rings to be correct.  Because
XDP requires a 1:1 mapping with RX rings in combined channel mode,
bp->tx_nr_rings_xdp must be equal to bp->tx_nr_rings_per_tc.  Any
leftover rings after integer division is intentionally left unused.
This will fix the underflow resulting in a negative (large) value.

Additionally, update bnxt_rings_ok() to require a minimum number of
TX rings based on the active configuration (at least 1 ring per TC,
plus 1 XDP ring if XDP is enabled). This guarantees that
bnxt_adj_tx_rings() always has enough rings to satisfy the minimum
viable configuration, gracefully failing the reservation otherwise.

The bnxt_rings_ok() check in __bnxt_reserve_rings() is moved earlier
to return -ENOMEM if we don't have the bare minimum resources before
we commit and update the software state.  Also add a check for
bnxt_trim_rings() failure earlier in the same function for the
same purpose.

Now that we have the proper bnxt_rings_ok() check for the bare
minimum and a more robust bnxt_adj_tx_rings() to handle fewer rings
than requested, we can remove the error path at the end of
bnxt_reserve_rings() that would abort if the rings could not
satisfy the TC requirements.

This existing issue was detetced by Sashiko when reviewing the
new kTLS patchset (patch #3 of 15):

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810051358.1244418-7-michael.chan@broadcom.com

Fixes: 1ee581c24dfd ("bnxt_en: Adjust TX rings if reservation is less than requested")
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 45 ++++++++++++-----------
 1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d59bcca73a2b..219a6f551f1d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -8135,8 +8135,14 @@ static void bnxt_copy_reserved_rings(struct bnxt *bp, struct bnxt_hw_rings *hwr)
 
 static bool bnxt_rings_ok(struct bnxt *bp, struct bnxt_hw_rings *hwr)
 {
-	return hwr->tx && hwr->rx && hwr->cp && hwr->grp && hwr->vnic &&
-	       hwr->stat && (hwr->cp_p5 || !(bp->flags & BNXT_FLAG_CHIP_P5_PLUS));
+	int min_tx = bp->num_tc ? bp->num_tc : 1;
+
+	if (bp->tx_nr_rings_xdp)
+		min_tx++;
+
+	return hwr->tx >= min_tx && hwr->rx && hwr->cp && hwr->grp &&
+	       hwr->vnic && hwr->stat &&
+	       (hwr->cp_p5 || !(bp->flags & BNXT_FLAG_CHIP_P5_PLUS));
 }
 
 static int bnxt_get_avail_msix(struct bnxt *bp, int num);
@@ -8211,10 +8217,16 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
 		hwr.stat -= bnxt_get_ulp_stat_ctxs(bp);
 	hwr.cp = min_t(int, hwr.cp, hwr.stat);
 	rc = bnxt_trim_rings(bp, &rx_rings, &hwr.tx, hwr.cp, sh);
+	if (rc)
+		return rc;
 	if (bp->flags & BNXT_FLAG_AGG_RINGS)
 		hwr.rx = rx_rings << 1;
 	tx_cp = bnxt_num_tx_to_cp(bp, hwr.tx);
 	hwr.cp = sh ? max_t(int, tx_cp, rx_rings) : tx_cp + rx_rings;
+
+	if (!bnxt_rings_ok(bp, &hwr))
+		return -ENOMEM;
+
 	if (hwr.tx != bp->tx_nr_rings) {
 		netdev_warn(bp->dev,
 			    "Able to reserve only %d out of %d requested TX rings\n",
@@ -8243,9 +8255,6 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
 	    hwr.rss_ctx < bnxt_get_total_rss_ctxs(bp, &hwr))
 		bp->rss_cap &= ~BNXT_RSS_CAP_LARGE_RSS_CTX;
 
-	if (!bnxt_rings_ok(bp, &hwr))
-		return -ENOMEM;
-
 	if (old_rx_rings != bp->hw_resc.resv_rx_rings &&
 	    !netif_is_rxfh_configured(bp->dev))
 		bnxt_set_dflt_rss_indir_tbl(bp, NULL);
@@ -11663,7 +11672,6 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 	struct bnxt_en_dev *edev = bp->edev[BNXT_AUXDEV_RDMA];
 	bool irq_cleared = false;
 	bool irq_change = false;
-	int tcs = bp->num_tc;
 	int irqs_required;
 	int rc;
 
@@ -11701,17 +11709,6 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
 		return rc;
 	}
-	if (tcs && (bp->tx_nr_rings_per_tc * tcs !=
-		    bp->tx_nr_rings - bp->tx_nr_rings_xdp)) {
-		netdev_err(bp->dev, "tx ring reservation failure\n");
-		netdev_reset_tc(bp->dev);
-		bp->num_tc = 0;
-		if (bp->tx_nr_rings_xdp)
-			bp->tx_nr_rings_per_tc = bp->tx_nr_rings_xdp;
-		else
-			bp->tx_nr_rings_per_tc = bp->tx_nr_rings;
-		return -ENOMEM;
-	}
 	return 0;
 }
 
@@ -13216,11 +13213,17 @@ static void bnxt_set_xdp_tx_rings(struct bnxt *bp)
 
 static void bnxt_adj_tx_rings(struct bnxt *bp)
 {
+	int tcs = bp->num_tc ? bp->num_tc : 1;
+
 	/* Make adjustments if reserved TX rings are less than requested */
-	bp->tx_nr_rings -= bp->tx_nr_rings_xdp;
-	bp->tx_nr_rings_per_tc = bnxt_tx_nr_rings_per_tc(bp);
-	if (bp->tx_nr_rings_xdp)
-		bnxt_set_xdp_tx_rings(bp);
+	if (bp->tx_nr_rings_xdp) {
+		tcs++;
+		bp->tx_nr_rings_per_tc = bp->tx_nr_rings / tcs;
+		bp->tx_nr_rings_xdp = bp->tx_nr_rings_per_tc;
+	} else {
+		bp->tx_nr_rings_per_tc = bnxt_tx_nr_rings_per_tc(bp);
+	}
+	bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tcs;
 }
 
 static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
-- 
2.51.0
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help