[PATCH net v2 0/9] bnxt_en: Bug fixes

[PATCH net v2 0/9] bnxt_en: Bug fixes

[Michael Chan]

This patchset includes fixes and some required refactoring.  The
first 5 patches are expanded fixes and refactoring since v1.
These first 5 new patches plus the revised patch #6 make the fix
more complete and address v1 issues reported by Sashiko.

The first 6 patches fix ring accounting logic when FW is unable to
reserve all the rings requested by the driver.  The driver will
scale down the rings when this happens, but there are multiple
issues with the existing code.  The first 6 patches fix all the
issues related to this code path, including resetting the deafult
RSS context if necessary.  Note that additional RSS contexts and
n-tuple filters have similar problems if the RX rings are reduced
and the fixes are deferred to a future patchset.

The 7th patch is unchanged.  It refactors a bnxt_clear_bars() helper
needed by patch 8 and 9.  Patch 8 fixes a possible issue during
driver init. in the kdump kernel by rewriting the BARs after FLR.
Patch 9 is a similar fix int the PCIe AER code path for non-fatal
errors.  Both patch 8 and patch 9 have been revised to address
Sashiko issues on v1.

v1: https://lore.kernel.org/netdev/20260831024342.2161156-1-michael.chan@broadcom.com/

Michael Chan (8):
  bnxt_en: Clear bp->total_irqs in bnxt_init_int_mode() during error
  bnxt_en: Fix bnxt_reinit_features() when irq_re_init is true
  bnxt_en: Refactor RSS table check logic
  bnxt_en: Refactor IRQs required logic
  bnxt_en: Reinit IRQ when configuring LRO/GRO/HDS
  bnxt_en: Fix ring accounting and validation when rings are constrained
  bnxt_en: Add bnxt_clear_bars() helper
  bnxt_en: Fix driver init in kdump kernel

Pavan Chebbi (1):
  bnxt_en: Re-write the BARs following any type of PCIe errors

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 418 ++++++++++++------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   2 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  10 +-
 3 files changed, 295 insertions(+), 135 deletions(-)

-- 
2.51.0

[PATCH net v2 1/9] bnxt_en: Clear bp->total_irqs in bnxt_init_int_mode() during error

[Michael Chan]

During error, bnxt_init_int_mode() will free bp->irq_tbl but
bp->total_irqs retains the old value.  If a subsequent
reinitialization happens, bnxt_reserve_rings() may see that
bp->total_irqs does not match a new irqs_required.  It will then
try to adjust if dynamic MSI-X is supported and call
bnxt_change_msix().  It will then crash when dereferencing the NULL
bp->irq_tbl.

Fix it by clearing bp->total_irqs when freeing bp->irq_tbl.  Dynamic
MSI-X adjustments should only proceed if bp->irq_tbl is valid which
means that MSI-X has been initialized and can be adjusted.

Add a bp->irq_tbl_size to prevent OOB bp->irq_tbl[] array access in
case MSI-X capabilities change during re-init.  For dynamic MSI-X,
allocate the biggest bp->irq_tbl that is not clamped by CP/NQ rings
to allow dynamic MSI-X to grow to the max.

In the AER path, if MSI-X capabilities change, bnxt_reserve_rings()
will initialize MSI-X if BNXT_NEW_RM() is true.  Add a check in
bnxt_io_resume() to skip doing it again later.

Fixes: e68256c8a73c ("bnxt_en: Support dynamic MSIX")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 18 ++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d7728d0c5b6e..51557ee6c9ad 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11511,6 +11511,12 @@ static int bnxt_change_msix(struct bnxt *bp, int total)
 	struct msi_map map;
 	int i;
 
+	if (!bp->irq_tbl)
+		return 0;
+
+	if (total > bp->irq_tbl_size)
+		return bp->total_irqs;
+
 	/* add MSIX to the end if needed */
 	for (i = bp->total_irqs; i < total; i++) {
 		map = pci_msix_alloc_irq_at(bp->pdev, i, NULL);
@@ -11653,12 +11659,13 @@ static int bnxt_init_int_mode(struct bnxt *bp)
 
 	tbl_size = total_vecs;
 	if (pci_msix_can_alloc_dyn(bp->pdev))
-		tbl_size = max;
+		tbl_size = bp->hw_resc.max_irqs;
 	bp->irq_tbl = kzalloc_objs(*bp->irq_tbl, tbl_size);
 	if (!bp->irq_tbl) {
 		rc = -ENOMEM;
 		goto msix_setup_exit;
 	}
+	bp->irq_tbl_size = tbl_size;
 
 	for (i = 0; i < total_vecs; i++)
 		bp->irq_tbl[i].vector = pci_irq_vector(bp->pdev, i);
@@ -11681,6 +11688,8 @@ static int bnxt_init_int_mode(struct bnxt *bp)
 	netdev_err(bp->dev, "bnxt_init_int_mode err: %x\n", rc);
 	kfree(bp->irq_tbl);
 	bp->irq_tbl = NULL;
+	bp->total_irqs = 0;
+	bp->irq_tbl_size = 0;
 	pci_free_irq_vectors(bp->pdev);
 	return rc;
 }
@@ -11691,6 +11700,7 @@ static void bnxt_clear_int_mode(struct bnxt *bp)
 
 	kfree(bp->irq_tbl);
 	bp->irq_tbl = NULL;
+	bp->irq_tbl_size = 0;
 }
 
 int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
