[PATCH net] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[PATCH net] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[Vlad Poenaru]

When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
vectors with bnxt_change_msix() once FW says the new rings would fit,
and keeps them.  FW only reserves the extra IRQs later, in
__bnxt_reserve_rings() on the following open.  The MSI-X table entries
beyond the function's reserved IRQs are not backed until then: the
MSI message written when the new vectors are activated is lost, and
the entries read back as zero.  After FW reserves them they are live
but still zero, so the NQ rings bound to them signal address 0 and
every completion on those rings is lost.  The next ring change (for
example ethtool -G) then times out freeing each of those rings and
the TX watchdog fires:

  bnxt_en 0002:01:00.0 eth0: Resp cmpl intr err msg: 0x51
  bnxt_en 0002:01:00.0 eth0: hwrm_ring_free type 1 failed. rc:fffffff0 err:0
  ...
  bnxt_en 0002:01:00.0 eth0: NETDEV WATCHDOG: CPU: 17: transmit queue 18 timed out 5716 ms

On x86 this is hidden: GENERIC_IRQ_RESERVATION_MODE makes the PCI MSI
domains MSI_FLAG_MUST_REACTIVATE, so the message is composed and
written again when the IRQ is requested, after FW has reserved the
vector.  Since commit 0d6a7c417983 ("irqchip/gic-v3-its: Enable
dynamic MSI-X allocation"), arm64 GICv3 systems take the dynamic path
as well, but write the message only once, at allocation.  On an
NVIDIA Grace system with a BCM57508, raising the channel count from
18 to 32 left MSI-X entries 18-31 all zero, and the 14 rings using
them lost their interrupts.

Keep the allocation in bnxt_check_rings() as a check that enough
vectors are available, but free the new vectors again right away when
FW manages IRQ reservations (BNXT_NEW_RM).  bnxt_reserve_rings() then
allocates them on the following open, after __bnxt_reserve_rings() has
reserved them.  Without BNXT_NEW_RM there is no IRQ reservation and
bnxt_reserve_rings() does not grow the vectors, so keep the current
behaviour there.

Fixes: 2d51eb0bd81c ("bnxt_en: Add MSIX check in bnxt_check_rings()")
Reported-by: Andrew Preece <apreece@meta.com>
Closes: https://lore.kernel.org/all/20261022193620.2152038-1-vlad.wing@gmail.com/
Signed-off-by: Vlad Poenaru <vlad.wing@gmail.com>
---
Tested on an NVIDIA Grace system (GICv3 ITS, ACPI) with a BCM57508,
FW 230.0.165.3: the boot-time channel change (18 -> 32) followed by a
ring resize no longer loses completions.  With instrumentation, MSI-X
entries 18-31 read back as all zero after bnxt_check_rings() allocated
them, and read back correctly once bnxt_reserve_rings() allocated them
after the FW reservation.

This conflicts trivially with "bnxt_en: Clear bp->total_irqs when
disabling IRQs" from the "bnxt_en: Ring accounting fixes" series on
net-next, which adds a bp->irq_tbl check to the same condition.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0682060b048ce..2d14d991a3ead 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -15093,6 +15093,7 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 			hwr.cp = min_t(int, hwr.cp, bnxt_get_max_func_irqs(bp));
 		}
 		if (hwr.cp > bp->total_irqs) {
+			int old_total = bp->total_irqs;
 			int total_msix = bnxt_change_msix(bp, hwr.cp);
 
 			if (total_msix < hwr.cp) {
@@ -15100,6 +15101,17 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 					    hwr.cp, total_msix);
 				rc = -ENOSPC;
 			}
+			/* FW has not reserved the new vectors yet, and writes to
+			 * MSI-X table entries beyond the reserved IRQs don't
+			 * stick (seen on BCM57508).  Free them again;
+			 * bnxt_reserve_rings() allocates them after
+			 * __bnxt_reserve_rings().  Without the new resource
+			 * manager there is no IRQ reservation and
+			 * bnxt_reserve_rings() does not grow the vectors, so
+			 * keep them.
+			 */
+			if (BNXT_NEW_RM(bp))
+				bnxt_change_msix(bp, old_total);
 		}
 	}
 	return rc;

Re: [PATCH net] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[netdev-bot+sashiko]

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

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

- [Medium] The fix is incomplete when FW grants fewer MSI-X vectors than
  requested.
