[PATCH net 0/4] bnxt_en: Bug fixes

[PATCH net 0/4] bnxt_en: Bug fixes

[Michael Chan]

This patchset includes 4 fixes.  The first one fixes the ring
accounting logic when FW is unable to reserve all the rings
requested by the driver.  The 2nd one is a refactoring patch to
add a bnxt_clear_bars() helper needed by the 3rd and 4th patch.
The 3rd patch fixes a possible error during driver init. in the
kdump kernel by rewriting the BARs after FLR.  The 4th patch is a
similar fix in the PCIe AER code path for non-fatal errors.

Michael Chan (3):
  bnxt_en: Fix ring accounting underflow 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 | 70 +++++++++++++----------
 1 file changed, 39 insertions(+), 31 deletions(-)

-- 
2.51.0

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

[Michael Chan]

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

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

[netdev-bot+sashiko]

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

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

- [High] bnxt_adj_tx_rings()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c:13214) now scales…
- [Medium] The patch deletes the only unwind of the netdev/driver TC
  configuration from bnxt_reserve_rings() (the 'tx ring reservation…

Pre-existing issues:
- [Medium] bnxt_adj_tx_rings() lowers bp->tx_nr_rings (leftover reserved
  rings are, per the new commit message, 'intentionally left unused')…

--

bnxt_en: Fix ring accounting underflow when rings are constrained

The patch reworks bnxt_adj_tx_rings() to scale the XDP and per-TC TX ring
counts down evenly when fewer TX rings were reserved than requested, adds
a minimum TX ring requirement to bnxt_rings_ok(), moves that check earlier
in __bnxt_reserve_rings(), and drops the TC reservation failure path at the
end of bnxt_reserve_rings().

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

> 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));
>  }

[ ... ]

> @@ -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;
>  }

[Severity: Medium]
This removed block was the only place that reset the TC configuration when
the reservation could not back the configured traffic classes.  Can dropping
it leave bp->num_tc permanently set after a failed reservation?

bnxt_setup_mq_tc() commits the configuration before the reservation happens:

	netdev_set_num_tc(dev, tc);
	bp->num_tc = tc;
	...
	if (netif_running(bp->dev))
		return bnxt_open_nic(bp, true, false);

and has no unwind of its own.  Its pre-flight bnxt_check_rings() only checks
against maximum resources, not against what the firmware will actually
reserve.  If the reservation later yields hwr.tx < bp->num_tc (+1 for XDP),
the new bnxt_rings_ok() check in __bnxt_reserve_rings() now returns -ENOMEM
before any software state is touched, so bp->num_tc stays at tc.

Since bnxt_setup_mq_tc() is the only place in the driver that ever assigns
bp->num_tc, does bnxt_rings_ok() then keep computing min_tx = bp->num_tc,
making every subsequent ifup fail with -ENOMEM until the mqprio
configuration is torn down?

There also seems to be a divergence between driver and core state on the
mqprio path.  When mqprio_init() propagates the error, qdisc_create() calls
mqprio_destroy(), which with priv->hw_offload == 0 does:

net/sched/sch_mqprio.c:mqprio_destroy() {
	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);
	else
		netdev_set_num_tc(dev, 0);
}

That leaves dev->num_tc == 0 while bp->num_tc == tc and bp->tx_nr_rings is
still sized for tc traffic classes.  On a later successful open, the
netdev_set_tc_queue() calls in bnxt_setup_msix() then have no traffic
classes to program while the driver still lays out rings per TC.  Was that
resync intentional to drop?

> @@ -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;

[Severity: High]
bp->tx_nr_rings_xdp is lowered here, but bp->rx_nr_rings is left untouched.
Can that break the 1:1 XDP TX ring to RX ring mapping the commit message
relies on?

bnxt_xdp_set() establishes the invariant:

drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c:bnxt_xdp_set() {
	...
	bp->tx_nr_rings_xdp = tx_xdp;		/* tx_xdp = bp->rx_nr_rings */
}