@@ -11717,7 +11727,7 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 
 	if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) {
 		irq_change = true;
-		if (!pci_msix_can_alloc_dyn(bp->pdev)) {
+		if (!pci_msix_can_alloc_dyn(bp->pdev) || !bp->irq_tbl) {
 			bnxt_ulp_irq_stop(bp);
 			bnxt_clear_int_mode(bp);
 			irq_cleared = true;
@@ -15081,7 +15091,7 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 			hwr.cp += bnxt_get_ulp_msix_num(bp);
 			hwr.cp = min_t(int, hwr.cp, bnxt_get_max_func_irqs(bp));
 		}
-		if (hwr.cp > bp->total_irqs) {
+		if (bp->irq_tbl && hwr.cp > bp->total_irqs) {
 			int total_msix = bnxt_change_msix(bp, hwr.cp);
 
 			if (total_msix < hwr.cp) {
@@ -17692,7 +17702,7 @@ static void bnxt_io_resume(struct pci_dev *pdev)
 			err = bnxt_open(netdev);
 		} else {
 			err = bnxt_reserve_rings(bp, true);
-			if (!err)
+			if (!err && !bp->irq_tbl)
 				err = bnxt_init_int_mode(bp);
 		}
 	}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index c673b2ce4a0d..a757d8258f71 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2490,6 +2490,7 @@ struct bnxt {
 	 */
 	unsigned long		*ring_affinity_set;
 	int			max_irqs;
+	int			irq_tbl_size;
 	int			total_irqs;
 	int			ulp_num_msix_want;
 	u8			mac_addr[ETH_ALEN];
-- 
2.51.0

Re: [PATCH net v2 1/9] bnxt_en: Clear bp->total_irqs in bnxt_init_int_mode() during error

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The bnxt_io_resume() hunk (`if (!err && !bp->irq_tbl) err =
  bnxt_init_int_mode(bp);`) fixes a defect introduced by c2d20a3814d1…

--

bnxt_en: Clear bp->total_irqs in bnxt_init_int_mode() during error

This patch clears bp->total_irqs when bnxt_init_int_mode() fails, so that
bnxt_reserve_rings() and bnxt_check_rings() do not call
bnxt_change_msix() with a NULL bp->irq_tbl. It adds bp->irq_tbl_size to
bound dynamic MSI-X growth. It also skips the second bnxt_init_int_mode()
call in bnxt_io_resume() when bnxt_reserve_rings() has already
initialized MSI-X.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index d7728d0c5b6e6..51557ee6c9ade 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -17692,7 +17702,7 @@ static void bnxt_io_resume(struct pci_dev *pdev)
>  			err = bnxt_open(netdev);
>  		} else {
>  			err = bnxt_reserve_rings(bp, true);
> -			if (!err)
> +			if (!err && !bp->irq_tbl)
>  				err = bnxt_init_int_mode(bp);
>  		}
>  	}

[Severity: Low]
This isn't a bug, but should the patch also carry a Fixes tag for
c2d20a3814d1 ("bnxt_en: delay pci_alloc_irq_vectors() in the AER path")?

The patch only has:

    Fixes: e68256c8a73c ("bnxt_en: Support dynamic MSIX")

Commit c2d20a3814d1 added the unconditional bnxt_init_int_mode() call
after bnxt_reserve_rings() in bnxt_io_resume(), and it is newer than
e68256c8a73c.

The double init can happen without dynamic MSI-X:

bnxt_io_slot_reset()
    bnxt_clear_int_mode()

bnxt_io_resume()
    bnxt_reserve_rings(bp, true)
        /* BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs */
        bnxt_clear_int_mode()
        bnxt_init_int_mode()      <-- MSI-X enabled here
    bnxt_init_int_mode()
        pci_alloc_irq_vectors()
            __pci_enable_msix_range()
                WARN_ON_ONCE(dev->msix_enabled) -> -EINVAL

The second bnxt_init_int_mode() then goes to its error path. It frees
bp->irq_tbl and the vectors and returns -ENODEV, so netif_device_attach()
is never called and the device stays detached after AER recovery.

Because e68256c8a73c is older, stable selection probably already covers
every tree that has c2d20a3814d1. Trees that have e68256c8a73c but not
c2d20a3814d1 would not need this hunk, though.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928041712.3467803-1-michael.chan%40broadcom.com

[PATCH net v2 2/9] bnxt_en: Fix bnxt_reinit_features() when irq_re_init is true

[Michael Chan]

bnxt_set_features() takes a snapshot of bp->flags, closes the device,
and then blindly restores the entire bp->flags.  bp->flags contains
statistics flags that may be cleared and the memory freed when
irq_re_init is true.  Do not restore these statistics flags until
bnxt_open_nic() reallocates the memory.  bnxt_open_nic() can
potentially fail to allocate the stats memory and the restored flags
can potentially cause NULL dereference of the stats memory.

Fixes: 93e90104bd12 ("bnxt_en: Create and setup the additional VNIC for adding ntuple filters")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 51557ee6c9ad..c34360f1e004 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14091,7 +14091,8 @@ static int bnxt_reinit_features(struct bnxt *bp, bool irq_re_init,
 				bool link_re_init, u32 flags, bool update_tpa)
 {
 	bnxt_close_nic(bp, irq_re_init, link_re_init);
-	bp->flags = flags;
+	bp->flags = (bp->flags & ~BNXT_FLAG_ALL_CONFIG_FEATS) |
+		    (flags & BNXT_FLAG_ALL_CONFIG_FEATS);
 	if (update_tpa)
 		bnxt_set_ring_params(bp);
 	return bnxt_open_nic(bp, irq_re_init, link_re_init);
-- 
2.51.0

[PATCH net v2 3/9] bnxt_en: Refactor RSS table check logic

[Michael Chan]

We have logic to see if we need to reset the user configured RSS
table for the default RSS context if the RX rings have changed
in __bnxt_reserve_rings().  Refactor this code into a helper that
will be used by other callers later in the patch series.  There is
no change in behavior.

Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c34360f1e004..f83f4c8e994f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -8169,6 +8169,20 @@ static bool bnxt_rings_ok(struct bnxt *bp, struct bnxt_hw_rings *hwr)
 	       hwr->stat && (hwr->cp_p5 || !(bp->flags & BNXT_FLAG_CHIP_P5_PLUS));
 }
 
+/* Check to see if we need to reset the user configured RSS table
+ * (default RSS context) because of change in RX rings from
+ * old_rx to new_rx.
+ */
+static void bnxt_check_rss_tbl_lost(struct bnxt *bp, int old_rx, int new_rx)
+{
+	if (netif_is_rxfh_configured(bp->dev) &&
+	    (bnxt_get_nr_rss_ctxs(bp, old_rx) !=
+	     bnxt_get_nr_rss_ctxs(bp, new_rx) ||
+	     bnxt_get_max_rss_ring(bp) >= new_rx)) {
+		ethtool_rxfh_indir_lost(bp->dev);
+	}
+}
+
 static int bnxt_get_avail_msix(struct bnxt *bp, int num);
 
 static int __bnxt_reserve_rings(struct bnxt *bp)
@@ -8258,12 +8272,7 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
 	if (rx_rings != bp->rx_nr_rings) {
 		netdev_warn(bp->dev, "Able to reserve only %d out of %d requested RX rings\n",
 			    rx_rings, bp->rx_nr_rings);
-		if (netif_is_rxfh_configured(bp->dev) &&
-		    (bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings) !=
-		     bnxt_get_nr_rss_ctxs(bp, rx_rings) ||
-		     bnxt_get_max_rss_ring(bp) >= rx_rings)) {
-			ethtool_rxfh_indir_lost(bp->dev);
-		}
+		bnxt_check_rss_tbl_lost(bp, bp->rx_nr_rings, rx_rings);
 	}
 	bp->rx_nr_rings = rx_rings;
 	bp->cp_nr_rings = hwr.cp;
-- 
2.51.0

[PATCH net v2 4/9] bnxt_en: Refactor IRQs required logic

[Michael Chan]

Refactor the logic to determine the required IRQ vectors into a new
helper.  Later patches in the series will make additional use of the
helper.  There is no change in behavior.

Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 25 ++++++++++++++---------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index f83f4c8e994f..aa40d5fd05da 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11712,9 +11712,22 @@ static void bnxt_clear_int_mode(struct bnxt *bp)
 	bp->irq_tbl_size = 0;
 }
 