- [Medium] New error path leaves the state inconsistent.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the DCB IEEE
  ETS path reaches bnxt_check_rings() and bnxt_change_msix() holding…

--

bnxt_en: Don't keep MSI-X vectors added before FW reserves them

When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
vectors before FW has reserved them. On arm64 GICv3 the MSI message is
written only once, at allocation, so the message for those entries is
lost. This patch keeps the allocation in bnxt_check_rings() as an
availability check. When BNXT_NEW_RM is set it frees the new vectors
again right away, so that bnxt_reserve_rings() allocates them after
__bnxt_reserve_rings().

Closes: https://lore.kernel.org/all/20261022193620.2152038-1-vlad.wing@gmail.com/

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 0682060b048ce..2d14d991a3ead 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -15093,6 +15093,7 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
>  			hwr.cp = min_t(int, hwr.cp, bnxt_get_max_func_irqs(bp));
>  		}
>  		if (hwr.cp > bp->total_irqs) {
> +			int old_total = bp->total_irqs;
>  			int total_msix = bnxt_change_msix(bp, hwr.cp);
>  
>  			if (total_msix < hwr.cp) {
> @@ -15100,6 +15101,17 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
>  					    hwr.cp, total_msix);
>  				rc = -ENOSPC;
>  			}
> +			/* FW has not reserved the new vectors yet, and writes to
> +			 * MSI-X table entries beyond the reserved IRQs don't
> +			 * stick (seen on BCM57508).  Free them again;
> +			 * bnxt_reserve_rings() allocates them after
> +			 * __bnxt_reserve_rings().  Without the new resource
> +			 * manager there is no IRQ reservation and
> +			 * bnxt_reserve_rings() does not grow the vectors, so
> +			 * keep them.
> +			 */
> +			if (BNXT_NEW_RM(bp))
> +				bnxt_change_msix(bp, old_total);

[Severity: Medium]

The comment says bnxt_reserve_rings() allocates the vectors after
__bnxt_reserve_rings(). Is that allocation capped at the number of
vectors FW actually granted?

In bnxt_reserve_rings(), irqs_required is computed before the
reservation is made:

	if (BNXT_NEW_RM(bp) && !bnxt_ulp_registered(edev)) {
		...
		irqs_required = ulp_msix + bp->cp_nr_rings;
	}
	...
	rc = __bnxt_reserve_rings(bp);
	...
	} else if (irq_change && !rc) {
		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
			rc = -ENOSPC;
	}

If FW grants fewer MSI-X vectors than requested, bnxt_hwrm_get_rings()
records the smaller count:

	hw_resc->resv_irqs = le16_to_cpu(resp->alloc_msix);

__bnxt_reserve_rings() then handles the partial grant by trimming
cp_nr_rings and ulp_msix:

	resv_msix = hw_resc->resv_irqs - bp->cp_nr_rings;
	ulp_msix = min_t(int, resv_msix, ulp_msix);

However, bnxt_reserve_rings() still calls bnxt_change_msix() with the
untrimmed irqs_required. Wouldn't the entries from resv_irqs up to
irqs_required then be allocated and activated before FW backs them?
That is the state this patch is trying to avoid.