and bnxt_alloc_mem() only installs XDP TX rings for the first
bp->tx_nr_rings_xdp bnapi entries:

			if (i >= bp->tx_nr_rings_xdp) {
				int k = j + BNXT_RING_TO_TC_OFF(bp, i);

				bnapi2 = bp->bnapi[k];
				...
				bnapi2->tx_int = bnxt_tx_int;
			} else {
				bnapi2 = bp->bnapi[j];
				bnapi2->flags |= BNXT_NAPI_FLAG_XDP;
				bnapi2->tx_ring[0] = txr;
				bnapi2->tx_int = bnxt_tx_int_xdp;

Meanwhile every RX ring still gets the program in bnxt_init_one_rx_ring():

	if (BNXT_RX_PAGE_MODE(bp) && bp->xdp_prog) {
		bpf_prog_add(bp->xdp_prog, 1);
		rxr->xdp_prog = bp->xdp_prog;
	}

and bnxt_rx_xdp() uses the paired TX ring unconditionally:

drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c:bnxt_rx_xdp() {
	txr = rxr->bnapi->tx_ring[0];
	...
}

Walking a P5+ combined-ring case with num_tc = 0, rx_nr_rings = 8 and XDP
attached (requested tx_nr_rings_xdp = 8, tx_nr_rings = 16):

Firmware reserves hwr.tx = 10.  The new min_tx in bnxt_rings_ok() is
1 + 1 = 2, so the reservation is accepted and bp->tx_nr_rings = 10 with
bp->rx_nr_rings = 8.  Here tcs = 2, so tx_nr_rings_per_tc = 5 and
tx_nr_rings_xdp = 5.  In bnxt_alloc_mem() the regular rings i in [5,10) map
to k = 5..9, so bnapi[5..7] own both an RX ring and a stack TX ring in
tx_ring[0].  RX rings 5-7 then run XDP on a stack-owned TX ring with no txq
lock, and __bnxt_tx_int() sees tx_buf->skb == NULL and calls
bnxt_sched_reset_txr().

Firmware reserves hwr.tx = 6.  Now tx_nr_rings_per_tc = tx_nr_rings_xdp = 3
and bp->tx_nr_rings = 6, while rx_nr_rings is still 8.  The regular rings
map to bnapi[3..5], so bnapi[6] and bnapi[7] get no TX ring at all and
rxr->bnapi->tx_ring[0] is NULL for RX rings 6 and 7.  Does the first packet
received on those rings then oops in bnxt_rx_xdp() via bnxt_tx_avail(),
which reads txr->tx_prod?

Before this patch this second reservation produced a negative
bp->tx_nr_rings whose allocation in bnxt_alloc_mem() failed and aborted the
open, so the device did not reach this state.

Nothing between bnxt_adj_tx_rings() and the data path re-establishes the
invariant: __bnxt_open_nic() calls bnxt_adj_tx_rings() and then
bnxt_alloc_mem() directly, and the only place that resyncs rx_nr_rings to
tx_nr_rings_per_tc is bnxt_trim_dflt_sh_rings()/bnxt_adj_dflt_rings() on the
probe/default-rings path:

	bp->cp_nr_rings = min_t(int, bp->tx_nr_rings_per_tc, bp->rx_nr_rings);
	bp->rx_nr_rings = bp->cp_nr_rings;

Would it work to have bnxt_rings_ok() require num_tc rings plus
bp->rx_nr_rings XDP rings, or to trim bp->rx_nr_rings down to
bp->tx_nr_rings_per_tc here?

> +	} 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;

[Severity: Medium]
This isn't a bug introduced by this patch, but bp->tx_nr_rings is reduced
here while bp->cp_nr_rings keeps the value __bnxt_reserve_rings() derived
from the un-adjusted hwr.tx:

	tx_cp = bnxt_num_tx_to_cp(bp, hwr.tx);
	hwr.cp = sh ? max_t(int, tx_cp, rx_rings) : tx_cp + rx_rings;
	...
	bp->cp_nr_rings = hwr.cp;