-int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
+static int bnxt_irqs_required(struct bnxt *bp)
 {
 	struct bnxt_en_dev *edev = bp->edev[BNXT_AUXDEV_RDMA];
+
+	if (BNXT_NEW_RM(bp) && !bnxt_ulp_registered(edev)) {
+		int ulp_msix = bnxt_get_avail_msix(bp, bp->ulp_num_msix_want);
+
+		if (ulp_msix > bp->ulp_num_msix_want)
+			ulp_msix = bp->ulp_num_msix_want;
+		return ulp_msix + bp->cp_nr_rings;
+	}
+	return bnxt_get_num_msix(bp);
+}
+
+int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
+{
 	bool irq_cleared = false;
 	bool irq_change = false;
 	int tcs = bp->num_tc;
@@ -11724,15 +11737,7 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 	if (!bnxt_need_reserve_rings(bp))
 		return 0;
 
-	if (BNXT_NEW_RM(bp) && !bnxt_ulp_registered(edev)) {
-		int ulp_msix = bnxt_get_avail_msix(bp, bp->ulp_num_msix_want);
-
-		if (ulp_msix > bp->ulp_num_msix_want)
-			ulp_msix = bp->ulp_num_msix_want;
-		irqs_required = ulp_msix + bp->cp_nr_rings;
-	} else {
-		irqs_required = bnxt_get_num_msix(bp);
-	}
+	irqs_required = bnxt_irqs_required(bp);
 
 	if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) {
 		irq_change = true;
-- 
2.51.0

[PATCH net v2 5/9] bnxt_en: Reinit IRQ when configuring LRO/GRO/HDS

[Michael Chan]

When configuring LRO/GRO/HDS, a new set of RX Aggregation rings may be
required.  It is possible that the FW cannot grant the desired number
of Agg rings, causing the driver to reduce the number of ethtool
channels to adjust.  This means that the number of IRQs/NAPIs may
change and we must set irq_re_init to true to make that work.  Without
this patch, when the driver is eventually shutdown, some memory for the
unused NAPIs may never be freed properly if the rings have shrunk.

Fixes: 87c8f8496a05 ("bnxt_en: add support for tcp-data-split ethtool command")
Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 11 ++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 10 ++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index aa40d5fd05da..bf902da945cb 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14116,10 +14116,11 @@ static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 {
 	bool update_tpa = false, update_ntuple = false;
 	struct bnxt *bp = netdev_priv(dev);
+	bool irq_re_init = false;
 	u32 flags = bp->flags;
+	bool re_init = false;
 	u32 changes;
 	int rc = 0;
-	bool re_init = false;
 
 	bp->tx_wake_thresh = max_t(int, bp->tx_ring_size / 2,
 				   bnxt_min_tx_desc_cnt(bp, features));
@@ -14146,8 +14147,12 @@ static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 		update_tpa = true;
 		if ((bp->flags & BNXT_FLAG_TPA) == 0 ||
 		    (flags & BNXT_FLAG_TPA) == 0 ||
-		    (bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
+		    (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)) {
 			re_init = true;
+			if (!(bp->flags & BNXT_FLAG_AGG_RINGS) &&
+			    (flags & BNXT_FLAG_AGG_RINGS))
+				irq_re_init = true;
+		}
 	}
 
 	if (changes & ~BNXT_FLAG_TPA)
@@ -14170,7 +14175,7 @@ static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 			return bnxt_reinit_features(bp, true, false, flags, update_tpa);
 
 		if (re_init)
-			return bnxt_reinit_features(bp, false, false, flags, update_tpa);
+			return bnxt_reinit_features(bp, irq_re_init, false, flags, update_tpa);
 
 		if (update_tpa) {
 			bp->flags = flags;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 622e89587e5d..5c9e770960e6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -853,6 +853,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
 {
 	u8 tcp_data_split = kernel_ering->tcp_data_split;
 	struct bnxt *bp = netdev_priv(dev);
+	bool irq_re_init = false;
 	u8 hds_config_mod;
 	int rc;
 
@@ -876,8 +877,13 @@ static int bnxt_set_ringparam(struct net_device *dev,
 		return -EINVAL;
 	}
 
+	if (hds_config_mod &&
+	    tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    !(bp->flags & BNXT_FLAG_AGG_RINGS))
+		irq_re_init = true;
+
 	if (netif_running(dev))
-		bnxt_close_nic(bp, false, false);
+		bnxt_close_nic(bp, irq_re_init, false);
 
 	if (hds_config_mod) {
 		if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED)
@@ -891,7 +897,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
 	bnxt_set_ring_params(bp);
 
 	if (netif_running(dev)) {
-		rc = bnxt_open_nic(bp, false, false);
+		rc = bnxt_open_nic(bp, irq_re_init, false);
 		if (rc)
 			return rc;
 	}
-- 
2.51.0

[PATCH net v2 6/9] bnxt_en: Fix ring accounting and validation when rings are constrained

[Michael Chan]

When __bnxt_reserve_rings() reserves fewer TX rings than requested,
and an XDP program is attached, the driver used to blindly subtract
bp->tx_nr_rings_xdp from bp->tx_nr_rings, potentially causing an integer
underflow.

This patch mainly fixes the existing bnxt_adj_tx_rings() and
bnxt_rings_ok():

1. bnxt_adj_tx_rings() is now renamed bnxt_adj_rings() to reflect that
all rings (tx, rx, cp) may adjust if needed.  It will now correctly
scale down TX rings for XDP and TCs evenly.  Because XDP requires
a 1:1 TX/RX ring mapping in combined channel mode, it will now clamp
the RX rings to match.  CP rings will also be reduced if necessary.
Any leftover rings after integer division are intentionally left unused.

2. bnxt_rings_ok() is now updated to robustly check for the absolute
minimum viable ring configuration.  We now call bnxt_rings_ok() to
make sure we have the bare minimum before calling the new
bnxt_adj_rings().  This now guarantees that bnxt_adj_rings() will
never underflow or truncate any rings to 0.

The special NITRO_A0 minimum requirement is handled by a new helper
in bnxt_init_int_mode() only.  This old chip does not require ring
reservations (bnxt_need_reserve_rings() returns false on this chip).

With these 2 main changes, we can now centralize bnxt_adj_rings() in
__bnxt_reserve_rings() and bnxt_init_int_mode() and delete other
adjustments and checks in other functions.  Note that in
__bnxt_reserve_rings(), we only need to call bnxt_adj_rings() if
irq_re_init is true which means that bp->bnapi has not been allocated.
In this context, we may be asking for more rings than FW can grant
and we need to call bnxt_adj_rings() for possible adjustments.  We
need to be careful when aborting __bnxt_reserve_rings() and
bnxt_init_int_mode() to restore the possibly truncated rings if
necessary so that the driver state is consistent.

We now remove the error path at the end of bnxt_reserve_rings() to abort
and reset the TCs if the rings cannot satisfy the TC requirements.  The
user will have to reduce the TCs and retry.

There are other related changes to reset the RSS table if needed and
to recalculate the IRQs required if the rings have shrunk.  Note that
the latter can only be done if the NAPIs have not been allocated yet.

As noted in the cover letter, we have similar existing issues regarding
RSS tables for non-default RSS contexts and user defined n-tuple
filters when RX rings shrink due to FW or MSI-X constraints.  The fixes
for these are deferred to a separate patchset.

These existing issues were detected 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")
Fixes: 674f50a5b026 ("bnxt_en: Implement new method to reserve rings.")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2:
Reduce RX and CP rings if needed when TX rings are reduced to keep it
consistent.
Handle NITRO_A0 special RX requirements to keep it consistent.
Fix all unwind issues in these code paths.

v1:
https://lore.kernel.org/netdev/20260831024342.2161156-2-michael.chan@broadcom.com/
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 177 +++++++++++++++-------
 1 file changed, 126 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index bf902da945cb..8e4bde720ef5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -8163,10 +8163,30 @@ static void bnxt_copy_reserved_rings(struct bnxt *bp, struct bnxt_hw_rings *hwr)
 	}
 }
 
+static bool bnxt_nitro_a0_rings_ok(struct bnxt *bp, int rx)
+{
+	if (BNXT_CHIP_TYPE_NITRO_A0(bp) && rx < 2)
+		return false;
+	return true;
+}
+
 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;
+	int min_rx = 1;
+	int min_cp = 1;
+	int tx_cp;
+
+	if (bp->tx_nr_rings_xdp)
+		min_tx++;
+
+	if (!(bp->flags & BNXT_FLAG_SHARED_RINGS)) {
+		tx_cp = bnxt_num_tx_to_cp(bp, min_tx);
+		min_cp = tx_cp + min_rx;
+	}
+	return hwr->tx >= min_tx && hwr->rx >= min_rx && hwr->cp >= min_cp &&
+	       hwr->grp && hwr->vnic && hwr->stat &&
+	       (hwr->cp_p5 || !(bp->flags & BNXT_FLAG_CHIP_P5_PLUS));
 }
 
 /* Check to see if we need to reset the user configured RSS table
@@ -8184,10 +8204,12 @@ static void bnxt_check_rss_tbl_lost(struct bnxt *bp, int old_rx, int new_rx)
 }
 
 static int bnxt_get_avail_msix(struct bnxt *bp, int num);
+static void bnxt_adj_rings(struct bnxt *bp);
 
 static int __bnxt_reserve_rings(struct bnxt *bp)
 {
 	struct bnxt_en_dev *edev = bp->edev[BNXT_AUXDEV_RDMA];
+	int req_rx_rings = bp->rx_nr_rings;
 	struct bnxt_hw_rings hwr = {0};
 	int rx_rings, old_rx_rings, rc;
 	int cp = bp->cp_nr_rings;
@@ -8255,49 +8277,68 @@ 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)
+		goto reserve_rings_exit;
+
+	if (bp->bnapi && (rx_rings < bp->rx_nr_rings ||
+			  hwr.tx < bp->tx_nr_rings)) {
+		netdev_err(bp->dev, "Unexpected ring shrinkage %d/%d RX/TX to %d/%d\n",
+			   bp->rx_nr_rings, bp->tx_nr_rings, rx_rings, hwr.tx);
+		rc = -ENOSPC;
+		goto reserve_rings_exit;
+	}
 	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)) {
+		rc = -ENOMEM;
+		goto reserve_rings_exit;
+	}
+
 	if (hwr.tx != bp->tx_nr_rings) {
 		netdev_warn(bp->dev,
 			    "Able to reserve only %d out of %d requested TX rings\n",
 			    hwr.tx, bp->tx_nr_rings);
 	}
 	bp->tx_nr_rings = hwr.tx;
+	bp->rx_nr_rings = rx_rings;
+	bp->cp_nr_rings = hwr.cp;
+
+	if (!bp->bnapi)
+		bnxt_adj_rings(bp);
 
 	/* If we cannot reserve all the RX rings, reset the RSS map only
 	 * if absolutely necessary
 	 */
-	if (rx_rings != bp->rx_nr_rings) {
-		netdev_warn(bp->dev, "Able to reserve only %d out of %d requested RX rings\n",
-			    rx_rings, bp->rx_nr_rings);
-		bnxt_check_rss_tbl_lost(bp, bp->rx_nr_rings, rx_rings);
+	if (req_rx_rings != bp->rx_nr_rings) {
+		netdev_warn(bp->dev, "RX rings reduced from %d to %d\n",
+			    req_rx_rings, bp->rx_nr_rings);
+		bnxt_check_rss_tbl_lost(bp, req_rx_rings, bp->rx_nr_rings);
 	}
-	bp->rx_nr_rings = rx_rings;
-	bp->cp_nr_rings = hwr.cp;
 
 	/* Fall back if we cannot reserve enough HW RSS contexts */
 	if ((bp->rss_cap & BNXT_RSS_CAP_LARGE_RSS_CTX) &&
 	    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 &&
+	if ((old_rx_rings != bp->hw_resc.resv_rx_rings ||
+	     req_rx_rings != bp->rx_nr_rings) &&
 	    !netif_is_rxfh_configured(bp->dev))
 		bnxt_set_dflt_rss_indir_tbl(bp, NULL);
 
+reserve_rings_exit:
 	if (!bnxt_ulp_registered(edev) && BNXT_NEW_RM(bp)) {
 		int resv_msix, resv_ctx, ulp_ctxs;
 		struct bnxt_hw_resc *hw_resc;
 
 		hw_resc = &bp->hw_resc;
-		resv_msix = hw_resc->resv_irqs - bp->cp_nr_rings;
+		resv_msix = max_t(int, hw_resc->resv_irqs - bp->cp_nr_rings, 0);
 		ulp_msix = min_t(int, resv_msix, ulp_msix);
 		bnxt_set_ulp_msix_num(bp, ulp_msix);
-		resv_ctx = hw_resc->resv_stat_ctxs  - bp->cp_nr_rings;
+		resv_ctx = max_t(int, hw_resc->resv_stat_ctxs - bp->cp_nr_rings,
+				 0);
 		ulp_ctxs = min(resv_ctx, bnxt_get_ulp_stat_ctxs(bp));
 		bnxt_set_ulp_stat_ctxs(bp, ulp_ctxs);
 	}
@@ -11645,7 +11686,9 @@ static int bnxt_get_num_msix(struct bnxt *bp)
 
 static int bnxt_init_int_mode(struct bnxt *bp)
 {
-	int i, total_vecs, max, rc, min = 1, ulp_msix, tx_cp, tbl_size;
+	int i, total_vecs, max, rc, min = 1, ulp_msix, tbl_size, req_rx_rings;
+	int min_tx, req_tx_rings, req_cp_rings, req_tx_per_tc, req_tx_xdp;
+	int tcs = bp->num_tc ? bp->num_tc : 1;
 
 	total_vecs = bnxt_get_num_msix(bp);
 	max = bnxt_get_max_func_irqs(bp);
@@ -11680,19 +11723,56 @@ static int bnxt_init_int_mode(struct bnxt *bp)
 		bp->irq_tbl[i].vector = pci_irq_vector(bp->pdev, i);
 
 	bp->total_irqs = total_vecs;
+	req_tx_rings = bp->tx_nr_rings;
+	req_tx_per_tc = bp->tx_nr_rings_per_tc;
+	req_tx_xdp = bp->tx_nr_rings_xdp;
+	req_rx_rings = bp->rx_nr_rings;
+	req_cp_rings = bp->cp_nr_rings;
+
 	/* Trim rings based upon num of vectors allocated */
 	rc = bnxt_trim_rings(bp, &bp->rx_nr_rings, &bp->tx_nr_rings,
 			     total_vecs - ulp_msix, min == 1);
 	if (rc)
 		goto msix_setup_exit;
 
-	tx_cp = bnxt_num_tx_to_cp(bp, bp->tx_nr_rings);
-	bp->cp_nr_rings = (min == 1) ?
-			  max_t(int, tx_cp, bp->rx_nr_rings) :
-			  tx_cp + bp->rx_nr_rings;
+	min_tx = bp->tx_nr_rings_xdp ? tcs + 1 : tcs;
+	if (bp->tx_nr_rings < min_tx) {
+		netdev_err(bp->dev, "Not enough MSI-X to satisfy min. TX rings\n");
+		rc = -ENOMEM;
+		goto msix_setup_exit_restore;
+	}
+
+	bnxt_adj_rings(bp);
 
+	if (!bnxt_nitro_a0_rings_ok(bp, bp->rx_nr_rings)) {
+		netdev_err(bp->dev, "Not enough MSI-X to satisfy min. RX rings\n");
+		rc = -ENOMEM;
+		goto msix_setup_exit_restore;
+	}
+
+	if (bp->bnapi && (req_tx_rings != bp->tx_nr_rings ||
+			  req_rx_rings != bp->rx_nr_rings)) {
+		netdev_err(bp->dev, "Cannot shrink rings once NAPI is allocated\n");
+		rc = -ENOSPC;
+		goto msix_setup_exit_restore;
+	}
+
+	if (req_rx_rings != bp->rx_nr_rings) {
+		netdev_warn(bp->dev, "RX rings reduced from %d to %d\n",
+			    req_rx_rings, bp->rx_nr_rings);
+		bnxt_check_rss_tbl_lost(bp, req_rx_rings, bp->rx_nr_rings);
+		if (!netif_is_rxfh_configured(bp->dev))
+			bnxt_set_dflt_rss_indir_tbl(bp, NULL);
+	}
 	return 0;
 
+msix_setup_exit_restore:
+	bp->tx_nr_rings = req_tx_rings;
+	bp->tx_nr_rings_per_tc = req_tx_per_tc;
+	bp->tx_nr_rings_xdp = req_tx_xdp;
+	bp->rx_nr_rings = req_rx_rings;
+	bp->cp_nr_rings = req_cp_rings;
+
 msix_setup_exit:
 	netdev_err(bp->dev, "bnxt_init_int_mode err: %x\n", rc);
 	kfree(bp->irq_tbl);
@@ -11729,8 +11809,6 @@ static int bnxt_irqs_required(struct bnxt *bp)
 int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 {
 	bool irq_cleared = false;
-	bool irq_change = false;
-	int tcs = bp->num_tc;
 	int irqs_required;
 	int rc;
 
@@ -11740,7 +11818,6 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 	irqs_required = bnxt_irqs_required(bp);
 
 	if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) {
-		irq_change = true;
 		if (!pci_msix_can_alloc_dyn(bp->pdev) || !bp->irq_tbl) {
 			bnxt_ulp_irq_stop(bp);
 			bnxt_clear_int_mode(bp);
@@ -11752,25 +11829,17 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 		if (!rc)
 			rc = bnxt_init_int_mode(bp);
 		bnxt_ulp_irq_restart(bp, rc);
-	} else if (irq_change && !rc) {
-		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
+	} else if (irq_re_init && BNXT_NEW_RM(bp) && !rc) {
+		/* __bnxt_reserve_rings() may have shrunk the rings */
+		irqs_required = bnxt_irqs_required(bp);
+		if (irqs_required != bp->total_irqs &&
+		    bnxt_change_msix(bp, irqs_required) != irqs_required)
 			rc = -ENOSPC;
 	}
 	if (rc) {
 		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;
 }
 
@@ -13275,13 +13344,29 @@ static void bnxt_set_xdp_tx_rings(struct bnxt *bp)
 	bp->tx_nr_rings += bp->tx_nr_rings_xdp;
 }
 
-static void bnxt_adj_tx_rings(struct bnxt *bp)
+static void bnxt_adj_rings(struct bnxt *bp)
 {
-	/* 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);
+	int tcs = bp->num_tc ? bp->num_tc : 1;
+	int rx = bp->rx_nr_rings;
+
+	/* Make adjustments to rings based on TC/XDP/shared rings policies */
+	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);
+	}
+
+	if (bp->flags & BNXT_FLAG_SHARED_RINGS) {
+		bp->tx_nr_rings_per_tc = min(bp->tx_nr_rings_per_tc, rx);
+		bp->rx_nr_rings = bp->tx_nr_rings_per_tc;
+		if (bp->tx_nr_rings_xdp)
+			bp->tx_nr_rings_xdp = bp->tx_nr_rings_per_tc;
+	}
+
+	bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tcs;
+	bnxt_set_cp_rings(bp, bp->flags & BNXT_FLAG_SHARED_RINGS);
 }
 
 static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
@@ -13301,7 +13386,6 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 	if (rc)
 		return rc;
 
-	bnxt_adj_tx_rings(bp);
 	rc = bnxt_alloc_mem(bp, irq_re_init);
 	if (rc) {
 		netdev_err(bp->dev, "bnxt_alloc_mem err: %x\n", rc);
@@ -16967,7 +17051,6 @@ static int bnxt_set_dflt_rings(struct bnxt *bp, bool sh)
 	if (rc && rc != -ENODEV)
 		netdev_warn(bp->dev, "Unable to reserve tx rings\n");
 
-	bnxt_adj_tx_rings(bp);
 	if (sh)
 		bnxt_adj_dflt_rings(bp, true);
 
@@ -16976,7 +17059,6 @@ static int bnxt_set_dflt_rings(struct bnxt *bp, bool sh)
 		rc = __bnxt_reserve_rings(bp);
 		if (rc && rc != -ENODEV)
 			netdev_warn(bp->dev, "2nd rings reservation failed.\n");
-		bnxt_adj_tx_rings(bp);
 	}
 	if (BNXT_CHIP_TYPE_NITRO_A0(bp)) {
 		bp->rx_nr_rings++;
@@ -17010,8 +17092,6 @@ static int bnxt_init_dflt_ring_mode(struct bnxt *bp)
 	if (rc)
 		goto init_dflt_ring_err;
 
-	bnxt_adj_tx_rings(bp);
-
 	bnxt_set_dflt_rfs(bp);
 
 init_dflt_ring_err:
@@ -17355,11 +17435,6 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		goto init_err_pci_clean;
 
-	/* No TC has been set yet and rings may have been trimmed due to
-	 * limited MSIX, so we re-initialize the TX rings per TC.
-	 */
-	bp->tx_nr_rings_per_tc = bp->tx_nr_rings;
-
 	if (BNXT_PF(bp)) {
 		if (!bnxt_pf_wq) {
 			bnxt_pf_wq =
-- 
2.51.0

Re: [PATCH net v2 6/9] bnxt_en: Fix ring accounting and validation when rings are constrained

[netdev-bot+sashiko]

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

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

- [High] NITRO_A0 regression: this patch makes bnxt_init_int_mode() call
  bnxt_adj_rings() every time.
- [Medium] bnxt_reserve_rings() gains a new branch, 'else if (irq_re_init
  && BNXT_NEW_RM(bp) && !rc)'.
- [Medium] With TCs and XDP both active, bnxt_trim_rings() converts the
  trimmed completion-ring count back to a TX count with…

--

bnxt_en: Fix ring accounting and validation when rings are constrained

This patch renames bnxt_adj_tx_rings() to bnxt_adj_rings(). The new
function rebalances TX, RX and CP rings for TCs, XDP and shared-ring
mode, and is called from __bnxt_reserve_rings() and bnxt_init_int_mode().
The patch also tightens bnxt_rings_ok() to check minimum ring counts,
and removes the TC reset error path from bnxt_reserve_rings().

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index bf902da945cb..8e4bde720ef5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -11680,19 +11723,56 @@ static int bnxt_init_int_mode(struct bnxt *bp)

[ ... ]

>  	/* Trim rings based upon num of vectors allocated */
>  	rc = bnxt_trim_rings(bp, &bp->rx_nr_rings, &bp->tx_nr_rings,
>  			     total_vecs - ulp_msix, min == 1);
>  	if (rc)
>  		goto msix_setup_exit;
>  
> -	tx_cp = bnxt_num_tx_to_cp(bp, bp->tx_nr_rings);
> -	bp->cp_nr_rings = (min == 1) ?
> -			  max_t(int, tx_cp, bp->rx_nr_rings) :
> -			  tx_cp + bp->rx_nr_rings;
> +	min_tx = bp->tx_nr_rings_xdp ? tcs + 1 : tcs;
> +	if (bp->tx_nr_rings < min_tx) {
> +		netdev_err(bp->dev, "Not enough MSI-X to satisfy min. TX rings\n");
> +		rc = -ENOMEM;
> +		goto msix_setup_exit_restore;
> +	}

[Severity: Medium]
Can this check reject a configuration that bnxt_adj_rings() could still
rebalance?

When both TCs and XDP are active, bnxt_trim_rings() turns the trimmed
completion-ring count back into a TX count with bnxt_num_cp_to_tx():

    return (tx_cp - bp->tx_nr_rings_xdp) * tcs +
           bp->tx_nr_rings_xdp;

This keeps the original XDP ring count fixed.

For example, take num_tc = 2, XDP attached and 8 combined channels. That
gives tx = 24, xdp = 8 and tx_cp = 16. If total_vecs - ulp_msix is 5,
shared-mode trimming sets tx_cp = 5, and the TX count becomes
(5 - 8) * 2 + 8 = 2.

That is below min_tx = 3, so this path returns -ENOMEM with "Not enough
MSI-X to satisfy min. TX rings". But a balanced setup with
per_tc = xdp = rx = 2 (tx = 6, tx_cp = 4) fits in 5 vectors.

The same conversion feeds bnxt_rings_ok() in __bnxt_reserve_rings(), so
FW-constrained reservations would fail the same way.

The commit message says bnxt_adj_rings() "will now correctly scale down
TX rings for XDP and TCs evenly". That seems to hold only for mild
shrinkage. Should the minimum check run after rebalancing, or should the
conversion scale the XDP count too?

> +
> +	bnxt_adj_rings(bp);
>  
> +	if (!bnxt_nitro_a0_rings_ok(bp, bp->rx_nr_rings)) {
> +		netdev_err(bp->dev, "Not enough MSI-X to satisfy min. RX rings\n");
> +		rc = -ENOMEM;
> +		goto msix_setup_exit_restore;
> +	}

[Severity: High]
Does this break NITRO_A0 chips at probe time?

After computing the shared-ring defaults, bnxt_set_dflt_rings() adds an
extra RX ring and an extra CP ring for this chip:

    if (BNXT_CHIP_TYPE_NITRO_A0(bp)) {
        bp->rx_nr_rings++;
        bp->cp_nr_rings++;
    }

NITRO_A0 always runs in shared mode. bnxt_init_int_mode() now always
calls bnxt_adj_rings(), and its shared branch does:

    bp->tx_nr_rings_per_tc = min(bp->tx_nr_rings_per_tc, rx);
    bp->rx_nr_rings = bp->tx_nr_rings_per_tc;

With a default ring count of N, rx drops from N + 1 back to N, and
bnxt_set_cp_rings() recomputes cp_nr_rings as N. By the time
bnxt_nitro_a0_rings_ok() runs, the extra ring is already gone.

With N == 1 (a kdump kernel, or a single-queue system), rx becomes 1,
bnxt_nitro_a0_rings_ok() fails, and probe returns -ENOMEM. The same
setup probed successfully before this patch.

With N >= 2, bnxt_init_napi() still gives the last bnapi to the special
handler:

    if (BNXT_CHIP_TYPE_NITRO_A0(bp)) {
        bnapi = bp->bnapi[cp_nr_rings];
        netif_napi_add_locked(bp->dev, &bnapi->napi, bnxt_poll_nitroa0);

In shared mode, bnxt_alloc_mem() now also puts regular TX ring N - 1 on
that bnapi. bnxt_poll_nitroa0() doesn't process TX completions; it only
logs "Invalid completion received on special ring" for them.

Would that TX queue then stall until the TX watchdog fires?

There are smaller effects as well. The default RSS table loses a ring.
bnxt_get_channels() reports rx_nr_rings - 1 combined channels, one fewer
than configured. Every probe also prints a spurious "RX rings reduced"
warning.

The commit message says "The special NITRO_A0 minimum requirement is
handled by a new helper in bnxt_init_int_mode() only". Should
bnxt_adj_rings() account for the extra RX ring on this chip instead?

[ ... ]

> @@ -11740,7 +11818,6 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  	irqs_required = bnxt_irqs_required(bp);
>  
>  	if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) {
> -		irq_change = true;
>  		if (!pci_msix_can_alloc_dyn(bp->pdev) || !bp->irq_tbl) {
>  			bnxt_ulp_irq_stop(bp);
>  			bnxt_clear_int_mode(bp);
> @@ -11752,25 +11829,17 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  		if (!rc)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
> -	} else if (irq_change && !rc) {
> -		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
> +	} else if (irq_re_init && BNXT_NEW_RM(bp) && !rc) {
> +		/* __bnxt_reserve_rings() may have shrunk the rings */
> +		irqs_required = bnxt_irqs_required(bp);
> +		if (irqs_required != bp->total_irqs &&
> +		    bnxt_change_msix(bp, irqs_required) != irqs_required)
>  			rc = -ENOSPC;
>  	}

[Severity: Medium]
Can this new branch call bnxt_change_msix() on a device that doesn't
support dynamic MSI-X allocation?

Before this patch, bnxt_change_msix() was only reached through
irq_change. That meant the first branch had already seen
pci_msix_can_alloc_dyn() && bp->irq_tbl; otherwise the clear/re-init
path ran instead. The new condition drops that check.

Take a NEW_RM device without dynamic MSI-X support where the initial
irqs_required equals bp->total_irqs, so irq_cleared stays false. Suppose
__bnxt_reserve_rings() then shrinks the rings: FW grants fewer, and
bnxt_adj_rings() runs because bp->bnapi is NULL. The recomputed
irqs_required drops, and bnxt_change_msix() enters its trim loop:

    for (i = bp->total_irqs; i > total; i--) {
        map.index = i - 1;
        map.virq = bp->irq_tbl[i - 1].vector;
        pci_msix_free_irq(bp->pdev, map);
        bp->total_irqs--;
    }

pci_msix_free_irq() starts with:

    if (WARN_ON_ONCE(!pci_msix_can_alloc_dyn(dev)))
        return;

So the vector is never freed, but bp->total_irqs is still decremented.
bnxt_change_msix() then returns a value equal to irqs_required, and no
error is reported.

Wouldn't this trigger the WARN and leave bp->total_irqs lower than the
number of vectors the PCI core has actually allocated? Should this
branch also check pci_msix_can_alloc_dyn(), or fall back to the
clear/re-init path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928041712.3467803-1-michael.chan%40broadcom.com

[PATCH net v2 7/9] bnxt_en: Add bnxt_clear_bars() helper

[Michael Chan]

In bnxt_io_slot_reset(), we clear the 6 BAR registers.  Add a helper
function to do that.  The helper will be used again in the next patch.

Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Joe Damato <joe@dama.to>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 8e4bde720ef5..8c6bf10aba8a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17221,6 +17221,14 @@ void bnxt_print_device_info(struct bnxt *bp)
 	pcie_print_link_status(bp->pdev);
 }
 
+static void bnxt_clear_bars(struct pci_dev *pdev)
+{
+	int off;
+
+	for (off = PCI_BASE_ADDRESS_0; off <= PCI_BASE_ADDRESS_5; off += 4)
+		pci_write_config_dword(pdev, off, 0);
+}
+
 static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct bnxt_hw_resc *hw_resc;
@@ -17703,7 +17711,6 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
 	struct bnxt *bp = netdev_priv(netdev);
 	int retry = 0;
 	int err = 0;
-	int off;
 
 	netdev_info(bp->dev, "PCI Slot Reset\n");
 
@@ -17732,11 +17739,8 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
 		 * write the BARs to 0 to force restore, in case of fatal error.
 		 */
 		if (test_and_clear_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN,
-				       &bp->state)) {
-			for (off = PCI_BASE_ADDRESS_0;
-			     off <= PCI_BASE_ADDRESS_5; off += 4)
-				pci_write_config_dword(bp->pdev, off, 0);
-		}
+				       &bp->state))
+			bnxt_clear_bars(pdev);
 		pci_restore_state(pdev);
 
 		bnxt_inv_fw_health_reg(bp);
-- 
2.51.0

[PATCH net v2 8/9] bnxt_en: Fix driver init in kdump kernel

[Michael Chan]

Fix and strengthen the FLR sequence when initializing in the kdump
kernel.  If the NIC is behind a PCIe switch in synthetic (smart)
mode, the switch may need to see that the BARs have been initialized
before it will pass mem read/write TLPs to the NIC.

Add a new bnxt_kdump_reset() to do the expanded FLR sequence in the
kdump kernel.  We now disable bus master and memory, save the PCI
state, do the FLR, clear the BARs, and restore the PCI state.  The
BARs have to be cleared to ensure that they get re-initialized and
visible to the PCIe switch.

Since it is the kdump kernel, we make every effort to continue in
the best possible way even if the device is unresponsive after FLR
or we encounter other errors.

To avoid dealing with a possible 0xffff value for the MSI-X capabilty
register after FLR, we call bnxt_get_max_irq() to get the valid
number of MSIX before we do the FLR.

Fixes: 8743db4a9acf ("bnxt_en: Issue PCIe FLR in kdump kernel to cleanup pending DMAs.")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2:
Disable device before pci_save_state() and pcie_flr() and check for
errors. 

v1:
https://lore.kernel.org/netdev/20260831024342.2161156-4-michael.chan@broadcom.com/
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 48 +++++++++++++++++++----
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 8c6bf10aba8a..e4530b091d3b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17229,6 +17229,42 @@ static void bnxt_clear_bars(struct pci_dev *pdev)
 		pci_write_config_dword(pdev, off, 0);
 }
 