bnxt_change_msix() only programs indices at or above bp->total_irqs:

	for (i = bp->total_irqs; i < total; i++) {

so those entries would never be written again. FW could grant them
later, for example on a channel increase where the new count is at or
below total_irqs. Could the NQ rings bound to them then signal address
0, giving the same lost completions and TX watchdog timeouts?

It isn't clear how often FW returns a partial grant after
bnxt_hwrm_check_rings() has succeeded (NPAR, multi-host or VFs may be
cases). The driver does handle partial grants explicitly, though.

Would capping the growth in bnxt_reserve_rings() at hw_resc->resv_irqs
cover this case? Freeing and reallocating any vectors activated beyond
the reservation would be another option.

[Severity: Medium]

Does freeing the vectors here change how an MSI-X allocation failure
is handled?

Before this patch, if pci_msix_alloc_irq_at() failed, bnxt_check_rings()
caught it before any FW or ring state had changed. Now the growth
always happens in bnxt_reserve_rings(). By that point,
__bnxt_reserve_rings() has already committed the FW reservation and
updated bp->hw_resc.

The allocation can fail there, for example under memory pressure or if
another device takes the vectors released here. bnxt_change_msix() then
returns early with bp->total_irqs below irqs_required.
bnxt_reserve_rings() returns -ENOSPC and nothing is rolled back.

On the next open, bnxt_need_reserve_rings() compares only hw_resc
against the ring config:

	if ((bp->flags & BNXT_FLAG_CHIP_P5_PLUS) && BNXT_PF(bp) &&
	    hw_resc->resv_irqs != hwr.cp)
		return true;

It returns false, so bnxt_reserve_rings() returns before growing the
vectors:

	if (!bnxt_need_reserve_rings(bp))
		return 0;

bnxt_setup_int_mode() also skips reinitialization, because bp->irq_tbl
is non-NULL.

bnxt_request_irq() then requests an IRQ for every cp ring using
bp->irq_tbl[map_idx].vector, without checking map_idx against
bp->total_irqs. For entries at or above total_irqs, wouldn't .vector be
either 0 or a stale virq? The trim loop in bnxt_change_msix() frees the
vector but leaves irq_tbl[].vector set:

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

The open would then fail. If another device has reused the stale virq,
could the bnxt handler end up attached to an interrupt it doesn't own?

Running ethtool -L again with the same count seems to repeat the grow
and trim in bnxt_check_rings() and the early return in
bnxt_reserve_rings(). Is there a way to recover other than picking a
different ring configuration or a FW reset?

[Severity: High]

This is a pre-existing issue, not one introduced by this patch.
However, the new rollback adds another access to bp->irq_tbl in a
window that doesn't appear to be protected against PCI error recovery.

The DCB IEEE ETS path reaches this code holding only RTNL:

dcbnl_ieee_set()
  bnxt_dcbnl_ieee_setets()
    bnxt_setup_mq_tc()
      bnxt_check_rings()
        bnxt_change_msix()

net/dcb/dcbnl.c doesn't take the netdev instance lock.
bnxt_io_error_detected(), on the other hand, takes netdev_lock() but
not RTNL. For pci_channel_io_frozen it calls:

bnxt_io_error_detected()
  bnxt_fw_fatal_close()
    bnxt_clear_int_mode()

This frees bp->irq_tbl and sets it to NULL, but leaves bp->total_irqs
unchanged.

Suppose that runs between the grow in bnxt_check_rings() and the new
bnxt_change_msix(bp, old_total). The trim loop would see the stale
total_irqs, which is above old_total, and read
bp->irq_tbl[i - 1].vector through a NULL or freed pointer. The existing
write in the grow loop, bp->irq_tbl[i].vector = map.virq, has the same
race.

Should bnxt_dcbnl_ieee_setets() take the netdev instance lock? Taking
it inside bnxt_check_rings() would deadlock callers that already hold
it.

>  		}
>  	}
>  	return rc;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023121321.2127263-1-vlad.wing%40gmail.com

Re: [PATCH net] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[Vlad Poenaru]

On Tue, Oct 27, 2026 at 12:39:01PM +0000, netdev-bot+sashiko@kernel.org wrote:
> - [Medium] The fix is incomplete when FW grants fewer MSI-X vectors than
>   requested.

Agreed. bnxt_reserve_rings() grows to the irqs_required it computed
before the reservation, so a partial grant would still activate
entries that FW has not reserved. In v2 the allocation is sized from
bnxt_get_num_msix() after __bnxt_reserve_rings(), i.e. from the rings
actually kept. With no RoCE driver registered, that fits within what
FW granted, and it is never more than the old pre-reservation count.
With RoCE registered, __bnxt_reserve_rings() can still keep more L2
rings than a partial grant covers, because the ULP share isn't taken
out of resv_irqs. That accounting predates this patch and belongs with
the ring accounting work on net-next, so I've left it out of this fix.

> - [Medium] New error path leaves the state inconsistent.

Agreed. This path existed before for the bnxt_reserve_rings() grow,
but the patch makes it the main one. In v2, bnxt_reserve_rings() also
grows the vectors when no reservation is needed but bp->total_irqs is
below what the reserved rings need, so the next open recovers instead
of returning early. bnxt_change_msix() also clears irq_tbl[].vector
when it trims, so a stale virq can't be requested again.

> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the DCB IEEE
>   ETS path reaches bnxt_check_rings() and bnxt_change_msix() holding...

This race between the DCB path (RTNL only) and PCI error recovery
(netdev lock only) is real as far as I can tell. It predates this
patch, and the grow loop already has it. I'll leave the locking in
bnxt_dcbnl_ieee_setets() to Michael and Pavan rather than mix it into
this regression fix.

v2 follows.

pw-bot: cr