Nothing recomputes it in the open path, since __bnxt_open_nic() calls
bnxt_adj_tx_rings() and then bnxt_alloc_mem(); only the probe path re-runs
bnxt_adj_dflt_rings()/bnxt_set_cp_rings().  Should bnxt_adj_tx_rings() also
resync bp->cp_nr_rings now that leftover rings are "intentionally left
unused"?

With P5+, combined rings, num_tc = 0, rx = 8, requested xdp = 8 and reserved
hwr.tx = 11: tx_cp = (11 - 8) + 8 = 11 so bp->cp_nr_rings = 11, while
bnxt_adj_tx_rings() gives tx_nr_rings_per_tc = tx_nr_rings_xdp = 5 and
bp->tx_nr_rings = 10.  For i = 10 in bnxt_alloc_all_cp_arrays() neither the
rx branch nor the tx branch applies, so cp_count is 0 and:

		cpr->cp_ring_arr = kzalloc_objs(*cpr, cp_count);
		if (!cpr->cp_ring_arr)
			return -ENOMEM;
		cpr->cp_ring_count = cp_count;

returns ZERO_SIZE_PTR, which passes the NULL check.  Besides the wasted
MSIX/NQ/stat context, bnxt_poll_p5() reads that array unguarded at its
poll_done label:

poll_done:
	cpr_rx = &cpr->cp_ring_arr[0];
	if (cpr_rx->cp_ring_type == BNXT_NQ_HDL_TYPE_RX &&

so anything that polls every NAPI of the device, such as netpoll's
poll_napi(), would dereference ZERO_SIZE_PTR.  The pre-patch code shrank
bp->tx_nr_rings even more aggressively and produced the same cp_count == 0
contexts.

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

[PATCH net 2/4] 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 2
patches.

Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
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 219a6f551f1d..a76674fd0d6b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17063,6 +17063,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;
@@ -17550,7 +17558,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");
 
@@ -17579,11 +17586,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

Re: [PATCH net 2/4] bnxt_en: Add bnxt_clear_bars() helper

[Joe Damato]

On Sun, Aug 30, 2026 at 07:43:40PM -0700, Michael Chan wrote:
> 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 2
> patches.
> 
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> 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(-)

Reviewed-by: Joe Damato <joe@dama.to>

[PATCH net 3/4] bnxt_en: Fix driver init in kdump kernel

[Michael Chan]

The driver forces an FLR during kdump kernel initialization to reset
the device.  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.  Save the PCI
state before FLR and restore the state after FLR.  The BARs have to
be cleared to ensure that they get re-initialized.

Fixes: 8743db4a9acf ("bnxt_en: Issue PCIe FLR in kdump kernel to cleanup pending DMAs.")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index a76674fd0d6b..c3d561ac53dc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17091,7 +17091,10 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	if (is_kdump_kernel()) {
 		pci_clear_master(pdev);
+		pci_save_state(pdev);
 		pcie_flr(pdev);
+		bnxt_clear_bars(pdev);
+		pci_restore_state(pdev);
 	}
 
 	max_irqs = bnxt_get_max_irq(pdev);
-- 
2.51.0

Re: [PATCH net 3/4] bnxt_en: Fix driver init in kdump kernel

[Joe Damato]

On Sun, Aug 30, 2026 at 07:43:41PM -0700, Michael Chan wrote:
> The driver forces an FLR during kdump kernel initialization to reset
> the device.  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.  Save the PCI
> state before FLR and restore the state after FLR.  The BARs have to
> be cleared to ensure that they get re-initialized.
> 
> Fixes: 8743db4a9acf ("bnxt_en: Issue PCIe FLR in kdump kernel to cleanup pending DMAs.")
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index a76674fd0d6b..c3d561ac53dc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -17091,7 +17091,10 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 */
>  	if (is_kdump_kernel()) {
>  		pci_clear_master(pdev);
> +		pci_save_state(pdev);
>  		pcie_flr(pdev);

idk if you gotta check the return value of pcie_flr. seems like most drivers
don't except ice :(

> +		bnxt_clear_bars(pdev);
> +		pci_restore_state(pdev);
>  	}
>  
>  	max_irqs = bnxt_get_max_irq(pdev);

the above aside:

Reviewed-by: Joe Damato <joe@dama.to>

Re: [net,3/4] bnxt_en: Fix driver init in kdump kernel

[netdev-bot+sashiko]

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

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

- [Medium] Unchecked pcie_flr() return combined with unconditional BAR
  reprogramming in bnxt_init_one()…
- [Low] The newly added pci_save_state(pdev) call in bnxt_init_one()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c:17094) discards a…
- [Low] The new save/clear-BARs/restore sequence in bnxt_init_one() is
  left undocumented in the tree, and the only in-tree rationale for…