+/* Clear any pending DMA transactions from crash kernel while loading driver in
+ * capture kernel.
+ */
+static void bnxt_kdump_reset(struct pci_dev *pdev)
+{
+	u16 cmd;
+	int rc;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+
+	if (pci_save_state(pdev))
+		dev_warn(&pdev->dev, "Failed to save PCI state, PCI restore may be incomplete\n");
+
+	/* Since it is kdump kernel, try to continue in the best possible way
+	 * even if the device is unresponsive after FLR.  The device may
+	 * eventually respond to HWRM_VER_GET later in the init sequence.
+	 */
+	rc = pcie_flr(pdev);
+	if (rc)
+		dev_warn(&pdev->dev, "pcie_flr() failed (rc: %d), trying to continue\n",
+			 rc);
+
+	/* A complete or partial reset has been done.  Clear the BARs
+	 * if the device is responsive.
+	 */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (PCI_POSSIBLE_ERROR(cmd))
+		dev_warn(&pdev->dev, "PCI config space inaccessible after FLR, not clearing BARs\n");
+	else
+		bnxt_clear_bars(pdev);
+
+	pci_restore_state(pdev);
+}
+
 static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct bnxt_hw_resc *hw_resc;
@@ -17244,15 +17280,11 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return -ENODEV;
 	}
 
-	/* Clear any pending DMA transactions from crash kernel
-	 * while loading driver in capture kernel.
-	 */
-	if (is_kdump_kernel()) {
-		pci_clear_master(pdev);
-		pcie_flr(pdev);
-	}
-
 	max_irqs = bnxt_get_max_irq(pdev);
+
+	if (is_kdump_kernel())
+		bnxt_kdump_reset(pdev);
+
 	dev = alloc_etherdev_mqs(sizeof(*bp), max_irqs * BNXT_MAX_QUEUE,
 				 max_irqs);
 	if (!dev)
-- 
2.51.0

Re: [PATCH net v2 8/9] bnxt_en: Fix driver init in kdump kernel

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] In bnxt_kdump_reset(), the path the patch adds for an
  'unresponsive after FLR' device cannot re-program the BARs.
- [Low] Two new comments in bnxt_kdump_reset()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c) promise more than…

--

bnxt_en: Fix driver init in kdump kernel

This commit adds bnxt_kdump_reset() to do a longer FLR sequence in the
kdump kernel. The sequence disables bus master and memory, saves PCI
state, does the FLR, clears the BARs and restores PCI state. It also moves
bnxt_get_max_irq() ahead of the FLR so the MSI-X capability is not read
as 0xffff afterwards.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 8c6bf10aba8a..e4530b091d3b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -17229,6 +17229,42 @@ static void bnxt_clear_bars(struct pci_dev *pdev)
>  		pci_write_config_dword(pdev, off, 0);
>  }
>  
> +/* Clear any pending DMA transactions from crash kernel while loading driver in
> + * capture kernel.
> + */
> +static void bnxt_kdump_reset(struct pci_dev *pdev)
> +{

[ ... ]

> +	rc = pcie_flr(pdev);
> +	if (rc)
> +		dev_warn(&pdev->dev, "pcie_flr() failed (rc: %d), trying to continue\n",
> +			 rc);
> +
> +	/* A complete or partial reset has been done.  Clear the BARs
> +	 * if the device is responsive.
> +	 */

[Severity: Low]
Is this comment accurate for every non-zero return from pcie_flr()?

pcie_flr() has two error returns that happen before it sets
PCI_EXP_DEVCTL_BCR_FLR:

drivers/pci/pci.c:pcie_flr() {
	if (!pci_dev_config_accessible(dev, "FLR"))
		return -ENOTTY;
	...
	ret = pci_dev_reset_iommu_prepare(dev);
	if (ret) {
		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", ret);
		return ret;
	}

	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
	...
}

Take an ATS-capable device where __iommu_group_alloc_blocking_domain() or
__iommu_attach_device() fails. The IOMMU prepare step fails but the device
still responds. The PCI_COMMAND read below succeeds, so
bnxt_clear_bars() and pci_restore_state() run and probe continues. No
reset was ever issued, and the "pcie_flr() failed" warning is the only
sign of it.

The old code ignored the return value of pcie_flr() completely, so
continuing without a reset is not new behaviour. Still, this comment and
the function header ("Clear any pending DMA transactions from crash
kernel") both suggest the pending DMA has been handled.

Could the comments or the warning tell "reset not attempted" apart from
"reset attempted but device slow to respond"? Only the pci_dev_wait()
timeout fits the second case.

> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	if (PCI_POSSIBLE_ERROR(cmd))
> +		dev_warn(&pdev->dev, "PCI config space inaccessible after FLR, not clearing BARs\n");
> +	else
> +		bnxt_clear_bars(pdev);
> +
> +	pci_restore_state(pdev);

[Severity: Medium]
If config space cannot be accessed here, does calling pci_restore_state()
on the same device have any effect?

Suppose pcie_flr() fails because pci_dev_wait() gives up after
PCIE_RESET_READY_POLL_MS:

drivers/pci/pci.c:pci_dev_wait() {
	...
		if (delay > timeout) {
			pci_err(dev, "not ready %dms after %s; giving up\n",
				delay - 1, reset_type);
			return -ENOTTY;
		}
	...
}

PCI_COMMAND then reads back as 0xffff, so bnxt_clear_bars() is skipped
because the device is unresponsive.

pci_restore_state() still runs, though. pci_restore_config_dword() writes
each saved BAR, COMMAND and DEVCTL value once, with no retry or read-back:

drivers/pci/pci.c:pci_restore_config_dword() {
	pci_read_config_dword(pdev, offset, &val);
	if (!force && val == saved_val)
		return;
	...
	pci_write_config_dword(pdev, offset, saved_val);
}

A function that is still in reset would drop those writes.

Nothing later in probe writes the BARs again. bnxt_init_board() only calls
pci_enable_device(), pci_set_master() and pci_ioremap_bar() with the
stored resource addresses.

The new comment above pcie_flr() says "The device may eventually respond
to HWRM_VER_GET later in the init sequence". If the device does recover
later, it has not seen its BARs programmed since the reset. Neither has a
synthetic-mode switch in front of it. Wouldn't HWRM_VER_GET over BAR0
still fail in that case?

DEVCTL MPS/MRRS would also stay at their post-reset defaults, while the
PCI core believes they were restored.

Would it make sense to fail the probe in this case? Another option is to
poll until config space is reachable and then do the clear and restore
again. The later patch in this series ("Re-write the BARs following any
type of PCIe errors") only changes bnxt_io_slot_reset(), so it does not
cover this path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928041712.3467803-1-michael.chan%40broadcom.com

[PATCH net v2 9/9] bnxt_en: Re-write the BARs following any type of PCIe errors

[Michael Chan]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Currently the driver zeroes the BARs only when fatal PCIe errors
are reported so that pci_restore_state() restores it.  However
firmware handles both fatal and non-fatal errors the same way when
it sees the slot reset resulting from the PCI_ERS_RESULT_NEED_RESET
return code from the driver.  This means that we must re-write the
BARs post recovery even during non-fatal errors.  Otherwise we will
see that every MMIO access returns all-ones and the firmware appears
dead.

Zero-out the BARs during PCIe error recovery regardless of type of
PCIe error, and make the wait after the hot reset unconditional.

Disable memory decode and bus mastering before rewriting the BARs so
the device doesn't decode a half-updated address, bailing out if
config space is still inaccessible.  Defer pci_enable_device() until
after the BAR rewrite and restore, so the device isn't re-enabled
while its BARs are still being rewritten, then re-enable the device
and re-assert bus mastering.  Guard the same Command register cleanup
on the re-enable failure path against an inaccessible device.

Skip re-enabling the device in bnxt_io_slot_reset() if it is already
enabled, so enable_cnt does not go unbalanced.

Fixes: f75d9a0aa967 ("bnxt_en: Re-write PCI BARs after PCI fatal error.")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2:
Disable device before rewriting the BARs.
Improve error checking.

v1: https://lore.kernel.org/netdev/20260831024342.2161156-5-michael.chan@broadcom.com/
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 105 ++++++++++++----------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |   1 -
 2 files changed, 59 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index e4530b091d3b..810219d9cae2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17710,10 +17710,8 @@ static pci_ers_result_t bnxt_io_error_detected(struct pci_dev *pdev,
 	 * so we disable bus master to prevent any potential bad DMAs before
 	 * freeing kernel memory.
 	 */
-	if (state == pci_channel_io_frozen) {
-		set_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN, &bp->state);
+	if (state == pci_channel_io_frozen)
 		bnxt_fw_fatal_close(bp);
-	}
 
 	if (netif_running(netdev))
 		__bnxt_close_nic(bp, true, true);
@@ -17743,65 +17741,80 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
 	struct bnxt *bp = netdev_priv(netdev);
 	int retry = 0;
 	int err = 0;
+	u16 cmd;
 
 	netdev_info(bp->dev, "PCI Slot Reset\n");
 
-	if (test_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN, &bp->state)) {
-		/* After DPC, the chip should return CRS when the vendor ID
-		 * config register is read until it is ready.  On all chips,
-		 * this is not happening reliably so add a 5-second delay as a
-		 * workaround.
-		 */
-		msleep(5000);
-	}
+	/* After a PCIe hot reset, the chip should return CRS when the
+	 * vendor ID config register is read until it is ready.  On all
+	 * chips, this is not happening reliably so add a 5-second delay
+	 * as a workaround.
+	 */
+	msleep(5000);
 
 	netdev_lock(netdev);
 
-	if (pci_enable_device(pdev)) {
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (PCI_POSSIBLE_ERROR(cmd)) {
 		dev_err(&pdev->dev,
-			"Cannot re-enable PCI device after reset.\n");
-	} else {
-		pci_set_master(pdev);
-		/* Upon fatal error, our device internal logic that latches to
-		 * BAR value is getting reset and will restore only upon
-		 * rewriting the BARs.
-		 *
-		 * As pci_restore_state() does not re-write the BARs if the
-		 * value is same as saved value earlier, driver needs to
-		 * write the BARs to 0 to force restore, in case of fatal error.
-		 */
-		if (test_and_clear_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN,
-				       &bp->state))
-			bnxt_clear_bars(pdev);
-		pci_restore_state(pdev);
+			"PCI config space inaccessible after reset\n");
+		goto reset_exit;
+	}
 
-		bnxt_inv_fw_health_reg(bp);
-		bnxt_try_map_fw_health_reg(bp);
+	/* Upon PCIe error, our device internal logic that latches to
+	 * BAR value is getting reset and will restore only upon
+	 * rewriting the BARs.
+	 *
+	 * As pci_restore_state() does not re-write the BARs if the
+	 * value is same as saved value earlier, driver needs to
+	 * write the BARs to 0 to force restore.
+	 */
+	pci_clear_master(pdev);
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	cmd &= ~PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
 
-		/* In some PCIe AER scenarios, firmware may take up to
-		 * 10 seconds to become ready in the worst case.
-		 */
-		do {
-			err = bnxt_try_recover_fw(bp);
-			if (!err)
-				break;
-			retry++;
-		} while (retry < BNXT_FW_SLOT_RESET_RETRY);
+	bnxt_clear_bars(pdev);
+	pci_restore_state(pdev);
 
-		if (err) {
-			dev_err(&pdev->dev, "Firmware not ready\n");
-			goto reset_exit;
+	if (!pci_is_enabled(pdev) && pci_enable_device(pdev)) {
+		dev_err(&pdev->dev,
+			"Cannot re-enable PCI device after reset.\n");
+		pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+		if (!PCI_POSSIBLE_ERROR(cmd)) {
+			cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
+			pci_write_config_word(pdev, PCI_COMMAND, cmd);
 		}
+		goto reset_exit;
+	}
+	pci_set_master(pdev);
 
-		err = bnxt_hwrm_func_reset(bp);
+	bnxt_inv_fw_health_reg(bp);
+	bnxt_try_map_fw_health_reg(bp);
+
+	/* In some PCIe AER scenarios, firmware may take up to
+	 * 10 seconds to become ready in the worst case.
+	 */
+	do {
+		err = bnxt_try_recover_fw(bp);
 		if (!err)
-			result = PCI_ERS_RESULT_RECOVERED;
+			break;
+		retry++;
+	} while (retry < BNXT_FW_SLOT_RESET_RETRY);
 
-		/* IRQ will be initialized later in bnxt_io_resume */
-		bnxt_ulp_irq_stop(bp);
-		bnxt_clear_int_mode(bp);
+	if (err) {
+		dev_err(&pdev->dev, "Firmware not ready\n");
+		goto reset_exit;
 	}
 
+	err = bnxt_hwrm_func_reset(bp);
+	if (!err)
+		result = PCI_ERS_RESULT_RECOVERED;
+
+	/* IRQ will be initialized later in bnxt_io_resume */
+	bnxt_ulp_irq_stop(bp);
+	bnxt_clear_int_mode(bp);
+
 reset_exit:
 	clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
 	bnxt_clear_reservations(bp, true);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index a757d8258f71..75428a585577 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2468,7 +2468,6 @@ struct bnxt {
 #define BNXT_STATE_ABORT_ERR	5
 #define BNXT_STATE_FW_FATAL_COND	6
 #define BNXT_STATE_DRV_REGISTERED	7
-#define BNXT_STATE_PCI_CHANNEL_IO_FROZEN	8
 #define BNXT_STATE_NAPI_DISABLED	9
 #define BNXT_STATE_FW_ACTIVATE		11
 #define BNXT_STATE_RECOVER		12
-- 
2.51.0

Re: [PATCH net v2 9/9] bnxt_en: Re-write the BARs following any type of PCIe errors

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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] In bnxt_io_slot_reset()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c), the new sequence of…
- [Low] In bnxt_io_slot_reset()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c), only the first…