--

bnxt_en: Fix driver init in kdump kernel

In the kdump path, bnxt_init_one() now saves the PCI state before the
forced FLR, zeroes the BAR registers with bnxt_clear_bars() and restores
the state afterwards, so that a PCIe switch in synthetic mode observes
the BARs being (re-)initialized.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index a76674fd0d6b..c3d561ac53dc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -17091,7 +17091,10 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 */
>  	if (is_kdump_kernel()) {
>  		pci_clear_master(pdev);
> +		pci_save_state(pdev);

[Severity: Low]
Should the return value of pci_save_state() be checked here?

pci_save_state() saves the 64-byte header and sets dev->state_saved = true
before it attempts the capability saves:

drivers/pci/pci.c:pci_save_state() {
	...
	dev->state_saved = true;

	i = pci_save_pcie_state(dev);
	if (i != 0)
		return i;

	i = pci_save_pcix_state(dev);
	if (i != 0)
		return i;
	...
}

and pci_save_pcie_state() can fail when no save buffer was preallocated at
device add time:

drivers/pci/pci.c:pci_save_pcie_state() {
	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
	if (!save_state) {
		pci_err(dev, "buffer not found in %s\n", __func__);
		return -ENOMEM;
	}
	...
}

In that case the header/BAR portion this fix depends on is still saved, but
the PCIe capability state that pcie_flr() resets (DEVCTL MPS/MRRS, LNKCTL)
is silently left at reset defaults with no driver-side diagnostic.  Note
the pre-existing pci_save_state() call in the bnxt error recovery path
ignores the return value in the same way, so this is not a new pattern in
this driver.

>  		pcie_flr(pdev);
> +		bnxt_clear_bars(pdev);
> +		pci_restore_state(pdev);

[Severity: Medium]
Is it safe to run bnxt_clear_bars() and pci_restore_state() unconditionally
when pcie_flr() may not have reset the function?

pcie_flr() has two paths that return without ever writing
PCI_EXP_DEVCTL_BCR_FLR, plus a timeout path:

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;
	}
	...
	ret = pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
done:
	pci_dev_reset_iommu_done(dev);
	return ret;
}

If the FLR never took effect, PCI_COMMAND still carries the crashed
kernel's Memory Space Enable bit, since pci_clear_master() only clears Bus
Master Enable.  bnxt_clear_bars() then zeroes all six BAR dwords on a
function that is still decoding:

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);
}

and pci_restore_state() reprograms them one dword at a time, high dword
first:

drivers/pci/pci.c:pci_restore_config_space_range() {
	for (index = end; index >= start; index--)
		pci_restore_config_dword(pdev, 4 * index,
					 pdev->saved_config_space[index],
					 force);
}

bnxt uses 64-bit memory BARs (pci_ioremap_bar(pdev, 0), pci_iomap(pdev, 2,
...), pci_ioremap_bar(pdev, 4)), so that ordering leaves a transient
(high << 32) | 0 decode window.  This is the hazard the PCI core avoids
explicitly:

drivers/pci/setup-res.c:pci_std_update_resource() {
	/*
	 * We can't update a 64-bit BAR atomically, so when possible,
	 * disable decoding so that a half-updated BAR won't conflict
	 * with another device.
	 */
	disable = (res->flags & IORESOURCE_MEM_64) && !dev->mmio_always_on;
	...
}

Would it make sense to clear PCI_COMMAND_MEMORY around the BAR writes here
as well?

The other case is a pci_dev_wait() timeout: per PCIe r4.0 sec 6.6.2 the
device may silently discard config writes while the reset is still in
progress, so the clear and restore can both be dropped.  The commit
message's guarantee that "The BARs have to be cleared to ensure that they
get re-initialized" would then not hold, and probe continues into
bnxt_get_max_irq() and the BAR0 ioremap in bnxt_init_board() with no
indication of why.

Would bailing out (or at least logging) on a non-zero pcie_flr() return be
preferable to proceeding?

>  	}
>  
>  	max_irqs = bnxt_get_max_irq(pdev);

[Severity: Low]
This isn't a bug, but the comment above this block still reads only:

	/* Clear any pending DMA transactions from crash kernel
	 * while loading driver in capture kernel.
	 */

which describes pci_clear_master() plus pcie_flr() and not the new
save / clear-BARs / restore sequence.  Could the switch-in-synthetic-mode
rationale from the changelog be added to the comment?

The reason matters because the only in-tree explanation of
bnxt_clear_bars() is in bnxt_io_slot_reset():

	/* 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.
	 */

After an FLR the function's own BAR registers read back as 0, so they no
longer match the saved non-zero values and pci_restore_config_dword()
would rewrite them regardless.  Applying that comment's reasoning to the
new call site would make bnxt_clear_bars() look like dead code that can be
removed, which would reintroduce the problem being fixed.  Is the intent
that the synthetic-mode switch shadows config-space reads, and can that be
spelled out next to the new call?

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

[PATCH net 4/4] 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.

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>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c3d561ac53dc..09fed8784a2a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17564,7 +17564,7 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
 
 	netdev_info(bp->dev, "PCI Slot Reset\n");
 
-	if (test_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN, &bp->state)) {
+	if (test_and_clear_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
@@ -17580,17 +17580,15 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
 			"Cannot re-enable PCI device after reset.\n");
 	} else {
 		pci_set_master(pdev);
-		/* Upon fatal error, our device internal logic that latches to
+		/* 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, in case of fatal error.
+		 * write the BARs to 0 to force restore.
 		 */
-		if (test_and_clear_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN,
-				       &bp->state))
-			bnxt_clear_bars(pdev);
+		bnxt_clear_bars(pdev);
 		pci_restore_state(pdev);
 
 		bnxt_inv_fw_health_reg(bp);
-- 
2.51.0

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

[Joe Damato]

On Sun, Aug 30, 2026 at 07:43:42PM -0700, Michael Chan wrote:
> 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.
> 
> 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>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>

Reviewed-by: Joe Damato <joe@dama.to>

Re: [net,4/4] bnxt_en: Re-write the BARs following any type of PCIe errors

[netdev-bot+sashiko]

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

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

- [Medium] The patch makes the BAR rewrite unconditional but leaves the
  chip-readiness workaround gated on the fatal-only bit.

Pre-existing issues:
- [Low] bnxt_io_slot_reset()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c:17578-17592) zeroes all six…

--

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

The patch makes bnxt_io_slot_reset() zero the BARs unconditionally so that
pci_restore_state() always rewrites them, instead of only doing so when
BNXT_STATE_PCI_CHANNEL_IO_FROZEN was set.  The clearing of that state bit
moves up into the existing readiness-delay test.

A couple of questions about the new ordering below.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index c3d561ac53dc..09fed8784a2a 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -17564,7 +17564,7 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
>  
>  	netdev_info(bp->dev, "PCI Slot Reset\n");
>  
> -	if (test_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN, &bp->state)) {
> +	if (test_and_clear_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

[Severity: Medium]
The BAR rewrite becomes unconditional, but the chip-readiness delay stays
gated on the fatal-only bit.  Is that intentional?

BNXT_STATE_PCI_CHANNEL_IO_FROZEN is only set for pci_channel_io_frozen in
bnxt_io_error_detected(), yet that function returns
PCI_ERS_RESULT_NEED_RESET for non-fatal errors too, and the core then does
a real secondary bus reset before calling back:

drivers/pci/pcie/err.c:pcie_do_recovery() {
	...
	if (status == PCI_ERS_RESULT_NEED_RESET ||
	    state == pci_channel_io_frozen) {
		if (reset_subordinates(bridge) != PCI_ERS_RESULT_RECOVERED) {
	...
}

So on the non-fatal path the driver now issues bnxt_clear_bars() plus
pci_restore_state() right after a bus reset with no delay, even though the
comment just above says the chip does not reliably signal readiness after
such a reset.

If one of those BAR-clear writes does not land while the function is still
coming out of reset, the BAR still equals the saved value and the restore
is skipped:

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

which leaves the internal BAR latch stale, MMIO reading all-ones, and the
bnxt_try_recover_fw() loop failing all BNXT_FW_SLOT_RESET_RETRY attempts
followed by "Firmware not ready" and PCI_ERS_RESULT_DISCONNECT - the same
failure mode the commit message aims to remove.  Should the readiness delay
be made unconditional along with the BAR clear, or should the rewrite be
verified with a read-back and retried?

Related: bnxt_clear_bars() discards the return value of every
pci_write_config_dword() and nothing reads the BARs back afterwards, so a
dropped write is silent:

	for (off = PCI_BASE_ADDRESS_0; off <= PCI_BASE_ADDRESS_5; off += 4)
		pci_write_config_dword(pdev, off, 0);

> @@ -17580,17 +17580,15 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
>  			"Cannot re-enable PCI device after reset.\n");
>  	} else {
>  		pci_set_master(pdev);
> -		/* Upon fatal error, our device internal logic that latches to
> +		/* 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, in case of fatal error.
> +		 * write the BARs to 0 to force restore.
>  		 */
> -		if (test_and_clear_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN,
> -				       &bp->state))
> -			bnxt_clear_bars(pdev);
> +		bnxt_clear_bars(pdev);
>  		pci_restore_state(pdev);

[Severity: Low]
This isn't a bug introduced by this patch, since the same sequence has been
used on the fatal path since f75d9a0aa967, but the patch now makes it run on
every PCIe error recovery, so it may be a good moment to ask about the write
ordering.

bnxt_clear_bars() programs all six BAR dwords to 0 after pci_enable_device()
and pci_set_master() have already re-enabled memory decode and bus
mastering.  bnxt_io_error_detected() calls pci_disable_device(), so
enable_cnt reaches 0 and the pci_enable_device() here really does set
PCI_COMMAND_MEMORY again.  Between bnxt_clear_bars() and pci_restore_state()
the function is therefore an enabled decoder claiming a window based at 0,
and for a 64-bit BAR written one dword at a time transiently at
old_high << 32.

The PCI core takes the opposite order:

drivers/pci/pci.c:pci_restore_config_space() {
	...
	/* Restore BARs before the command register. */
	...
}

and pci_std_update_resource() clears PCI_COMMAND_MEMORY before rewriting a
memory BAR.  The other caller, bnxt_init_one(), matches that convention:

	if (is_kdump_kernel()) {
		pci_clear_master(pdev);
		pci_save_state(pdev);
		pcie_flr(pdev);
		bnxt_clear_bars(pdev);
		pci_restore_state(pdev);
	}

Would it be preferable to zero the BARs with decode disabled, i.e. before
pci_enable_device()/pci_set_master(), or to clear PCI_COMMAND_MEMORY around
bnxt_clear_bars()?

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