--

bnxt_en: Re-write the BARs following any type of PCIe errors

This patch makes bnxt_io_slot_reset() zero and restore the BARs after
any PCIe error, not only fatal ones, and makes the 5 second post-reset
wait unconditional. It turns off bus mastering and memory decode before
the BAR rewrite. It also moves pci_enable_device() to after
pci_restore_state(), and skips it if the device is already enabled.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index e4530b091d3b..810219d9cae2 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -17743,65 +17741,80 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)

[ ... ]

> +	/* Upon PCIe error, our device internal logic that latches to
> +	 * BAR value is getting reset and will restore only upon
> +	 * rewriting the BARs.
> +	 *
> +	 * As pci_restore_state() does not re-write the BARs if the
> +	 * value is same as saved value earlier, driver needs to
> +	 * write the BARs to 0 to force restore.
> +	 */
> +	pci_clear_master(pdev);
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	cmd &= ~PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);

[Severity: Low]
Should this second PCI_COMMAND read also be checked with
PCI_POSSIBLE_ERROR()?

If this read returns 0xFFFF, cmd becomes 0xFFFD. The write then sets
PCI_COMMAND_MASTER, PCI_COMMAND_IO, SERR and parity right before
bnxt_clear_bars().

pci_clear_master() goes through __pci_set_master(), which does not check
its own read either. So nothing confirms that mastering and decode are
off before the BARs are zeroed.

pci_restore_state() restores the saved Command register afterwards, so
the bad value would only be transient. However, the same read-modify-write
on the re-enable failure path below is guarded with
!PCI_POSSIBLE_ERROR(cmd).

[ ... ]

> +	bnxt_clear_bars(pdev);
> +	pci_restore_state(pdev);
>  
> -		if (err) {
> -			dev_err(&pdev->dev, "Firmware not ready\n");
> -			goto reset_exit;
> +	if (!pci_is_enabled(pdev) && pci_enable_device(pdev)) {

[Severity: High]
Can enable_cnt still go unbalanced when bnxt_io_error_detected() took
its abort path?

Every path in bnxt_io_error_detected() that returns NEED_RESET disables
the device. So the only way to reach bnxt_io_slot_reset() with the
device still enabled seems to be the abort path, where a firmware reset
is already in progress:

bnxt_io_error_detected() {
    ...
	if (test_and_set_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
		netdev_err(bp->dev, "Firmware reset already in progress\n");
		abort = true;
	}

	if (abort || state == pci_channel_io_perm_failure) {
		netdev_unlock(netdev);
		return PCI_ERS_RESULT_DISCONNECT;
	}
    ...
}

A sibling under the same bridge might vote NEED_RESET. That could be the
other PF, a VF, or any device below the same root port or switch. In
that case merge_result() in drivers/pci/pcie/err.c promotes the
DISCONNECT vote to NEED_RESET:

	case PCI_ERS_RESULT_DISCONNECT:
		if (new == PCI_ERS_RESULT_NEED_RESET)
			orig = PCI_ERS_RESULT_NEED_RESET;

report_slot_reset() then calls ->slot_reset() on this function too. At
that point bnxt_fw_reset_task() is still active, and nothing tells
slot_reset that it does not own recovery.

In the common case bnxt_fw_reset()->bnxt_fw_reset_close() has already
called pci_disable_device(), so enable_cnt is 0. The following sequence
then looks possible:

bnxt_io_slot_reset()
    pci_enable_device()              enable_cnt = 1
    bnxt_try_recover_fw() retry loop ...

bnxt_fw_reset_task(), ENABLE_DEV state, no netdev_lock
    pci_enable_device()              enable_cnt = 2
    pci_set_master()

bnxt_io_slot_reset()
    reset_exit:
    clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state)

pci_enable_device_flags() increments enable_cnt every time:

	if (atomic_inc_return(&dev->enable_cnt) > 1)
		return 0;		/* already enabled */

The commit message says:

  "Skip re-enabling the device in bnxt_io_slot_reset() if it is already
  enabled, so enable_cnt does not go unbalanced."

Does the pci_is_enabled() check cover this case?

Before this patch, the BARs were never touched on this path, because
BNXT_STATE_PCI_CHANNEL_IO_FROZEN was only set after the abort check. Now
the decode-off, bnxt_clear_bars() and pci_restore_state() sequence also
runs here. bnxt_fw_reset_task() may be doing bar0 MMIO at the same time
in POLL_FW_DOWN, RESET_FW or POLL_FW (bnxt_fw_health_readl(),
bnxt_reset_all(), bnxt_hwrm_poll()).

Could those accesses get all-ones or UR responses, and confuse the
health status checks or raise a new AER event?

The worker's pci_enable_device() might also land between clearing
PCI_COMMAND_MEMORY and pci_restore_state(). If so, do_pci_enable_device()
would turn memory decode back on while the BARs are zero, which is the
case the commit message says this avoids:

  "Disable memory decode and bus mastering before rewriting the BARs so
  the device doesn't decode a half-updated address"

The worker's unlocked pci_set_master() read-modify-write of PCI_COMMAND
could also overwrite the update made here.

Some of this is not new with this patch. On this path slot_reset already
sent HWRM_FUNC_RESET, called bnxt_clear_int_mode(), and cleared
BNXT_STATE_IN_FW_RESET even though it never acquired that bit.

Would it be better to record the abort in bnxt_io_error_detected()?
bnxt_io_slot_reset() could then return PCI_ERS_RESULT_DISCONNECT in that
case, or serialize with or cancel bnxt_fw_reset_task(), instead of
relying on the pci_is_enabled() check.

> +		dev_err(&pdev->dev,
> +			"Cannot re-enable PCI device after reset.\n");
> +		pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +		if (!PCI_POSSIBLE_ERROR(cmd)) {
> +			cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
> +			pci_write_config_word(pdev, PCI_COMMAND, cmd);
>  		}
> +		goto reset_exit;
> +	}
> +	pci_set_master(pdev);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928041712.3467803-1-michael.chan%40broadcom.com

Re: [PATCH net v2 9/9] bnxt_en: Re-write the BARs following any type of PCIe errors

[Pavan Chebbi]

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

> [Severity: High]
> Can enable_cnt still go unbalanced when bnxt_io_error_detected() took
> its abort path?

<-->

> Does the pci_is_enabled() check cover this case?
>

Agree with this. I will fix the possible imbalance in enable_cnt.

> Before this patch, the BARs were never touched on this path, because
> BNXT_STATE_PCI_CHANNEL_IO_FROZEN was only set after the abort check. Now
> the decode-off, bnxt_clear_bars() and pci_restore_state() sequence also
> runs here. bnxt_fw_reset_task() may be doing bar0 MMIO at the same time
> in POLL_FW_DOWN, RESET_FW or POLL_FW (bnxt_fw_health_readl(),
> bnxt_reset_all(), bnxt_hwrm_poll()).
>
> Could those accesses get all-ones or UR responses, and confuse the
> health status checks or raise a new AER event?
>
> The worker's pci_enable_device() might also land between clearing
> PCI_COMMAND_MEMORY and pci_restore_state(). If so, do_pci_enable_device()
> would turn memory decode back on while the BARs are zero, which is the
> case the commit message says this avoids:
>
>   "Disable memory decode and bus mastering before rewriting the BARs so
>   the device doesn't decode a half-updated address"
>
> The worker's unlocked pci_set_master() read-modify-write of PCI_COMMAND
> could also overwrite the update made here.
>
> Some of this is not new with this patch. On this path slot_reset already
> sent HWRM_FUNC_RESET, called bnxt_clear_int_mode(), and cleared
> BNXT_STATE_IN_FW_RESET even though it never acquired that bit.
>
> Would it be better to record the abort in bnxt_io_error_detected()?
> bnxt_io_slot_reset() could then return PCI_ERS_RESULT_DISCONNECT in that
> case, or serialize with or cancel bnxt_fw_reset_task(), instead of
> relying on the pci_is_enabled() check.

The FW reset handling and the PCIe errors handling paths are not
synchronized 100% even before this patch.
So I would treat these issues as pre-existing and fixing them requires
additional, lengthier changes..
I would like this to be treated as a separate issue and will try to
address thru separate series

>
> > +             dev_err(&pdev->dev,
> > +                     "Cannot re-enable PCI device after reset.\n");
> > +             pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > +             if (!PCI_POSSIBLE_ERROR(cmd)) {
> > +                     cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
> > +                     pci_write_config_word(pdev, PCI_COMMAND, cmd);
> >               }
> > +             goto reset_exit;
> > +     }
> > +     pci_set_master(pdev);
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928041712.3467803-1-michael.chan%40broadcom.com

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

Re: [PATCH net v2 0/9] bnxt_en: Bug fixes

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2 0/9] bnxt_en: Bug fixes

[Jakub Kicinski]

On Mon, 28 Sep 2026 04:25:08 +0000 netdev-bot+sinfo@kernel.org wrote:
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Michael, please respond to this and the AI review feedback.
We need to sort fixes more aggressively now, basically we need
to how if any of the issues fixed here are LLM-induced and
expected to be irrelevant.

Re: [PATCH net v2 0/9] bnxt_en: Bug fixes

[Michael Chan]

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

Sorry for the late reply.  This ended up in my spam folder.

On Sun, Sep 27, 2026 at 9:25 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

The ring accounting bugs fixed by the 1st 6 patches were originally
reported by Sashiko while posting the kTLS patches to net-next:

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

kTLS needs to use the new MPC rings, so the ring accounting logic is
touched, exposing the issues to Sashiko.  These are pre-existing bugs
that affect normal RX/TX rings when FW is running short on rings.  The
issues are real, but not likely unless many VFs cause ring shortages.
Patches 7, 8, 9 are real issues found in the lab or by partners.

>
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.
>

The changes were regression tested on the 5760X and 5750X chips (the 2
latest chip generations) using production FW (237.x.x.x).

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

Re: [PATCH net v2 0/9] bnxt_en: Bug fixes

[Jakub Kicinski]

On Thu, 1 Oct 2026 12:22:38 -0700 Michael Chan wrote:
> > This is an automated message. This series looks like a fix, but its
> > commit messages seem to be missing some information:
> >
> >  - How the issue was discovered, e.g. hit in production, hit during
> >    development, syzbot report, manual code inspection, LLM or static
> >    analysis tool scan.
> >
> >  - Whether the issue was actually triggered, or is only theoretical
> >    (e.g. found by code inspection). If it was triggered please include
> >    the symptoms, like the stack trace or error messages.  
> 
> The ring accounting bugs fixed by the 1st 6 patches were originally
> reported by Sashiko while posting the kTLS patches to net-next:
> 
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810051358.1244418-7-michael.chan@broadcom.com
> 
> kTLS needs to use the new MPC rings, so the ring accounting logic is
> touched, exposing the issues to Sashiko.  These are pre-existing bugs
> that affect normal RX/TX rings when FW is running short on rings.  The
> issues are real, but not likely unless many VFs cause ring shortages.
> Patches 7, 8, 9 are real issues found in the lab or by partners.
> 
> >
> >  - What hardware the change was tested on. For driver fixes please
> >    mention the device (and if relevant firmware version) used for
> >    testing, or say that the change was not tested on real hardware.
> >  
> 
> The changes were regression tested on the 5760X and 5750X chips (the 2
> latest chip generations) using production FW (237.x.x.x).

Hm, someone set this to changes requested.

Can we split this series in two? Let's take the last 3 via net and the
AI-discovered, un-proven (meaning - I'm assuming you haven't actually
triggered the issue) 6 via net-next?

Hopefully Pavan can adjust the net patches according to the AI review
while at it, at least the suggestion to explicitly remember state on
patch 9 looks trivial, on a quick read?

Re: [PATCH net v2 0/9] bnxt_en: Bug fixes

[Pavan Chebbi]

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

> Hopefully Pavan can adjust the net patches according to the AI review
> while at it, at least the suggestion to explicitly remember state on
> patch 9 looks trivial, on a quick read?

Yes it does but only until I tried the options and each had side effects like:
Recording the abort and returning DISCONNECT from slot_reset() fails
the whole reset domain.
Returning NONE avoids that, but the early return also skipped the
restore, resume()/netif_device_attach(), ULP restart and IN_FW_RESET
cleanup.
Serializing with bnxt_fw_reset_task() via netdev_lock can't cover the
window before slot_reset() takes the lock, and risks stalling the
shared bnxt_pf_wq.
Stopping the worker (ABORT_ERR + cancel_delayed_work_sync()) is the
only option that may close the race, but since it changes how the
firmware reset state machine is stopped (lock ordering, and the close
path running twice), it needs proper hardware testing.

So all these just started to grow the fix further and well beyond its
scope..  so I replied to Sashiko that I better exclude it since the
issues exist already

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

Re: [PATCH net v2 0/9] bnxt_en: Bug fixes

[Jakub Kicinski]

On Thu, 1 Oct 2026 12:22:38 -0700 Michael Chan wrote:
> kTLS needs to use the new MPC rings, so the ring accounting logic is
> touched, exposing the issues to Sashiko.  These are pre-existing bugs
> that affect normal RX/TX rings when FW is running short on rings.  The
> issues are real, but not likely unless many VFs cause ring shortages.
> Patches 7, 8, 9 are real issues found in the lab or by partners.
> 
> >
> >  - What hardware the change was tested on. For driver fixes please
> >    mention the device (and if relevant firmware version) used for
> >    testing, or say that the change was not tested on real hardware.
> >  
> 
> The changes were regression tested on the 5760X and 5750X chips (the 2
> latest chip generations) using production FW (237.x.x.x).

Ah, and to be clear, when you repost, you can just put this sort of
info in the cover letter. Doesn't have to be repeated in each patch